Skip to content

i3x: bind subscription ownership to the authenticated principal #711

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AlexGodbehere wants to merge 2 commits into
base: main
Choose a base branch
from
Open

i3x: bind subscription ownership to the authenticated principal #711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3526586
i3x: own subscriptions by principal, not by client-supplied clientId
AlexGodbehere Aug 6, 2026
a03b7a6
i3x: test subscription ownership against the authenticated principal
AlexGodbehere Aug 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 53 additions & 27 deletions acs-i3x/lib/api-v1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -550,23 +571,28 @@ export class APIv1 {
* do NOT call `res.json` here.
*/
async stream_subscription(req: Request, res: Response, _next: NextFunction): Promise<void> {
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));
}
}
68 changes: 42 additions & 26 deletions acs-i3x/lib/subscriptions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 ?? "",
Expand All @@ -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,
Expand All @@ -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 }));
Expand All @@ -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) {
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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`);
Expand Down Expand Up @@ -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;
}

Expand Down
Loading
Loading