Add user and profile claims

[kjbtech]

Description of Change

To complete https://github.com/WildGums/Blorc.OpenIdConnect/pull/579

[kjbtech]

@GeertvanHorrik @alexfdezsauco Regarding the current implementation .AsClaims() can't work because of reflection usage. We need to add two [ClaimType("aud")]:

        [ClaimType("aud")]
        [JsonPropertyName("aud")]
        public string? Aud
        {
        //...
        }

        [ClaimType("aud")]
        public string[] Audiences
        //....

But how EnumClaimsFromObjectProperties can choose? Take both values? Have you any suggestion?

At this time, I do not have enough experience on this project to have the big picture! So I probably miss something that could explain the reflection choice. But, it could worth a shot to think to get rid of reflection and use dedicate class/struct/record. That will be more readable and avoid primitive obsession.

What do you think?

[GeertvanHorrik]

I think these are reasonable comments. We will discuss this internally and I hope to have time to work on this on Monday.

[GeertvanHorrik]

@alexfdezsauco can you respond to the reflection question? I am happy to move to a dedicated class if that improves things.

[alexfdezsauco]

@GeertvanHorrik @alexfdezsauco Regarding the current implementation .AsClaims() can't work because of reflection usage. We need to add two [ClaimType("aud")]:

        [ClaimType("aud")]
        [JsonPropertyName("aud")]
        public string? Aud
        {
        //...
        }

        [ClaimType("aud")]
        public string[] Audiences
        //....

But how EnumClaimsFromObjectProperties can choose? Take both values? Have you any suggestion?

At this time, I do not have enough experience on this project to have the big picture! So I probably miss something that could explain the reflection choice. But, it could worth a shot to think to get rid of reflection and use dedicate class/struct/record. That will be more readable and avoid primitive obsession.

What do you think?

When we wrote this, we set the minimal claims in the Profile class that we needed and chose reflection as the extensibility approach. If someone needs more claims, they just need to inherit from Profile or User and set up the serialization and claim attributes so the algorithm can select them. Having a fixed structure doesn’t allow for extension and the inclusion of more claims on demand. So, reflection is a good solution for us in this case.

It looks like the proposal for audience resolution has a ‘custom’ implementation because it could be an array or a single element. So, probably the only thing needed here is to remove the [ClaimType("aud")] in the first property, to use the second as a claim and not the first.

[kjbtech]

@GeertvanHorrik @alexfdezsauco Regarding the current implementation .AsClaims() can't work because of reflection usage. We need to add two [ClaimType("aud")]:

        [ClaimType("aud")]
        [JsonPropertyName("aud")]
        public string? Aud
        {
        //...
        }

        [ClaimType("aud")]
        public string[] Audiences
        //....

But how EnumClaimsFromObjectProperties can choose? Take both values? Have you any suggestion? At this time, I do not have enough experience on this project to have the big picture! So I probably miss something that could explain the reflection choice. But, it could worth a shot to think to get rid of reflection and use dedicate class/struct/record. That will be more readable and avoid primitive obsession. What do you think?

When we wrote this, we set the minimal claims in the Profile class that we needed and chose reflection as the extensibility approach. If someone needs more claims, they just need to inherit from Profile or User and set up the serialization and claim attributes so the algorithm can select them. Having a fixed structure doesn’t allow for extension and the inclusion of more claims on demand. So, reflection is a good solution for us in this case.

It looks like the proposal for audience resolution has a ‘custom’ implementation because it could be an array or a single element. So, probably the only thing needed here is to remove the [ClaimType("aud")] in the first property, to use the second as a claim and not the first.

I'm not sure I understand why if you have a fixed structure, it doesn't allow extension? Custom implementation could inherit from a custom default serializer for example (instead of using reflection for AsClaims()). Attributes can be replaced by struct/record or class type. It's only more verbose.

Let's go back to the original issue here, regarding the official RFC for audience claim, aud is

• optional
• will mostly be an array of string
• but can also be a string

Naïvely, I will choose array of string implementation, it's easier to add in the actual implementation and it is understandable. Having code that handle both will be hard to maintain and add more complexity. It is abreaking change but it seems that you plan to move to a v3.

Thereby:

        // we remove this implementation
        public string? Aud
        {
        //...
        }

        // we add this one
        [ClaimType("aud")]
        [JsonPropertyName("aud")]
        public string[] Audiences
        //....

[kjbtech]

Hi @GeertvanHorrik @alexfdezsauco, Any news?

[GeertvanHorrik]

I have a deadline until the end of August, so need to prioritize that first.

I think @alexfdezsauco meant that it's more flexibel with the attributes than with models? Maybe he can elaborate a bit more so we have a working example?