[provision group] Check emails / usernames against existing users

[jameshadfield]

The provision-users command requires us to provide both a username and email for each user we want to add. A common use case is being given a (long) list of emails and wanting to check if a user already exists for each email, which is exactly what this script does.

Once this PR is merged i'll add instructions to the wiki docs

• Checks pass
• Check if changes affect the resource index JSON revision

[victorlin]

This seems useful! Instead of a separate script, how about adding it as a validation step within the provision-group script? Example: 434dff7

$ ./scripts/provision-group test --members new-members.yaml

PROGRAMMING ERROR: Caught unhandled rejection of 1 promise:
  Promise {
    <rejected> Error: Email [email protected] doesn't yet exist but the username 'victorlin-test' is already associated with '[email protected]'
        at queryUsernames (file:///Users/vlin/Dropbox/repos/nextstrain/nextstrain.org/scripts/provision-group.js:487:17)
        at readMembersFile (file:///Users/vlin/Dropbox/repos/nextstrain/nextstrain.org/scripts/provision-group.js:123:3)
        at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
        at async main (file:///Users/vlin/Dropbox/repos/nextstrain/nextstrain.org/scripts/provision-group.js:69:7)
  }

[jameshadfield]

Instead of a separate script, how about adding it as a validation step within the provision-group script?

There's a few reasons I chose not to go down this road. My script needs to list all the cognito users (as you can't query by email, AFAIK), which is unnecessary overhead for most use cases of provision-group. Secondly, I allow the username to be left out, whereas provision-group enforces it (could be fixed). Thirdly, I wanted to use the script with confidence no changes would be made on the cognito side; I sketched out adding --dry-run to provision-group (which I think would be a very good thing) but ultimately found it easier to make a separate script. Finally I deemed the bar for addition to provision-group to be higher than a script like this (ensuring all exceptions are handled etc), as their purposes and importance are quite different. If you want to take this direction on then by all means please do - there are a few emails with multiple usernames in the production pool, which I don't think is ideal.

[victorlin]

Thanks for the explanation. I only checked against the testing user pool so I didn't realize the overhead on the production user pool. I understand your decision better now and feel less strongly about adding to provision-group.

Thread for tangential discussion below.

[victorlin]

there are a few emails with multiple usernames in the production pool, which I don't think is ideal.

The ability to do so seems intentional. Skimming through the code, everything is keyed on username (also apparent on https://nextstrain.org/login). Many services allow multiple accounts for a single email. This can be useful for things such as one person having separate accounts for different levels of access.

[jameshadfield]

Yeah - for sure cognito is keyed off username, and by extension nextstrain is as well.

This can be useful for things such as one person having separate accounts for different levels of access

I guess so? Outside of testing purposes I don't quite see the point of one person maintaining different access levels for the same group, but I'm sure there are use cases.

In the few examples I've seen (and thus anecdotal) I think we've created a username/email combo for one group, then for a subsequent group been given a list of emails and used the provision-groups script to add a new username for the same email. From the users perspective that's not ideal (need to login with different usernames to see different groups), but for the users in question I believe their usage is very low at the moment.

[victorlin]

Example use case for multiple usernames under the same email.

I don't doubt that some of the existing many-to-one username/email instances are not ideal, so maybe it'd be good to print a warning such as

WARNING: Email <email> exists with username(s) <name> and you are asking for the new username <name> to be created. Confirm that this is intentional.

In the future when user creation is self-service, it would be good to add an extra confirmation step to this scenario.

[tsibley]

My script needs to list all the cognito users (as you can't query by email, AFAIK),

You can absolutely search by email, e.g.
https://docs.aws.amazon.com/cognito/latest/developerguide/how-to-manage-user-accounts.html#manage-user-accounts-searching-user-attributes

[victorlin]

I just used this for 88c3092 – very useful! I'm good for this to be merged pending one small change below.

It would be nice to run this automatically as part of validation in provision-group. But given the long-term plan to allow self-service, maybe no need to invest much additional effort here.

[victorlin]

Suggested change

	console.log(`${existingMsg} and neither does the username '${member.username}. ALL GOOD.'`)
	console.log(`${existingMsg} and neither does the username '${member.username}'. ALL GOOD.`)

[victorlin]

Shebang indicates that this should be executable but it isn't:

$ ./scripts/provision-group-check-users.js members.yaml       
zsh: permission denied: ./scripts/provision-group-check-users.js

Fix:

chmod a+x scripts/provision-group-check-users.js

[victorlin]

Bump: It's been helpful to run this while handling Groups requests. Currently I switch to the PR branch just to run it.