diff --git a/src/main/java/org/broadinstitute/consent/http/db/SamDAO.java b/src/main/java/org/broadinstitute/consent/http/db/SamDAO.java index b581ff42e3..aa59a1e0ae 100644 --- a/src/main/java/org/broadinstitute/consent/http/db/SamDAO.java +++ b/src/main/java/org/broadinstitute/consent/http/db/SamDAO.java @@ -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; @@ -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. Please contact support for help with this error.", + authUser.getEmail())); + } String errorMsg = String.format( "Error getting combined user status info for user %s: %s", diff --git a/src/main/java/org/broadinstitute/consent/http/exceptions/SamAzureB2CException.java b/src/main/java/org/broadinstitute/consent/http/exceptions/SamAzureB2CException.java new file mode 100644 index 0000000000..378918af3c --- /dev/null +++ b/src/main/java/org/broadinstitute/consent/http/exceptions/SamAzureB2CException.java @@ -0,0 +1,8 @@ +package org.broadinstitute.consent.http.exceptions; + +public class SamAzureB2CException extends Exception { + + public SamAzureB2CException(String message) { + super(message); + } +} diff --git a/src/main/java/org/broadinstitute/consent/http/resources/UserResource.java b/src/main/java/org/broadinstitute/consent/http/resources/UserResource.java index d2cead9a1b..5668ecdba0 100644 --- a/src/main/java/org/broadinstitute/consent/http/resources/UserResource.java +++ b/src/main/java/org/broadinstitute/consent/http/resources/UserResource.java @@ -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; @@ -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. diff --git a/src/main/java/org/broadinstitute/consent/http/service/NihService.java b/src/main/java/org/broadinstitute/consent/http/service/NihService.java index 54ae9af7a4..5562b4900c 100644 --- a/src/main/java/org/broadinstitute/consent/http/service/NihService.java +++ b/src/main/java/org/broadinstitute/consent/http/service/NihService.java @@ -47,7 +47,18 @@ public User syncAccount(DuosUser duosUser) throws Exception { try { HttpResponse response = clientUtil.handleHttpRequest(request); if (!response.isSuccessStatusCode()) { - throw new ServerErrorException(response.getStatusMessage(), response.getStatusCode()); + // ECM returns a 500 Internal Server Error when there is an AzureB2C error in Sam. + if (response.getStatusCode() == HttpStatusCodes.STATUS_CODE_SERVER_ERROR) { + // Log this case and remove the user's current account link until problem can be resolved + logException( + new Exception( + "ECM Server error while syncing account for user: %s" + .formatted(duosUser.getEmail()))); + serviceDAO.deleteNihAccountById(user.getUserId()); + return userDAO.findUserWithPropertiesById(user.getUserId(), UserFields.getValues()); + } else { + throw new ServerErrorException(response.getStatusMessage(), response.getStatusCode()); + } } String body = response.parseAsString(); NIHUserAccount nihAccount = parseNihUserAccount(body); diff --git a/src/test/java/org/broadinstitute/consent/http/resources/UserResourceTest.java b/src/test/java/org/broadinstitute/consent/http/resources/UserResourceTest.java index 2acdba5b97..25d4654cd6 100644 --- a/src/test/java/org/broadinstitute/consent/http/resources/UserResourceTest.java +++ b/src/test/java/org/broadinstitute/consent/http/resources/UserResourceTest.java @@ -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; @@ -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() { diff --git a/src/test/java/org/broadinstitute/consent/http/service/NihServiceTest.java b/src/test/java/org/broadinstitute/consent/http/service/NihServiceTest.java index aba9ec5855..c5c46304eb 100644 --- a/src/test/java/org/broadinstitute/consent/http/service/NihServiceTest.java +++ b/src/test/java/org/broadinstitute/consent/http/service/NihServiceTest.java @@ -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; @@ -92,18 +94,23 @@ void testSyncAccountBadRequestError() { } @Test - void testSyncAccountServerError() { + void testSyncAccountServerError() throws Exception { + // A 500 from ECM is treated specially, 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 diff --git a/src/test/java/org/broadinstitute/consent/http/service/dao/SamDAOTest.java b/src/test/java/org/broadinstitute/consent/http/service/dao/SamDAOTest.java index dfc973efe3..cfcce31dd5 100644 --- a/src/test/java/org/broadinstitute/consent/http/service/dao/SamDAOTest.java +++ b/src/test/java/org/broadinstitute/consent/http/service/dao/SamDAOTest.java @@ -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; @@ -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; @@ -304,6 +306,35 @@ void testGetCombinedUserStatusInfoNonSuccessStatus() { assertThrows(WebApplicationException.class, () -> samDAO.getCombinedUserStatusInfo(duosUser)); } + private static Stream 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";