From 3d91df42d8025f7ee967cbc9cdbcc1d6d6d02507 Mon Sep 17 00:00:00 2001 From: vramik Date: Thu, 10 Oct 2024 14:20:26 +0200 Subject: [PATCH] Declining terms and conditions in account-console results in error Closes #28328 Signed-off-by: vramik --- .../authentication/RequiredActionContext.java | 12 ++++- .../org/keycloak/protocol/LoginProtocol.java | 4 ++ .../RequiredActionContextResult.java | 10 +++- .../requiredactions/TermsAndConditions.java | 3 +- .../protocol/oidc/OIDCLoginProtocol.java | 17 ++++--- .../keycloak/services/messages/Messages.java | 2 + .../resources/LoginActionsService.java | 2 +- .../org/keycloak/testsuite/pages/AppPage.java | 7 +-- .../keycloak/testsuite/pages/LoginPage.java | 2 + .../actions/TermsAndConditionsTest.java | 47 +++++++++++++++++-- 10 files changed, 89 insertions(+), 17 deletions(-) diff --git a/server-spi-private/src/main/java/org/keycloak/authentication/RequiredActionContext.java b/server-spi-private/src/main/java/org/keycloak/authentication/RequiredActionContext.java index a8dc578561..66baf9f859 100755 --- a/server-spi-private/src/main/java/org/keycloak/authentication/RequiredActionContext.java +++ b/server-spi-private/src/main/java/org/keycloak/authentication/RequiredActionContext.java @@ -120,6 +120,8 @@ public interface RequiredActionContext { Status getStatus(); + String getErrorMessage(); + /** * Send a challenge Response back to user * @@ -127,11 +129,19 @@ public interface RequiredActionContext { */ void challenge(Response response); + /** + * Abort the authentication with an error, optionally with an erroMessage. + * + */ + void failure(String errorMessage); + /** * Abort the authentication with an error * */ - void failure(); + default void failure() { + failure(null); + } /** * Mark this required action as successful. The required action will be removed from the UserModel diff --git a/server-spi-private/src/main/java/org/keycloak/protocol/LoginProtocol.java b/server-spi-private/src/main/java/org/keycloak/protocol/LoginProtocol.java index 4f5b650380..71f630cc04 100755 --- a/server-spi-private/src/main/java/org/keycloak/protocol/LoginProtocol.java +++ b/server-spi-private/src/main/java/org/keycloak/protocol/LoginProtocol.java @@ -86,6 +86,10 @@ public interface LoginProtocol extends Provider { Response sendError(AuthenticationSessionModel authSession, Error error); + default Response sendError(AuthenticationSessionModel authSession, Error error, String errorMessage) { + return sendError(authSession, error); + } + /** * Returns client data, which will be wrapped in the "clientData" parameter sent within "authentication flow" requests. The purpose of clientData is to be able to send HTTP error * response back to the client if authentication fails due some error and authenticationSession is not available anymore (was either expired or removed). So clientData need to contain diff --git a/services/src/main/java/org/keycloak/authentication/RequiredActionContextResult.java b/services/src/main/java/org/keycloak/authentication/RequiredActionContextResult.java index df7bcc9a68..03c7517503 100755 --- a/services/src/main/java/org/keycloak/authentication/RequiredActionContextResult.java +++ b/services/src/main/java/org/keycloak/authentication/RequiredActionContextResult.java @@ -46,6 +46,7 @@ public class RequiredActionContextResult implements RequiredActionContext { protected EventBuilder eventBuilder; protected KeycloakSession session; protected Status status; + protected String errorMessage; protected Response challenge; protected HttpRequest httpRequest; protected UserModel user; @@ -66,6 +67,7 @@ public class RequiredActionContextResult implements RequiredActionContext { this.config = realm.getRequiredActionConfigByAlias(factory.getId()); } + @Override public RequiredActionConfigModel getConfig() { return config; } @@ -119,6 +121,11 @@ public class RequiredActionContextResult implements RequiredActionContext { return status; } + @Override + public String getErrorMessage() { + return errorMessage; + } + @Override public void challenge(Response response) { status = Status.CHALLENGE; @@ -127,7 +134,8 @@ public class RequiredActionContextResult implements RequiredActionContext { } @Override - public void failure() { + public void failure(String errorMessage) { + this.errorMessage = errorMessage; status = Status.FAILURE; } diff --git a/services/src/main/java/org/keycloak/authentication/requiredactions/TermsAndConditions.java b/services/src/main/java/org/keycloak/authentication/requiredactions/TermsAndConditions.java index fa75404f1f..fac17fe2ad 100755 --- a/services/src/main/java/org/keycloak/authentication/requiredactions/TermsAndConditions.java +++ b/services/src/main/java/org/keycloak/authentication/requiredactions/TermsAndConditions.java @@ -23,6 +23,7 @@ import org.keycloak.common.util.Time; import org.keycloak.models.KeycloakSession; import org.keycloak.models.KeycloakSessionFactory; import org.keycloak.models.UserModel; +import org.keycloak.services.messages.Messages; import jakarta.ws.rs.core.Response; import java.util.Arrays; @@ -80,7 +81,7 @@ public class TermsAndConditions implements RequiredActionProvider, RequiredActio if (context.getHttpRequest().getDecodedFormParameters().containsKey("cancel")) { context.getUser().removeAttribute(USER_ATTRIBUTE); - context.failure(); + context.failure(Messages.TERMS_AND_CONDITIONS_DECLINED); return; } diff --git a/services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java b/services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java index 7e6d26e93c..45ef54c794 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java @@ -342,6 +342,11 @@ public class OIDCLoginProtocol implements LoginProtocol { @Override public Response sendError(AuthenticationSessionModel authSession, Error error) { + return sendError(authSession, error, null); + } + + @Override + public Response sendError(AuthenticationSessionModel authSession, Error error, String errorMessage) { if (isOAuth2DeviceVerificationFlow(authSession)) { return denyOAuth2DeviceAuthorization(authSession, error, session); } @@ -352,7 +357,7 @@ public class OIDCLoginProtocol implements LoginProtocol { String redirect = authSession.getRedirectUri(); String state = authSession.getClientNote(OIDCLoginProtocol.STATE_PARAM); - OIDCRedirectUriBuilder redirectUri = buildErrorRedirectUri(redirect, state, error); + OIDCRedirectUriBuilder redirectUri = buildErrorRedirectUri(redirect, state, error, errorMessage); // Remove authenticationSession from current tab new AuthenticationSessionManager(session).removeTabIdInAuthenticationSession(realm, authSession); @@ -360,10 +365,10 @@ public class OIDCLoginProtocol implements LoginProtocol { return buildRedirectUri(redirectUri, authSession, null, null, null, error); } - private OIDCRedirectUriBuilder buildErrorRedirectUri(String redirect, String state, Error error) { + private OIDCRedirectUriBuilder buildErrorRedirectUri(String redirect, String state, Error error, String errorMessage) { OIDCRedirectUriBuilder redirectUri = OIDCRedirectUriBuilder.fromUri(redirect, responseMode, session, null); - OAuth2ErrorRepresentation oauthError = translateError(error); + OAuth2ErrorRepresentation oauthError = translateError(error, errorMessage); if (oauthError.getError() != null) { redirectUri.addParam(OAuth2Constants.ERROR, oauthError.getError()); } @@ -411,11 +416,11 @@ public class OIDCLoginProtocol implements LoginProtocol { } setupResponseTypeAndMode(clientData.getResponseType(), clientData.getResponseMode()); - OIDCRedirectUriBuilder redirectUri = buildErrorRedirectUri(clientData.getRedirectUri(), clientData.getState(), error); + OIDCRedirectUriBuilder redirectUri = buildErrorRedirectUri(clientData.getRedirectUri(), clientData.getState(), error, null); return buildRedirectUri(redirectUri, null, null, null, null, error); } - private OAuth2ErrorRepresentation translateError(Error error) { + private OAuth2ErrorRepresentation translateError(Error error, String errorMessage) { switch (error) { case CANCELLED_AIA_SILENT: return new OAuth2ErrorRepresentation(null, null); @@ -423,7 +428,7 @@ public class OIDCLoginProtocol implements LoginProtocol { return new OAuth2ErrorRepresentation(OAuthErrorException.ACCESS_DENIED, "User cancelled application-initiated action."); case CANCELLED_BY_USER: case CONSENT_DENIED: - return new OAuth2ErrorRepresentation(OAuthErrorException.ACCESS_DENIED, "User denied consent"); + return new OAuth2ErrorRepresentation(OAuthErrorException.ACCESS_DENIED, errorMessage); case PASSIVE_INTERACTION_REQUIRED: return new OAuth2ErrorRepresentation(OAuthErrorException.INTERACTION_REQUIRED, null); case PASSIVE_LOGIN_REQUIRED: diff --git a/services/src/main/java/org/keycloak/services/messages/Messages.java b/services/src/main/java/org/keycloak/services/messages/Messages.java index aa01a637b7..1f87eb3e15 100755 --- a/services/src/main/java/org/keycloak/services/messages/Messages.java +++ b/services/src/main/java/org/keycloak/services/messages/Messages.java @@ -271,6 +271,8 @@ public class Messages { public static final String CONSENT_DENIED="consentDenied"; + public static final String TERMS_AND_CONDITIONS_DECLINED="termsAndConditionsDeclined"; + public static final String ALREADY_LOGGED_IN="alreadyLoggedIn"; public static final String DIFFERENT_USER_AUTHENTICATED = "differentUserAuthenticated"; 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 017878c608..5747f16319 100755 --- a/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java +++ b/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java @@ -1203,7 +1203,7 @@ public class LoginActionsService { event.detail(Details.CUSTOM_REQUIRED_ACTION, action); event.error(Errors.REJECTED_BY_USER); - return protocol.sendError(authSession, error); + return protocol.sendError(authSession, error, context.getErrorMessage()); } private boolean isCancelAppInitiatedAction(String providerId, AuthenticationSessionModel authSession, RequiredActionContextResult context) { diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/AppPage.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/AppPage.java index fbb074ee3a..e327c3b5ec 100755 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/AppPage.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/AppPage.java @@ -17,6 +17,7 @@ package org.keycloak.testsuite.pages; +import org.keycloak.testsuite.util.DroneUtils; import org.keycloak.testsuite.util.OAuthClient; import org.openqa.selenium.WebElement; import org.openqa.selenium.support.FindBy; @@ -34,16 +35,16 @@ public class AppPage extends AbstractPage { @Override public void open() { - driver.navigate().to(OAuthClient.APP_AUTH_ROOT); + DroneUtils.getCurrentDriver().navigate().to(OAuthClient.APP_AUTH_ROOT); } @Override public boolean isCurrent() { - return removeDefaultPorts(driver.getCurrentUrl()).startsWith(OAuthClient.APP_AUTH_ROOT); + return removeDefaultPorts(DroneUtils.getCurrentDriver().getCurrentUrl()).startsWith(OAuthClient.APP_AUTH_ROOT); } public RequestType getRequestType() { - return RequestType.valueOf(driver.getTitle()); + return RequestType.valueOf(DroneUtils.getCurrentDriver().getTitle()); } public void openAccount() { diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/LoginPage.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/LoginPage.java index 6d959092ef..dc33b0f3c9 100755 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/LoginPage.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/LoginPage.java @@ -199,11 +199,13 @@ public class LoginPage extends LanguageComboboxAwarePage { } } + @Override public boolean isCurrent() { String realm = "test"; return isCurrent(realm); } + @Override public boolean isCurrent(String realm) { return DroneUtils.getCurrentDriver().getTitle().equals("Sign in to " + realm) || DroneUtils.getCurrentDriver().getTitle().equals("Anmeldung bei " + realm); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/TermsAndConditionsTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/TermsAndConditionsTest.java index fac14e1517..7b4a247f53 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/TermsAndConditionsTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/TermsAndConditionsTest.java @@ -17,6 +17,7 @@ package org.keycloak.testsuite.actions; import org.hamcrest.Matchers; +import org.jboss.arquillian.drone.api.annotation.Drone; import org.jboss.arquillian.graphene.page.Page; import org.junit.Assert; import org.junit.Before; @@ -26,6 +27,7 @@ import org.keycloak.authentication.requiredactions.TermsAndConditions; import org.keycloak.events.Details; import org.keycloak.events.Errors; import org.keycloak.events.EventType; +import org.keycloak.models.Constants; import org.keycloak.models.UserModel; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.RequiredActionProviderRepresentation; @@ -36,15 +38,24 @@ import org.keycloak.testsuite.pages.AppPage; import org.keycloak.testsuite.pages.AppPage.RequestType; import org.keycloak.testsuite.pages.LoginPage; import org.keycloak.testsuite.pages.TermsAndConditionsPage; +import org.keycloak.testsuite.util.DroneUtils; +import org.keycloak.testsuite.util.JavascriptBrowser; +import org.keycloak.testsuite.util.UIUtils; import org.keycloak.testsuite.util.UserBuilder; +import org.openqa.selenium.By; +import org.openqa.selenium.WebDriver; +import org.openqa.selenium.WebElement; import java.util.List; import java.util.Map; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.keycloak.testsuite.util.ServerURLs.getAuthServerContextRoot; /** * @author Stian Thorgersen @@ -54,19 +65,34 @@ public class TermsAndConditionsTest extends AbstractTestRealmKeycloakTest { @Rule public AssertEvents events = new AssertEvents(this); + @Drone + @JavascriptBrowser + private WebDriver jsDriver; + @Page + @JavascriptBrowser protected AppPage appPage; @Page + @JavascriptBrowser protected LoginPage loginPage; @Page + @JavascriptBrowser protected TermsAndConditionsPage termsPage; @Override public void configureTestRealm(RealmRepresentation testRealm) { } + @Before + public void driver() { + appPage.setDriver(jsDriver); + termsPage.setDriver(jsDriver); + loginPage.setDriver(jsDriver); + DroneUtils.addWebDriver(jsDriver); + } + @Before public void addTermsAndConditionRequiredAction() { UserRepresentation user = ActionUtil.findUserWithAdminClient(adminClient, "test-user@localhost"); @@ -105,16 +131,18 @@ public class TermsAndConditionsTest extends AbstractTestRealmKeycloakTest { assertNotNull("expected non-null timestamp for terms acceptance in user attribute " + TermsAndConditions.USER_ATTRIBUTE, timestamp); try { - Integer.parseInt(timestamp); - } - catch (NumberFormatException e) { + Integer.valueOf(timestamp); + } catch (NumberFormatException e) { fail("timestamp for terms acceptance is not a valid integer: '" + timestamp + "'"); } } @Test public void termsDeclined() { - loginPage.open(); + appPage.open(); + appPage.openAccount(); + + loginPage.assertCurrent(); loginPage.login("test-user@localhost", "password"); @@ -126,6 +154,8 @@ public class TermsAndConditionsTest extends AbstractTestRealmKeycloakTest { .error(Errors.REJECTED_BY_USER) .removeDetail(Details.CONSENT) .session(Matchers.nullValue(String.class)) + .client(Constants.ACCOUNT_CONSOLE_CLIENT_ID) + .detail(Details.REDIRECT_URI, getAuthServerContextRoot() + "/auth/realms/" + TEST_REALM_NAME + "/account") .assertEvent(); @@ -136,6 +166,15 @@ public class TermsAndConditionsTest extends AbstractTestRealmKeycloakTest { assertNull("expected null for terms acceptance user attribute " + TermsAndConditions.USER_ATTRIBUTE, attributes.get(TermsAndConditions.USER_ATTRIBUTE)); } + assertThat(DroneUtils.getCurrentDriver().getTitle(), equalTo("Account Management")); + Assert.assertTrue(DroneUtils.getCurrentDriver().getPageSource().contains("You need to agree to Terms and Conditions")); + Assert.assertFalse(DroneUtils.getCurrentDriver().getPageSource().contains("An unexpected error occurred")); + + WebElement tryAgainButton = DroneUtils.getCurrentDriver().findElement(By.tagName("button")); + assertThat(tryAgainButton.getText(), equalTo("Try again")); + UIUtils.click(tryAgainButton); + + loginPage.assertCurrent(); }