oauthenticator
oauthenticator copied to clipboard
[GitLab] Implement refresh_user() to keep `auth_state` updated
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 returningNonecan 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 ofAuthenticator.auth_refresh_ageis 300 for the base authenticator class, it means that by implementingrefresh_user(), users of the GitLab authenticator may experience a performance hit if they don't end up needing this unless that it set to0to disable refreshing by default.
Original PR description
- Shared logic exists between
authenticate()andrefresh_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
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.
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:
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_userdocstring to help people find there way back to what therefresh_userfunction is about - you could link to this PR description if you want for example.
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.
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.