acs-i3x: clear the ground before adding access control by AlexGodbehere · Pull Request #710 · AMRC-FactoryPlus/amrc-connectivity-stack

AlexGodbehere · 2026-08-06T07:58:03Z

Four preparatory fixes ahead of adding access control to acs-i3x. No enforcement is added here - the service still authorizes nothing. This clears the ground so the enforcement PR is a single, reviewable change.

1. The OAuth guide described authorization that does not exist

docs/auth/oauth-clients.md cited acs-i3x as an example of "Option B - call back into Factory+ with the user's identity", specifically minting a service-to-service token and asking "what can principal <uuid> do?".

That is false. grep -rn "check_acl\|fetch_acl\|req.auth" acs-i3x/lib acs-i3x/bin returns nothing. This is the harmful direction to be wrong in: it would lead an integrator to assume a shared acs-i3x credential is scoped by Factory+ ACLs when it grants read access to the entire object tree.

Replaced with a "What acs-i3x does today" subsection that states what the service actually does:

authenticates via the shared FplusHttpAuth middleware - Kerberos (Negotiate, or Basic with UPN + password), an opaque bearer from POST /token, or a Keycloak JWT carrying fp_principal_uuid;
GET /v1/info is public, everything else requires auth;
performs no ACL check and never consults the auth service about the caller. Per-object access control is not yet implemented.

No date promised, and no overclaiming the other way - the service does authenticate.

Other claim fixed in the same file: the guide offered OAuth2 token exchange as an alternative under Option B, "once the i3x shim work lands". The F+ auth service does not implement token exchange today, so that has been replaced with a plain statement that a service-to-service call is the only option.

2. `ROOT_PRINCIPAL` on the i3x deployment

deploy/templates/i3x/i3x.yaml was the only ACL-relevant service without it. Added admin@{{ .Values.identity.realm }}, copying deploy/templates/data-access/data-access.yaml exactly.

The point worth not losing: the client-side root bypass in lib/js-service-client/lib/service/auth.js (fetch_acl, ~line 141) is Kerberos-UPN-only:

if (this.root_principal
    && type == "kerberos"
    && principal == this.root_principal
) {

FplusHttpAuth.auth_jwt sets req.auth to the fp_principal_uuid claim, which decodes as a uuid-type principal, not kerberos. So a Keycloak-authenticated administrator can never hit the root bypass, no matter what ROOT_PRINCIPAL is set to. Once enforcement lands, the admin either authenticates with Kerberos to get root, or needs an explicit ACL grant against their principal UUID. This should be decided deliberately in the enforcement PR rather than discovered in the field.

3. Metric values in stdout at info level

acs-i3x/lib/api-v1.ts printed value-lookup traces with raw console.log, including the metric values themselves - real plant data landing in cluster logs.

Two fixes:

Values removed from the messages. Kept element id, cache hit/miss, source, and the source timestamp, which is what these were actually useful for when debugging a stale reading.
Routed through the request logger. WebAPI installs a buffered per-request logger on req.log (lib/js-service-api/lib/webapi.js ~54-67), and that buffer is where FplusHttpAuth records the authenticated principal. console.log bypasses it entirely, so the messages were unattributable. A small log_req helper prefers req.log and falls back to a house-pattern opts.fplus.debug.bound("api-v1") module logger, which is what the unit tests hit (they mount the routers on a bare Express app with no req.log).

debug is plumbed bin/api.ts -> routes() -> APIv1 as fplus.debug, matching how ObjectTree already takes its logger.

4. `GET /v1/info` envelope - reasoning

Conclusion: the spec requires the envelope. The bug was in acs-i3x, not acs-admin.

acs-admin/src/composables/useI3xClient.js unwraps body.result for every call including getInfo(), and was getting undefined. Two candidate fixes; I checked rather than assumed.

The evidence:

The in-repo notes are ambiguous. acs-i3x/docs/design.md says "Express middleware wraps all responses in the i3X envelope" and pitch.md item 10 says "All responses wrapped in { success: true, result: ... }" - but pitch.md item 2 shows the GET /info example as a bare object, which is presumably where the implementation took its cue.
The authority is the i3X OpenAPI spec (https://api.i3x.dev/v0/openapi.json). It types the 200 response of GET /info as SuccessResponse_ServerInfo_, i.e. { success, result: ServerInfo } - the same SuccessResponse wrapper used by /namespaces, /objecttypes, /objects/{id}/value and the rest. The only genuinely un-enveloped responses in the spec are PUT /objects/{id}/history and POST /subscriptions/stream, both of which return {}.

So /info is enveloped like everything else. The separate infoRoute exists only so /info can bypass auth and the readiness gate, which is orthogonal to the response shape - the envelope should always have applied to it.

One implementation detail worth a look during review: the middleware is attached to the route, not via infoRoute.use(). Both routers are mounted on /v1, so a router-level use() on infoRoute runs for every request destined for the main router too and double-wraps them. I hit exactly this - 91 test failures - before switching to the per-route form.

No change to acs-admin; its unwrapping was already correct and now gets a defined value.

Test results

Full acs-i3x suite, after the change:

Test Suites: 13 passed, 13 total
Tests:       342 passed, 342 total
Snapshots:   0 total
Time:        3.517 s

Baseline on the unmodified tree was also 13/13 and 342/342, so nothing was already failing and nothing regressed. npx tsc --noEmit is clean.

Test changes: test/api-v1.test.ts and test/e2e.test.ts updated for the new /info shape, and /v1/info added to the "every success response has { success: true, result }" compliance sweep in e2e.test.ts - it was previously excluded with a comment explaining that the info route returns raw data. That exclusion is now gone.

acs-i3x/dist/ is gitignored (root .gitignore, dist/) and not tracked, so no rebuild was needed.

Helm

helm lint and helm template both pass and the deployment renders ROOT_PRINCIPAL: admin@EXAMPLE.COM. Rendered from a scratch copy of the chart with the dependencies: block stripped, because the subcharts (traefik, grafana, influxdb2, cert-manager) are not vendored into deploy/charts/. No command was run against a live cluster.

Noticed, not fixed

acs-i3x/lib/subscriptions.ts has the same problem as item 3: seven raw console.log calls, and the one at line 216 prints value=${JSON.stringify(vqt.value)} for every SSE frame - i.e. plant data on the hot path, at higher volume than the ones fixed here. Left alone because another agent is working in that file.
acs-i3x/docs/design.md (~line 262) claims "ACL checks via Auth.check_acl() per request" under Authentication. Same false claim as item 1, in the service's own design doc. Out of scope for this PR, which was scoped to docs/auth/oauth-clients.md, but it should be corrected by whoever lands enforcement - by which point it may become true.
deploy/templates/i3x/i3x.yaml hardcodes VERBOSE: ALL, unlike siblings which template it from a verbosity value. With ALL every debug tag prints, so the new api-v1 logger will be verbose in production. Worth templating, but it is a values-schema change and did not belong in this PR.

How to test

cd acs-i3x && npm install && npm test - expect 13 suites, 342 tests, all passing.
npx tsc --noEmit - expect no output.
Envelope, by hand: start the service and curl -s http://i3x.<baseUrl>/v1/info | jq. Expect { "success": true, "result": { "specVersion": ..., "serverName": ..., "capabilities": {...} } }, not a bare object.
Confirm nothing else double-wrapped: curl -s -u <user> http://i3x.<baseUrl>/v1/namespaces | jq - expect { success: true, result: [...] } with result an array, not result.result.
acs-admin: open the i3X tree view. getInfo() now resolves to the server info object instead of undefined.
Logging: request a value (GET /v1/objects/<id>/value) and check the pod logs. Expect lines like value <elementId>: UNS cache hit, ts ..., age 1234ms, grouped with the >>> GET ... / Auth succeeded for [...] lines for that request. Expect no metric values in the output.
Deployment: helm template the chart and confirm the i3x Deployment carries ROOT_PRINCIPAL: admin@<your realm>.
Docs: read the "What acs-i3x does today" subsection in docs/auth/oauth-clients.md and check it matches your reading of the code.

Release notes

acs-i3x: GET /v1/info now returns the i3X success envelope. The endpoint previously returned the bare server-info object while every other endpoint returned { success, result }. This is a breaking change for any client that read specVersion and friends off the top level of the response; read them from .result instead. The ACS admin UI already expected the enveloped form and was receiving undefined from this route.
acs-i3x: metric values are no longer written to the service log. Value lookups were logged with the process value included. They now log the element id, cache hit/miss and source timestamp only, and go through the standard per-request logger so entries are attributable to the authenticated caller.
acs-i3x: ROOT_PRINCIPAL is now set on the deployment, matching every other ACL-checking ACS service. No behavioural change today, since acs-i3x performs no ACL checks; this prepares for access control being added.
Documentation: the OAuth client integration guide previously stated that acs-i3x authorizes callers against Factory+ ACLs. It does not. The guide now states plainly that acs-i3x authenticates callers but does not authorize them, and that any principal that authenticates can read the whole object tree. If you have issued an acs-i3x credential on the assumption that F+ ACLs scoped it, review who holds it.

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.

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@<realm> form as data-access.

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.

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.

AlexGodbehere · 2026-08-06T07:58:53Z

Review: four preparatory fixes before access control lands in acs-i3x

Branch: agent-ab5cd3ada6fbd7da0
Worktree: .claude/worktrees/agent-ab5cd3ada6fbd7da0
PR: #710

What changed (functional level)

acs-i3x performs no authorization. Once FplusHttpAuth admits a request, that principal can read the whole object tree. This PR does not change that - it removes four things that would otherwise get in the way of, or be made worse by, adding enforcement.

The published integration guide claimed the opposite of the truth, saying acs-i3x asks the auth service "what can principal <uuid> do?". That claim is now replaced with an accurate description: it authenticates (Kerberos, opaque bearer, or Keycloak JWT), GET /v1/info is public, and there is no per-object access control.

ROOT_PRINCIPAL is now set on the i3x deployment, as it is on every other ACL-checking service, so the first ACL check added does not lock the administrator out of the service they are debugging.

Value lookups no longer print process values to stdout, and now log through the per-request buffer where the caller's identity is recorded.

GET /v1/info now returns { success, result } like every other endpoint.

Walkthrough

Docs: what acs-i3x actually does

The "Option B" example in docs/auth/oauth-clients.md no longer names acs-i3x as an implementation of ACL callback. It isn't one.
A new "What acs-i3x does today" subsection lists the three accepted credential types and states plainly that no ACL check happens, with the practical consequence spelled out: a credential handed to an acs-i3x consumer grants read access to the whole tree and is scoped only by who you give it to.
Also removed from the same file: the promise of an OAuth2 token-exchange flow "once the i3x shim work lands". The auth service does not implement token exchange, so the guide now says a service-to-service call is the only route.
No date promised for enforcement, and no swing to the other extreme - the service does authenticate, and the doc says so.

Deployment: ROOT_PRINCIPAL

deploy/templates/i3x/i3x.yaml now sets ROOT_PRINCIPAL: admin@{{ .Values.identity.realm }}, sourced the same way data-access does.
No behavioural change today - nothing in acs-i3x reads it yet. It is deployed now so it is already in place when enforcement arrives.
The flag worth carrying into the enforcement PR: the root bypass in lib/js-service-client/lib/service/auth.js is gated on type == "kerberos". A Keycloak-authenticated admin's principal is a UUID, not a Kerberos UPN, so they can never be root via this path and will need an explicit grant.

Logging: no more plant data on stdout

The seven console.log calls in the value paths of api-v1.ts are gone. The messages that printed value=${JSON.stringify(...)} now print element id, cache hit/miss, and source timestamp only.
They go through req.log when present. That is WebAPI's buffered per-request logger, the same buffer FplusHttpAuth writes "Auth succeeded for [...]" into, so the lines are now attributable to a principal.
Fallback is a house-pattern fplus.debug.bound("api-v1") logger, which is what the unit tests exercise since they mount the routers bare.
subscriptions.ts has the same problem, including a per-SSE-frame value dump. Left alone - another agent is in that file. Flagged in the PR body.

Envelope on /v1/info

GET /v1/info now returns { success: true, result: { specVersion, serverName, serverVersion, capabilities } }.
I checked the i3X OpenAPI spec rather than guessing which side was wrong: it types the 200 response as SuccessResponse_ServerInfo_, the same wrapper as /namespaces and friends. The only un-enveloped 200s in the spec are PUT /objects/{id}/history and POST /subscriptions/stream, both returning {}. So acs-i3x was wrong, acs-admin was right, and acs-admin is untouched.
The middleware is attached to the route, not the router. Both routers mount on /v1, so infoRoute.use(i3xEnvelope) also runs for every main-router request and double-wraps it. That produced 91 failures before I switched to the per-route form; worth a second pair of eyes.
This is a breaking change for any client reading specVersion off the top level of the response.

How to test

cd acs-i3x && npm install && npm test - expect 13 suites, 342 tests, all passing.
npx tsc --noEmit - expect no output.
curl -s http://i3x.<baseUrl>/v1/info | jq - expect { "success": true, "result": { ... } }, not a bare object.
curl -s -u <user> http://i3x.<baseUrl>/v1/namespaces | jq - expect result to be an array, not result.result. This is the double-wrap check.
Open the i3X tree view in acs-admin. getInfo() resolves to the server info object instead of undefined.
GET /v1/objects/<id>/value, then read the pod log. Expect value <elementId>: UNS cache hit, ts ..., age 1234ms grouped with that request's >>> GET and Auth succeeded for [...] lines, and no metric values anywhere.
helm template the chart; confirm the i3x Deployment carries ROOT_PRINCIPAL: admin@<realm>.
Read the "What acs-i3x does today" subsection in docs/auth/oauth-clients.md against the code and check it is accurate.

Decisions / open questions

Item 4 could have gone either way and I resolved it on the OpenAPI spec, not on the in-repo notes, which conflict: design.md and pitch.md item 10 say all responses are enveloped, but pitch.md item 2 shows /info bare - probably where the implementation took its cue. If anyone has a newer 1.0-branch spec that disagrees, say so and I will revert this item.
Kerberos-only root is a real decision, not a detail. Setting ROOT_PRINCIPAL does not give a Keycloak-authenticated admin root. Whoever writes the enforcement PR must decide whether to extend the bypass, or to require Kerberos for admin access, or to grant the admin's principal UUID explicitly. Doing nothing means the admin gets locked out through the UI.
Verbosity. The new logger uses the api-v1 debug tag, but the i3x deployment hardcodes VERBOSE: ALL, so it prints regardless. Templating that from a values.i3x.verbosity like the sibling services is worth doing and is a values-schema change, so it is not in this PR.
Scope held deliberately. No ACL check is added anywhere. history.ts, subscriptions.ts and the rest of api-v1.ts are untouched, as other work is in flight there.

Files of note

Docs:

docs/auth/oauth-clients.md - Option B corrected, "What acs-i3x does today" added

Deployment:

deploy/templates/i3x/i3x.yaml - ROOT_PRINCIPAL

Service:

acs-i3x/lib/api-v1.ts - log_req helper, value-path logging, /info envelope
acs-i3x/lib/routes.ts, acs-i3x/bin/api.ts - debug plumbed through as fplus.debug

Tests:

acs-i3x/test/api-v1.test.ts, acs-i3x/test/e2e.test.ts - /info shape; /v1/info added to the envelope-compliance sweep it was previously excluded from

AlexGodbehere added 4 commits August 6, 2026 08:55

Navigation Menu

Uh oh!

acs-i3x: clear the ground before adding access control - #710

acs-i3x: clear the ground before adding access control#710
AlexGodbehere wants to merge 4 commits into
mainfrom
agent-ab5cd3ada6fbd7da0

AlexGodbehere commented Aug 6, 2026

Uh oh!

AlexGodbehere commented Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

AlexGodbehere commented Aug 6, 2026

1. The OAuth guide described authorization that does not exist

2. ROOT_PRINCIPAL on the i3x deployment

3. Metric values in stdout at info level

4. GET /v1/info envelope - reasoning

Test results

Helm

Noticed, not fixed

How to test

Release notes

Uh oh!

AlexGodbehere commented Aug 6, 2026

Review: four preparatory fixes before access control lands in acs-i3x

What changed (functional level)

Walkthrough

Docs: what acs-i3x actually does

Deployment: ROOT_PRINCIPAL

Logging: no more plant data on stdout

Envelope on /v1/info

How to test

Decisions / open questions

Files of note

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

2. `ROOT_PRINCIPAL` on the i3x deployment

4. `GET /v1/info` envelope - reasoning