[server] Sync token refresh #19470
[server] Sync token refresh #19470
Conversation
|
Sorry, commented on wrong PR... |
| } | ||
|
|
||
| const aboutToExpireTime = new Date(); | ||
| aboutToExpireTime.setTime(aboutToExpireTime.getTime() + 5 * 60 * 1000); |
There was a problem hiding this comment.
[nit] will a provider's token always expired in 5 minutes, then never make it works? 🤣
There was a problem hiding this comment.
I think the intend here is to treat all tokens as invalid that will expire in the next 5 minutes.
|
|
||
| // EXPERIMENT(sync_refresh_token_exchange) | ||
| const syncRefreshTokenExchange = await getExperimentsClientForBackend().getValueAsync( | ||
| "sync_refresh_token_exchange", |
There was a problem hiding this comment.
retry_refresh_token_exchange and sync_refresh_token_exchange are different
- sync_refresh_token_exchange will store them in database
- retry_refresh_token_exchange will revoke (internally) token after it's used
Is it intent?
There was a problem hiding this comment.
The intention behind both paths is the same. Deep down they both call:
await authProvider.refreshToken(user); // updating/inserting a fresh token into the DB as side effect
return await this.userDB.findTokenForIdentity(identity); // return said token
It's just that the existing path has this poor-man's sync approach (from before we had redis), which I did not want for the pure redis solution.
What I basically did for sync_refresh_token_exchange is to wrap it into a redis mutex, nothing more. The rest is noise/slight code re-arrangements.
| const syncRefreshTokenExchange = await getExperimentsClientForBackend().getValueAsync( | ||
| "sync_refresh_token_exchange", | ||
| false, | ||
| {}, |
There was a problem hiding this comment.
Should we add host as a property of Feature Flag? Just in case they have multiple SCM
There was a problem hiding this comment.
We are interested to role this out for all, so I think we should test it as such. The difference between SCMs should be properly abstracted behind the Token abstraction.
|
Approved to unblock |
|
/unhold |
Description
Attempt two to fix token refresh (attempt one is here) using a redis mutex.
Code is guarded with the
sync_refresh_token_exchangefeature flag.Related Issue(s)
Fixes EXP-1413
How to test
Documentation
Preview status
gitpod:summary
Build Options
Build
Run the build with werft instead of GHA
Run Leeway with
--dont-testPublish
Installer
Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
If enabled this will build
install/previewIf enabled this will create the environment on GCE infra
Saves cost. Untick this only if you're really sure you need a non-preemtible machine.
Valid options are
all,workspace,webapp,ide,jetbrains,vscode,ssh. If enabled,with-previewandwith-large-vmwill be enabled./hold