oauthenticator icon indicating copy to clipboard operation
oauthenticator copied to clipboard

[GitLab] Implement refresh_user() to keep `auth_state` updated

Open Wykiki opened this issue 3 years ago • 4 comments

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

Wykiki avatar Mar 02 '22 13:03 Wykiki

Thanks for submitting your first pull request! You are awesome! :hugs:
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! :wave:
Welcome to the Jupyter community! :tada:

welcome[bot] avatar Mar 02 '22 13:03 welcome[bot]

Review points

  • [x] 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.
  • [x] 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.

consideRatio avatar May 20 '22 16:05 consideRatio

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.

Wykiki avatar Jun 03 '22 09:06 Wykiki

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.

jthiltges avatar Jun 22 '22 17:06 jthiltges