Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TSPS-404 Throw correct exception for Sam user not found #223

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

mmorgantaylor
Copy link
Contributor

Teaspoons uses TCL's SamUserFactory to check that the user is registered in Sam.

However, if a user is not registered in Sam/Terra, TCL is currently throwing an InternalServerErrorException that results in a 500 response, with the error message stating that the user was not found in Sam and an error code of 403 (FORBIDDEN). That's because TCL expects Sam to return a 404 (NOT_FOUND), so it doesn't properly handle the 403 that Sam returns in this case .

This PR updates the logic so that TCL treats both a 404 and a 403 as "User not found".

Teaspoons Jira ticket: https://broadworkbench.atlassian.net/browse/TSPS-404

Copy link

sonarqubecloud bot commented Feb 5, 2025

Copy link
Member

@pshapiro4broad pshapiro4broad left a comment

Choose a reason for hiding this comment

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

This seems correct with respect to Sam's behavior, but I wonder why 403/forbidden is being returned instead of 404/not found in the first place? I would expect a 404 if a user isn't found.

Comment on lines 66 to 71
} catch (final ApiException e) {
if (e.getCode() == HttpStatus.NOT_FOUND.value()) {
if (e.getCode() == HttpStatus.NOT_FOUND.value()
|| e.getCode() == HttpStatus.FORBIDDEN.value()) {
throw new UnauthorizedException("User not found", e);
} else {
throw new InternalServerErrorException(e);
Copy link
Member

Choose a reason for hiding this comment

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

It seems odd to have an exception that contains a http status and then use it to map to a different http status. Another approach would be to map the ApiException's status directly to the new exception value

      throw new ErrorReportException(e, HttpStatus.valueOf(e.getCode())) {};

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