diff --git a/services/src/main/java/org/keycloak/services/managers/ClientManager.java b/services/src/main/java/org/keycloak/services/managers/ClientManager.java index 524b31e258..0f8b16e0e7 100644 --- a/services/src/main/java/org/keycloak/services/managers/ClientManager.java +++ b/services/src/main/java/org/keycloak/services/managers/ClientManager.java @@ -52,6 +52,8 @@ import java.util.Set; import java.util.TreeSet; import java.util.stream.Collectors; +import static org.keycloak.models.Constants.defaultClients; + /** * @author Bill Burke * @version $Revision: 1 $ @@ -74,7 +76,6 @@ public class ClientManager { * @param session * @param realm * @param rep - * @param addDefaultRoles * @return */ public static ClientModel createClient(KeycloakSession session, RealmModel realm, ClientRepresentation rep) { @@ -96,7 +97,7 @@ public class ClientManager { public boolean removeClient(RealmModel realm, ClientModel client) { - if (realm.removeClient(client.getId())) { + if (!isInternalClient(realm.getName(), client.getClientId()) && realm.removeClient(client.getId())) { UserSessionProvider sessions = realmManager.getSession().sessions(); if (sessions != null) { sessions.onClientRemoved(realm, client); @@ -366,4 +367,21 @@ public class ClientManager { return authenticator.getAdapterConfiguration(client); } + private boolean isInternalClient(String realmName, String clientId) { + if (defaultClients.contains(clientId)) return true; + + if (!"master".equals(realmName)) { + return false; + } + + final String internalClientSuffix = "-realm"; + + if (clientId.endsWith(internalClientSuffix)) { + return realmManager.getSession().realms() + .getRealmByName( + clientId.substring(0, clientId.length() - internalClientSuffix.length())) != null; + } + + return false; + } } diff --git a/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java b/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java index 429ae44210..ffe1d593dc 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java @@ -20,6 +20,7 @@ import org.jboss.logging.Logger; import org.jboss.resteasy.annotations.cache.NoCache; import org.jboss.resteasy.spi.BadRequestException; import org.jboss.resteasy.spi.ResteasyProviderFactory; +import org.keycloak.OAuthErrorException; import org.keycloak.authorization.admin.AuthorizationService; import org.keycloak.common.ClientConnection; import org.keycloak.common.Profile; @@ -223,8 +224,13 @@ public class ClientResource { throw new ErrorResponseException(cpe.getError(), cpe.getErrorDetail(), Response.Status.BAD_REQUEST); } - new ClientManager(new RealmManager(session)).removeClient(realm, client); - adminEvent.operation(OperationType.DELETE).resourcePath(session.getContext().getUri()).success(); + if (new ClientManager(new RealmManager(session)).removeClient(realm, client)) { + adminEvent.operation(OperationType.DELETE).resourcePath(session.getContext().getUri()).success(); + } + else { + throw new ErrorResponseException(OAuthErrorException.INVALID_REQUEST, "Could not delete client", + Response.Status.BAD_REQUEST); + } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ClientTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ClientTest.java index e2702fa88f..df7d9f2aaa 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ClientTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ClientTest.java @@ -17,6 +17,21 @@ package org.keycloak.testsuite.admin; +import static java.util.Arrays.asList; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.keycloak.models.Constants.defaultClients; + import org.junit.Test; import org.keycloak.OAuth2Constants; import org.keycloak.admin.client.resource.ClientResource; @@ -41,6 +56,8 @@ import org.keycloak.representations.idm.RoleRepresentation; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.representations.idm.UserSessionRepresentation; import org.keycloak.testsuite.Assert; +import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude; +import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.AuthServer; import org.keycloak.testsuite.util.AdminEventPaths; import org.keycloak.testsuite.util.ClientBuilder; import org.keycloak.testsuite.util.CredentialBuilder; @@ -49,9 +66,6 @@ import org.keycloak.testsuite.util.OAuthClient.AccessTokenResponse; import org.keycloak.testsuite.util.RoleBuilder; import org.keycloak.testsuite.util.UserBuilder; -import javax.ws.rs.BadRequestException; -import javax.ws.rs.NotFoundException; -import javax.ws.rs.core.Response; import java.io.IOException; import java.util.Arrays; import java.util.Collections; @@ -63,14 +77,9 @@ import java.util.Objects; import java.util.Set; import java.util.stream.Collectors; -import static java.util.Arrays.asList; -import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.Matchers.hasItem; -import static org.hamcrest.Matchers.not; -import static org.hamcrest.Matchers.containsInAnyOrder; -import static org.junit.Assert.*; -import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude; -import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.AuthServer; +import javax.ws.rs.BadRequestException; +import javax.ws.rs.NotFoundException; +import javax.ws.rs.core.Response; /** * @author Stian Thorgersen @@ -220,15 +229,6 @@ public class ClientTest extends AbstractAdminTest { assertEquals("invalid_input", errorRep.getError()); } - private void updateClientExpectingSuccessfulClientUpdate(ClientRepresentation rep, String expectedRootUrl, String expectedBaseUrl) { - - realm.clients().get(rep.getId()).update(rep); - - ClientRepresentation stored = realm.clients().get(rep.getId()).toRepresentation(); - assertEquals(expectedRootUrl, stored.getRootUrl()); - assertEquals(expectedBaseUrl, stored.getBaseUrl()); - } - @Test public void removeClient() { String id = createClient().getId(); @@ -239,6 +239,23 @@ public class ClientTest extends AbstractAdminTest { assertAdminEvents.assertEvent(realmId, OperationType.DELETE, AdminEventPaths.clientResourcePath(id), ResourceType.CLIENT); } + @Test + public void removeInternalClientExpectingBadRequestException() { + final String testRealmClientId = ApiUtil.findClientByClientId(realmsResouce().realm("master"), "test-realm") + .toRepresentation().getId(); + + assertThrows(BadRequestException.class, + () -> realmsResouce().realm("master").clients().get(testRealmClientId).remove()); + + defaultClients.forEach(defaultClient -> { + final String defaultClientId = ApiUtil.findClientByClientId(realm, defaultClient) + .toRepresentation().getId(); + + assertThrows(BadRequestException.class, + () -> realm.clients().get(defaultClientId).remove()); + }); + } + @Test public void getClientRepresentation() { String id = createClient().getId();