Add ClaimTypes constants #216
Add YahooClaimTypes
- Loading branch information
commit c4b7113a39bc7b5fc62218a140eed8e0139e167d
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| /* | ||
| * Licensed under the Apache License, Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0) | ||
| * See https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers | ||
| * for more information concerning the license and the contributors participating to this project. | ||
| */ | ||
|
|
||
| namespace AspNet.Security.OAuth.Yahoo | ||
| { | ||
| /// <summary> | ||
| /// Contains claim types specific to the <see cref="YahooAuthenticationHandler"/>. | ||
| /// </summary> | ||
| public static class YahooClaimTypes | ||
| { | ||
| public const string FamilyName = "urn:yahoo:familyname"; | ||
|
|
||
| public const string GivenName = "urn:yahoo:givenname"; | ||
|
|
||
| public const string ProfileUrl = "urn:yahoo:profile"; | ||
|
|
||
| public const string ImageUrl = "urn:yahoo:profileimage"; | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with the built-in
ClaimTypeswhen we have a good constant for these claims. Consistency always wins, I guess.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking changes are kinda acceptable as we're still in RC. But yeah, separate PR if you prefer 👍