refactor ScmService.getToken to return token of undefined

gitpod-io · Nov 22, 2023 · baaadc3 · baaadc3
1 parent 59096d5
commit baaadc3
Show file tree

Hide file tree

Showing 13 changed files with 45 additions and 33 deletions.
diff --git a/components/server/src/api/scm-service-api.ts b/components/server/src/api/scm-service-api.ts
@@ -38,11 +38,9 @@ export class ScmServiceAPI implements ServiceImpl<typeof ScmServiceInterface> {
     async searchSCMTokens(request: SearchSCMTokensRequest, _: HandlerContext): Promise<SearchSCMTokensResponse> {
         const userId = ctxUserId();
         const response = new SearchSCMTokensResponse();
-        try {
-            const token = await this.scmService.getToken(userId, request);
+        const token = await this.scmService.getToken(userId, request);
+        if (token) {
             response.tokens.push(this.apiConverter.toSCMToken(token));
-        } catch (error) {
-            throw new ApplicationError(ErrorCodes.NOT_FOUND, "Token not found.");
         }
         return response;
     }

diff --git a/components/server/src/auth/authenticator.ts b/components/server/src/auth/authenticator.ts
@@ -293,23 +293,25 @@ export class Authenticator {
         const state = await this.signInJWT.sign({ host, returnTo, overrideScopes: override });
         authProvider.authorize(req, res, next, this.deriveAuthState(state), wantedScopes);
     }
-    protected mergeScopes(a: string[], b: string[]) {
+    private mergeScopes(a: string[], b: string[]) {
         const set = new Set(a);
         b.forEach((s) => set.add(s));
         return Array.from(set).sort();
     }
-    protected async getCurrentScopes(user: any, authProvider: AuthProvider) {
+    private async getCurrentScopes(user: any, authProvider: AuthProvider) {
         if (User.is(user)) {
             try {
                 const token = await this.tokenProvider.getTokenForHost(user, authProvider.params.host);
-                return token.scopes;
+                if (token) {
+                    return token.scopes;
+                }
             } catch {
                 // no token
             }
         }
         return [];
     }
-    protected getSorryUrl(message: string) {
+    private getSorryUrl(message: string) {
         return this.config.hostUrl.asSorry(message).toString();
     }
 }
diff --git a/components/server/src/bitbucket-server/bitbucket-server-token-handler.ts b/components/server/src/bitbucket-server/bitbucket-server-token-handler.ts
@@ -30,7 +30,7 @@ export class BitbucketServerTokenHelper {
         const { host } = this.config;
         try {
             const token = await this.tokenProvider.getTokenForHost(user, host);
-            if (this.containsScopes(token, requiredScopes)) {
+            if (token && this.containsScopes(token, requiredScopes)) {
                 return token;
             }
         } catch {

diff --git a/components/server/src/bitbucket/bitbucket-token-handler.ts b/components/server/src/bitbucket/bitbucket-token-handler.ts
@@ -30,7 +30,7 @@ export class BitbucketTokenHelper {
         const { host } = this.config;
         try {
             const token = await this.tokenProvider.getTokenForHost(user, host);
-            if (this.containsScopes(token, requiredScopes)) {
+            if (token && this.containsScopes(token, requiredScopes)) {
                 return token;
             }
         } catch {

diff --git a/components/server/src/github/github-token-helper.ts b/components/server/src/github/github-token-helper.ts
@@ -30,7 +30,7 @@ export class GitHubTokenHelper {
         const { host } = this.config;
         try {
             const token = await this.tokenProvider.getTokenForHost(user, host);
-            if (this.containsScopes(token, requiredScopes)) {
+            if (token && this.containsScopes(token, requiredScopes)) {
                 return token;
             }
         } catch {

diff --git a/components/server/src/gitlab/gitlab-token-helper.ts b/components/server/src/gitlab/gitlab-token-helper.ts
@@ -30,7 +30,7 @@ export class GitLabTokenHelper {
         const { host } = this.config;
         try {
             const token = await this.tokenProvider.getTokenForHost(user, host);
-            if (this.containsScopes(token, requiredScopes)) {
+            if (token && this.containsScopes(token, requiredScopes)) {
                 return token;
             }
         } catch (e) {

diff --git a/components/server/src/scm/scm-service.ts b/components/server/src/scm/scm-service.ts
@@ -36,7 +36,15 @@ export class ScmService {
         @inject(GitTokenScopeGuesser) private readonly gitTokenScopeGuesser: GitTokenScopeGuesser,
     ) {}
 
-    public async getToken(userId: string, query: { host: string }): Promise<Token> {
+    /**
+     *
+     * @param userId subject and current user.
+     * @param query specifies the `host` of the auth provider to search for a token.
+     * @returns promise which resolves to a `Token`, or `undefined` if no token for the specified user and host exists.
+     *
+     * @throws 404/NOT_FOUND if the user is not found.
+     */
+    public async getToken(userId: string, query: { host: string }): Promise<Token | undefined> {
         // FIXME(at) this doesn't sound right. "token" is pretty overloaded, thus `read_scm_tokens` would be correct
         await this.auth.checkPermissionOnUser(userId, "read_tokens", userId);
         const { host } = query;
@@ -73,7 +81,8 @@ export class ScmService {
             throw new ApplicationError(ErrorCodes.NOT_FOUND, `Auth provider not found.`);
         }
 
-        const { value: currentToken } = await this.getToken(userId, { host });
+        const token = await this.getToken(userId, { host });
+        const currentToken = token?.value;
         return await this.gitTokenScopeGuesser.guessGitTokenScopes(provider, {
             host,
             repoUrl,

diff --git a/components/server/src/test/service-testing-container-module.ts b/components/server/src/test/service-testing-container-module.ts
@@ -37,6 +37,7 @@ import { TokenProvider } from "../user/token-provider";
 import { GitHubScope } from "../github/scopes";
 import { GitpodHostUrl } from "@gitpod/gitpod-protocol/lib/util/gitpod-host-url";
 import * as crypto from "crypto";
+import { ApplicationError, ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error";
 
 const signingKeyPair = crypto.generateKeyPairSync("rsa", { modulusLength: 2048 });
 const validatingKeyPair1 = crypto.generateKeyPairSync("rsa", { modulusLength: 2048 });
@@ -124,7 +125,10 @@ const mockApplyingContainerModule = new ContainerModule((bind, unbound, isbound,
         },
     });
     rebind(TokenProvider).toConstantValue(<TokenProvider>{
-        getTokenForHost: async () => {
+        getTokenForHost: async (user, host) => {
+            if (host != "github.com") {
+                throw new ApplicationError(ErrorCodes.NOT_FOUND, `SCM Token not found.`);
+            }
             return {
                 value: "test",
                 scopes: [GitHubScope.EMAIL, GitHubScope.PUBLIC, GitHubScope.PRIVATE],

diff --git a/components/server/src/user/token-provider.ts b/components/server/src/user/token-provider.ts
@@ -13,5 +13,5 @@ export interface TokenProvider {
      * @param user
      * @param host
      */
-    getTokenForHost(user: User | string, host: string): Promise<Token>;
+    getTokenForHost(user: User | string, host: string): Promise<Token | undefined>;
 }
diff --git a/components/server/src/user/token-service.ts b/components/server/src/user/token-service.ts
@@ -11,6 +11,7 @@ import { UserDB } from "@gitpod/gitpod-db/lib";
 import { v4 as uuidv4 } from "uuid";
 import { TokenProvider } from "./token-provider";
 import { ApplicationError, ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error";
+import { GarbageCollectedCache } from "@gitpod/gitpod-protocol/lib/util/garbage-collected-cache";
 
 @injectable()
 export class TokenService implements TokenProvider {
@@ -19,9 +20,12 @@ export class TokenService implements TokenProvider {
     @inject(HostContextProvider) protected readonly hostContextProvider: HostContextProvider;
     @inject(UserDB) protected readonly userDB: UserDB;
 
-    protected getTokenForHostCache = new Map<string, Promise<Token>>();
+    // 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,
+    // if applicable.
+    private readonly getTokenForHostCache = new GarbageCollectedCache<Promise<Token | undefined>>(10, 5);
 
-    async getTokenForHost(user: User | string, host: string): Promise<Token> {
+    async getTokenForHost(user: User | string, host: string): Promise<Token | undefined> {
         const userId = User.is(user) ? user.id : user;
         // (AT) when it comes to token renewal, the awaited http requests may
         // cause "parallel" calls to repeat the renewal, which will fail.
@@ -36,18 +40,15 @@ export class TokenService implements TokenProvider {
         return promise;
     }
 
-    private async doGetTokenForHost(userId: string, host: string): Promise<Token> {
+    private async doGetTokenForHost(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);
         let token = await this.userDB.findTokenForIdentity(identity);
         if (!token) {
-            throw new ApplicationError(
-                ErrorCodes.NOT_FOUND,
-                `SCM Token not found: (${userId}/${identity?.authId}/${identity?.authName}).`,
-            );
+            return undefined;
         }
         const aboutToExpireTime = new Date();
         aboutToExpireTime.setTime(aboutToExpireTime.getTime() + 5 * 60 * 1000);

diff --git a/components/server/src/workspace/git-token-scope-guesser.ts b/components/server/src/workspace/git-token-scope-guesser.ts
@@ -15,7 +15,7 @@ export class GitTokenScopeGuesser {
 
     async guessGitTokenScopes(
         authProvider: AuthProviderInfo,
-        params: GuessGitTokenScopesParams & { currentToken: string },
+        params: GuessGitTokenScopesParams & { currentToken?: string },
     ): Promise<GuessedGitTokenScopes> {
         const { repoUrl, gitCommand, currentToken } = params;
 
@@ -27,7 +27,7 @@ export class GitTokenScopeGuesser {
         const { host, repo, owner, repoKind } = parsedRepoUrl;
 
         // in case of git operation which require write access to a remote
-        if (gitCommand === "push") {
+        if (currentToken && gitCommand === "push") {
             const validationResult = await this.tokenValidator.checkWriteAccess({
                 host,
                 owner,

diff --git a/components/server/src/workspace/gitpod-server-impl.ts b/components/server/src/workspace/gitpod-server-impl.ts
@@ -634,7 +634,9 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
         const { host } = query;
         try {
             const token = await this.scmService.getToken(user.id, { host });
-            await this.guardAccess({ kind: "token", subject: token, tokenOwnerID: user.id }, "get");
+            if (token) {
+                await this.guardAccess({ kind: "token", subject: token, tokenOwnerID: user.id }, "get");
+            }
 
             return token;
         } catch (error) {

diff --git a/components/server/src/workspace/workspace-starter.ts b/components/server/src/workspace/workspace-starter.ts
@@ -1822,20 +1822,16 @@ export class WorkspaceStarter {
         }
 
         const gitToken = await this.tokenProvider.getTokenForHost(user, host);
+        if (!gitToken) {
+            throw new Error(`No token for host: ${host}`);
+        }
         const username = gitToken.username || "oauth2";
 
         const gitConfig = new GitConfig();
         gitConfig.setAuthentication(GitAuthMethod.BASIC_AUTH);
         gitConfig.setAuthUser(username);
         gitConfig.setAuthPassword(gitToken.value);
 
-        if (this.config.insecureNoDomain) {
-            const token = await this.tokenProvider.getTokenForHost(user, host);
-            gitConfig.setAuthentication(GitAuthMethod.BASIC_AUTH);
-            gitConfig.setAuthUser(token.username || "oauth2");
-            gitConfig.setAuthPassword(token.value);
-        }
-
         const userGitConfig = workspace.config.gitConfig;
         if (!!userGitConfig) {
             Object.keys(userGitConfig)