When the link is clicked for reset password, the code is exchanged so

that the link can not be used again
cloudfoundry · Aug 6, 2015 · cd31cc3 · cd31cc3
1 parent 7c70a85
commit cd31cc3
Show file tree

Hide file tree

Showing 11 changed files with 155 additions and 69 deletions.
diff --git a/common/src/main/java/org/cloudfoundry/identity/uaa/codestore/ExpiringCodeStore.java b/common/src/main/java/org/cloudfoundry/identity/uaa/codestore/ExpiringCodeStore.java
@@ -12,10 +12,10 @@
  *******************************************************************************/
 package org.cloudfoundry.identity.uaa.codestore;
 
-import java.sql.Timestamp;
-
 import org.springframework.security.oauth2.common.util.RandomValueStringGenerator;
 
+import java.sql.Timestamp;
+
 public interface ExpiringCodeStore {
 
     /**
@@ -26,7 +26,7 @@ public interface ExpiringCodeStore {
      * @throws java.lang.NullPointerException if data or expiresAt is null
      * @throws java.lang.IllegalArgumentException if expiresAt is in the past
      */
-    public ExpiringCode generateCode(String data, Timestamp expiresAt);
+    ExpiringCode generateCode(String data, Timestamp expiresAt);
 
     /**
      * Retrieve a code and delete it if it exists.
@@ -35,12 +35,12 @@ public interface ExpiringCodeStore {
      * @return code or null if the code is not found
      * @throws java.lang.NullPointerException if the code is null
      */
-    public ExpiringCode retrieveCode(String code);
+    ExpiringCode retrieveCode(String code);
 
     /**
      * Set the code generator for this store.
      * 
      * @param generator Code generator
      */
-    public void setGenerator(RandomValueStringGenerator generator);
+    void setGenerator(RandomValueStringGenerator generator);
 }
diff --git a/login/src/main/java/org/cloudfoundry/identity/uaa/#/ResetPasswordController.java b/login/src/main/java/org/cloudfoundry/identity/uaa/#/ResetPasswordController.java
@@ -16,6 +16,8 @@
 import org.apache.commons.logging.LogFactory;
 import org.cloudfoundry.identity.uaa.authentication.Origin;
 import org.cloudfoundry.identity.uaa.authentication.UaaPrincipal;
+import org.cloudfoundry.identity.uaa.codestore.ExpiringCode;
+import org.cloudfoundry.identity.uaa.codestore.ExpiringCodeStore;
 import org.cloudfoundry.identity.uaa.error.UaaException;
 import org.cloudfoundry.identity.uaa.scim.ScimUser;
 import org.cloudfoundry.identity.uaa.scim.exception.InvalidPasswordException;
@@ -36,9 +38,9 @@
 import org.thymeleaf.TemplateEngine;
 import org.thymeleaf.context.Context;
 
-import java.util.Map;
-import java.util.regex.Pattern;
 import javax.servlet.http.HttpServletResponse;
+import java.sql.Timestamp;
+import java.util.regex.Pattern;
 
 @Controller
 public class ResetPasswordController {
@@ -50,14 +52,21 @@ public class ResetPasswordController {
     private final UaaUrlUtils uaaUrlUtils;
     private final String brand;
     private final Pattern emailPattern;
-
-    public ResetPasswordController(ResetPasswordService resetPasswordService, MessageService messageService, TemplateEngine templateEngine, UaaUrlUtils uaaUrlUtils, String brand) {
+    private final ExpiringCodeStore codeStore;
+
+    public ResetPasswordController(ResetPasswordService resetPasswordService,
+                                   MessageService messageService,
+                                   TemplateEngine templateEngine,
+                                   UaaUrlUtils uaaUrlUtils,
+                                   String brand,
+                                   ExpiringCodeStore codeStore) {
         this.resetPasswordService = resetPasswordService;
         this.messageService = messageService;
         this.templateEngine = templateEngine;
         this.uaaUrlUtils = uaaUrlUtils;
         this.brand = brand;
         emailPattern = Pattern.compile("^\\S+@\\S+\\.\\S+$");
+        this.codeStore = codeStore;
     }
 
     @RequestMapping(value = "/forgot_password", method = RequestMethod.GET)
@@ -139,8 +148,20 @@ public String emailSentPage(@ModelAttribute("code") String code) {
     }
 
     @RequestMapping(value = "/reset_password", method = RequestMethod.GET, params = { "email", "code" })
-    public String resetPasswordPage() {
-        return "reset_password";
+    public String resetPasswordPage(Model model,
+                                    HttpServletResponse response,
+                                    @RequestParam("code") String code,
+                                    @RequestParam("email") String email) {
+
+        ExpiringCode expiringCode = codeStore.retrieveCode(code);
+        if (expiringCode==null) {
+            return handleUnprocessableEntity(model, response, "message_code", "bad_code");
+        } else {
+            Timestamp fiveMinutes = new Timestamp(System.currentTimeMillis()+(1000*60*5));
+            model.addAttribute("code", codeStore.generateCode(expiringCode.getData(), fiveMinutes).getCode());
+            model.addAttribute("email", email);
+            return "reset_password";
+        }
     }
 
     @RequestMapping(value = "/reset_password.do", method = RequestMethod.POST)
@@ -162,11 +183,9 @@ public String resetPassword(Model model,
 
         try {
             ScimUser user = resetPasswordService.resetPassword(code, password);
-
             UaaPrincipal uaaPrincipal = new UaaPrincipal(user.getId(), user.getUserName(), user.getPrimaryEmail(), Origin.UAA, null, IdentityZoneHolder.get().getId());
             UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken(uaaPrincipal, null, UaaAuthority.USER_AUTHORITIES);
             SecurityContextHolder.getContext().setAuthentication(token);
-
             return "redirect:home";
         } catch (UaaException e) {
             return handleUnprocessableEntity(model, response, "message_code", "bad_code");

diff --git a/login/src/main/resources/#-ui.xml b/login/src/main/resources/#-ui.xml
@@ -470,14 +470,15 @@
         <constructor-arg ref="mailTemplateEngine"/>
         <constructor-arg ref="uaaUrlUtils"/>
         <constructor-arg value="${login.brand:oss}"/>
+        <constructor-arg ref="codeStore"/>
     </bean>
 
     <bean id="changeEmailController" class="org.cloudfoundry.identity.uaa.login.ChangeEmailController">
         <constructor-arg ref="changeEmailService"/>
         <property name="uaaUserDatabase" ref="userDatabase"/>
     </bean>
 
-    <bean class="org.cloudfoundry.identity.uaa.login.UaaExpiringCodeService">
+    <bean class="org.cloudfoundry.identity.uaa.login.UaaExpiringCodeService" id="uaaExpiringCodeService">
         <constructor-arg ref="codeStore"/>
     </bean>
 

diff --git a/login/src/main/resources/templates/web/reset_password.html b/login/src/main/resources/templates/web/reset_password.html
@@ -8,10 +8,10 @@
 <div class="island" layout:fragment="page-content">
     <h1>Reset Password</h1>
     <div class="island-content">
-        <div th:text="|Email: ${param.email[0]}|" class="email-display">Email: user@example.com</div>
+        <div th:text="|Email: ${email}|" class="email-display">Email: user@example.com</div>
         <form th:action="@{/reset_password.do}" method="post" novalidate="novalidate">
-            <input type="hidden" name="code" th:value="${param.code[0]}"/>
-            <input type="hidden" name="email" th:value="${param.email[0]}"/>
+            <input type="hidden" name="code" th:value="${code}"/>
+            <input type="hidden" name="email" th:value="${email}"/>
             <div th:if="${message_code}" th:text="#{'reset_password.' + ${message_code}}" class="error-message"></div>
             <input name="password" type="password" placeholder="New Password" autocomplete="off" class="form-control"/>
             <input name="password_confirmation" type="password" placeholder="Confirm" autocomplete="off" class="form-control"/>

diff --git a/login/src/test/java/org/cloudfoundry/identity/uaa/#/ResetPasswordControllerTest.java b/login/src/test/java/org/cloudfoundry/identity/uaa/#/ResetPasswordControllerTest.java
@@ -14,6 +14,7 @@
 
 import org.cloudfoundry.identity.uaa.TestClassNullifier;
 import org.cloudfoundry.identity.uaa.codestore.ExpiringCode;
+import org.cloudfoundry.identity.uaa.codestore.ExpiringCodeStore;
 import org.cloudfoundry.identity.uaa.error.UaaException;
 import org.cloudfoundry.identity.uaa.login.test.ThymeleafConfig;
 import org.cloudfoundry.identity.uaa.scim.ScimMeta;
@@ -47,6 +48,8 @@
 import static com.google.common.collect.Lists.newArrayList;
 import static org.hamcrest.Matchers.containsString;
 import static org.junit.Assert.assertThat;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyString;
 import static org.mockito.Matchers.contains;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.mock;
@@ -67,6 +70,7 @@ public class ResetPasswordControllerTest extends TestClassNullifier {
     private MockMvc mockMvc;
     private ResetPasswordService resetPasswordService;
     private MessageService messageService;
+    private ExpiringCodeStore codeStore;
 
     @Autowired
     @Qualifier("mailTemplateEngine")
@@ -78,7 +82,8 @@ public void setUp() throws Exception {
         IdentityZoneHolder.set(IdentityZone.getUaa());
         resetPasswordService = mock(ResetPasswordService.class);
         messageService = mock(MessageService.class);
-        ResetPasswordController controller = new ResetPasswordController(resetPasswordService, messageService, templateEngine, new UaaUrlUtils("http://foo/uaa"), "pivotal");
+        codeStore = mock(ExpiringCodeStore.class);
+        ResetPasswordController controller = new ResetPasswordController(resetPasswordService, messageService, templateEngine, new UaaUrlUtils("http://foo/uaa"), "pivotal", codeStore);
 
         InternalResourceViewResolver viewResolver = new InternalResourceViewResolver();
         viewResolver.setPrefix("/WEB-INF/jsp");
@@ -200,6 +205,9 @@ public void testInstructions() throws Exception {
 
     @Test
     public void testResetPasswordPage() throws Exception {
+        ExpiringCode code = new ExpiringCode("code1", new Timestamp(System.currentTimeMillis()), "someData");
+        when(codeStore.generateCode(anyString(), any(Timestamp.class))).thenReturn(code);
+        when(codeStore.retrieveCode(anyString())).thenReturn(code);
         mockMvc.perform(get("/reset_password").param("email", "user@example.com").param("code", "secret_code"))
                 .andExpect(status().isOk())
                 .andExpect(view().name("reset_password"));

diff --git a/scim/src/main/java/org/cloudfoundry/identity/uaa/#/UaaResetPasswordService.java b/scim/src/main/java/org/cloudfoundry/identity/uaa/#/UaaResetPasswordService.java
@@ -70,16 +70,18 @@ private ScimUser changePasswordCodeAuthenticated(String code, String newPassword
         }
         String userId;
         String userName = null;
+        Date passwordLastModified = null;
         try {
             PasswordChange change = JsonUtils.readValue(expiringCode.getData(), PasswordChange.class);
             userId = change.getUserId();
             userName = change.getUsername();
+            passwordLastModified = change.getPasswordModifiedTime();
         } catch (JsonUtils.JsonUtilException x) {
             userId = expiringCode.getData();
         }
         ScimUser user = scimUserProvisioning.retrieve(userId);
         try {
-            if (isUserModified(user, expiringCode.getExpiresAt(), userName)) {
+            if (isUserModified(user, expiringCode.getExpiresAt(), userName, passwordLastModified)) {
                 throw new UaaException("Invalid password reset request.");
             }
             if (!user.isVerified()) {
@@ -110,22 +112,21 @@ public ForgotPasswordInfo forgotPassword(String email) {
             }
         }
         ScimUser scimUser = results.get(0);
-        PasswordChange change = new PasswordChange(scimUser.getId(), scimUser.getUserName());
+        PasswordChange change = new PasswordChange(scimUser.getId(), scimUser.getUserName(), scimUser.getPasswordLastModified());
         ExpiringCode code = expiringCodeStore.generateCode(JsonUtils.writeValueAsString(change), new Timestamp(System.currentTimeMillis() + PASSWORD_RESET_LIFETIME));
         publish(new ResetPasswordRequestEvent(email, code.getCode(), SecurityContextHolder.getContext().getAuthentication()));
         return new ForgotPasswordInfo(scimUser.getId(), code);
     }
 
-    private boolean isUserModified(ScimUser user, Timestamp expiresAt, String userName) {
+    private boolean isUserModified(ScimUser user, Timestamp expiresAt, String userName, Date passwordLastModified) {
+        boolean modified = false;
         if (userName!=null) {
-            return ! userName.equals(user.getUserName());
+            modified = ! (userName.equals(user.getUserName()));
         }
-        //left over from when all we stored in the code was the user ID
-        //here we will check the timestamp
-        //TODO - REMOVE THIS IN FUTURE RELEASE, ALL LINKS HAVE BEEN EXPIRED (except test created ones)
-        long codeCreated = expiresAt.getTime() - PASSWORD_RESET_LIFETIME;
-        long userModified = user.getMeta().getLastModified().getTime();
-        return (userModified > codeCreated);
+        if (passwordLastModified != null && (!modified)) {
+            modified = user.getPasswordLastModified().getTime() != passwordLastModified.getTime();
+        }
+        return modified;
     }
 
     private UaaUser getUaaUser(ScimUser scimUser) {

diff --git a/scim/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/PasswordChange.java b/scim/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/PasswordChange.java
@@ -3,13 +3,16 @@
 import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 import com.fasterxml.jackson.annotation.JsonProperty;
 
+import java.util.Date;
+
 @JsonIgnoreProperties(ignoreUnknown = true)
 public class PasswordChange {
     public PasswordChange() {}
 
-    public PasswordChange(String userId, String username) {
+    public PasswordChange(String userId, String username, Date passwordModifiedTime) {
         this.userId = userId;
         this.username = username;
+        this.passwordModifiedTime = passwordModifiedTime;
     }
 
     @JsonProperty("user_id")
@@ -18,6 +21,9 @@ public PasswordChange(String userId, String username) {
     @JsonProperty("username")
     private String username;
 
+    @JsonProperty("passwordModifiedTime")
+    private Date passwordModifiedTime;
+
     public String getUsername() {
         return username;
     }
@@ -33,4 +39,12 @@ public String getUserId() {
     public void setUserId(String userId) {
         this.userId = userId;
     }
+
+    public Date getPasswordModifiedTime() {
+        return passwordModifiedTime;
+    }
+
+    public void setPasswordModifiedTime(Date passwordModifiedTime) {
+        this.passwordModifiedTime = passwordModifiedTime;
+    }
 }
diff --git a/scim/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java b/scim/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java
@@ -269,7 +269,7 @@ public void changePassword(final String id, String oldPassword, final String new
         int updated = jdbcTemplate.update(CHANGE_PASSWORD_SQL, new PreparedStatementSetter() {
             @Override
             public void setValues(PreparedStatement ps) throws SQLException {
-                Timestamp t = new Timestamp(new Date().getTime());
+                Timestamp t = new Timestamp(System.currentTimeMillis());
                 ps.setTimestamp(1, t);
                 ps.setString(2, encNewPassword);
                 ps.setTimestamp(3, getPasswordLastModifiedTimestamp(t));

diff --git a/...src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/PasswordResetEndpointTest.java b/...src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/PasswordResetEndpointTest.java
@@ -17,8 +17,8 @@
 import org.cloudfoundry.identity.uaa.codestore.ExpiringCode;
 import org.cloudfoundry.identity.uaa.codestore.ExpiringCodeStore;
 import org.cloudfoundry.identity.uaa.error.ExceptionReportHttpMessageConverter;
-import org.cloudfoundry.identity.uaa.login.UaaResetPasswordService;
 import org.cloudfoundry.identity.uaa.login.ResetPasswordService;
+import org.cloudfoundry.identity.uaa.login.UaaResetPasswordService;
 import org.cloudfoundry.identity.uaa.scim.ScimMeta;
 import org.cloudfoundry.identity.uaa.scim.ScimUser;
 import org.cloudfoundry.identity.uaa.scim.ScimUserProvisioning;
@@ -67,6 +67,7 @@ public class PasswordResetEndpointTest extends TestClassNullifier {
     private ExpiringCodeStore expiringCodeStore;
     private PasswordValidator passwordValidator;
     private ResetPasswordService resetPasswordService;
+    Date yesterday = new Date(System.currentTimeMillis()-(1000*60*60*24));
 
     @Before
     public void setUp() throws Exception {
@@ -78,23 +79,26 @@ public void setUp() throws Exception {
         controller.setMessageConverters(new HttpMessageConverter[] { new ExceptionReportHttpMessageConverter() });
         mockMvc = MockMvcBuilders.standaloneSetup(controller).build();
 
+        PasswordChange change = new PasswordChange("id001", "user@example.com", yesterday);
+
         when(expiringCodeStore.generateCode(eq("id001"), any(Timestamp.class)))
                 .thenReturn(new ExpiringCode("secret_code", new Timestamp(System.currentTimeMillis() + UaaResetPasswordService.PASSWORD_RESET_LIFETIME), "id001"));
 
-        PasswordChange change = new PasswordChange("id001", "user@example.com");
+
         when(expiringCodeStore.generateCode(eq(JsonUtils.writeValueAsString(change)), any(Timestamp.class)))
-            .thenReturn(new ExpiringCode("secret_code", new Timestamp(System.currentTimeMillis() + UaaResetPasswordService.PASSWORD_RESET_LIFETIME), "id001"));
+            .thenReturn(new ExpiringCode("secret_code", new Timestamp(System.currentTimeMillis() + UaaResetPasswordService.PASSWORD_RESET_LIFETIME), JsonUtils.writeValueAsString(change)));
 
-        change = new PasswordChange("id001", "user\"'@example.com");
+        change = new PasswordChange("id001", "user\"'@example.com", yesterday);
         when(expiringCodeStore.generateCode(eq(JsonUtils.writeValueAsString(change)), any(Timestamp.class)))
-            .thenReturn(new ExpiringCode("secret_code", new Timestamp(System.currentTimeMillis() + UaaResetPasswordService.PASSWORD_RESET_LIFETIME), "id001"));
+            .thenReturn(new ExpiringCode("secret_code", new Timestamp(System.currentTimeMillis() + UaaResetPasswordService.PASSWORD_RESET_LIFETIME), JsonUtils.writeValueAsString(change)));
     }
 
     @Test
     public void testCreatingAPasswordResetWhenTheUsernameExists() throws Exception {
         ScimUser user = new ScimUser("id001", "user@example.com", null, null);
-        user.setMeta(new ScimMeta(new Date(System.currentTimeMillis()-(1000*60*60*24)), new Date(System.currentTimeMillis()-(1000*60*60*24)), 0));
+        user.setMeta(new ScimMeta(yesterday, yesterday, 0));
         user.addEmail("user@example.com");
+        user.setPasswordLastModified(yesterday);
         when(scimUserProvisioning.query("userName eq \"user@example.com\" and origin eq \"" + Origin.UAA + "\""))
                 .thenReturn(Arrays.asList(user));
 
@@ -148,7 +152,8 @@ public void testCreatingAPasswordResetWhenTheUserHasNonUaaOrigin() throws Except
     @Test
     public void testCreatingAPasswordResetWithAUsernameContainingSpecialCharacters() throws Exception {
         ScimUser user = new ScimUser("id001", "user\"'@example.com", null, null);
-        user.setMeta(new ScimMeta(new Date(System.currentTimeMillis()-(1000*60*60*24)), new Date(System.currentTimeMillis()-(1000*60*60*24)), 0));
+        user.setMeta(new ScimMeta(yesterday, yesterday, 0));
+        user.setPasswordLastModified(yesterday);
         user.addEmail("user\"'@example.com");
         when(scimUserProvisioning.query("userName eq \"user\\\"'@example.com\" and origin eq \"" + Origin.UAA + "\""))
             .thenReturn(Arrays.asList(user));

diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/ResetPasswordIT.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/ResetPasswordIT.java
@@ -120,10 +120,6 @@ public void resettingAPassword() throws Exception {
 
         webDriver.get(link);
 
-        webDriver.findElement(By.name("password")).sendKeys("newsecr3T");
-        webDriver.findElement(By.name("password_confirmation")).sendKeys("newsecr3T");
-        webDriver.findElement(By.xpath("//input[@value='Create new password']")).click();
-
         assertThat(webDriver.findElement(By.cssSelector(".error-message")).getText(), containsString("Sorry, your reset password link is no longer valid. You can request another one below."));
     }