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

ID-1300 Admin Get Users by ID #1498

Merged
merged 4 commits into from
Jul 24, 2024
Merged

Conversation

tlangs
Copy link
Contributor

@tlangs tlangs commented Jul 22, 2024

Ticket: https://broadworkbench.atlassian.net/browse/ID-1300

In order for Orchestration to modify group membership using Sam User ID from ECM, it needs to be able to add users from the response to Sam Groups. Since Sam Group Endpoints only deal in email addresses, we need a way to get emails from Sam User IDs.

This is an admin-only functionality, as Sam User IDs should not be discoverable by other users. IE: A user can know their own Sam User ID, but should not be able to find out the Sam User IDs of others. Because of this, only authorized Terra Services will be able to call this API endpoint. In this case, its Thurloe and Orch.

Another idea is to make a V2 Sam Group API, which deals only in Sam User IDs, but that’s a much larger change and not necessary to accomplish the goal of deprecating Shibboleth and Thurloe.


PR checklist

  • I've followed the instructions if I've made any changes to the API, especially if they're breaking changes
  • I've filled out the Security Risk Assessment (requires Broad Internal network access) and attached the result to the JIRA ticket

@@ -53,6 +54,13 @@ trait ServiceAdminRoutes extends SecurityDirectives with SamRequestContextDirect
.map(users => (if (users.nonEmpty) OK else NotFound) -> users)
}
}
} ~
postWithTelemetry(samRequestContext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldnt this be get?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking POST, since the number of IDs could get pretty unwieldy in a url.

complete {
userService.getUsersByIds(samUserIds, samRequestContext)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a limit on how many you can get at one time?

// Assert
assert(response.isEmpty, "Getting a nonexistent user should not find a user")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

should we have any test cases that assert the admin only functionality or is that properly covered by other tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is covered by other tests, but I'll check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found the test, but it was just named slightly wrong. It tests that only authorized service accounts can call the API

$ref: '#/components/schemas/User'
403:
description: You do not have service admin privileges
content: { }
Copy link
Contributor

@samanehsan samanehsan Jul 23, 2024

Choose a reason for hiding this comment

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

Why isn't this #/components/schemas/ErrorReport like the 500 error?

type: array
items:
$ref: '#/components/schemas/User'
403:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you also need a block for a 400 response here (if someone requests > 1000 users)

Copy link

@tlangs tlangs merged commit 29debc4 into develop Jul 24, 2024
24 checks passed
@tlangs tlangs deleted the tl_ID-1300_sam_get_users_by_id branch July 24, 2024 14:33
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