-
Notifications
You must be signed in to change notification settings - Fork 49
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
Support for Nextstrain CLI's new means of authentication with IdPs #757
Conversation
…son.gz Based on changes @jameshadfield made in the AWS Console, but stripped down to just the single object necessary by the current consuming code. (Cherry-picked from d6198a4 so I can temporarily deploy changes to testing for my branch without reverting the changes in this commit. I'd prefer to set up a separate branch-specific Terraform config/environment, but that's not a workable solution for this issue until IAM _users_ are also integrated into our Terraform config.)
Expanding the information on each client will make this necessary. Plus, it's more clear what's shared by both with that information now outside of either section.
env/production/config.json
Outdated
@@ -3,6 +3,7 @@ | |||
"OIDC_IDP_URL": "https://cognito-idp.us-east-1.amazonaws.com/us-east-1_Cg5rcTged", | |||
"OAUTH2_CLIENT_ID": "rki99ml8g2jb9sm1qcq9oi5n", | |||
"OAUTH2_CLI_CLIENT_ID": "2vmc93kj4fiul8uv40uqge93m5", | |||
"OAUTH2_CLI_CLIENT_REDIRECT_URIS": [], |
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.
This should be filled out before deploy, after applying Terraform updates to production.
To clarify something just discussed with Victor: this could get merged and deployed without affecting current Nextstrain CLI clients. The main changes are a new nextstrain.org server endpoint and additional allowed OAuth flows for the CLI's client registration with Cognito. Both are used by work-in-progress Nextstrain CLI changes TKTK, but both are additive and not impacting the current CLI auth flow. |
@victorlin @jameshadfield Thoughts on if I should just deploy 1019326 with this branch? Or drop it and leave it for #719? |
(I'm inclined to just deploy it since it's read-only and public data.) |
Seems fine to me, it'll make the dev process easier. |
Nextstrain CLI will start using OIDC/OAuth2's authorization code flow to interact with not just AWS Cognito but other IdPs as well (i.e. as used in other deployments of nextstrain.org). To support this without hardcoding or onerous user-side configuration, Nextstrain CLI will start using the standard OIDC configuration endpoint, /.well-known/openid-configuration, to auto-discover necessary configuration about both the IdP to talk to and the client it should be. Terraform changes are deployed to both production and testing as they're additive and will not impact current CLI auth flow. Related-to: <nextstrain/private#94>
17722df
to
895d308
Compare
Nextstrain CLI will start using OIDC/OAuth2's authorization code flow to interact with not just AWS Cognito but other IdPs as well (i.e. as used in other deployments of nextstrain.org).
To support this without hardcoding or onerous user-side configuration, Nextstrain CLI will start using the standard OIDC configuration endpoint, /.well-known/openid-configuration, to auto-discover necessary configuration about both the IdP to talk to and the client it should be.
Related-to: https://github.com/nextstrain/private/issues/94
Related CLI PR: TKTK
Checklist