Skip to content
This repository has been archived by the owner on Jan 12, 2024. It is now read-only.

Commit

Permalink
Fix incorrect error codes, and add better messaging (#37)
Browse files Browse the repository at this point in the history 
* Fix OneLogin authentication, IAM auth and duplicate SDB name error codes, and messages
  • Loading branch information
@sdford
sdford authored May 9, 2017
1 parent 307a913 commit 0074453
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 22 deletions.
27 changes: 14 additions & 13 deletions src/main/java/com/nike/cerberus/auth/connector/onelogin/OneLoginAuthConnector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down
8 changes: 7 additions & 1 deletion src/main/java/com/nike/cerberus/aws/KmsClientFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
}
}
9 changes: 7 additions & 2 deletions src/main/java/com/nike/cerberus/error/DefaultApiError.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
*/
Expand Down
73 changes: 68 additions & 5 deletions src/test/java/com/nike/cerberus/auth/connector/onelogin/OneLoginAuthConnectorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -103,22 +105,22 @@ 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
oneLoginAuthConnector.authenticate(USERNAME, PASSWORD);

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
Expand All @@ -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));

}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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());
}

}
}
3 changes: 2 additions & 1 deletion src/test/java/com/nike/cerberus/aws/KmsClientFactoryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}
Expand Down

0 comments on commit 0074453

Please sign in to comment.