Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/main/java/org/broadinstitute/consent/http/db/SamDAO.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.List;
import java.util.concurrent.ExecutorService;
import org.broadinstitute.consent.http.configurations.ServicesConfiguration;
import org.broadinstitute.consent.http.exceptions.SamAzureB2CException;
import org.broadinstitute.consent.http.models.AuthUser;
import org.broadinstitute.consent.http.models.DuosUser;
import org.broadinstitute.consent.http.models.sam.CombinedState;
Expand Down Expand Up @@ -120,6 +121,12 @@ public UserStatusInfo getCombinedUserStatusInfo(AuthUser authUser) throws Except
HttpResponse response = executeRequest(request);
String body = response.parseAsString();
if (!response.isSuccessStatusCode()) {
if (body.toLowerCase().contains("cannot update azureb2cid for user")) {
throw new SamAzureB2CException(
String.format(
"AzureB2C authentication Error for user %s: %s. Please contact support for help with this error.",
authUser.getEmail(), body));
Comment thread
rushtong marked this conversation as resolved.
Outdated
}
String errorMsg =
String.format(
"Error getting combined user status info for user %s: %s",
Expand All @@ -142,6 +149,9 @@ public UserStatusInfo getCombinedUserStatusInfo(AuthUser authUser) throws Except
.setEnabled(combinedState.getSamUser().enabled())
// Ensure that the user has both accepted the ToS and that it is the most recent version.
.setTosAccepted(tosAccepted);
} catch (SamAzureB2CException e) {
logException(e);
throw e;
Comment thread
rushtong marked this conversation as resolved.
Outdated
} catch (ForbiddenException e) {
// Sam throws a 403, not a 404, when the user is not found at this API
// which is re-thrown in executeRequest
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package org.broadinstitute.consent.http.exceptions;

public class ECMException extends Exception {

public ECMException(String message) {
super(message);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package org.broadinstitute.consent.http.exceptions;

public class SamAzureB2CException extends Exception {

public SamAzureB2CException(String message) {
super(message);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import java.util.Objects;
import org.broadinstitute.consent.http.configurations.ServicesConfiguration;
import org.broadinstitute.consent.http.enumeration.UserRoles;
import org.broadinstitute.consent.http.exceptions.SamAzureB2CException;
import org.broadinstitute.consent.http.models.Acknowledgement;
import org.broadinstitute.consent.http.models.ApprovedDataset;
import org.broadinstitute.consent.http.models.AuthUser;
Expand Down Expand Up @@ -134,9 +135,11 @@ public Response getUser(@Auth DuosUser duosUser) {
}
}

private UserStatusInfo getUserStatusInfo(DuosUser duosUser) {
private UserStatusInfo getUserStatusInfo(DuosUser duosUser) throws SamAzureB2CException {
try {
return samService.getCombinedUserStatusInfo(duosUser);
} catch (SamAzureB2CException e) {
throw e;
} catch (Exception ex) {
logWarn("Unable to retrieve user status info from Sam: " + ex.getMessage());
// Intentionally ignore Sam errors here to avoid failing /me on transient outages.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.broadinstitute.consent.http.configurations.ServicesConfiguration;
import org.broadinstitute.consent.http.db.UserDAO;
import org.broadinstitute.consent.http.enumeration.UserFields;
import org.broadinstitute.consent.http.exceptions.ECMException;
import org.broadinstitute.consent.http.models.DuosUser;
import org.broadinstitute.consent.http.models.NIHUserAccount;
import org.broadinstitute.consent.http.models.User;
Expand Down Expand Up @@ -46,12 +47,21 @@ public User syncAccount(DuosUser duosUser) throws Exception {
HttpRequest request = clientUtil.buildGetRequest(ecmRasProviderUrl, duosUser);
try {
HttpResponse response = clientUtil.handleHttpRequest(request);
// ECM returns a 500 Internal Server Error when there is an AzureB2C error in Sam.
if (response.getStatusCode() == HttpStatusCodes.STATUS_CODE_SERVER_ERROR) {
throw new ECMException(
"ECM Server error while syncing account for user: %s".formatted(duosUser.getEmail()));
}
Comment thread
rushtong marked this conversation as resolved.
Outdated
if (!response.isSuccessStatusCode()) {
throw new ServerErrorException(response.getStatusMessage(), response.getStatusCode());
}
String body = response.parseAsString();
NIHUserAccount nihAccount = parseNihUserAccount(body);
serviceDAO.updateUserNihStatus(user, nihAccount);
} catch (ECMException ecmException) {
// Log this case and remove the user's current account link until problem can be resolved
logException(ecmException);
serviceDAO.deleteNihAccountById(user.getUserId());
} catch (NotFoundException _) {
serviceDAO.deleteNihAccountById(user.getUserId());
} catch (NotAuthorizedException _) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import org.broadinstitute.consent.http.AbstractTestHelper;
import org.broadinstitute.consent.http.configurations.ServicesConfiguration;
import org.broadinstitute.consent.http.enumeration.UserRoles;
import org.broadinstitute.consent.http.exceptions.SamAzureB2CException;
import org.broadinstitute.consent.http.models.Acknowledgement;
import org.broadinstitute.consent.http.models.ApprovedDataset;
import org.broadinstitute.consent.http.models.AuthUser;
Expand Down Expand Up @@ -170,6 +171,34 @@ void testGetMeSamFailure() throws Exception {
assertEquals(Status.OK.getStatusCode(), response.getStatus());
}

@Test
void testGetMe_SamAzureB2CException_Returns500() throws Exception {
// SamAzureB2CException should NOT be silently swallowed - it propagates and returns a 500
User user = createUserWithRole();
DuosUser du = new DuosUser(authUser, user);
when(samService.getCombinedUserStatusInfo(du))
.thenThrow(new SamAzureB2CException("AzureB2C error for user test@test.org"));

Response response = userResource.getUser(du);
assertEquals(Status.INTERNAL_SERVER_ERROR.getStatusCode(), response.getStatus());
}

@Test
void testGetMe_SamAzureB2CException_ErrorMessageContainsDetails() throws Exception {
// The error message from SamAzureB2CException should be returned in the response
User user = createUserWithRole();
DuosUser du = new DuosUser(authUser, user);
String errorMessage =
"AzureB2C authentication Error for user test@test.org: Please contact support.";
when(samService.getCombinedUserStatusInfo(du))
.thenThrow(new SamAzureB2CException(errorMessage));

Response response = userResource.getUser(du);
assertEquals(Status.INTERNAL_SERVER_ERROR.getStatusCode(), response.getStatus());
assertNotNull(response.getEntity());
assertTrue(response.getEntity().toString().contains(errorMessage));
}

@Test
void testGetUserById() {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
import static com.github.tomakehurst.wiremock.client.WireMock.anyUrl;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

Expand Down Expand Up @@ -92,18 +94,23 @@ void testSyncAccountBadRequestError() {
}

@Test
void testSyncAccountServerError() {
void testSyncAccountServerError() throws Exception {
// A 500 from ECM is treated as an ECMException, which is caught gracefully:
// the NIH account is deleted locally and the user is returned without throwing.
User user = new User();
user.setUserId(1);
when(duosUser.getUser()).thenReturn(user);
LinkInfo ecmResponse = new LinkInfo("test", "test", true);
when(duosUser.getEmail()).thenReturn("test@test.org");
when(userDAO.findUserWithPropertiesById(user.getUserId(), UserFields.getValues()))
.thenReturn(user);
wireMockServer.stubFor(
any(anyUrl())
.willReturn(
aResponse()
.withStatus(HttpStatusCodes.STATUS_CODE_SERVER_ERROR)
.withBody(GsonUtil.getInstance().toJson(ecmResponse))));
assertThrows(ServerErrorException.class, () -> service.syncAccount(duosUser));
any(anyUrl()).willReturn(aResponse().withStatus(HttpStatusCodes.STATUS_CODE_SERVER_ERROR)));

User syncedUser = service.syncAccount(duosUser);

assertEquals(user.getUserId(), syncedUser.getUserId());
verify(nihServiceDAO, times(1)).deleteNihAccountById(user.getUserId());
verify(nihServiceDAO, never()).updateUserNihStatus(any(), any());
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import static org.mockito.Mockito.when;

Expand All @@ -27,6 +28,7 @@
import org.broadinstitute.consent.http.configurations.ServicesConfiguration;
import org.broadinstitute.consent.http.db.SamDAO;
import org.broadinstitute.consent.http.exceptions.ConsentConflictException;
import org.broadinstitute.consent.http.exceptions.SamAzureB2CException;
import org.broadinstitute.consent.http.models.DuosUser;
import org.broadinstitute.consent.http.models.sam.CombinedState;
import org.broadinstitute.consent.http.models.sam.EmailResponse;
Expand Down Expand Up @@ -304,6 +306,35 @@ void testGetCombinedUserStatusInfoNonSuccessStatus() {
assertThrows(WebApplicationException.class, () -> samDAO.getCombinedUserStatusInfo(duosUser));
}

private static Stream<Arguments> provideAzureB2CErrorBodies() {
return Stream.of(
// JSON message field with mixed case
Arguments.of("{\"message\": \"Cannot update azureB2cId for user test@test.org\"}"),
// All uppercase (case-insensitive check)
Arguments.of("CANNOT UPDATE AZUREB2CID FOR USER test@test.org"),
// All lowercase
Arguments.of("cannot update azureb2cid for user test@test.org"),
// Plain string with surrounding context
Arguments.of("Error: cannot update azureB2cId for user test@test.org due to conflict"));
}

@ParameterizedTest
@MethodSource("provideAzureB2CErrorBodies")
void testGetCombinedUserStatusInfoAzureB2CError(String errorBody) {
wireMockServer.stubFor(
any(anyUrl())
.willReturn(
aResponse()
.withStatus(HttpStatusCodes.STATUS_CODE_SERVER_ERROR)
.withBody(errorBody)));
when(duosUser.getEmail()).thenReturn("test@test.org");

SamAzureB2CException ex =
assertThrows(SamAzureB2CException.class, () -> samDAO.getCombinedUserStatusInfo(duosUser));
assertNotNull(ex.getMessage());
assertTrue(ex.getMessage().contains("test@test.org"));
}

@Test
void testGetToSText() {
String mockText = "Plain Text";
Expand Down
Loading