Add RESTful API for managing Groups members

[tsibley]

Description of proposed changes

Adds RESTful API endpoints for listing/adding/removing existing users to group membership roles.

Things to address later:

• Currently no way to invite new users. Need to sort out the desired interface/mechanism for this. Discussion.

  Split out into Design new user invitation / user signup flow #729.

• Automated tests for these endpoints/functions now that we have a shiny new testing environment (Terraform #598, Add a testing environment for Cognito #656, aws/iam: Manage IAM resources for the testing environment #657).

  Split out into Automated integration tests for Groups member management API endpoints #728.

• Currently this branch is missing a good, safe solution for dev and testing of these API endpoints. I really don't want to grant all dev/test instances (local, GitHub Actions, Heroku review apps, etc) permissions to modify all groups, but AWS IAM policies don't support a finer resource specificity than the user pool, so we can't use the same approach we do with S3 buckets and prefix-scoping.

  Given these conflicting constraints, a separate dev-only user pool seems most appropriate, but that's a bit more initial work to setup and more ongoing work to manage/keep relevant over time. I'm a little loathe to address that now or as a prerequisite for this work, but I also don't like not having a dev/testing story here.

  We're rapidly reaching (or are at?) the point where I really want separate dev vs. prod AWS accounts controlled by Terraform or CloudFormation something.

Testing

• Manual testing of endpoints
• Add new tests
• Local tests pass
• CI passes

After merge

• Deploy env/production Terraform config

[tsibley]

Code is ready to review as-is as it's in what I'd consider to be a deployable state (though still missing some important functionality). I appreciate feedback/thoughts from folks on the two items "to address" listed at the top. I haven't had a chance to think on them any more yet, but will do so tomorrow or next week as well.

[victorlin]

Currently no way to invite new users. Need to sort out the desired interface/mechanism for this.

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 existing scripts/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 list
• https://github.com/orgs/<org>/outside-collaborators: outside collaborators
• https://github.com/orgs/<org>/pending_collaborators: pending collaborators
• https://github.com/orgs/<org>/invitations/member_adder_add: clicking the Invite button sent a request to this URL
• https://github.com/orgs/<org>/invitations/<username>/edit: direct link that works in browser
• ...

[tsibley]

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.

[tsibley]

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).

[joverlee521]

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.

[tsibley]

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.

[joverlee521]

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?

[tsibley]

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.

[tsibley]

Reconverted to back to draft. I'm going to:

• Rebase this on top of @victorlin's work in Add RESTful API endpoints for Groups customizations #570 and wait to mark this ready for review until that's finalized (soon I think!).
• Bite the bullet and make the dev/test Cognito user pool so we can properly test this. I think it'll be best in the long run too, so I might as well do it now.
• Address @victorlin's two small review comments above.
• Think thru the story for new user invitations / user signup and try to write something up for those flows… but not condition this PR on it in order to keep things moving forward.

[tsibley]

As part of figuring out the story here for dev/test, I'm going to (finally) get a Terraform config set up for us. I'll start by modeling a subset of our current AWS resources (the most relevant ones), then importing live state for them into Terraform, and then from there have a better basis for a prod/dev+test split.

[tsibley]

Ok, I wrote a base Terraform configuration in #598. Next week I'll rebase this branch on top of that and then write the bits for a dev/prod split (at least in Cognito, but maybe larger? not sure yet).

[tsibley]

Taking this out of draft because I'd like to merge it without blocking any longer on comprehensive automated tests for the new endpoints. The Terraform and testing environment work I've done (e.g. see this comment) allows for safe manual testing and development. Automated tests would still be great, and I'd started at one point down that hole, but quickly realized that some crucial infrastructural pieces were still missing, including:

1. obtaining authn tokens (e.g. logging in) in tests (or mechanisms to bypass authn in tests 😬)
2. test fixtures for groups and users, and how to deal with concurrency (e.g. multiple instances of the tests running at the same time)

At that point I started to shimmy back up out of the rabbit hole. These missing pieces are totally doable and we should absolutely have those things some day, but I can't (and don't want to) keep implementing development infrastructure at the expense of feature development.

The desire to merge this comes up again now because I've been using this PR branch as the base branch for my prototyping/WIP for CDC to generalize authn to support any OIDC/OAuth2 IdP. That's because this branch has changes that would need to be accounted for by that work, and I'd rather account for that up front rather than later, given that we do still want this branch's endpoints so we can make the UI and other stuff.

Requested changes made long ago

[victorlin]

Testing commands

# Get a token
export NEXTSTRAIN_DOT_ORG=http://localhost:5000
export NEXTSTRAIN_COGNITO_USER_POOL_ID="$(jq -r .COGNITO_USER_POOL_ID ../nextstrain.org/env/testing/config.json)"
export NEXTSTRAIN_COGNITO_CLI_CLIENT_ID="$(jq -r .COGNITO_CLI_CLIENT_ID ../nextstrain.org/env/testing/config.json)"
nextstrain login

# Listing members works
curl http://localhost:5000/groups/test/settings/members \
    --header @<(nextstrain authorization) | jq

# Listing roles works
curl http://localhost:5000/groups/test/settings/roles \
    --header @<(nextstrain authorization) | jq

# Listing role members works
curl http://localhost:5000/groups/test/settings/roles/editors/members \
    --header @<(nextstrain authorization) | jq

# Add myself as an editor
curl -X PUT http://localhost:5000/groups/test/settings/roles/editors/members/victorlin-owner \
    --header @<(nextstrain authorization)

nextstrain login --renew

# Remove myself as an editor
curl -X DELETE http://localhost:5000/groups/test/settings/roles/editors/members/victorlin-owner \
    --header @<(nextstrain authorization)

[victorlin]

Note: I cherry-picked ff55997 and applied these changes via Terraform while testing.

[tsibley]

@victorlin Note that for easier curl usage, you can use the nextstrain authorization command like so:

curl ... --header @<(nextstrain authorization)

(This also has the advantage of not leaving the token in shell variables and not making it machine-visible via process arguments.)

[tsibley]

Rebased to resolve conflict introduced by #720. Will wait for tests to pass in CI before merging.