8000 New client service - PUT/POST - update or create client by mabartos · Pull Request #47029 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

New client service - PUT/POST - update or create client #47029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vmuzikar merged 4 commits into from
Apr 13, 2026
Merged

New client service - PUT/POST - update or create client #47029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dcd598d
New client service - PUT/POST - update or create client
mabartos Mar 10, 2026
f6287d3
Assert same payload and path clientId for PUT/PATCH as in legacy service
mabartos Mar 27, 2026
da64696
Provide link to a spike around client policices rep
mabartos Mar 30, 2026
c5fb47f
Remove phishing check for CREATE
mabartos Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rest/admin-v2/api/src/main/java/org/keycloak/models/mapper/BaseClientModelMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public ClientModel toModel(BaseClientRepresentation rep, ClientModel existingMod
existingModel = createClientModel(rep);
}

existingModel.setProtocol(rep.getProtocol());
existingModel.setEnabled(Boolean.TRUE.equals(rep.getEnabled()));
existingModel.setClientId(rep.getClientId());
existingModel.setDescription(rep.getDescription());
Expand Down
28 changes: 28 additions & 0 deletions rest/admin-v2/api/src/main/java/org/keycloak/services/RolesService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package org.keycloak.services;

import org.keycloak.models.KeycloakSession;
import org.keycloak.models.RealmModel;
import org.keycloak.models.RoleContainerModel;
import org.keycloak.services.resources.admin.AdminEventBuilder;
import org.keycloak.services.resources.admin.RoleContainerResource;
import org.keycloak.services.resources.admin.fgap.AdminPermissionEvaluator;

public class RolesService {
Comment thread
vmuzikar marked this conversation as resolved.
Resolved
private final KeycloakSession session;
private final RealmModel realm;
private final AdminPermissionEvaluator permissions;
private final AdminEventBuilder adminEventBuilder;

public RolesService(KeycloakSession session, RealmModel realm, AdminPermissionEvaluator permissions, AdminEventBuilder adminEventBuilder) {
this.session = session;
this.realm = realm;
this.permissions = permissions;
this.adminEventBuilder = adminEventBuilder;
}

public RoleContainerResource resource(RoleContainerModel roleContainer) {
var resource = new RoleContainerResource(session, session.getContext().getUri(), realm, permissions, adminEventBuilder);
resource.setRoleContainer(roleContainer);
return resource;
}
}
31 changes: 15 additions & 16 deletions rest/admin-v2/api/src/main/java/org/keycloak/services/client/DefaultClientService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,13 @@
import org.keycloak.representations.idm.RoleRepresentation;
import org.keycloak.services.PatchType;
import org.keycloak.services.ServiceException;
import org.keycloak.services.managers.ClientManager;
import org.keycloak.services.managers.RealmManager;
import org.keycloak.services.resources.admin.AdminEventBuilder;
import org.keycloak.services.resources.admin.ClientResource;
import org.keycloak.services.resources.admin.ClientsResource;
import org.keycloak.services.resources.admin.RealmAdminResource;
import org.keycloak.services.resources.admin.RoleContainerResource;
import org.keycloak.services.resources.admin.fgap.AdminPermissionEvaluator;
import org.keycloak.services.util.ObjectMapperResolver;
import org.keycloak.utils.StringUtil;
Expand Down Expand Up @@ -114,7 +117,7 @@ public CreateOrUpdateResult createOrUpdateClient(RealmModel realm, String client
return createOrUpdate(realm, clientId, client, CreateOrUpdateStrategy.PUT);
}

private enum CreateOrUpdateStrategy {
protected enum CreateOrUpdateStrategy {
ONLY_CREATE(CreateClient.class),
PUT(PutClient.class),
PATCH(PatchClient.class);
Expand Down Expand Up @@ -194,9 +197,9 @@ private CreateOrUpdateResult createOrUpdate(RealmModel realm, String clientId, B
});
}

handleRoles(clientResource, client.getRoles());
handleRoles(clientResource.getRoleContainerResource(), client.getRoles());
if (client instanceof OIDCClientRepresentation oidcClient) {
handleServiceAccount(clientResource, model, oidcClient);
handleServiceAccount(clientResource.getRoleContainerResource(), realmResource.getRoleContainerResource(), model, oidcClient);
}

fireAdminEvent(created ? OperationType.CREATE : OperationType.UPDATE, mapper.fromModel(model));
Expand Down Expand Up @@ -282,21 +285,19 @@ protected void assertSameClientIds(String pathId, String payloadId) {
* <p>
* Reuses API v1 logic
*/
protected void handleRoles(ClientResource clientResource, Set<String> rolesFromRep) {
var roleResource = clientResource.getRoleContainerResource();

protected void handleRoles(RoleContainerResource clientRoles, Set<String> rolesFromRep) {
Set<String> desiredRoleNames = Optional.ofNullable(rolesFromRep)
.orElse(Collections.emptySet());

Set<String> currentRoleNames = roleResource.getRoles(null, null, null, false)
Set<String> currentRoleNames = clientRoles.getRoles(null, null, null, false)
.map(RoleRepresentation::getName)
.collect(Collectors.toSet());

// Add missing roles (in desiredRoleNames but not in currentRoleNames)
desiredRoleNames.stream()
.filter(roleName -> !currentRoleNames.contains(roleName))
.forEach(roleName -> {
try (var response = roleResource.createRole(new RoleRepresentation(roleName, "", false))) {
try (var response = clientRoles.createRole(new RoleRepresentation(roleName, "", false))) {
// close response and consume payload due to performance reasons
EntityUtils.consumeQuietly((HttpEntity) response.getEntity());
}
Expand All @@ -305,28 +306,26 @@ protected void handleRoles(ClientResource clientResource, Set<String> rolesFromR
// Remove extra roles (in currentRoleNames but not in desiredRoleNames)
currentRoleNames.stream()
.filter(role -> !desiredRoleNames.contains(role))
.forEach(roleResource::deleteRole);
.forEach(clientRoles::deleteRole);
}

/**
* Declaratively manage service account - enables/disables it and ensures it has exactly the roles specified (realm and client roles)
* <p>
* Reuses API v1 logic
*/
protected void handleServiceAccount(ClientResource clientResource, ClientModel model, OIDCClientRepresentation rep) {
protected void handleServiceAccount(RoleContainerResource clientRoleResource, RoleContainerResource realmRoleResource, ClientModel model, OIDCClientRepresentation rep) {
boolean serviceAccountEnabled = rep.getLoginFlows().contains(OIDCClientRepresentation.Flow.SERVICE_ACCOUNT);

ClientResource.updateClientServiceAccount(session, model, serviceAccountEnabled);
ClientManager.updateClientServiceAccount(session, model, serviceAccountEnabled);

if (!serviceAccountEnabled) {
return;
}

var clientRoleResource = clientResource.getRoleContainerResource();
var realmRoleResource = realmResource.getRoleContainerResource();

var serviceAccountUser = session.users().getServiceAccount(model);
var serviceAccountRoleResource = realmResource.users().user(clientResource.getServiceAccountUser().getId()).getRoleMappings();
var serviceAccountUser = new ClientManager(new RealmManager(session)).getServiceAccountUser(model)
.orElseThrow(() -> new ServiceException(" 8000 ;Cannot find service account user", Response.Status.BAD_REQUEST));
var serviceAccountRoleResource = realmResource.users().user(serviceAccountUser.getId()).getRoleMappings();

Set<String> desiredRoleNames = Optional.ofNullable(rep.getServiceAccountRoles()).orElse(Collections.emptySet());
Set<RoleModel> currentRoles = serviceAccountUser.getRoleMappingsStream().collect(Collectors.toSet());
Expand Down
129 changes: 129 additions & 0 deletions rest/admin-v2/api/src/main/java/org/keycloak/services/client/NewClientService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import java.util.stream.Stream;

import jakarta.annotation.Nonnull;
import jakarta.validation.groups.Default;
import jakarta.ws.rs.core.Response;

import org.keycloak.authorization.fgap.AdminPermissionsSchema;
Expand All @@ -15,16 +16,33 @@
import org.keycloak.models.ModelException;
import org.keycloak.models.ModelValidationException;
import org.keycloak.models.RealmModel;
import org.keycloak.models.mapper.ClientModelMapper;
import org.keycloak.models.utils.KeycloakModelUtils;
import org.keycloak.models.utils.ModelToRepresentation;
import org.keycloak.representations.admin.v2.BaseClientRepresentation;
import org.keycloak.representations.admin.v2.OIDCClientRepresentation;
import org.keycloak.representations.idm.ClientRepresentation;
import org.keycloak.services.RolesService;
import org.keycloak.services.ServiceException;
import org.keycloak.services.clientpolicy.ClientPolicyException;
import org.keycloak.services.clientpolicy.context.AdminClientRegisterContext;
import org.keycloak.services.clientpolicy.context.AdminClientRegisteredContext;
import org.keycloak.services.clientpolicy.context.AdminClientUnregisterContext;
import org.keycloak.services.clientpolicy.context.AdminClientUpdateContext;
import org.keycloak.services.clientpolicy.context.AdminClientUpdatedContext;
import org.keycloak.services.clientpolicy.context.AdminClientViewContext;
import org.keycloak.services.managers.ClientManager;
import org.keycloak.services.managers.RealmManager;
import org.keycloak.services.resources.admin.AdminEventBuilder;
import org.keycloak.services.resources.admin.RealmAdminResource;
import org.keycloak.services.resources.admin.fgap.AdminPermissionEvaluator;
import org.keycloak.validation.ValidationUtil;
import org.keycloak.validation.jakarta.HibernateValidatorProvider;
import org.keycloak.validation.jakarta.JakartaValidatorProvider;
import org.keycloak.validation.jakarta.ValidationContext;

import static org.keycloak.representations.admin.v2.validation.ClientSecretNotBlankValidator.isClientSecret;
import static org.keycloak.utils.StringUtil.isBlank;

/**
* New client service (name is just temporary) that should not rely on the Admin API v1
Expand All @@ -35,6 +53,8 @@ public class NewClientService extends DefaultClientService implements ClientServ
private final KeycloakSession session;
private final AdminPermissionEvaluator permissions;
private final AdminEventBuilder adminEventBuilder;
private final JakartaValidatorProvider validator;
private final RolesService rolesService;

public NewClientService(@Nonnull KeycloakSession session,
@Nonnull RealmModel realm,
Expand All @@ -45,6 +65,8 @@ public NewClientService(@Nonnull KeycloakSession session,
this.session = session;
this.permissions = permissions;
this.adminEventBuilder = new AdminEventV2Builder(realm, permissions.adminAuth(), session, session.getContext().getConnection()).resource(ResourceType.CLIENT);
this.validator = new HibernateValidatorProvider(new ValidationContext(session, realm));
this.rolesService = new RolesService(session, realm, permissions, adminEventBuilder);
}

protected Optional<ClientModel> avoidClientIdPhishing(ClientModel client) throws ServiceException {
Expand Down Expand Up @@ -91,6 +113,16 @@ public Stream<BaseClientRepresentation> getClients(@Nonnull RealmModel realm,
}
}

@Override
public BaseClientRepresentation createClient(RealmModel realm, BaseClientRepresentation client) throws ServiceException {
return createOrUpdate(realm, null, client, CreateOrUpdateStrategy.ONLY_CREATE).representation();
}

@Override
public CreateOrUpdateResult createOrUpdateClient(RealmModel realm, String clientId, BaseClientRepresentation client) throws ServiceException {
return createOrUpdate(realm, clientId, client, CreateOrUpdateStrategy.PUT);
}

@Override
public void deleteClient(RealmModel realm, String clientId) throws ServiceException {
ClientModel client = avoidClientIdPhishing(realm.getClientByClientId(clientId))
Expand All @@ -115,4 +147,101 @@ public void deleteClient(RealmModel realm, String clientId) throws ServiceExcept
throw new ServiceException("Could not delete client", Response.Status.BAD_REQUEST);
}
}

private CreateOrUpdateResult createOrUpdate(RealmModel realm, String clientId, BaseClientRepresentation client, CreateOrUpdateStrategy strategy) throws ServiceException {
validateUnknownFields(client);
ClientModel model = null;
if (!strategy.equals(CreateOrUpdateStrategy.ONLY_CREATE)) {
assertSameClientIds(clientId, client.getClientId());
model = avoidClientIdPhishing(realm.getClientByClientId(clientId)).orElse(null);
}
boolean alreadyExists = model != null;
ClientModelMapper mapper = getMapper(client.getProtocol());

try {
if (alreadyExists) {
switch (strategy) {
case ONLY_CREATE -> throw new ServiceException("Client already exists", Response.Status.CONFLICT);
case PUT, PATCH -> {
// Check permissions, execute validations and trigger client policies
permissions.clients().requireConfigure(model);
validator.validate(client, strategy.getValidationGroup(), Default.class);
var proposedRepresentation = getProposedOldRepresentation(realm, client, mapper);
Comment thread
mabartos marked this conversation as resolved.
Resolved
session.clientPolicy().triggerOnEvent(new AdminClientUpdateContext(proposedRepresentation, model, permissions.adminAuth()));

// Update model
model = mapper.toModel(client, model);

// Validate the fully populated model
ValidationUtil.validateClient(session, model, false, r -> {
session.getTransactionManager().setRollbackOnly();
throw new ServiceException(r.getAllErrorsAsString(), Response.Status.BAD_REQUEST);
});
Comment thread
vmuzikar marked this conversation as resolved.
Resolved

session.clientPolicy().triggerOnEvent(new AdminClientUpdatedContext(proposedRepresentation, model, permissions.adminAuth()));
}
}
} else {
// Check permissions, execute validations and trigger client policies
permissions.clients().requireManage();
validator.validate(client, strategy.getValidationGroup(), Default.class);
var proposedRepresentation = getProposedOldRepresentation(realm, client, mapper);
Comment thread
mabartos marked this conversation as resolved.
Resolved
session.clientPolicy().triggerOnEvent(new AdminClientRegisterContext(proposedRepresentation, permissions.adminAuth()));

// Add basic attributes
model = realm.addClient(clientId);
model.setProtocol(client.getProtocol());

// Generate random secret if applicable
if (client.getProtocol().equals(OIDCClientRepresentation.PROTOCOL)) {
// TODO we should find a way on how to evoke it on the mapper level?
var auth = ((OIDCClientRepresentation) client).getAuth();
if (auth != null && isClientSecret(auth.getMethod()) && isBlank(auth.getSecret())) {
auth.setSecret(KeycloakModelUtils.generateSecret(model));
}
}
mapper.toModel(client, model);

// Validate the fully populated model
ValidationUtil.validateClient(session, model, true, r -> {
session.getTransactionManager().setRollbackOnly();
throw new ServiceException(r.getAllErrorsAsString(), Response.Status.BAD_REQUEST);
});
session.clientPolicy().triggerOnEvent(new AdminClientRegisteredContext(model, permissions.adminAuth()));
}
} catch (ClientPolicyException e) {
throw new ServiceException(e.getErrorDetail(), Response.Status.BAD_REQUEST);
}

// Setup roles
var clientRoles = rolesService.resource(model);
handleRoles(clientRoles, client.getRoles());

// OIDC specific
if (client instanceof OIDCClientRepresentation oidcClient) {
handleServiceAccount(clientRoles, rolesService.resource(realm), model, oidcClient);
}

fireAdminEvent(alreadyExists ? OperationType.UPDATE : OperationType.CREATE, mapper.fromModel(model));
return new CreateOrUpdateResult(mapper.fromModel(model), !alreadyExists);
Comment on lines +225 to +226
@Pepo48 Pepo48 Apr 2, 2026
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mapper.fromModel(model) called twice:

Suggested change
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);

@mabartos mabartos Apr 8, 2026
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pepo48 Expected. Related to this:

}

/**
* 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) {
@vmuzikar vmuzikar Mar 25, 2026
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@mabartos mabartos Mar 30, 2026
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created spike:

@Pepo48 Pepo48 Apr 2, 2026
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we directly maps v2 fields to a v1 ClientRepresentation in memory? (see suggestion below)

@mabartos mabartos Apr 8, 2026
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pepo48 Commented on the other comment.

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());
}
}
}
Comment on lines +229 to 247
@Pepo48 Pepo48 Apr 2, 2026
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* 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.

@mabartos mabartos Apr 8, 2026
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not sure what's better. I'm still inclining to reuse the model for this to prevent any maintenance burden - as providing a new field to the representation, you need to keep track of places that touch that.

@vmuzikar @Pepo48 WDYT? Or would you like to complete it in a follow-up?

@vmuzikar vmuzikar Apr 10, 2026
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading
Loading
0