Implement OAuthenticator.refresh_user

[Wykiki]

This MR aims to implement the refresh_user()'s Authenticator method in OAuthenticator class.

Following #398 comment.

At the time of writing, nothing has been tested, and provider specific code would be needed, as the OAuth2 spec does not force the expires_in field in the access_token response body.

I may not have bandwidth soon to work again on this, feel free to take the lead !

closes #475
closes #490
closes #398

[yuvipanda]

I'll be super excited to try get this to eventually land :)

[Wykiki]

@yuvipanda I don't have bandwidth to work on it anymore, so feel free to take over the code !

[yuvipanda]

@Wykiki I'll poke around and try to find someone!

[yuvipanda]

And thank you for your contribution :)

[epstein6]

Hello! Was any progress made here? This would be useful for my work, and this looks like a fairly complete implementation (thanks!).

[epstein6]

I've been using this with gitlab with a couple of compatibility changes (adding the redirect_url and the client info) and it seems to work. Are there any other particular changes that would need to be done here for this to be merged?

[minrk]

I finally have time to look this over, and I think there is some simplification we can do. In particular, I think we should rely on JupyterHub's own refresh_user expiration (Authenticator.auth_refresh_age) config rather than adding our own that does the same thing within refresh_user, which makes two separate timers interact.

I also think refresh_user should always refresh if there is a refresh token available. Motivating use case: refresh_pre_spawn = True to maximize the lifetime of a token for a user session. This should always refresh the token at spawn time.

Another affected case where we need to make a choice: oauth without refresh tokens. In the absence of refresh_tokens, should refresh_user:

1. always expire (may be annoying, likely not correct - e.g. GitHub OAuth does not provide a refresh token when using access tokens that don't expire)
2. check if access_token is valid (can check expiration and/or make an API request, e.g. calling token_to_user)
3. check if access_token will be valid for a given amount of time (requires expires_in and some config about minimum lifetime)

[minrk]

This now implements refresh_user for the cases:

• enable_auth_state is False - never refresh (current default)
• refresh_token defined - always refresh
• refresh_token unavailable - refresh auth model with current token, force login if no longer valid

I think the one problem with this always-refresh design is that using a refresh_token often invalidates existing access tokens (GitHub does this, for. This is a problem for the case:

1. spawn server with access token
2. later request to Hub after auth_max_age calls refresh_user, access token provided to server is revoked.

This is very likely to happen, as the activity notifications coming from the user server will eventually trigger refresh_user after auth_max_age.

As a result, I think preemptively refreshing the access token is not something we should do, even though I really want to do it on spawn start. Unfortunately, JupyterHub doesn't provide Authenticators the info they need to refresh auth on spawn, but not the rest of the time. Plus, it would be a problem for named servers, where multiple spawners may be concurrent - forcefully refreshing the token on every start would actually mean only the latest spawn has a valid token.

[minrk]

This now only refreshes access tokens after they expire.

The flow:

• refresh_user always refreshes the user model with the current access token
• if that refresh fails (likely due to expired token) and refresh_token is defined, request a new access token

There is still no need to check an expiration date, because we need to attempt to use the token to refresh anyway, and we can assume failure means expiration is likely (note: this handling is currently unconditional - all failures are assumed to indicate a need for a fresh login. Intermittent network issues on the Hub could trigger expired tokens).

[minrk]

I've tested this with the mock-provider example and it's working as expected. I think it's ready to go, but I could also explore adding a fixture for running the mock-oauth2-provider if folks want deeper integration testing (this PR exercises our code just fine, but assumes the mocks are accurate).

[GeorgianaElena]

Thanks for picking this up @minrk and for explaining the reasoning behind simplifying the logic and why this doesn't preemptively refreshes the access token.

I think get_prev_refresh_token needs to be removed now as it not used and confusing. Other than this, the PR looks ready to merge to me. I think creating a fixture for running the mock-oauth2-provider should be tracked by another issue and PR as a more general improvement to the testing infra.

[manics]

The docstring for build_auth_state_dict isn't quite correct anymore, it still says it's (only) called by authenticate:

oauthenticator/oauthenticator/oauth2.py

Line 1153 in 90da9d7

Called by the :meth:`oauthenticator.OAuthenticator.authenticate`

If it's easier you could take out the cross reference and similarly for other docstrings. It's helpful for understanding the code, but it's a pain to keep in sync.

[minrk]

Thanks for the review! I think I've addressed it. I removed the unused method, updated the docstrings, and updated all the method cross-links so they resolve properly. See here for the rendered refresh_user docstring.

I think the last question is if this should be enabled by default, and I think the answer is yes. You can opt-out by setting:

c.Authenticator.auth_refresh_age = 0

[manics]

Should we treat the removal of get_prev_refresh_token as breaking, or an internal implementation detail?

[minrk]

I don't think it should be considered breaking, but I can put it back as deprecated if folks want.

[YStrauchP4]

I'm very keen on this PR and it looks like it's ready to be merged? (Hopeful smile)

[minrk]

Going ahead with merge since this is approved. Thanks @Wykiki for getting it started and everyone for their patience!