Remove refresh token from RefreshTokenStore when access token renewal fails

[m-mohr]

In the QGIS plugin, we implement our own RefreshTokenStore. It seems that the refresh token is not removed from the RefreshTokenStore when access token renewal fails. First we check the exp claim in the refresh token, but if it doesn't exist we rely on the refresh token being removed when the access token renewal fails. It seems that's not foreseen?

[soxofaan]

There has never been need for removal indeed.
In most practical situations (.authenticate_oidc() usage), failure of request doing the refresh token grant is immediately followed by another request with another grant (usually the device code grant), causing the outdated refresh token to be overwritten by a new one. Removing the refresh token in between isn't worth the hassle then.

First we check the exp claim in the refresh token, but if it doesn't exist we rely on the refresh token being removed when the access token renewal fails. It seems that's not foreseen?

I'm not sure I understand the logic here and why removal of the refresh token is important.
If there is a refresh token client-side, it still have to be checked against the identity provider to see if it is valid (to produce an access token). Not being expired according to the "exp" claim is not enough to be valid. There are other ways for a token to be be invalid (e.g. a failed signature check, or user-triggered session invalidation by some kind of logout action).

[m-mohr]

We just need to quickly get an idea of whether a user is likely logged in or not for the UI to act accordinly. We can't always run requests in all code sections (e.g. when a UI is getting refreshed as it would block the UI). We also can't just run authenticate_oidc from all places in the code/UI. Currently the UI often thinks it's still logged in (especially when opening QGIS after some days), although only an outdated refresh token is stored.

Thus we just try to get an answer that is as precise as possible, it's a compromise between UI responsiveness and exactness.

[soxofaan]

We also can't just run authenticate_oidc from all places in the code/UI

FYI: to just check the refresh token, you don't have to do .authenticate_oidc() (which could indeed be blocking in annoying ways). You can just do .authenticate_oidc_refresh_token()(which should be pretty fast).
(If necessary we can add a timeout parameter there if you want some guarantees about getting a quick response)

Currently the UI often thinks it's still logged in (especially when opening QGIS after some days), although only an outdated refresh token is stored.

I guess that's exactly illustrates the mismatch in expectations here: how would the client have to remove the refresh token at the appropriate time when there is no client code running?

Or framed differently: while it's technically possible to remove a non-working refresh token when the client can't resolve it to an access token, that would only cover a tiny slice of the whole problem. In the majority of cases the refresh token will expire when the client isn't around to observe that and handle accordingly.

I understand that you don't want to do possibly blocking HTTP requests, but then there is, I'm afraid, very little you can assume about being logged out or logged in.
You can only be sure about being logged out (e.g. there is no refresh token, it is corrupt, the "exp" claim is outdated, ...),
but when the refresh token still looks fine, there is no guarantee that it will resolve to an access token without checking with the identity provider (through a HTTP request).

we just try to get an answer that is as precise as possible

what is most important kind of "precise" here?
That the answer is correct about being logged out, or that it is correct about being logged in?

Without doing an HTTP request, the safest bet is to assume that the user is not logged in I'm afraid.

If you want a better answer, I would recommend to try to do the HTTP request (a single "refresh token grant" one), which should be fast. I'd have to double check, but I think a vanilla .authenticate_oidc_refresh_token() readily does this.
And also note that this request goes to the identity provider, not the openeo backend, so I think you can assume higher availability or faster response time than the average openeo backend 😄

[m-mohr]

You can just do .authenticate_oidc_refresh_token()(which should be pretty fast).

We actually already do this, but that slows down the UI already significantly for only two connections.
If you have 5 connections stored it's 5 requests being made before the user can interact with the plugin. If one of them is slow you run into issues already. (And they run in sequence due to the way QGIS is implemented.)

The issue we have here:
Either we do this for all backends at once when the plugin loads (current behaviour) or we do it when you first interact with a single backend (which might be right click to open a context menu, so the plugin may open that context menu for a couple of seconds after the right click, which seems even worse UX.)

Currently, we are considering just to have "Login / Logout" and not tell the user what the current login/out status is. So they click and either they login or logout, which of course is worst UX.

I guess that's exactly illustrates the mismatch in expectations here: how would the client have to remove the refresh token at the appropriate time when there is no client code running?

authenticate_oidc_refresh_token is actually running (and potentially other requests), but it doesn't seem to remove the refresh token once expired / a request fails due to expiry. The main issue here is that we don't have a single entry point for this check. So we need to wrap all requests in try/error and on a 401/3 response delete the refresh token manually. That makes the whole codebase super difficult and prone to errors/mistakes.

what is most important kind of "precise" here? That the answer is correct about being logged out, or that it is correct about being logged in?

I just want to reduce the inconcenience to the user. Either we log them in too often or show them error messages when credentials don't work. So the answer is: Whatever is less problematic.