Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[server] Sync token refresh #19470

Merged
merged 2 commits into from
Feb 27, 2024
Merged

[server] Sync token refresh #19470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a4b32b
[server] Sync token refresh
geropl Feb 26, 2024
083b7b3
[proxy] api.: Handle /auth/*/callback
geropl Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion components/proxy/conf/Caddyfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ api.{$GITPOD_DOMAIN} {
allowed_origins https://{$GITPOD_DOMAIN}
}

@to_server path /auth/github/callback
@to_server path /auth/*/callback
handle @to_server {
import compression

Expand Down
61 changes: 61 additions & 0 deletions components/server/src/user/token-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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",
Copy link
Contributor

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?

Copy link
Member Author

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.

false,
{},
Copy link
Contributor

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

Copy link
Member Author

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.

);
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.
Expand All @@ -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);
Copy link
Contributor

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? 🤣

Copy link
Member Author

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.

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) {
Expand Down