-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Update documentation for social authentication with Amazon Cognito #8557
base: develop
Are you sure you want to change the base?
Conversation
WalkthroughThe document "Social auth configuration" has been updated to clarify the process of enabling social authentication for Enterprise clients in a Self-Hosted solution. Key changes include an introductory note about client eligibility, an expanded section on the benefits of social authentication, and a restructured two-step process for configuring Amazon Cognito. This includes detailed instructions for creating a user pool and setting up an Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
site/content/en/docs/enterprise/social-accounts-configuration.md (2)
Line range hint
1-22
: Excellent additions to the introductory section!The new note about Enterprise client availability and the expanded benefits of social authentication provide valuable context for users. This improvement helps set clear expectations and highlights the advantages of the feature.
Consider adding a brief sentence or two about the security measures implemented by social auth providers to reinforce the security benefit mentioned.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~126-~126: Possible missing comma found.
Context: ...-1.amazoncognito.com`). Copy this domain as you'll need it later when configurin...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~146-~146: Possible missing comma found.
Context: ...etcould be found on the page with app details while the
domain` value could be...(AI_HYDRA_LEO_MISSING_COMMA)
Line range hint
102-155
: Excellent improvements to the Amazon Cognito authentication section!The expanded and restructured content provides a much more comprehensive guide for setting up Amazon Cognito authentication. The addition of detailed steps and references to visual aids significantly enhances the user experience and clarity of the instructions.
Consider the following minor improvements:
- Line 118: Change "Configure a new app on the step
Integrate your app
:" to "Configure a new app in theIntegrate your app
step:"- Line 126: Add a comma after "Copy this domain" for better readability.
- Line 137: The indentation of the YAML content seems off. Ensure consistent indentation throughout the YAML snippet.
- Line 146: Add a comma after "app details" for better readability.
- Line 151: There's a backtick (`) instead of a quotation mark (") at the end of the line. Replace it with a quotation mark.
These minor adjustments will further improve the overall quality and consistency of the documentation.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~126-~126: Possible missing comma found.
Context: ...-1.amazoncognito.com`). Copy this domain as you'll need it later when configurin...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~146-~146: Possible missing comma found.
Context: ...etcould be found on the page with app details while the
domain` value could be...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
site/content/en/images/cognito_pool_1.png
is excluded by!**/*.png
site/content/en/images/cognito_pool_2.png
is excluded by!**/*.png
site/content/en/images/cognito_pool_3.png
is excluded by!**/*.png
📒 Files selected for processing (1)
- site/content/en/docs/enterprise/social-accounts-configuration.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
site/content/en/docs/enterprise/social-accounts-configuration.md
[uncategorized] ~126-~126: Possible missing comma found.
Context: ...-1.amazoncognito.com`). Copy this domain as you'll need it later when configurin...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~146-~146: Possible missing comma found.
Context: ...etcould be found on the page with app details while the
domain` value could be...(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (3)
site/content/en/docs/enterprise/social-accounts-configuration.md (3)
Line range hint
24-61
: Google authentication section looks good!The instructions for setting up Google authentication remain clear and comprehensive. The minor formatting improvements enhance readability and maintain consistency with other sections.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~126-~126: Possible missing comma found.
Context: ...-1.amazoncognito.com`). Copy this domain as you'll need it later when configurin...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~146-~146: Possible missing comma found.
Context: ...etcould be found on the page with app details while the
domain` value could be...(AI_HYDRA_LEO_MISSING_COMMA)
Line range hint
63-100
: GitHub authentication section is well-maintained!The instructions for setting up GitHub authentication remain clear and comprehensive. The minor formatting improvements enhance readability and maintain consistency with other sections.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~126-~126: Possible missing comma found.
Context: ...-1.amazoncognito.com`). Copy this domain as you'll need it later when configurin...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~146-~146: Possible missing comma found.
Context: ...etcould be found on the page with app details while the
domain` value could be...(AI_HYDRA_LEO_MISSING_COMMA)
Line range hint
157-161
: Final command section is correct and consistent.The docker compose command provided for running the setup remains unchanged and is consistent with the instructions in other sections. This ensures a uniform approach across different authentication methods.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~126-~126: Possible missing comma found.
Context: ...-1.amazoncognito.com`). Copy this domain as you'll need it later when configurin...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~146-~146: Possible missing comma found.
Context: ...etcould be found on the page with app details while the
domain` value could be...(AI_HYDRA_LEO_MISSING_COMMA)
0d6e439
to
c9b0b06
Compare
Quality Gate passedIssues Measures |
3. Create conпiguration file in CVAT: | ||
|
||
1. Create the `auth_config.yml` file with the following content: | ||
## Enable Authentication with Amazon Cognito |
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.
## Enable Authentication with Amazon Cognito | |
## Enable authentication with Amazon Cognito |
The other headings don't use title case.
|
||
- Configure a new app on the step `Integrate your app`: | ||
- Select the `Confidential client` type, as CVAT securely stores | ||
client secrets on the server side. |
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.
client secrets on the server side. | |
client secrets on the server side. |
- Configure a new app on the step `Integrate your app`: | ||
- Select the `Confidential client` type, as CVAT securely stores | ||
client secrets on the server side. | ||
(_Note_: the `Public client` type is also supported.) |
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.
Why add this note? CVAT is a confidential client, Cognito supports confidential clients... there's no reason to configure it as a public client.
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.
CVAT is a confidential client, Cognito supports confidential clients...
I know, but there was such a situation when the customer configured a public client instead of a confidential one. I decided to mention that, but okay, I'll delete this mention.
|
||
1. **Create and configure an [Amazon Cognito user pool](https://docs.aws.amazon.com/cognito/latest/developerguide/cognito-user-identity-pools.html)**. | ||
Follow these detailed steps to set up the user pool: | ||
- Create a user pool. For example, you might use settings like those shown in the image below: |
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 seems to start in the middle. I would assume the first step would be "go to the AWS Management Console", then "click XXX to go to Cognito settings"...
Also, the steps should be in a numbered list, since they are sequential.
![](/images/cognito_pool_2.png) | ||
|
||
- Once your pool is configured, go to the `App integration` tab | ||
on the pool details page and then to the `Domain` section. |
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.
on the pool details page and then to the `Domain` section. | |
on the pool details page and then to the `Domain` section. |
|
||
To enable authentication with Amazon Cognito for your CVAT instance, you need to complete two main steps: | ||
|
||
1. **Create and configure an [Amazon Cognito user pool](https://docs.aws.amazon.com/cognito/latest/developerguide/cognito-user-identity-pools.html)**. |
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 wonder if this is really the right approach here.
I would assume a sizable fraction of users who want to configure authentication via Cognito already have a pool there. The whole point of Cognito is to share users between multiple apps, so unless you're only starting to set up SSO and CVAT happens to be your first app, you would already have a pool.
Given this, perhaps it would be better to focus this section on configuring an existing pool, and add an optional extra step in front that tells people to create a new pool if they don't have one.
|
||
- Once your pool is configured, go to the `App integration` tab | ||
on the pool details page and then to the `Domain` section. | ||
Create either a custom domain or Cognito domain (e.g., `https://cvat.auth.us-east-1.amazoncognito.com`). |
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.
cvat
is probably not the best suggestion here, as a) we shouldn't assume the pool is just for CVAT (if it is, why use Cognito at all?), and b) the name has to be unique within the region, so different organizations would need to use different names. I would replace cvat
with a placeholder like <my_org>
. Or just stick with custom-cognito-prefix
, like in the config below.
# schema: http|https, CVAT_PORT is optional | ||
export CVAT_BASE_URL="<SCHEMA>://${CVAT_HOST}:<CVAT_PORT>" |
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.
# schema: http|https, CVAT_PORT is optional | |
export CVAT_BASE_URL="<SCHEMA>://${CVAT_HOST}:<CVAT_PORT>" | |
# cvat_port is optional | |
export CVAT_BASE_URL="<http|https>://${CVAT_HOST}:<cvat_port>" |
The other placeholders are lowercase, so cvat_port
ought to be too.
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.
"CVAT" would make more sense as the client name.
Motivation and context
Added more details on how to configure Amazon Cognito pool
How has this been tested?
Checklist
develop
branch- [ ] I have created a changelog fragment- [ ] I have added tests to cover my changes- [ ] I have linked related issues (see GitHub docs)- [ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit