-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
CVAT authentication #5147
CVAT authentication #5147
Conversation
cvat/apps/iam/templates/account/email/email_confirmation_signup_message.html
Fixed
Show fixed
Hide fixed
/check |
❌ Some checks failed |
cb62be7
to
707edac
Compare
707edac
to
b52285f
Compare
/check |
✔️ All checks completed successfully |
<meta http-equiv="x-ua-compatible" content="ie=edge"> | ||
<title>Email Confirmation</title> | ||
<meta name="viewport" content="width=device-width, initial-scale=1"> | ||
<style type="text/css"> |
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.
please make sure the template is correctly rendered in the popular email clients like gmail, apple mail, etc
} | ||
) | ||
@action(detail=False, methods=['GET'], url_path='advanced-auth', permission_classes=[]) | ||
def advanced_authentication(request): |
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.
Shouldn't this endpoint be a part of IAM app?
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 guess no, because it's part of server configuration
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.
@Marishka17 , I agree that the question is very ambiguous. But I also think that it is more IAM configuration than the server. I will prefer to have something like /api/auth/methods
or something similar.
@@ -571,3 +575,47 @@ def add_ssh_keys(): | |||
'SCHEMA_PATH_PREFIX': '/api', | |||
'SCHEMA_PATH_PREFIX_TRIM': False, | |||
} | |||
|
|||
# allauth configuration | |||
USE_ALLAUTH_SOCIAL_ACCOUNTS = strtobool(os.getenv('USE_ALLAUTH_SOCIAL_ACCOUNTS') or 'False') |
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.
or 'False'
is really need here?
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.
if an env variable is set to empty string then strtobool will not be able to convert it to bool
…&& fix login when email is private
/check |
✔️ All checks completed successfully |
/check |
✔️ All checks completed successfully |
@@ -574,3 +579,52 @@ def add_ssh_keys(): | |||
'SCHEMA_PATH_PREFIX': '/api', | |||
'SCHEMA_PATH_PREFIX_TRIM': False, | |||
} | |||
|
|||
# allauth configuration | |||
USE_ALLAUTH_SOCIAL_ACCOUNTS = strtobool(os.getenv('USE_ALLAUTH_SOCIAL_ACCOUNTS') or 'False') |
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.
USE_ALLAUTH_SOCIAL_ACCOUNTS = strtobool(os.getenv('USE_ALLAUTH_SOCIAL_ACCOUNTS') or 'False') | |
USE_ALLAUTH_SOCIAL_ACCOUNTS = strtobool(os.getenv('USE_ALLAUTH_SOCIAL_ACCOUNTS', 'False')) |
SOCIALACCOUNT_QUERY_EMAIL = True | ||
SOCIALACCOUNT_CALLBACK_CANCELLED_URL = '/auth/login' | ||
|
||
GITHUB_CALLBACK_URL = 'http://localhost:8080/api/auth/github/login/callback/' |
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.
@Marishka17 , Can we avoid specifying here http://localhost:8080 and calculate it automatically in runtime?
@@ -14,6 +14,15 @@ export type StringObject = { | |||
[index: string]: string; | |||
}; | |||
|
|||
enum AdvancedAuthMethods { | |||
GOOGLE_ACCOUNT_AUTHENTICATION = 'GOOGLE_ACCOUNT_AUTHENTICATION', |
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.
@Marishka17 , @bsekachev , do you think we can avoid duplication? We get the same information from the server. I will prefer to have a REST API method to get all the necessary information from the server to render UI. Now we have a list of available methods on the server and UI at the same time.
@Marishka17 , let's merge as is. But I expect that you fix the comments in your next PRs. |
Motivation and context
PR contains changes to support social authentication with google and github providers. Also several bugs with related field were fixed. Resolved issue #4820. There is now possible to request another mail with email confirmation link.
Resolved issue #4832 with attempt to confirm email one more time with the same confirmation link.
I guess this issue #4837 can also be closed because now we can send another confirmation email.
How has this been tested?
Manually
Checklist
develop
branchcvat-core, cvat-data and cvat-ui)
License
Feel free to contact the maintainers if that's a concern.