Skip to content
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

[GitLab] Implement refresh_user() to keep auth_state updated #490

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Wykiki
Copy link

@Wykiki Wykiki commented Mar 2, 2022

Background to help review this PR added by Erik

Authenticator.refresh_user is part of the Authenticator base class. By overriding it, the Authenticator can make sure auth_state is updated.

  • `Authenticator.refresh_pre_spawn set to True, the authentication state can be ensured to be freshly updated.
  • Authenticator.auth_refresh_age is set to 300 seconds by default, and JupyterHub will ensure that the state isn't older than that when the user makes requests to JupyterHub itself (not /user/... but /hub/...).

Misc notes

  • It seems that refresh_user, by returning None can force a user to re-login. This is perhaps not relevant in this PR.
  • It seems that by implementing refresh_user, anything done in the authentication flow could be done multiple times again. Because the default value of Authenticator.auth_refresh_age is 300 for the base authenticator class, it means that by implementing refresh_user(), users of the GitLab authenticator may experience a performance hit if they don't end up needing this unless that it set to 0 to disable refreshing by default.

Original PR description

  • Shared logic exists between authenticate() and refresh_user(), it has been split into a separate method.
  • Not sure about whole OAuth2 content being put into auth_state
  • Didn't read contributing guide yet, I'll have a look tonight sorry

@welcome
Copy link

welcome bot commented Mar 2, 2022

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@consideRatio consideRatio changed the title Implement refresh_user() for Gitlab provider [GitLab] Implement refresh_user() to keep auth_state updated May 20, 2022
@consideRatio
Copy link
Member

consideRatio commented May 20, 2022

Review points

  • To simplify the review effort for me and others, could you split your commits into two or more? If possible, I'd like to overview in one commit that a lot of logic was just relocated to another function for re-use, and then another commit that implemented refresh_user and possibly also made adjustments to shared logic.
  • Let's discuss and try arrive at a decision on what default value we should have on auth_refresh_age. Should it remain 300, be a longer duration, or should it be updated to 0, to disable this functionality by default?
  • Is there situations where refresh_user should return None and force a relogin? I assume no, just looking to clarify.
  • I updated the PR description with some background, perhaps you can add a short note or reference to the refresh_user docstring to help people find there way back to what the refresh_user function is about - you could link to this PR description if you want for example.

@Wykiki
Copy link
Author

Wykiki commented Jun 3, 2022

Review points

* [ ]  To simplify the review effort for me and others, could you split your commits into two or more? If possible, I'd like to overview in one commit that a lot of logic was just relocated to another function for re-use, and then another commit that implemented refresh_user and possibly also made adjustments to shared logic.

Indeed, sorry for that point, commits should be way better to review now, tell me if anything else needed to ease the review.

* [ ]  Let's discuss and try arrive at a decision on what default value we should have on `auth_refresh_age`. Should it remain 300, be a longer duration, or should it be updated to 0, to disable this functionality by default?

I really don't have any ideas about that point, I'd rather keep 300 as it was there already.

* [ ]  Is there situations where refresh_user should return None and force a relogin? I assume no, just looking to clarify.

If user had permissions updates while logged-in, the _oauth_call() function may return None, and this would cause the user to relogin, I guess.

* [ ]  I updated the PR description with some background, perhaps you can add a short note or reference to the `refresh_user` docstring to help people find there way back to what the `refresh_user` function is about - you could link to this PR description if you want for example.

Docstring updated, tell me if more information should be included.

@jthiltges
Copy link

This looks similar my PR #475 with an attempt to add OAuth token refresh to generic. In the momentum to implement refresh, please consider it as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants