From 6bdb0f45a349ae56c985a54cd8bff1e1727a6bb4 Mon Sep 17 00:00:00 2001 From: Filip Hanik Date: Wed, 7 Oct 2026 11:31:27 -0700 Subject: [PATCH 1/4] test: confirmation must not authorize a pre-registered password Self-service account confirmation (/verify_user) currently marks the account verified while leaving the password that was submitted on the registration form in place. Clicking the confirmation link only proves ownership of the email address; it does not prove that the clicker chose the stored password. As a result the password supplied by whoever registered the address becomes a usable credential once the owner confirms the account. Add two red MockMvc tests on the self-service flow: - confirmingAccountDoesNotAuthorizeThePreRegisteredPassword: a single registration followed by confirmation must not let the registration password authenticate. - confirmingAccountDoesNotAuthorizePasswordFromEarlierRegistration: when the address is registered twice before confirmation, the earlier password must not authenticate after the owner confirms via the most recent link. Both tests fail today, demonstrating the gap. The following commit closes it. --- .../login/AccountsControllerMockMvcTests.java | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcTests.java index 288012441b1..d64235d70bf 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcTests.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcTests.java @@ -49,6 +49,7 @@ import static org.cloudfoundry.identity.uaa.mock.util.MockMvcUtils.CookieCsrfPostProcessor.cookieCsrf; import static org.springframework.http.MediaType.APPLICATION_JSON; import static org.springframework.security.test.web.servlet.response.SecurityMockMvcResultMatchers.authenticated; +import static org.springframework.security.test.web.servlet.response.SecurityMockMvcResultMatchers.unauthenticated; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.model; @@ -259,6 +260,78 @@ void creatingAnAccount() throws Exception { assertThat(principal.getOrigin()).isEqualTo(OriginKeys.UAA); } + @Test + void confirmingAccountDoesNotAuthorizeThePreRegisteredPassword() throws Exception { + PredictableGenerator generator = new PredictableGenerator(); + JdbcExpiringCodeStore store = webApplicationContext.getBean(JdbcExpiringCodeStore.class); + store.setGenerator(generator); + + String registrationPassword = "firstPass1"; + + mockMvc.perform(post("/create_account.do") + .with(cookieCsrf()) + .param("email", userEmail) + .param("password", registrationPassword) + .param("password_confirmation", registrationPassword)) + .andExpect(status().isFound()) + .andExpect(redirectedUrl("accounts/email_sent")); + + mockMvc.perform(get("/verify_user") + .param("code", "test" + generator.counter.get())) + .andExpect(status().isFound()) + .andReturn(); + + // Clicking the confirmation link only proves ownership of the email address. The + // password supplied on the registration form must not become a usable credential once + // the account is confirmed; the owner must establish the password through the link. + mockMvc.perform(post("/login.do") + .with(cookieCsrf()) + .param("username", userEmail) + .param("password", registrationPassword)) + .andExpect(unauthenticated()); + } + + @Test + void confirmingAccountDoesNotAuthorizePasswordFromEarlierRegistration() throws Exception { + PredictableGenerator generator = new PredictableGenerator(); + JdbcExpiringCodeStore store = webApplicationContext.getBean(JdbcExpiringCodeStore.class); + store.setGenerator(generator); + + String firstRegistrationPassword = "firstPass1"; + String laterRegistrationPassword = "laterPass2"; + + // First registration for the address. + mockMvc.perform(post("/create_account.do") + .with(cookieCsrf()) + .param("email", userEmail) + .param("password", firstRegistrationPassword) + .param("password_confirmation", firstRegistrationPassword)) + .andExpect(status().isFound()) + .andExpect(redirectedUrl("accounts/email_sent")); + + // A later registration for the same still-unconfirmed address. + mockMvc.perform(post("/create_account.do") + .with(cookieCsrf()) + .param("email", userEmail) + .param("password", laterRegistrationPassword) + .param("password_confirmation", laterRegistrationPassword)) + .andExpect(status().isFound()) + .andExpect(redirectedUrl("accounts/email_sent")); + + // The owner confirms the account using the most recent link. + mockMvc.perform(get("/verify_user") + .param("code", "test" + generator.counter.get())) + .andExpect(status().isFound()) + .andReturn(); + + // Confirmation must not authorize the password chosen by the earlier registration. + mockMvc.perform(post("/login.do") + .with(cookieCsrf()) + .param("username", userEmail) + .param("password", firstRegistrationPassword)) + .andExpect(unauthenticated()); + } + @Test void creatingAnAccountWithAnEmptyClientId() throws Exception { PredictableGenerator generator = new PredictableGenerator(); From 7b0a4dac8cf60c68e9435c96fd45e1b220f41d30 Mon Sep 17 00:00:00 2001 From: Filip Hanik Date: Wed, 7 Oct 2026 11:48:14 -0700 Subject: [PATCH 2/4] fix: establish self-service password at email confirmation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-service registration stored the password from the sign-up form and activated it the moment the account was confirmed. Because confirming the activation link only proves ownership of the email address — not that the person confirming chose the stored password — a password set before the address was verified could end up authorizing access to the confirmed account. Establish the password at confirmation instead: - EmailAccountCreationService.completeActivation now verifies the account, discards the stored password by replacing it with an unguessable random value, flags the account as requiring a password change, and issues an ownership-bound, single-use reset code for the user. - AccountsController redirects /verify_user to the reset-password page (with force_change=true) so the confirming owner sets their own password through the same code-authenticated flow used for password recovery; no previous password is required. - ResetPasswordController surfaces a "password change required due to changes in the system" notice on that page (new reset_password.force_change message). A setting-independent consequence: the password typed on the sign-up form is never usable to authenticate; only the owner who confirms via the emailed link can set the account's password. Existing self-service tests are updated to complete the new confirm-then-set-password flow, and the configuration reference documents the behavior. The security tests added in the previous commit now pass. --- docs/UAA-Configuration-Reference.md | 7 + .../uaa/account/AccountCreationService.java | 10 ++ .../uaa/account/AccountsController.java | 10 +- .../account/EmailAccountCreationService.java | 24 ++- .../uaa/account/ResetPasswordController.java | 6 +- .../uaa/login/AccountsControllerTest.java | 14 +- .../EmailAccountCreationServiceTests.java | 9 ++ uaa/src/main/resources/messages.properties | 1 + .../integration/feature/CreateAccountIT.java | 16 +- .../util/IntegrationTestUtils.java | 6 + .../login/AccountsControllerMockMvcTests.java | 150 +++++++++++++----- ...ccountsControllerMockMvcZonePathTests.java | 96 ++++++----- 12 files changed, 255 insertions(+), 94 deletions(-) diff --git a/docs/UAA-Configuration-Reference.md b/docs/UAA-Configuration-Reference.md index 1adb79baaed..69bfa781128 100644 --- a/docs/UAA-Configuration-Reference.md +++ b/docs/UAA-Configuration-Reference.md @@ -1758,6 +1758,13 @@ List of usernames to delete on bootstrap. Used to remove specific users during s When `true`, "Create Account" and "Forgot Password" links are displayed on the login page. +> **Note on self-service account confirmation.** Clicking the activation link in the confirmation +> email verifies ownership of the email address but does **not** activate the password typed on the +> registration form. The owner is sent to a password-setup page (stating that a password change is +> required due to changes in the system) and must establish their own password through the link +> before the account can be used to sign in. This ensures a password chosen before the email address +> was verified can never be used to authenticate. + [Back to table](#login--branding) --- diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/account/AccountCreationService.java b/server/src/main/java/org/cloudfoundry/identity/uaa/account/AccountCreationService.java index bd790c6812b..3292a91c11a 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/account/AccountCreationService.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/account/AccountCreationService.java @@ -19,6 +19,8 @@ class AccountCreationResponse { private String email; @JsonProperty("redirect_location") private String redirectLocation; + @JsonProperty("password_reset_code") + private String passwordResetCode; public AccountCreationResponse(String userId, String username, String email, String redirectLocation) { this.userId = userId; @@ -50,5 +52,13 @@ public String getRedirectLocation() { public String getEmail() { return email; } + + public String getPasswordResetCode() { + return passwordResetCode; + } + + public void setPasswordResetCode(String passwordResetCode) { + this.passwordResetCode = passwordResetCode; + } } } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/account/AccountsController.java b/server/src/main/java/org/cloudfoundry/identity/uaa/account/AccountsController.java index 975be8c2494..aab576a47a9 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/account/AccountsController.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/account/AccountsController.java @@ -124,12 +124,10 @@ public String verifyUser(Model model, return "accounts/link_prompt"; } - String redirectLocation = accountCreation.getRedirectLocation(); - String res = "redirect:/login?success=verify_success"; - if (!redirectLocation.equals(accountCreationService.getDefaultRedirect())) { - res += "&form_redirect_uri=" + redirectLocation; - } - return res; + // Confirmation establishes email ownership but leaves the account without a usable + // password. Send the owner to the reset-password page to set their own password; any + // client redirect is carried by the reset code and applied once the password is set. + return "redirect:/reset_password?code=" + accountCreation.getPasswordResetCode() + "&force_change=true"; } private String handleUnprocessableEntity(Model model, HttpServletResponse response, String attributeKey, String attributeValue) { diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/account/EmailAccountCreationService.java b/server/src/main/java/org/cloudfoundry/identity/uaa/account/EmailAccountCreationService.java index fab9727183c..785b0567a13 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/account/EmailAccountCreationService.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/account/EmailAccountCreationService.java @@ -14,6 +14,7 @@ import org.cloudfoundry.identity.uaa.scim.exception.ScimResourceAlreadyExistsException; import org.cloudfoundry.identity.uaa.scim.util.ScimUtils; import org.cloudfoundry.identity.uaa.scim.validate.PasswordValidator; +import org.cloudfoundry.identity.uaa.util.AlphanumericRandomValueStringGenerator; import org.cloudfoundry.identity.uaa.util.JsonUtils; import org.cloudfoundry.identity.uaa.zone.IdentityZone; import org.cloudfoundry.identity.uaa.zone.MergedZoneBrandingInformation; @@ -52,6 +53,7 @@ public class EmailAccountCreationService implements AccountCreationService { private final ScimUserProvisioning scimUserProvisioning; private final MultitenantClientServices clientDetailsService; private final PasswordValidator passwordValidator; + private final ResetPasswordService resetPasswordService; private final IdentityZoneManager identityZoneManager; public EmailAccountCreationService( @@ -61,6 +63,7 @@ public EmailAccountCreationService( ScimUserProvisioning scimUserProvisioning, MultitenantClientServices clientDetailsService, PasswordValidator passwordValidator, + ResetPasswordService resetPasswordService, IdentityZoneManager identityZoneManager) { this.templateEngine = templateEngine; @@ -69,6 +72,7 @@ public EmailAccountCreationService( this.scimUserProvisioning = scimUserProvisioning; this.clientDetailsService = clientDetailsService; this.passwordValidator = passwordValidator; + this.resetPasswordService = resetPasswordService; this.identityZoneManager = identityZoneManager; } @@ -121,14 +125,28 @@ public AccountCreationResponse completeActivation(String code) { Map data = JsonUtils.readValue(expiringCode.getData(), new TypeReference>() { }); - ScimUser user = scimUserProvisioning.retrieve(data.get("user_id"), identityZoneManager.getCurrentIdentityZoneId()); - user = scimUserProvisioning.verifyUser(user.getId(), user.getVersion(), identityZoneManager.getCurrentIdentityZoneId()); + String zoneId = identityZoneManager.getCurrentIdentityZoneId(); + ScimUser user = scimUserProvisioning.retrieve(data.get("user_id"), zoneId); + user = scimUserProvisioning.verifyUser(user.getId(), user.getVersion(), zoneId); + + // Confirming the activation link proves ownership of the email address, but it does not + // prove that the person confirming is the one who chose the password on the registration + // form. Discard the registration password so it can never authenticate, and require the + // confirming owner to establish their own password through the ownership-proving link. + scimUserProvisioning.changePassword(user.getId(), null, new AlphanumericRandomValueStringGenerator(40).generate(), zoneId); + scimUserProvisioning.updatePasswordChangeRequired(user.getId(), true, zoneId); String clientId = data.get("client_id"); String redirectUri = data.get("redirect_uri") != null ? data.get("redirect_uri") : ""; String redirectLocation = getRedirect(clientId, redirectUri); - return new AccountCreationResponse(user.getId(), user.getUserName(), user.getUserName(), redirectLocation); + // Issue an ownership-bound, single-use code so the owner sets their password through the + // same reset flow used for password recovery (no previous password is required). + ForgotPasswordInfo forgotPasswordInfo = resetPasswordService.forgotPassword(user.getUserName(), clientId, redirectUri); + + AccountCreationResponse response = new AccountCreationResponse(user.getId(), user.getUserName(), user.getUserName(), redirectLocation); + response.setPasswordResetCode(forgotPasswordInfo.getResetPasswordCode().getCode()); + return response; } private String getRedirect(String clientId, String redirectUri) { diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/account/ResetPasswordController.java b/server/src/main/java/org/cloudfoundry/identity/uaa/account/ResetPasswordController.java index b10f38d66b7..3c98b278aaa 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/account/ResetPasswordController.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/account/ResetPasswordController.java @@ -176,7 +176,8 @@ public void resetPassword() { @GetMapping(value = "/reset_password", params = {"code"}) public String resetPasswordPage(Model model, HttpServletResponse response, - @RequestParam("code") String code) { + @RequestParam("code") String code, + @RequestParam(value = "force_change", required = false) boolean forceChange) { ExpiringCode expiringCode = checkIfUserExists(codeStore.retrieveCode(code, identityZoneManager.getCurrentIdentityZone().getId())); if (expiringCode == null) { @@ -189,6 +190,9 @@ public String resetPasswordPage(Model model, model.addAttribute("code", newCode); model.addAttribute("email", uaaUser.getEmail()); model.addAttribute("username", uaaUser.getUsername()); + if (forceChange) { + model.addAttribute("message_code", "force_change"); + } return "reset_password"; } } diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerTest.java index 732fb7efb76..63199a8e9cf 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerTest.java @@ -203,30 +203,32 @@ void passwordMismatch() throws Exception { @Test void verifyUser() throws Exception { - when(accountCreationService.completeActivation("the_secret_code")) - .thenReturn(new AccountCreationService.AccountCreationResponse("newly-created-user-id", "username", "user@example.com", "//example.com/callback")); + AccountCreationService.AccountCreationResponse response = new AccountCreationService.AccountCreationResponse("newly-created-user-id", "username", "user@example.com", "//example.com/callback"); + response.setPasswordResetCode("the_reset_code"); + when(accountCreationService.completeActivation("the_secret_code")).thenReturn(response); MockHttpServletRequestBuilder get = get("/verify_user") .param("code", "the_secret_code"); mockMvc.perform(get) .andExpect(status().isFound()) - .andExpect(redirectedUrl("/login?success=verify_success&form_redirect_uri=//example.com/callback")); + .andExpect(redirectedUrl("/reset_password?code=the_reset_code&force_change=true")); assertThat(SecurityContextHolder.getContext().getAuthentication()).isNull(); } @Test void verifyUserWithPriorHeadRequest() throws Exception { - when(accountCreationService.completeActivation("the_secret_code")) - .thenReturn(new AccountCreationService.AccountCreationResponse("newly-created-user-id", "username", "user@example.com", "//example.com/callback")); + AccountCreationService.AccountCreationResponse response = new AccountCreationService.AccountCreationResponse("newly-created-user-id", "username", "user@example.com", "//example.com/callback"); + response.setPasswordResetCode("the_reset_code"); + when(accountCreationService.completeActivation("the_secret_code")).thenReturn(response); mockMvc.perform(head("/verify_user").param("code", "the_secret_code")) .andExpect(status().isFound()) .andExpect(redirectedUrl("/login")); mockMvc.perform(get("/verify_user").param("code", "the_secret_code")) .andExpect(status().isFound()) - .andExpect(redirectedUrl("/login?success=verify_success&form_redirect_uri=//example.com/callback")); + .andExpect(redirectedUrl("/reset_password?code=the_reset_code&force_change=true")); assertThat(SecurityContextHolder.getContext().getAuthentication()).isNull(); Mockito.verify(accountCreationService, times(1)).completeActivation("the_secret_code"); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/login/EmailAccountCreationServiceTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/login/EmailAccountCreationServiceTests.java index eb1e2bde8bc..be75e701144 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/login/EmailAccountCreationServiceTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/login/EmailAccountCreationServiceTests.java @@ -3,6 +3,8 @@ import org.assertj.core.api.InstanceOfAssertFactories; import org.cloudfoundry.identity.uaa.account.AccountCreationService; import org.cloudfoundry.identity.uaa.account.EmailAccountCreationService; +import org.cloudfoundry.identity.uaa.account.ForgotPasswordInfo; +import org.cloudfoundry.identity.uaa.account.ResetPasswordService; import org.cloudfoundry.identity.uaa.codestore.ExpiringCode; import org.cloudfoundry.identity.uaa.codestore.ExpiringCodeStore; import org.cloudfoundry.identity.uaa.constants.OriginKeys; @@ -72,6 +74,7 @@ class EmailAccountCreationServiceTests { private MultitenantClientServices mockClientDetailsService; private ClientDetails mockClientDetails; private PasswordValidator mockPasswordValidator; + private ResetPasswordService mockResetPasswordService; private IdentityZoneManager mockIdentityZoneManager; private ScimUser user; private ExpiringCode code; @@ -91,6 +94,7 @@ void setUp() { mockClientDetailsService = mock(MultitenantClientServices.class); mockClientDetails = mock(ClientDetails.class); mockPasswordValidator = mock(PasswordValidator.class); + mockResetPasswordService = mock(ResetPasswordService.class); mockIdentityZoneManager = mock(IdentityZoneManager.class); emailAccountCreationService = initEmailAccountCreationService(); @@ -104,6 +108,10 @@ void setUp() { currentIdentityZoneId = "zoneId" + randomValueStringGenerator.generate(); when(mockIdentityZoneManager.getCurrentIdentityZoneId()).thenReturn(currentIdentityZoneId); + + ExpiringCode resetCode = new ExpiringCode("reset_password_code", new Timestamp(System.currentTimeMillis()), "{}", "forgot_password_for_id:newly-created-user-id"); + when(mockResetPasswordService.forgotPassword(anyString(), any(), any())) + .thenReturn(new ForgotPasswordInfo("newly-created-user-id", "user@example.com", resetCode)); } private EmailAccountCreationService initEmailAccountCreationService() { @@ -114,6 +122,7 @@ private EmailAccountCreationService initEmailAccountCreationService() { mockScimUserProvisioning, mockClientDetailsService, mockPasswordValidator, + mockResetPasswordService, mockIdentityZoneManager ); } diff --git a/uaa/src/main/resources/messages.properties b/uaa/src/main/resources/messages.properties index 8cac851020c..80ec61c0705 100644 --- a/uaa/src/main/resources/messages.properties +++ b/uaa/src/main/resources/messages.properties @@ -39,6 +39,7 @@ change_password.unauthorized=Please enter a valid current password. forgot_password.form_error=Please enter a valid email address. forgot_password.bad_code=Sorry, your reset password link is no longer valid. You can request another one below. reset_password.form_error=Passwords must match and not be empty. +reset_password.force_change=A password change is required due to changes in the system. Please set a new password. create_account.form_error=Passwords must match and not be empty. create_account.other_idp=Account sign-up is not required for this email domain. Please login with the identity provider force_password_change.form_error=Passwords must match and not be empty. diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/CreateAccountIT.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/CreateAccountIT.java index e0db98e33e7..dd1ee4a143c 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/CreateAccountIT.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/CreateAccountIT.java @@ -95,8 +95,14 @@ void userInitiatedSignup() throws Exception { .doesNotContain("%40"); webDriver.get(link); - assertThat(webDriver.findElement(By.cssSelector("h1")).getText()).doesNotContain("Where to?"); + // Confirming the account lands on the forced password-set page, not a sign-in page. + assertThat(webDriver.findElement(By.cssSelector("h1")).getText()).isEqualTo("Reset Password"); + webDriver.findElement(By.name("password")).sendKeys(SECRET); + webDriver.findElement(By.name("password_confirmation")).sendKeys(SECRET); + webDriver.clickAndWait(By.xpath("//input[@value='Create new password']")); + + // The owner can now sign in with the password they set. webDriver.findElement(By.name("username")).sendKeys(userEmail); webDriver.findElement(By.name("password")).sendKeys(SECRET); webDriver.clickAndWait(By.xpath("//input[@value='Sign in']")); @@ -130,8 +136,14 @@ void clientInitiatedSignup() throws Exception { assertThat(link).isNotEmpty(); webDriver.get(link); - assertThat(webDriver.findElement(By.cssSelector("h1")).getText()).doesNotContain("Where to?"); + // Confirming the account lands on the forced password-set page, not a sign-in page. + assertThat(webDriver.findElement(By.cssSelector("h1")).getText()).isEqualTo("Reset Password"); + + webDriver.findElement(By.name("password")).sendKeys(SECRET); + webDriver.findElement(By.name("password_confirmation")).sendKeys(SECRET); + webDriver.clickAndWait(By.xpath("//input[@value='Create new password']")); + // The owner can now sign in with the password they set. webDriver.findElement(By.name("username")).sendKeys(userEmail); webDriver.findElement(By.name("password")).sendKeys(SECRET); webDriver.clickAndWait(By.xpath("//input[@value='Sign in']")); diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/util/IntegrationTestUtils.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/util/IntegrationTestUtils.java index 922c8083d56..96b8d340409 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/util/IntegrationTestUtils.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/util/IntegrationTestUtils.java @@ -1618,6 +1618,12 @@ public static String createAnotherUser(UaaWebDriver webDriver, String password, throw new RuntimeException(e); } + // Confirming the account now lands on a forced password-set page; the owner must establish + // their password through the link before the account can be used to log in. + webDriver.findElement(By.name("password")).sendKeys(password); + webDriver.findElement(By.name("password_confirmation")).sendKeys(password); + webDriver.clickAndWait(By.xpath("//input[@value='Create new password']")); + return userEmail; } diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcTests.java index d64235d70bf..91c68488463 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcTests.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcTests.java @@ -42,6 +42,7 @@ import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder; import org.springframework.web.context.WebApplicationContext; import org.springframework.web.context.support.StandardServletEnvironment; +import org.springframework.web.util.UriComponentsBuilder; import java.util.Collections; @@ -63,7 +64,6 @@ @DefaultTestContext class AccountsControllerMockMvcTests { - private static final String LOGIN_REDIRECT = "/login?success=verify_success"; private static final String USER_PASSWORD = "secr3T"; private final AlphanumericRandomValueStringGenerator generator = new AlphanumericRandomValueStringGenerator(); private String userEmail; @@ -242,11 +242,7 @@ void creatingAnAccount() throws Exception { ScimUser scimUser = scimUserProvisioning.query("userName eq '" + userEmail + "' and origin eq '" + OriginKeys.UAA + "'", IdentityZoneHolder.get().getId()).getFirst(); assertThat(scimUser.isVerified()).isFalse(); - mockMvc.perform(get("/verify_user") - .param("code", "test" + generator.counter.get())) - .andExpect(status().isFound()) - .andExpect(redirectedUrl(LOGIN_REDIRECT)) - .andReturn(); + confirmAccountAndSetPassword("test" + generator.counter.get(), "", userEmail); MvcResult mvcResult = loginWithAccount("") .andExpect(authenticated()) @@ -333,7 +329,7 @@ void confirmingAccountDoesNotAuthorizePasswordFromEarlierRegistration() throws E } @Test - void creatingAnAccountWithAnEmptyClientId() throws Exception { + void confirmingAccountSendsOwnerToForcedPasswordSetup() throws Exception { PredictableGenerator generator = new PredictableGenerator(); JdbcExpiringCodeStore store = webApplicationContext.getBean(JdbcExpiringCodeStore.class); store.setGenerator(generator); @@ -342,17 +338,65 @@ void creatingAnAccountWithAnEmptyClientId() throws Exception { .with(cookieCsrf()) .param("email", userEmail) .param("password", "secr3T") - .param("password_confirmation", "secr3T") - .param("client_id", "")) + .param("password_confirmation", "secr3T")) .andExpect(status().isFound()) .andExpect(redirectedUrl("accounts/email_sent")); - mockMvc.perform(get("/verify_user") + // Confirming the account sends the owner to the reset-password page to establish a password. + MvcResult verifyResult = mockMvc.perform(get("/verify_user") .param("code", "test" + generator.counter.get())) .andExpect(status().isFound()) - .andExpect(redirectedUrl(LOGIN_REDIRECT)) .andReturn(); + String location = verifyResult.getResponse().getRedirectedUrl(); + assertThat(location) + .startsWith("/reset_password?code=") + .contains("force_change=true"); + String resetCode = UriComponentsBuilder.fromUriString(location).build().getQueryParams().getFirst("code"); + + // The page explains that a password change is required due to changes in the system. + MvcResult pageResult = mockMvc.perform(get("/reset_password") + .param("code", resetCode) + .param("force_change", "true")) + .andExpect(status().isOk()) + .andExpect(view().name("reset_password")) + .andExpect(model().attribute("message_code", "force_change")) + .andReturn(); + String formCode = (String) pageResult.getModelAndView().getModel().get("code"); + + mockMvc.perform(post("/reset_password.do") + .with(cookieCsrf()) + .param("code", formCode) + .param("email", userEmail) + .param("password", USER_PASSWORD) + .param("password_confirmation", USER_PASSWORD)) + .andExpect(status().isFound()); + + // Once the owner has set their own password, the account can be used to log in. + mockMvc.perform(post("/login.do") + .with(cookieCsrf()) + .param("username", userEmail) + .param("password", USER_PASSWORD)) + .andExpect(authenticated()); + } + + @Test + void creatingAnAccountWithAnEmptyClientId() throws Exception { + PredictableGenerator generator = new PredictableGenerator(); + JdbcExpiringCodeStore store = webApplicationContext.getBean(JdbcExpiringCodeStore.class); + store.setGenerator(generator); + + mockMvc.perform(post("/create_account.do") + .with(cookieCsrf()) + .param("email", userEmail) + .param("password", "secr3T") + .param("password_confirmation", "secr3T") + .param("client_id", "")) + .andExpect(status().isFound()) + .andExpect(redirectedUrl("accounts/email_sent")); + + confirmAccountAndSetPassword("test" + generator.counter.get(), "", userEmail); + MvcResult mvcResult = loginWithAccount("") .andExpect(authenticated()) .andReturn(); @@ -367,12 +411,12 @@ void creatingAnAccountWithAnEmptyClientId() throws Exception { @Test void creatingAnAccountWithClientRedirect() throws Exception { - createAccount("http://redirect.uri/client", "http://redirect.uri/client"); + createAccount("http://redirect.uri/client"); } @Test void creatingAnAccountWithFallbackClientRedirect() throws Exception { - createAccount("http://redirect.uri/fallback", null); + createAccount(null); } @Test @@ -393,11 +437,7 @@ void creatingAnAccountWithNoClientRedirect() throws Exception { assertThat(message.getContentString()).contains("Cloud Foundry"); assertThat(message.getMessage().getHeader("From")).contains("Cloud Foundry "); - mockMvc.perform(get("/verify_user") - .param("code", "test" + generator.counter.get())) - .andExpect(status().isFound()) - .andExpect(redirectedUrl(LOGIN_REDIRECT)) - .andReturn(); + confirmAccountAndSetPassword("test" + generator.counter.get(), "", userEmail); MvcResult mvcResult = loginWithAccount("") .andExpect(authenticated()) @@ -448,12 +488,7 @@ void creatingAnAccountInAnotherZoneWithNoClientRedirect() throws Exception { assertThat(hasLength(link)).isTrue(); assertThat(link).contains(subdomain + ".localhost"); - mockMvc.perform(get("/verify_user") - .param("code", "test" + generator.counter.get()) - .with(new SetServerNameRequestPostProcessor(subdomain + ".localhost"))) - .andExpect(status().isFound()) - .andExpect(redirectedUrl(LOGIN_REDIRECT)) - .andReturn(); + confirmAccountAndSetPassword("test" + generator.counter.get(), subdomain, userEmail); MvcResult mvcResult = loginWithAccount(subdomain) .andExpect(redirectedUrl("/")) @@ -498,11 +533,7 @@ void creatingAnAccountInAnotherZoneWithClientRedirect() throws Exception { assertThat(hasLength(link)).isTrue(); assertThat(link).contains(subdomain + ".localhost"); - mockMvc.perform(get("/verify_user") - .param("code", "test" + generator.counter.get()) - .with(new SetServerNameRequestPostProcessor(subdomain + ".localhost"))) - .andExpect(redirectedUrl(LOGIN_REDIRECT + "&form_redirect_uri=http://myzoneclient.example.com")) - .andReturn(); + confirmAccountAndSetPassword("test" + generator.counter.get(), subdomain, userEmail); MvcResult mvcResult = loginWithAccount(subdomain) .andExpect(authenticated()) @@ -541,14 +572,29 @@ void redirectToSavedRequest_ifPresent() throws Exception { .param("password_confirmation", "test-password")) .andExpect(redirectedUrl("accounts/email_sent")); - mockMvc.perform(get("/verify_user") + MvcResult verifyResult = mockMvc.perform(get("/verify_user") .session(session) .param("code", "test" + generator.counter.get())) .andExpect(status().isFound()) - .andExpect(redirectedUrl(LOGIN_REDIRECT)) .andReturn(); + String location = verifyResult.getResponse().getRedirectedUrl(); + assertThat(location) + .startsWith("/reset_password?code=") + .contains("force_change=true"); + assertThat(SessionUtils.getSavedRequestSession(MockMvcUtils.getZoneSession(session)).getRedirectUrl()).isNotNull(); + + // Consume the reset code so the predictable code sequence does not collide with later tests. + String resetCode = UriComponentsBuilder.fromUriString(location).build().getQueryParams().getFirst("code"); + mockMvc.perform(post("/reset_password.do") + .with(cookieCsrf()) + .session(session) + .param("code", resetCode) + .param("email", "testuser@test.org") + .param("password", USER_PASSWORD) + .param("password_confirmation", USER_PASSWORD)) + .andExpect(status().isFound()); } @Test @@ -653,7 +699,7 @@ private UaaClientDetails createTestClient() throws Exception { return MockMvcUtils.createClient(mockMvc, adminToken, clientDetails); } - private void createAccount(String expectedRedirectUri, String redirectUri) throws Exception { + private void createAccount(String redirectUri) throws Exception { PredictableGenerator generator = new PredictableGenerator(); JdbcExpiringCodeStore store = webApplicationContext.getBean(JdbcExpiringCodeStore.class); store.setGenerator(generator); @@ -673,11 +719,7 @@ private void createAccount(String expectedRedirectUri, String redirectUri) throw assertThat(message.getContentString()).contains("Cloud Foundry"); assertThat(message.getMessage().getHeader("From")).contains("Cloud Foundry "); - mockMvc.perform(get("/verify_user") - .param("code", "test" + generator.counter.get())) - .andExpect(status().isFound()) - .andExpect(redirectedUrl(LOGIN_REDIRECT + "&form_redirect_uri=" + expectedRedirectUri)) - .andReturn(); + confirmAccountAndSetPassword("test" + generator.counter.get(), "", userEmail); MvcResult mvcResult = loginWithAccount("") .andExpect(authenticated()) @@ -691,6 +733,40 @@ private void createAccount(String expectedRedirectUri, String redirectUri) throw assertThat(principal.getOrigin()).isEqualTo(OriginKeys.UAA); } + /** + * Account confirmation no longer leaves a usable password: clicking the activation link sends + * the owner to the reset-password page to establish their own password. This helper completes + * that flow, confirming the account via {@code /verify_user} and setting the password to + * {@link #USER_PASSWORD} so the account can then be used to log in. + */ + private void confirmAccountAndSetPassword(String activationCode, String subdomain, String email) throws Exception { + MockHttpServletRequestBuilder verify = get("/verify_user").param("code", activationCode); + if (hasText(subdomain)) { + verify.with(new SetServerNameRequestPostProcessor(subdomain + ".localhost")); + } + MvcResult verifyResult = mockMvc.perform(verify) + .andExpect(status().isFound()) + .andReturn(); + + String location = verifyResult.getResponse().getRedirectedUrl(); + assertThat(location) + .startsWith("/reset_password?code=") + .contains("force_change=true"); + String resetCode = UriComponentsBuilder.fromUriString(location).build().getQueryParams().getFirst("code"); + + MockHttpServletRequestBuilder reset = post("/reset_password.do") + .with(cookieCsrf()) + .param("code", resetCode) + .param("email", email) + .param("password", USER_PASSWORD) + .param("password_confirmation", USER_PASSWORD); + if (hasText(subdomain)) { + reset.with(new SetServerNameRequestPostProcessor(subdomain + ".localhost")); + } + mockMvc.perform(reset) + .andExpect(status().isFound()); + } + private ResultActions loginWithAccount(String subdomain) throws Exception { MockHttpServletRequestBuilder req = post("/login.do") diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcZonePathTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcZonePathTests.java index e510aedcc09..dfc573c7ad4 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcZonePathTests.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcZonePathTests.java @@ -45,6 +45,7 @@ import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder; import org.springframework.web.context.WebApplicationContext; import org.springframework.web.context.support.StandardServletEnvironment; +import org.springframework.web.util.UriComponentsBuilder; import org.cloudfoundry.identity.uaa.extensions.EnabledIfZonePathsEnabled; import java.util.Collections; @@ -61,12 +62,12 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.view; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.xpath; import static org.springframework.util.StringUtils.hasLength; +import static org.springframework.util.StringUtils.hasText; @DefaultTestContext @EnabledIfZonePathsEnabled class AccountsControllerMockMvcZonePathTests { - private static final String LOGIN_REDIRECT = "/login?success=verify_success"; private static final String USER_PASSWORD = "secr3T"; private final AlphanumericRandomValueStringGenerator generator = new AlphanumericRandomValueStringGenerator(); private String userEmail; @@ -303,11 +304,7 @@ void creatingAnAccount() throws Exception { ScimUser scimUser = scimUserProvisioning.query("userName eq '" + userEmail + "' and origin eq '" + OriginKeys.UAA + "'", IdentityZoneHolder.get().getId()).getFirst(); assertThat(scimUser.isVerified()).isFalse(); - mockMvc.perform(get("/verify_user") - .param("code", "test" + generator.counter.get())) - .andExpect(status().isFound()) - .andExpect(redirectedUrl(LOGIN_REDIRECT)) - .andReturn(); + confirmAccountAndSetPassword(ZoneResolutionMode.SUBDOMAIN, "", "test" + generator.counter.get(), userEmail); MvcResult mvcResult = loginWithAccount("") .andExpect(authenticated()) @@ -336,11 +333,7 @@ void creatingAnAccountWithAnEmptyClientId() throws Exception { .andExpect(status().isFound()) .andExpect(redirectedUrl("accounts/email_sent")); - mockMvc.perform(get("/verify_user") - .param("code", "test" + generator.counter.get())) - .andExpect(status().isFound()) - .andExpect(redirectedUrl(LOGIN_REDIRECT)) - .andReturn(); + confirmAccountAndSetPassword(ZoneResolutionMode.SUBDOMAIN, "", "test" + generator.counter.get(), userEmail); MvcResult mvcResult = loginWithAccount("") .andExpect(authenticated()) @@ -356,12 +349,12 @@ void creatingAnAccountWithAnEmptyClientId() throws Exception { @Test void creatingAnAccountWithClientRedirect() throws Exception { - createAccount("http://redirect.uri/client", "http://redirect.uri/client"); + createAccount("http://redirect.uri/client"); } @Test void creatingAnAccountWithFallbackClientRedirect() throws Exception { - createAccount("http://redirect.uri/fallback", null); + createAccount(null); } @Test @@ -382,11 +375,7 @@ void creatingAnAccountWithNoClientRedirect() throws Exception { assertThat(message.getContentString()).contains("Cloud Foundry"); assertThat(message.getMessage().getHeader("From")).contains("Cloud Foundry "); - mockMvc.perform(get("/verify_user") - .param("code", "test" + generator.counter.get())) - .andExpect(status().isFound()) - .andExpect(redirectedUrl(LOGIN_REDIRECT)) - .andReturn(); + confirmAccountAndSetPassword(ZoneResolutionMode.SUBDOMAIN, "", "test" + generator.counter.get(), userEmail); MvcResult mvcResult = loginWithAccount("") .andExpect(authenticated()) @@ -441,12 +430,7 @@ void creatingAnAccountInAnotherZoneWithNoClientRedirect(ZoneResolutionMode mode) assertThat(link).contains(subdomain + ".localhost"); } - String expectedVerifyRedirect = mode == ZoneResolutionMode.ZONE_PATH ? "/z/" + subdomain + "/login?success=verify_success" : LOGIN_REDIRECT; - mockMvc.perform(mode.createRequestBuilder(subdomain, HttpMethod.GET, "/verify_user") - .param("code", "test" + generator.counter.get())) - .andExpect(status().isFound()) - .andExpect(redirectedUrl(expectedVerifyRedirect)) - .andReturn(); + confirmAccountAndSetPassword(mode, subdomain, "test" + generator.counter.get(), userEmail); MvcResult mvcResult = loginWithAccount(subdomain) .andExpect(redirectedUrl("/")) @@ -495,13 +479,7 @@ void creatingAnAccountInAnotherZoneWithClientRedirect(ZoneResolutionMode mode) t assertThat(link).contains(subdomain + ".localhost"); } - String expectedVerifyRedirect = mode == ZoneResolutionMode.ZONE_PATH - ? "/z/" + subdomain + "/login?success=verify_success&form_redirect_uri=http://myzoneclient.example.com" - : LOGIN_REDIRECT + "&form_redirect_uri=http://myzoneclient.example.com"; - mockMvc.perform(mode.createRequestBuilder(subdomain, HttpMethod.GET, "/verify_user") - .param("code", "test" + generator.counter.get())) - .andExpect(redirectedUrl(expectedVerifyRedirect)) - .andReturn(); + confirmAccountAndSetPassword(mode, subdomain, "test" + generator.counter.get(), userEmail); MvcResult mvcResult = loginWithAccount(subdomain) .andExpect(authenticated()) @@ -540,14 +518,29 @@ void redirectToSavedRequest_ifPresent() throws Exception { .param("password_confirmation", "test-password")) .andExpect(redirectedUrl("accounts/email_sent")); - mockMvc.perform(get("/verify_user") + MvcResult verifyResult = mockMvc.perform(get("/verify_user") .session(session) .param("code", "test" + generator.counter.get())) .andExpect(status().isFound()) - .andExpect(redirectedUrl(LOGIN_REDIRECT)) .andReturn(); + String location = verifyResult.getResponse().getRedirectedUrl(); + assertThat(location) + .startsWith("/reset_password?code=") + .contains("force_change=true"); + assertThat(SessionUtils.getSavedRequestSession(MockMvcUtils.getZoneSession(session)).getRedirectUrl()).isNotNull(); + + // Consume the reset code so the predictable code sequence does not collide with later tests. + String resetCode = UriComponentsBuilder.fromUriString(location).build().getQueryParams().getFirst("code"); + mockMvc.perform(post("/reset_password.do") + .with(cookieCsrf()) + .session(session) + .param("code", resetCode) + .param("email", "testuser@test.org") + .param("password", USER_PASSWORD) + .param("password_confirmation", USER_PASSWORD)) + .andExpect(status().isFound()); } @Test @@ -649,7 +642,7 @@ private UaaClientDetails createTestClient() throws Exception { return MockMvcUtils.createClient(mockMvc, adminToken, clientDetails); } - private void createAccount(String expectedRedirectUri, String redirectUri) throws Exception { + private void createAccount(String redirectUri) throws Exception { PredictableGenerator generator = new PredictableGenerator(); JdbcExpiringCodeStore store = webApplicationContext.getBean(JdbcExpiringCodeStore.class); store.setGenerator(generator); @@ -669,11 +662,7 @@ private void createAccount(String expectedRedirectUri, String redirectUri) throw assertThat(message.getContentString()).contains("Cloud Foundry"); assertThat(message.getMessage().getHeader("From")).contains("Cloud Foundry "); - mockMvc.perform(get("/verify_user") - .param("code", "test" + generator.counter.get())) - .andExpect(status().isFound()) - .andExpect(redirectedUrl(LOGIN_REDIRECT + "&form_redirect_uri=" + expectedRedirectUri)) - .andReturn(); + confirmAccountAndSetPassword(ZoneResolutionMode.SUBDOMAIN, "", "test" + generator.counter.get(), userEmail); MvcResult mvcResult = loginWithAccount("") .andExpect(authenticated()) @@ -687,6 +676,35 @@ private void createAccount(String expectedRedirectUri, String redirectUri) throw assertThat(principal.getOrigin()).isEqualTo(OriginKeys.UAA); } + /** + * Account confirmation no longer leaves a usable password: clicking the activation link sends + * the owner to the reset-password page to establish their own password. This helper completes + * that flow (zone-resolution aware), confirming the account via {@code /verify_user} and setting + * the password to {@link #USER_PASSWORD} so the account can then be used to log in. + */ + private void confirmAccountAndSetPassword(ZoneResolutionMode mode, String subdomain, String activationCode, String email) throws Exception { + MvcResult verifyResult = mockMvc.perform(mode.createRequestBuilder(subdomain != null ? subdomain : "", HttpMethod.GET, "/verify_user") + .param("code", activationCode)) + .andExpect(status().isFound()) + .andReturn(); + + String location = verifyResult.getResponse().getRedirectedUrl(); + String expectedPrefix = (mode == ZoneResolutionMode.ZONE_PATH && hasText(subdomain)) + ? "/z/" + subdomain + "/reset_password?code=" : "/reset_password?code="; + assertThat(location) + .startsWith(expectedPrefix) + .contains("force_change=true"); + String resetCode = UriComponentsBuilder.fromUriString(location).build().getQueryParams().getFirst("code"); + + mockMvc.perform(mode.createRequestBuilder(subdomain != null ? subdomain : "", HttpMethod.POST, "/reset_password.do") + .with(cookieCsrf()) + .param("code", resetCode) + .param("email", email) + .param("password", USER_PASSWORD) + .param("password_confirmation", USER_PASSWORD)) + .andExpect(status().isFound()); + } + private ResultActions loginWithAccount(String subdomain) throws Exception { return loginWithAccount(ZoneResolutionMode.SUBDOMAIN, subdomain); } From 210c98f15ece414afb54d3aa4825fb7162090053 Mon Sep 17 00:00:00 2001 From: Filip Hanik Date: Wed, 7 Oct 2026 12:50:11 -0700 Subject: [PATCH 3/4] fix: preserve the signup redirect across account confirmation The confirmation flow now carries the resolved signup redirect (including the client's signup_redirect_url fallback) into the reset code, so the owner is returned to it after setting their password. Previously only an explicit, registered redirect_uri survived; the signup_redirect_url fallback was lost and the owner landed on "home". completeActivation now passes the resolved redirect location (from getRedirect, which honours signup_redirect_url) to forgotPassword instead of the raw redirect_uri. The reset flow re-matches it against the client's registered redirect URIs before applying it. Restores the redirect assertions in the self-service tests for both the explicit and fallback client-redirect cases. test: cover confirmation password/redirect behavior end to end Add coverage for the self-service confirmation changes: - EmailAccountCreationServiceTests: assert completeActivation discards the registration-form password (replacing it with a different value), marks the account as requiring a password change, and issues an ownership-bound reset code. - CreateAccountIT: end-to-end test confirming that the registration-form password cannot sign in after confirmation, and that only the password the owner sets through the link works. Also completes the configuration-reference note to describe that a configured client/signup redirect is applied after the owner sets their password. --- docs/UAA-Configuration-Reference.md | 4 ++- .../account/EmailAccountCreationService.java | 4 ++- .../EmailAccountCreationServiceTests.java | 24 +++++++++++++ .../integration/feature/CreateAccountIT.java | 34 +++++++++++++++++++ .../login/AccountsControllerMockMvcTests.java | 19 +++++++---- ...ccountsControllerMockMvcZonePathTests.java | 19 +++++++---- 6 files changed, 88 insertions(+), 16 deletions(-) diff --git a/docs/UAA-Configuration-Reference.md b/docs/UAA-Configuration-Reference.md index 69bfa781128..d047daa6942 100644 --- a/docs/UAA-Configuration-Reference.md +++ b/docs/UAA-Configuration-Reference.md @@ -1763,7 +1763,9 @@ When `true`, "Create Account" and "Forgot Password" links are displayed on the l > registration form. The owner is sent to a password-setup page (stating that a password change is > required due to changes in the system) and must establish their own password through the link > before the account can be used to sign in. This ensures a password chosen before the email address -> was verified can never be used to authenticate. +> was verified can never be used to authenticate. Any client or signup redirect configured for the +> registration (an explicit `redirect_uri` or the client's `signup_redirect_url`) is applied after the +> owner has set their password, provided it matches one of the client's registered redirect URIs. [Back to table](#login--branding) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/account/EmailAccountCreationService.java b/server/src/main/java/org/cloudfoundry/identity/uaa/account/EmailAccountCreationService.java index 785b0567a13..c9a1127839a 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/account/EmailAccountCreationService.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/account/EmailAccountCreationService.java @@ -138,11 +138,13 @@ public AccountCreationResponse completeActivation(String code) { String clientId = data.get("client_id"); String redirectUri = data.get("redirect_uri") != null ? data.get("redirect_uri") : ""; + // Resolve the signup redirect (honouring the client's signup_redirect_url fallback) so it is + // preserved across the reset flow and applied once the owner has set their password. String redirectLocation = getRedirect(clientId, redirectUri); // Issue an ownership-bound, single-use code so the owner sets their password through the // same reset flow used for password recovery (no previous password is required). - ForgotPasswordInfo forgotPasswordInfo = resetPasswordService.forgotPassword(user.getUserName(), clientId, redirectUri); + ForgotPasswordInfo forgotPasswordInfo = resetPasswordService.forgotPassword(user.getUserName(), clientId, redirectLocation); AccountCreationResponse response = new AccountCreationResponse(user.getId(), user.getUserName(), user.getUserName(), redirectLocation); response.setPasswordResetCode(forgotPasswordInfo.getResetPasswordCode().getCode()); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/login/EmailAccountCreationServiceTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/login/EmailAccountCreationServiceTests.java index be75e701144..69de4bc5233 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/login/EmailAccountCreationServiceTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/login/EmailAccountCreationServiceTests.java @@ -52,6 +52,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.cloudfoundry.identity.uaa.codestore.ExpiringCodeType.REGISTRATION; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.Mockito.anyInt; import static org.mockito.Mockito.anyString; import static org.mockito.Mockito.doThrow; @@ -267,6 +268,29 @@ void completeActivation() { assertThat(accountCreation.getUserId()).isNotNull(); } + @Test + void completeActivationDiscardsRegistrationPasswordAndRequiresPasswordReset() { + setUpForSuccess(""); + when(mockCodeStore.retrieveCode("the_secret_code", currentIdentityZoneId)).thenReturn(code); + when(mockScimUserProvisioning.retrieve(anyString(), eq(currentIdentityZoneId))).thenReturn(user); + when(mockScimUserProvisioning.verifyUser(anyString(), anyInt(), eq(currentIdentityZoneId))).thenReturn(user); + when(mockClientDetailsService.loadClientByClientId(anyString(), anyString())).thenReturn(mockClientDetails); + + AccountCreationService.AccountCreationResponse accountCreation = emailAccountCreationService.completeActivation("the_secret_code"); + + // The password supplied on the registration form must be discarded (replaced with a value + // that is not it), so it can never be used to authenticate once the account is confirmed. + ArgumentCaptor passwordCaptor = ArgumentCaptor.forClass(String.class); + verify(mockScimUserProvisioning).changePassword(eq("newly-created-user-id"), isNull(), passwordCaptor.capture(), eq(currentIdentityZoneId)); + assertThat(passwordCaptor.getValue()).isNotEqualTo("password"); + + // The account must require a password change, and an ownership-bound reset code must be + // issued so the confirming owner can set their own password through the link. + verify(mockScimUserProvisioning).updatePasswordChangeRequired("newly-created-user-id", true, currentIdentityZoneId); + verify(mockResetPasswordService).forgotPassword(eq("user@example.com"), anyString(), anyString()); + assertThat(accountCreation.getPasswordResetCode()).isEqualTo("reset_password_code"); + } + @Test void completeActivation_usesAntPathMatching() { setUpForSuccess("http://redirect.uri/"); diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/CreateAccountIT.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/CreateAccountIT.java index dd1ee4a143c..bdd63968a5a 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/CreateAccountIT.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/CreateAccountIT.java @@ -110,6 +110,40 @@ void userInitiatedSignup() throws Exception { assertThat(webDriver.findElement(By.cssSelector("h1")).getText()).contains("Where to?"); } + @Test + void confirmationDoesNotActivateTheRegistrationFormPassword() throws Exception { + String registrationPassword = SECRET; + String ownerPassword = "0wnerPass"; + + int receivedEmailSize = greenMail.getReceivedMessages().length; + String userEmail = startCreateUserFlow(registrationPassword); + + greenMail.waitForIncomingEmail(5000, receivedEmailSize + 1); + MimeMessage message = greenMail.getReceivedMessages()[receivedEmailSize]; + String link = testClient.extractLink(GreenMailUtil.getBody(message)); + + webDriver.get(link); + // Confirmation requires the owner to establish their own password through the link. + assertThat(webDriver.findElement(By.cssSelector("h1")).getText()).isEqualTo("Reset Password"); + webDriver.findElement(By.name("password")).sendKeys(ownerPassword); + webDriver.findElement(By.name("password_confirmation")).sendKeys(ownerPassword); + webDriver.clickAndWait(By.xpath("//input[@value='Create new password']")); + + // The password typed on the registration form must not grant access. + webDriver.get(baseUrl + "/login"); + webDriver.findElement(By.name("username")).sendKeys(userEmail); + webDriver.findElement(By.name("password")).sendKeys(registrationPassword); + webDriver.clickAndWait(By.xpath("//input[@value='Sign in']")); + assertThat(webDriver.findElement(By.className("alert-error")).getText()).contains("Provided credentials are invalid. Please try again."); + + // Only the password the owner set through the link works. + webDriver.get(baseUrl + "/login"); + webDriver.findElement(By.name("username")).sendKeys(userEmail); + webDriver.findElement(By.name("password")).sendKeys(ownerPassword); + webDriver.clickAndWait(By.xpath("//input[@value='Sign in']")); + assertThat(webDriver.findElement(By.cssSelector("h1")).getText()).contains("Where to?"); + } + @Test void clientInitiatedSignup() throws Exception { String userEmail = "user" + new SecureRandom().nextInt() + "@example.com"; diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcTests.java index 91c68488463..afc20734b53 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcTests.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcTests.java @@ -411,12 +411,12 @@ void creatingAnAccountWithAnEmptyClientId() throws Exception { @Test void creatingAnAccountWithClientRedirect() throws Exception { - createAccount("http://redirect.uri/client"); + createAccount("http://redirect.uri/client", "http://redirect.uri/client"); } @Test void creatingAnAccountWithFallbackClientRedirect() throws Exception { - createAccount(null); + createAccount("http://redirect.uri/fallback", null); } @Test @@ -699,7 +699,7 @@ private UaaClientDetails createTestClient() throws Exception { return MockMvcUtils.createClient(mockMvc, adminToken, clientDetails); } - private void createAccount(String redirectUri) throws Exception { + private void createAccount(String expectedRedirectUri, String redirectUri) throws Exception { PredictableGenerator generator = new PredictableGenerator(); JdbcExpiringCodeStore store = webApplicationContext.getBean(JdbcExpiringCodeStore.class); store.setGenerator(generator); @@ -719,7 +719,8 @@ private void createAccount(String redirectUri) throws Exception { assertThat(message.getContentString()).contains("Cloud Foundry"); assertThat(message.getMessage().getHeader("From")).contains("Cloud Foundry "); - confirmAccountAndSetPassword("test" + generator.counter.get(), "", userEmail); + String afterReset = confirmAccountAndSetPassword("test" + generator.counter.get(), "", userEmail); + assertThat(afterReset).isEqualTo("/login?success=password_reset&form_redirect_uri=" + expectedRedirectUri); MvcResult mvcResult = loginWithAccount("") .andExpect(authenticated()) @@ -739,7 +740,7 @@ private void createAccount(String redirectUri) throws Exception { * that flow, confirming the account via {@code /verify_user} and setting the password to * {@link #USER_PASSWORD} so the account can then be used to log in. */ - private void confirmAccountAndSetPassword(String activationCode, String subdomain, String email) throws Exception { + private String confirmAccountAndSetPassword(String activationCode, String subdomain, String email) throws Exception { MockHttpServletRequestBuilder verify = get("/verify_user").param("code", activationCode); if (hasText(subdomain)) { verify.with(new SetServerNameRequestPostProcessor(subdomain + ".localhost")); @@ -763,8 +764,12 @@ private void confirmAccountAndSetPassword(String activationCode, String subdomai if (hasText(subdomain)) { reset.with(new SetServerNameRequestPostProcessor(subdomain + ".localhost")); } - mockMvc.perform(reset) - .andExpect(status().isFound()); + // Returns the post-reset redirect so callers can assert the carried signup redirect. + return mockMvc.perform(reset) + .andExpect(status().isFound()) + .andReturn() + .getResponse() + .getRedirectedUrl(); } private ResultActions loginWithAccount(String subdomain) throws Exception { diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcZonePathTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcZonePathTests.java index dfc573c7ad4..a421da578f1 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcZonePathTests.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/AccountsControllerMockMvcZonePathTests.java @@ -349,12 +349,12 @@ void creatingAnAccountWithAnEmptyClientId() throws Exception { @Test void creatingAnAccountWithClientRedirect() throws Exception { - createAccount("http://redirect.uri/client"); + createAccount("http://redirect.uri/client", "http://redirect.uri/client"); } @Test void creatingAnAccountWithFallbackClientRedirect() throws Exception { - createAccount(null); + createAccount("http://redirect.uri/fallback", null); } @Test @@ -642,7 +642,7 @@ private UaaClientDetails createTestClient() throws Exception { return MockMvcUtils.createClient(mockMvc, adminToken, clientDetails); } - private void createAccount(String redirectUri) throws Exception { + private void createAccount(String expectedRedirectUri, String redirectUri) throws Exception { PredictableGenerator generator = new PredictableGenerator(); JdbcExpiringCodeStore store = webApplicationContext.getBean(JdbcExpiringCodeStore.class); store.setGenerator(generator); @@ -662,7 +662,8 @@ private void createAccount(String redirectUri) throws Exception { assertThat(message.getContentString()).contains("Cloud Foundry"); assertThat(message.getMessage().getHeader("From")).contains("Cloud Foundry "); - confirmAccountAndSetPassword(ZoneResolutionMode.SUBDOMAIN, "", "test" + generator.counter.get(), userEmail); + String afterReset = confirmAccountAndSetPassword(ZoneResolutionMode.SUBDOMAIN, "", "test" + generator.counter.get(), userEmail); + assertThat(afterReset).isEqualTo("/login?success=password_reset&form_redirect_uri=" + expectedRedirectUri); MvcResult mvcResult = loginWithAccount("") .andExpect(authenticated()) @@ -682,7 +683,7 @@ private void createAccount(String redirectUri) throws Exception { * that flow (zone-resolution aware), confirming the account via {@code /verify_user} and setting * the password to {@link #USER_PASSWORD} so the account can then be used to log in. */ - private void confirmAccountAndSetPassword(ZoneResolutionMode mode, String subdomain, String activationCode, String email) throws Exception { + private String confirmAccountAndSetPassword(ZoneResolutionMode mode, String subdomain, String activationCode, String email) throws Exception { MvcResult verifyResult = mockMvc.perform(mode.createRequestBuilder(subdomain != null ? subdomain : "", HttpMethod.GET, "/verify_user") .param("code", activationCode)) .andExpect(status().isFound()) @@ -696,13 +697,17 @@ private void confirmAccountAndSetPassword(ZoneResolutionMode mode, String subdom .contains("force_change=true"); String resetCode = UriComponentsBuilder.fromUriString(location).build().getQueryParams().getFirst("code"); - mockMvc.perform(mode.createRequestBuilder(subdomain != null ? subdomain : "", HttpMethod.POST, "/reset_password.do") + // Returns the post-reset redirect so callers can assert the carried signup redirect. + return mockMvc.perform(mode.createRequestBuilder(subdomain != null ? subdomain : "", HttpMethod.POST, "/reset_password.do") .with(cookieCsrf()) .param("code", resetCode) .param("email", email) .param("password", USER_PASSWORD) .param("password_confirmation", USER_PASSWORD)) - .andExpect(status().isFound()); + .andExpect(status().isFound()) + .andReturn() + .getResponse() + .getRedirectedUrl(); } private ResultActions loginWithAccount(String subdomain) throws Exception { From 6b462801f495a2599ac4d518e45f3d2a42e263fc Mon Sep 17 00:00:00 2001 From: Filip Hanik Date: Wed, 7 Oct 2026 13:38:59 -0700 Subject: [PATCH 4/4] fix: verify the account only after discarding the registration password completeActivation verified the account before replacing the registration password and setting the change-required flag. Because these are separate provisioning updates, a concurrent login in the gap could authenticate with the registration password against the already-verified account. Replace the password and set the flag while the account is still unverified, then verify it last (version -1, matching the password-reset flow), so the account is never simultaneously verified and holding the registration password. Add an ordering assertion to the service test to lock this in. Addresses PR review feedback. --- .../uaa/account/EmailAccountCreationService.java | 5 ++++- .../login/EmailAccountCreationServiceTests.java | 16 +++++++++++----- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/account/EmailAccountCreationService.java b/server/src/main/java/org/cloudfoundry/identity/uaa/account/EmailAccountCreationService.java index c9a1127839a..3b3603ec3bb 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/account/EmailAccountCreationService.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/account/EmailAccountCreationService.java @@ -127,14 +127,17 @@ public AccountCreationResponse completeActivation(String code) { }); String zoneId = identityZoneManager.getCurrentIdentityZoneId(); ScimUser user = scimUserProvisioning.retrieve(data.get("user_id"), zoneId); - user = scimUserProvisioning.verifyUser(user.getId(), user.getVersion(), zoneId); // Confirming the activation link proves ownership of the email address, but it does not // prove that the person confirming is the one who chose the password on the registration // form. Discard the registration password so it can never authenticate, and require the // confirming owner to establish their own password through the ownership-proving link. + // Replace the password and set the flag while the account is still unverified, then verify + // it last: these are separate updates, so verifying first would briefly leave the account + // verified while still holding the registration password, which a concurrent login could use. scimUserProvisioning.changePassword(user.getId(), null, new AlphanumericRandomValueStringGenerator(40).generate(), zoneId); scimUserProvisioning.updatePasswordChangeRequired(user.getId(), true, zoneId); + user = scimUserProvisioning.verifyUser(user.getId(), -1, zoneId); String clientId = data.get("client_id"); String redirectUri = data.get("redirect_uri") != null ? data.get("redirect_uri") : ""; diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/login/EmailAccountCreationServiceTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/login/EmailAccountCreationServiceTests.java index 69de4bc5233..edf7870ebe8 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/login/EmailAccountCreationServiceTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/login/EmailAccountCreationServiceTests.java @@ -32,6 +32,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.ArgumentCaptor; +import org.mockito.InOrder; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.http.HttpStatus; @@ -54,6 +55,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.Mockito.anyInt; +import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.anyString; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.eq; @@ -279,14 +281,18 @@ void completeActivationDiscardsRegistrationPasswordAndRequiresPasswordReset() { AccountCreationService.AccountCreationResponse accountCreation = emailAccountCreationService.completeActivation("the_secret_code"); // The password supplied on the registration form must be discarded (replaced with a value - // that is not it), so it can never be used to authenticate once the account is confirmed. + // that is not it) and the change-required flag set while the account is still unverified, + // and the account must be verified last. Verifying first would briefly leave the account + // verified while still accepting the registration password. ArgumentCaptor passwordCaptor = ArgumentCaptor.forClass(String.class); - verify(mockScimUserProvisioning).changePassword(eq("newly-created-user-id"), isNull(), passwordCaptor.capture(), eq(currentIdentityZoneId)); + InOrder inOrder = inOrder(mockScimUserProvisioning); + inOrder.verify(mockScimUserProvisioning).changePassword(eq("newly-created-user-id"), isNull(), passwordCaptor.capture(), eq(currentIdentityZoneId)); + inOrder.verify(mockScimUserProvisioning).updatePasswordChangeRequired("newly-created-user-id", true, currentIdentityZoneId); + inOrder.verify(mockScimUserProvisioning).verifyUser(eq("newly-created-user-id"), anyInt(), eq(currentIdentityZoneId)); assertThat(passwordCaptor.getValue()).isNotEqualTo("password"); - // The account must require a password change, and an ownership-bound reset code must be - // issued so the confirming owner can set their own password through the link. - verify(mockScimUserProvisioning).updatePasswordChangeRequired("newly-created-user-id", true, currentIdentityZoneId); + // An ownership-bound reset code must be issued so the confirming owner can set their own + // password through the link. verify(mockResetPasswordService).forgotPassword(eq("user@example.com"), anyString(), anyString()); assertThat(accountCreation.getPasswordResetCode()).isEqualTo("reset_password_code"); }