-
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
authn: Generalize support to ~any OIDC/OAuth2 IdP, not just AWS Cognito #731
Merged
Merged
Changes from all commits
Commits
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
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 |
---|---|---|
@@ -1,6 +1,9 @@ | ||
{ | ||
"COGNITO_BASE_URL": "https://login.nextstrain.org", | ||
"COGNITO_CLIENT_ID": "rki99ml8g2jb9sm1qcq9oi5n", | ||
"COGNITO_CLI_CLIENT_ID": "2vmc93kj4fiul8uv40uqge93m5", | ||
"COGNITO_USER_POOL_ID": "us-east-1_Cg5rcTged" | ||
"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_LOGOUT_URL": "https://login.nextstrain.org/logout", | ||
"COGNITO_USER_POOL_ID": "us-east-1_Cg5rcTged", | ||
"OIDC_USERNAME_CLAIM": "cognito:username", | ||
"OIDC_GROUPS_CLAIM": "cognito:groups" | ||
} |
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 |
---|---|---|
@@ -1,6 +1,9 @@ | ||
{ | ||
"COGNITO_BASE_URL": "https://nextstrain-testing.auth.us-east-1.amazoncognito.com", | ||
"COGNITO_CLIENT_ID": "6qiojrhr8tibt0f6hphnm1osp1", | ||
"COGNITO_CLI_CLIENT_ID": "9opa27o74f4jsq8g4a34e1mqr", | ||
"COGNITO_USER_POOL_ID": "us-east-1_zqpCrjM7I" | ||
"OIDC_IDP_URL": "https://cognito-idp.us-east-1.amazonaws.com/us-east-1_zqpCrjM7I", | ||
"OAUTH2_CLIENT_ID": "6qiojrhr8tibt0f6hphnm1osp1", | ||
"OAUTH2_CLI_CLIENT_ID": "9opa27o74f4jsq8g4a34e1mqr", | ||
"OAUTH2_LOGOUT_URL": "https://nextstrain-testing.auth.us-east-1.amazoncognito.com/logout", | ||
"COGNITO_USER_POOL_ID": "us-east-1_zqpCrjM7I", | ||
"OIDC_USERNAME_CLAIM": "cognito:username", | ||
"OIDC_GROUPS_CLAIM": "cognito:groups" | ||
} |
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.
It's interesting how in AWS, there is a clear distinction between IAM users/groups and Cognito users/groups, while in Azure with the current setup, it's all under AAD. I wonder if this has implications for organizations that manages Azure resources with AAD users.
Did you look into something like Azure AD B2C? I haven't used it personally, but it looks to be quite different from AAD, made clearer by name with the recent rebranding.
I'm asking these questions, but I don't think we should ponder too much because this seems to be working well for CDC's use case.
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 didn't look into Azure AD B2C, but looking now, yes, it's much more akin to Cognito. e.g. conceptually we could swap out our usage of Cognito for Azure AD B2C, but we wouldn't swap Cognito for Azure AD/Entra ID.
In CDC's case, they want to use their existing user directory in Azure AD and since they're also hosting the application themselves, it's simplest to authenticate against Azure AD directly. While they could set up an Azure AD B2C instance just for this app and then invite specific users from their Azure AD into their Azure AD B2C, it would just be more layers of indirection for not any functional gain. The gain would be more administrative/organizational, e.g. if for some reason they didn't want to directly authn against Azure AD or if they wanted a harder adminstrative split.
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.
Maybe to sum that up: There's a lot of ways to do user directories for authn purposes, but no one right way; it's highly dependent on the needs/context/where the users already are.