Provide SCIM2 client capabilities behing an experimental Feature Profile#34733
Provide SCIM2 client capabilities behing an experimental Feature Profile#34733alexsedlex wants to merge 1 commit into
Conversation
|
I'm still struggling on an Hibernate issue : the modules provides a new hibernate entity (ScimResourceMapping) through the JpaEntityProviderFactory mechanism. It works perfectly if the SCIM extension is declared on its own (see original implementation ), but not if the module is included in the main codebase (in that case I get an IllegalArgumentException: org.hibernate.query.sqm.UnknownEntityException: Could not resolve root entity 'ScimResourceMapping' at org.hibernate.internal.ExceptionConverterImpl.convert(ExceptionConverterImpl.java:143)). |
|
I just gave it a quick spin, and checked out your branch, rebased it on current master and could start the server with the scim feature enabled. I could also create a scim federation. When do you see the IllegalArgumentException? Do you have a scim mock server to test this with? |
|
@thomasdarimont thanks a lot for your time ! The issue occurs when you :
To test, I use a Nextcloud instance using this [usefull docker-compose https://github.com/juliushaertl/nextcloud-docker-dev] and installed the SCIM Service provider extension |
|
@alexsedlex That is great stuff, thanks for your time on it. I did not check it yet but I'll. Recently, we started to review #33286. The idea was to start with a very basic implementation and, in this case, very specific to integrating with ipa-tuura. There is a bit of overlap here but your proposal seems more standard-compliant and generic enough to integrate with any SCIM server. In terms of implementation, I see you are adding a new dependency to a SCIM client library. Would be possible to just use our |
|
|
||
| OID4VC_VCI("Support for the OID4VCI protocol as part of OID4VC.", Type.EXPERIMENTAL), | ||
|
|
||
| SCIM("Synchronise users and groups with registered SCIM endpoints", Type.EXPERIMENTAL), |
There was a problem hiding this comment.
Can we have a name that makes it clear that we are not really delivering SCIM but a part of it? In this case, we are adding a SCIM-compliant user storage so perhaps something like scim-federation or something ...
There was a problem hiding this comment.
SCIM Federation seems perfect. Should I amend and replace the previous commit or include the fix in a new one ?
There was a problem hiding this comment.
Sorry, a new commit sounds good. We can squash them all before merging.
| @@ -0,0 +1,10 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
Looks like you have been using this extension but running the Wildfly-based releases of Keycloak?
We should not need it anymore as we are running on Quarkus now.
1c7f7e7 to
af620cb
Compare
|
Hello @pedroigor thanks a lot for your review ! Indeed we use a SCIM SDK Library rather than implementing our own. This allows to reduce the scope of this Keycloak extension, which focuses only on bridging Keycloak with SCIM calls. That being said, if this is too problematic to introduce this dependency I think rewriting the SCIM calls by using simple Http requests is possible. But then this Keycloack extension will have to make sure to stay compatible with evolutions of the protocol, which can mean a bit more maintenance work. |
|
Thank you, @alexsedlex. It is OK to keep pace with the core specifications we support. I had some discussions with the team about how to proceed with the dependency you are proposing and we all agree we should go with our own implementation (as simple as possible) based on the HTTP client we currently support. I'm working on some tests while looking at how everything works. Do you want us to take over the implementation of the SCIM client or are you fine doing it? |
|
If you have resource to do it, please go ahead :) |
|
Thanks for the notification. My SCIM-SDK comes with a minimal set of dependencies so it shouldn't be a problem to add it to Keycloak because all of its transitive core-dependencies are already included in Keycloak. |
Closes keycloak#1234 Signed-off-by: Alex Morel <amorel@codelutin.com>
af620cb to
17c6f2a
Compare
| session.getContext().setRealm(realm); | ||
| ScimDispatcher dispatcher = new ScimDispatcher(session); | ||
| // Identify groups marked as dirty by the ScimEventListenerProvider | ||
| for (GroupModel group : session.groups().getGroupsStream(realm).filter(this::isDirtyGroup).toList()) { |
There was a problem hiding this comment.
We need a specific query to fetch dirty groups. Otherwise, it won't scale well when using a database with thousands or millions of groups. Mainly during start-up.
| * concurrent group membership changes. | ||
| */ | ||
| public class ScimBackgroundGroupMembershipUpdater { | ||
| public static final String GROUP_DIRTY_SINCE_ATTRIBUTE_NAME = "scim-dirty-since"; |
There was a problem hiding this comment.
The group attribute will be visible as a regular group attribute and allow updating and removing the attribute when managing a group via Group API. Do we want to prevent that and enforce this attribute being managed only by the provider?
Wouldn't be better to add a property to the GroupModel (and a column in the database) to track the last time a group was updated? The idea is to have a better and indexable query to fetch dirty groups from the database by comparing the last time a group was updated with the last time the job ran. The latter can be stored in the component model of the provider, I think.
There was a problem hiding this comment.
I'm also fine if you think we can drop this scheduled task for now and focus on the core capabilities you are delivering. We can always revisit this later and start discussions to decide on the best solution for this problem before making the feature supported.
It should not be a blocker, IMO, and we should probably discuss it in a follow-up.
|
|
||
| .Procedure | ||
| . Click on *Admin Console > Realm Settings > Events* in the menu. | ||
| . Add `scim` to the list of event listeners |
There was a problem hiding this comment.
Wouldn't be better if we skip this step and manage the event listener within the provider behind a setting? Or even automatically add the listener to the realm whenever a SCIM provider is created in the realm?
I'm trying to see the use cases where you don't want the listener enabled but still want to manually sync data by executing the different synchronization actions (e.g.: sync all users, sync changed users, etc).
| session.getTransactionManager().rollback(); | ||
| LOGGER.error("TRANSACTION ROLLBACK - " + errorMessage, e); | ||
| } else { | ||
| LOGGER.warn(errorMessage, e); |
There was a problem hiding this comment.
We need to propagate errors when failing to manage resources. For instance, if the SCIM server returns user:emails field is required we should be able to transform it into a message so that you can understand what is going on.
We should also avoid creating the user in the local database if not able to create the user at the SCIM server.
Did you consider implementing the UserRegistrationProvider in the SCIM provider to be able to handle the creation and removal of users? Similar to how our LDAP provider works.
| personRole.setValue(roleName); | ||
| return personRole; | ||
| }).toList(); | ||
| User user = new User(); |
There was a problem hiding this comment.
For users, we could leverage user profile to configure the mapping between attributes in the UserModel to SCIM attributes.
I'm also wondering what are your thoughts about supporting SCIM schemas (e.g.: users) other than the core schema. Does it make sense as a follow-up?
| UserModel user = getKeycloakDao().addUser(username); | ||
| resource.getEmails().stream().findFirst().flatMap(MultiComplexNode::getValue).ifPresent(user::setEmail); | ||
| boolean userEnabled = resource.isActive().orElse(false); | ||
| user.setEnabled(userEnabled); |
There was a problem hiding this comment.
We should create users through the UserProfileProvider in order to enforce realm-level settings when managing users in a realm. For instance, the validations defined for user attributes in the User Profile tab in the Realm Settings.
Perhaps it also makes sense to allow pulling users without any validation so that they can update their profiles later. For that, we might want a switch.
For now, I don't see it as a blocker but looking at this code it seems we are only setting the email from the response from the SCIM server?
| String username = resource.getUserName().filter(StringUtils::isNotBlank) | ||
| .orElseThrow(() -> new UnexpectedScimDataException( | ||
| "Remote Scim user has empty username, can't create. Resource id = %s".formatted(resource.getId()))); | ||
| UserModel user = getKeycloakDao().addUser(username); |
There was a problem hiding this comment.
We also need to set the federationLink to the UserModel to bind the local user with this provider. The provider should also implement UserRegistrationProvider to be able to remove users properly.
| * Empty implementation : we used this {@link ScimEndpointConfigurationStorageProviderFactory} to generate Admin Console | ||
| * page. | ||
| */ | ||
| public static final class ScimEndpointConfigurationStorageProvider implements UserStorageProvider { |
There was a problem hiding this comment.
I had a chat with @stianst about this one and we go with a UserStorageProvider implementation instead of an event listener.
The reasons are that we should be able to deliver much more federation capabilities as well as limitations when using event listeners. For instance, you cannot delete users without a UserRegistrationProvider implementation. Also, event listeners are executed after removing resources which makes it hard to fetch data from these resources other than sharing data using an event detail.
There was a problem hiding this comment.
@alexsedlex When you have some time, please look at https://github.com/pedroigor/keycloak/commits/scim-support-13484-review/. It is a WIP branch with some changes I'm testing to fix some issues I found when reviewing your contribution and make it simpler. Please, let me know if you want me to update your branch with the commits from there.
In terms of planning, we would love to deliver SCIM Federation support in the following sprints but due to other priorities, we should be able to help with reviews and feedback as I'm already trying to do.
From my side, I think we are very close to have this experimental feature. The main points we need to sort out are:
- Remove the dependency to the 3rd party SCIM client SDK
- Replace the SCIM event listener with a SCIM
UserStorageProviderimplementation
That means we can focus on delivering the experimental feature with the following capabilities enabled:
- Connect Keycloak to a SCIM Service Provider
- Synchronize changes from and to a SCIM Service Provider using the provider synchronization mechanisms
- Synchronize changes from and to a SCIM Service Provider when managing user and groups
- Support only the basic/core user attributes/schema
- Support only the basic/core group attributes/schema
Anything else can be a follow-up. Some of the things we might want to consider are:
- Support for SCIM Mappers and integrated with user profile
- Support for linking a SCIM Federation with an identity provider
- Better HTTP mechanism mainly in regard to failover
- Continous Testing
It would be perfect if you could help move forward with this PR. Otherwise, we'll be able to focus 100% on it only after April next year.
|
any updates on this? would be great to have this support in keycloak. |
|
@pedroigor I'm willing to takeover the feature implementation and created a new PR. Let's continue there to get this feature merged. |
…eycloak#34733) This PR provides SCIM 2.0 federation support for Keycloak: ## Key Changes (Addressing Review Feedback) - Use UserStorageProvider instead of EventListener for user synchronization - Use native Java HTTP client instead of 3rd party SCIM SDK ## New Components - ScimUserStorageProvider: Main UserStorageProvider implementation - ScimUserStorageProviderFactory: Factory with configuration options - ScimClient: Native HTTP client for SCIM protocol (no external SDK) - ScimUser/ScimUserAdapter: User model mapping ## Features - Connect Keycloak to SCIM Service Providers - User lookup by username, email, or ID - User search with SCIM filter support - User synchronization via storage provider mechanisms - Basic and Bearer token authentication support Enabled behind experimental feature profile. Refs: keycloak#13848 keycloak#13484
As discussed in #13848 #13484 this PR provides a first draft implementation for bringing SCIM2 client capabilities into keycloak, behing an Experimental Feature profile.
This PR adds a SCIM module allowing to :
Keycloack instance through this declaration.
the modification is fowarded to all declared SCIM endpoints through SCIM calls within the transaction scope. If
propagation fails, changes can be rolled back or not according to a configurable rollback strategy.
See RFC7643
and RFC7644) for further details