[authorizer] prepare Authorizer for SubjectId rollout

components/server/src/auth/subject-id.ts

@@ -73,7 +73,7 @@ export class SubjectId {
 /**
  * Interface type meant for backwards compatibility
  */
-export type Subject = string | SubjectId;
+export type Subject = string | SubjectId | undefined;
 export namespace Subject {
     export function toId(subject: Subject): SubjectId {
         if (SubjectId.is(subject)) {

components/server/src/authorization/authorizer.spec.db.ts

@@ -11,9 +11,13 @@ import * as chai from "chai";
 import { Container } from "inversify";
 import "mocha";
 import { createTestContainer } from "../test/service-testing-container-module";
-import { Authorizer } from "./authorizer";
+import { Authorizer, getSubjectFromCtx } from "./authorizer";
 import { rel } from "./definitions";
 import { v4 } from "uuid";
+import { Subject, SubjectId } from "../auth/subject-id";
+import { runWithRequestContext } from "../util/request-context";
+import { fail } from "assert";
+import { ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error";
 
 const expect = chai.expect;
 
@@ -141,3 +145,122 @@ describe("Authorizer", async () => {
         expect(rs).to.be.undefined;
     }
 });
+
+describe("getSubjectFromCtx", async () => {
+    it("all tests", async () => {
+        interface Test {
+            name: string;
+            passedSubject: Subject;
+            contextSubjectId: SubjectId | undefined;
+            authWithRequestContext: boolean;
+            expected: SubjectId | number;
+        }
+        const tests: Test[] = [
+            // Feature flag is OFF
+            {
+                name: "both given and match, ff off",
+                passedSubject: "u1",
+                contextSubjectId: SubjectId.fromUserId("u1"),
+                authWithRequestContext: false,
+                expected: SubjectId.fromUserId("u1"),
+            },
+            {
+                name: "both given and mismatch, ff off",
+                passedSubject: "u1",
+                contextSubjectId: SubjectId.fromUserId("u2"),
+                authWithRequestContext: false,
+                expected: SubjectId.fromUserId("u1"),
+            },
+            {
+                name: "passed only, ff off",
+                passedSubject: "u1",
+                contextSubjectId: undefined,
+                authWithRequestContext: false,
+                expected: SubjectId.fromUserId("u1"),
+            },
+            {
+                name: "ctx only, ff off",
+                passedSubject: undefined,
+                contextSubjectId: SubjectId.fromUserId("u1"),
+                authWithRequestContext: false,
+                expected: ErrorCodes.PERMISSION_DENIED,
+            },
+            {
+                name: "none passed, ff off",
+                passedSubject: undefined,
+                contextSubjectId: undefined,
+                authWithRequestContext: false,
+                expected: ErrorCodes.PERMISSION_DENIED,
+            },
+            // Feature flag is ON
+            {
+                name: "both given and match, ff on",
+                passedSubject: "u1",
+                contextSubjectId: SubjectId.fromUserId("u1"),
+                authWithRequestContext: true,
+                expected: SubjectId.fromUserId("u1"),
+            },
+            {
+                name: "both given and mismatch, ff on",
+                passedSubject: "u1",
+                contextSubjectId: SubjectId.fromUserId("u2"),
+                authWithRequestContext: true,
+                expected: ErrorCodes.PERMISSION_DENIED,
+            },
+            {
+                name: "passed only, ff on",
+                passedSubject: "u1",
+                contextSubjectId: undefined,
+                authWithRequestContext: true,
+                expected: ErrorCodes.PERMISSION_DENIED,
+            },
+            {
+                name: "ctx only, ff on",
+                passedSubject: undefined,
+                contextSubjectId: SubjectId.fromUserId("u1"),
+                authWithRequestContext: true,
+                expected: SubjectId.fromUserId("u1"),
+            },
+            {
+                name: "none passed, ff on",
+                passedSubject: undefined,
+                contextSubjectId: undefined,
+                authWithRequestContext: true,
+                expected: ErrorCodes.PERMISSION_DENIED,
+            },
+        ];
+
+        for (const test of tests) {
+            Experiments.configureTestingClient({
+                authWithRequestContext: test.authWithRequestContext,
+            });
+
+            await runWithRequestContext(
+                {
+                    requestKind: "test",
+                    requestMethod: test.name,
+                    signal: new AbortController().signal,
+                    subjectId: test.contextSubjectId,
+                },
+                async () => {
+                    try {
+                        const actual = await getSubjectFromCtx(test.passedSubject);
+                        expect(actual, `${test.name}, expected ${test.expected}, got ${actual}`).to.deep.equal(
+                            test.expected,
+                        );
+                    } catch (err) {
+                        if (typeof test.expected === "number") {
+                            expect(
+                                err.code,
+                                `${test.name}, expected ${test.expected}, got ${err.code} (${err.message})`,
+                            ).to.equal(test.expected);
+                        } else {
+                            const msg = err?.message || JSON.stringify(err) || "unknown error";
+                            fail(`${test.name}, ${msg}`);
+                        }
+                    }
+                },
+            );
+        }
+    });
+});

components/server/src/authorization/authorizer.ts

@@ -520,18 +520,26 @@ export class Authorizer {
     }
 }
 
-async function getSubjectFromCtx(passed: Subject): Promise<SubjectId> {
+export async function getSubjectFromCtx(passed: Subject): Promise<SubjectId> {
     const ctxSubjectId = ctxTrySubjectId();
     const ctxUserId = ctxSubjectId?.userId();
 
-    const passedSubjectId = Subject.toId(passed);
-    const passedUserId = passedSubjectId.userId();
+    const passedSubjectId = !!passed ? Subject.toId(passed) : undefined;
+    const passedUserId = passedSubjectId?.userId();
 
     // Check: Do the subjectIds match?
-    const matchingSubjectId = ctxUserId === passedUserId;
-    const match = !ctxUserId ? "ctx-user-id-missing" : matchingSubjectId ? "match" : "mismatch";
+    function matchSubjectIds(ctxUserId: string | undefined, passedSubjectId: string | undefined) {
+        if (!ctxUserId) {
+            return "ctx-user-id-missing";
+        }
+        if (!passedSubjectId) {
+            return "passed-subject-id-missing";
+        }
+        return ctxUserId === passedUserId ? "match" : "mismatch";
+    }
+    const match = matchSubjectIds(ctxUserId, passedUserId);
     reportAuthorizerSubjectId(match);
-    if (match !== "match") {
+    if (match === "mismatch") {
         try {
             // Get hold of the stack trace
             throw new Error("Authorizer: SubjectId mismatch");
@@ -553,6 +561,16 @@ async function getSubjectFromCtx(passed: Subject): Promise<SubjectId> {
             : undefined,
     });
     if (!authViaContext) {
+        if (!passedSubjectId) {
+            const err = new ApplicationError(ErrorCodes.PERMISSION_DENIED, `Cannot authorize request`);
+            log.error("Authorizer: Cannot authorize request: missing SubjectId", err, {
+                match,
+                ctxSubjectId,
+                ctxUserId,
+                passedUserId,
+            });
+            throw err;
+        }
         return passedSubjectId;
     }
 

components/server/src/prometheus-metrics.ts

@@ -376,7 +376,7 @@ export const authorizerSubjectId = new prometheusClient.Counter({
     help: "Counter for the number of authorizer permission checks",
     labelNames: ["match"],
 });
-type AuthorizerSubjectIdMatch = "ctx-user-id-missing" | "match" | "mismatch";
+type AuthorizerSubjectIdMatch = "ctx-user-id-missing" | "passed-subject-id-missing" | "match" | "mismatch";
 export function reportAuthorizerSubjectId(match: AuthorizerSubjectIdMatch) {
     authorizerSubjectId.labels(match).inc();
 }

components/server/src/test/service-testing-container-module.ts

@@ -334,12 +334,18 @@ export function createTestContainer() {
 }
 
 export function withTestCtx<T>(subject: Subject | User, p: () => Promise<T>): Promise<T> {
+    let subjectId: SubjectId | undefined = undefined;
+    if (SubjectId.is(subject)) {
+        subjectId = subject;
+    } else if (subject !== undefined) {
+        subjectId = SubjectId.fromUserId(User.is(subject) ? subject.id : subject);
+    }
     return runWithRequestContext(
         {
             requestKind: "testContext",
             requestMethod: "testMethod",
             signal: new AbortController().signal,
-            subjectId: SubjectId.is(subject) ? subject : SubjectId.fromUserId(User.is(subject) ? subject.id : subject),
+            subjectId,
         },
         p,
     );

components/server/src/util/request-context.ts

@@ -10,6 +10,7 @@ import { v4 } from "uuid";
 import { SubjectId } from "../auth/subject-id";
 import { ApplicationError, ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error";
 import { takeFirst } from "../express-util";
+import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
 
 /**
  * RequestContext is the context that all our request-handling code runs in.
@@ -21,11 +22,12 @@ import { takeFirst } from "../express-util";
  *
  * It's meant to be nestable, so that we can run code in a child context with different properties.
  * The only example we have for now is "runWithSubjectId", which executes the child context with different authorization.
+ * @see runWithRequestContext
  * @see runWithSubjectId
  */
 export interface RequestContext {
     /**
-     * Unique, artificial ID for this request.
+     * Unique, artificial, backend-controlled ID for this request.
      */
     readonly requestId: string;
 
@@ -165,18 +167,39 @@ export type RequestContextSeed = Omit<RequestContext, "requestId" | "startTime"
 };
 
 /**
- * The context all our request-handling code should run in.
+ * Creates a _root_ context request-handling code should run in. MANDATORY for any authorization to work.
+ * Uses AsyncLocalStorage under the hood, but offers a more convenient API.
  * @param context
  * @param fun
  * @returns
  */
 export function runWithRequestContext<T>(context: RequestContextSeed, fun: () => T): T {
+    // TODO(gpl): Turn this into an exception
+    const parent = ctxTryGet();
+    if (!!parent) {
+        try {
+            throw new Error("Nested context detected");
+        } catch (err) {
+            log.error("runWithRequestContext", err, {
+                parent: { requestKind: parent.requestKind, requestMethod: parent.requestMethod },
+            });
+        }
+    }
+
     const requestId = context.requestId || v4();
     const startTime = context.startTime || performance.now();
     const cache = {};
     return runWithContext({ ...context, requestId, startTime, cache }, fun);
 }
 
+/**
+ * Creates a _child_ context with the given subjectId. It takes all top-level values from the parent context, and only overrides subjectId.
+ * This is useful for running code with a (different) authorization than the parent context.
+ * @param subjectId
+ * @param fun
+ * @throws If there is no parent context available
+ * @returns The result of the given function
+ */
 export function runWithSubjectId<T>(subjectId: SubjectId | undefined, fun: () => T): T {
     const parent = ctxTryGet();
     if (!parent) {