Skip to content
This repository has been archived by the owner on Nov 21, 2024. It is now read-only.

Orcid login #62

Merged
merged 1 commit into from
Sep 25, 2024
Merged

Orcid login #62

merged 1 commit into from
Sep 25, 2024

Conversation

DonHaul
Copy link
Contributor

@DonHaul DonHaul commented Aug 5, 2024

  • Configure ORCID login properly
  • Generate jwt tokens to user logged in through ORCID

Fixes #509

@DonHaul DonHaul marked this pull request as draft August 5, 2024 11:10
@DonHaul DonHaul force-pushed the orcid-login branch 11 times, most recently from 3f91172 to 4a2d39f Compare August 6, 2024 13:08
@DonHaul DonHaul marked this pull request as ready for review August 6, 2024 13:37
@DonHaul DonHaul requested a review from drjova August 6, 2024 13:37
@DonHaul DonHaul self-assigned this Aug 7, 2024


def user_login_success(request):
refresh = RefreshToken.for_user(request.user)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't this fail if someone goes directly to login/success and there is no user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed it should fail with a 403 Forbidden, instead an AnonymousUser was being used that somehow could still access this route without any problem

drjova
drjova previously requested changes Aug 7, 2024
Copy link
Contributor

@drjova drjova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comment

@@ -11,4 +12,5 @@
path("~redirect/", view=user_redirect_view, name="redirect"),
path("~update/", view=user_update_view, name="update"),
path("<int:pk>/", view=user_detail_view, name="detail"),
path("login/success/", user_login_success, name="login_success"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DonHaul can we rename this as /me and @karolina-siemieniuk-morawska could integrate it in her PR. This will be similar to hep, user is logged in to orcid and UI check it's status from there.

@DonHaul
Copy link
Contributor Author

DonHaul commented Sep 17, 2024

Diagram of wanted workflow

UI -> Orcid -> BO Callback -> UI success
                           -> BO/UI Email -> BO Save Social Account -> UI success

To do:

  1. Test setting the callback route to our own custom view to see if nothing fails.
  2. Check headless implementation - https://github.com/pennersr/django-allauth/tree/main/examples/react-spa
  3. Does it make sense to use the dj-rest-auth or the all-auth only with headless?
  4. Does it make sense for the code to be sent to the frontend (meaning callback is in the FE not BO) and then simply store the credentials in BO via one of these libraries

Notes & links 2024/09/18

@DonHaul
Copy link
Contributor Author

DonHaul commented Sep 18, 2024

  1. Changing the callback to sample one that does not do anything, makes it so that the error "Failed to exchange code for access token" does not occur anymore whe we call /dj-rest-auth/orcid/ with the retrieved code the first time - it gives us the access token and all other info - including if the user has an email associated to him or not.
  • Sending the code a second time give us the error "Failed to exchange code for access token" likely as the code as already been "spent" as expected

@DonHaul
Copy link
Contributor Author

DonHaul commented Sep 18, 2024

Currently trying an implementation with only all-auth

Managed to find the path to authenticate - https://docs.allauth.org/en/latest/headless/openapi-specification/#tag/Authentication:-Providers/paths/~1_allauth~1browser~1v1~1auth~1provider~1redirect/post - it returns a code - althought it seems its not fully doing his job since when I try to get the user via https://docs.allauth.org/en/latest/headless/openapi-specification/#tag/Authentication:-Current-Session I get 401 meaning authentication has not been completely done.
Managed to find the path to send the email - https://docs.allauth.org/en/latest/headless/openapi-specification/#tag/Authentication:-Providers/paths/~1_allauth~1%7Bclient%7D~1v1~1auth~1provider~1signup/post

Next Actions:
Understand whats going on with the callbacks as it seems that they are the sources of the issue

JWT Token seems to not be implemented by default - token strategy must be implemented, or use dj-rest-auth.

I've figured the auth workflow that must be followed (using django-allauth headless) - This is called the oauth 2.0 authorization code grant type

  • /_allauth/browser/v1/auth/provider/redirect - fetches a code
  • /_allauth/{client}/v1/auth/code/confirm - exchanges code for a actual user login (not yet managed to make it work, so unsure if its the correct endpoint)
  • /_allauth/{client}/v1/auth/provider/signup - sets up email if it had not been set up previously

Now implementing it using dj-rest-auth by following https://medium.com/@michal.drozdze/django-rest-framework-jwt-authentication-social-login-login-with-google-8911332f1008

@DonHaul
Copy link
Contributor Author

DonHaul commented Sep 20, 2024

Facts:

  • dj-rest-auth seems to be skipping the verify email is there step present in all-auth.
  • It seems that function process_signup is not being entered (link) to be understood why

Next Steps:

  • Debug step by step whats happening during the /dj-rest-auth/orcid to confirm it is indeed whats happening. how to debug

@DonHaul
Copy link
Contributor Author

DonHaul commented Sep 23, 2024

  • Using simply dj-rest-auth out of the box is not recommended to orcid as the Callback is not implemented

Possible Implementations:
a) Implement our own custom callback view - Based it on OAuth2CallbackView
b) Implement it using only all-auth headless - (will require meddling w/ JWT Auth as its not implemented by default )

Trying option b)
Tried Option b) it seems that all-auth headless also does not support a workflow where we need to fill the email.

Only solution - Create a custom callback view OAuth2CallbackView, that requests for an email if the user is new / first time

@DonHaul DonHaul force-pushed the orcid-login branch 5 times, most recently from 2b6263f to a2fc9bf Compare September 24, 2024 12:00
@DonHaul DonHaul dismissed drjova’s stale review September 24, 2024 12:38

old implementation

Copy link
Contributor

@drjova drjova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @DonHaul just the comment and could you please change a bit the commit message? Something like:

backoffice: Addition of headless `allauth` for ORCID login 

backoffice/config/settings/base.py Outdated Show resolved Hide resolved
@drjova
Copy link
Contributor

drjova commented Sep 25, 2024

@DonHaul there is a conflict here with poetry

@DonHaul DonHaul merged commit 093e346 into inspirehep:main Sep 25, 2024
3 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate UI login with orcid
2 participants