Skip to content
This repository has been archived by the owner. It is now read-only.

Improve mechanism for flowing additional claims to identity cookie on SignIn #628

Closed
TerribleDev opened this issue Nov 8, 2015 · 30 comments

Comments

@TerribleDev
Copy link

commented Nov 8, 2015

In ExternalLoginCallback there are claims set by middlewear components that are being dropped in ExternalLoginSignInAsync.

Specifically, I am using the github external provider which sets a bunch of claims, and I see the claims during ExternalLoginCallback, but they are not carried through. What I want to get at is the auth token which is set as a claim when configured.

@HaoK

This comment has been minimized.

Copy link
Member

commented Nov 13, 2015

@rustd can you point him at the samples of how this is typically handled, this is always how things have worked

@HaoK HaoK added the Samples label Nov 13, 2015

@HaoK HaoK added this to the Discussions milestone Nov 13, 2015

@HaoK

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

See #916 which is basically the same issue

@HaoK HaoK closed this Jul 25, 2016

@PinpointTownes

This comment has been minimized.

Copy link

commented Jul 26, 2016

@HaoK this issue is actually different and only about flowing claims from the external authentication cookie to the application/internal one. Unlike the other issue, claims are there. They are just ignored when converting the external cookie.

@HaoK

This comment has been minimized.

Copy link
Member

commented Jul 26, 2016

So the general class of issue I assumed both of these to be was: there are claims in the external cookie, how do I get them to flow to the identity cookie?

If that is accurate, the answer is that the app needs to manually add them at some point (either by adding them to the user's claims, or adding them to the ClaimsPrincipal before SignIn is called). The default template flow only uses the name identifier from the external cookie, and potentially the email. Anything else extra in the external identity is ignored...

@PinpointTownes

This comment has been minimized.

Copy link

commented Jul 26, 2016

Anything else extra in the external identity is ignored...

I guess that was the whole point of this topic. Wouldn't automatically flowing a few (listed?) claim types make sense?

@HaoK

This comment has been minimized.

Copy link
Member

commented Jul 26, 2016

We can repurpose this as a feature ask sure, what new behavior would you like to see?

The initial external login linking is the easiest, we can certainly flow (all/some) claims from the external as part of the login creation.

On subsequent logins should the claims be refreshed/updated each time?

But never the less, at the end of the day, this might still end up being template/app controller work, as I think all of the identity features needed to do this are already there.

@HaoK HaoK reopened this Jul 26, 2016

@HaoK HaoK changed the title External Authentication claims being stripped out Improve mechanism for flowing external cookie claims to identity cookie Jul 26, 2016

@HaoK HaoK unassigned rustd Jul 26, 2016

@HaoK

This comment has been minimized.

Copy link
Member

commented Jul 26, 2016

I guess we could add overloads of signInmanager.ExternalLoginSignInAsync and _userManager.AddLoginAsync(user, info) with some kind of importExternalClaims: true

@HaoK

This comment has been minimized.

Copy link
Member

commented Jul 26, 2016

@blowdart thoughts? This is going to be a common ask, (it already was a common ask for v2)

@HaoK

This comment has been minimized.

Copy link
Member

commented Jul 26, 2016

This sort of falls into the general bucket of claims transformation scenarios which still need some love...

@HaoK HaoK added enhancement and removed Samples labels Jul 27, 2016

@HaoK HaoK modified the milestones: 1.0.1, Discussions Jul 27, 2016

@blowdart

This comment has been minimized.

Copy link
Member

commented Jul 27, 2016

So what happens if a user has flowed the claims initially, and saved them, do we then stomp them? Because let's face it, folks don't look at issuer.

@PinpointTownes

@PinpointTownes

This comment has been minimized.

Copy link

commented Jul 27, 2016

@blowdart I like @HaoK's idea (i.e adding a new ExternalLoginSignInAsync overload that allows preserving the external claims).

If you think there's an external/internal claims mixup risk, I guess one option is to prevent duplicate claims by ensuring they don't exist in the set returned by CreateUserPrincipalAsync before copying them from the external identity. Another one might be to add a "white-list" parameter to force the developer to specify the claims he/she wants to preserve.

@blowdart

This comment has been minimized.

Copy link
Member

commented Jul 27, 2016

I like the white list approach, and we should leverage the issuer field just to make sure.

@HaoK what's the issuer when claims from Identity?

@HaoK

This comment has been minimized.

Copy link
Member

commented Jul 27, 2016

We don't currently set the issuer in identity, as part of this change, we could introduce an Issuer option in ClaimsIdentityOptions and use that in our default implementation of UserClaimsPrincipalFactory.

There's also the question of whether we want the claims from the external identity permanently saved for the user (via AddClaim), or if we just want to add them for a particular single sign in My inclination is for the latter, and to only merge the claims from the external identity to the identity user generated principal for calls to signInmanager.ExternalLoginSignInAsync and to leave AddLogin unchanged.

@HaoK HaoK added this to the 1.0.1 milestone Jul 27, 2016

@PinpointTownes

This comment has been minimized.

Copy link

commented Jul 27, 2016

My inclination is for the latter, and to only merge the claims from the external identity to the identity user generated principal for calls to signInmanager.ExternalLoginSignInAsync and to leave AddLogin unchanged.

Sounds like the best option, IMHO. And it would make things like aspnet/Security#852 much easier to use with Identity.

@brockallen

This comment has been minimized.

Copy link

commented Jul 28, 2016

This should all be application logic, not something the framework automatically does. The more magic that the framework automatically does, the more work someone has to do to prevent it.

@leastprivilege

This comment has been minimized.

Copy link

commented Jul 28, 2016

Totally agreed

@TerribleDev

This comment has been minimized.

Copy link
Author

commented Aug 5, 2016

There could be a setting to turn on "the magic"

@HaoK

This comment has been minimized.

Copy link
Member

commented Aug 5, 2016

Yes, the idea is that this would be a new overload of ExternalSignIn(user, importExternalClaims: true) that would only then suck over any claims on the external identity

@HaoK

This comment has been minimized.

Copy link
Member

commented Aug 5, 2016

Or if we are doing whitelist I guess: ExternalSignIn(user, importExternalClaims: [ "SomeClaim1", "SomeClaim2" ])

@divega divega modified the milestones: 1.1.0, 1.1.0-preview1 Oct 12, 2016

@HaoK HaoK removed this from the 1.1.0 milestone Oct 26, 2016

@HaoK

This comment has been minimized.

Copy link
Member

commented Oct 26, 2016

Clearing to revisit in triage to see if we want to do something in 1.1

@PinpointTownes

This comment has been minimized.

Copy link

commented Oct 26, 2016

@HaoK independently on what you decide concerning a "sugar" method, that would be nice to update the template and/or the documentation to explain how custom claims can be flowed from external cookies to the application cookie. It's really a recurrent demand (e.g aspnet-contrib/AspNet.Security.OpenId.Providers#26)

@HaoK HaoK changed the title Improve mechanism for flowing external cookie claims to identity cookie Improve mechanism for flowing additional claims to identity cookie on SignIn Jan 18, 2017

@blowdart blowdart added this to the 2.1.0 milestone Sep 14, 2017

@blowdart blowdart modified the milestones: 2.1.0, 2.2.0 Sep 14, 2017

@ozonni

This comment has been minimized.

Copy link

commented Nov 21, 2017

is there some updates on this issue? We also face the same issue with including additional claims during the signup.

@HaoK

This comment has been minimized.

Copy link
Member

commented Dec 12, 2017

I'll prob add a sample "soonish" via aspnet/AuthSamples#6, I don't think we'll be adding any built in feature support for this in 2.1 at this point.

@HaoK

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

The sample demonstrating this flow is https://github.com/aspnet/AuthSamples/tree/dev/samples/Identity.ExternalClaims

We can revisit if we need additional framework support for this in the future

@HaoK HaoK closed this Mar 5, 2018

@PinpointTownes

This comment has been minimized.

Copy link

commented Mar 5, 2018

The sample doesn't demonstrate how to flow claims from the external cookie to the application one: it just stores them in the database so they are later restored when building the application cookie identity.

There are cases where storing claims in the database is not appropriate (e.g because you correlate a remote session with the local session). And in this case, it's a huge PITA to implement, as you basically have to copy all the SignInAsync method from SignInManager.

@PinpointTownes

This comment has been minimized.

Copy link

commented Mar 5, 2018

TL;DR: please re-open.

@HaoK

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

In the case you describe where you are trying to override SignInAsync, can your app just use a custom SignInManager to have it do what it needs?

@PinpointTownes

This comment has been minimized.

Copy link

commented Mar 5, 2018

can your app just use a custom SignInManager to have it do what it needs?

That's what I meant by this:

And in this case, it's a huge PITA to implement, as you basically have to copy all the SignInAsync method from SignInManager.

https://github.com/aspnet/Identity/blob/dev/src/Identity/SignInManager.cs#L186-L197

Implementing such a method is absolutely not trivial for beginners as the external identity is not available from this method (you have to retrieve it manually using AuthenticateAsync([external cookie scheme]). Since it's a frequent demand, I don't see why we don't have a built-in hook to handle this scenario way more easily.

@HaoK

This comment has been minimized.

Copy link
Member

commented Mar 15, 2018

Yeah I filed https://github.com/aspnet/Identity/issues/1685 to be a larger issue trying to address the whole thing, as opposed to just addressing this at purely a sign in manager level

@chrisckc

This comment has been minimized.

Copy link

commented May 23, 2018

@HaoK @PinpointTownes , just found this issue, also wanted to pass the claims through to the application cookie but had to use _userManager.AddClaimAsync() to add them to the database first as in the sample supplied by @HaoK.

Some i would like to be in the database, others not so there needs to be a way to do this.

I have just found a way to add claims by overriding the UserClaimsPrincipalFactory.CreateAsync() method, but there is no access to the external claims, just the user object so that additional claims can be created from the User's properties. Claims added there are not added to the database and are not visible in _signInManager.GetExternalLoginInfoAsync().Principal.Claims, which was fine for what i wanted to try out.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
10 participants
You can’t perform that action at this time.