From 972678da1ca89d40d91773ef24d0edc4173ac475 Mon Sep 17 00:00:00 2001 From: Erik Jan de Wit Date: Mon, 27 Apr 2026 11:38:08 +0200 Subject: [PATCH 1/2] move phishing check fixes: #48010 Signed-off-by: Erik Jan de Wit --- .../admin/api/client/DefaultClientApi.java | 2 + .../admin/api/client/DefaultClientsApi.java | 13 + .../services/client/DefaultClientService.java | 21 +- .../services/client/NewClientService.java | 247 ------------------ .../admin/cli/v2/KcAdmV2ClientCLITest.java | 6 +- 5 files changed, 27 insertions(+), 262 deletions(-) delete mode 100644 rest/admin-v2/services/src/main/java/org/keycloak/services/client/NewClientService.java diff --git a/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientApi.java b/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientApi.java index 92d39db8935c..90c526d76933 100644 --- a/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientApi.java +++ b/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientApi.java @@ -28,6 +28,7 @@ public class DefaultClientApi implements ClientApi { private final KeycloakSession session; private final String clientId; private final RealmModel realm; + private final AdminPermissionEvaluator permissions; private final ClientService clientService; public DefaultClientApi(@Nonnull KeycloakSession session, @@ -39,6 +40,7 @@ public DefaultClientApi(@Nonnull KeycloakSession session, this.session = session; this.clientId = clientId; this.realm = realm; + this.permissions = permissions; this.clientService = new DefaultClientService(session, realm, permissions, realmAdminResource); } diff --git a/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientsApi.java b/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientsApi.java index 1bf834883651..fe8180fba474 100644 --- a/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientsApi.java +++ b/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientsApi.java @@ -4,6 +4,7 @@ import jakarta.annotation.Nonnull; import jakarta.validation.Valid; +import jakarta.ws.rs.ForbiddenException; import jakarta.ws.rs.GET; import jakarta.ws.rs.POST; import jakarta.ws.rs.Path; @@ -55,9 +56,21 @@ public Response createClient(@Valid BaseClientRepresentation client) { .build(); } + /** + * When the path {@code clientId} does not resolve, return 403 if the caller cannot list clients + * (anti client-ID phishing), matching {@code ClientsResource#getClient} for Admin API v1. + * Not applied to PUT upsert so {@code manage} without {@code list} can create by clientId. + */ + private void enforceAntiPhishingIfClientMissing(String clientId) { + if (realm.getClientByClientId(clientId) == null && !permissions.clients().canList()) { + throw new ForbiddenException(); + } + } + @Path("{id}") @Override public ClientApi client(@PathParam("id") String clientId) { + enforceAntiPhishingIfClientMissing(clientId); return new DefaultClientApi(session, realm, clientId, permissions, realmAdminResource); } diff --git a/rest/admin-v2/services/src/main/java/org/keycloak/services/client/DefaultClientService.java b/rest/admin-v2/services/src/main/java/org/keycloak/services/client/DefaultClientService.java index 497318ff02f3..357950b9c0de 100644 --- a/rest/admin-v2/services/src/main/java/org/keycloak/services/client/DefaultClientService.java +++ b/rest/admin-v2/services/src/main/java/org/keycloak/services/client/DefaultClientService.java @@ -95,19 +95,14 @@ public DefaultClientService(@Nonnull KeycloakSession session, this.rolesService = new RolesService(session, realm, permissions, adminEventBuilder); } - protected Optional avoidClientIdPhishing(ClientModel client) throws ServiceException { - if (client == null && !permissions.clients().canList()) { - // we do this to make sure somebody can't phish client IDs - throw new ServiceException(Response.Status.FORBIDDEN); - } - return Optional.ofNullable(client); - } - @Override public Optional getClient(@Nonnull RealmModel realm, @Nonnull String clientId, ClientProjectionOptions projectionOptions) throws ServiceException { - ClientModel client = avoidClientIdPhishing(realm.getClientByClientId(clientId)).orElseThrow(() -> new ServiceException("Could not find client", Response.Status.NOT_FOUND)); + ClientModel client = realm.getClientByClientId(clientId); + if (client == null) { + return Optional.empty(); + } permissions.clients().requireView(client); try { @@ -151,8 +146,10 @@ public CreateOrUpdateResult createOrUpdateClient(RealmModel realm, String client @Override public void deleteClient(RealmModel realm, String clientId) throws ServiceException { - ClientModel client = avoidClientIdPhishing(realm.getClientByClientId(clientId)) - .orElseThrow(() -> new ServiceException("Could not find client", Response.Status.NOT_FOUND)); + ClientModel client = realm.getClientByClientId(clientId); + if (client == null) { + throw new ServiceException("Could not find client", Response.Status.NOT_FOUND); + } permissions.clients().requireManage(client); try { @@ -234,7 +231,7 @@ private CreateOrUpdateResult createOrUpdate(RealmModel realm, String clientId, B ClientModel model = null; if (!strategy.equals(CreateOrUpdateStrategy.ONLY_CREATE)) { assertSameClientIds(clientId, client.getClientId()); - model = avoidClientIdPhishing(realm.getClientByClientId(clientId)).orElse(null); + model = realm.getClientByClientId(clientId); } boolean alreadyExists = model != null; ClientModelMapper mapper = getMapper(client.getProtocol()); diff --git a/rest/admin-v2/services/src/main/java/org/keycloak/services/client/NewClientService.java b/rest/admin-v2/services/src/main/java/org/keycloak/services/client/NewClientService.java deleted file mode 100644 index f4ec58019307..000000000000 --- a/rest/admin-v2/services/src/main/java/org/keycloak/services/client/NewClientService.java +++ /dev/null @@ -1,247 +0,0 @@ -package org.keycloak.services.client; - -import java.util.Optional; -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; -import org.keycloak.events.admin.OperationType; -import org.keycloak.events.admin.ResourceType; -import org.keycloak.events.admin.v2.AdminEventV2Builder; -import org.keycloak.models.ClientModel; -import org.keycloak.models.KeycloakSession; -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.validators.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 - *

- * During development, it will use the legacy client service to unblock incremental development of this service - */ -public class NewClientService extends DefaultClientService implements ClientService { - 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, - @Nonnull AdminPermissionEvaluator permissions, - // TODO remove the v1 resource once all methods are overridden - @Nonnull RealmAdminResource realmResource) { - super(session, realm, permissions, realmResource); - 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 avoidClientIdPhishing(ClientModel client) throws ServiceException { - if (client == null && !permissions.clients().canList()) { - // we do this to make sure somebody can't phish client IDs - throw new ServiceException(Response.Status.FORBIDDEN); - } - return Optional.ofNullable(client); - } - - @Override - public Optional getClient(@Nonnull RealmModel realm, - @Nonnull String clientId, - ClientProjectionOptions projectionOptions) throws ServiceException { - ClientModel client = avoidClientIdPhishing(realm.getClientByClientId(clientId)).orElseThrow(() -> new ServiceException("Could not find client", Response.Status.NOT_FOUND)); - permissions.clients().requireView(client); - - try { - session.clientPolicy().triggerOnEvent(new AdminClientViewContext(client, permissions.adminAuth())); - return Optional.ofNullable(getMapper(client.getProtocol()).fromModel(client)); - } catch (ClientPolicyException e) { - throw new ServiceException(e.getErrorDetail(), Response.Status.BAD_REQUEST); - } - } - - @Override - public Stream getClients(@Nonnull RealmModel realm, - ClientProjectionOptions projectionOptions, - ClientSearchOptions searchOptions, - ClientSortAndSliceOptions sortAndSliceOptions) { - permissions.clients().requireList(); - - // When FGAP is enabled, authorization filtering is applied at the JPA layer (via PartialEvaluator predicates), so we trust the DB results. - // When disabled, we fall back to in-memory filtering by VIEW_CLIENTS role. - boolean canView = AdminPermissionsSchema.SCHEMA.isAdminPermissionsEnabled(realm) || permissions.clients().canView(); - try { - return realm.getClientsStream() - .filter(client -> canView || permissions.clients().canView(client)) - .filter(client -> client.getProtocol() != null) - .map(client -> getMapper(client.getProtocol()).fromModel(client)) - .filter(java.util.Objects::nonNull); - } catch (ModelException e) { - throw new ServiceException(e.getMessage(), Response.Status.BAD_REQUEST); - } - } - - @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)) - .orElseThrow(() -> new ServiceException("Could not find client", Response.Status.NOT_FOUND)); - - permissions.clients().requireManage(client); - try { - AdminPermissionsSchema.SCHEMA.throwExceptionIfAdminPermissionClient(session, client.getId()); - session.clientPolicy().triggerOnEvent(new AdminClientUnregisterContext(client, permissions.adminAuth())); - } catch (ModelValidationException e) { - throw new ServiceException(e.getMessage(), Response.Status.BAD_REQUEST); - } catch (ClientPolicyException e) { - throw new ServiceException(e.getErrorDetail(), Response.Status.BAD_REQUEST); - } - - var clientRepresentation = Optional.ofNullable(getMapper(client.getProtocol()).fromModel(client)) - .orElseThrow(() -> new ServiceException("Cannot map client model", Response.Status.BAD_REQUEST)); - - if (new ClientManager(new RealmManager(session)).removeClient(realm, client)) { - fireAdminEvent(OperationType.DELETE, clientRepresentation); - } else { - 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); - 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); - }); - - 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); - 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); - } - - /** - * 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. - *

- * For more details, see the keycloak#47576. - */ - 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()); - } - } -} diff --git a/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/cli/v2/KcAdmV2ClientCLITest.java b/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/cli/v2/KcAdmV2ClientCLITest.java index 7b94542ea63d..e2110a8194c0 100644 --- a/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/cli/v2/KcAdmV2ClientCLITest.java +++ b/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/cli/v2/KcAdmV2ClientCLITest.java @@ -102,7 +102,7 @@ void testGetNonExistentClient() { CommandResult result = kcAdmV2Cmd("client", "get", "non-existent-id"); assertThat("get non-existent client should fail", result.exitCode(), is(not(0))); - assertThat(result.err(), containsString("Could not find client")); + assertThat(result.err(), containsString("Cannot find the specified client")); } @Test @@ -470,7 +470,7 @@ void testGetClientWithSpecialCharsInId() { assertThat("should fail for non-existent client", result.exitCode(), is(not(0))); assertThat("URL-encoded ID should reach the server and get a proper 404, not a malformed URL error", - result.err(), containsString("Could not find client")); + result.err(), containsString("Cannot find the specified client")); } @Test @@ -572,7 +572,7 @@ void testDeleteClient() throws Exception { getResult = kcAdmV2Cmd("client", "get", id); assertThat("get after delete should fail", getResult.exitCode(), is(not(0))); - assertThat(getResult.err(), containsString("Could not find client")); + assertThat(getResult.err(), containsString("Cannot find the specified client")); } @Test From 8d722b597d75dff97c5945de48e84c59f11306a6 Mon Sep 17 00:00:00 2001 From: Steven Hawkins Date: Tue, 5 May 2026 07:05:35 -0400 Subject: [PATCH 2/2] Apply suggestion from @shawkins Signed-off-by: Steven Hawkins --- .../org/keycloak/rest/admin/api/client/DefaultClientsApi.java | 1 - 1 file changed, 1 deletion(-) diff --git a/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientsApi.java b/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientsApi.java index fe8180fba474..add0f889af04 100644 --- a/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientsApi.java +++ b/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientsApi.java @@ -59,7 +59,6 @@ public Response createClient(@Valid BaseClientRepresentation client) { /** * When the path {@code clientId} does not resolve, return 403 if the caller cannot list clients * (anti client-ID phishing), matching {@code ClientsResource#getClient} for Admin API v1. - * Not applied to PUT upsert so {@code manage} without {@code list} can create by clientId. */ private void enforceAntiPhishingIfClientMissing(String clientId) { if (realm.getClientByClientId(clientId) == null && !permissions.clients().canList()) {