-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,13 +14,15 @@ import { ApplicationError, ErrorCodes } from "@gitpod/gitpod-protocol/lib/messag | |
import { GarbageCollectedCache } from "@gitpod/gitpod-protocol/lib/util/garbage-collected-cache"; | ||
import { log } from "@gitpod/gitpod-protocol/lib/util/logging"; | ||
import { getExperimentsClientForBackend } from "@gitpod/gitpod-protocol/lib/experiments/configcat-server"; | ||
import { RedisMutex } from "../redis/mutex"; | ||
|
||
@injectable() | ||
export class TokenService implements TokenProvider { | ||
static readonly GITPOD_AUTH_PROVIDER_ID = "Gitpod"; | ||
|
||
@inject(HostContextProvider) protected readonly hostContextProvider: HostContextProvider; | ||
@inject(UserDB) protected readonly userDB: UserDB; | ||
@inject(RedisMutex) private readonly redisMutex: RedisMutex; | ||
|
||
// Introducing GC to token cache to guard from potentialy stale fetch requests. This is setting | ||
// a hard limit at 10s (+5s) after which after which compteting request will trigger a new request, | ||
|
@@ -29,6 +31,17 @@ export class TokenService implements TokenProvider { | |
|
||
async getTokenForHost(user: User | string, host: string): Promise<Token | undefined> { | ||
const userId = User.is(user) ? user.id : user; | ||
|
||
// EXPERIMENT(sync_refresh_token_exchange) | ||
const syncRefreshTokenExchange = await getExperimentsClientForBackend().getValueAsync( | ||
"sync_refresh_token_exchange", | ||
false, | ||
{}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
); | ||
if (syncRefreshTokenExchange) { | ||
return this.doGetTokenForHostSync(userId, host); | ||
} | ||
|
||
// (AT) when it comes to token renewal, the awaited http requests may | ||
// cause "parallel" calls to repeat the renewal, which will fail. | ||
// Caching for pending operations should solve this issue. | ||
|
@@ -42,6 +55,54 @@ export class TokenService implements TokenProvider { | |
return promise; | ||
} | ||
|
||
// EXPERIMENT(sync_refresh_token_exchange) | ||
private async doGetTokenForHostSync(userId: string, host: string): Promise<Token | undefined> { | ||
const user = await this.userDB.findUserById(userId); | ||
if (!user) { | ||
throw new ApplicationError(ErrorCodes.NOT_FOUND, `User (${userId}) not found.`); | ||
} | ||
const identity = this.getIdentityForHost(user, host); | ||
|
||
const doRefreshToken = async () => { | ||
// Check: Current token so we can actually refresh? | ||
const token = await this.userDB.findTokenForIdentity(identity); | ||
if (!token) { | ||
return undefined; | ||
} | ||
|
||
const aboutToExpireTime = new Date(); | ||
aboutToExpireTime.setTime(aboutToExpireTime.getTime() + 5 * 60 * 1000); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
if (!token.expiryDate || token.expiryDate >= aboutToExpireTime.toISOString()) { | ||
return token; | ||
} | ||
|
||
// Can we refresh these kind of tokens? | ||
const { authProvider } = this.hostContextProvider.get(host)!; | ||
if (!authProvider.refreshToken) { | ||
return undefined; | ||
} | ||
|
||
await authProvider.refreshToken(user); | ||
return await this.userDB.findTokenForIdentity(identity); | ||
}; | ||
|
||
try { | ||
const refreshedToken = await this.redisMutex.using( | ||
[`token-refresh-${host}-${userId}`], | ||
2000, // After 2s without extension the lock is released | ||
doRefreshToken, | ||
{ retryCount: 10, retryDelay: 500 }, // We wait at most 10s until we give up, and conclude that we can't refresh the token now. | ||
); | ||
return refreshedToken; | ||
} catch (err) { | ||
if (RedisMutex.isLockedError(err)) { | ||
log.error({ userId }, `Failed to refresh token (timeout waiting on lock)`, err, { host }); | ||
throw new Error(`Failed to refresh token (timeout waiting on lock)`); | ||
geropl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
throw err; | ||
} | ||
} | ||
|
||
private async doGetTokenForHost(userId: string, host: string): Promise<Token | undefined> { | ||
const user = await this.userDB.findUserById(userId); | ||
if (!user) { | ||
|
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
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:
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.