Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should JsonWebTokenHandler take SecurityTokenDescriptor.Subject into account? #1193

Closed
kevinchalet opened this issue May 28, 2019 · 12 comments · Fixed by #1235
Closed

Should JsonWebTokenHandler take SecurityTokenDescriptor.Subject into account? #1193

kevinchalet opened this issue May 28, 2019 · 12 comments · Fixed by #1235
Assignees
Labels
Bug Product is not functioning as expected Customer reported Indicates issue was opened by customer
Milestone

Comments

@kevinchalet
Copy link
Contributor

For some reasons, it seems that the new JsonWebTokenHandler no longer honors the Claim claims stored in SecurityTokenDescriptor.Subject. Instead, you have to populate the Claims property (an IDictionary<string, object>).

Is it the intended behavior?

@mafurman
Copy link
Member

@PinpointTownes
Yes, that is the expected behavior. SecurityTokenDescriptor.Subject will be marked with an Obsolete attribute in our 6.x release.

@kevinchalet
Copy link
Contributor Author

Good to know. Maybe an if check in JsonWebTokenHandler to ensure Subject is not set would help make this "change" more obvious?

Now, the real question is the one I asked here: #1194. If ClaimsIdentity is no longer supported when creating tokens, why is it still so massively used for the validation part?

@brentschmaltz
Copy link
Member

@PinpointTownes @mafurman Do you seem any harm in using SecurityTokenDescriptor.Subject to add values in JsonWebTokenHandler.CreateToken(SecurityTokenDescriptor ...)? Other than the 'dup' issue and what 'set' takes precedent.
I would expect a user choose to set Subject or Claims, but I suppose both could be set. So we could honor the Subject property OR as you suggest inform the user better.

We originally had the API CreateToken(JObject, SigningCredentials, EncryptingCredentials), which seemed natural. Our position in the stack forced us to remove references to external libraries for the 3.0 release. We see value in staying 'break free' moving forward. The breaks from 4.x -> 5.x were hard to manage. So, we removed that API and added Claims to SecurityTokenDescriptor as the properties provide a way to set common values. An option is to add the API CreateToken(IDictionary<string, object>, SigningCredentials, EncryptingCredentials), but this forces one to add common claims into the dictionary.

Moving forward, we may benefit from letting the user decide if the 'ClaimsIdentity' should be created when validating, since some users won't need it.

@brentschmaltz brentschmaltz added Bug Product is not functioning as expected Customer reported Indicates issue was opened by customer labels May 29, 2019
@brentschmaltz brentschmaltz added this to the 6.x milestone Jun 4, 2019
@mafurman
Copy link
Member

@PinpointTownes What do you think about JsonWebTokenHandler taking the SecurityTokenDescriptor.Subject property into account, but throwing if both the Subject and Claims properties are set?

@brentschmaltz
Copy link
Member

@PinpointTownes any comment?

@kevinchalet
Copy link
Contributor Author

It's certainly an option, but I wonder if it wouldn't be worth supporting both (with the ClaimsPrincipal being processed first so that you can "override" the claims set via the untyped dictionary).

Is there a particular reason not to support that?

@brentschmaltz
Copy link
Member

@PinpointTownes I was thinking it would be more deterministic and easier to debug.
I suppose an explanation will be required in either case - support both in an order OR only one.

I'm open to either, with a bias to what users want.

@mafurman mafurman self-assigned this Jul 23, 2019
@kevinchalet
Copy link
Contributor Author

I'm open to either, with a bias to what users want.

Personally, I'd prefer supporting both, as I'll keep using ClaimsPrincipal for JWT creation. Being able to override certain claims via the untyped dictionary would be awesome.

@brentschmaltz
Copy link
Member

@PinpointTownes ok, so the ClaimsIdentity is first, any items in the dictionary will override. That would allow a base set of claims and to act as an instance.

@kevinchalet
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Product is not functioning as expected Customer reported Indicates issue was opened by customer
Projects
None yet
5 participants
@GeoK @brentschmaltz @kevinchalet @mafurman and others