Repository navigation
New client service - PUT/POST - update or create client - #47029
Conversation
2c4bc7f to
fb97143
Compare
fb97143 to
9c62205
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements a new client service (NewClientService) that handles PUT/POST operations to create or update clients without relying on the Admin API v1. The implementation includes:
- NewClientService: A new service class that extends DefaultClientService and provides dedicated create/update logic with proper validation and client policy trigger handling
- RolesService: A new utility service to manage client roles through RoleContainerResource instances
- ClientManager enhancements: Added methods to safely handle service account management with Optional-based return types
- Bug fixes: Fixed a typo in AdminClientUpdatedContext and refactored service account logic in ClientResource
Changes:
- Implemented
createClient()andcreateOrUpdateClient()methods in NewClientService - Added RolesService for declarative role management
- Refactored service account handling to use ClientManager static methods
- Fixed typo in constructor parameter name
- Added protocol setting to BaseClientModelMapper for consistency
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rest/admin-v2/api/src/main/java/org/keycloak/services/client/NewClientService.java | Main implementation of new client service with create/update logic |
| rest/admin-v2/api/src/main/java/org/keycloak/services/RolesService.java | New service for declarative role management |
| rest/admin-v2/api/src/main/java/org/keycloak/services/client/DefaultClientService.java | Refactored handleServiceAccount and handleRoles methods to use RoleContainerResource |
| services/src/main/java/org/keycloak/services/managers/ClientManager.java | Added getServiceAccountUser() and updateClientServiceAccount() methods |
| services/src/main/java/org/keycloak/services/resources/admin/RoleContainerResource.java | Added new constructor and setter for flexible initialization |
| rest/admin-v2/api/src/main/java/org/keycloak/models/mapper/BaseClientModelMapper.java | Added protocol setting in toModel method |
| services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java | Removed unused import, refactored service account logic |
| services/src/main/java/org/keycloak/services/clientpolicy/context/AdminClientUpdatedContext.java | Fixed typo in parameter name |
| rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientPoliciesV2Test.java | Added conditional assertions for legacy vs new service |
You can also share your feedback on Copilot code review. Take the survey.
9c62205 to
d7ab2a2
Compare
There was a problem hiding this comment.
Pull request overview
This pull request implements the new client service's PUT/POST logic for creating and updating clients without relying on Admin API v1. The implementation extracts and unifies common roles and service account handling logic, creates a new RolesService for better separation of concerns, and fixes a bug where protocol wasn't being mapped correctly during updates.
Changes:
- Implements
createOrUpdatemethod in NewClientService with full validation and policy event triggers - Extracts service account logic to ClientManager with static methods for broader reusability
- Creates new RolesService for decoupled role resource management
- Enhances RoleContainerResource with flexible initialization via new constructor and setter
- Fixes protocol mapping bug in BaseClientModelMapper by ensuring protocol is set during updates
- Updates test assertions to accommodate differences between legacy and new client service implementations
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| AdminClientUpdatedContext.java | Fixes typo in parameter name from "roposed" to "proposed" |
| BaseClientModelMapper.java | Adds protocol setting for existing models to fix update bug |
| ClientManager.java | Adds public methods for service account management and Optional-returning helper |
| ClientResource.java | Refactors to use new ClientManager methods, removing duplicate code |
| RoleContainerResource.java | Adds new constructor and setter for flexible role container initialization |
| RolesService.java | New service class for managing role resources with proper dependency injection |
| NewClientService.java | Implements complete createOrUpdate logic with validation, policy events, and role/service account handling |
| DefaultClientService.java | Refactors handleRoles and handleServiceAccount to use RoleContainerResource directly |
| ClientPoliciesV2Test.java | Adds transaction rollback verification and conditional assertions for service implementation differences |
You can also share your feedback on Copilot code review. Take the survey.
d7ab2a2 to
6175e6b
Compare
6175e6b to
d653280
Compare
| * Required because client policy contexts expect ClientRepresentation (v1), but there's no | ||
| * direct converter from BaseClientRepresentation (v2 API). The temp client is immediately removed. | ||
| */ | ||
| private ClientRepresentation getProposedOldRepresentation(RealmModel realm, BaseClientRepresentation client, ClientModelMapper mapper) { |
There was a problem hiding this comment.
As discussed offline, this is not ideal. @mabartos Can you please create a follow-up issue to look into how we could resolve that so we don't need to create fake clients?
There was a problem hiding this comment.
There was a problem hiding this comment.
Could we directly maps v2 fields to a v1 ClientRepresentation in memory? (see suggestion below)
|
We should wait for the: Or we can make some additional changes in follow-ups - I don't have strong preferences. |
04877d4 to
4297d09
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.broker.KcOidcBrokerTest#loginWithExistingUserWithBruteForceEnabledKeycloak CI - Java Distribution IT (windows-latest - temurin - 17) |
4297d09 to
2bfabc7
Compare
|
@vmuzikar Your comments should be addressed now. |
| * Required because client policy contexts expect ClientRepresentation (v1), but there's no | ||
| * direct converter from BaseClientRepresentation (v2 API). The temp client is immediately removed. | ||
| */ | ||
| private ClientRepresentation getProposedOldRepresentation(RealmModel realm, BaseClientRepresentation client, ClientModelMapper mapper) { |
There was a problem hiding this comment.
Could we directly maps v2 fields to a v1 ClientRepresentation in memory? (see suggestion below)
| /** | ||
| * Creates a temporary client to convert BaseClientRepresentation to ClientRepresentation. | ||
| * Required because client policy contexts expect ClientRepresentation (v1), but there's no | ||
| * direct converter from BaseClientRepresentation (v2 API). The temp client is immediately removed. | ||
| * <p> | ||
| * For more details, see the <a href="https://github.com/keycloak/keycloak/issues/47576">keycloak#47576</a>. | ||
| */ | ||
| private ClientRepresentation getProposedOldRepresentation(RealmModel realm, BaseClientRepresentation client, ClientModelMapper mapper) { | ||
| String tempId = "__temp__" + client.getClientId() + "__" + System.nanoTime(); | ||
| ClientModel tempModel = mapper.toModel(client, realm.addClient(tempId)); | ||
| try { | ||
| var proposedRepresentation = ModelToRepresentation.toRepresentation(tempModel, session); | ||
| proposedRepresentation.setClientId(client.getClientId()); | ||
| return proposedRepresentation; | ||
| } finally { | ||
| realm.removeClient(tempModel.getId()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| /** | |
| * Creates a temporary client to convert BaseClientRepresentation to ClientRepresentation. | |
| * Required because client policy contexts expect ClientRepresentation (v1), but there's no | |
| * direct converter from BaseClientRepresentation (v2 API). The temp client is immediately removed. | |
| * <p> | |
| * For more details, see the <a href="https://github.com/keycloak/keycloak/issues/47576">keycloak#47576</a>. | |
| */ | |
| private ClientRepresentation getProposedOldRepresentation(RealmModel realm, BaseClientRepresentation client, ClientModelMapper mapper) { | |
| String tempId = "__temp__" + client.getClientId() + "__" + System.nanoTime(); | |
| ClientModel tempModel = mapper.toModel(client, realm.addClient(tempId)); | |
| try { | |
| var proposedRepresentation = ModelToRepresentation.toRepresentation(tempModel, session); | |
| proposedRepresentation.setClientId(client.getClientId()); | |
| return proposedRepresentation; | |
| } finally { | |
| realm.removeClient(tempModel.getId()); | |
| } | |
| } | |
| } | |
| /** | |
| * Converts a v2 BaseClientRepresentation to a v1 ClientRepresentation without creating | |
| * any database entities. This is needed because client policy contexts expect ClientRepresentation (v1). | |
| * <p> | |
| * For more details, see the <a href="https://github.com/keycloak/keycloak/issues/47576">keycloak#47576</a>. | |
| */ | |
| private ClientRepresentation toProposedV1Representation(BaseClientRepresentation client) { | |
| ClientRepresentation rep = new ClientRepresentation(); | |
| rep.setClientId(client.getClientId()); | |
| rep.setName(client.getDisplayName()); | |
| rep.setDescription(client.getDescription()); | |
| rep.setEnabled(Boolean.TRUE.equals(client.getEnabled())); | |
| rep.setBaseUrl(client.getAppUrl()); | |
| rep.setRootUrl(client.getAppUrl()); | |
| rep.setProtocol(client.getProtocol()); | |
| rep.setRedirectUris(client.getRedirectUris() != null ? new ArrayList<>(client.getRedirectUris()) : null); | |
| if (client instanceof OIDCClientRepresentation oidc) { | |
| boolean isPublic = oidc.getAuth() == null; | |
| rep.setPublicClient(isPublic); | |
| if (!isPublic) { | |
| rep.setClientAuthenticatorType(oidc.getAuth().getMethod()); | |
| rep.setSecret(oidc.getAuth().getSecret()); | |
| } | |
| Set<OIDCClientRepresentation.Flow> flows = Optional.ofNullable(oidc.getLoginFlows()).orElse(Collections.emptySet()); | |
| rep.setStandardFlowEnabled(flows.contains(OIDCClientRepresentation.Flow.STANDARD)); | |
| rep.setImplicitFlowEnabled(flows.contains(OIDCClientRepresentation.Flow.IMPLICIT)); | |
| rep.setDirectAccessGrantsEnabled(flows.contains(OIDCClientRepresentation.Flow.DIRECT_GRANT)); | |
| rep.setServiceAccountsEnabled(flows.contains(OIDCClientRepresentation.Flow.SERVICE_ACCOUNT)); | |
| if (oidc.getWebOrigins() != null) { | |
| rep.setWebOrigins(new ArrayList<>(oidc.getWebOrigins())); | |
| } | |
| } | |
| return rep; | |
| } | |
| } |
This should preserve the exact semantics for client policy executors.
I also did some checks with this fix in place, all fields seems to be mapped and ClientApiV2Test, ClientPoliciesV2Test, PatchClientValidationTest, PostClientValidationTest and PutClientValidationTest passed.
There was a problem hiding this comment.
There was a problem hiding this comment.
Creating a model is far beyond ideal here. However, so is maintaining another mapping logic (v2 --> v1 rep). Let's keep as is and reassess in a follow-up.
There was a problem hiding this comment.
| existingModel.setProtocol(rep.getProtocol()); | |
| existingModel.setEnabled(Boolean.TRUE.equals(rep.getEnabled())); | |
| existingModel.setClientId(rep.getClientId()); | |
| existingModel.setDescription(rep.getDescription()); | |
| existingModel.setName(rep.getDisplayName()); | |
| existingModel.setBaseUrl(rep.getAppUrl()); | |
| existingModel.setRedirectUris(new HashSet<>(rep.getRedirectUris())); | |
| // Roles are not handled here | |
| toModelSpecific((T) rep, existingModel); | |
| return existingModel; | |
| } |
I think this is just a leftover the single-arg toModel(rep) was used in the original mapper commit (421835c1e6) but was abandoned by the time DefaultClientService introduction (see the comment on the line 165 that says that we can't use mapper.toModel(client) directly for creation).
Thus, I believe createClientModel() here and the default toModel(rep) in RepModelMapper can be cleaned up.
There was a problem hiding this comment.
I guess it's related to your comment here: #47029 (comment)
@Pepo48 @vmuzikar Are you ok with cleaning this up in the follow-up? Could you please create an issue?
| if (model == null) { | ||
| throw new ServiceException("Cannot create/update client"); | ||
| } |
There was a problem hiding this comment.
model is always non-null at this point, no?
| assertSameClientIds(clientId, client.getClientId()); | ||
| } | ||
|
|
||
| ClientModel model = avoidClientIdPhishing(realm.getClientByClientId(clientId)).orElse(null); |
There was a problem hiding this comment.
I know it's probably an edge case (since even in the tests we always grant both VIEW and MANAGE together), but about scenario, when a user has MANAGE permission but no realm roles (canManage() = true, canList() = false). I suspect that for MANAGE-only grant, the phishing check would block a legitimate creation with 403.
Regardless of it, isn't the phishing check is semantically wrong for create/update? Like there is no risk of an information leak during these operations, these are expected conditions and requireManage() and/or requireConfigure() should cover it in terms of authorization.
| ClientModel model = avoidClientIdPhishing(realm.getClientByClientId(clientId)).orElse(null); | |
| ClientModel model = realm.getClientByClientId(clientId); |
There was a problem hiding this comment.
@Pepo48 Were you able to reproduce? At least in FGAP v1 it seems view permissions also consider manage:
There was a problem hiding this comment.
Yes, I tested it during the review. As I understand it, in V1, canView() checks for the MANAGE_CLIENTS role - not the FGAP manage scope. And since canManage() in V1 is purely role-based (MANAGE_CLIENTS role), any user who passes requireManage() automatically passes canList() too. The phishing check never blocks.
V2 introduced a new path: a user can pass requireManage() via FGAP manage scope without the MANAGE_CLIENTS role. But canView() in V2 only checks for the FGAP view scope - manage doesn't imply view. So canList() returns false, and avoidClientIdPhishing blocks before requireManage() ever runs.
tl;dr: avoidClientIdPhishing at line 157 was safe in V1 (role-based manage always implied list) but breaks in V2 (FGAP manage doesn't imply list).
| fireAdminEvent(alreadyExists ? OperationType.UPDATE : OperationType.CREATE, mapper.fromModel(model)); | ||
| return new CreateOrUpdateResult(mapper.fromModel(model), !alreadyExists); |
There was a problem hiding this comment.
mapper.fromModel(model) called twice:
| fireAdminEvent(alreadyExists ? OperationType.UPDATE : OperationType.CREATE, mapper.fromModel(model)); | |
| return new CreateOrUpdateResult(mapper.fromModel(model), !alreadyExists); | |
| var result = mapper.fromModel(model); | |
| fireAdminEvent(alreadyExists ? OperationType.UPDATE : OperationType.CREATE, result); | |
| return new CreateOrUpdateResult(result, !alreadyExists); |
|
During the review I noticed one more thing. In The behaviour was introduced in #44587, this PR just inherits it. Same issue exists for I think that for now it should be scoped to realm roles only until the roles v2 API #47631 lands. To reproduce it: |
2bfabc7 to
ea40932
Compare
@Pepo48 Thanks for the detailed analysis! It'd be good if you could create follow-ups for your findings. WDYT? |
Closes keycloak#46785 Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
ea40932 to
c5fb47f
Compare
|
Rebased |
|
* New client service - PUT/POST - update or create client Closes keycloak#46785 Signed-off-by: Martin Bartoš <mabartos@redhat.com> * Assert same payload and path clientId for PUT/PATCH as in legacy service Signed-off-by: Martin Bartoš <mabartos@redhat.com> * Provide link to a spike around client policices rep Signed-off-by: Martin Bartoš <mabartos@redhat.com> * Remove phishing check for CREATE Signed-off-by: Martin Bartoš <mabartos@redhat.com> --------- Signed-off-by: Martin Bartoš <mabartos@redhat.com>
createOrUpdate/servicesto be able to use it on other places