From 5a66f577ebb65eb3d9274dc2f5ef69169c7dc52a Mon Sep 17 00:00:00 2001 From: mposolda Date: Thu, 14 Dec 2017 17:46:48 +0100 Subject: [PATCH] KEYCLOAK-5982 Fix NPEs when client 'account' was renamed/removed --- .../models/utils/SystemClientUtil.java | 60 +++++++++++++++++++ .../actiontoken/ActionTokenContext.java | 3 +- .../managers/AuthenticationManager.java | 3 +- .../resources/LoginActionsService.java | 3 +- .../updaters/ClientAttributeUpdater.java | 5 ++ .../keycloak/testsuite/forms/LogoutTest.java | 22 +++++++ .../testsuite/forms/ResetPasswordTest.java | 32 +++++++++- 7 files changed, 124 insertions(+), 4 deletions(-) create mode 100644 server-spi-private/src/main/java/org/keycloak/models/utils/SystemClientUtil.java diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/SystemClientUtil.java b/server-spi-private/src/main/java/org/keycloak/models/utils/SystemClientUtil.java new file mode 100644 index 0000000000..0decf93891 --- /dev/null +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/SystemClientUtil.java @@ -0,0 +1,60 @@ +/* + * Copyright 2017 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.keycloak.models.utils; + +import org.jboss.logging.Logger; +import org.keycloak.models.ClientModel; +import org.keycloak.models.Constants; +import org.keycloak.models.RealmModel; + +/** + * @author Marek Posolda + */ +public class SystemClientUtil { + + public static final String SYSTEM_CLIENT_ID = "_system"; + + private static final Logger logger = Logger.getLogger(SystemClientUtil.class); + + + /** + * @return system client used during usecases when some "metaclient" is needed (EG. For fresh authenticationSession used during actionTokenFlow when email link is opened in new browser) + */ + public static ClientModel getSystemClient(RealmModel realm) { + // Try to return builtin "account" client first + ClientModel client = realm.getClientByClientId(Constants.ACCOUNT_MANAGEMENT_CLIENT_ID); + if (client != null) { + return client; + } + + + // Fallback to "system" client + client = realm.getClientByClientId(SYSTEM_CLIENT_ID); + if (client != null) { + return client; + } else { + // Return system client + logger.warnf("Client '%s' not available. Creating system client '%s' for system operations", Constants.ACCOUNT_MANAGEMENT_CLIENT_ID, SYSTEM_CLIENT_ID); + client = realm.addClient(SYSTEM_CLIENT_ID); + client.setName(SYSTEM_CLIENT_ID); + return client; + } + + } + +} diff --git a/services/src/main/java/org/keycloak/authentication/actiontoken/ActionTokenContext.java b/services/src/main/java/org/keycloak/authentication/actiontoken/ActionTokenContext.java index 282a3bed3a..cb2ce3b085 100644 --- a/services/src/main/java/org/keycloak/authentication/actiontoken/ActionTokenContext.java +++ b/services/src/main/java/org/keycloak/authentication/actiontoken/ActionTokenContext.java @@ -21,6 +21,7 @@ import org.keycloak.authentication.AuthenticationProcessor; import org.keycloak.common.ClientConnection; import org.keycloak.events.EventBuilder; import org.keycloak.models.*; +import org.keycloak.models.utils.SystemClientUtil; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.representations.JsonWebToken; import org.keycloak.services.Urls; @@ -110,7 +111,7 @@ public class ActionTokenContext { AuthenticationSessionModel authSession; // set up the account service as the endpoint to call. - ClientModel client = realm.getClientByClientId(clientId == null ? Constants.ACCOUNT_MANAGEMENT_CLIENT_ID : clientId); + ClientModel client = clientId != null ? realm.getClientByClientId(clientId) : SystemClientUtil.getSystemClient(realm); RootAuthenticationSessionModel rootAuthSession = new AuthenticationSessionManager(session).createAuthenticationSession(realm, true); authSession = rootAuthSession.createAuthenticationSession(client); diff --git a/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java b/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java index 55eb9c717d..678bbbffd6 100755 --- a/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java +++ b/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java @@ -42,6 +42,7 @@ import org.keycloak.jose.jws.JWSBuilder; import org.keycloak.models.*; import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.models.utils.SessionTimeoutHelper; +import org.keycloak.models.utils.SystemClientUtil; import org.keycloak.protocol.LoginProtocol; import org.keycloak.protocol.LoginProtocol.Error; import org.keycloak.protocol.oidc.TokenManager; @@ -218,7 +219,7 @@ public class AuthenticationManager { private static AuthenticationSessionModel createOrJoinLogoutSession(KeycloakSession session, RealmModel realm, final AuthenticationSessionManager asm, UserSessionModel userSession, boolean browserCookie) { // Account management client is used as a placeholder - ClientModel client = realm.getClientByClientId(Constants.ACCOUNT_MANAGEMENT_CLIENT_ID); + ClientModel client = SystemClientUtil.getSystemClient(realm); // Try to lookup current authSessionId from browser cookie. If doesn't exists, use the same as current userSession String authSessionId = null; diff --git a/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java b/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java index 87711c7766..f09cfbb2a4 100755 --- a/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java +++ b/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java @@ -54,6 +54,7 @@ import org.keycloak.models.RoleModel; import org.keycloak.models.UserConsentModel; import org.keycloak.models.UserModel; import org.keycloak.models.utils.FormMessage; +import org.keycloak.models.utils.SystemClientUtil; import org.keycloak.protocol.AuthorizationEndpointBase; import org.keycloak.protocol.LoginProtocol; import org.keycloak.protocol.LoginProtocol.Error; @@ -364,7 +365,7 @@ public class LoginActionsService { AuthenticationSessionModel authSession; // set up the account service as the endpoint to call. - ClientModel client = realm.getClientByClientId(Constants.ACCOUNT_MANAGEMENT_CLIENT_ID); + ClientModel client = SystemClientUtil.getSystemClient(realm); RootAuthenticationSessionModel rootAuthSession = new AuthenticationSessionManager(session).createAuthenticationSession(realm, true); authSession = rootAuthSession.createAuthenticationSession(client); diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java index 7b82f9b9a7..fec19a8b99 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java @@ -25,6 +25,11 @@ public class ClientAttributeUpdater { } } + public ClientAttributeUpdater setClientId(String clientId) { + this.rep.setClientId(clientId); + return this; + } + public ClientAttributeUpdater setAttribute(String name, String value) { this.rep.getAttributes().put(name, value); return this; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/LogoutTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/LogoutTest.java index 790475b9a8..54e0714850 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/LogoutTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/LogoutTest.java @@ -19,7 +19,10 @@ package org.keycloak.testsuite.forms; import org.jboss.arquillian.graphene.page.Page; import org.junit.Rule; import org.junit.Test; +import org.keycloak.admin.client.resource.ClientResource; import org.keycloak.events.Details; +import org.keycloak.models.Constants; +import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.testsuite.Assert; @@ -30,6 +33,7 @@ import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.pages.AppPage; import org.keycloak.testsuite.pages.LoginPage; +import java.io.Closeable; import java.io.IOException; import static org.junit.Assert.assertEquals; @@ -37,6 +41,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertTrue; import org.keycloak.testsuite.auth.page.account.AccountManagement; +import org.keycloak.testsuite.updaters.ClientAttributeUpdater; /** * @author Stian Thorgersen @@ -235,4 +240,21 @@ public class LogoutTest extends AbstractTestRealmKeycloakTest { }, 10, 200); } + + // KEYCLOAK-5982 + @Test + public void testLogoutWhenAccountClientRenamed() throws IOException { + // Rename client "account" + ClientResource accountClient = ApiUtil.findClientByClientId(adminClient.realm("test"), Constants.ACCOUNT_MANAGEMENT_CLIENT_ID); + + // Temporarily rename client "account" . Revert it back after the test + try (Closeable accountClientUpdater = new ClientAttributeUpdater(accountClient) + .setClientId("account-changed") + .update()) { + + // Assert logout works + logoutRedirect(); + } + } + } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java index e7b0470383..633494bb64 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java @@ -16,11 +16,15 @@ */ package org.keycloak.testsuite.forms; +import org.keycloak.admin.client.resource.ClientResource; import org.keycloak.authentication.actiontoken.resetcred.ResetCredentialsActionToken; import org.jboss.arquillian.graphene.page.Page; import org.keycloak.events.Details; import org.keycloak.events.Errors; import org.keycloak.events.EventType; +import org.keycloak.models.Constants; +import org.keycloak.models.utils.SystemClientUtil; +import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.EventRepresentation; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.UserRepresentation; @@ -35,6 +39,7 @@ import org.keycloak.testsuite.pages.LoginPage; import org.keycloak.testsuite.pages.LoginPasswordResetPage; import org.keycloak.testsuite.pages.LoginPasswordUpdatePage; import org.keycloak.testsuite.pages.VerifyEmailPage; +import org.keycloak.testsuite.updaters.ClientAttributeUpdater; import org.keycloak.testsuite.util.GreenMailRule; import org.keycloak.testsuite.util.MailUtils; import org.keycloak.testsuite.util.OAuthClient; @@ -43,6 +48,8 @@ import org.keycloak.testsuite.util.UserBuilder; import javax.mail.MessagingException; import javax.mail.internet.MimeMessage; + +import java.io.Closeable; import java.io.IOException; import java.util.Collections; import java.util.HashMap; @@ -941,6 +948,11 @@ public class ResetPasswordTest extends AbstractTestRealmKeycloakTest { @Test public void resetPasswordLinkOpenedInNewBrowser() throws IOException, MessagingException { + resetPasswordLinkOpenedInNewBrowser(Constants.ACCOUNT_MANAGEMENT_CLIENT_ID); + } + + + private void resetPasswordLinkOpenedInNewBrowser(String expectedSystemClientId) throws IOException, MessagingException { String username = "login-test"; String resetUri = oauth.AUTH_SERVER_ROOT + "/realms/test/login-actions/reset-credentials"; driver.navigate().to(resetUri); @@ -956,7 +968,7 @@ public class ResetPasswordTest extends AbstractTestRealmKeycloakTest { events.expectRequiredAction(EventType.SEND_RESET_PASSWORD) .user(userId) .detail(Details.REDIRECT_URI, oauth.AUTH_SERVER_ROOT + "/realms/test/account/") - .client("account") + .client(expectedSystemClientId) .detail(Details.USERNAME, username) .detail(Details.EMAIL, "login@test.com") .session((String)null) @@ -982,4 +994,22 @@ public class ResetPasswordTest extends AbstractTestRealmKeycloakTest { infoPage.assertCurrent(); assertEquals("Your account has been updated.", infoPage.getInfo()); } + + + // KEYCLOAK-5982 + @Test + public void resetPasswordLinkOpenedInNewBrowserAndAccountClientRenamed() throws IOException, MessagingException { + ClientResource accountClient = ApiUtil.findClientByClientId(adminClient.realm("test"), Constants.ACCOUNT_MANAGEMENT_CLIENT_ID); + + // Temporarily rename client "account" . Revert it back after the test + try (Closeable accountClientUpdater = new ClientAttributeUpdater(accountClient) + .setClientId("account-changed") + .update()) { + + // Assert resetPassword link opened in new browser works even if client "account" not available + resetPasswordLinkOpenedInNewBrowser(SystemClientUtil.SYSTEM_CLIENT_ID); + + } + } + }