[Feature] ORCID login [#OSF-5162, 6881, 6885] #6192
Merged
Conversation
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
- add `oauth` dictionary field
- update `get_user()` to retrieve user based on oauth provider and id
- add `get_user_from_cas_response(cas_resp)` to recognize oauth credential
- add `oauth_first_time_authenticate(oauth_user, response)` to
- create a unauthenticated session to store user oauth credentials
- redirect to a new view where user is asked to enter email
- if user exists (either fromm user id or oauth credential), go to `authenticate(user, access_token, response)`
- if oauth credential exists but no user found, go to user `oauth_first_time_authenticate(oauth_user, response)`
- otherwise, unauthorized
… create or link OSF accounts. Part 1.
- general struture guide
- update routes, makos, and forms
- add `Session.is_oauth_first_time_login` and only allow the views to be reached by oauth first login
- both link and create requires user verify email
- session and cookie should be removed after done
- create user, send confirmation email, verify email
- use ${oauth_provider} in `get_oauth_email.mako`
- add push notifications
- `User.oauth` -> `User.external_identity`
- update related mehtods, views, routes, makos change
- 'VERIFIED', 'CREATE', 'LINK'
- update `get_user` to check `status` and only return verified user
- `send_confirm_email` takes extra parameter `external_identity` and use dedicated email templates
- `exteranl_login_email_post()`: return a warn message if user is already associated with another external idp
- update mail templates and messsages
…ccount creation.
- add new routes `/confirm/external/<uid>/<token>` for confirmation link
- add new view `external_login_confirm_email_get` to simplify account creation and linking
- update `add_unconfirmed_email`, `send_confirm_email`,`get_confirmation_email` and `create_unconfirmed`
- update TODO list and comments
| @@ -19,6 +19,7 @@ | |||
| 'get_user', | |||
| 'check_password', | |||
| 'authenticate', | |||
| 'oauth_first_time_authenticate', | |||
There was a problem hiding this comment.
This probably needs to be updated.
-still incorrect, fix later
|
|
||
| return { | ||
| 'form': form, | ||
| 'external_id_provider': external_id_provider.upper() |
There was a problem hiding this comment.
This shouldn't be calling .upper()
There was a problem hiding this comment.
Agreed. external_id_provider used to be all lower-case and this .upper() is only for ORCID . With the new PR, it becomes what they should be in default settings for different external identity providers. There are other places that I used .upper() and I will fix all.
- `get_user_from_cas_resp()` returns a tuple: user, external_credential, action
- `validate_external_credential()` parse and valid provider and id
- update user data for `external_first_login_authenticate()`
- fix typo in comments
- update #TODO comments
- use `.split()` instead of `.rsplit()`
- remove `.upper()` on `external_id_provider
mfraezz
changed the title
- fix typo in comments
- update #TODO comments
- use `.split()` instead of `.rsplit()`
- remove `.upper()` on `external_id_provider
-link TODOs to tickets
|
Related CAS PR: CenterForOpenScience/cas-overlay#22. |
* Add front-end, v1 code for revoking ORCID * Update social field on link
-because google
mfraezz
force-pushed
the
feature/orcid-login
branch
from
7b94876
to
19e7300
Compare
| @@ -264,7 +264,7 @@ def make_response_from_ticket(ticket, service_url): | |||
| user = { | |||
| 'external_id_provider': external_credential['provider'], | |||
| 'external_id': external_credential['id'], | |||
| 'fullname': cas_resp.attributes['given-names'] + ' ' + cas_resp.attributes['family-name'], | |||
| 'fullname': '{} {}'.format(cas_resp.attributes.get('given-names'), cas_resp.attributes.get('family-name')), | |||
There was a problem hiding this comment.
Just FYI: If both names are empty, use the ORCiD id instead. (HotFixed)
There was a problem hiding this comment.
When testing on the ORCiD website, they seemed to disallow an empty given-names.
There was a problem hiding this comment.
User can make their profile private and only ORCiD is returned. This is a very rare case we happened to run into. :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Purpose
Allow users to login via ORCID's OAuth.
Changes
DictionaryFieldonUserto store external auth ID'sSample CAS Response:
Side effects
None expected
Ticket
OSF-6885
OSF-6881
OSF-5162