Add user and profile claims #579

gao-sun · 2024-02-01T03:53:17Z

Description of Change

Models

Extend User and Profile class by adding claims defined in oidc-client-ts:

User is mapped to User
Profile is mapped to IdTokenClaims

Add AdditionalData property in Profile to allow unknown properties in the ID token, as identity providers may add new claims from time to time.

Extenstions

Support AsClaims for JsonElement.

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 for User, 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

I have included examples or tests
I have updated the change log (didn't find)
I am listed in the CONTRIBUTORS file (didn't find)
Changes adhere to coding standard
I checked the licenses of Third Party software and discussed new dependencies with at least 1 other team member

gao-sun · 2024-02-01T04:14:36Z

@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.

GeertvanHorrik · 2024-02-01T08:16:57Z

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 :)

GeertvanHorrik

My compliments, really great work, thank you!

Please see the comments, hopefully they all make sense. If not, feel free to comment back 👍

src/Blorc.OpenIdConnect.Tests/Extensions/ObjectExtensionsFacts.cs

GeertvanHorrik · 2024-02-01T08:21:33Z

src/Blorc.OpenIdConnect.Tests/Extensions/ObjectExtensionsFacts.cs

@@ -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));


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.

GeertvanHorrik · 2024-02-01T08:22:37Z

src/Blorc.OpenIdConnect/Models/Profile.cs

+        {
+            get;
+            set;
+        } = JsonDocument.Parse("[]").RootElement;


Do we need to recreate / parse every time or can we use a static readonly cache object as empty array?

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.

GeertvanHorrik · 2024-02-01T08:23:55Z

src/Blorc.OpenIdConnect/Models/Profile.cs

+
+                    foreach (var item in enumerator)
+                    {
+                        result.Add(item.GetString()!);


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); }

GeertvanHorrik · 2024-02-01T08:24:26Z

src/Blorc.OpenIdConnect/Models/Profile.cs

+                } 
+                else if (Aud.ValueKind == JsonValueKind.String)
+                {
+                    return [ Aud.GetString()! ];


same here, do we want to support empty strings or return an empty array in case of empty string?

GeertvanHorrik · 2024-02-01T08:29:01Z

src/Blorc.OpenIdConnect/Extensions/JsonElementExtensions.cs

+                break;
+
+            case JsonValueKind.String:
+                yield return new Claim(claimType, element.GetString()!);


Claim value can never be null. But.... do we want to ignore the yield return in case element.GetString() returns null?

@GeertvanHorrik Do you?

Yes, if the claim returns null, it should not be part of the collection. @alexfdezsauco , you agree?

GeertvanHorrik · 2024-02-01T08:29:47Z

src/Blorc.OpenIdConnect/Extensions/JsonElementExtensions.cs

+
+            case JsonValueKind.True:
+            case JsonValueKind.False:
+                yield return new Claim(claimType, element.GetBoolean().ToString());


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?

GeertvanHorrik · 2024-02-01T08:30:45Z

src/Blorc.OpenIdConnect.Tests/Models/UserFacts.cs

+            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"));
+        }


await Verifier.Verify(user) would be a very nice addition (easier than writing all the asserts). They can live side-by-side.

@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?

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 :-)

src/Blorc.OpenIdConnect.Tests/Models/UserFacts.cs

gao-sun · 2024-02-01T08:56:15Z

My compliments, really great work, thank you!

Please see the comments, hopefully they all make sense. If not, feel free to comment back 👍

I went through the comments, they are awesome! I'll update per your comments soon. Thank you for your time!

kjbtech · 2024-04-29T07:00:29Z

Can we help on that pull request to close it?

GeertvanHorrik · 2024-04-29T22:51:01Z

@alexfdezsauco can we close this one or does it need a lot of work?

kjbtech · 2024-05-16T09:54:04Z

Any news on this one?

kjbtech · 2024-07-19T08:21:49Z

Hello,
Any update on this?

kjbtech · 2024-07-31T12:03:16Z

@GeertvanHorrik @alexfdezsauco

Can I help to finish this pull request? What is the matter?

alexfdezsauco · 2024-07-31T14:04:23Z

We requested some changes to @gao-sun

kjbtech · 2024-08-01T16:08:34Z

@alexfdezsauco I'm gonna try to take over his work.

GeertvanHorrik · 2024-08-05T14:13:36Z

Closing this PR in favor of #636 . Thanks @gao-sun for the initial changes, the new PR will still contain your changes but we will try to finalize this PR with the help of @kjbtech .

gao-sun added 4 commits February 1, 2024 01:28

Add user and profile claims

5b74ed2

Update tests and demo app

b82117b

Code cleanup

74bd2c1

Fix Aud default value

092d75e

gao-sun marked this pull request as ready for review February 1, 2024 04:12

GeertvanHorrik requested changes Feb 1, 2024

View reviewed changes

Fix format issues

487e2a5

kjbtech mentioned this pull request Jul 31, 2024

LoadUserInfo with Resource seems to not being handled #611

Open

3 tasks

kjbtech mentioned this pull request Aug 2, 2024

Add user and profile claims #636

Draft

GeertvanHorrik closed this Aug 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add user and profile claims #579

Add user and profile claims #579

gao-sun commented Feb 1, 2024 •

edited

Loading

gao-sun commented Feb 1, 2024

GeertvanHorrik commented Feb 1, 2024

GeertvanHorrik left a comment

GeertvanHorrik Feb 1, 2024

GeertvanHorrik Feb 1, 2024

gao-sun Feb 1, 2024

GeertvanHorrik Feb 1, 2024

GeertvanHorrik Feb 1, 2024

GeertvanHorrik Feb 1, 2024

kjbtech Aug 2, 2024

GeertvanHorrik Aug 5, 2024

GeertvanHorrik Feb 1, 2024

GeertvanHorrik Feb 1, 2024

kjbtech Aug 2, 2024

GeertvanHorrik Aug 5, 2024

gao-sun commented Feb 1, 2024

kjbtech commented Apr 29, 2024

GeertvanHorrik commented Apr 29, 2024

kjbtech commented May 16, 2024

kjbtech commented Jul 19, 2024

kjbtech commented Jul 31, 2024

alexfdezsauco commented Jul 31, 2024

kjbtech commented Aug 1, 2024

GeertvanHorrik commented Aug 5, 2024

Add user and profile claims #579

Add user and profile claims #579

Conversation

gao-sun commented Feb 1, 2024 • edited Loading

Description of Change

Issues Resolved

API Changes

Behavioral Changes

Testing Procedure

PR Checklist

gao-sun commented Feb 1, 2024

GeertvanHorrik commented Feb 1, 2024

GeertvanHorrik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gao-sun commented Feb 1, 2024

kjbtech commented Apr 29, 2024

GeertvanHorrik commented Apr 29, 2024

kjbtech commented May 16, 2024

kjbtech commented Jul 19, 2024

kjbtech commented Jul 31, 2024

alexfdezsauco commented Jul 31, 2024

kjbtech commented Aug 1, 2024

GeertvanHorrik commented Aug 5, 2024

gao-sun commented Feb 1, 2024 •

edited

Loading