[KEYCLOAK-16521] - Fixing secret for non-confidential clients

server-spi-private/src/main/java/org/keycloak/migration/migrators/MigrateTo1_2_0.java

@@ -41,7 +41,7 @@ public class MigrateTo1_2_0 implements Migration {
     public void setupBrokerService(RealmModel realm) {
         ClientModel client = realm.getClientByClientId(Constants.BROKER_SERVICE_CLIENT_ID);
         if (client == null) {
-            client = KeycloakModelUtils.createClient(realm, Constants.BROKER_SERVICE_CLIENT_ID);
+            client = KeycloakModelUtils.createManagementClient(realm, Constants.BROKER_SERVICE_CLIENT_ID);
             client.setEnabled(true);
             client.setName("${client_" + Constants.BROKER_SERVICE_CLIENT_ID + "}");
             client.setFullScopeAllowed(false);

server-spi-private/src/main/java/org/keycloak/migration/migrators/MigrateTo9_0_0.java

@@ -79,11 +79,10 @@ public class MigrateTo9_0_0 implements Migration {
 
     protected void addAccountConsoleClient(RealmModel realm) {
         if (realm.getClientByClientId(Constants.ACCOUNT_CONSOLE_CLIENT_ID) == null) {
-            ClientModel client = KeycloakModelUtils.createClient(realm, Constants.ACCOUNT_CONSOLE_CLIENT_ID);
+            ClientModel client = KeycloakModelUtils.createPublicClient(realm, Constants.ACCOUNT_CONSOLE_CLIENT_ID);
             client.setName("${client_" + Constants.ACCOUNT_CONSOLE_CLIENT_ID + "}");
             client.setEnabled(true);
             client.setFullScopeAllowed(false);
-            client.setPublicClient(true);
             client.setDirectAccessGrantsEnabled(false);
 
             client.setRootUrl(Constants.AUTH_BASE_URL_PROP);

server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java

@@ -164,13 +164,28 @@ public final class KeycloakModelUtils {
         return UUID.randomUUID().toString();
     }
 
-    public static ClientModel createClient(RealmModel realm, String name) {
+    public static ClientModel createManagementClient(RealmModel realm, String name) {
-        ClientModel app = realm.addClient(name);
+        ClientModel client = createClient(realm, name);
-        app.setClientAuthenticatorType(getDefaultClientAuthenticatorType());
-        generateSecret(app);
-        app.setFullScopeAllowed(true);
 
-        return app;
+        client.setBearerOnly(true);
+
+        return client;
+    }
+
+    public static ClientModel createPublicClient(RealmModel realm, String name) {
+        ClientModel client = createClient(realm, name);
+
+        client.setPublicClient(true);
+
+        return client;
+    }
+
+    private static ClientModel createClient(RealmModel realm, String name) {
+        ClientModel client = realm.addClient(name);
+
+        client.setClientAuthenticatorType(getDefaultClientAuthenticatorType());
+
+        return client;
     }
 
     /**

server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java

@@ -1368,9 +1368,6 @@ public class RepresentationToModel {
         }
 
         client.setSecret(resourceRep.getSecret());
-        if (client.getSecret() == null) {
-            KeycloakModelUtils.generateSecret(client);
-        }
 
         if (resourceRep.getAttributes() != null) {
             for (Map.Entry<String, String> entry : resourceRep.getAttributes().entrySet()) {
@@ -1564,7 +1561,18 @@ public class RepresentationToModel {
             }
         }
 
-        if (rep.getSecret() != null) resource.setSecret(rep.getSecret());
+        if (resource.isPublicClient() || resource.isBearerOnly()) {
+            resource.setSecret(null);
+        } else {
+            String currentSecret = resource.getSecret();
+            String newSecret = rep.getSecret();
+
+            if (newSecret == null && currentSecret == null) {
+                KeycloakModelUtils.generateSecret(resource);
+            } else if (newSecret != null) {
+                resource.setSecret(newSecret);
+            }
+        }
 
         resource.updateClient();
     }

services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocolFactory.java

@@ -367,9 +367,16 @@ public class OIDCLoginProtocolFactory extends AbstractLoginProtocolFactory {
             origins.add(origin);
             newClient.setWebOrigins(origins);
         }
+        // if no client type provided, default to public client
         if (rep.isBearerOnly() == null
                 && rep.isPublicClient() == null) {
             newClient.setPublicClient(true);
+            newClient.setSecret(null);
+        } else if (!(Boolean.TRUE.equals(rep.isBearerOnly()) || Boolean.TRUE.equals(rep.isPublicClient()))) {
+            // if client is confidential, generate a secret if none is defined
+            if (newClient.getSecret() == null) {
+                KeycloakModelUtils.generateSecret(newClient);
+            }
         }
         if (rep.isBearerOnly() == null) newClient.setBearerOnly(false);
         if (rep.getAdminUrl() == null && rep.getRootUrl() != null) {

services/src/main/java/org/keycloak/services/clientregistration/AbstractClientRegistrationProvider.java

@@ -84,6 +84,8 @@ public abstract class AbstractClientRegistrationProvider implements ClientRegist
                 session.realms().decreaseRemainingCount(realm, initialAccessModel);
             }
 
+            client.setDirectAccessGrantsEnabled(false);
+
             event.client(client.getClientId()).success();
             return client;
         } catch (ModelDuplicateException e) {
@@ -97,7 +99,7 @@ public abstract class AbstractClientRegistrationProvider implements ClientRegist
         auth.requireView(client);
 
         ClientRepresentation rep = ModelToRepresentation.toRepresentation(client, session);
-        if (client.getSecret() != null) {
+        if (!(Boolean.TRUE.equals(rep.isBearerOnly()) || Boolean.TRUE.equals(rep.isPublicClient()))) {
             rep.setSecret(client.getSecret());
         }
 

services/src/main/java/org/keycloak/services/clientregistration/AdapterInstallationClientRegistrationProvider.java

@@ -51,7 +51,7 @@ public class AdapterInstallationClientRegistrationProvider implements ClientRegi
         event.event(EventType.CLIENT_INFO);
 
         ClientModel client = session.getContext().getRealm().getClientByClientId(clientId);
-        auth.requireView(client);
+        auth.requireView(client, true);
 
         ClientManager clientManager = new ClientManager(new RealmManager(session));
         Object rep = clientManager.toInstallationRepresentation(session.getContext().getRealm(), client, session.getContext().getUri().getBaseUri());

services/src/main/java/org/keycloak/services/clientregistration/ClientRegistrationAuth.java

@@ -162,6 +162,10 @@ public class ClientRegistrationAuth {
     }
 
     public void requireView(ClientModel client) {
+        requireView(client, false);
+    }
+
+    public void requireView(ClientModel client, boolean allowPublicClient) {
         RegistrationAuth authType = null;
         boolean authenticated = false;
 
@@ -182,16 +186,15 @@ public class ClientRegistrationAuth {
             }
         } else if (isRegistrationAccessToken()) {
             if (client != null && client.getRegistrationToken() != null && client.getRegistrationToken().equals(jwt.getId())) {
+                checkClientProtocol(client);
                 authenticated = true;
                 authType = getRegistrationAuth();
             }
         } else if (isInitialAccessToken()) {
             throw unauthorized("Not initial access token allowed");
-        } else {
+        } else if (allowPublicClient && authenticatePublicClient(client)) {
-            if (authenticateClient(client)) {
+            authenticated = true;
-                authenticated = true;
+            authType = RegistrationAuth.AUTHENTICATED;
-                authType = RegistrationAuth.AUTHENTICATED;
-            }
         }
 
         if (authenticated) {
@@ -341,7 +344,7 @@ public class ClientRegistrationAuth {
         return false;
     }
 
-    private boolean authenticateClient(ClientModel client) {
+    private boolean authenticatePublicClient(ClientModel client) {
         if (client == null) {
             return false;
         }

services/src/main/java/org/keycloak/services/clientregistration/oidc/DescriptionConverter.java

@@ -64,6 +64,7 @@ public class DescriptionConverter {
         client.setName(clientOIDC.getClientName());
         client.setRedirectUris(clientOIDC.getRedirectUris());
         client.setBaseUrl(clientOIDC.getClientUri());
+        client.setProtocol(OIDCLoginProtocol.LOGIN_PROTOCOL);
 
         String scopeParam = clientOIDC.getScope();
         if (scopeParam != null) client.setOptionalClientScopes(new ArrayList<>(Arrays.asList(scopeParam.split(" "))));

services/src/main/java/org/keycloak/services/managers/RealmManager.java

@@ -163,7 +163,7 @@ public class RealmManager {
 
     protected void setupAdminConsole(RealmModel realm) {
         ClientModel adminConsole = realm.getClientByClientId(Constants.ADMIN_CONSOLE_CLIENT_ID);
-        if (adminConsole == null) adminConsole = KeycloakModelUtils.createClient(realm, Constants.ADMIN_CONSOLE_CLIENT_ID);
+        if (adminConsole == null) adminConsole = KeycloakModelUtils.createPublicClient(realm, Constants.ADMIN_CONSOLE_CLIENT_ID);
         adminConsole.setName("${client_" + Constants.ADMIN_CONSOLE_CLIENT_ID + "}");
 
         adminConsole.setRootUrl(Constants.AUTH_ADMIN_URL_PROP);
@@ -196,11 +196,10 @@ public class RealmManager {
     public void setupAdminCli(RealmModel realm) {
         ClientModel adminCli = realm.getClientByClientId(Constants.ADMIN_CLI_CLIENT_ID);
         if (adminCli == null) {
-            adminCli = KeycloakModelUtils.createClient(realm, Constants.ADMIN_CLI_CLIENT_ID);
+            adminCli = KeycloakModelUtils.createPublicClient(realm, Constants.ADMIN_CLI_CLIENT_ID);
             adminCli.setName("${client_" + Constants.ADMIN_CLI_CLIENT_ID + "}");
             adminCli.setEnabled(true);
             adminCli.setAlwaysDisplayInConsole(false);
-            adminCli.setPublicClient(true);
             adminCli.setFullScopeAllowed(false);
             adminCli.setStandardFlowEnabled(false);
             adminCli.setDirectAccessGrantsEnabled(true);
@@ -326,10 +325,9 @@ public class RealmManager {
         }
         adminRole.setDescription("${role_"+AdminRoles.ADMIN+"}");
 
-        ClientModel realmAdminApp = KeycloakModelUtils.createClient(adminRealm, KeycloakModelUtils.getMasterRealmAdminApplicationClientId(realm.getName()));
+        ClientModel realmAdminApp = KeycloakModelUtils.createManagementClient(adminRealm, KeycloakModelUtils.getMasterRealmAdminApplicationClientId(realm.getName()));
         // No localized name for now
         realmAdminApp.setName(realm.getName() + " Realm");
-        realmAdminApp.setBearerOnly(true);
         realm.setMasterAdminClient(realmAdminApp);
 
         for (String r : AdminRoles.ALL_REALM_ROLES) {
@@ -361,7 +359,7 @@ public class RealmManager {
         String realmAdminClientId = getRealmAdminClientId(realm);
         ClientModel realmAdminClient = realm.getClientByClientId(realmAdminClientId);
         if (realmAdminClient == null) {
-            realmAdminClient = KeycloakModelUtils.createClient(realm, realmAdminClientId);
+            realmAdminClient = KeycloakModelUtils.createManagementClient(realm, realmAdminClientId);
             realmAdminClient.setName("${client_" + realmAdminClientId + "}");
         }
         RoleModel adminRole = realmAdminClient.addRole(AdminRoles.REALM_ADMIN);
@@ -409,7 +407,7 @@ public class RealmManager {
     private void setupAccountManagement(RealmModel realm) {
         ClientModel accountClient = realm.getClientByClientId(Constants.ACCOUNT_MANAGEMENT_CLIENT_ID);
         if (accountClient == null) {
-            accountClient = KeycloakModelUtils.createClient(realm, Constants.ACCOUNT_MANAGEMENT_CLIENT_ID);
+            accountClient = KeycloakModelUtils.createPublicClient(realm, Constants.ACCOUNT_MANAGEMENT_CLIENT_ID);
             accountClient.setName("${client_" + Constants.ACCOUNT_MANAGEMENT_CLIENT_ID + "}");
             accountClient.setEnabled(true);
             accountClient.setAlwaysDisplayInConsole(false);
@@ -443,12 +441,11 @@ public class RealmManager {
 
             ClientModel accountConsoleClient = realm.getClientByClientId(Constants.ACCOUNT_CONSOLE_CLIENT_ID);
             if (accountConsoleClient == null) {
-                accountConsoleClient = KeycloakModelUtils.createClient(realm, Constants.ACCOUNT_CONSOLE_CLIENT_ID);
+                accountConsoleClient = KeycloakModelUtils.createPublicClient(realm, Constants.ACCOUNT_CONSOLE_CLIENT_ID);
                 accountConsoleClient.setName("${client_" + Constants.ACCOUNT_CONSOLE_CLIENT_ID + "}");
                 accountConsoleClient.setEnabled(true);
                 accountConsoleClient.setAlwaysDisplayInConsole(false);
                 accountConsoleClient.setFullScopeAllowed(false);
-                accountConsoleClient.setPublicClient(true);
                 accountConsoleClient.setDirectAccessGrantsEnabled(false);
 
                 accountConsoleClient.setRootUrl(Constants.AUTH_BASE_URL_PROP);
@@ -478,7 +475,7 @@ public class RealmManager {
     public void setupBrokerService(RealmModel realm) {
         ClientModel client = realm.getClientByClientId(Constants.BROKER_SERVICE_CLIENT_ID);
         if (client == null) {
-            client = KeycloakModelUtils.createClient(realm, Constants.BROKER_SERVICE_CLIENT_ID);
+            client = KeycloakModelUtils.createManagementClient(realm, Constants.BROKER_SERVICE_CLIENT_ID);
             client.setEnabled(true);
             client.setAlwaysDisplayInConsole(false);
             client.setName("${client_" + Constants.BROKER_SERVICE_CLIENT_ID + "}");

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountFormServiceTest.java

@@ -1302,7 +1302,7 @@ public class AccountFormServiceTest extends AbstractTestRealmKeycloakTest {
         applicationsPage.assertCurrent();
 
         Map<String, AccountApplicationsPage.AppEntry> apps = applicationsPage.getApplications();
-        Assert.assertThat(apps.keySet(), containsInAnyOrder("root-url-client", "Account", "Account Console", "Broker", "test-app", "test-app-scope", "third-party", "test-app-authz", "My Named Test App", "Test App Named - ${client_account}", "direct-grant", "custom-audience"));
+        Assert.assertThat(apps.keySet(), containsInAnyOrder("root-url-client", "Account", "Account Console", "test-app", "test-app-scope", "third-party", "test-app-authz", "My Named Test App", "Test App Named - ${client_account}", "direct-grant", "custom-audience"));
 
         AccountApplicationsPage.AppEntry accountEntry = apps.get("Account");
         Assert.assertThat(accountEntry.getRolesAvailable(), containsInAnyOrder(

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ClientTest.java

@@ -106,7 +106,9 @@ public class ClientTest extends AbstractAdminTest {
     public void createClientVerify() {
         String id = createClient().getId();
 
-        assertNotNull(realm.clients().get(id));
+        ClientResource client = realm.clients().get(id);
+        assertNotNull(client);
+        assertNull(client.toRepresentation().getSecret());
         Assert.assertNames(realm.clients().findAll(), "account", "account-console", "realm-management", "security-admin-console", "broker", "my-app", Constants.ADMIN_CLI_CLIENT_ID);
     }
 

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/partialexport/PartialExportTest.java

@@ -141,7 +141,9 @@ public class PartialExportTest extends AbstractAdminTest {
 
         // Client secret
         for (ClientRepresentation client: rep.getClients()) {
-            Assert.assertEquals("Client secret masked", ComponentRepresentation.SECRET_VALUE, client.getSecret());
+            if (Boolean.FALSE.equals(client.isPublicClient()) && Boolean.FALSE.equals(client.isBearerOnly())) {
+                Assert.assertEquals("Client secret masked", ComponentRepresentation.SECRET_VALUE, client.getSecret());
+            }
         }
 
         // IdentityProvider clientSecret

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/OIDCClientRegistrationTest.java

@@ -260,6 +260,7 @@ public class OIDCClientRegistrationTest extends AbstractClientRegistrationTest {
         String clientId = response.getClientId();
         ClientRepresentation kcClientRep = getKeycloakClient(clientId);
         Assert.assertTrue(kcClientRep.isPublicClient());
+        Assert.assertNull(kcClientRep.getSecret());
 
         // Update client to hybrid and check it's not public client anymore
         reg.auth(Auth.token(response));
@@ -409,21 +410,30 @@ public class OIDCClientRegistrationTest extends AbstractClientRegistrationTest {
     }
 
     @Test
-    public void testOIDCEndpointGetWithSamlClient() {
+    public void testOIDCEndpointGetWithSamlClient() throws Exception {
+        OIDCClientRepresentation response = create();
+        reg.auth(Auth.token(response));
+        assertNotNull(reg.oidc().get(response.getClientId()));
         ClientsResource clientsResource = adminClient.realm(TEST).clients();
-        ClientRepresentation samlClient = clientsResource.findByClientId("saml-client").get(0);
+        ClientRepresentation client = clientsResource.findByClientId(response.getClientId()).get(0);
-
-        reg.auth(Auth.client("saml-client", "secret"));
 
         // change client to saml
-        samlClient.setProtocol("saml");
+        client.setProtocol("saml");
-        clientsResource.get(samlClient.getId()).update(samlClient);
+        clientsResource.get(client.getId()).update(client);
 
-        assertGetFail(samlClient.getClientId(), 400, Errors.INVALID_CLIENT);
+        assertGetFail(client.getClientId(), 400, Errors.INVALID_CLIENT);
+    }
 
-        // revert client
+    @Test
-        samlClient.setProtocol("openid-connect");
+    public void testOIDCEndpointGetWithToken() throws Exception {
-        clientsResource.get(samlClient.getId()).update(samlClient);
+        OIDCClientRepresentation response = create();
+        reg.auth(Auth.token(response));
+        assertNotNull(reg.oidc().get(response.getClientId()));
+    }
+
+    @Test
+    public void testOIDCEndpointGetWithoutToken() throws Exception {
+        assertGetFail(create().getClientId(), 401, null);
     }
 
     @Test

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPMultipleAttributesTest.java

@@ -90,7 +90,7 @@ public class LDAPMultipleAttributesTest extends AbstractLDAPTest {
             LDAPTestUtils.updateLDAPPassword(ldapFedProvider, bruce, "Password1");
 
             // Create ldap-portal client
-            ClientModel ldapClient = KeycloakModelUtils.createClient(appRealm, "ldap-portal");
+            ClientModel ldapClient = appRealm.addClient("ldap-portal");
             ldapClient.setProtocol(OIDCLoginProtocol.LOGIN_PROTOCOL);
             ldapClient.addRedirectUri("/ldap-portal");
             ldapClient.addRedirectUri("/ldap-portal/*");

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java

@@ -578,13 +578,16 @@ public class AccessTokenTest extends AbstractKeycloakTest {
 
 
             Response response = executeGrantAccessTokenRequest(grantTarget);
-            assertEquals(400, response.getStatus());
+            // 401 because the client is now a bearer without a secret
+            assertEquals(401, response.getStatus());
             response.close();
 
             {
                 ClientResource clientResource = findClientByClientId(adminClient.realm("test"), "test-app");
                 ClientRepresentation clientRepresentation = clientResource.toRepresentation();
                 clientRepresentation.setBearerOnly(false);
+                // reset to the old secret
+                clientRepresentation.setSecret("password");
                 clientResource.update(clientRepresentation);
             }
 

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/openshift/OpenShiftTokenReviewEndpointTest.java

@@ -317,6 +317,7 @@ public class OpenShiftTokenReviewEndpointTest extends AbstractTestRealmKeycloakT
             new Review().invoke().assertError(401, "Public client is not permitted to invoke token review endpoint");
         } finally {
             clientRep.setPublicClient(false);
+            clientRep.setSecret("password");
             testRealm().clients().get(clientRep.getId()).update(clientRep);
         }
     }