From 0074453a7dcd50a025c39a90e52cb88187fcd735 Mon Sep 17 00:00:00 2001 From: Shaun Ford Date: Tue, 9 May 2017 12:07:52 -0700 Subject: [PATCH] Fix incorrect error codes, and add better messaging (#37) * Fix OneLogin authentication, IAM auth and duplicate SDB name error codes, and messages --- .../onelogin/OneLoginAuthConnector.java | 27 +++---- .../nike/cerberus/aws/KmsClientFactory.java | 8 +- .../nike/cerberus/error/DefaultApiError.java | 9 ++- .../onelogin/OneLoginAuthConnectorTest.java | 73 +++++++++++++++++-- .../cerberus/aws/KmsClientFactoryTest.java | 3 +- 5 files changed, 98 insertions(+), 22 deletions(-) diff --git a/src/main/java/com/nike/cerberus/auth/connector/onelogin/OneLoginAuthConnector.java b/src/main/java/com/nike/cerberus/auth/connector/onelogin/OneLoginAuthConnector.java index 9655cb043..79cdcedec 100644 --- a/src/main/java/com/nike/cerberus/auth/connector/onelogin/OneLoginAuthConnector.java +++ b/src/main/java/com/nike/cerberus/auth/connector/onelogin/OneLoginAuthConnector.java @@ -157,7 +157,7 @@ protected SessionLoginTokenData verifyFactor(final String deviceId, String msg = String.format("stateToken: %s failed to verify 2nd factor for reason: %s", stateToken, verifyFactorResponse.getStatus().getMessage()); - if (verifyFactorResponse.getStatus().getCode() == 400L) { + if (verifyFactorResponse.getStatus().getCode() == 401) { throw ApiException.newBuilder() .withApiErrors(DefaultApiError.AUTH_BAD_CREDENTIALS) .withExceptionMessage(msg) @@ -184,20 +184,21 @@ protected SessionLoginTokenData createSessionLoginToken(final String username, f final CreateSessionLoginTokenResponse createSessionLoginTokenResponse = oneLoginClient.createSessionLoginToken(username, password); + long statusCode = createSessionLoginTokenResponse.getStatus().getCode(); + if (createSessionLoginTokenResponse.getStatus().isError()) { String msg = String.format("The user %s failed to authenticate for reason: %s", username, createSessionLoginTokenResponse.getStatus().getMessage()); - if (createSessionLoginTokenResponse.getStatus().getCode() == 400L) { - if (StringUtils.startsWithIgnoreCase(createSessionLoginTokenResponse.getStatus().getMessage(), "MFA")) { - throw ApiException.newBuilder() - .withApiErrors(DefaultApiError.MFA_SETUP_REQUIRED) - .withExceptionMessage(msg) - .build(); - } else { - throw ApiException.newBuilder() - .withApiErrors(DefaultApiError.AUTH_BAD_CREDENTIALS) - .withExceptionMessage(msg) - .build(); - } + if (statusCode == 400 && + StringUtils.startsWithIgnoreCase(createSessionLoginTokenResponse.getStatus().getMessage(), "MFA")) { + throw ApiException.newBuilder() + .withApiErrors(DefaultApiError.MFA_SETUP_REQUIRED) + .withExceptionMessage(msg) + .build(); + } else if (statusCode == 400 || statusCode == 401) { + throw ApiException.newBuilder() + .withApiErrors(DefaultApiError.AUTH_BAD_CREDENTIALS) + .withExceptionMessage(msg) + .build(); } else { throw ApiException.newBuilder() .withApiErrors(DefaultApiError.GENERIC_BAD_REQUEST) diff --git a/src/main/java/com/nike/cerberus/aws/KmsClientFactory.java b/src/main/java/com/nike/cerberus/aws/KmsClientFactory.java index b5ff3c75f..69bb3e280 100644 --- a/src/main/java/com/nike/cerberus/aws/KmsClientFactory.java +++ b/src/main/java/com/nike/cerberus/aws/KmsClientFactory.java @@ -20,6 +20,8 @@ import com.amazonaws.regions.Regions; import com.amazonaws.services.kms.AWSKMSClient; import com.google.common.collect.Maps; +import com.nike.backstopper.exception.ApiException; +import com.nike.cerberus.error.DefaultApiError; import javax.inject.Singleton; import java.util.Map; @@ -62,7 +64,11 @@ public AWSKMSClient getClient(String regionName) { final Region region = Region.getRegion(Regions.fromName(regionName)); return getClient(region); } catch (IllegalArgumentException iae) { - throw new IllegalArgumentException("Specified region is not valid.", iae); + throw ApiException.newBuilder() + .withApiErrors(DefaultApiError.AUTHENTICATION_ERROR_INVALID_REGION) + .withExceptionCause(iae.getCause()) + .withExceptionMessage("Specified region is not valid.") + .build(); } } } diff --git a/src/main/java/com/nike/cerberus/error/DefaultApiError.java b/src/main/java/com/nike/cerberus/error/DefaultApiError.java index 615a6fa2e..5471af906 100644 --- a/src/main/java/com/nike/cerberus/error/DefaultApiError.java +++ b/src/main/java/com/nike/cerberus/error/DefaultApiError.java @@ -64,7 +64,7 @@ public enum DefaultApiError implements ApiError { /** * Supplied credentials are invalid. */ - AUTH_BAD_CREDENTIALS(99106, "Invalid credentials", HttpServletResponse.SC_BAD_REQUEST), + AUTH_BAD_CREDENTIALS(99106, "Invalid credentials", HttpServletResponse.SC_UNAUTHORIZED), /** * Category display name is blank. @@ -119,7 +119,7 @@ public enum DefaultApiError implements ApiError { /** * SDB name isn't unique. */ - SDB_UNIQUE_NAME(99210, "The name is not unique.", HttpServletResponse.SC_BAD_REQUEST), + SDB_UNIQUE_NAME(99210, "Duplicate SDB names are not allowed.", HttpServletResponse.SC_BAD_REQUEST), /** * SDB action requires caller to be the owner. @@ -217,6 +217,11 @@ public enum DefaultApiError implements ApiError { */ AUTH_IAM_PRINCIPAL_AWS_REGION_BLANK(99228, "AWS region is malformed.", HttpServletResponse.SC_BAD_REQUEST), + /** + * Invalid region provided in authentication + */ + AUTHENTICATION_ERROR_INVALID_REGION(99229, "Invalid AWS region provided during authentication.", HttpServletResponse.SC_BAD_REQUEST), + /** * Generic not found error. */ diff --git a/src/test/java/com/nike/cerberus/auth/connector/onelogin/OneLoginAuthConnectorTest.java b/src/test/java/com/nike/cerberus/auth/connector/onelogin/OneLoginAuthConnectorTest.java index 93109c589..493854f08 100644 --- a/src/test/java/com/nike/cerberus/auth/connector/onelogin/OneLoginAuthConnectorTest.java +++ b/src/test/java/com/nike/cerberus/auth/connector/onelogin/OneLoginAuthConnectorTest.java @@ -2,6 +2,7 @@ import com.google.common.collect.Lists; import com.google.common.collect.Sets; +import com.nike.backstopper.apierror.ApiError; import com.nike.backstopper.exception.ApiException; import com.nike.cerberus.auth.connector.AuthResponse; import com.nike.cerberus.auth.connector.AuthStatus; @@ -11,6 +12,7 @@ import java.util.Set; +import static com.nike.cerberus.error.DefaultApiError.MFA_SETUP_REQUIRED; import static org.junit.Assert.*; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -103,7 +105,7 @@ public void test_authenticate_with_mfa_required() { @Test public void test_authenticate_when_mfa_is_not_setup() { - setupMockWhereLoginGivesError(400L, "mfa something error message"); + setupMockWhereLoginGivesError(400, "mfa something error message"); try { // invoke method under test @@ -111,14 +113,14 @@ public void test_authenticate_when_mfa_is_not_setup() { fail("expected exception not thrown"); } catch (ApiException e) { - assertTrue(e.getApiErrors().contains(DefaultApiError.MFA_SETUP_REQUIRED)); + assertTrue(e.getApiErrors().contains(MFA_SETUP_REQUIRED)); assertFalse(e.getApiErrors().contains(DefaultApiError.AUTH_BAD_CREDENTIALS)); } } @Test public void test_authenticate_with_bad_creds() { - setupMockWhereLoginGivesError(400L, "any other error message"); + setupMockWhereLoginGivesError(401, "any other error message"); try { // invoke method under test @@ -127,7 +129,7 @@ public void test_authenticate_with_bad_creds() { fail("expected exception not thrown"); } catch (ApiException e) { assertTrue(e.getApiErrors().contains(DefaultApiError.AUTH_BAD_CREDENTIALS)); - assertFalse(e.getApiErrors().contains(DefaultApiError.MFA_SETUP_REQUIRED)); + assertFalse(e.getApiErrors().contains(MFA_SETUP_REQUIRED)); } } @@ -240,7 +242,7 @@ public void test_getUserById_gives_error() { @Test public void test_verifyFactor_with_400_error() { - setupMockWhereVerifyGivesError(400L, "any error message"); + setupMockWhereVerifyGivesError(401, "any error message"); try { // invoke method under test @@ -298,4 +300,65 @@ public void test_createSessionLoginToken() { assertEquals(expectedData, actualData); } + @Test + public void test_createSessionLoginToken_fails_with_401_when_bad_username_is_given() { + ResponseStatus status = new ResponseStatus(); + status.setError(true); + status.setCode(400); + status.setMessage("bad request"); + + CreateSessionLoginTokenResponse createSessionLoginTokenResponse = new CreateSessionLoginTokenResponse(); + createSessionLoginTokenResponse.setStatus(status); + + when(oneLoginClient.createSessionLoginToken(USERNAME, PASSWORD)).thenReturn(createSessionLoginTokenResponse); + + // invoke method under test + try { + oneLoginAuthConnector.createSessionLoginToken(USERNAME, PASSWORD); + } catch (ApiException ae) { + assertEquals(401, ae.getApiErrors().get(0).getHttpStatusCode()); + } + } + + @Test + public void test_createSessionLoginToken_fails_with_401_when_bad_password_is_given() { + ResponseStatus status = new ResponseStatus(); + status.setError(true); + status.setCode(401); + status.setMessage("Authentication Failed"); + + CreateSessionLoginTokenResponse createSessionLoginTokenResponse = new CreateSessionLoginTokenResponse(); + createSessionLoginTokenResponse.setStatus(status); + + when(oneLoginClient.createSessionLoginToken(USERNAME, PASSWORD)).thenReturn(createSessionLoginTokenResponse); + + // invoke method under test + try { + oneLoginAuthConnector.createSessionLoginToken(USERNAME, PASSWORD); + } catch (ApiException ae) { + assertEquals(401, ae.getApiErrors().get(0).getHttpStatusCode()); + } + + } + + @Test + public void test_createSessionLoginToken_fails_with_when_MFA_setup_is_required() { + ResponseStatus status = new ResponseStatus(); + status.setError(true); + status.setCode(400); + status.setMessage("MFA: rest doesnt matter"); + + CreateSessionLoginTokenResponse createSessionLoginTokenResponse = new CreateSessionLoginTokenResponse(); + createSessionLoginTokenResponse.setStatus(status); + + when(oneLoginClient.createSessionLoginToken(USERNAME, PASSWORD)).thenReturn(createSessionLoginTokenResponse); + + // invoke method under test + try { + oneLoginAuthConnector.createSessionLoginToken(USERNAME, PASSWORD); + } catch (ApiException ae) { + assertEquals(MFA_SETUP_REQUIRED.getHttpStatusCode(), ae.getApiErrors().get(0).getHttpStatusCode()); + } + + } } \ No newline at end of file diff --git a/src/test/java/com/nike/cerberus/aws/KmsClientFactoryTest.java b/src/test/java/com/nike/cerberus/aws/KmsClientFactoryTest.java index 6b0369886..ca091add6 100644 --- a/src/test/java/com/nike/cerberus/aws/KmsClientFactoryTest.java +++ b/src/test/java/com/nike/cerberus/aws/KmsClientFactoryTest.java @@ -19,6 +19,7 @@ import com.amazonaws.regions.Region; import com.amazonaws.regions.Regions; import com.amazonaws.services.kms.AWSKMSClient; +import com.nike.backstopper.exception.ApiException; import org.junit.Before; import org.junit.Test; @@ -56,7 +57,7 @@ public void get_client_by_region_string_returns_configured_kms_client() { assertThat(client).isNotNull(); } - @Test(expected = IllegalArgumentException.class) + @Test(expected = ApiException.class) public void get_client_by_region_string_throws_exception_if_bad_region_passed() { subject.getClient(badRegionName); }