From 133cab5cd80e35937cdfbd51fab6afdfcfc26c91 Mon Sep 17 00:00:00 2001 From: Gero Posmyk-Leinemann Date: Wed, 6 Dec 2023 08:48:04 +0000 Subject: [PATCH] [authorizer] prepare Authorizer for SubjectId rollout --- components/server/src/auth/subject-id.ts | 2 +- .../src/authorization/authorizer.spec.db.ts | 125 +++++++++++++++++- .../server/src/authorization/authorizer.ts | 30 ++++- components/server/src/prometheus-metrics.ts | 2 +- .../test/service-testing-container-module.ts | 8 +- components/server/src/util/request-context.ts | 27 +++- 6 files changed, 182 insertions(+), 12 deletions(-) diff --git a/components/server/src/auth/subject-id.ts b/components/server/src/auth/subject-id.ts index 2ed6e367984f80..af74291e1573f2 100644 --- a/components/server/src/auth/subject-id.ts +++ b/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)) { diff --git a/components/server/src/authorization/authorizer.spec.db.ts b/components/server/src/authorization/authorizer.spec.db.ts index 4a5b3093310c23..b2fe4b82b8093b 100644 --- a/components/server/src/authorization/authorizer.spec.db.ts +++ b/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}`); + } + } + }, + ); + } + }); +}); diff --git a/components/server/src/authorization/authorizer.ts b/components/server/src/authorization/authorizer.ts index 671ff376dac0a9..bc3cfc99ca576a 100644 --- a/components/server/src/authorization/authorizer.ts +++ b/components/server/src/authorization/authorizer.ts @@ -520,18 +520,26 @@ export class Authorizer { } } -async function getSubjectFromCtx(passed: Subject): Promise { +export async function getSubjectFromCtx(passed: Subject): Promise { 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 { : 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; } diff --git a/components/server/src/prometheus-metrics.ts b/components/server/src/prometheus-metrics.ts index 8db1ef40cd54cb..6e512f7b713789 100644 --- a/components/server/src/prometheus-metrics.ts +++ b/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(); } diff --git a/components/server/src/test/service-testing-container-module.ts b/components/server/src/test/service-testing-container-module.ts index 67edb870d7e742..441bcb2c771e85 100644 --- a/components/server/src/test/service-testing-container-module.ts +++ b/components/server/src/test/service-testing-container-module.ts @@ -334,12 +334,18 @@ export function createTestContainer() { } export function withTestCtx(subject: Subject | User, p: () => Promise): Promise { + 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, ); diff --git a/components/server/src/util/request-context.ts b/components/server/src/util/request-context.ts index 0a6485b650db49..b7bf887b5f86c5 100644 --- a/components/server/src/util/request-context.ts +++ b/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(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(subjectId: SubjectId | undefined, fun: () => T): T { const parent = ctxTryGet(); if (!parent) {