KEYCLOAK-16633 Prevent deletion of internal clients.

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 <a href="mailto:bill@burkecentral.com">Bill Burke</a>
  * @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;
+    }
 }

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);
+        if (new ClientManager(new RealmManager(session)).removeClient(realm, client)) {
-        adminEvent.operation(OperationType.DELETE).resourcePath(session.getContext().getUri()).success();
+            adminEvent.operation(OperationType.DELETE).resourcePath(session.getContext().getUri()).success();
+        }
+        else {
+            throw new ErrorResponseException(OAuthErrorException.INVALID_REQUEST, "Could not delete client",
+                    Response.Status.BAD_REQUEST);
+        }
     }
 
 

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 javax.ws.rs.BadRequestException;
-import static org.hamcrest.CoreMatchers.is;
+import javax.ws.rs.NotFoundException;
-import static org.hamcrest.Matchers.hasItem;
+import javax.ws.rs.core.Response;
-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;
 
 /**
  * @author <a href="mailto:sthorger@redhat.com">Stian Thorgersen</a>
@@ -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();