-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add user and profile claims #579
Conversation
@GeertvanHorrik @alexfdezsauco Apologize for the large pull request. Please let me know if it aligns with the direction of this project. Thank you for your time in advance. |
Thanks, we'll review ASAP. I noticed you did a non-breaking change, but.... since this is a large update (improvement) and we want to upgrade to the 3.0.0 client anyway, this might be a good time to introduce breaking changes. So if you feel it will improve the component, go ahead :) |
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.
My compliments, really great work, thank you!
Please see the comments, hopefully they all make sense. If not, feel free to comment back 👍
@@ -112,7 +176,7 @@ public void Collects_Claims_From_Complex_Type() | |||
|
|||
var claims = complexType.AsClaims().ToList(); | |||
|
|||
Assert.That(claims.Count, Is.EqualTo(18)); | |||
Assert.That(claims.Count, Is.EqualTo(30)); |
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 want to introduce something else here. We have having really good experiences with VerifyTests. This will give you a visual representation of the expected and actual results, making it easier to compare and approve.
Should be a "one-liner", something like:
await Verifier.Verify(claims);
If you don't want to investigate this, we can do it ourselves later.
{ | ||
get; | ||
set; | ||
} = JsonDocument.Parse("[]").RootElement; |
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.
Do we need to recreate / parse every time or can we use a static readonly cache object as empty array?
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 think it'll be better to create every time for reference isolation. However, if we can use "required" for breaking changes, it should be no longer needed. Let me have a try.
|
||
foreach (var item in enumerator) | ||
{ | ||
result.Add(item.GetString()!); |
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.
It might not be possible to have a "null" value in the array, but even then it could be wise to add a null check:
var itemString = item.GetString();
if (itemString is not null)
{
result.Add(itemString);
}
} | ||
else if (Aud.ValueKind == JsonValueKind.String) | ||
{ | ||
return [ Aud.GetString()! ]; |
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.
same here, do we want to support empty strings or return an empty array in case of empty string?
break; | ||
|
||
case JsonValueKind.String: | ||
yield return new Claim(claimType, element.GetString()!); |
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.
Claim value can never be null. But.... do we want to ignore the yield return in case element.GetString() returns null?
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.
@GeertvanHorrik Do you?
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.
Yes, if the claim returns null, it should not be part of the collection. @alexfdezsauco , you agree?
|
||
case JsonValueKind.True: | ||
case JsonValueKind.False: | ||
yield return new Claim(claimType, element.GetBoolean().ToString()); |
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 think we can return a cached "true" or "false" instead of doing ToString() every time? In such case, we also don't have to call GetBoolean() since we know the type based on the value kind?
Assert.That(user.Scopes, Is.DeepEqualTo(new[] { "openid", "profile", "email", "roles" })); | ||
Assert.That(user.SessionState, Is.EqualTo("session_state_value")); | ||
Assert.That(user.TokenType, Is.EqualTo("Bearer")); | ||
} |
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.
await Verifier.Verify(user) would be a very nice addition (easier than writing all the asserts). They can live side-by-side.
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.
@GeertvanHorrik
You mentionned above that this kind of refactor could be done later, I quote:
If you don't want to investigate this, we can do it ourselves later.
Can we apply same rule here?
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.
Yes, we can delay, but Verifier.Verify
is super handy for your own toolset / knowledge as well. So if you want to skip, that's fine. But if you want to experiment with it, this is a great chance, but I'll leave it up to you :-)
I went through the comments, they are awesome! I'll update per your comments soon. Thank you for your time! |
Can we help on that pull request to close it? |
@alexfdezsauco can we close this one or does it need a lot of work? |
Any news on this one? |
Hello, |
@GeertvanHorrik @alexfdezsauco Can I help to finish this pull request? What is the matter? |
We requested some changes to @gao-sun |
@alexfdezsauco I'm gonna try to take over his work. |
Description of Change
Models
User
andProfile
class by adding claims defined inoidc-client-ts
:User
is mapped toUser
Profile
is mapped toIdTokenClaims
AdditionalData
property inProfile
to allow unknown properties in the ID token, as identity providers may add new claims from time to time.Extenstions
Support
AsClaims
forJsonElement
.Issues Resolved
It providers a better dev experience for integrating various identity providers. E.g. some developers may need to read the
sub
claim to handle user ID.API Changes
None. I considered aligning nullable definitions, for example, making
AccessToken
required forUser
, but it appears to be a breaking change. So I've decided to leave them as they are.Behavioral Changes
None
Testing Procedure
Updated and added unit tests for JSON and claim deserialization.
Tested locally with Logto, the sign-in flow works as expected.
PR Checklist