Merge pull request #1540 from alphagov/auth-1480-email-validation-refactor

AUTH-1480 -  Validation refactor

Author: JHjava

account-management-api/src/main/java/uk/gov/di/accountmanagement/lambda/SendOtpNotificationHandler.java

@@ -24,7 +24,6 @@
 import uk.gov.di.authentication.shared.services.ConfigurationService;
 import uk.gov.di.authentication.shared.services.DynamoService;
 import uk.gov.di.authentication.shared.services.RedisConnectionService;
-import uk.gov.di.authentication.shared.services.ValidationService;
 
 import java.util.Optional;
 
@@ -44,7 +43,6 @@ public class SendOtpNotificationHandler
     private static final Logger LOG = LogManager.getLogger(SendOtpNotificationHandler.class);
 
     private final ConfigurationService configurationService;
-    private final ValidationService validationService;
     private final AwsSqsClient sqsClient;
     private final CodeGeneratorService codeGeneratorService;
     private final CodeStorageService codeStorageService;
@@ -54,14 +52,12 @@ public class SendOtpNotificationHandler
 
     public SendOtpNotificationHandler(
             ConfigurationService configurationService,
-            ValidationService validationService,
             AwsSqsClient sqsClient,
             CodeGeneratorService codeGeneratorService,
             CodeStorageService codeStorageService,
             DynamoService dynamoService,
             AuditService auditService) {
         this.configurationService = configurationService;
-        this.validationService = validationService;
         this.sqsClient = sqsClient;
         this.codeGeneratorService = codeGeneratorService;
         this.codeStorageService = codeStorageService;
@@ -76,7 +72,6 @@ public SendOtpNotificationHandler(ConfigurationService configurationService) {
                         configurationService.getAwsRegion(),
                         configurationService.getEmailQueueUri(),
                         configurationService.getSqsEndpointUri());
-        this.validationService = new ValidationService();
         this.codeGeneratorService = new CodeGeneratorService();
         this.codeStorageService =
                 new CodeStorageService(new RedisConnectionService(configurationService));
@@ -107,7 +102,7 @@ public APIGatewayProxyResponseEvent handleRequest(
                                     case VERIFY_EMAIL:
                                         LOG.info("NotificationType is VERIFY_EMAIL");
                                         Optional<ErrorResponse> emailErrorResponse =
-                                                validationService.validateEmailAddress(
+                                                ValidationHelper.validateEmailAddress(
                                                         sendNotificationRequest.getEmail());
                                         if (emailErrorResponse.isPresent()) {
                                             return generateApiGatewayProxyErrorResponse(

account-management-api/src/main/java/uk/gov/di/accountmanagement/lambda/UpdateEmailHandler.java

@@ -21,11 +21,11 @@
 import uk.gov.di.authentication.shared.helpers.PersistentIdHelper;
 import uk.gov.di.authentication.shared.helpers.RequestBodyHelper;
 import uk.gov.di.authentication.shared.helpers.RequestHeaderHelper;
+import uk.gov.di.authentication.shared.helpers.ValidationHelper;
 import uk.gov.di.authentication.shared.services.AuditService;
 import uk.gov.di.authentication.shared.services.ConfigurationService;
 import uk.gov.di.authentication.shared.services.DynamoService;
 import uk.gov.di.authentication.shared.services.RedisConnectionService;
-import uk.gov.di.authentication.shared.services.ValidationService;
 
 import java.util.Map;
 import java.util.Optional;
@@ -42,7 +42,6 @@ public class UpdateEmailHandler
     private final ObjectMapper objectMapper = new ObjectMapper();
     private final DynamoService dynamoService;
     private final AwsSqsClient sqsClient;
-    private final ValidationService validationService;
     private final CodeStorageService codeStorageService;
     private static final Logger LOG = LogManager.getLogger(UpdateEmailHandler.class);
     private final AuditService auditService;
@@ -54,12 +53,10 @@ public UpdateEmailHandler() {
     public UpdateEmailHandler(
             DynamoService dynamoService,
             AwsSqsClient sqsClient,
-            ValidationService validationService,
             CodeStorageService codeStorageService,
             AuditService auditService) {
         this.dynamoService = dynamoService;
         this.sqsClient = sqsClient;
-        this.validationService = validationService;
         this.codeStorageService = codeStorageService;
         this.auditService = auditService;
     }
@@ -71,7 +68,6 @@ public UpdateEmailHandler(ConfigurationService configurationService) {
                         configurationService.getAwsRegion(),
                         configurationService.getEmailQueueUri(),
                         configurationService.getSqsEndpointUri());
-        this.validationService = new ValidationService();
         this.codeStorageService =
                 new CodeStorageService(new RedisConnectionService(configurationService));
         this.auditService = new AuditService(configurationService);
@@ -102,7 +98,7 @@ public APIGatewayProxyResponseEvent handleRequest(
                                             400, ErrorResponse.ERROR_1020);
                                 }
                                 Optional<ErrorResponse> emailValidationErrors =
-                                        validationService.validateEmailAddressUpdate(
+                                        ValidationHelper.validateEmailAddressUpdate(
                                                 updateInfoRequest.getExistingEmailAddress(),
                                                 updateInfoRequest.getReplacementEmailAddress());
                                 if (emailValidationErrors.isPresent()) {

account-management-api/src/main/java/uk/gov/di/accountmanagement/lambda/UpdatePhoneNumberHandler.java

@@ -21,14 +21,12 @@
 import uk.gov.di.authentication.shared.helpers.PersistentIdHelper;
 import uk.gov.di.authentication.shared.helpers.RequestBodyHelper;
 import uk.gov.di.authentication.shared.helpers.RequestHeaderHelper;
-import uk.gov.di.authentication.shared.helpers.ValidationHelper;
 import uk.gov.di.authentication.shared.services.AuditService;
 import uk.gov.di.authentication.shared.services.ConfigurationService;
 import uk.gov.di.authentication.shared.services.DynamoService;
 import uk.gov.di.authentication.shared.services.RedisConnectionService;
 
 import java.util.Map;
-import java.util.Optional;
 
 import static uk.gov.di.authentication.shared.domain.RequestHeaders.SESSION_ID_HEADER;
 import static uk.gov.di.authentication.shared.helpers.ApiGatewayResponseHelper.generateApiGatewayProxyErrorResponse;
@@ -97,13 +95,6 @@ public APIGatewayProxyResponseEvent handleRequest(
                                     return generateApiGatewayProxyErrorResponse(
                                             400, ErrorResponse.ERROR_1020);
                                 }
-                                Optional<ErrorResponse> phoneValidationErrors =
-                                        ValidationHelper.validatePhoneNumber(
-                                                updatePhoneNumberRequest.getPhoneNumber());
-                                if (phoneValidationErrors.isPresent()) {
-                                    return generateApiGatewayProxyErrorResponse(
-                                            400, phoneValidationErrors.get());
-                                }
                                 UserProfile userProfile =
                                         dynamoService.getUserProfileByEmail(
                                                 updatePhoneNumberRequest.getEmail());

account-management-api/src/test/java/uk/gov/di/accountmanagement/lambda/SendOtpNotificationHandlerTest.java

@@ -20,10 +20,8 @@
 import uk.gov.di.authentication.shared.services.CodeGeneratorService;
 import uk.gov.di.authentication.shared.services.ConfigurationService;
 import uk.gov.di.authentication.shared.services.DynamoService;
-import uk.gov.di.authentication.shared.services.ValidationService;
 
 import java.util.Map;
-import java.util.Optional;
 
 import static java.lang.String.format;
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -50,7 +48,6 @@ class SendOtpNotificationHandlerTest {
     private static final String TEST_SIX_DIGIT_CODE = "123456";
     private static final String TEST_PHONE_NUMBER = "07755551084";
     private static final long CODE_EXPIRY_TIME = 900;
-    private final ValidationService validationService = mock(ValidationService.class);
     private final ConfigurationService configurationService = mock(ConfigurationService.class);
     private final AwsSqsClient awsSqsClient = mock(AwsSqsClient.class);
     private final CodeGeneratorService codeGeneratorService = mock(CodeGeneratorService.class);
@@ -62,7 +59,6 @@ class SendOtpNotificationHandlerTest {
     private final SendOtpNotificationHandler handler =
             new SendOtpNotificationHandler(
                     configurationService,
-                    validationService,
                     awsSqsClient,
                     codeGeneratorService,
                     codeStorageService,
@@ -78,8 +74,6 @@ void setup() {
     @Test
     void shouldReturn204AndPutMessageOnQueueForAValidEmailRequest() throws JsonProcessingException {
         String persistentIdValue = "some-persistent-session-id";
-        when(validationService.validateEmailAddress(eq(TEST_EMAIL_ADDRESS)))
-                .thenReturn(Optional.empty());
         NotifyRequest notifyRequest =
                 new NotifyRequest(TEST_EMAIL_ADDRESS, VERIFY_EMAIL, TEST_SIX_DIGIT_CODE);
         ObjectMapper objectMapper = new ObjectMapper();
@@ -156,7 +150,7 @@ void shouldReturn204AndPutMessageOnQueueForAValidPhoneRequest() throws JsonProce
     }
 
     @Test
-    public void shouldReturn400IfRequestIsMissingEmail() {
+    void shouldReturn400IfRequestIsMissingEmail() {
         APIGatewayProxyRequestEvent event = new APIGatewayProxyRequestEvent();
         event.setHeaders(Map.of());
         event.setBody("{ }");
@@ -169,11 +163,7 @@ public void shouldReturn400IfRequestIsMissingEmail() {
     }
 
     @Test
-    public void shouldReturn400IfEmailAddressIsInvalid() {
-
-        when(validationService.validateEmailAddress(eq("joe.bloggs")))
-                .thenReturn(Optional.of(ErrorResponse.ERROR_1004));
-
+    void shouldReturn400IfEmailAddressIsInvalid() {
         APIGatewayProxyRequestEvent event = new APIGatewayProxyRequestEvent();
         event.setHeaders(Map.of());
         event.setBody(
@@ -190,7 +180,7 @@ public void shouldReturn400IfEmailAddressIsInvalid() {
     }
 
     @Test
-    public void shouldReturn400IfPhoneNumberIsInvalid() {
+    void shouldReturn400IfPhoneNumberIsInvalid() {
         APIGatewayProxyRequestEvent event = new APIGatewayProxyRequestEvent();
         event.setHeaders(Map.of());
         event.setBody(
@@ -207,9 +197,7 @@ public void shouldReturn400IfPhoneNumberIsInvalid() {
     }
 
     @Test
-    public void shouldReturn500IfMessageCannotBeSentToQueue() throws JsonProcessingException {
-        when(validationService.validateEmailAddress(eq(TEST_EMAIL_ADDRESS)))
-                .thenReturn(Optional.empty());
+    void shouldReturn500IfMessageCannotBeSentToQueue() throws JsonProcessingException {
         NotifyRequest notifyRequest =
                 new NotifyRequest(TEST_EMAIL_ADDRESS, VERIFY_EMAIL, TEST_SIX_DIGIT_CODE);
         ObjectMapper objectMapper = new ObjectMapper();
@@ -231,10 +219,7 @@ public void shouldReturn500IfMessageCannotBeSentToQueue() throws JsonProcessingE
     }
 
     @Test
-    public void shouldReturn400WhenInvalidNotificationType() {
-        when(validationService.validateEmailAddress(eq(TEST_EMAIL_ADDRESS)))
-                .thenReturn(Optional.empty());
-
+    void shouldReturn400WhenInvalidNotificationType() {
         APIGatewayProxyRequestEvent event = new APIGatewayProxyRequestEvent();
         event.setHeaders(Map.of());
         event.setBody(
@@ -254,9 +239,7 @@ public void shouldReturn400WhenInvalidNotificationType() {
     }
 
     @Test
-    public void shouldReturn400WhenAccountAlreadyExistsWithGivenEmail() {
-        when(validationService.validateEmailAddress(eq(TEST_EMAIL_ADDRESS)))
-                .thenReturn(Optional.empty());
+    void shouldReturn400WhenAccountAlreadyExistsWithGivenEmail() {
         when(dynamoService.userExists(eq(TEST_EMAIL_ADDRESS))).thenReturn(true);
 
         APIGatewayProxyRequestEvent event = new APIGatewayProxyRequestEvent();

account-management-api/src/test/java/uk/gov/di/accountmanagement/lambda/UpdateEmailHandlerTest.java

@@ -18,11 +18,9 @@
 import uk.gov.di.authentication.shared.helpers.PersistentIdHelper;
 import uk.gov.di.authentication.shared.services.AuditService;
 import uk.gov.di.authentication.shared.services.DynamoService;
-import uk.gov.di.authentication.shared.services.ValidationService;
 
 import java.util.HashMap;
 import java.util.Map;
-import java.util.Optional;
 
 import static java.lang.String.format;
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -43,7 +41,6 @@ class UpdateEmailHandlerTest {
     private final Context context = mock(Context.class);
     private final DynamoService dynamoService = mock(DynamoService.class);
     private final AwsSqsClient sqsClient = mock(AwsSqsClient.class);
-    private final ValidationService validationService = mock(ValidationService.class);
     private final CodeStorageService codeStorageService = mock(CodeStorageService.class);
     private UpdateEmailHandler handler;
     private static final String EXISTING_EMAIL_ADDRESS = "joe.bloggs@digital.cabinet-office.gov.uk";
@@ -54,18 +51,13 @@ class UpdateEmailHandlerTest {
     private final AuditService auditService = mock(AuditService.class);
 
     @BeforeEach
-    public void setUp() {
+    void setUp() {
         handler =
-                new UpdateEmailHandler(
-                        dynamoService,
-                        sqsClient,
-                        validationService,
-                        codeStorageService,
-                        auditService);
+                new UpdateEmailHandler(dynamoService, sqsClient, codeStorageService, auditService);
     }
 
     @Test
-    public void shouldReturn204ForValidUpdateEmailRequest() throws JsonProcessingException {
+    void shouldReturn204ForValidUpdateEmailRequest() throws JsonProcessingException {
         String persistentIdValue = "some-persistent-session-id";
         UserProfile userProfile = new UserProfile().setPublicSubjectID(SUBJECT.getValue());
         when(dynamoService.getUserProfileByEmail(EXISTING_EMAIL_ADDRESS)).thenReturn(userProfile);
@@ -84,9 +76,6 @@ public void shouldReturn204ForValidUpdateEmailRequest() throws JsonProcessingExc
         event.setRequestContext(proxyRequestContext);
         when(codeStorageService.isValidOtpCode(NEW_EMAIL_ADDRESS, OTP, VERIFY_EMAIL))
                 .thenReturn(true);
-        when(validationService.validateEmailAddressUpdate(
-                        EXISTING_EMAIL_ADDRESS, NEW_EMAIL_ADDRESS))
-                .thenReturn(Optional.empty());
 
         APIGatewayProxyResponseEvent result = handler.handleRequest(event, context);
 
@@ -109,7 +98,7 @@ public void shouldReturn204ForValidUpdateEmailRequest() throws JsonProcessingExc
     }
 
     @Test
-    public void shouldReturn400WhenReplacementEmailAlreadyExists() throws JsonProcessingException {
+    void shouldReturn400WhenReplacementEmailAlreadyExists() throws JsonProcessingException {
         when(dynamoService.getSubjectFromEmail(EXISTING_EMAIL_ADDRESS)).thenReturn(SUBJECT);
         APIGatewayProxyRequestEvent event = new APIGatewayProxyRequestEvent();
         event.setBody(
@@ -124,9 +113,6 @@ public void shouldReturn400WhenReplacementEmailAlreadyExists() throws JsonProces
         event.setRequestContext(proxyRequestContext);
         when(codeStorageService.isValidOtpCode(NEW_EMAIL_ADDRESS, OTP, VERIFY_EMAIL))
                 .thenReturn(true);
-        when(validationService.validateEmailAddressUpdate(
-                        EXISTING_EMAIL_ADDRESS, NEW_EMAIL_ADDRESS))
-                .thenReturn(Optional.empty());
         when(dynamoService.userExists(NEW_EMAIL_ADDRESS)).thenReturn(true);
         APIGatewayProxyResponseEvent result = handler.handleRequest(event, context);
 
@@ -139,7 +125,7 @@ public void shouldReturn400WhenReplacementEmailAlreadyExists() throws JsonProces
     }
 
     @Test
-    public void shouldReturn400WhenRequestIsMissingParameters() {
+    void shouldReturn400WhenRequestIsMissingParameters() {
         APIGatewayProxyRequestEvent.ProxyRequestContext proxyRequestContext =
                 new APIGatewayProxyRequestEvent.ProxyRequestContext();
         Map<String, Object> authorizerParams = new HashMap<>();
@@ -181,8 +167,7 @@ public void shouldReturnErrorWhenOtpCodeIsNotValid() throws JsonProcessingExcept
     }
 
     @Test
-    public void shouldReturn400AndNotUpdateEmailWhenEmailIsInvalid()
-            throws JsonProcessingException {
+    void shouldReturn400AndNotUpdateEmailWhenEmailIsInvalid() throws JsonProcessingException {
         when(dynamoService.getSubjectFromEmail(EXISTING_EMAIL_ADDRESS)).thenReturn(SUBJECT);
         APIGatewayProxyRequestEvent event = new APIGatewayProxyRequestEvent();
         event.setBody(
@@ -197,9 +182,6 @@ public void shouldReturn400AndNotUpdateEmailWhenEmailIsInvalid()
         event.setRequestContext(proxyRequestContext);
         when(codeStorageService.isValidOtpCode(INVALID_EMAIL_ADDRESS, OTP, VERIFY_EMAIL))
                 .thenReturn(true);
-        when(validationService.validateEmailAddressUpdate(
-                        EXISTING_EMAIL_ADDRESS, INVALID_EMAIL_ADDRESS))
-                .thenReturn(Optional.of(ErrorResponse.ERROR_1004));
         APIGatewayProxyResponseEvent result = handler.handleRequest(event, context);
 
         assertThat(result, hasStatus(400));
@@ -211,7 +193,7 @@ public void shouldReturn400AndNotUpdateEmailWhenEmailIsInvalid()
     }
 
     @Test
-    public void shouldFormatAllEmailsToLowerCase() {
+    void shouldFormatAllEmailsToLowerCase() {
         final UpdateEmailRequest updateEmailRequest =
                 new UpdateEmailRequest(
                         "Joe.Bloggs@digital.cabinet-office.gov.uk",

account-management-api/src/test/java/uk/gov/di/accountmanagement/lambda/UpdatePhoneNumberHandlerTest.java

@@ -145,32 +145,4 @@ public void shouldReturnErrorWhenOtpCodeIsNotValid() throws JsonProcessingExcept
         assertThat(result, hasBody(expectedResponse));
         verifyNoInteractions(auditService);
     }
-
-    @Test
-    public void shouldReturn400AndNotUpdatePhoneNumberWhenPhoneNumberIsInvalid()
-            throws JsonProcessingException {
-        when(dynamoService.getSubjectFromEmail(EMAIL_ADDRESS)).thenReturn(SUBJECT);
-        APIGatewayProxyRequestEvent event = new APIGatewayProxyRequestEvent();
-        event.setBody(
-                format(
-                        "{\"email\": \"%s\", \"phoneNumber\": \"%s\", \"otp\": \"%s\"  }",
-                        EMAIL_ADDRESS, INVALID_PHONE_NUMBER, OTP));
-        APIGatewayProxyRequestEvent.ProxyRequestContext proxyRequestContext =
-                new APIGatewayProxyRequestEvent.ProxyRequestContext();
-        Map<String, Object> authorizerParams = new HashMap<>();
-        authorizerParams.put("principalId", SUBJECT.getValue());
-        proxyRequestContext.setAuthorizer(authorizerParams);
-        event.setRequestContext(proxyRequestContext);
-        when(codeStorageService.isValidOtpCode(EMAIL_ADDRESS, OTP, VERIFY_PHONE_NUMBER))
-                .thenReturn(true);
-        APIGatewayProxyResponseEvent result = handler.handleRequest(event, context);
-
-        assertThat(result, hasStatus(400));
-        verify(dynamoService, times(0)).updatePhoneNumber(EMAIL_ADDRESS, INVALID_PHONE_NUMBER);
-        NotifyRequest notifyRequest = new NotifyRequest(INVALID_PHONE_NUMBER, PHONE_NUMBER_UPDATED);
-        verify(sqsClient, times(0)).send(new ObjectMapper().writeValueAsString(notifyRequest));
-        String expectedResponse = new ObjectMapper().writeValueAsString(ErrorResponse.ERROR_1012);
-        assertThat(result, hasBody(expectedResponse));
-        verifyNoInteractions(auditService);
-    }
 }