Add ClaimTypes constants #216
Conversation
| ClaimActions.MapJsonKey("urn:yahoo:profile", "profileUrl"); | ||
| ClaimActions.MapJsonKey("urn:yahoo:profileimage", "imageUrl"); | ||
| ClaimActions.MapJsonKey(YahooClaimTypes.FamilyName, "familyName"); | ||
| ClaimActions.MapJsonKey(YahooClaimTypes.GivenName, "givenName"); |
There was a problem hiding this comment.
Is there a reason a lot of these are using custom claim types instead of the built-in ones, like ClaimTypes.GivenName, ClaimTypes.Surname, etc.? Same in a lot of other places.
There was a problem hiding this comment.
Most likely an oversight (most of the aspnet-contrib providers have been written by different contributors, who may not be super familiar with all the built-in claim names).
FWIW, I try to avoid using the "legacy" WS-Fed claims as much as possible (because they're super long and make cookies/tokens bigger) but the MSFT OAuth2 providers use them when it's possible so we use them too for consistency.
There was a problem hiding this comment.
Right, so as long as there's no transformations for them anywhere, it doesn't really matter? Or, it might even be better to use custom claim types because of the smaller size?
There was a problem hiding this comment.
Let's go with the built-in ClaimTypes when we have a good constant for these claims. Consistency always wins, I guess.
There was a problem hiding this comment.
I didn't want to change any claim types in this PR, as it would be a breaking change. Should we open an issue for later?
There was a problem hiding this comment.
Breaking changes are kinda acceptable as we're still in RC. But yeah, separate PR if you prefer 👍
|
@khellang FYI, we use a little different pattern in the other aspnet-contrib projects. E.g: public static class GitHubAuthenticationConstants
{
public static class Claims
{
public const string Name = "name";
}
}Do you strongly prefer your pattern, or would adopting the one I suggest be acceptable for ya? |
It's your project. I'm just following orders 😉It'll have to be another day, cause it's bedtime here 😴 |
Let's go with the nested classes pattern, then (I like when it's consistent).
Sure, no hurry. Thanks for this PR! 👏 |
| { | ||
| public static class Claims | ||
| { | ||
| public const string Emailverified = "urn:autodesk:emailverified"; |
|
Merged, thanks! 4f46357 |
3 participants
I think this should cover them all 😄
Closes #97