Correctly moves to the next required action (#31358)

Closes #31014

Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>


Co-authored-by: Giuseppe Graziano <g.graziano94@gmail.com>
Co-authored-by: rmartinc <rmartinc@redhat.com>

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

@@ -958,7 +958,14 @@ public class AuthenticationManager {
     public static Response nextActionAfterAuthentication(KeycloakSession session, AuthenticationSessionModel authSession,
                                                   ClientConnection clientConnection,
                                                   HttpRequest request, UriInfo uriInfo, EventBuilder event) {
-        Response requiredAction = actionRequired(session, authSession, request, event);
+        return nextActionAfterAuthentication(session, authSession, clientConnection, request, uriInfo, event, new HashSet<>());
+    }
+
+    private static Response nextActionAfterAuthentication(KeycloakSession session, AuthenticationSessionModel authSession,
+                                                  ClientConnection clientConnection,
+                                                  HttpRequest request, UriInfo uriInfo, EventBuilder event,
+                                                  Set<String> ignoredActions) {
+        Response requiredAction = actionRequired(session, authSession, request, event, ignoredActions);
         if (requiredAction != null) return requiredAction;
         return finishedRequiredActions(session, authSession, null, clientConnection, request, uriInfo, event);
 
@@ -1041,7 +1048,7 @@ public class AuthenticationManager {
         final var realm = authSession.getRealm();
         final var user = authSession.getAuthenticatedUser();
 
-        evaluateRequiredActionTriggers(session, authSession, request, event, realm, user);
+        evaluateRequiredActionTriggers(session, authSession, request, event, realm, user, new HashSet<>());
 
         final var kcAction = authSession.getClientNote(Constants.KC_ACTION);
         final var nextApplicableAction =
@@ -1093,17 +1100,21 @@ public class AuthenticationManager {
         }
     }
 
-
     public static Response actionRequired(final KeycloakSession session, final AuthenticationSessionModel authSession,
             final HttpRequest request, final EventBuilder event) {
+        return actionRequired(session, authSession, request, event, new HashSet<>());
+    }
+
+    private static Response actionRequired(final KeycloakSession session, final AuthenticationSessionModel authSession,
+            final HttpRequest request, final EventBuilder event, Set<String> ignoredActions) {
         final var realm = authSession.getRealm();
         final var user = authSession.getAuthenticatedUser();
 
-        evaluateRequiredActionTriggers(session, authSession, request, event, realm, user);
+        evaluateRequiredActionTriggers(session, authSession, request, event, realm, user, ignoredActions);
 
         event.detail(Details.CODE_ID, authSession.getParentSession().getId());
 
-        final var actionResponse = executionActions(session, authSession, request, event, realm, user);
+        final var actionResponse = executionActions(session, authSession, request, event, realm, user, ignoredActions);
         if (actionResponse != null) {
             return actionResponse;
         }
@@ -1218,21 +1229,22 @@ public class AuthenticationManager {
 
 
     protected static Response executionActions(KeycloakSession session, AuthenticationSessionModel authSession,
-            HttpRequest request, EventBuilder event, RealmModel realm, UserModel user) {
+            HttpRequest request, EventBuilder event, RealmModel realm, UserModel user, Set<String> ignoredActions) {
         final var kcAction = authSession.getClientNote(Constants.KC_ACTION);
         final var firstApplicableRequiredAction =
                 getFirstApplicableRequiredAction(realm, authSession, user, kcAction);
 
         if (firstApplicableRequiredAction != null) {
             return executeAction(session, authSession, firstApplicableRequiredAction, request, event, realm, user,
-                    kcAction != null);
+                    kcAction != null, ignoredActions);
         }
 
         return null;
     }
 
     private static Response executeAction(KeycloakSession session, AuthenticationSessionModel authSession, RequiredActionProviderModel model,
-                                          HttpRequest request, EventBuilder event, RealmModel realm, UserModel user, boolean kcActionExecution) {
+                                          HttpRequest request, EventBuilder event, RealmModel realm, UserModel user, boolean kcActionExecution,
+                                          Set<String> ignoredActions) {
         RequiredActionFactory factory = (RequiredActionFactory) session.getKeycloakSessionFactory()
                 .getProviderFactory(RequiredActionProvider.class, model.getProviderId());
         if (factory == null) {
@@ -1279,12 +1291,20 @@ public class AuthenticationManager {
             authSession.setAuthNote(AuthenticationProcessor.CURRENT_AUTHENTICATION_EXECUTION, model.getProviderId());
             return context.getChallenge();
         }
+        else if (context.getStatus() == RequiredActionContext.Status.IGNORE) {
+            authSession.getAuthenticatedUser().removeRequiredAction(factory.getId());
+            authSession.removeRequiredAction(factory.getId());
+            setKcActionStatus(factory.getId(), RequiredActionContext.KcActionStatus.SUCCESS, authSession);
+            ignoredActions.add(factory.getId());
+            return nextActionAfterAuthentication(session, authSession, session.getContext().getConnection(), request, session.getContext().getUri(), event, ignoredActions);
+        }
         else if (context.getStatus() == RequiredActionContext.Status.SUCCESS) {
             event.clone().event(EventType.CUSTOM_REQUIRED_ACTION).detail(Details.CUSTOM_REQUIRED_ACTION, factory.getId()).success();
             // don't have to perform the same action twice, so remove it from both the user and session required actions
             authSession.getAuthenticatedUser().removeRequiredAction(factory.getId());
             authSession.removeRequiredAction(factory.getId());
             setKcActionStatus(factory.getId(), RequiredActionContext.KcActionStatus.SUCCESS, authSession);
+            return nextActionAfterAuthentication(session, authSession, session.getContext().getConnection(), request, session.getContext().getUri(), event, ignoredActions);
         }
 
         return null;
@@ -1370,9 +1390,16 @@ public class AuthenticationManager {
     public static void evaluateRequiredActionTriggers(final KeycloakSession session, final AuthenticationSessionModel authSession,
                                                       final HttpRequest request, final EventBuilder event,
                                                       final RealmModel realm, final UserModel user) {
+        evaluateRequiredActionTriggers(session, authSession, request, event, realm, user, new HashSet<>());
+    }
+
+    private static void evaluateRequiredActionTriggers(final KeycloakSession session, final AuthenticationSessionModel authSession,
+                                                      final HttpRequest request, final EventBuilder event,
+                                                      final RealmModel realm, final UserModel user, Set<String> ignoredActions) {
         // see if any required actions need triggering, i.e. an expired password
         realm.getRequiredActionProvidersStream()
                 .filter(RequiredActionProviderModel::isEnabled)
+                .filter(model -> !ignoredActions.contains(model.getProviderId()))
                 .map(model -> toRequiredActionFactory(session, model, realm))
                 .filter(Objects::nonNull)
                 .forEachOrdered(f -> evaluateRequiredAction(session, authSession, request, event, realm, user, f));

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionEmailVerificationTest.java

@@ -31,6 +31,7 @@ import org.keycloak.events.Errors;
 import org.keycloak.events.EventType;
 import org.keycloak.models.Constants;
 import org.keycloak.models.RealmModel;
+import org.keycloak.models.UserModel;
 import org.keycloak.models.UserModel.RequiredAction;
 import org.keycloak.models.utils.KeycloakModelUtils;
 import org.keycloak.representations.idm.EventRepresentation;
@@ -51,6 +52,7 @@ import org.keycloak.testsuite.pages.InfoPage;
 import org.keycloak.testsuite.pages.LoginPage;
 import org.keycloak.testsuite.pages.RegisterPage;
 import org.keycloak.testsuite.pages.VerifyEmailPage;
+import org.keycloak.testsuite.pages.VerifyProfilePage;
 import org.keycloak.testsuite.updaters.UserAttributeUpdater;
 import org.keycloak.testsuite.util.GreenMailRule;
 import org.keycloak.testsuite.util.InfinispanTestTimeServiceRule;
@@ -108,6 +110,9 @@ public class RequiredActionEmailVerificationTest extends AbstractTestRealmKeyclo
     @Page
     protected VerifyEmailPage verifyEmailPage;
 
+    @Page
+    protected VerifyProfilePage verifyProfilePage;
+
     @Page
     protected RegisterPage registerPage;
 
@@ -137,10 +142,18 @@ public class RequiredActionEmailVerificationTest extends AbstractTestRealmKeyclo
         ApiUtil.removeUserByUsername(testRealm(), "test-user@localhost");
         UserRepresentation user = UserBuilder.create().enabled(true)
                 .username("test-user@localhost")
+                .firstName("test-user")
+                .lastName("test-user")
+                .emailVerified(false)
                 .email("test-user@localhost").build();
         testUserId = ApiUtil.createUserAndResetPasswordWithAdminClient(testRealm(), user, "password");
     }
 
+    protected boolean removeVerifyProfileAtImport() {
+        // in this test verify profile is enabled
+        return false;
+    }
+
     /**
      * see KEYCLOAK-4163
      */
@@ -1104,4 +1117,49 @@ public class RequiredActionEmailVerificationTest extends AbstractTestRealmKeyclo
         // Required action included in the action token is not valid anymore, because we don't know the provider for it
         assertThat(errorPage.getError(), is("Required actions included in the link are not valid"));
     }
+
+    @Test
+    public void testVerifyEmailWithNoEmailAndVerifyProfile() throws Exception {
+        UserResource user = testRealm().users().get(testUserId);
+        UserRepresentation userRep = user.toRepresentation();
+        userRep.setEmail("");
+        user.update(userRep);
+
+        loginPage.open();
+        loginPage.login("test-user@localhost", "password");
+
+        // verify profile should be presented first as the verify email is ignored without email
+        verifyProfilePage.assertCurrent();
+        events.expectRequiredAction(EventType.VERIFY_PROFILE)
+                .user(testUserId)
+                .detail(Details.FIELDS_TO_UPDATE, UserModel.EMAIL)
+                .assertEvent();
+
+        verifyProfilePage.updateEmail("test-user@localhost", "test-user", "test-user");
+
+        verifyEmailPage.assertCurrent();
+
+        events.expectRequiredAction(EventType.UPDATE_PROFILE)
+                .user(testUserId)
+                .detail(Details.UPDATED_EMAIL, "test-user@localhost")
+                .assertEvent();
+
+        // verify email is presented now
+        Assert.assertEquals(1, greenMail.getReceivedMessages().length);
+
+        final MimeMessage message = greenMail.getLastReceivedMessage();
+
+        final String verificationUrl = getEmailLink(message);
+
+        // confirm
+        driver.navigate().to(verificationUrl);
+
+        // back to app, already logged in
+        appPage.assertCurrent();
+
+        // email should be verified and required actions empty
+        userRep = user.toRepresentation();
+        assertTrue(userRep.isEmailVerified());
+        assertThat(userRep.getRequiredActions(), Matchers.empty());
+    }
 }

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionPriorityTest.java

@@ -416,6 +416,41 @@ public class RequiredActionPriorityTest extends AbstractTestRealmKeycloakTest {
 
     }
 
+    @Test
+    public void skipToNextRequiredActionWithCustomPriority() {
+        enableRequiredActionForUser(RequiredAction.VERIFY_EMAIL);
+        enableRequiredActionForUser(RequiredAction.UPDATE_PASSWORD);
+
+        RealmRepresentation realmRep = testRealm().toRepresentation();
+        realmRep.setVerifyEmail(true);
+        testRealm().update(realmRep);
+
+        final var userResource = testRealm().users().get(testUserId);
+        final var user = userResource.toRepresentation();
+        user.setEmailVerified(true);
+        userResource.update(user);
+
+        final var requiredActionsCustomOrdered = List.of(
+                RequiredAction.VERIFY_EMAIL,
+                RequiredAction.UPDATE_PASSWORD
+        );
+        ApiUtil.updateRequiredActionsOrder(testRealm(), requiredActionsCustomOrdered);
+
+        // Login
+        loginPage.open();
+        loginPage.login(USERNAME, PASSWORD);
+        events.expectRequiredAction(EventType.CUSTOM_REQUIRED_ACTION).assertEvent();
+
+        // change password
+        changePasswordPage.assertCurrent();
+        changePasswordPage.changePassword(NEW_PASSWORD, NEW_PASSWORD);
+        events.expectRequiredAction(EventType.UPDATE_PASSWORD).assertEvent();
+
+        appPage.assertCurrent();
+        assertThat(appPage.getRequestType(), is(RequestType.AUTH_RESPONSE));
+        events.expectLogin().assertEvent();
+    }
+
     private void enableRequiredActionForUser(final RequiredAction requiredAction) {
         setRequiredActionEnabled(TEST_REALM_NAME, testUserId, requiredAction.name(), true);
     }

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractFirstBrokerLoginTest.java

@@ -20,7 +20,6 @@ import org.keycloak.events.Details;
 import org.keycloak.events.EventType;
 import org.keycloak.models.IdentityProviderMapperModel;
 import org.keycloak.models.IdentityProviderSyncMode;
-import org.keycloak.models.UserModel;
 import org.keycloak.representations.idm.ComponentRepresentation;
 import org.keycloak.representations.idm.EventRepresentation;
 import org.keycloak.representations.idm.FederatedIdentityRepresentation;
@@ -33,7 +32,6 @@ import org.keycloak.testsuite.Assert;
 import org.keycloak.testsuite.AssertEvents;
 import org.keycloak.testsuite.admin.ApiUtil;
 import org.keycloak.testsuite.federation.UserMapStorageFactory;
-import org.keycloak.testsuite.forms.VerifyProfileTest;
 import org.keycloak.testsuite.pages.LoginPasswordUpdatePage;
 import org.keycloak.testsuite.util.AccountHelper;
 import org.keycloak.testsuite.util.FederatedIdentityBuilder;
@@ -45,7 +43,6 @@ import org.openqa.selenium.By;
 import org.openqa.selenium.NoSuchElementException;
 import org.openqa.selenium.WebDriver;
 import org.openqa.selenium.WebElement;
-import org.openqa.selenium.htmlunit.HtmlUnitDriver;
 import org.openqa.selenium.support.PageFactory;
 
 import static org.junit.Assert.assertEquals;
@@ -893,9 +890,6 @@ public abstract class AbstractFirstBrokerLoginTest extends AbstractInitializedBa
 
         List<UserRepresentation> users = realm.users().search("no-email");
         assertEquals(1, users.size());
-        List<String> requiredActions = users.get(0).getRequiredActions();
-        assertEquals(1, requiredActions.size());
-        assertEquals(UserModel.RequiredAction.VERIFY_EMAIL.name(), requiredActions.get(0));
 
     }