From 3cb9a24809a94a7d478d9ca606c608dd12327a5d Mon Sep 17 00:00:00 2001 From: Alex Godbehere Date: Thu, 6 Aug 2026 08:55:22 +0100 Subject: [PATCH 1/4] docs: correct the acs-i3x authorization claim in the OAuth guide The OAuth client guide cited acs-i3x as an example of calling back into the F+ auth service to authorize a user per object. It does not: no ACL check exists anywhere in the service. An integrator reading that would reasonably conclude a shared acs-i3x credential is scoped by F+ ACLs, when in fact it grants read access to the whole object tree. Describe what the service actually does - authenticates via Kerberos, opaque bearer or Keycloak JWT, and authorizes nothing - and drop the reference to an OAuth2 token-exchange flow the auth service does not implement. --- docs/auth/oauth-clients.md | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/docs/auth/oauth-clients.md b/docs/auth/oauth-clients.md index bb80fd904..79a8817e2 100644 --- a/docs/auth/oauth-clients.md +++ b/docs/auth/oauth-clients.md @@ -257,18 +257,44 @@ Use this when authorization is dynamic, fine-grained, or per-object 1. Reads `fp_principal_uuid` from the JWT. 2. Calls the F+ auth service (e.g. `GET /authz/acl`) on behalf of - the user. Either: - - mint a service-to-service token and ask "what can principal - `` do?" - acs-i3x is taking this approach, or - - support the OAuth2 token-exchange flow once the i3x shim work - lands (planned, not yet shipped). + the user, by minting a service-to-service token and asking "what + can principal `` do?". 3. Caches the answer for whatever TTL fits the app's freshness needs. +OAuth2 token exchange is not supported by the F+ auth service today, +so a service-to-service call is the only option for Option B. + Either option avoids the duplicate-source-of-truth problem that existed before this branch (where Keycloak roles and F+ ACLs had to be administered separately). +#### What acs-i3x does today + +`acs-i3x` is **not** an example of either option. It authenticates +callers and then serves them the whole object tree. + +Authentication is the shared `FplusHttpAuth` middleware from +`@amrc-factoryplus/service-api`, so it accepts: + +- Kerberos, via `Negotiate` or via `Basic` with a UPN and password; +- an opaque bearer token obtained from `POST /token`; +- a Keycloak-issued JWT carrying `fp_principal_uuid`. + +`GET /v1/info` is public; every other endpoint requires one of the +above. + +There is currently **no authorization**. `acs-i3x` performs no ACL +check and does not consult the F+ auth service about the calling +principal, so any principal that authenticates can read every object, +value, history series and subscription the service exposes. Per-object +access control is not yet implemented. + +The practical consequence: a credential handed to an acs-i3x consumer +is not scoped by Factory+ ACLs. Treat it as granting read access to +the whole tree, and scope it by deciding who gets the credential at +all. + ## Worked example: minimal Express + openid-client For a Node.js application using From bba13bb242397d47ad93286fbb2f4f57d3126445 Mon Sep 17 00:00:00 2001 From: Alex Godbehere Date: Thu, 6 Aug 2026 08:55:26 +0100 Subject: [PATCH 2/4] deploy: set ROOT_PRINCIPAL on the i3x deployment Every other ACS service that performs ACL checks gets ROOT_PRINCIPAL; the service-client root bypass in fetch_acl is gated on it. i3x was missing it, so the first ACL check added to the service would lock the site administrator out. Add it ahead of enforcement landing, using the same admin@ form as data-access. --- deploy/templates/i3x/i3x.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/deploy/templates/i3x/i3x.yaml b/deploy/templates/i3x/i3x.yaml index efa8ab3cc..a378894e0 100644 --- a/deploy/templates/i3x/i3x.yaml +++ b/deploy/templates/i3x/i3x.yaml @@ -47,6 +47,8 @@ spec: value: i3x.{{ .Release.Namespace }}.svc.cluster.local - name: REALM value: {{ .Values.identity.realm | required "values.identity.realm is required!" }} + - name: ROOT_PRINCIPAL + value: admin@{{ .Values.identity.realm | required "values.identity.realm is required!" }} {{ include "amrc-connectivity-stack.oidc-env" . | indent 12 }} - name: SPARKPLUG_ADDRESS value: {{ .Values.acs.organisation | required "values.acs.organisation is required!" }}-Service-Core/i3X From c761274a2b1146df45a5d7c7910630126b46c244 Mon Sep 17 00:00:00 2001 From: Alex Godbehere Date: Thu, 6 Aug 2026 08:55:45 +0100 Subject: [PATCH 3/4] i3x: log value lookups through the request logger, not console.log The value endpoints printed lookup traces with raw console.log, and the messages included the metric values themselves. That puts real plant data into cluster logs, and console.log bypasses the buffered per-request logger WebAPI installs on req.log - which is where the authenticated principal for the request is recorded - so the values landed with no identity attached. Route these through req.log where present, falling back to a fplus.debug.bound("api-v1") module logger, and drop the value payloads. The element id, cache hit/miss and source timestamp are kept, which is what the messages were actually useful for. --- acs-i3x/bin/api.ts | 1 + acs-i3x/lib/api-v1.ts | 40 ++++++++++++++++++++++++++++++++-------- acs-i3x/lib/routes.ts | 1 + 3 files changed, 34 insertions(+), 8 deletions(-) diff --git a/acs-i3x/bin/api.ts b/acs-i3x/bin/api.ts index 3a0623416..097a1216c 100644 --- a/acs-i3x/bin/api.ts +++ b/acs-i3x/bin/api.ts @@ -83,6 +83,7 @@ const api = await new WebAPI({ subscriptions, mcpServer, maxDepthCap: parseInt(env.I3X_MAX_DEPTH_CAP || "0"), + debug: fplus.debug, }), }).init(); diff --git a/acs-i3x/lib/api-v1.ts b/acs-i3x/lib/api-v1.ts index 67a2be77d..a38dcd9e7 100644 --- a/acs-i3x/lib/api-v1.ts +++ b/acs-i3x/lib/api-v1.ts @@ -27,6 +27,11 @@ interface APIv1Opts { * clamped and the response is returned with HTTP 206. */ maxDepthCap?: number; + /** + * The service-client Debug object. Used to build the module + * logger; omitted in tests, where logging is silenced. + */ + debug?: any; } /** @@ -68,6 +73,7 @@ export class APIv1 { private history: History; private subscriptions: SubscriptionManager; private maxDepthCap: number; + private log: (msg: string, ...args: any[]) => void; /** * Stores the collaborator services and builds both routers. @@ -78,6 +84,7 @@ export class APIv1 { this.history = opts.history; this.subscriptions = opts.subscriptions; this.maxDepthCap = opts.maxDepthCap ?? 0; + this.log = opts.debug?.bound("api-v1") ?? (() => {}); this.routes = Router(); this.infoRoute = Router(); @@ -85,6 +92,20 @@ export class APIv1 { this.setup_routes(); } + /** + * Logs a message against a request. WebAPI installs a buffered + * per-request logger on `req.log`, and that buffer is where the + * authenticated principal for the request is recorded, so we + * prefer it: messages logged this way are attributable. Falls + * back to the module logger when there is no request logger (the + * unit tests mount the routers on a bare Express app). + */ + private log_req(req: Request, msg: string, ...args: any[]): void { + const rlog = (req as any).log; + if (rlog) rlog(msg, ...args); + else this.log(msg, ...args); + } + /** * Applies the server-imposed `maxDepthCap` to a client's requested * depth. Returns the effective depth to use and whether the @@ -311,19 +332,20 @@ export class APIv1 { // Try UNS cache first (real-time), fall back to InfluxDB last() const cached = this.valueCache.getValue(id); if (cached) { - console.log(`[VALUE] ${id.slice(0,16)} → UNS cache hit: value=${JSON.stringify(cached.value)} age=${Date.now() - new Date(cached.timestamp).getTime()}ms`); + this.log_req(req, "value %s: UNS cache hit, age %dms", + id, Date.now() - new Date(cached.timestamp).getTime()); return { success: true, elementId: id, result: cached }; } - console.log(`[VALUE] ${id.slice(0,16)} → UNS cache miss, querying InfluxDB...`); + this.log_req(req, "value %s: UNS cache miss, querying InfluxDB", id); const obj = this.objectTree.getObject(id); const item = obj?.isComposition ? await this.history.getCompositionValue(id, effective) : await this.history.getCurrentValue(id); if (item) { - console.log(`[VALUE] ${id.slice(0,16)} → InfluxDB hit: value=${JSON.stringify(item.value)} ts=${item.timestamp}`); + this.log_req(req, "value %s: InfluxDB hit, ts %s", id, item.timestamp); return { success: true, elementId: id, result: item }; } - console.log(`[VALUE] ${id.slice(0,16)} → no data`); + this.log_req(req, "value %s: no data", id); return { success: false, elementId: id, error: { code: 404, message: `No value for ${id}` } }; })); if (clamped) res.status(206); @@ -390,18 +412,20 @@ export class APIv1 { // Try UNS cache first (real-time), fall back to InfluxDB last() const cached = this.valueCache.getValue(id); if (cached) { - console.log(`[VALUE] ${id} → UNS cache hit: value=${JSON.stringify(cached.value)} ts=${cached.timestamp} age=${Date.now() - new Date(cached.timestamp).getTime()}ms`); + this.log_req(req, "value %s: UNS cache hit, ts %s, age %dms", + id, cached.timestamp, + Date.now() - new Date(cached.timestamp).getTime()); res.json(cached); return; } - console.log(`[VALUE] ${id} → UNS cache miss, querying InfluxDB...`); + this.log_req(req, "value %s: UNS cache miss, querying InfluxDB", id); const result = obj?.isComposition ? await this.history.getCompositionValue(id) : await this.history.getCurrentValue(id); if (result) { - console.log(`[VALUE] ${id} → InfluxDB hit: value=${JSON.stringify(result.value)} ts=${result.timestamp}`); + this.log_req(req, "value %s: InfluxDB hit, ts %s", id, result.timestamp); } else { - console.log(`[VALUE] ${id} → InfluxDB miss: no data`); + this.log_req(req, "value %s: InfluxDB miss, no data", id); } if (!result) return next(notFound(`No value for ${id}`)); res.json(result); diff --git a/acs-i3x/lib/routes.ts b/acs-i3x/lib/routes.ts index 0a51f9c04..ad7ff688c 100644 --- a/acs-i3x/lib/routes.ts +++ b/acs-i3x/lib/routes.ts @@ -14,6 +14,7 @@ export function routes(opts: { subscriptions: SubscriptionManager; mcpServer?: McpServer; maxDepthCap?: number; + debug?: any; }) { const api = new APIv1(opts); From 08726a22f84ca6160e1c6f7761cef7f7e152cfa3 Mon Sep 17 00:00:00 2001 From: Alex Godbehere Date: Thu, 6 Aug 2026 08:56:10 +0100 Subject: [PATCH 4/4] i3x: wrap GET /v1/info in the i3X response envelope Every other non-bulk endpoint returns { success, result }; /info returned the bare ServerInfo object because the envelope middleware was attached to the main router only and the info route is mounted separately so it can bypass auth and the readiness gate. The i3X OpenAPI spec types the 200 response of GET /info as SuccessResponse_ServerInfo_, so the envelope is correct and the service was wrong; acs-admin's useI3xClient, which unwraps body.result and was getting undefined for this route, needs no change. The middleware is attached to the route rather than the router: both routers are mounted on /v1, so a router-level use() would double-wrap every other endpoint. Compliance test now includes /v1/info in the "every success response is enveloped" sweep. --- acs-i3x/lib/api-v1.ts | 12 +++++++++++- acs-i3x/test/api-v1.test.ts | 23 ++++++++++++++--------- acs-i3x/test/e2e.test.ts | 18 ++++++++++-------- 3 files changed, 35 insertions(+), 18 deletions(-) diff --git a/acs-i3x/lib/api-v1.ts b/acs-i3x/lib/api-v1.ts index a38dcd9e7..3a96562ba 100644 --- a/acs-i3x/lib/api-v1.ts +++ b/acs-i3x/lib/api-v1.ts @@ -127,7 +127,17 @@ export class APIv1 { setup_routes() { /* ---- /info router (unauthenticated) ---- */ - this.infoRoute.get("/info", this.get_info.bind(this)); + /* The i3X OpenAPI spec types the 200 response of GET /info as + * SuccessResponse_ServerInfo_, i.e. { success, result }, the + * same as every other non-bulk endpoint. Only the router is + * separate here (so /info can bypass auth and the readiness + * gate); the envelope still applies. + * + * The middleware is attached to the route rather than via + * `infoRoute.use()`: both routers are mounted on the same /v1 + * path, so a router-level `use()` here would also run for every + * request destined for the main router and wrap it twice. */ + this.infoRoute.get("/info", i3xEnvelope, this.get_info.bind(this)); /* ---- Main router (authenticated) ---- */ diff --git a/acs-i3x/test/api-v1.test.ts b/acs-i3x/test/api-v1.test.ts index 086b95528..01df763bc 100644 --- a/acs-i3x/test/api-v1.test.ts +++ b/acs-i3x/test/api-v1.test.ts @@ -167,13 +167,16 @@ describe("APIv1", () => { expect(res.status).toBe(200); expect(res.body).toEqual({ - specVersion: I3X_SPEC_VERSION, - serverName: "AMRC Connectivity Stack", - serverVersion: Version, - capabilities: { - query: { history: true }, - update: { current: false, history: false }, - subscribe: { stream: true }, + success: true, + result: { + specVersion: I3X_SPEC_VERSION, + serverName: "AMRC Connectivity Stack", + serverVersion: Version, + capabilities: { + query: { history: true }, + update: { current: false, history: false }, + subscribe: { stream: true }, + }, }, }); }); @@ -183,7 +186,8 @@ describe("APIv1", () => { const res = await request(app).get("/info"); expect(res.status).toBe(200); - expect(res.body.capabilities.query).toEqual({ + expect(res.body.success).toBe(true); + expect(res.body.result.capabilities.query).toEqual({ history: true, maxDepthCap: 5, }); @@ -193,7 +197,7 @@ describe("APIv1", () => { const { app } = createApp(); const res = await request(app).get("/info"); - expect(res.body.capabilities.query).not.toHaveProperty("maxDepthCap"); + expect(res.body.result.capabilities.query).not.toHaveProperty("maxDepthCap"); }); }); @@ -217,6 +221,7 @@ describe("APIv1", () => { const res = await request(app).get("/info"); expect(res.status).toBe(200); + expect(res.body.success).toBe(true); }); }); diff --git a/acs-i3x/test/e2e.test.ts b/acs-i3x/test/e2e.test.ts index 017573e65..4590b3eff 100644 --- a/acs-i3x/test/e2e.test.ts +++ b/acs-i3x/test/e2e.test.ts @@ -347,7 +347,8 @@ describe("E2E Compliance Tests", () => { expect(res.status).toBe(200); expect(res.headers["content-type"]).toMatch(/application\/json/); - const info = res.body; + expect(res.body).toHaveProperty("success", true); + const info = res.body.result; expect(info).toHaveProperty("specVersion"); expect(info).toHaveProperty("serverName"); expect(info).toHaveProperty("serverVersion"); @@ -362,7 +363,7 @@ describe("E2E Compliance Tests", () => { const { app } = createE2eApp(); const res = await request(app).get("/v1/info"); - const caps = res.body.capabilities; + const caps = res.body.result.capabilities; expect(caps.query.history).toBe(true); expect(caps.update.current).toBe(false); expect(caps.subscribe.stream).toBe(true); @@ -372,8 +373,8 @@ describe("E2E Compliance Tests", () => { const { app } = createE2eApp(); const res = await request(app).get("/v1/info"); - expect(res.body.specVersion).toBe(I3X_SPEC_VERSION); - expect(res.body.serverVersion).toBe(Version); + expect(res.body.result.specVersion).toBe(I3X_SPEC_VERSION); + expect(res.body.result.serverVersion).toBe(Version); }); }); @@ -785,7 +786,8 @@ describe("E2E Compliance Tests", () => { const res = await request(app).get("/v1/info"); expect(res.status).toBe(200); - expect(res.body.capabilities.query).toEqual({ + expect(res.body.success).toBe(true); + expect(res.body.result.capabilities.query).toEqual({ history: true, maxDepthCap: 4, }); @@ -1155,9 +1157,8 @@ describe("E2E Compliance Tests", () => { it("every success response has { success: true, result: ... }", async () => { const { app } = createE2eApp(); - /* /v1/info is excluded: the info route does not use the - envelope middleware and returns raw data. */ const responses = await Promise.all([ + request(app).get("/v1/info"), request(app).get("/v1/namespaces"), request(app).get("/v1/objecttypes"), request(app).get("/v1/objecttypes/type-cnc"), @@ -1268,7 +1269,8 @@ describe("E2E Compliance Tests", () => { const res = await request(app).get("/v1/info"); expect(res.status).toBe(200); - expect(res.body).toHaveProperty("specVersion"); + expect(res.body).toHaveProperty("success", true); + expect(res.body.result).toHaveProperty("specVersion"); }); });