Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Declining terms and conditions in account-console results in error #7

Merged
merged 1 commit into from
Oct 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -120,18 +120,28 @@ enum KcActionStatus {

Status getStatus();

String getErrorMessage();

/**
* Send a challenge Response back to user
*
* @param response
*/
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ enum Error {

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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -66,6 +67,7 @@ public RequiredActionContextResult(AuthenticationSessionModel authSession,
this.config = realm.getRequiredActionConfigByAlias(factory.getId());
}

@Override
public RequiredActionConfigModel getConfig() {
return config;
}
Expand Down Expand Up @@ -119,6 +121,11 @@ public Status getStatus() {
return status;
}

@Override
public String getErrorMessage() {
return errorMessage;
}

@Override
public void challenge(Response response) {
status = Status.CHALLENGE;
Expand All @@ -127,7 +134,8 @@ public void challenge(Response response) {
}

@Override
public void failure() {
public void failure(String errorMessage) {
this.errorMessage = errorMessage;
status = Status.FAILURE;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
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;
Expand Down Expand Up @@ -80,7 +81,7 @@ public void processAction(RequiredActionContext context) {

if (context.getHttpRequest().getDecodedFormParameters().containsKey("cancel")) {
context.getUser().removeAttribute(USER_ATTRIBUTE);
context.failure();
context.failure(Messages.TERMS_AND_CONDITIONS_DECLINED);
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,11 @@ private boolean isIdTokenAsDetachedSignature(ClientModel client) {

@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);
}
Expand All @@ -351,18 +356,18 @@ public Response sendError(AuthenticationSessionModel authSession, Error error) {
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);

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());
}
Expand Down Expand Up @@ -410,19 +415,19 @@ public Response sendError(ClientModel client, ClientData clientData, Error error
}

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);
case CANCELLED_AIA:
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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1203,7 +1203,7 @@ private Response interruptionResponse(RequiredActionContextResult context, Authe
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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,11 +199,13 @@ public String getInfoMessage() {
}
}

@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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -26,6 +27,7 @@
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;
Expand All @@ -36,15 +38,24 @@
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 <a href="mailto:[email protected]">Stian Thorgersen</a>
Expand All @@ -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");
Expand Down Expand Up @@ -105,16 +131,18 @@ public void termsAccepted() {
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");

Expand All @@ -126,6 +154,8 @@ public void termsDeclined() {
.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();


Expand All @@ -136,6 +166,15 @@ public void termsDeclined() {
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();
}


Expand Down