-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add RESTful API for managing Groups members #581
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
../variable-COGNITO_USER_POOL_ID.tf |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
variable "COGNITO_USER_POOL_ID" { | ||
type = string | ||
description = "Id of the Cognito user pool for this environment" | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,4 +46,6 @@ module "iam" { | |
} | ||
|
||
env = "testing" | ||
|
||
COGNITO_USER_POOL_ID = module.cognito.COGNITO_USER_POOL_ID | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
/** | ||
tsibley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* Cognito user pool (IdP) management. | ||
* | ||
* @module cognito | ||
*/ | ||
/* eslint-disable no-await-in-loop */ | ||
import { | ||
CognitoIdentityProviderClient, | ||
AdminAddUserToGroupCommand, | ||
AdminRemoveUserFromGroupCommand, | ||
paginateListUsersInGroup, | ||
UserNotFoundException, | ||
} from "@aws-sdk/client-cognito-identity-provider"; | ||
|
||
import { COGNITO_USER_POOL_ID } from "./config.js"; | ||
import { NotFound } from "./httpErrors.js"; | ||
|
||
|
||
const REGION = COGNITO_USER_POOL_ID.split("_")[0]; | ||
|
||
const cognito = new CognitoIdentityProviderClient({ region: REGION }); | ||
|
||
|
||
/** | ||
* Retrieve AWS Cognito users in a Cognito group. | ||
* | ||
* @param {string} name - Name of the AWS Cognito group | ||
* @yields {object} user, see <https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/clients/client-cognito-identity-provider/interfaces/usertype.html> | ||
*/ | ||
export async function* listUsersInGroup(name) { | ||
const paginator = paginateListUsersInGroup({client: cognito}, { | ||
UserPoolId: COGNITO_USER_POOL_ID, | ||
GroupName: name, | ||
}); | ||
|
||
for await (const page of paginator) { | ||
yield* page.Users; | ||
} | ||
} | ||
|
||
|
||
/** | ||
* Add an AWS Cognito user to a Cognito group. | ||
* | ||
* @param {string} username | ||
* @param {string} group - Name of the AWS Cognito group | ||
* @throws {NotFound} if username is unknown | ||
*/ | ||
export async function addUserToGroup(username, group) { | ||
try { | ||
await cognito.send(new AdminAddUserToGroupCommand({ | ||
UserPoolId: COGNITO_USER_POOL_ID, | ||
Username: username, | ||
GroupName: group, | ||
})); | ||
} catch (err) { | ||
if (err instanceof UserNotFoundException) { | ||
throw new NotFound(`unknown user: ${username}`); | ||
} | ||
throw err; | ||
} | ||
} | ||
|
||
|
||
/** | ||
* Remove an AWS Cognito user from a Cognito group. | ||
* | ||
* @param {string} username | ||
* @param {string} group - Name of the AWS Cognito group | ||
* @throws {NotFound} if username is unknown | ||
*/ | ||
export async function removeUserFromGroup(username, group) { | ||
try { | ||
await cognito.send(new AdminRemoveUserFromGroupCommand({ | ||
UserPoolId: COGNITO_USER_POOL_ID, | ||
Username: username, | ||
GroupName: group, | ||
})); | ||
} catch (err) { | ||
if (err instanceof UserNotFoundException) { | ||
throw new NotFound(`unknown user: ${username}`); | ||
} | ||
throw err; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something you want to start implementing in this PR with an endpoint like
POST /groups/:groupName/settings/members/add
? I assume it would behave similarly to some functions in the existingscripts/provision-group
.For naming reference, I went on a tangent and compiled a list of GitHub's endpoints for adding a new member to an organization. Their URLs are all over the place, so probably not a good source of inspiration:
https://github.com/orgs/<org>/people
: members listhttps://github.com/orgs/<org>/outside-collaborators
: outside collaboratorshttps://github.com/orgs/<org>/pending_collaborators
: pending collaboratorshttps://github.com/orgs/<org>/invitations/member_adder_add
: clicking the Invite button sent a request to this URLhttps://github.com/orgs/<org>/invitations/<username>/edit
: direct link that works in browserThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like that. Not sure. Behaviour would be similar to internal provisioning, yep. I don't think I want to gate this PR on it, though, as I don't think it's strictly necessary to do so. We still have work to do for UI for these endpoints anyway (CLI and/or web).
Some questions around invitations:
Are we implementing a true invite → accept/decline invite → membership added flow from the start? Or just letting folks add anyone without an accept/decline step? Latter is much easier (esp. when we don't have our own database), but former is what we'll eventually want (although it may be quite a while till it's actually necessary to prevent abuse).
How do we customize emails from Cognito when a new person is invited for a group? We probably have to send our own separately if we're going to include details of the membership transaction.
Inviting is a little awkward with Cognito alone because it means the the invitee doesn't get a say in their username. Either we let the person inviting pick it (ha, nope!) or we force it internally (e.g. to match email). The latter is probably workable, but has privacy implications later, e.g. a username can be expected to be public but an email not so much.
and there are surely more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we almost certainly need to do the inviting new users part mostly outside of Cognito, and only touch Cognito when the user accepts (at which point they get a say in their user details and we add them to the right group+role).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to open up the user pool for self sign-up with admin confirmation? We can add an endpoint for the owner to just confirm new users and add them to the appropriate roles.
Then the user -> group communication can happen outside of our domain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, that sounds like a useful piece of the puzzle. I'd plain forgot about the self sign-up options! I'm not sure we'd even need the admin confirmation bit. Would need to think thru all the flows a bit more.
Earlier in the day, I'd idly noodled a bit on issuing stateless invitations (where all the state is encoded in a token we send the invited user, who hands it back to us (or not) when accepting/declining (or ignoring)). This ~works if invitations have a TTL and we're ok holding a small amount of state for a short time (TTL+1) when an invitation is rescinded. But what we can do with that approach is necessarily less than if we hold state on outstanding invitations, so it's not entirely appealing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggested the admin confirmation as a way for group owners to "vouch" for new users, but I guess adding them to their group has the same effect.
How much do we care about random people signing up for accounts? They won't have access to things unless they are added to a group, but do we have to worry about filling up Cognito quotas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we care too much about random signups unaffiliated with a group. It's pointless, but I don't think we need to prevent it if it's easier to do a group invite flow without a confirmation step. We need not be concerned about user quotas in Cognito: the limit is 40 million. There are also operational quotas that apply to user signups, logins, etc. which would be impacted by additional not-necessary users, but I think we're unlikely to hit them either.