diff --git a/acs-i3x/lib/api-v1.ts b/acs-i3x/lib/api-v1.ts index 67a2be77d..0056e5f37 100644 --- a/acs-i3x/lib/api-v1.ts +++ b/acs-i3x/lib/api-v1.ts @@ -59,6 +59,22 @@ function badRequest(message: string): Error & { status: number } { return err; } +/** + * Returns the authenticated principal which owns any subscription + * created or accessed by this request. + * + * `req.auth` is set by the shared FplusHttpAuth middleware in + * @amrc-factoryplus/service-api. Every subscription route sits behind + * it — only `/v1/info` is public, and that route never touches + * subscriptions — so this should always be a principal. If it is not, + * we return undefined and SubscriptionManager fails closed with 404, + * rather than letting an unauthenticated request own or reach + * anything. + */ +function subscription_owner(req: Request): string { + return (req as any).auth; +} + export class APIv1 { public routes: Router; public infoRoute: Router; @@ -439,18 +455,19 @@ export class APIv1 { } /** - * POST /subscriptions/list — looks up subscriptions by id for a - * client. Per-id success/error envelope: missing ids surface as - * 404 and ids owned by a different client as 403, rather than - * being silently dropped. Each success entry includes a - * `monitoredObjects: [{ elementId, maxDepth }]` array built from - * the subscription's registered elements. + * POST /subscriptions/list — looks up subscriptions by id for the + * authenticated principal. Per-id success/error envelope: ids that + * do not exist, and ids owned by another principal, both surface + * as 404 rather than being silently dropped. Each success entry + * includes a `monitoredObjects: [{ elementId, maxDepth }]` array + * built from the subscription's registered elements. */ list_subscriptions(req: Request, res: Response): void { - const { clientId, subscriptionIds } = req.body; + const owner = subscription_owner(req); + const { subscriptionIds } = req.body; const results = (subscriptionIds as string[]).map(id => { try { - const sub = this.subscriptions.getOne(clientId, id); + const sub = this.subscriptions.getOne(owner, id); return { success: true, subscriptionId: id, result: sub }; } catch (err: any) { return { @@ -466,14 +483,16 @@ export class APIv1 { /** * POST /subscriptions/delete — deletes the listed subscriptions for - * a client. Per-id success/error envelope: missing or wrong-client - * ids are reported as failures rather than aborting the batch. + * the authenticated principal. Per-id success/error envelope: ids + * which do not exist, or belong to another principal, are reported + * as 404 failures rather than aborting the batch. */ delete_subscriptions(req: Request, res: Response): void { - const { clientId, subscriptionIds } = req.body; + const owner = subscription_owner(req); + const { subscriptionIds } = req.body; const results = (subscriptionIds as string[]).map(id => { try { - this.subscriptions.deleteOne(clientId, id); + this.subscriptions.deleteOne(owner, id); return { success: true, subscriptionId: id, result: null }; } catch (err: any) { return { @@ -491,17 +510,18 @@ export class APIv1 { * POST /subscriptions/register — adds element ids to an existing * subscription, with optional composition `maxDepth`. Per-id * success/error envelope: unknown ids are reported as 404, sub-level - * errors (missing sub / wrong client) surface from `registerOne` as - * 404/403 per-id rather than aborting the batch. + * errors (missing sub, or one owned by another principal) surface + * from `registerOne` as 404 per-id rather than aborting the batch. */ register_subscriptions(req: Request, res: Response): void { - const { clientId, subscriptionId, elementIds, maxDepth } = req.body; + const owner = subscription_owner(req); + const { subscriptionId, elementIds, maxDepth } = req.body; const results = (elementIds as string[]).map(id => { if (!this.objectTree.getObject(id)) { return { success: false, elementId: id, error: { code: 404, message: `Object ${id} not found` } }; } try { - this.subscriptions.registerOne(clientId, subscriptionId, id, maxDepth); + this.subscriptions.registerOne(owner, subscriptionId, id, maxDepth); return { success: true, elementId: id, result: null }; } catch (err: any) { return { @@ -518,18 +538,19 @@ export class APIv1 { /** * POST /subscriptions/unregister — removes element ids from an * existing subscription. Per-id success/error envelope: unknown - * ids are reported as 404, sub-level errors (missing sub / wrong - * client) surface from `unregisterOne` as 404/403 per-id rather - * than aborting the batch. + * ids are reported as 404, sub-level errors (missing sub, or one + * owned by another principal) surface from `unregisterOne` as 404 + * per-id rather than aborting the batch. */ unregister_subscriptions(req: Request, res: Response): void { - const { clientId, subscriptionId, elementIds } = req.body; + const owner = subscription_owner(req); + const { subscriptionId, elementIds } = req.body; const results = (elementIds as string[]).map(id => { if (!this.objectTree.getObject(id)) { return { success: false, elementId: id, error: { code: 404, message: `Object ${id} not found` } }; } try { - this.subscriptions.unregisterOne(clientId, subscriptionId, id); + this.subscriptions.unregisterOne(owner, subscriptionId, id); return { success: true, elementId: id, result: null }; } catch (err: any) { return { @@ -550,23 +571,28 @@ export class APIv1 { * do NOT call `res.json` here. */ async stream_subscription(req: Request, res: Response, _next: NextFunction): Promise { - const { clientId, subscriptionId } = req.body; - this.subscriptions.stream(clientId, subscriptionId, res); + const { subscriptionId } = req.body; + this.subscriptions.stream(subscription_owner(req), subscriptionId, res); } /** * POST /subscriptions/sync — replays missed updates after `lastSequenceNumber`. **/ sync_subscription(req: Request, res: Response): void { - const { clientId, subscriptionId, lastSequenceNumber } = req.body; - res.json(this.subscriptions.sync(clientId, subscriptionId, lastSequenceNumber)); + const { subscriptionId, lastSequenceNumber } = req.body; + res.json(this.subscriptions.sync( + subscription_owner(req), subscriptionId, lastSequenceNumber)); } /** - * POST /subscriptions — creates a new subscription for the given client. + * POST /subscriptions — creates a new subscription owned by the + * authenticated principal. The client-supplied `clientId` is + * stored and echoed back because it is part of the i3X wire shape, + * but ownership is the principal, not the clientId. **/ create_subscription(req: Request, res: Response): void { const { clientId, displayName } = req.body; - res.json(this.subscriptions.create(clientId, displayName)); + res.json(this.subscriptions.create( + subscription_owner(req), clientId, displayName)); } } diff --git a/acs-i3x/lib/subscriptions.ts b/acs-i3x/lib/subscriptions.ts index 4e7868163..8a47ce852 100644 --- a/acs-i3x/lib/subscriptions.ts +++ b/acs-i3x/lib/subscriptions.ts @@ -17,6 +17,12 @@ interface SubscriptionManagerOpts { } interface Subscription { + /* The authenticated Factory+ principal which created the + * subscription. This is what ownership is checked against; it is + * never supplied by the client and is never sent on the wire. */ + owner: string; + /* The client's own handle for itself. Part of the i3X wire shape, + * so we store and echo it, but it protects nothing. */ clientId: string; subscriptionId: string; displayName: string; @@ -41,9 +47,13 @@ export class SubscriptionManager { this.valueCache.onValueChange(this.boundOnValueChange); } - create(clientId: string, displayName?: string): I3xSubscription { + /* `owner` is the authenticated principal (`req.auth`); `clientId` + * is the client-supplied i3X handle. Only `owner` grants access to + * the subscription afterwards. */ + create(owner: string, clientId: string, displayName?: string): I3xSubscription { const subscriptionId = randomUUID(); const sub: Subscription = { + owner, clientId, subscriptionId, displayName: displayName ?? "", @@ -64,11 +74,11 @@ export class SubscriptionManager { }; } - list(clientId: string, subscriptionIds: string[]): I3xSubscription[] { + list(owner: string, subscriptionIds: string[]): I3xSubscription[] { const results: I3xSubscription[] = []; for (const id of subscriptionIds) { const sub = this.subscriptions.get(id); - if (sub && sub.clientId === clientId) { + if (sub && owner && sub.owner === owner) { results.push({ clientId: sub.clientId, subscriptionId: sub.subscriptionId, @@ -79,8 +89,8 @@ export class SubscriptionManager { return results; } - getOne(clientId: string, subscriptionId: string): I3xSubscription { - const sub = this.getAndVerify(clientId, subscriptionId); + getOne(owner: string, subscriptionId: string): I3xSubscription { + const sub = this.getAndVerify(owner, subscriptionId); const monitoredObjects = [...sub.registeredElements.entries()] .map(([elementId, maxDepth]) => ({ elementId, maxDepth })); @@ -95,8 +105,8 @@ export class SubscriptionManager { }; } - deleteOne(clientId: string, subscriptionId: string): void { - const sub = this.getAndVerify(clientId, subscriptionId); + deleteOne(owner: string, subscriptionId: string): void { + const sub = this.getAndVerify(owner, subscriptionId); clearTimeout(sub.ttlTimer); if (sub.activeStream) { @@ -106,8 +116,8 @@ export class SubscriptionManager { this.subscriptions.delete(subscriptionId); } - register(clientId: string, subscriptionId: string, elementIds: string[], maxDepth: number = 1): void { - const sub = this.getAndVerify(clientId, subscriptionId); + register(owner: string, subscriptionId: string, elementIds: string[], maxDepth: number = 1): void { + const sub = this.getAndVerify(owner, subscriptionId); for (const elementId of elementIds) { sub.registeredElements.set(elementId, maxDepth); @@ -117,15 +127,15 @@ export class SubscriptionManager { this.resetTtl(sub); } - registerOne(clientId: string, subscriptionId: string, elementId: string, maxDepth: number = 1): void { - const sub = this.getAndVerify(clientId, subscriptionId); + registerOne(owner: string, subscriptionId: string, elementId: string, maxDepth: number = 1): void { + const sub = this.getAndVerify(owner, subscriptionId); sub.registeredElements.set(elementId, maxDepth); console.log(`[SUB] register: sub=${subscriptionId.slice(0,8)} element=${elementId} maxDepth=${maxDepth}`); this.resetTtl(sub); } - unregister(clientId: string, subscriptionId: string, elementIds: string[]): void { - const sub = this.getAndVerify(clientId, subscriptionId); + unregister(owner: string, subscriptionId: string, elementIds: string[]): void { + const sub = this.getAndVerify(owner, subscriptionId); for (const elementId of elementIds) { sub.registeredElements.delete(elementId); @@ -134,14 +144,14 @@ export class SubscriptionManager { this.resetTtl(sub); } - unregisterOne(clientId: string, subscriptionId: string, elementId: string): void { - const sub = this.getAndVerify(clientId, subscriptionId); + unregisterOne(owner: string, subscriptionId: string, elementId: string): void { + const sub = this.getAndVerify(owner, subscriptionId); sub.registeredElements.delete(elementId); this.resetTtl(sub); } - sync(clientId: string, subscriptionId: string, lastSequenceNumber?: number): I3xSyncItem[] { - const sub = this.getAndVerify(clientId, subscriptionId); + sync(owner: string, subscriptionId: string, lastSequenceNumber?: number): I3xSyncItem[] { + const sub = this.getAndVerify(owner, subscriptionId); if (lastSequenceNumber !== undefined) { sub.queue = sub.queue.filter(item => item.sequenceNumber > lastSequenceNumber); @@ -151,8 +161,8 @@ export class SubscriptionManager { return [...sub.queue]; } - stream(clientId: string, subscriptionId: string, res: any): void { - const sub = this.getAndVerify(clientId, subscriptionId); + stream(owner: string, subscriptionId: string, res: any): void { + const sub = this.getAndVerify(owner, subscriptionId); if (sub.activeStream) { throw new Error(`Subscription ${subscriptionId} already has an active stream`); @@ -234,18 +244,24 @@ export class SubscriptionManager { } } - private getAndVerify(clientId: string, subscriptionId: string): Subscription { + /* Ownership is checked against the authenticated principal, not + * against the client-supplied clientId. A subscription owned by + * someone else reports 404, identically to one that does not + * exist, so that the pair cannot be used to probe which + * subscription ids are live. acs-directory does the same thing for + * alerts, deliberately, for the same reason. + * + * A falsy `owner` means the request reached us unauthenticated. + * That should be impossible — every subscription route sits behind + * FplusHttpAuth — but it fails closed here rather than matching a + * subscription stored with a falsy owner. */ + private getAndVerify(owner: string, subscriptionId: string): Subscription { const sub = this.subscriptions.get(subscriptionId); - if (!sub) { + if (!sub || !owner || sub.owner !== owner) { const err: any = new Error(`Subscription ${subscriptionId} not found`); err.status = 404; throw err; } - if (sub.clientId !== clientId) { - const err: any = new Error(`Subscription ${subscriptionId} does not belong to client ${clientId}`); - err.status = 403; - throw err; - } return sub; } diff --git a/acs-i3x/test/api-v1.test.ts b/acs-i3x/test/api-v1.test.ts index 086b95528..374ce61da 100644 --- a/acs-i3x/test/api-v1.test.ts +++ b/acs-i3x/test/api-v1.test.ts @@ -49,24 +49,29 @@ function mockHistory() { }; } +/* The authenticated principal the test app presents. Subscription + * ownership is keyed on this, not on the clientId in the request + * body. */ +const TEST_PRINCIPAL = "test-principal@REALM"; + function mockSubscriptions() { return { - create: jest.fn<(clientId: string, displayName?: string) => I3xSubscription>() + create: jest.fn<(owner: string, clientId: string, displayName?: string) => I3xSubscription>() .mockReturnValue({ clientId: "c1", subscriptionId: "sub-1", displayName: "Test", }), - list: jest.fn<(clientId: string, ids: string[]) => I3xSubscription[]>() + list: jest.fn<(owner: string, ids: string[]) => I3xSubscription[]>() .mockReturnValue([]), - getOne: jest.fn<(clientId: string, id: string) => I3xSubscription>(), - deleteOne: jest.fn<(clientId: string, id: string) => void>(), - register: jest.fn<(clientId: string, subId: string, ids: string[], maxDepth?: number) => void>(), - registerOne: jest.fn<(clientId: string, subId: string, id: string, maxDepth?: number) => void>(), - unregister: jest.fn<(clientId: string, subId: string, ids: string[]) => void>(), - unregisterOne: jest.fn<(clientId: string, subId: string, id: string) => void>(), - stream: jest.fn<(clientId: string, subId: string, res: any) => void>(), - sync: jest.fn<(clientId: string, subId: string, lastSeq?: number) => I3xSyncItem[]>() + getOne: jest.fn<(owner: string, id: string) => I3xSubscription>(), + deleteOne: jest.fn<(owner: string, id: string) => void>(), + register: jest.fn<(owner: string, subId: string, ids: string[], maxDepth?: number) => void>(), + registerOne: jest.fn<(owner: string, subId: string, id: string, maxDepth?: number) => void>(), + unregister: jest.fn<(owner: string, subId: string, ids: string[]) => void>(), + unregisterOne: jest.fn<(owner: string, subId: string, id: string) => void>(), + stream: jest.fn<(owner: string, subId: string, res: any) => void>(), + sync: jest.fn<(owner: string, subId: string, lastSeq?: number) => I3xSyncItem[]>() .mockReturnValue([]), }; } @@ -90,6 +95,9 @@ function createApp(opts: { maxDepthCap?: number } = {}) { const app = express(); app.use(express.json()); + /* Stand in for FplusHttpAuth, which sets req.auth to the verified + * principal on every non-public route. */ + app.use((req, _res, next) => { (req as any).auth = TEST_PRINCIPAL; next(); }); app.use("/", api.infoRoute); app.use("/", api.routes); @@ -710,7 +718,7 @@ describe("APIv1", () => { .send({ clientId: "c1", displayName: "Test" }); expect(res.status).toBe(200); - expect(subscriptions.create).toHaveBeenCalledWith("c1", "Test"); + expect(subscriptions.create).toHaveBeenCalledWith(TEST_PRINCIPAL, "c1", "Test"); expect(res.body.result).toEqual({ clientId: "c1", subscriptionId: "sub-1", @@ -722,8 +730,8 @@ describe("APIv1", () => { describe("POST /subscriptions/list", () => { it("returns bulk envelope with monitoredObjects per subscription", async () => { const { app, subscriptions } = createApp(); - subscriptions.getOne.mockImplementation((clientId: string, id: string) => ({ - clientId, + subscriptions.getOne.mockImplementation((_owner: string, id: string) => ({ + clientId: "c1", subscriptionId: id, displayName: `Sub ${id}`, monitoredObjects: [{ elementId: "obj-1", maxDepth: 2 }], @@ -734,8 +742,8 @@ describe("APIv1", () => { .send({ clientId: "c1", subscriptionIds: ["sub-1", "sub-2"] }); expect(res.status).toBe(200); - expect(subscriptions.getOne).toHaveBeenCalledWith("c1", "sub-1"); - expect(subscriptions.getOne).toHaveBeenCalledWith("c1", "sub-2"); + expect(subscriptions.getOne).toHaveBeenCalledWith(TEST_PRINCIPAL, "sub-1"); + expect(subscriptions.getOne).toHaveBeenCalledWith(TEST_PRINCIPAL, "sub-2"); expect(res.body).toEqual({ success: true, results: [ @@ -765,14 +773,14 @@ describe("APIv1", () => { it("reports missing subscriptions as 404 without aborting the batch", async () => { const { app, subscriptions } = createApp(); - subscriptions.getOne.mockImplementation((clientId: string, id: string) => { + subscriptions.getOne.mockImplementation((_owner: string, id: string) => { if (id === "missing") { const err: any = new Error(`Subscription ${id} not found`); err.status = 404; throw err; } return { - clientId, + clientId: "c1", subscriptionId: id, displayName: `Sub ${id}`, monitoredObjects: [], @@ -804,11 +812,14 @@ describe("APIv1", () => { ]); }); - it("reports wrong-client subscriptions as 403", async () => { + /* A subscription owned by a different principal is reported as + * 404, exactly like one that does not exist, so the pair cannot + * be used to discover which subscription ids are live. */ + it("reports another principal's subscriptions as 404, not 403", async () => { const { app, subscriptions } = createApp(); - subscriptions.getOne.mockImplementation((_clientId: string, _id: string) => { - const err: any = new Error("Subscription sub-1 does not belong to client c1"); - err.status = 403; + subscriptions.getOne.mockImplementation((_owner: string, _id: string) => { + const err: any = new Error("Subscription sub-1 not found"); + err.status = 404; throw err; }); @@ -823,11 +834,29 @@ describe("APIv1", () => { { success: false, subscriptionId: "sub-1", - error: { code: 403, message: "Subscription sub-1 does not belong to client c1" }, + error: { code: 404, message: "Subscription sub-1 not found" }, }, ], }); }); + + /* The clientId in the body is echoed but never trusted: it does + * not decide which subscriptions the caller can reach. */ + it("ignores the clientId in the body when resolving ownership", async () => { + const { app, subscriptions } = createApp(); + subscriptions.getOne.mockReturnValue({ + clientId: "someone-else", + subscriptionId: "sub-1", + displayName: "Sub sub-1", + }); + + await request(app) + .post("/subscriptions/list") + .send({ clientId: "someone-else", subscriptionIds: ["sub-1"] }); + + expect(subscriptions.getOne).toHaveBeenCalledWith(TEST_PRINCIPAL, "sub-1"); + expect(subscriptions.getOne).not.toHaveBeenCalledWith("someone-else", "sub-1"); + }); }); describe("POST /subscriptions/register", () => { @@ -849,8 +878,8 @@ describe("APIv1", () => { }); expect(res.status).toBe(200); - expect(subscriptions.registerOne).toHaveBeenCalledWith("c1", "sub-1", "obj-1", 2); - expect(subscriptions.registerOne).toHaveBeenCalledWith("c1", "sub-1", "obj-2", 2); + expect(subscriptions.registerOne).toHaveBeenCalledWith(TEST_PRINCIPAL, "sub-1", "obj-1", 2); + expect(subscriptions.registerOne).toHaveBeenCalledWith(TEST_PRINCIPAL, "sub-1", "obj-2", 2); expect(res.body).toEqual({ success: true, results: [ @@ -888,7 +917,7 @@ describe("APIv1", () => { { success: true, elementId: "obj-2", result: null }, ]); expect(subscriptions.registerOne).not.toHaveBeenCalledWith( - "c1", "sub-1", "missing", expect.anything(), + TEST_PRINCIPAL, "sub-1", "missing", expect.anything(), ); }); @@ -941,8 +970,8 @@ describe("APIv1", () => { }); expect(res.status).toBe(200); - expect(subscriptions.unregisterOne).toHaveBeenCalledWith("c1", "sub-1", "obj-1"); - expect(subscriptions.unregisterOne).toHaveBeenCalledWith("c1", "sub-1", "obj-2"); + expect(subscriptions.unregisterOne).toHaveBeenCalledWith(TEST_PRINCIPAL, "sub-1", "obj-1"); + expect(subscriptions.unregisterOne).toHaveBeenCalledWith(TEST_PRINCIPAL, "sub-1", "obj-2"); expect(res.body).toEqual({ success: true, results: [ @@ -980,7 +1009,7 @@ describe("APIv1", () => { { success: true, elementId: "obj-2", result: null }, ]); expect(subscriptions.unregisterOne).not.toHaveBeenCalledWith( - "c1", "sub-1", "missing", + TEST_PRINCIPAL, "sub-1", "missing", ); }); @@ -1085,8 +1114,8 @@ describe("APIv1", () => { .send({ clientId: "c1", subscriptionIds: ["sub-1", "sub-2"] }); expect(res.status).toBe(200); - expect(subscriptions.deleteOne).toHaveBeenCalledWith("c1", "sub-1"); - expect(subscriptions.deleteOne).toHaveBeenCalledWith("c1", "sub-2"); + expect(subscriptions.deleteOne).toHaveBeenCalledWith(TEST_PRINCIPAL, "sub-1"); + expect(subscriptions.deleteOne).toHaveBeenCalledWith(TEST_PRINCIPAL, "sub-2"); expect(res.body).toEqual({ success: true, results: [ diff --git a/acs-i3x/test/e2e.test.ts b/acs-i3x/test/e2e.test.ts index 017573e65..d4aded75c 100644 --- a/acs-i3x/test/e2e.test.ts +++ b/acs-i3x/test/e2e.test.ts @@ -187,6 +187,12 @@ const relationships = new Map>([ let readyFlag = true; +/* The authenticated principal the e2e app presents, and the clientId + * the requests carry. They are deliberately different strings: the + * principal owns the subscription, the clientId is only echoed. */ +const TEST_PRINCIPAL = "test-principal@REALM"; +const TEST_CLIENT_ID = "test-client"; + function createPreloadedMocks() { readyFlag = true; @@ -253,29 +259,29 @@ function createPreloadedMocks() { }; const subscriptions = { - create: jest.fn<(clientId: string, displayName?: string) => I3xSubscription>() - .mockImplementation((clientId: string, displayName?: string) => ({ + create: jest.fn<(owner: string, clientId: string, displayName?: string) => I3xSubscription>() + .mockImplementation((_owner: string, clientId: string, displayName?: string) => ({ clientId, subscriptionId: "sub-generated-1", displayName: displayName ?? "Default", })), - list: jest.fn<(clientId: string, ids: string[]) => I3xSubscription[]>() - .mockImplementation((clientId: string, ids: string[]) => + list: jest.fn<(owner: string, ids: string[]) => I3xSubscription[]>() + .mockImplementation((_owner: string, ids: string[]) => ids.map(id => ({ - clientId, + clientId: TEST_CLIENT_ID, subscriptionId: id, displayName: "Sub " + id, })), ), - getOne: jest.fn<(clientId: string, id: string) => I3xSubscription>() - .mockImplementation((clientId: string, id: string) => { + getOne: jest.fn<(owner: string, id: string) => I3xSubscription>() + .mockImplementation((_owner: string, id: string) => { if (id === "does-not-exist") { const err: any = new Error(`Subscription ${id} not found`); err.status = 404; throw err; } return { - clientId, + clientId: TEST_CLIENT_ID, subscriptionId: id, displayName: "Sub " + id, monitoredObjects: [ @@ -283,20 +289,20 @@ function createPreloadedMocks() { ], }; }), - deleteOne: jest.fn<(clientId: string, id: string) => void>() - .mockImplementation((_clientId: string, id: string) => { + deleteOne: jest.fn<(owner: string, id: string) => void>() + .mockImplementation((_owner: string, id: string) => { if (id === "does-not-exist") { const err: any = new Error(`Subscription ${id} not found`); err.status = 404; throw err; } }), - register: jest.fn<(clientId: string, subId: string, ids: string[], maxDepth?: number) => void>(), - registerOne: jest.fn<(clientId: string, subId: string, id: string, maxDepth?: number) => void>(), - unregister: jest.fn<(clientId: string, subId: string, ids: string[]) => void>(), - unregisterOne: jest.fn<(clientId: string, subId: string, id: string) => void>(), - stream: jest.fn<(clientId: string, subId: string, res: any) => void>(), - sync: jest.fn<(clientId: string, subId: string, lastSeq?: number) => I3xSyncItem[]>() + register: jest.fn<(owner: string, subId: string, ids: string[], maxDepth?: number) => void>(), + registerOne: jest.fn<(owner: string, subId: string, id: string, maxDepth?: number) => void>(), + unregister: jest.fn<(owner: string, subId: string, ids: string[]) => void>(), + unregisterOne: jest.fn<(owner: string, subId: string, id: string) => void>(), + stream: jest.fn<(owner: string, subId: string, res: any) => void>(), + sync: jest.fn<(owner: string, subId: string, lastSeq?: number) => I3xSyncItem[]>() .mockReturnValue([ { sequenceNumber: 1, elementId: "obj-cnc-1", ...vqtCnc1 }, { sequenceNumber: 2, elementId: "obj-robot-1", ...vqtRobot }, @@ -324,6 +330,10 @@ function createE2eApp(opts: { maxDepthCap?: number } = {}) { const app = express(); app.use(express.json()); + /* Stand in for FplusHttpAuth, which sets req.auth on every + * non-public route in the deployed service. */ + app.use((req, _res, next) => { (req as any).auth = TEST_PRINCIPAL; next(); }); + /* Mount exactly as the real routes.ts does */ app.use("/v1", api.infoRoute); app.use("/v1", api.routes); diff --git a/acs-i3x/test/subscriptions.test.ts b/acs-i3x/test/subscriptions.test.ts index 95d080076..1e0fe8b25 100644 --- a/acs-i3x/test/subscriptions.test.ts +++ b/acs-i3x/test/subscriptions.test.ts @@ -53,7 +53,7 @@ describe("SubscriptionManager", () => { describe("create", () => { it("returns subscription with clientId, subscriptionId, displayName", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); expect(sub.clientId).toBe("client-1"); expect(sub.subscriptionId).toBeDefined(); @@ -63,20 +63,20 @@ describe("SubscriptionManager", () => { }); it("uses provided displayName", () => { - const sub = mgr.create("client-1", "My Subscription"); + const sub = mgr.create("client-1", "client-1", "My Subscription"); expect(sub.displayName).toBe("My Subscription"); }); it("uses empty string when no displayName provided", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); expect(sub.displayName).toBe(""); }); it("creates two separate subscriptions for same clientId", () => { - const sub1 = mgr.create("client-1"); - const sub2 = mgr.create("client-1"); + const sub1 = mgr.create("client-1", "client-1"); + const sub2 = mgr.create("client-1", "client-1"); expect(sub1.subscriptionId).not.toBe(sub2.subscriptionId); }); @@ -86,8 +86,8 @@ describe("SubscriptionManager", () => { describe("list", () => { it("returns matching subscriptions", () => { - const sub1 = mgr.create("client-1", "Sub A"); - const sub2 = mgr.create("client-1", "Sub B"); + const sub1 = mgr.create("client-1", "client-1", "Sub A"); + const sub2 = mgr.create("client-1", "client-1", "Sub B"); const result = mgr.list("client-1", [sub1.subscriptionId, sub2.subscriptionId]); @@ -98,7 +98,7 @@ describe("SubscriptionManager", () => { }); it("returns empty for unknown subscriptionId", () => { - mgr.create("client-1"); + mgr.create("client-1", "client-1"); const result = mgr.list("client-1", ["nonexistent-id"]); @@ -106,7 +106,7 @@ describe("SubscriptionManager", () => { }); it("filters out subscriptions belonging to different clientId", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); const result = mgr.list("client-2", [sub.subscriptionId]); @@ -118,7 +118,7 @@ describe("SubscriptionManager", () => { describe("getOne", () => { it("returns subscription with empty monitoredObjects initially", () => { - const sub = mgr.create("client-1", "Sub A"); + const sub = mgr.create("client-1", "client-1", "Sub A"); const result = mgr.getOne("client-1", sub.subscriptionId); @@ -131,7 +131,7 @@ describe("SubscriptionManager", () => { }); it("includes registered elements with their maxDepth", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); mgr.register("client-1", sub.subscriptionId, ["elem-1", "elem-2"], 3); const result = mgr.getOne("client-1", sub.subscriptionId); @@ -146,7 +146,7 @@ describe("SubscriptionManager", () => { }); it("reflects different maxDepth per element when registered separately", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); mgr.registerOne("client-1", sub.subscriptionId, "elem-1", 1); mgr.registerOne("client-1", sub.subscriptionId, "elem-2", 5); @@ -169,13 +169,13 @@ describe("SubscriptionManager", () => { } }); - it("throws 403 for wrong clientId", () => { - const sub = mgr.create("client-1"); + it("throws 404 for a different owner", () => { + const sub = mgr.create("client-1", "client-1"); try { mgr.getOne("client-2", sub.subscriptionId); fail("expected getOne to throw"); } catch (err: any) { - expect(err.status).toBe(403); + expect(err.status).toBe(404); } }); }); @@ -184,7 +184,7 @@ describe("SubscriptionManager", () => { describe("deleteOne", () => { it("removes subscription", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); mgr.deleteOne("client-1", sub.subscriptionId); const result = mgr.list("client-1", [sub.subscriptionId]); @@ -201,13 +201,13 @@ describe("SubscriptionManager", () => { } }); - it("throws 403 when subscription belongs to different clientId", () => { - const sub = mgr.create("client-1"); + it("throws 404 when subscription belongs to a different owner", () => { + const sub = mgr.create("client-1", "client-1"); try { mgr.deleteOne("client-2", sub.subscriptionId); throw new Error("expected throw"); } catch (err: any) { - expect(err.status).toBe(403); + expect(err.status).toBe(404); } /* Subscription is untouched */ @@ -220,7 +220,7 @@ describe("SubscriptionManager", () => { describe("register", () => { it("adds elementIds to subscription", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); mgr.register("client-1", sub.subscriptionId, ["elem-1", "elem-2"]); // After registering, value changes for elem-1 should be captured @@ -236,7 +236,7 @@ describe("SubscriptionManager", () => { }); it("is idempotent re-registering same elementId", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); expect(() => { mgr.register("client-1", sub.subscriptionId, ["elem-1"]); @@ -244,14 +244,14 @@ describe("SubscriptionManager", () => { }).not.toThrow(); }); - it("throws 403 for wrong clientId", () => { - const sub = mgr.create("client-1"); + it("throws 404 for a different owner", () => { + const sub = mgr.create("client-1", "client-1"); try { mgr.register("client-2", sub.subscriptionId, ["elem-1"]); fail("expected register to throw"); } catch (err: any) { - expect(err.status).toBe(403); + expect(err.status).toBe(404); } }); @@ -269,7 +269,7 @@ describe("SubscriptionManager", () => { describe("registerOne", () => { it("adds a single elementId to subscription", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); mgr.registerOne("client-1", sub.subscriptionId, "elem-1"); const listener = valueCache.onValueChange.mock.calls[0][0] as ( @@ -292,13 +292,13 @@ describe("SubscriptionManager", () => { } }); - it("throws 403 for wrong clientId", () => { - const sub = mgr.create("client-1"); + it("throws 404 for a different owner", () => { + const sub = mgr.create("client-1", "client-1"); try { mgr.registerOne("client-2", sub.subscriptionId, "elem-1"); fail("expected registerOne to throw"); } catch (err: any) { - expect(err.status).toBe(403); + expect(err.status).toBe(404); } }); }); @@ -307,7 +307,7 @@ describe("SubscriptionManager", () => { describe("unregister", () => { it("removes elementIds from subscription", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); mgr.register("client-1", sub.subscriptionId, ["elem-1", "elem-2"]); mgr.unregister("client-1", sub.subscriptionId, ["elem-1"]); @@ -322,7 +322,7 @@ describe("SubscriptionManager", () => { }); it("still receives changes for remaining registered elements", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); mgr.register("client-1", sub.subscriptionId, ["elem-1", "elem-2"]); mgr.unregister("client-1", sub.subscriptionId, ["elem-1"]); @@ -346,13 +346,13 @@ describe("SubscriptionManager", () => { } }); - it("throws 403 for wrong clientId", () => { - const sub = mgr.create("client-1"); + it("throws 404 for a different owner", () => { + const sub = mgr.create("client-1", "client-1"); try { mgr.unregister("client-2", sub.subscriptionId, ["elem-1"]); fail("expected unregister to throw"); } catch (err: any) { - expect(err.status).toBe(403); + expect(err.status).toBe(404); } }); }); @@ -361,7 +361,7 @@ describe("SubscriptionManager", () => { describe("unregisterOne", () => { it("removes a single elementId from subscription", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); mgr.register("client-1", sub.subscriptionId, ["elem-1", "elem-2"]); mgr.unregisterOne("client-1", sub.subscriptionId, "elem-1"); @@ -384,13 +384,13 @@ describe("SubscriptionManager", () => { } }); - it("throws 403 for wrong clientId", () => { - const sub = mgr.create("client-1"); + it("throws 404 for a different owner", () => { + const sub = mgr.create("client-1", "client-1"); try { mgr.unregisterOne("client-2", sub.subscriptionId, "elem-1"); fail("expected unregisterOne to throw"); } catch (err: any) { - expect(err.status).toBe(403); + expect(err.status).toBe(404); } }); }); @@ -399,7 +399,7 @@ describe("SubscriptionManager", () => { describe("onValueChange", () => { it("queues item with correct sequenceNumber", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); mgr.register("client-1", sub.subscriptionId, ["elem-1"]); const listener = valueCache.onValueChange.mock.calls[0][0] as ( @@ -416,8 +416,8 @@ describe("SubscriptionManager", () => { }); it("only queues for subscriptions that have the elementId registered", () => { - const sub1 = mgr.create("client-1"); - const sub2 = mgr.create("client-1"); + const sub1 = mgr.create("client-1", "client-1"); + const sub2 = mgr.create("client-1", "client-1"); mgr.register("client-1", sub1.subscriptionId, ["elem-1"]); mgr.register("client-1", sub2.subscriptionId, ["elem-2"]); @@ -438,7 +438,7 @@ describe("SubscriptionManager", () => { describe("sync", () => { it("returns all queued items without lastSequenceNumber", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); mgr.register("client-1", sub.subscriptionId, ["elem-1"]); const listener = valueCache.onValueChange.mock.calls[0][0] as ( @@ -453,7 +453,7 @@ describe("SubscriptionManager", () => { }); it("removes acknowledged items when lastSequenceNumber provided", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); mgr.register("client-1", sub.subscriptionId, ["elem-1"]); const listener = valueCache.onValueChange.mock.calls[0][0] as ( @@ -471,14 +471,14 @@ describe("SubscriptionManager", () => { }); it("returns empty array on empty queue", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); const items = mgr.sync("client-1", sub.subscriptionId); expect(items).toHaveLength(0); }); it("sequence numbers are monotonically increasing", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); mgr.register("client-1", sub.subscriptionId, ["elem-1"]); const listener = valueCache.onValueChange.mock.calls[0][0] as ( @@ -495,14 +495,14 @@ describe("SubscriptionManager", () => { } }); - it("throws 403 for wrong clientId", () => { - const sub = mgr.create("client-1"); + it("throws 404 for a different owner", () => { + const sub = mgr.create("client-1", "client-1"); try { mgr.sync("client-2", sub.subscriptionId); fail("expected sync to throw"); } catch (err: any) { - expect(err.status).toBe(403); + expect(err.status).toBe(404); } }); @@ -520,7 +520,7 @@ describe("SubscriptionManager", () => { describe("stream", () => { it("sets SSE headers on response", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); const res = mockSseRes(); mgr.stream("client-1", sub.subscriptionId, res); @@ -532,7 +532,7 @@ describe("SubscriptionManager", () => { }); it("flushes queued items immediately", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); mgr.register("client-1", sub.subscriptionId, ["elem-1"]); const listener = valueCache.onValueChange.mock.calls[0][0] as ( @@ -554,7 +554,7 @@ describe("SubscriptionManager", () => { }); it("sends new items as they arrive", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); mgr.register("client-1", sub.subscriptionId, ["elem-1"]); const res = mockSseRes(); @@ -574,7 +574,7 @@ describe("SubscriptionManager", () => { }); it("throws error on second stream call (one stream per subscription)", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); const res1 = mockSseRes(); mgr.stream("client-1", sub.subscriptionId, res1); @@ -585,7 +585,7 @@ describe("SubscriptionManager", () => { }); it("clears activeStream on res close", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); const res = mockSseRes(); mgr.stream("client-1", sub.subscriptionId, res); @@ -605,7 +605,7 @@ describe("SubscriptionManager", () => { }); it("uses correct SSE format for each item", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); mgr.register("client-1", sub.subscriptionId, ["elem-1"]); const res = mockSseRes(); @@ -635,14 +635,14 @@ describe("SubscriptionManager", () => { } }); - it("throws 403 for wrong clientId", () => { - const sub = mgr.create("client-1"); + it("throws 404 for a different owner", () => { + const sub = mgr.create("client-1", "client-1"); const res = mockSseRes(); try { mgr.stream("client-2", sub.subscriptionId, res); fail("expected stream to throw"); } catch (err: any) { - expect(err.status).toBe(403); + expect(err.status).toBe(404); } }); }); @@ -651,7 +651,7 @@ describe("SubscriptionManager", () => { describe("TTL", () => { it("subscription expires after timeout", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); jest.advanceTimersByTime(TTL + 1); @@ -660,7 +660,7 @@ describe("SubscriptionManager", () => { }); it("access resets the timer", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); // Advance just under TTL jest.advanceTimersByTime(TTL - 1000); @@ -677,7 +677,7 @@ describe("SubscriptionManager", () => { }); it("subscription is gone after reset TTL elapses", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); // Advance just under TTL jest.advanceTimersByTime(TTL - 1000); @@ -693,7 +693,7 @@ describe("SubscriptionManager", () => { }); it("closes active stream on expiry", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); const res = mockSseRes(); mgr.stream("client-1", sub.subscriptionId, res); @@ -707,8 +707,8 @@ describe("SubscriptionManager", () => { describe("destroy", () => { it("cleans up all subscriptions and timers", () => { - const sub1 = mgr.create("client-1"); - const sub2 = mgr.create("client-2"); + const sub1 = mgr.create("client-1", "client-1"); + const sub2 = mgr.create("client-2", "client-2"); mgr.destroy(); @@ -725,7 +725,7 @@ describe("SubscriptionManager", () => { }); it("closes active streams on destroy", () => { - const sub = mgr.create("client-1"); + const sub = mgr.create("client-1", "client-1"); const res = mockSseRes(); mgr.stream("client-1", sub.subscriptionId, res); @@ -735,6 +735,133 @@ describe("SubscriptionManager", () => { }); }); + /* ---- ownership ---- */ + + describe("ownership", () => { + /* The two principals both call themselves the same thing on the + * wire. Under the old clientId-only check that was enough to + * take the subscription over. */ + const OWNER = "alice@REALM"; + const ATTACKER = "mallory@REALM"; + const CLIENT_ID = "shared-client-id"; + + function expect_not_found(fn: () => unknown, what: string) { + try { + fn(); + fail(`expected ${what} to throw`); + } catch (err: any) { + expect(err.status).toBe(404); + expect(err.message).toMatch(/not found/); + } + } + + it("does not let a caller claim a subscription by sending its clientId", () => { + const sub = mgr.create(OWNER, CLIENT_ID); + + /* Mallory knows the subscriptionId and the clientId. */ + expect_not_found(() => mgr.getOne(ATTACKER, sub.subscriptionId), "getOne"); + expect_not_found(() => mgr.sync(ATTACKER, sub.subscriptionId), "sync"); + expect_not_found( + () => mgr.stream(ATTACKER, sub.subscriptionId, mockSseRes()), "stream"); + expect_not_found( + () => mgr.register(ATTACKER, sub.subscriptionId, ["elem-1"]), "register"); + expect_not_found( + () => mgr.registerOne(ATTACKER, sub.subscriptionId, "elem-1"), "registerOne"); + expect_not_found( + () => mgr.unregister(ATTACKER, sub.subscriptionId, ["elem-1"]), "unregister"); + expect_not_found( + () => mgr.unregisterOne(ATTACKER, sub.subscriptionId, "elem-1"), "unregisterOne"); + expect_not_found(() => mgr.deleteOne(ATTACKER, sub.subscriptionId), "deleteOne"); + + /* Nothing was destroyed along the way. */ + expect(mgr.list(OWNER, [sub.subscriptionId])).toHaveLength(1); + }); + + it("reports a foreign subscription identically to an unknown one", () => { + const sub = mgr.create(OWNER, CLIENT_ID); + + const foreign = (() => { + try { mgr.getOne(ATTACKER, sub.subscriptionId); return null; } + catch (err: any) { return err; } + })(); + const unknown = (() => { + try { mgr.getOne(ATTACKER, "9d1e0e4e-0000-0000-0000-000000000000"); return null; } + catch (err: any) { return err; } + })(); + + expect(foreign.status).toBe(404); + expect(unknown.status).toBe(404); + /* Both messages are the plain "not found" form; neither + * leaks that the id exists or who owns it. */ + expect(foreign.message).toBe(`Subscription ${sub.subscriptionId} not found`); + expect(foreign.message).not.toMatch(new RegExp(OWNER)); + expect(foreign.message).not.toMatch(/belong/); + }); + + it("never reports 403 from any subscription operation", () => { + const sub = mgr.create(OWNER, CLIENT_ID); + const calls: Array<() => unknown> = [ + () => mgr.getOne(ATTACKER, sub.subscriptionId), + () => mgr.sync(ATTACKER, sub.subscriptionId), + () => mgr.deleteOne(ATTACKER, sub.subscriptionId), + () => mgr.getOne("", sub.subscriptionId), + () => mgr.getOne(undefined as any, sub.subscriptionId), + ]; + for (const call of calls) { + try { + call(); + fail("expected throw"); + } catch (err: any) { + expect(err.status).toBe(404); + } + } + }); + + it("keeps two principals which share a clientId apart", () => { + const a = mgr.create("alice@REALM", CLIENT_ID); + const b = mgr.create("bob@REALM", CLIENT_ID); + + expect(mgr.list("alice@REALM", [a.subscriptionId, b.subscriptionId])) + .toEqual([expect.objectContaining({ subscriptionId: a.subscriptionId })]); + expect(mgr.list("bob@REALM", [a.subscriptionId, b.subscriptionId])) + .toEqual([expect.objectContaining({ subscriptionId: b.subscriptionId })]); + }); + + it("lets one principal use two different clientIds", () => { + /* Two browser tabs, one login, different random clientIds. */ + const tab1 = mgr.create(OWNER, "tab-1"); + const tab2 = mgr.create(OWNER, "tab-2"); + + expect(mgr.getOne(OWNER, tab1.subscriptionId).clientId).toBe("tab-1"); + expect(mgr.getOne(OWNER, tab2.subscriptionId).clientId).toBe("tab-2"); + }); + + it("survives the full lifecycle for a single principal", () => { + const sub = mgr.create(OWNER, CLIENT_ID, "Lifecycle"); + expect(sub.clientId).toBe(CLIENT_ID); + + mgr.register(OWNER, sub.subscriptionId, ["elem-1"], 2); + expect(mgr.getOne(OWNER, sub.subscriptionId).monitoredObjects) + .toEqual([{ elementId: "elem-1", maxDepth: 2 }]); + + const listener = valueCache.onValueChange.mock.calls[0][0] as + (elementId: string, vqt: I3xVqt) => void; + listener("elem-1", makeVqt(7)); + + const items = mgr.sync(OWNER, sub.subscriptionId); + expect(items).toHaveLength(1); + expect(items[0].value).toBe(7); + + const res = mockSseRes(); + mgr.stream(OWNER, sub.subscriptionId, res); + listener("elem-1", makeVqt(8)); + expect(res.write).toHaveBeenCalled(); + + mgr.deleteOne(OWNER, sub.subscriptionId); + expect(mgr.list(OWNER, [sub.subscriptionId])).toHaveLength(0); + }); + }); + /* ---- constructor ---- */ describe("constructor", () => {