oauthenticator icon indicating copy to clipboard operation
oauthenticator copied to clipboard

[Generic] Implement refresh_user to periodically refresh OAuth tokens

Open jthiltges opened this issue 3 years ago • 5 comments

An attempt at OAuth token refresh, following the pattern in PR https://github.com/jupyterhub/oauthenticator/pull/287, without relying on tokens to be JWTs. This should address at least some of issue https://github.com/jupyterhub/oauthenticator/issues/398.

Requirements for token refresh:

  • OAuth server response includes expires_in and refresh_token
  • State is persisted on hub with c.Authenticator.enable_auth_state = True (to store the refresh_token)

jthiltges avatar Dec 28 '21 20:12 jthiltges

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 Dec 28 '21 20:12 welcome[bot]

As in the next release of jupyterhub there will be authenticator user group management https://github.com/jupyterhub/jupyterhub/pull/3548 what happens if we have manage_groups=true and the user's group membership changes after auth and before refresh? Your refresh code is not updating user_info['groups'] as it seem.

kkaraivanov1 avatar Feb 16 '22 14:02 kkaraivanov1

@kkaraivanov1 Thanks for looking at the PR. I agree that group data should be refreshed as well.

Is group membership info already being stored on authentication?

jthiltges avatar Feb 16 '22 19:02 jthiltges

The way I understand the intentions behind the PR I mention is if c.Authenticator.manage_groups is set AND if the GenericOAuthenticator adds the groups to user_info['groups'] they will be populated automatically. So to answer your question - it could be Perhaps you can hide that behind a check like if hasattr(self,manage_groups): ........ Please take my thoughts with big grain of salt, I'm not a dev and my python is rudimentary.

kkaraivanov1 avatar Feb 17 '22 06:02 kkaraivanov1

That makes sense, and thank you for the clarification.

Adding support for user_info['groups'] is probably best addressed in a separate PR. I'll be happy to revise this PR to follow the pattern that the project maintainers prefer.

jthiltges avatar Feb 18 '22 15:02 jthiltges