Skip to content

DT-2845: Handle known AzureB2C exception cases#2866

Open
rushtong wants to merge 3 commits intodevelopfrom
gr-DT-2845-handle-azure-error
Open

DT-2845: Handle known AzureB2C exception cases#2866
rushtong wants to merge 3 commits intodevelopfrom
gr-DT-2845-handle-azure-error

Conversation

@rushtong
Copy link
Copy Markdown
Contributor

@rushtong rushtong commented Apr 16, 2026

Addresses

https://broadworkbench.atlassian.net/browse/DT-2845

Summary

See companion front end PR: DataBiosphere/duos-ui#3442

This PR adds new error handling for Sam errors wrt AzureB2C errors. When a user first uses MS as an identity provider with a google account, then subsequently uses Google as an identity provider, Sam will respond with an error similar to:

AzureB2C authentication Error for user user@test.com: {
    "causes [],
    "exceptionClass":"org.broadinstitute.dsde.workbench.model.WorkbenchException",
    "message":"Cannot update azureB2cId for user 123... because user does not exist or the azureB2cId has already been set for this user",
    "source":"sam",
    "stackTrace":[]
}

When ECM encounters this underlying error (because it proxies requests to Sam), it merely responds with a 500 Internal Server Error.

This PR does two things:

  1. When ECM responds with a 500 error, we assume the worst and remove the user's current linked account status. This needs to be done to ensure that users are not improperly linked.
  2. When Sam responds with an AzureB2C error, we percolate that back up to the client in the primary case of a user getting their own information: GET /api/user/me

Testing

Local testing is challenging. Using swagger in Consent, one uses google sign in. When using swagger in Sam, the google sign in is OIDC. Therefore, the bearer tokens you get in Consent swagger are the more compact ones, e.g. ya29..... The bearer tokens you get in Sam swagger are encoded jwts, e.g. eyJhbGci.... When using the compact token, Sam does NOT error out. When using the jwt token, Sam DOES error out. So, when using Consent locally, a problematic user will have successful responses from Sam. To test a problematic user, one has to use the jwt token in Consent using curl (OIDC auth does not work in Consent swagger). The DUOS UI uses OIDC, so generates full jwt bearer tokens that DO fail when Consent proxies those tokens against Sam and other services like ECM.

The fact that Sam works with compact tokens but fails with full jwt tokens opens up another approach. Since most UI requests are proxied through Consent, we could extract the compact token and use that against Sam. The UI does also communicate directly with ECM so that would still be problematic unless we were able to do that same manipulation in the UI.


Have you read CONTRIBUTING.md lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

@rushtong rushtong marked this pull request as ready for review April 16, 2026 19:03
@rushtong rushtong requested a review from a team as a code owner April 16, 2026 19:03
@rushtong rushtong requested review from Copilot, kevinmarete and otchet-broad and removed request for a team April 16, 2026 19:03
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds explicit handling for known AzureB2C-related failures coming from Sam/ECM so Consent can (a) safely unlink NIH/ECM account state on ECM 500s and (b) surface a more specific failure when fetching the current user (GET /api/user/me).

Changes:

  • Detect “cannot update azureB2cId…” failures in SamDAO.getCombinedUserStatusInfo and throw SamAzureB2CException.
  • Treat ECM 500s during NIH sync as a special case: log and delete the local NIH link rather than throwing.
  • Add unit tests covering the new AzureB2C detection and the revised NIH sync behavior, plus /me behavior when AzureB2C errors occur.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/main/java/org/broadinstitute/consent/http/db/SamDAO.java Adds AzureB2C error detection and throws SamAzureB2CException from combined-state lookup.
src/main/java/org/broadinstitute/consent/http/resources/UserResource.java Ensures SamAzureB2CException is not swallowed when building /me response.
src/main/java/org/broadinstitute/consent/http/service/NihService.java Treats ECM 500 as a special case; deletes local NIH link and returns user.
src/main/java/org/broadinstitute/consent/http/exceptions/SamAzureB2CException.java New checked exception to represent the known Sam AzureB2C failure case.
src/main/java/org/broadinstitute/consent/http/exceptions/ECMException.java New checked exception used for ECM 500 control flow in NIH sync.
src/test/java/org/broadinstitute/consent/http/service/dao/SamDAOTest.java Adds parameterized coverage for various AzureB2C error-body formats.
src/test/java/org/broadinstitute/consent/http/service/NihServiceTest.java Updates expectations: ECM 500 no longer throws; local NIH link is deleted.
src/test/java/org/broadinstitute/consent/http/resources/UserResourceTest.java Adds tests verifying /me returns 500 and includes AzureB2C details when thrown.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/main/java/org/broadinstitute/consent/http/service/NihService.java Outdated
Comment thread src/main/java/org/broadinstitute/consent/http/db/SamDAO.java Outdated
Comment thread src/main/java/org/broadinstitute/consent/http/db/SamDAO.java Outdated
@sonarqubecloud
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants