Replacing current "user" objects with ClaimsPrincipal/ClaimsIdentity #1820

damianh · 2015-02-19T19:59:15Z

PR to fix #1800

Mandatory:

Remove IUserIdentity
Replace UserIdentityExtensions with a version that has the same (as much as possible - will need to be updated for the new, correct, structure of a claim) API but uses ClaimsPrincipal
Update SecurityHooks to use the new extensions and claim structure
Update ModuleSecurity to use the new extensions and claim structure
Update NancyContext to use ClaimsIdentiy/Principal
Allow the principal to "flow" from hosting
Update OWIN to automatically flow the identity
Update tests

Preferred:

Update Forms Auth to use the new extensions and claim structure
Update Stateless Auth to use the new extensions and claim structure
~~Update Token Auth to use the new extensions and claim structure~~ Remove Token Auth 🔥
Update Basic Auth to use the new extensions and claim structure

Additional

Don't use bootstrapper static location in NancyOptions. It WILL cause conflicts and composition problems in an OWIN pipeline.

…erIdentity extension methods to ClaimsPrincipal.

…ons.

…d related tests due to ClaimsPrincipal support.

…g predicate in RequriesClaims

damianh · 2015-02-19T20:11:14Z

@NancyFx/owners @NancyFx/most-valued-minions Right lads, bit of a big fish landing here. Going to need your careful attention. I'd like to get feedback before going any further.

At this point everything other that Nancy.Authentication.Token and Nancy.Authentication.Token.Tests compiles. Tests pass in all of the remaining projects, though I did delete a couple of tests that may be contentions. I'll highlight those.

To review, best start by looking at changes to ClaimsPrincipalExtensions, ModuleSecurity and SecurityHook.

damianh · 2015-02-19T20:22:13Z

What should we do with Nancy.Authentication.Token ? There is already a standard for this, OAuth, that uses JWT

jchannon · 2015-02-19T20:23:29Z

https://github.com/jchannon/Owin.StatelessAuth

On Thursday, 19 February 2015, Damian Hickey [email protected]
wrote:

What should we do with Nancy.Authentication.Token ? There is already a
standard for this, OAuth, that uses JWT

Reply to this email directly or view it on GitHub
#1820 (comment).

damianh · 2015-02-19T20:24:40Z

src/Nancy.Tests/Unit/Security/ModuleSecurityFixture.cs

@@ -286,84 +207,15 @@ public void Should_return_null_with_RequiresAnyClaim_and_any_claim_met()
        }

        [Fact]
-        public void Should_return_forbidden_response_with_RequiresValidatedClaims_enabled_but_claims_missing()


Any tests around RequiresValidatedClaims were removed. HasClaim(s) performs that function.

thecodejunkie · 2015-02-26T22:01:47Z

src/Nancy.Tests.Functional/Modules/PerRouteAuthModule.cs

@@ -19,21 +17,14 @@ public PerRouteAuthModule()

            Get["/requiresclaims"] = _ =>
            {
-                this.RequiresClaims(new[] { "test", "test2" });
+                this.RequiresClaims(c => c.Type == "test", c => c.Type == "test2");


Perhaps we could have an overload of RequireClaims that takes a params string[] claim. The overload would iterate over the params and compare them against the Type of each claim. It would give us a nicer syntax. Thoughts?

It would have to be called 'RequiredClaimTypes'.

Regardless, its not particularly useful as you generally need to inspect
at least 2 attributes of a claim - the Type and the Value. See HasClaim
https://msdn.microsoft.com/en-us/library/hh194432%28v=vs.110%29.aspx and
the predicate overload
https://msdn.microsoft.com/en-us/library/hh159713(v=vs.110).aspx
On 26 Feb 2015 23:01, "Andreas Håkansson" [email protected] wrote:

In src/Nancy.Tests.Functional/Modules/PerRouteAuthModule.cs
#1820 (comment):

@@ -19,21 +17,14 @@ public PerRouteAuthModule()

Get["/requiresclaims"] = _ => {

this.RequiresClaims(new[] { "test", "test2" });

this.RequiresClaims(c => c.Type == "test", c => c.Type == "test2");

Perhaps we could have an overload of RequireClaims that takes a params
string[] claim. The overload would iterate over the params and compare
them against the Type of each claim. It would give us a nicer syntax.
Thoughts?

—
Reply to this email directly or view it on GitHub
https://github.com/NancyFx/Nancy/pull/1820/files#r25469286.

thecodejunkie · 2015-02-26T22:16:04Z

I think the changes so far looks alright 😄 👍

thecodejunkie · 2015-02-26T22:27:30Z

Why is Nancy.Authentication.Token an issue here? I get it that there are other, existing, things for this but could we leave it as is for now so we don't get stuck on that? I know it makes use of IUserIdentity, are you seeing issues with making it compatible with the new stuff?

grumpydev · 2015-02-27T07:53:46Z

Might be nice to loop in @leastprivilege and @brockallen - be nice to get some identityserver integration/samples/lovin' alongside this.

leastprivilege · 2015-02-27T08:06:10Z

We could add a Nancy sample API here:

https://github.com/IdentityServer/Thinktecture.IdentityServer3.Samples/tree/master/source/Clients

(analogous to https://github.com/IdentityServer/Thinktecture.IdentityServer3.Samples/tree/master/source/Clients/SampleAspNetWebApi)

leastprivilege · 2015-02-27T08:07:02Z

And with "we" I really mean "you" ;) I am currently busy with working on the PR on my family repo ;)

khellang · 2015-02-27T09:28:17Z

We should loop in @jeffdoolittle as well, he was talking about JWT etc. over at #1550.

damianh · 2015-03-08T15:54:41Z

We can't carry Nancy.Authentication.Token forward with just swapping ClaimsPrincipal for IUserIdentity

It currently does two things:

Serializing the ClaimsPrincipal in a similar way is going to a lot heavier in terms of token size. In anycase, it's also questionable if that is even a reasonable idea considering JWTs (which are not necessarily trivial - https://tools.ietf.org/html/draft-ietf-oauth-json-web-token and https://auth0.com/blog/2014/01/27/ten-things-you-should-know-about-tokens-and-cookies/ ). JWTs, because they're standard, means the authorization header is verifiable in other middleware / frameworks etc. Even if serialization of ClaimsPrincipal worked, do we really want a Nancy specific token auth scheme?

I suggest we delete Nancy.Authentication.Token and open a separate issue for JWT support (if we want that in nancy).

damianh · 2015-03-08T15:58:31Z

I've also looked into Owin.StatelessAuth ( @jchannon correct me if I'm wrong ), it appears it just checks for the existence of the header and offloads the token verification to the user and it doesn't assist in generating the tokens.

khellang · 2015-03-12T16:00:06Z

This will break A LOT of app.UseNancy() cases 😞

damianh · 2015-03-12T16:29:12Z

Yes it will. Allowing this is pit-of-fail. User does app.UserNancy() twice in a pipeline and stuff breaks. Remember, middleware can contain middleware, and it may come from other places, so it may not be obvious to them in their wireups.

Nancy must no longer assume it's the only framework in an application.

Fwiw, there is no such thing as app.UseWebApi(). It explicitly requires a configuration parameter for the same reasons too.

grumpydev · 2015-03-12T16:38:33Z

I disagree, we still need our happy path - the 99% use case is Nancy on its own, or potentially nancy with some auth middleware - anyone using complex middleware setups should already be aware of how to set it up.

damianh · 2015-03-12T16:39:03Z

I knew you would :)

grumpydev · 2015-03-12T16:39:27Z

And WebApi as an example on how to do things? You'll be suggesting sharepoint next :)

damianh · 2015-03-12T16:39:51Z

One data point a projection does not make :)

damianh · 2015-03-12T16:43:18Z

Will happy-path users still be coming via OWIN(\whatever) pipelines though? hmm

grumpydev · 2015-03-12T16:53:49Z

We could take a similar approach to the one I suggested for the razor config wrecking problem - have two "levels" of nuget, our normal ones with happy paths, then a .core that is just bare bones.

In razor's case .core would be everything but the config transforms, razor main package would have a dependency on that, and add in the transforms.

For the owin stuff perhaps it could be that the happy path stuff isn't in core, but is included as a dependency in Nancy.hosting.aspnet (just thinking out loud to give an idea)

damianh · 2015-03-12T17:03:02Z

Could Nancy.Hosting.AspNet could do the bootstrapper static location?

grumpydev · 2015-03-12T17:17:38Z

That's what it does currently, but not sure how it will work if we shift it to owin.. Maybe the "hosting" packages that use katana will bring in a startup.cs

grumpydev · 2015-03-13T08:35:09Z

Regardless of the outcome of the wireup discussion, I don't think it has any relevance to the scope of this issue/PR, so if you could roll it back that'd be awesome.

damianh · 2015-03-13T08:58:17Z

Yah fair enough. What about that failing test because it's finding multiple bootstrappers in the test assembly? Test for the exception?

grumpydev · 2015-03-18T09:31:27Z

Sorry @damianh I missed your question.. yeah, those tests are a bit broken I think.. something like this makes more sense:

namespace Nancy.Owin.Tests
{
    using Nancy.Bootstrapper;
    using Nancy.Tests;

    using Xunit;

    public class NancyOptionsFixture
    {
        private readonly NancyOptions nancyOptions;

        public NancyOptionsFixture()
        {
            this.nancyOptions = new NancyOptions();
        }

        [Fact]
        public void Bootstrapper_should_use_locator_if_not_specified()
        {
            // Given
            var bootstrapper = new DefaultNancyBootstrapper();
            NancyBootstrapperLocator.Bootstrapper = bootstrapper;

            //When
            //Then
            this.nancyOptions.Bootstrapper.ShouldNotBeNull();
            this.nancyOptions.Bootstrapper.ShouldBeSameAs(bootstrapper);
        }

        [Fact]
        public void Bootstrapper_should_use_chosen_bootstrapper_if_specified()
        {
            // Given
            var bootstrapper = new DefaultNancyBootstrapper();
            var specificBootstrapper = new DefaultNancyBootstrapper();
            NancyBootstrapperLocator.Bootstrapper = bootstrapper;

            //When
            this.nancyOptions.Bootstrapper = specificBootstrapper;

            //Then
            this.nancyOptions.Bootstrapper.ShouldNotBeNull();
            this.nancyOptions.Bootstrapper.ShouldBeSameAs(specificBootstrapper);
        }

        [Fact]
        public void PerformPassThrough_should_not_be_null()
        {
            this.nancyOptions.PerformPassThrough.ShouldNotBeNull();
        }

        [Fact]
        public void PerformPassThrough_delegate_should_return_false()
        {
            this.nancyOptions.PerformPassThrough(new NancyContext()).ShouldBeFalse();
        }
    }
}

… all sort of problems if there is more than one Nancy instance / bootstrapper in a owin pipeline. (reverted from commit e8ae7c4)

damianh · 2015-03-20T14:03:00Z

@grumpydev only problem with these tests is they can't be run in parallel (static mutable state being the enemy of parallel testing). May or not be a concern for @NancyFx/owners .

damianh · 2015-03-20T14:05:06Z

Test fixed. I guess a rebase is required. @NancyFx/owners Could one of you rebase the 2.0 branch (if deemed necessary)?

grumpydev · 2015-03-20T14:10:08Z

I thought about that @damianh, but I couldn't think of anywhere else that used that property - I suppose it could be refactored so NancyOptions has:

public Func BootstrapperLocator { get; set; }

ctor() {
this.BootstrapperLocator = () => NancyBootstrapperLocator.Bootstrapper;
}

Then that can be swapped out to whatever you like.. quite possibly overkill though :)

damianh · 2015-03-20T14:13:00Z

Yeah, think it's a bit of overkill right now. Lets move on and address it separately as you've suggested.

Replacing current "user" objects with ClaimsPrincipal/ClaimsIdentity

xt0rted · 2015-03-20T14:59:23Z

Is there a CI feed for the v2 branch so I can try this out?

grumpydev · 2015-03-20T15:00:07Z

@xt0rted : not yet, we will put a beta out after 1.2 in the next week.

khellang · 2015-03-20T15:55:59Z

@xt0rted You can grab the bits from here 😄

damianh added 16 commits February 19, 2015 17:54

Deleting IUserIdentity. This breaks all the things.

dc3a9ff

Rename UserIdentityExtensions to ClaimsPrincipalExtension. Mapped IUs…

8be6af0

…erIdentity extension methods to ClaimsPrincipal.

ModuleSecurity and SecurityHooks fixed to use ClaimsPrincipal extensi…

ca5447a

…ons.

Basic Auth IUserValidator returns a ClaimsPrincipal.

f7513e7

Froms Auth IUserMapper returns a ClaimsPrincipal

3f3639c

Authentication.Stateless user lookup func returns a ClaimsPrincipal

9cafef2

Fix, changes (and some deletions) to ModuleSecurity, SecurityHooks an…

942f4fa

…d related tests due to ClaimsPrincipal support.

Replace IUserIdentity with ClaimsPrincipal.

c99698b

Unused using

eaf2412

Nancy.Tests.Function fixed to use ClaimsPrincipal.

688bfdf

Removing RequiresValidatedClaims - doesn't make sense give we're usin…

9f628ff

…g predicate in RequriesClaims

Nancy.Demo.Authentication.Basic fixed to use ClaimsPrincipal

e75f8ee

Nancy.Demo.Authentication fixed to use ClaimsPrincipal

462f8c1

Fix Nancy.Damo.Authentication.Forms to support ClaimsPrincipal

54faddd

Nancy.Demo.Authentication.Stateless fixed to use ClaimsPrincipal.

f9cf3f5

Delete tests /requiresvalidatedclaims isn't a thing.

f4be219

damianh reviewed Feb 19, 2015
View reviewed changes

khellang mentioned this pull request Feb 20, 2015

Claims identity #1821

Closed

thecodejunkie reviewed Feb 26, 2015
View reviewed changes

khellang added Update Documentation Feature labels Mar 15, 2015

Do NOT use static service location in NancyMiddleware. The will cause…

0135cdd

… all sort of problems if there is more than one Nancy instance / bootstrapper in a owin pipeline. (reverted from commit e8ae7c4)

Fix test - explicitly set the NancyBootstrapperLocator.Bootstrapper

1ebb69f

grumpydev added a commit that referenced this pull request Mar 20, 2015

Merge pull request #1820 from damianh/v2-claims

65914a5

Replacing current "user" objects with ClaimsPrincipal/ClaimsIdentity

grumpydev merged commit 65914a5 into NancyFx:v2 Mar 20, 2015

damianh deleted the v2-claims branch March 20, 2015 14:17

damianh mentioned this pull request Apr 20, 2015

Claims support #1800

Closed

17 tasks

horsdal mentioned this pull request Jun 16, 2015

Feature Request: Enchance IUserIdentity #1954

Closed

khellang mentioned this pull request Nov 12, 2015

Nancy.Authentication.Token removed? #2109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replacing current "user" objects with ClaimsPrincipal/ClaimsIdentity #1820

Replacing current "user" objects with ClaimsPrincipal/ClaimsIdentity #1820

damianh commented Feb 19, 2015

damianh commented Feb 19, 2015

damianh commented Feb 19, 2015

jchannon commented Feb 19, 2015

damianh Feb 19, 2015

thecodejunkie Feb 26, 2015

damianh Feb 26, 2015

thecodejunkie commented Feb 26, 2015

thecodejunkie commented Feb 26, 2015

grumpydev commented Feb 27, 2015

leastprivilege commented Feb 27, 2015

leastprivilege commented Feb 27, 2015

khellang commented Feb 27, 2015

damianh commented Mar 8, 2015

damianh commented Mar 8, 2015

khellang commented Mar 12, 2015

damianh commented Mar 12, 2015

grumpydev commented Mar 12, 2015

damianh commented Mar 12, 2015

grumpydev commented Mar 12, 2015

damianh commented Mar 12, 2015

damianh commented Mar 12, 2015

grumpydev commented Mar 12, 2015

damianh commented Mar 12, 2015

grumpydev commented Mar 12, 2015

grumpydev commented Mar 13, 2015

damianh commented Mar 13, 2015 via email

grumpydev commented Mar 18, 2015

damianh commented Mar 20, 2015

damianh commented Mar 20, 2015

grumpydev commented Mar 20, 2015

damianh commented Mar 20, 2015

xt0rted commented Mar 20, 2015

grumpydev commented Mar 20, 2015

khellang commented Mar 20, 2015

Replacing current "user" objects with ClaimsPrincipal/ClaimsIdentity #1820

Replacing current "user" objects with ClaimsPrincipal/ClaimsIdentity #1820

Conversation

damianh commented Feb 19, 2015

Mandatory:

Preferred:

Additional

damianh commented Feb 19, 2015

damianh commented Feb 19, 2015

jchannon commented Feb 19, 2015

damianh Feb 19, 2015

Choose a reason for hiding this comment

thecodejunkie Feb 26, 2015

Choose a reason for hiding this comment

damianh Feb 26, 2015

Choose a reason for hiding this comment

thecodejunkie commented Feb 26, 2015

thecodejunkie commented Feb 26, 2015

grumpydev commented Feb 27, 2015

leastprivilege commented Feb 27, 2015

leastprivilege commented Feb 27, 2015

khellang commented Feb 27, 2015

damianh commented Mar 8, 2015

damianh commented Mar 8, 2015

khellang commented Mar 12, 2015

damianh commented Mar 12, 2015

grumpydev commented Mar 12, 2015

damianh commented Mar 12, 2015

grumpydev commented Mar 12, 2015

damianh commented Mar 12, 2015

damianh commented Mar 12, 2015

grumpydev commented Mar 12, 2015

damianh commented Mar 12, 2015

grumpydev commented Mar 12, 2015

grumpydev commented Mar 13, 2015

damianh commented Mar 13, 2015 via email

grumpydev commented Mar 18, 2015

damianh commented Mar 20, 2015

damianh commented Mar 20, 2015

grumpydev commented Mar 20, 2015

damianh commented Mar 20, 2015

xt0rted commented Mar 20, 2015

grumpydev commented Mar 20, 2015

khellang commented Mar 20, 2015