[authorization] Align HTTP handlers before RequestContext rollout (#19214)

* [middleware] RequestContext: don't error on nested contexts + ctxOnAbort

* [auth] HTTP handlers: Add FGA guards and runWithSubjectId where missing

* [code-sync] Guard with FGA

Author: geropl

components/server/src/auth/resource-access.ts

@@ -24,6 +24,7 @@ import { RepoURL } from "../repohost";
 import { HostContextProvider } from "./host-context-provider";
 import { isFgaChecksEnabled } from "../authorization/authorizer";
 import { reportGuardAccessCheck } from "../prometheus-metrics";
+import { FunctionAccessGuard } from "./function-access";
 
 declare let resourceInstance: GuardedResource;
 export type GuardedResourceKind = typeof resourceInstance.kind;
@@ -174,6 +175,26 @@ export class FGAResourceAccessGuard implements ResourceAccessGuard {
     }
 }
 
+/**
+ * FGAFunctionAccessGuard can disable the delegate if FGA is enabled.
+ */
+export class FGAFunctionAccessGuard {
+    constructor(private readonly userId: string, private readonly delegate: FunctionAccessGuard) {}
+
+    async canAccess(name: string): Promise<boolean> {
+        const authorizerEnabled = await isFgaChecksEnabled(this.userId);
+        if (authorizerEnabled) {
+            // Authorizer takes over, so we should not check.
+            reportGuardAccessCheck("fga");
+            return true;
+        }
+        reportGuardAccessCheck("function-access");
+
+        // FGA can't take over yet, so we delegate
+        return this.delegate.canAccess(name);
+    }
+}
+
 export class TeamMemberResourceGuard implements ResourceAccessGuard {
     constructor(readonly userId: string) {}
 

components/server/src/authorization/definitions.ts

@@ -43,7 +43,8 @@ export type UserPermission =
     | "read_tokens"
     | "write_tokens"
     | "read_env_var"
-    | "write_env_var";
+    | "write_env_var"
+    | "code_sync";
 
 export type InstallationResourceType = "installation";
 

components/server/src/code-sync/code-sync-service.ts

@@ -11,7 +11,7 @@ import * as util from "util";
 import express from "express";
 import { inject, injectable } from "inversify";
 import { BearerAuth } from "../auth/bearer-authenticator";
-import { isWithFunctionAccessGuard } from "../auth/function-access";
+import { WithFunctionAccessGuard } from "../auth/function-access";
 import { CodeSyncResourceDB, ALL_SERVER_RESOURCES, ServerResource, SyncResource } from "@gitpod/gitpod-db/lib";
 import {
     DeleteRequest,
@@ -26,6 +26,8 @@ import { v4 as uuidv4 } from "uuid";
 import { accessCodeSyncStorage, UserRateLimiter } from "../auth/rate-limiter";
 import { Config } from "../config";
 import { CachingBlobServiceClientProvider } from "../util/content-service-sugar";
+import { Authorizer } from "../authorization/authorizer";
+import { FGAFunctionAccessGuard } from "../auth/resource-access";
 
 // By default: 5 kind of resources * 20 revs * 1Mb = 100Mb max in the content service for user data.
 const defaultRevLimit = 20;
@@ -71,17 +73,22 @@ function toObjectName(resource: ServerResource, rev: string, collection: string
 
 @injectable()
 export class CodeSyncService {
-    @inject(Config)
-    private readonly config: Config;
+    constructor(
+        @inject(Config)
+        private readonly config: Config,
 
-    @inject(BearerAuth)
-    private readonly auth: BearerAuth;
+        @inject(BearerAuth)
+        private readonly auth: BearerAuth,
 
-    @inject(CachingBlobServiceClientProvider)
-    private readonly blobsProvider: CachingBlobServiceClientProvider;
+        @inject(CachingBlobServiceClientProvider)
+        private readonly blobsProvider: CachingBlobServiceClientProvider,
 
-    @inject(CodeSyncResourceDB)
-    private readonly db: CodeSyncResourceDB;
+        @inject(CodeSyncResourceDB)
+        private readonly db: CodeSyncResourceDB,
+
+        @inject(Authorizer)
+        private readonly authorizer: Authorizer,
+    ) {}
 
     get apiRouter(): express.Router {
         const config = this.config.codeSync;
@@ -91,16 +98,16 @@ export class CodeSyncService {
             res.setHeader("x-operation-id", uuidv4());
             return next();
         });
-        router.use(this.auth.restHandler);
+        router.use(this.auth.restHandler); // expects Bearer token and authenticates req.user, throws otherwise
         router.use(async (req, res, next) => {
             if (!User.is(req.user)) {
                 res.sendStatus(400);
                 return;
             }
 
-            const id = req.user.id;
+            const userId = req.user.id;
             try {
-                await UserRateLimiter.instance(this.config.rateLimiter).consume(id, accessCodeSyncStorage);
+                await UserRateLimiter.instance(this.config.rateLimiter).consume(userId, accessCodeSyncStorage);
             } catch (e) {
                 if (e instanceof Error) {
                     throw e;
@@ -110,20 +117,26 @@ export class CodeSyncService {
                 return;
             }
 
-            if (!isWithFunctionAccessGuard(req) || !req.functionGuard?.canAccess(accessCodeSyncStorage)) {
+            const functionGuard = (req.user as WithFunctionAccessGuard).functionGuard;
+            if (!!functionGuard) {
+                // If we authorize with scopes, there is a FunctionGuard
+                const guard = new FGAFunctionAccessGuard(userId, functionGuard);
+                if (!(await guard.canAccess(accessCodeSyncStorage))) {
+                    res.sendStatus(403);
+                    return;
+                }
+            }
+
+            if (!(await this.authorizer.hasPermissionOnUser(userId, "code_sync", userId))) {
                 res.sendStatus(403);
                 return;
             }
+
             return next();
         });
 
         router.get("/v1/manifest", async (req, res) => {
-            if (!User.is(req.user)) {
-                res.sendStatus(400);
-                return;
-            }
-
-            const manifest = await this.db.getManifest(req.user.id);
+            const manifest = await this.db.getManifest(req.user!.id);
             if (!manifest) {
                 res.sendStatus(204);
                 return;
@@ -149,13 +162,8 @@ export class CodeSyncService {
         router.delete("/v1/collection/:collection/resource/:resource/:ref?", this.deleteResource.bind(this));
         router.delete("/v1/resource/:resource/:ref?", this.deleteResource.bind(this));
         router.delete("/v1/resource", async (req, res) => {
-            if (!User.is(req.user)) {
-                res.sendStatus(400);
-                return;
-            }
-
             // This endpoint is used to delete settings-sync data only
-            const userId = req.user.id;
+            const userId = req.user!.id;
             await this.db.deleteSettingsSyncResources(userId, async () => {
                 const request = new DeleteRequest();
                 request.setOwnerId(userId);
@@ -173,36 +181,21 @@ export class CodeSyncService {
         });
 
         router.get("/v1/collection", async (req, res) => {
-            if (!User.is(req.user)) {
-                res.sendStatus(400);
-                return;
-            }
-
-            const collections = await this.db.getCollections(req.user.id);
+            const collections = await this.db.getCollections(req.user!.id);
 
             res.json(collections);
             return;
         });
         router.post("/v1/collection", async (req, res) => {
-            if (!User.is(req.user)) {
-                res.sendStatus(400);
-                return;
-            }
-
-            const collection = await this.db.createCollection(req.user.id);
+            const collection = await this.db.createCollection(req.user!.id);
 
             res.type("text/plain");
             res.send(collection);
             return;
         });
         router.delete("/v1/collection/:collection?", async (req, res) => {
-            if (!User.is(req.user)) {
-                res.sendStatus(400);
-                return;
-            }
-
             const { collection } = req.params;
-            await this.deleteCollection(req.user.id, collection);
+            await this.deleteCollection(req.user!.id, collection);
 
             res.sendStatus(200);
         });
@@ -211,11 +204,6 @@ export class CodeSyncService {
     }
 
     private async getResources(req: express.Request<{ resource: string; collection?: string }>, res: express.Response) {
-        if (!User.is(req.user)) {
-            res.sendStatus(400);
-            return;
-        }
-
         const { resource, collection } = req.params;
         const resourceKey = ALL_SERVER_RESOURCES.find((key) => key === resource);
         if (!resourceKey) {
@@ -224,14 +212,14 @@ export class CodeSyncService {
         }
 
         if (collection) {
-            const valid = await this.db.isCollection(req.user.id, collection);
+            const valid = await this.db.isCollection(req.user!.id, collection);
             if (!valid) {
                 res.sendStatus(405);
                 return;
             }
         }
 
-        const revs = await this.db.getResources(req.user.id, resourceKey, collection);
+        const revs = await this.db.getResources(req.user!.id, resourceKey, collection);
         if (!revs.length) {
             res.sendStatus(204);
             return;
@@ -249,11 +237,6 @@ export class CodeSyncService {
         req: express.Request<{ resource: string; ref: string; collection?: string }>,
         res: express.Response,
     ) {
-        if (!User.is(req.user)) {
-            res.sendStatus(400);
-            return;
-        }
-
         const { resource, ref, collection } = req.params;
         const resourceKey = ALL_SERVER_RESOURCES.find((key) => key === resource);
         if (!resourceKey) {
@@ -262,14 +245,14 @@ export class CodeSyncService {
         }
 
         if (collection) {
-            const valid = await this.db.isCollection(req.user.id, collection);
+            const valid = await this.db.isCollection(req.user!.id, collection);
             if (!valid) {
                 res.sendStatus(405);
                 return;
             }
         }
 
-        const resourceRev = (await this.db.getResource(req.user.id, resourceKey, ref, collection))?.rev;
+        const resourceRev = (await this.db.getResource(req.user!.id, resourceKey, ref, collection))?.rev;
         if (!resourceRev) {
             res.setHeader("etag", "0");
             res.sendStatus(204);
@@ -283,7 +266,7 @@ export class CodeSyncService {
         let content: string;
         const contentType = req.headers["content-type"] || "*/*";
         const request = new DownloadUrlRequest();
-        request.setOwnerId(req.user.id);
+        request.setOwnerId(req.user!.id);
         request.setName(toObjectName(resourceKey, resourceRev, collection));
         request.setContentType(contentType);
         try {
@@ -317,11 +300,6 @@ export class CodeSyncService {
     }
 
     private async postResource(req: express.Request<{ resource: string; collection?: string }>, res: express.Response) {
-        if (!User.is(req.user)) {
-            res.sendStatus(400);
-            return;
-        }
-
         const { resource, collection } = req.params;
         const resourceKey = ALL_SERVER_RESOURCES.find((key) => key === resource);
         if (!resourceKey) {
@@ -330,7 +308,7 @@ export class CodeSyncService {
         }
 
         if (collection) {
-            const valid = await this.db.isCollection(req.user.id, collection);
+            const valid = await this.db.isCollection(req.user!.id, collection);
             if (!valid) {
                 res.sendStatus(405);
                 return;
@@ -346,7 +324,7 @@ export class CodeSyncService {
                   this.config.codeSync?.revLimit ||
                   defaultRevLimit;
         const isEditSessionsResource = resourceKey === "editSessions";
-        const userId = req.user.id;
+        const userId = req.user!.id;
         const contentType = req.headers["content-type"] || "*/*";
         const newRev = await this.db.insert(
             userId,
@@ -403,11 +381,6 @@ export class CodeSyncService {
         req: express.Request<{ resource: string; ref?: string; collection?: string }>,
         res: express.Response,
     ) {
-        if (!User.is(req.user)) {
-            res.sendStatus(400);
-            return;
-        }
-
         // This endpoint is used to delete edit sessions data for now
 
         const { resource, ref, collection } = req.params;
@@ -418,14 +391,14 @@ export class CodeSyncService {
         }
 
         if (collection) {
-            const valid = await this.db.isCollection(req.user.id, collection);
+            const valid = await this.db.isCollection(req.user!.id, collection);
             if (!valid) {
                 res.sendStatus(405);
                 return;
             }
         }
 
-        await this.doDeleteResource(req.user.id, resourceKey, ref, collection);
+        await this.doDeleteResource(req.user!.id, resourceKey, ref, collection);
         res.sendStatus(200);
         return;
     }

components/server/src/prometheus-metrics.ts

@@ -350,7 +350,7 @@ export const guardAccessChecksTotal = new prometheusClient.Counter({
     labelNames: ["type"],
 });
 
-export type GuardAccessCheckType = "fga" | "resource-access";
+export type GuardAccessCheckType = "fga" | "resource-access" | "function-access";
 export function reportGuardAccessCheck(type: GuardAccessCheckType) {
     guardAccessChecksTotal.labels(type).inc();
 }

components/server/src/server.ts

@@ -48,7 +48,6 @@ import { JobRunner } from "./jobs/runner";
 import { RedisSubscriber } from "./messaging/redis-subscriber";
 import { HEADLESS_LOGS_PATH_PREFIX, HEADLESS_LOG_DOWNLOAD_PATH_PREFIX } from "./workspace/headless-log-service";
 import { runWithRequestContext } from "./util/request-context";
-import { SubjectId } from "./auth/subject-id";
 
 @injectable()
 export class Server {
@@ -143,13 +142,14 @@ export class Server {
 
         // Use RequestContext for authorization
         app.use((req: express.Request, res: express.Response, next: express.NextFunction) => {
-            const userId = req.user ? req.user.id : undefined;
+            const abortController = new AbortController();
+            req.on("abort", () => abortController.abort());
             runWithRequestContext(
                 {
                     requestKind: "http",
                     requestMethod: req.path,
-                    signal: new AbortController().signal,
-                    subjectId: userId ? SubjectId.fromUserId(userId) : undefined, // TODO(gpl) Can we assume this? E.g., has this been verified? It should: It means we could decode the cookie, right?
+                    signal: abortController.signal,
+                    subjectId: undefined, // Don't use req.user, as that would elevate permissions once we rollout scope-based API tokens
                     headers: toHeaders(req.headers),
                 },
                 () => next(),
@@ -296,19 +296,27 @@ export class Server {
     }
 
     protected async registerRoutes(app: express.Application) {
+        // Authorization: Session only
         app.use(this.userController.apiRouter);
-        app.use(this.oneTimeSecretServer.apiRouter);
         app.use("/workspace-download", this.workspaceDownloadService.apiRouter);
-        app.use("/code-sync", this.codeSyncService.apiRouter);
+        app.use(this.oauthController.oauthRouter);
+
+        // Authorization: Session or Bearer token
         app.use(HEADLESS_LOGS_PATH_PREFIX, this.headlessLogController.headlessLogs);
         app.use(HEADLESS_LOG_DOWNLOAD_PATH_PREFIX, this.headlessLogController.headlessLogDownload);
-        app.use("/live", this.livenessController.apiRouter);
+
+        // Authorization: Bearer token only
+        app.use("/code-sync", this.codeSyncService.apiRouter);
+
+        // Authorization: none
+        app.use(this.oneTimeSecretServer.apiRouter);
         app.use(this.newsletterSubscriptionController.apiRouter);
+        app.use("/live", this.livenessController.apiRouter);
         app.use("/version", (req: express.Request, res: express.Response, next: express.NextFunction) => {
             res.send(this.config.version);
         });
-        app.use(this.oauthController.oauthRouter);
 
+        // Authorization: Custom
         if (this.config.githubApp?.enabled && this.githubApp.server) {
             log.info("Registered GitHub app at /apps/github");
             app.use("/apps/github/", this.githubApp.server?.expressApp);