Stamp updated_by with the requesting seat's principal, so a web edit names who made it - #3151
Conversation
The OIDC work resolved the subject, published the seat on HttpContext.Items, and left both consumers reading literals: the two custom-view write sites passed WebEditorPrincipal and /api/session answered a hardcoded true. Both now read DarlingWebSeat.FromContext(context), so a web edit names the person who made it and a read-only seat is rendered as one. The MCP stamp stays an unconditional constant and now says why: MCP has no sign-in flow to carry a person through, so there is no subject to prefer. Guards derived from the property's violation routes rather than from reading the existing assertions, including a discovered write-site population so a third write site is covered without being named, and a second census that fails differently. The live round-trip's fixture principal is no longer the web constant, which a store that dropped the argument used to satisfy.
WebEditorPrincipal ends in EditorPrincipal, so a substring test for the seat read passed on exactly the constant-stamping code it rejects. Found by reverting one write site and watching every check stay green.
|
Reviewed the diff (DarlingWebEndpoints.cs + the two test files). No correctness, security, or Lite/Darling parity issues found. Notes from the review:
No inline comments — nothing rose to the level of a blocking or even a nitpick finding. |
The stamp and fixture comments explained themselves by narrating what the code did before, which reads as a diff annotation rather than as the reason the current shape is required. Same arguments, stated as properties that hold now. Also folds the inline System.IO qualifications into a using.
Every argument the store's writes take around updatedBy is a string, so a positional swap with description compiles, stamps the description into updated_by, and satisfies a check that only asks whether the principal appears somewhere in the argument list. Naming the parameter makes that swap CS8323 instead, and the guards require the name so it cannot quietly go away.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed the diff (Darling-only: One real finding, left as an inline comment on The two new test files verify the requested No performance concerns — this only changes what gets bound into an existing parameterized write, and adds one |
An honest can_edit makes a UI path reachable that only ever described a failed probe, so an OIDC viewer was told to reload to edit — something their role will never allow. The client's fail-closed default now marks itself with probe_failed, which the server's own answer never carries, and each composer branches its notice on it. Every discovered composer is held to that. The attribution pins read source through the raw reader. Their anchors all sit on one line, so the LF-normalising variant would change nothing about what they match while placing the file in a census whose membership means the opposite.
Asserting probe_failed appears anywhere in views-api.js was satisfied by this mechanism's own doc comment, so stripping the flag from the fail-closed default left the check green. Read the session probe's body instead: prose about a mechanism is not the mechanism.
ReviewWent through the diff ( Lite/Darling parity: No parity gap. Custom views, OIDC, and the web dashboard are Darling-only concepts — confirmed T-SQL style: N/A — no Correctness:
Security: No injection risk — Tests: No blocking findings. Nice, well-scoped fix with unusually strong regression coverage for a UI/attribution change. |
1 participant
Closes #2550 — the residual it was rescoped to.
9da00121elanded OIDC token validation and claim-to-role mapping, delivering three of the issue's four benefits. The fourth was identity, and it did not land:DarlingWebEndpoints.csstill read "The web surface has no per-user identity, so a constant is honest" — verbatim the comment the issue opened by quoting — and every custom view created or edited over the web was stampedweb, so the surface could not answer who changed this dashboard.Nothing was missing except the wiring.
DarlingWebSeat.EditorPrincipal(Subject ?? "web") andDarlingWebSeat.FromContextboth already existed, both were already unit-tested, and neither had a production caller. The auth middleware's own comment named its two consumers — "/api/session, the updated_by stamps" — while neither of them read the seat it published. No schema change, as the issue predicted:config.custom_views.updated_byis unbounded nullabletext(PgMigrationsV31) and a subject is capped at 251 characters, so it already fits.What changed
Both custom-view write sites and
/api/sessionnow readDarlingWebSeat.FromContext(context). A web edit names the person who made it; a read-only seat is rendered as one./api/sessionis here rather than filed because it is the same one-line defect in the same function:DarlingConfig'sviewerRolesdocuments the SPA hiding edit affordances "when/api/sessionreportscan_edit: false", and the endpoint returned a hardcodedtrue, so that documented behaviour was unreachable and a viewer saw buttons whose every click the middleware then refused. Nothing changes for a deployment without OIDC — the shared-token seat'sCanEditistrue.The two decisions
What
updated_bycarries with no OIDC — kept as one heterogeneous column. The literalweb, which makes the column sometimes a person and sometimes a provenance marker. Rejected the separate column: the only consumer is a rendered byline (views.js:"updated " + relTime(...) + " by " + updated_by), which wants exactly one value, and splitting it would make every reader join two columns to recover the string it already displays. Nothing filters, sorts or groups on it — the two reads are bare projections inListSql/GetSql, andListSqlorders byname— so heterogeneity costs nothing a query has to cope with. It is also less of a change than it sounds: an IdP configured to stamp an opaquesubGUID already produces values no more person-like thanweb. Recorded on the constant so the next reader gets the argument, plus when to revisit it.MCP keeps its constant. MCP authenticates a client on its own network block with its own token and has no sign-in flow to carry a person through, so there is no subject to prefer — the same argument that used to justify a constant on the web surface, still true there. Left alone as the issue asked, but now pinned and explained, so the asymmetry survives someone reasonably "finishing the job" on a surface that has nothing to finish it with.
The seat the honest answer newly exposes
Making
can_edithonest makes a UI path reachable that had only ever described a failed probe.editor.jsandnotebook.jsboth told acan_edit: falsecaller "Couldn't confirm your session… Reload the page to edit" — correct for a broken probe, and wrong for an OIDC viewer, who is correctly signed in and will never gain edit rights by reloading.views-api.js's doc comment asserted the old always-true behaviour outright.The client could not tell the two apart, because both arrive as
can_edit: false. The fail-closed default is built client-side, so it now marks itself —{can_edit: false, probe_failed: true}— and the server's own answer never carries the flag. Each composer branches its notice on it. This is in the PR rather than filed because my change is what made the wrong copy reachable; shipping the honest server answer and filing the misleading message it exposes would be shipping the half nobody sees.The guard
Derived from the property, not from reading the existing assertions: every custom-view write arriving over the web stamps
updated_bywith the requesting seat's ownEditorPrincipal, and no web write site stamps a constant of its own. Enumerating how that becomes false found two things reading the assertions would not have.The write-site population is discovered from the source rather than listed, because a pin naming today's two sites is blind to tomorrow's third — with a floor on the discovered count, since a per-site loop over an empty population passes vacuously. A second census, over the whole service project rather than one file, catches the constant being reached for in a file the first scan never opens. The one route with no symptom at all gets its own pin: if the middleware stopped publishing the seat,
FromContextreturns its documented shared-token default and every named operator's rows silently revert towebwith nothing thrown and nothing logged.Two coverage gaps found by mutating rather than by reading:
EditorPrincipal— andWebEditorPrincipalends inEditorPrincipal, so it passed on precisely the constant-stamping code it existed to reject. Reverting one write site left all eight green. Now matched as a whole identifier.probe_failedappeared anywhere inviews-api.js— and the mechanism's own doc comment mentions it, so removing the flag from the code left the check green. Scoped to the session probe's body."web"and asserted"web"came back, which a store that dropped the argument and hardcoded the constant satisfied perfectly. The fixture now uses distinct subject-shaped placeholders, and the update's is different from the create's — which also pinsUpdateSql's$5end-to-end for the first time, since nothing asserted the update's stamp at all before.Sixteen mutations, each applied in isolation and confirmed present in the file before running. Both single-site reverts red individually, naming the offending line and argument list, so coverage is per-site rather than aggregate. A new third write site passing a bare
"web"literal reds — the literals appear blanked in the failure message, which is why the assertion is positive: a negative check for the constant could not have seen that spelling. Also red: the constant referenced from a third file, the middleware not publishing the seat, publishing it after forwarding,/api/sessionback totrue,EditorPrincipaldropping the subject, the fallback's spelling changing, an MCP site drifting off its constant, the store's write methods renamed so discovery finds nothing, one composer's notice un-branched, and the client default no longer marking itself.A third route came out of enumerating rather than reading: every argument around
updatedByis a string, so a positional swap withdescriptioncompiles and stamps the description. That one is closed at the compiler — the parameter is named at all four write sites, and the swap is nowCS8323— with the guards requiring the name so it cannot quietly go away.Verification, scoped
The new pins were executed against the real built assembly —
Darling.Teststargetsnet10.0-windowsand cannot run on macOS, so the actual test file was compiled into anet10.0console harness with the genuinexunit.v3.assert: 8 passed, and each mutation above observed red there.Darling PostgreSQL testsis the arbiter for the live round-trip,buildfor the Windows suites.What this does not prove: these are source-derived. They establish that each write site passes the seat's principal and that the middleware publishes the seat — not that an end-to-end request lands a subject in the column. There is no HTTP-level harness for these endpoints and standing one up needs a live Postgres and a real bind. The behavioural half is the seat's own resolution over real
HttpContextinstances plus the live store round-trip; the join between them is asserted from the source. Said plainly because "it's tested" unscoped is how #2213's wiring gap survived a green suite.CHANGELOG
Entry text for the
[Unreleased]block, not applied here:Out of scope, described rather than fixed
The write population is closed at four sites, checked rather than assumed: two web (
CreateAsync/UpdateAsyncinDarlingWebEndpoints) and two MCP (DarlingMcpCustomViewTools). There is no third surface —ViewerDataService's twocustom_viewsmentions are the schema-probe sentinel and a comment, andMainWindow.Tags.cs's is about the viewer database role's grant, not a write path. The WPF viewer never stamps this column.MCP per-user identity is the one genuinely deferred thing, and it is a different credential model rather than a line change: MCP would need a sign-in flow before it had a subject to stamp.
Do not conflate the two
updated_bycolumns.config.config_service.updated_byis a different table, stamped'cli'by the endpoint-toggle commands, and is untouched here.