Skip to content

Commit 2b5a36e

Browse files
geroplgeropl
authored
[authorizer] prepare Authorizer for SubjectId rollout (#19195)
1 parent ca02c26 commit 2b5a36e

File tree

6 files changed

+182
-12
lines changed

6 files changed

+182
-12
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ export class SubjectId {
7373
/**
7474
* Interface type meant for backwards compatibility
7575
*/
76-
export type Subject = string | SubjectId;
76+
export type Subject = string | SubjectId | undefined;
7777
export namespace Subject {
7878
export function toId(subject: Subject): SubjectId {
7979
if (SubjectId.is(subject)) {

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

Lines changed: 124 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,13 @@ import * as chai from "chai";
1111
import { Container } from "inversify";
1212
import "mocha";
1313
import { createTestContainer } from "../test/service-testing-container-module";
14-
import { Authorizer } from "./authorizer";
14+
import { Authorizer, getSubjectFromCtx } from "./authorizer";
1515
import { rel } from "./definitions";
1616
import { v4 } from "uuid";
17+
import { Subject, SubjectId } from "../auth/subject-id";
18+
import { runWithRequestContext } from "../util/request-context";
19+
import { fail } from "assert";
20+
import { ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error";
1721

1822
const expect = chai.expect;
1923

@@ -141,3 +145,122 @@ describe("Authorizer", async () => {
141145
expect(rs).to.be.undefined;
142146
}
143147
});
148+
149+
describe("getSubjectFromCtx", async () => {
150+
it("all tests", async () => {
151+
interface Test {
152+
name: string;
153+
passedSubject: Subject;
154+
contextSubjectId: SubjectId | undefined;
155+
authWithRequestContext: boolean;
156+
expected: SubjectId | number;
157+
}
158+
const tests: Test[] = [
159+
// Feature flag is OFF
160+
{
161+
name: "both given and match, ff off",
162+
passedSubject: "u1",
163+
contextSubjectId: SubjectId.fromUserId("u1"),
164+
authWithRequestContext: false,
165+
expected: SubjectId.fromUserId("u1"),
166+
},
167+
{
168+
name: "both given and mismatch, ff off",
169+
passedSubject: "u1",
170+
contextSubjectId: SubjectId.fromUserId("u2"),
171+
authWithRequestContext: false,
172+
expected: SubjectId.fromUserId("u1"),
173+
},
174+
{
175+
name: "passed only, ff off",
176+
passedSubject: "u1",
177+
contextSubjectId: undefined,
178+
authWithRequestContext: false,
179+
expected: SubjectId.fromUserId("u1"),
180+
},
181+
{
182+
name: "ctx only, ff off",
183+
passedSubject: undefined,
184+
contextSubjectId: SubjectId.fromUserId("u1"),
185+
authWithRequestContext: false,
186+
expected: ErrorCodes.PERMISSION_DENIED,
187+
},
188+
{
189+
name: "none passed, ff off",
190+
passedSubject: undefined,
191+
contextSubjectId: undefined,
192+
authWithRequestContext: false,
193+
expected: ErrorCodes.PERMISSION_DENIED,
194+
},
195+
// Feature flag is ON
196+
{
197+
name: "both given and match, ff on",
198+
passedSubject: "u1",
199+
contextSubjectId: SubjectId.fromUserId("u1"),
200+
authWithRequestContext: true,
201+
expected: SubjectId.fromUserId("u1"),
202+
},
203+
{
204+
name: "both given and mismatch, ff on",
205+
passedSubject: "u1",
206+
contextSubjectId: SubjectId.fromUserId("u2"),
207+
authWithRequestContext: true,
208+
expected: ErrorCodes.PERMISSION_DENIED,
209+
},
210+
{
211+
name: "passed only, ff on",
212+
passedSubject: "u1",
213+
contextSubjectId: undefined,
214+
authWithRequestContext: true,
215+
expected: ErrorCodes.PERMISSION_DENIED,
216+
},
217+
{
218+
name: "ctx only, ff on",
219+
passedSubject: undefined,
220+
contextSubjectId: SubjectId.fromUserId("u1"),
221+
authWithRequestContext: true,
222+
expected: SubjectId.fromUserId("u1"),
223+
},
224+
{
225+
name: "none passed, ff on",
226+
passedSubject: undefined,
227+
contextSubjectId: undefined,
228+
authWithRequestContext: true,
229+
expected: ErrorCodes.PERMISSION_DENIED,
230+
},
231+
];
232+
233+
for (const test of tests) {
234+
Experiments.configureTestingClient({
235+
authWithRequestContext: test.authWithRequestContext,
236+
});
237+
238+
await runWithRequestContext(
239+
{
240+
requestKind: "test",
241+
requestMethod: test.name,
242+
signal: new AbortController().signal,
243+
subjectId: test.contextSubjectId,
244+
},
245+
async () => {
246+
try {
247+
const actual = await getSubjectFromCtx(test.passedSubject);
248+
expect(actual, `${test.name}, expected ${test.expected}, got ${actual}`).to.deep.equal(
249+
test.expected,
250+
);
251+
} catch (err) {
252+
if (typeof test.expected === "number") {
253+
expect(
254+
err.code,
255+
`${test.name}, expected ${test.expected}, got ${err.code} (${err.message})`,
256+
).to.equal(test.expected);
257+
} else {
258+
const msg = err?.message || JSON.stringify(err) || "unknown error";
259+
fail(`${test.name}, ${msg}`);
260+
}
261+
}
262+
},
263+
);
264+
}
265+
});
266+
});

‎components/server/src/authorization/authorizer.ts

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -520,18 +520,26 @@ export class Authorizer {
520520
}
521521
}
522522

523-
async function getSubjectFromCtx(passed: Subject): Promise<SubjectId> {
523+
export async function getSubjectFromCtx(passed: Subject): Promise<SubjectId> {
524524
const ctxSubjectId = ctxTrySubjectId();
525525
const ctxUserId = ctxSubjectId?.userId();
526526

527-
const passedSubjectId = Subject.toId(passed);
528-
const passedUserId = passedSubjectId.userId();
527+
const passedSubjectId = !!passed ? Subject.toId(passed) : undefined;
528+
const passedUserId = passedSubjectId?.userId();
529529

530530
// Check: Do the subjectIds match?
531-
const matchingSubjectId = ctxUserId === passedUserId;
532-
const match = !ctxUserId ? "ctx-user-id-missing" : matchingSubjectId ? "match" : "mismatch";
531+
function matchSubjectIds(ctxUserId: string | undefined, passedSubjectId: string | undefined) {
532+
if (!ctxUserId) {
533+
return "ctx-user-id-missing";
534+
}
535+
if (!passedSubjectId) {
536+
return "passed-subject-id-missing";
537+
}
538+
return ctxUserId === passedUserId ? "match" : "mismatch";
539+
}
540+
const match = matchSubjectIds(ctxUserId, passedUserId);
533541
reportAuthorizerSubjectId(match);
534-
if (match !== "match") {
542+
if (match === "mismatch") {
535543
try {
536544
// Get hold of the stack trace
537545
throw new Error("Authorizer: SubjectId mismatch");
@@ -553,6 +561,16 @@ async function getSubjectFromCtx(passed: Subject): Promise<SubjectId> {
553561
: undefined,
554562
});
555563
if (!authViaContext) {
564+
if (!passedSubjectId) {
565+
const err = new ApplicationError(ErrorCodes.PERMISSION_DENIED, `Cannot authorize request`);
566+
log.error("Authorizer: Cannot authorize request: missing SubjectId", err, {
567+
match,
568+
ctxSubjectId,
569+
ctxUserId,
570+
passedUserId,
571+
});
572+
throw err;
573+
}
556574
return passedSubjectId;
557575
}
558576

‎components/server/src/prometheus-metrics.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -376,7 +376,7 @@ export const authorizerSubjectId = new prometheusClient.Counter({
376376
help: "Counter for the number of authorizer permission checks",
377377
labelNames: ["match"],
378378
});
379-
type AuthorizerSubjectIdMatch = "ctx-user-id-missing" | "match" | "mismatch";
379+
type AuthorizerSubjectIdMatch = "ctx-user-id-missing" | "passed-subject-id-missing" | "match" | "mismatch";
380380
export function reportAuthorizerSubjectId(match: AuthorizerSubjectIdMatch) {
381381
authorizerSubjectId.labels(match).inc();
382382
}

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -334,12 +334,18 @@ export function createTestContainer() {
334334
}
335335

336336
export function withTestCtx<T>(subject: Subject | User, p: () => Promise<T>): Promise<T> {
337+
let subjectId: SubjectId | undefined = undefined;
338+
if (SubjectId.is(subject)) {
339+
subjectId = subject;
340+
} else if (subject !== undefined) {
341+
subjectId = SubjectId.fromUserId(User.is(subject) ? subject.id : subject);
342+
}
337343
return runWithRequestContext(
338344
{
339345
requestKind: "testContext",
340346
requestMethod: "testMethod",
341347
signal: new AbortController().signal,
342-
subjectId: SubjectId.is(subject) ? subject : SubjectId.fromUserId(User.is(subject) ? subject.id : subject),
348+
subjectId,
343349
},
344350
p,
345351
);

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

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { v4 } from "uuid";
1010
import { SubjectId } from "../auth/subject-id";
1111
import { ApplicationError, ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error";
1212
import { takeFirst } from "../express-util";
13+
import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
1314

1415
/**
1516
* RequestContext is the context that all our request-handling code runs in.
@@ -21,11 +22,12 @@ import { takeFirst } from "../express-util";
2122
*
2223
* It's meant to be nestable, so that we can run code in a child context with different properties.
2324
* The only example we have for now is "runWithSubjectId", which executes the child context with different authorization.
25+
* @see runWithRequestContext
2426
* @see runWithSubjectId
2527
*/
2628
export interface RequestContext {
2729
/**
28-
* Unique, artificial ID for this request.
30+
* Unique, artificial, backend-controlled ID for this request.
2931
*/
3032
readonly requestId: string;
3133

@@ -165,18 +167,39 @@ export type RequestContextSeed = Omit<RequestContext, "requestId" | "startTime"
165167
};
166168

167169
/**
168-
* The context all our request-handling code should run in.
170+
* Creates a _root_ context request-handling code should run in. MANDATORY for any authorization to work.
171+
* Uses AsyncLocalStorage under the hood, but offers a more convenient API.
169172
* @param context
170173
* @param fun
171174
* @returns
172175
*/
173176
export function runWithRequestContext<T>(context: RequestContextSeed, fun: () => T): T {
177+
// TODO(gpl): Turn this into an exception
178+
const parent = ctxTryGet();
179+
if (!!parent) {
180+
try {
181+
throw new Error("Nested context detected");
182+
} catch (err) {
183+
log.error("runWithRequestContext", err, {
184+
parent: { requestKind: parent.requestKind, requestMethod: parent.requestMethod },
185+
});
186+
}
187+
}
188+
174189
const requestId = context.requestId || v4();
175190
const startTime = context.startTime || performance.now();
176191
const cache = {};
177192
return runWithContext({ ...context, requestId, startTime, cache }, fun);
178193
}
179194

195+
/**
196+
* Creates a _child_ context with the given subjectId. It takes all top-level values from the parent context, and only overrides subjectId.
197+
* This is useful for running code with a (different) authorization than the parent context.
198+
* @param subjectId
199+
* @param fun
200+
* @throws If there is no parent context available
201+
* @returns The result of the given function
202+
*/
180203
export function runWithSubjectId<T>(subjectId: SubjectId | undefined, fun: () => T): T {
181204
const parent = ctxTryGet();
182205
if (!parent) {

0 commit comments

Comments
 (0)