-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[server] Sync token refresh #19470
[server] Sync token refresh #19470
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, but I don't understand why we add endpoint (Caddyfile) for public.v1
APIs. Not block but would be good to understand
Sorry, commented on wrong PR... |
} | ||
|
||
const aboutToExpireTime = new Date(); | ||
aboutToExpireTime.setTime(aboutToExpireTime.getTime() + 5 * 60 * 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] will a provider's token always expired in 5 minutes, then never make it works? 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add host as a property of Feature Flag? Just in case they have multiple SCM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_exchange
feature 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-test
Publish
Installer
Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
If enabled this will build
install/preview
If 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-preview
andwith-large-vm
will be enabled./hold