From 535c1b1b9099c688dec101700319e0cef3cc1c4a Mon Sep 17 00:00:00 2001 From: Radoslaw Orlowski Date: Fri, 1 May 2020 13:59:21 +0100 Subject: [PATCH 1/5] Make /activate POST request redirect to a GET endpoint that can have its language changes. --- .../hmcts/reform/idam/web/UserController.java | 51 ++++++++++++------- .../reform/idam/web/AppControllerTest.java | 2 +- .../reform/idam/web/UserControllerTest.java | 7 +-- .../reform/idam/web/util/TestConstants.java | 5 +- 4 files changed, 40 insertions(+), 25 deletions(-) diff --git a/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java b/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java index 383a92f00..cd4705a3e 100644 --- a/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java +++ b/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java @@ -4,6 +4,7 @@ import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ObjectNode; +import com.google.common.collect.ImmutableMap; import lombok.extern.slf4j.Slf4j; import org.bouncycastle.util.encoders.Base64; import org.owasp.encoder.Encode; @@ -16,11 +17,13 @@ import org.springframework.validation.annotation.Validated; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.ModelAttribute; +import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.client.HttpClientErrorException; import org.springframework.web.client.HttpServerErrorException; +import org.springframework.web.servlet.ModelAndView; import uk.gov.hmcts.reform.idam.api.internal.model.ActivationResult; import uk.gov.hmcts.reform.idam.api.internal.model.ErrorResponse; import uk.gov.hmcts.reform.idam.api.internal.model.Service; @@ -32,19 +35,14 @@ import uk.gov.hmcts.reform.idam.web.strategic.ValidationService; import java.io.IOException; +import java.util.HashMap; import java.util.Map; import java.util.Objects; import java.util.Optional; import static org.apache.commons.lang3.StringUtils.isBlank; import static org.apache.commons.lang3.StringUtils.isEmpty; -import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.CLIENTID; -import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.ERRORPAGE_VIEW; -import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.EXPIRED_ACTIVATION_LINK_VIEW; -import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.REDIRECTURI; -import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.SCOPE; -import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.SELF_REGISTER_VIEW; -import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.STATE; +import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.*; /** * @author Ivano @@ -83,9 +81,9 @@ public String users(final Map model) { * @should return expiredtoken view and have redirect_uri attribute in model if token expired * @should return useractivation view and no redirect_uri attribute in model if the token is valid * @should return errorpage view error message and no redirect_uri attribute in model if api returns server error - * @should return errorpage view error message for alredy activated account and no redirect_uri attribute in model if api returns status 409 + * @should return errorpage view error message for already activated account and no redirect_uri attribute in model if api returns status 409 */ - @RequestMapping(path = "/register", method = RequestMethod.GET) + @GetMapping("/register") public String userActivation(@RequestParam("token") String token, @RequestParam("code") String code, final Map model) { ValidateRequest validateRequest = new ValidateRequest(); validateRequest.setCode(code); @@ -253,9 +251,10 @@ public String selfRegisterUser(@ModelAttribute("selfRegisterCommand") @Validated * @should return expiredtoken view if HttpClientErrorException occurs and http status is 400 and token is invalid * @should return redirect expiredtoken page if selfRegisterUser service throws HttpClientErrorException and Http code is 404 */ - @RequestMapping(path = "/activate", method = RequestMethod.POST) - public String activateUser(@RequestParam("token") String token, @RequestParam("code") String code, @RequestParam("password1") String password1, @RequestParam("password2") String password2, - final Map model) throws IOException { + @PostMapping("/activate") + public ModelAndView activateUser(@RequestParam("token") String token, @RequestParam("code") String code, + @RequestParam("password1") String password1, @RequestParam("password2") String password2, + final Map model) throws IOException { model.put("token", token); model.put("code", code); try { @@ -263,15 +262,18 @@ public String activateUser(@RequestParam("token") String token, @RequestParam("c String activation = "{\"token\":\"" + token + "\",\"code\":\"" + code + "\",\"password\":\"" + password1 + "\"}"; ResponseEntity response = spiService.activateUser(activation); String redirectUri = getRedirectUri(response.getBody()); + // don't expose parameters other than the url to a GET request + Map successModel = new HashMap<>(); if (redirectUri != null) { - model.put("redirectUri", redirectUri); + successModel.put("redirectUri", redirectUri); } - return "useractivated"; + return new ModelAndView("redirect:useractivated", successModel); } } catch (HttpClientErrorException e) { if (e.getStatusCode() == HttpStatus.NOT_FOUND) { - return "redirect:expiredtoken"; + // don't expose the token in the error page + return new ModelAndView("redirect:expiredtoken", ImmutableMap.of()); } if (e.getStatusCode() == HttpStatus.BAD_REQUEST) { @@ -281,7 +283,7 @@ public String activateUser(@RequestParam("token") String token, @RequestParam("c "public.common.error.blacklisted.password", "public.common.error.enter.password", model); - return "useractivation"; + return new ModelAndView("useractivation"); } if (validationService.isErrorInResponse(e.getResponseBodyAsString(), ErrorResponse.CodeEnum.PASSWORD_CONTAINS_PERSONAL_INFO)) { @@ -290,11 +292,11 @@ public String activateUser(@RequestParam("token") String token, @RequestParam("c "public.common.error.containspersonalinfo.password", "public.common.error.enter.password", model); - return "useractivation"; + return new ModelAndView("useractivation"); } if (validationService.isErrorInResponse(e.getResponseBodyAsString(), ErrorResponse.CodeEnum.TOKEN_INVALID)) { - return "expiredtoken"; + return new ModelAndView("redirect:expiredtoken", model); } } @@ -305,7 +307,18 @@ public String activateUser(@RequestParam("token") String token, @RequestParam("c model); } - return "useractivation"; + return new ModelAndView("useractivation"); + } + + @GetMapping("/useractivated") + public String userActivated(@RequestParam final String redirectUri, final Map model) { + model.put("redirectUri", redirectUri); + return "useractivated"; + } + + @GetMapping("/expiredtoken") + public String expiredToken(final Map model) { + return "expiredtoken"; } private String getRedirectUri(String json) throws IOException { diff --git a/src/test/java/uk/gov/hmcts/reform/idam/web/AppControllerTest.java b/src/test/java/uk/gov/hmcts/reform/idam/web/AppControllerTest.java index e3df17064..2a7cc54be 100644 --- a/src/test/java/uk/gov/hmcts/reform/idam/web/AppControllerTest.java +++ b/src/test/java/uk/gov/hmcts/reform/idam/web/AppControllerTest.java @@ -1035,7 +1035,7 @@ public void resetPassword_shouldRedirectToExpiredTokenIfHttpClientErrorException .param(TOKEN_PARAMETER, RESET_PASSWORD_TOKEN) .param(CODE_PARAMETER, RESET_PASSWORD_CODE)) .andExpect(status().is3xxRedirection()) - .andExpect(redirectedUrl(EXPIREDTOKEN_VIEW_NAME)); + .andExpect(redirectedUrl(EXPIREDTOKEN_REDIRECTED_VIEW_NAME)); verify(spiService).resetPassword(eq(PASSWORD_ONE), eq(RESET_PASSWORD_TOKEN), eq(RESET_PASSWORD_CODE)); } diff --git a/src/test/java/uk/gov/hmcts/reform/idam/web/UserControllerTest.java b/src/test/java/uk/gov/hmcts/reform/idam/web/UserControllerTest.java index c95e968c1..5460709e9 100644 --- a/src/test/java/uk/gov/hmcts/reform/idam/web/UserControllerTest.java +++ b/src/test/java/uk/gov/hmcts/reform/idam/web/UserControllerTest.java @@ -55,6 +55,7 @@ import static uk.gov.hmcts.reform.idam.web.util.TestConstants.ERROR_MSG; import static uk.gov.hmcts.reform.idam.web.util.TestConstants.ERROR_TITLE; import static uk.gov.hmcts.reform.idam.web.util.TestConstants.ERROR_VIEW_NAME; +import static uk.gov.hmcts.reform.idam.web.util.TestConstants.EXPIREDTOKEN_REDIRECTED_VIEW_NAME; import static uk.gov.hmcts.reform.idam.web.util.TestConstants.EXPIREDTOKEN_VIEW_NAME; import static uk.gov.hmcts.reform.idam.web.util.TestConstants.EXPIRED_ACTIVATION_TOKEN_VIEW_NAME; import static uk.gov.hmcts.reform.idam.web.util.TestConstants.FORM_DATA; @@ -295,7 +296,7 @@ public void activateUser_shouldReturnUseractivatedViewAndRedirectUriInModelIfRet given(spiService.activateUser(eq("{\"token\":\"" + USER_ACTIVATION_TOKEN + "\",\"code\":\"" + USER_ACTIVATION_CODE + "\",\"password\":\"" + USER_PASSWORD + "\"}"))).willReturn(ResponseEntity.ok("{\"redirectUri\":\"" + REDIRECT_URI + "\"}")); mockMvc.perform(getActivateUserPostRequest(USER_ACTIVATION_TOKEN, USER_ACTIVATION_CODE, USER_PASSWORD, USER_PASSWORD)) - .andExpect(status().isOk()) + .andExpect(status().is3xxRedirection()) .andExpect(model().attribute(REDIRECTURI, REDIRECT_URI)) .andExpect(view().name(USER_ACTIVATED_VIEW_NAME)); } @@ -350,7 +351,7 @@ public void activateUser_shouldReturnExpiredtokenViewIfHttpClientErrorExceptionO given(spiService.activateUser(eq("{\"token\":\"" + USER_ACTIVATION_TOKEN + "\",\"code\":\"" + USER_ACTIVATION_CODE + "\",\"password\":\"" + USER_PASSWORD + "\"}"))).willThrow(new HttpClientErrorException(HttpStatus.BAD_REQUEST, "Bad Request", TOKEN_INVALID_RESPONSE.getBytes(), null)); given(validationService.isErrorInResponse(eq(TOKEN_INVALID_RESPONSE), eq(ErrorResponse.CodeEnum.TOKEN_INVALID))).willReturn(true); mockMvc.perform(getActivateUserPostRequest(USER_ACTIVATION_TOKEN, USER_ACTIVATION_CODE, USER_PASSWORD, USER_PASSWORD)) - .andExpect(status().isOk()) + .andExpect(status().is3xxRedirection()) .andExpect(view().name(EXPIREDTOKEN_VIEW_NAME)); } @@ -384,7 +385,7 @@ public void activateUser_shouldReturnRedirectExpiredtokenPageIfSelfRegisterUserS mockMvc.perform(getActivateUserPostRequest(USER_ACTIVATION_TOKEN, USER_ACTIVATION_CODE, USER_PASSWORD, USER_PASSWORD)) .andExpect(status().is3xxRedirection()) - .andExpect(redirectedUrl(EXPIREDTOKEN_VIEW_NAME)); + .andExpect(redirectedUrl(EXPIREDTOKEN_REDIRECTED_VIEW_NAME)); } diff --git a/src/test/java/uk/gov/hmcts/reform/idam/web/util/TestConstants.java b/src/test/java/uk/gov/hmcts/reform/idam/web/util/TestConstants.java index 366e52e09..86a018c52 100644 --- a/src/test/java/uk/gov/hmcts/reform/idam/web/util/TestConstants.java +++ b/src/test/java/uk/gov/hmcts/reform/idam/web/util/TestConstants.java @@ -63,11 +63,12 @@ public class TestConstants { public static final String EXPIRED_PASSWORD_RESET_TOKEN_VIEW_NAME = "expiredPasswordResetLink"; public static final String EXPIRED_ACTIVATION_TOKEN_VIEW_NAME = "expiredActivationLink"; public static final String USER_ACTIVATION_VIEW_NAME = "useractivation"; - public static final String USER_ACTIVATED_VIEW_NAME = "useractivated"; + public static final String USER_ACTIVATED_VIEW_NAME = "redirect:useractivated"; public static final String ERROR_VIEW_NAME = "errorpage"; public static final String USER_CREATED_VIEW_NAME = "usercreated"; public static final String RESETPASSWORD_VIEW_NAME = "resetpassword"; - public static final String EXPIREDTOKEN_VIEW_NAME = "expiredtoken"; + public static final String EXPIREDTOKEN_VIEW_NAME = "redirect:expiredtoken"; + public static final String EXPIREDTOKEN_REDIRECTED_VIEW_NAME = "expiredtoken"; public static final String FORGOT_PASSWORD_VIEW = "forgotpassword"; public static final String FORGOT_PASSWORD_SUCCESS_VIEW = "forgotpasswordsuccess"; public static final String RESET_PASSWORD_SUCCESS_VIEW = "resetpasswordsuccess"; From 8c9ee0e73a1d1170632a150a69cb34c1192c539b Mon Sep 17 00:00:00 2001 From: Radoslaw Orlowski Date: Fri, 1 May 2020 14:01:29 +0100 Subject: [PATCH 2/5] Restore individual imports. --- .../java/uk/gov/hmcts/reform/idam/web/UserController.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java b/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java index cd4705a3e..fa186b97e 100644 --- a/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java +++ b/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java @@ -42,7 +42,13 @@ import static org.apache.commons.lang3.StringUtils.isBlank; import static org.apache.commons.lang3.StringUtils.isEmpty; -import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.*; +import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.CLIENTID; +import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.ERRORPAGE_VIEW; +import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.EXPIRED_ACTIVATION_LINK_VIEW; +import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.REDIRECTURI; +import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.SCOPE; +import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.SELF_REGISTER_VIEW; +import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.STATE; /** * @author Ivano From db34f966ad8686c30fa8466e152fdcb168a19908 Mon Sep 17 00:00:00 2001 From: Radoslaw Orlowski Date: Fri, 15 May 2020 09:59:44 +0100 Subject: [PATCH 3/5] Code review changes. --- .../java/uk/gov/hmcts/reform/idam/web/UserController.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java b/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java index fa186b97e..7caab76ed 100644 --- a/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java +++ b/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java @@ -4,7 +4,6 @@ import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ObjectNode; -import com.google.common.collect.ImmutableMap; import lombok.extern.slf4j.Slf4j; import org.bouncycastle.util.encoders.Base64; import org.owasp.encoder.Encode; @@ -50,9 +49,6 @@ import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.SELF_REGISTER_VIEW; import static uk.gov.hmcts.reform.idam.web.helper.MvcKeys.STATE; -/** - * @author Ivano - */ @Controller @RequestMapping("/users") @Slf4j @@ -279,7 +275,7 @@ public ModelAndView activateUser(@RequestParam("token") String token, @RequestPa } catch (HttpClientErrorException e) { if (e.getStatusCode() == HttpStatus.NOT_FOUND) { // don't expose the token in the error page - return new ModelAndView("redirect:expiredtoken", ImmutableMap.of()); + return new ModelAndView("redirect:expiredtoken", (Map) null); } if (e.getStatusCode() == HttpStatus.BAD_REQUEST) { From edb6d7ceaf2c31e5d6b4d6e5fe54d181127f6948 Mon Sep 17 00:00:00 2001 From: Radoslaw Orlowski Date: Fri, 15 May 2020 10:20:52 +0100 Subject: [PATCH 4/5] Fix broken functional test. --- .../java/uk/gov/hmcts/reform/idam/web/UserController.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java b/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java index 7caab76ed..1755e229b 100644 --- a/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java +++ b/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java @@ -313,8 +313,10 @@ public ModelAndView activateUser(@RequestParam("token") String token, @RequestPa } @GetMapping("/useractivated") - public String userActivated(@RequestParam final String redirectUri, final Map model) { - model.put("redirectUri", redirectUri); + public String userActivated(@RequestParam(required = false) final String redirectUri, final Map model) { + if (redirectUri != null) { + model.put("redirectUri", redirectUri); + } return "useractivated"; } From db708230018a2fbae40ada03a83a8c2204ed2415 Mon Sep 17 00:00:00 2001 From: Radoslaw Orlowski Date: Fri, 15 May 2020 11:39:52 +0100 Subject: [PATCH 5/5] Empty commit. --- src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java b/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java index 1755e229b..217894847 100644 --- a/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java +++ b/src/main/java/uk/gov/hmcts/reform/idam/web/UserController.java @@ -336,4 +336,4 @@ private String getRedirectUri(String json) throws IOException { return null; } } -} \ No newline at end of file +}