OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.21k stars 2.34k forks source link

Map claims to CustomUserSettings #7671

Open jptissot opened 3 years ago

jptissot commented 3 years ago

From the discussion started here: https://github.com/OrchardCMS/OrchardCore/issues/7669#issuecomment-728981913

@MichaelPetrinolis reaching out for some advice.

We have a new feature for creating custom user properties from content types, but no way to map them to a user claim without implementing a custom claims factory, i.e. as per DemoUserClaimsPrincipalFactory.

I know you wrote a script option for the external login providers, to assist with claims etc.

What do you think about a hook in the DefaultUserClaimsPrincipalFactory and custom script to map claims to profile properties?

@jptissot has volunteered to write it :) if it's a good idea

https://github.com/OrchardCMS/OrchardCore/issues/7669#issuecomment-728985326 @deanmarcussen I am also thinking of the inverse. Mapping claims from profile properties. I guess this could be another script ? This would be very useful for tenants that implement the OpenID Server. To allow us to support exporting claims based on the CustomUserSettings.

MichaelPetrinolis commented 3 years ago

@jptissot you can extend the IExternalLoginEventHandler with an UpdateProfile method that returns a JSON object and has the same parameters as GenerateUserName. Then in UsersController, where we update the roles, we could also merge this JSON object to the UserProfile and save it. Just use the same logic regarding priority of the admin panel script and a registered handler through a module.

@deanmarcussen I like the idea to extend the DefaultUserClaimsPrincipalFactory and add a hook to retrieve a list of serializable claims to be added to the ClaimsIdentity. It should get the profile as parameter and return an IEnumerable that would add to ClaimsIdentity. We should then add the required claims like email and email_verified.

I think we should also make configurable the claims that are included in the IdentityToken based on user scope access. @kevinchalet ?

Skrypt commented 3 years ago

Question I've had is. Is it a common thing to add every User properties to Claims? Example, would an address or phone number would make sense to be kept inside a Claim? I would tend to think that these Claims are necessary for User functionalities but other than that I'm not sure this is how we should use them. Maybe it's a good way to retrieve User metadata quickly though but then is it appropriate?

MichaelPetrinolis commented 3 years ago

The application should add claims that are needed to validate/identify the user. There should be a UserProfileService to retrieve the extra data. Like OpenId Connect. There is the id_token that when requested has user claims, and there is the userinfo endpoint to obtain user information through the access_token.

Skrypt commented 3 years ago

So it makes a Query to some persistency storage "And/or a cached dictionnary" every time right? My concern is that these Claims are added when the user Logs in and if the settings are changed then the Claims needs also to be updated which seems less common to do. Else if that's how we should do it then we might need to refresh our custom email Claims when a user changes it. I remember seing some issues with User accounts requiring to relog in when updating some role permissions for example. Maybe the issue is there. We never refresh Claims other than when a user logs in.

kevinchalet commented 3 years ago

I think we should also make configurable the claims that are included in the IdentityToken based on user scope access. @kevinchalet ?

Great idea! While we're at it, we'll likely want a similar thing to determine what claims should go in the access tokens. Or maybe it should be a single script returning the list of destinations for each claim? (id_token, id_token AND access_token, access_token or nothing).

MikeAlhayek commented 3 years ago

PR #8097 should help here.

deanmarcussen commented 3 years ago

@MichaelPetrinolis @kevinchalet reaching out for advice again.

There's a pr in for extending the DefaultUserClaimsPrincipleFactory with providers. It needs a bit of work, so not reviewed yet.

However I just saw that @petedavis implemented the IUserClaimStore some time back https://github.com/OrchardCMS/OrchardCore/pull/4070

which adds a potentially more useful way of mapping these CustomUserSettings, and other user claims, than extending the DefaultUserClaimsPrincipleFactory

There's a list on User for any extra claims, which can just be added (and removed of course) when updating the user through a workflow, so it seems support is already there.

Conveniently has an index as well, so good for querying.

Question is could we leverage something similar regards OpenId and the access tokens?

And do we then want to have providers for the DefaultUserClaimsPrincipleFactory in addition to UserClaims

MichaelPetrinolis commented 3 years ago

@deanmarcussen let me write down my understanding and my thoughts for the issue

Concluding, the claims are stored in the IUserClaimStore, and we provide a function to filter them based on the destination. We could make the 'claims destination script' more user friendly by defining a list of claim types to include for each destination instead of a js script.

deanmarcussen commented 3 years ago

Thanks @MichaelPetrinolis good summary, all makes sense. Will see what I can come up with ;)

hyzx86 commented 1 year ago

I prefer to extend IExternalLoginEventHandler

Add the function of updating user information based on scripts

The mapping between the external user and OC user , including the CustomUserSettings, can be more easily realized through the script

hyzx86 commented 1 year ago

Can someone help me review this PR? https://github.com/OrchardCMS/OrchardCore/pull/12845

hyzx86 commented 1 year ago

Hi @jptissot , I made a module to solve this problem

https://github.com/EasyOC/EasyOC.Modules/tree/main/src/Modules/EasyOC.Users