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 7cf9252ca4..557251864a 100644 --- a/src/main/java/org/broadinstitute/consent/http/db/SamDAO.java +++ b/src/main/java/org/broadinstitute/consent/http/db/SamDAO.java @@ -124,10 +124,7 @@ public UserStatusInfo getCombinedUserStatusInfo(AuthUser authUser) throws Except 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())); + throw new SamAzureB2CException(getAzureB2CErrorMessage(authUser.getEmail())); } String errorMsg = String.format( @@ -170,16 +167,20 @@ public static String getErrorMessage(DuosUser duosUser, String body) { JsonElement messageElement = JsonParser.parseString(body).getAsJsonObject().get("message"); String message = messageElement != null ? messageElement.getAsString() : body; if (message.contains("Cannot update azureB2cId")) { - return String.format( - "Email: %s. You may have previously signed in with a different authentication provider (Google or Microsoft). Please sign in with that provider. For more information visit: https://support.terra.bio/hc/en-us/community/posts/24089648317467-Cannot-update-azureB2cId-for-user", - duosUser.getEmail()); + return getAzureB2CErrorMessage(duosUser.getEmail()); } - return String.format(errorMsg + " %s.", message); - } catch (JsonSyntaxException e) { // If the body is not a valid JSON - return String.format(errorMsg + " %s.", body); + return String.format("%s %s.", errorMsg, message); + } catch (JsonSyntaxException _) { // If the body is not a valid JSON + return String.format("%s %s.", errorMsg, body); } } + public static String getAzureB2CErrorMessage(String email) { + return String.format( + "Email: %s. You may have previously signed in with a different authentication provider (Google or Microsoft). Please sign in with that provider. For more information visit: https://support.terra.bio/hc/en-us/community/posts/24089648317467-Cannot-update-azureB2cId-for-user", + email); + } + public void asyncPostRegistrationInfo(DuosUser duosUser) { ListeningExecutorService listeningExecutorService = MoreExecutors.listeningDecorator(executorService); diff --git a/src/main/java/org/broadinstitute/consent/http/resources/Resource.java b/src/main/java/org/broadinstitute/consent/http/resources/Resource.java index 02050a28c7..51efcf35eb 100644 --- a/src/main/java/org/broadinstitute/consent/http/resources/Resource.java +++ b/src/main/java/org/broadinstitute/consent/http/resources/Resource.java @@ -26,6 +26,7 @@ import org.broadinstitute.consent.http.exceptions.ConsentConflictException; import org.broadinstitute.consent.http.exceptions.LibraryCardRequiredException; import org.broadinstitute.consent.http.exceptions.NIHComplianceRuleException; +import org.broadinstitute.consent.http.exceptions.SamAzureB2CException; import org.broadinstitute.consent.http.exceptions.SubmittedDARCannotBeEditedException; import org.broadinstitute.consent.http.exceptions.UnknownIdentifierException; import org.broadinstitute.consent.http.exceptions.UnprocessableEntityException; @@ -106,6 +107,13 @@ public abstract class Resource implements ConsentLogger { .type(MediaType.APPLICATION_JSON) .entity(new Error(e.getMessage(), Response.Status.CONFLICT.getStatusCode())) .build()); + DISPATCH.put( + SamAzureB2CException.class, + e -> + Response.status(Response.Status.CONFLICT) + .type(MediaType.APPLICATION_JSON) + .entity(new Error(e.getMessage(), Response.Status.CONFLICT.getStatusCode())) + .build()); DISPATCH.put( UnprocessableEntityException.class, e -> 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 f2cb0973b6..3c224164d3 100644 --- a/src/main/java/org/broadinstitute/consent/http/resources/UserResource.java +++ b/src/main/java/org/broadinstitute/consent/http/resources/UserResource.java @@ -124,9 +124,13 @@ public Response getUser(@Auth AuthUser authUser) { DuosUser duosUser = new DuosUser(authUser, findRegisteredUser(authUser)); UserStatusInfo userStatusInfo = duosUser.getUserStatusInfo(); if (userStatusInfo == null) { - samService.asyncPostRegistrationInfo(duosUser); - // Refresh the user status info after posting registration info to Sam + // Query Sam before the registration post: a B2C identity conflict throws a + // mapped exception here, and the registration post always answers 409 for + // those users, so we must not send it. userStatusInfo = getUserStatusInfo(duosUser); + if (userStatusInfo == null) { + samService.asyncPostRegistrationInfo(duosUser); + } } User user = nihService.syncAccount(duosUser); if (userStatusInfo != null) { @@ -142,23 +146,28 @@ public Response getUser(@Auth AuthUser authUser) { * Resolve the DUOS account for an authenticated caller. Unlike the {@code @Auth DuosUser} * endpoints, /me does its own lookup so that a valid token with no account answers 404 instead of * the 401 an unresolved principal produces — callers need those distinguishable to know whether - * to start registration or drop the session. + * to start registration or drop the session. A 404 sends the client into registration, which + * cannot succeed for a user whose Sam record has a B2C identity conflict, so when the auth filter + * got no Sam status we ask Sam again and let a B2C conflict answer 409 instead of 404. */ - private User findRegisteredUser(AuthUser authUser) { + private User findRegisteredUser(AuthUser authUser) throws SamAzureB2CException { try { return userService.findUserByEmail(authUser.getEmail()); - } catch (NotFoundException e) { + } catch (NotFoundException _) { + if (authUser.getUserStatusInfo() == null) { + getUserStatusInfo(authUser); + } throw new NotFoundException("Authenticated user is not registered"); } } - private UserStatusInfo getUserStatusInfo(DuosUser duosUser) throws SamAzureB2CException { + private UserStatusInfo getUserStatusInfo(AuthUser authUser) throws SamAzureB2CException { try { - return samService.getCombinedUserStatusInfo(duosUser); + return samService.getCombinedUserStatusInfo(authUser); } catch (SamAzureB2CException e) { logWarn( "Sam azure b2c exception: %s for user: %s" - .formatted(e.getMessage(), duosUser.getEmail())); + .formatted(e.getMessage(), authUser.getEmail())); throw e; } catch (Exception ex) { logWarn("Unable to retrieve user status info from Sam: " + ex.getMessage()); diff --git a/src/main/java/org/broadinstitute/consent/http/service/sam/SamService.java b/src/main/java/org/broadinstitute/consent/http/service/sam/SamService.java index b6285b0cff..56cfdd74ea 100644 --- a/src/main/java/org/broadinstitute/consent/http/service/sam/SamService.java +++ b/src/main/java/org/broadinstitute/consent/http/service/sam/SamService.java @@ -3,6 +3,7 @@ import com.google.inject.Inject; import java.util.List; import org.broadinstitute.consent.http.db.SamDAO; +import org.broadinstitute.consent.http.models.AuthUser; import org.broadinstitute.consent.http.models.DuosUser; import org.broadinstitute.consent.http.models.sam.ResourceType; import org.broadinstitute.consent.http.models.sam.TosResponse; @@ -40,8 +41,8 @@ public void asyncPostRegistrationInfo(DuosUser duosUser) { samDAO.asyncPostRegistrationInfo(duosUser); } - public UserStatusInfo getCombinedUserStatusInfo(DuosUser duosUser) throws Exception { - return samDAO.getCombinedUserStatusInfo(duosUser); + public UserStatusInfo getCombinedUserStatusInfo(AuthUser authUser) throws Exception { + return samDAO.getCombinedUserStatusInfo(authUser); } public String getToSText() throws Exception { diff --git a/src/main/resources/assets/api-docs.yaml b/src/main/resources/assets/api-docs.yaml index c7c57d6ad3..67ba207bd0 100644 --- a/src/main/resources/assets/api-docs.yaml +++ b/src/main/resources/assets/api-docs.yaml @@ -913,6 +913,14 @@ paths: description: Unauthorized - missing or unusable credentials 404: description: Authenticated, but no DUOS account exists for this user + 409: + description: The Sam record for this user has an azureB2cId conflict. The user + may have previously signed in with a different authentication provider. The + message explains how to resolve the conflict; registration cannot succeed. + content: + application/json: + schema: + $ref: '#/components/schemas/ErrorResponse' 429: description: Too Many Requests - rate limit exceeded content: 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 c94ddb4eda..0a3aba4c73 100644 --- a/src/test/java/org/broadinstitute/consent/http/resources/UserResourceTest.java +++ b/src/test/java/org/broadinstitute/consent/http/resources/UserResourceTest.java @@ -153,6 +153,45 @@ void testGetMeUnregisteredUserReturnsNotFound() throws Exception { verify(nihService, never()).syncAccount(any()); } + /** + * A caller with no DUOS account whose Sam record has a B2C identity conflict gets 409, not 404. A + * 404 would send them into registration, which cannot succeed for them. + */ + @Test + void testGetMeUnregisteredUserWithB2CConflictReturns409() throws Exception { + when(userService.findUserByEmail(TEST_EMAIL)) + .thenThrow(new NotFoundException("Unable to find user with email: " + TEST_EMAIL)); + when(samService.getCombinedUserStatusInfo(any(AuthUser.class))) + .thenThrow(new SamAzureB2CException("AzureB2C error for user " + TEST_EMAIL)); + + Response response = userResource.getUser(authUser); + + assertEquals(Status.CONFLICT.getStatusCode(), response.getStatus()); + verify(samService, never()).asyncPostRegistrationInfo(any()); + verify(nihService, never()).syncAccount(any()); + } + + /** + * When the auth filter already has a Sam status, an unregistered caller gets 404 with no second + * Sam query. + */ + @Test + void testGetMeUnregisteredUserWithStatusSkipsSamQuery() throws Exception { + AuthUser withStatus = + new AuthUser() + .setAuthToken("auth-token") + .setName("Test User") + .setEmail(TEST_EMAIL) + .setUserStatusInfo(new UserStatusInfo().setUserEmail(TEST_EMAIL)); + when(userService.findUserByEmail(TEST_EMAIL)) + .thenThrow(new NotFoundException("Unable to find user with email: " + TEST_EMAIL)); + + Response response = userResource.getUser(withStatus); + + assertEquals(Status.NOT_FOUND.getStatusCode(), response.getStatus()); + verify(samService, never()).getCombinedUserStatusInfo(any(AuthUser.class)); + } + @Test void testGetMeWithUserStatusInfo() throws Exception { User user = createUserWithRole(); @@ -195,15 +234,17 @@ void testGetMeSamFailure() throws Exception { } @Test - void testGetMe_SamAzureB2CException_Returns500() throws Exception { - // SamAzureB2CException should NOT be silently swallowed - it propagates and returns a 500 + void testGetMe_SamAzureB2CException_Returns409() throws Exception { + // SamAzureB2CException is not swallowed - it propagates and maps to a 409 Conflict User user = createUserWithRole(); when(userService.findUserByEmail(TEST_EMAIL)).thenReturn(user); when(samService.getCombinedUserStatusInfo(any(DuosUser.class))) .thenThrow(new SamAzureB2CException("AzureB2C error for user test@test.org")); Response response = userResource.getUser(authUser); - assertEquals(Status.INTERNAL_SERVER_ERROR.getStatusCode(), response.getStatus()); + assertEquals(Status.CONFLICT.getStatusCode(), response.getStatus()); + // The registration post always answers 409 for these users - it must not fire + verify(samService, never()).asyncPostRegistrationInfo(any(DuosUser.class)); } @Test @@ -217,7 +258,7 @@ void testGetMe_SamAzureB2CException_ErrorMessageContainsDetails() throws Excepti .thenThrow(new SamAzureB2CException(errorMessage)); Response response = userResource.getUser(authUser); - assertEquals(Status.INTERNAL_SERVER_ERROR.getStatusCode(), response.getStatus()); + assertEquals(Status.CONFLICT.getStatusCode(), response.getStatus()); assertNotNull(response.getEntity()); assertTrue(response.getEntity().toString().contains(errorMessage)); } 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 9561565f5e..f1fb7682bf 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 @@ -343,6 +343,8 @@ void testGetCombinedUserStatusInfoAzureB2CError(String errorBody) { assertThrows(SamAzureB2CException.class, () -> samDAO.getCombinedUserStatusInfo(duosUser)); assertNotNull(ex.getMessage()); assertTrue(ex.getMessage().contains("test@test.org")); + assertTrue(ex.getMessage().contains("different authentication provider")); + assertTrue(ex.getMessage().contains("https://support.terra.bio")); } @Test