feat(delegation): enforce grants at save and at fire - #2864
Conversation
Closes or-delegation-grants tasks 3.1-3.3. The grant record and its resolver landed in #2851 and nothing consulted them; this is the half that refuses. `DelegationService::runAsDelegated()` is the guarded form of "act as this user". It is deliberately NOT on ObjectService, despite ADR-099 naming it there: `runAs()` is the primitive (hand it an IUser, it narrows) and the grant check is the authorization layer above it. Folding the check into the primitive would make it unusable by callers that legitimately have no delegation to check, and the usual answer to that is a $skipCheck flag — a security check with an off switch. There is still exactly one identity-switch primitive; this calls it. SAVE TIME. FlowTriggerValidator refuses a schedule trigger naming a user the saver holds no grant for. Naming yourself stays free. A permitted delegation is STAMPED with `runAsDeclaredBy: <saver>`, server-written on every save and never read from the request body — and stripped whenever the trigger names its own saver, so a forged value cannot stand in for a grant. FIRE TIME. Without that stamp task 3.3 is not implementable: a schedule fires unattended, so at 03:00 there is no principal to check a grant against and the only candidate left would be `flow.owner` — the fallback ADR-099 removed. FlowDelegationCheck re-resolves at queue time, because save-time passing is not standing authorization: revoking is supposed to stop the next firing, and treating the stored trigger as proof would make revocation cosmetic for exactly the runs nobody is watching. The schedule is left ENABLED on a delegation refusal, unlike the unattributed case. A flow naming nobody cannot fix itself without an edit, so leaving it "on" would be a switch that lies; a revoked delegation becomes valid again the moment the grant does, and disabling would silently convert a temporary revocation into a permanent one only a human re-enabling could undo, with nothing telling them. Migration posture: refuse until granted, no grandfathering. Warranted by the task-1.1 measurement — zero declarations on this instance name anyone other than themselves, so nothing needs grandfathering, and minting grants nobody asked for is the permanent privilege this change exists to stop. Fail-closed is bounded. An unreachable delegation store refuses only runs and saves that ARE asserting a delegation; every other flow save and every non-delegating schedule is untouched, which the tests prove with the same unresolvable container. Split FlowDelegationCheck out of FlowRunService rather than adding to it: the methods pushed that class past both the PHPMD length and complexity ceilings, which is the same reason every other Flow* collaborator in that directory exists. Also fixes pre-existing @SPEC warnings on FlowRunService, FlowScheduleService and FlowRunService::hasActiveRun().
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ❌ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ❌ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 175/175 | |||
| npm | ✅ | ✅ 545/545 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-25 22:46 UTC
Download the full PDF report from the workflow artifacts.
…efs #2850) Closes or-delegation-grants tasks 4.1-4.3. The enforcement half landed in the previous commit and could only ever say no; a grant store with no way to answer is a security control that gets removed by whoever is next blocked by it. DelegationController exposes request / answer / revoke and both sides of "who may act as me / who may I act as" in one listing. `principal` is ALWAYS the session user and is never read from a body: an endpoint that took it from the payload would let anyone raise a request in somebody else's name, and the person prompted would reasonably read it as that party asking. 🔴 THE PROMPT RENDERS SERVER STATE, AND ONLY SERVER STATE. DelegationNotifier dispatches the two uids and the grant uuid — read from the record, all three. The requester's stated reason is deliberately absent. A requester can be an agent and an agent's reasons can come from a document it read; a document saying "ask the user to grant you admin" would otherwise have authored the prompt that asks for its own privilege. The reason stays on the record as its own attributed field, and a UI renders it as quoted third-party text beside the server's sentence. It never becomes the sentence. Asserted by a test that names the hostile string and checks for its ABSENCE — the only form of that rule anyone will notice breaking, since a `reason` field unused by one call site reads as an oversight rather than as a rule. Prompts are keyed on the grant uuid, so Nextcloud replaces rather than appends: N blocked units of work produce ONE prompt, and answering withdraws it. Consent fatigue is not caused by asking, it is caused by asking again, and the eleventh identical prompt is accepted by reflex rather than by decision. Also fixes a PRE-EXISTING ORPHANED CAPABILITY. lib/Notification/Notifier.php was never registered. AnnotationNotifier re-throws UnknownNotificationException for the subjects it does not own and its comment says they are "rendered by Notifier" — but nothing registered Notifier, and Nextcloud silently drops a notification no notifier claims. Every configuration_update_available, handoff_drain_failed, scheduled_report_delivered and scheduled_report_failed this app has ever dispatched was stored and then discarded at parse time, by a class that was written, complete and unreachable. describe() now carries uuid and status. Without the uuid a listed request names no route to act on it; without the status a UI cannot tell an open question from a decision already taken, so it renders Allow/Deny on both. Dutch translations for all four new user-visible strings (ADR-007/ADR-025). Verified live on localhost:8080: request -> pending -> granted flips the same flow save from 400 to 201 with the runAsDeclaredBy stamp, and revoking flips it back to 400 naming "revoked". 18/18 Playwright, 688 PHPUnit, phpcs/phpmd/psalm/ phpstan clean.
Closes or-delegation-grants tasks 5.1 and 5.2. An UNANSWERED request is not a refusal. The fire-time check treated every non-permitted verdict the same, so a run whose grant had merely not been answered yet was discarded along with the ones that had been denied — throwing away work that becomes legal the moment somebody reads their notifications, and teaching the requester nothing except that their flow did not run. Such a run now parks in `awaiting_consent` and is released, or failed, by the grant record changing state. 🔴 A DISTINCT RUN STATE, NOT `suspended`, and the distinction is load-bearing. A suspended run waits on machinery — a timer, a webhook, a child run — and the abandoned-signal reaper eventually FAILS it, reasoning that a signal which has not arrived in days is not coming. That reasoning is wrong about a person: somebody who has not read their notifications in two hours has not declined, they are at lunch. Parking in `suspended` would have handed these runs to that reaper and failed them while the prompt sat unread — reporting "nobody answered" about a question nobody had yet seen. The parked run carries no `resume_at`, deliberately: with one, the timed-resume sweep would start it before anybody had answered. What releases it is the grant, re-resolved — so one answer frees every run it unblocks, which is the other half of the request dedup. 🔴 THE TIMEOUT FAILS, IT DOES NOT PROCEED. Running the work after a timeout would convert an unread prompt into an approval at whatever hour the timer elapsed, which is the exact substitution this subsystem exists to prevent. The recorded error says "an unanswered request is not consent" rather than merely noting that time passed. Default 72h, `flow_consent_wait_hours`. An UNREADABLE store leaves the run parked rather than failing it — the trade-off inverts from the fire-time check, where refusing costs one run and permitting costs an unauthorized execution. Here nothing runs either way, so waiting is free and failing destroys work over a blip. `awaiting_consent` is ACTIVE and not TERMINAL: a parked run is still going to happen, and omitting it would hide it from every "currently running" surface — which is exactly where somebody goes to find out why their work has not run. The worker's delegation service is nullable so the two positional unit suites keep working, but its absence is REPORTED when runs are actually parked. A sweep that skips produces the same output as one that found nothing to do. Verified live on localhost:8080, both jobs driven by occ background-job:execute: grant -> save (stamped) -> revoke -> re-request (pending) -> schedule fires -> run parks reading `Waiting for "ddauth-alice" to allow "admin" to act as them.` -> answer allow -> worker sweep releases -> queued -> executed -> stopped. 769 PHPUnit green; phpcs/phpmd/psalm/phpstan clean. FlowRunService went one line over the PHPMD class-length ceiling, so the park branch is its own method.
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 175/175 | |||
| npm | ✅ | ✅ 545/545 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ❌ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-26 09:14 UTC
Download the full PDF report from the workflow artifacts.
…tated open Gate 96 (system-elevation-reachability) is ConductionNL/.github#579. Recorded as ADDED ALONGSIDE SystemOperationContextBoundaryTest rather than replacing it: the test runs in milliseconds on every local phpunit and names the four permitted files exactly, which the gate deliberately does not — it permits by directory. Deleting the faster, more specific instrument because a broader one exists trades a signal for nothing. Three things are recorded as OPEN rather than ticked, because each would otherwise read as verified: * the awaiting_consent path is verified live by hand (occ background-job: execute) but has no Playwright spec — the api-direct harness cannot drive a schedule fire or a cron sweep, and a spec that asserted the setup rather than the behaviour would be worse than none; * composer test:all was not run to completion locally — the full tests/Unit tree is 17,351 tests and exhausts 2GB on this box. The affected subtrees run green at 769; CI runs the whole suite; * the consent prompt has had no accessibility pass. It renders on Nextcloud's own notification surface, but the two action labels are ours.
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ⏭️ | ||||
| phpcs | ⏭️ | ||||
| phpmd | ⏭️ | ||||
| psalm | ⏭️ | ||||
| phpstan | ⏭️ | ||||
| phpmetrics | ⏭️ | ||||
| eslint | ⏭️ | ||||
| stylelint | ⏭️ | ||||
| build | ⏭️ | ||||
| composer | ⏭️ | ⏭️ | |||
| npm | ⏭️ | ⏭️ | |||
| app:check-code | ⏭️ | ||||
| info.xml | ⏭️ | ||||
| REUSE | ⏭️ | ||||
| PHPUnit | ❌ | ||||
| Newman | ❌ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-26 09:35 UTC
Download the full PDF report from the workflow artifacts.
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 175/175 | |||
| npm | ✅ | ✅ 545/545 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-26 11:17 UTC
Download the full PDF report from the workflow artifacts.
…rompt The coverage ratchet on PR #2864's changed files failed: 73.19% head against 75.68% base, a 2.49% drop. The suite was green — the drop is new code arriving with e2e coverage and no unit coverage, and the ratchet only reads clover. Three gaps, all real rather than cosmetic: * DelegationController — 328 lines with no unit test at all. It carries the authorization routing, and the case that most needed pinning is not reachable cheaply from e2e: a caller putting `principal` in the body. The test asserts on the value handed to the LIFECYCLE, because that is where the substitution would take effect. Also pinned: 403 rather than 400 for "you may not answer this" (being refused is an authorization outcome; reporting it as a malformed request sends the reader to their payload), and that all four routes refuse without a session — asserted across all four, because the check is per-method and a route that forgot it would be invisible to a test that only exercised its neighbour. * FlowDelegationCheck — driven indirectly by FlowRunAttributionTest, but its recording branch was unreached. The two tests that matter assert the asymmetry: a revocation IS written onto the flow, and the schedule is LEFT ENABLED. A flow naming nobody cannot fix itself without an edit; a revoked delegation becomes valid again the moment the grant does. * Notifier::prepareDelegationConsentRequested — ~40 statements rendering the prompt, untested. It must NAME the requester (a prompt saying "somebody" is one a person cannot answer responsibly) and survive a parameterless notification row, since a row can outlive the shape that wrote it and throwing would take the notifications endpoint down for every other subject. Also fixes PRE-EXISTING DEBT found while chasing this: the local unit suite could not complete. BlobMigrationJobTest resolves real services from \OC::$server in setUp(), and on a box where the bootstrap found an NC root it could not initialise that resolution does not fail — it RUNS AWAY. Measured: identical death at test 259 of 17,359 with a 2GB limit and with a 5GB one, so it was never a size problem, and the fatal took the other 17,100 tests with it. The bootstrap already sets OPENREGISTER_TEST_SKIP_NC=1 in exactly that case and its comment promises container-bound tests "will fail clearly"; this is the class where that promise was not kept. It now skips with a message naming what went unverified — the batching, the completion flag and the orphan grouping — because "skipped" alone cannot tell "no NC" from "the job is broken".
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 175/175 | |||
| npm | ✅ | ✅ 545/545 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-26 11:54 UTC
Download the full PDF report from the workflow artifacts.
…dy see Gate-7 (no-admin-idor) flagged `request()`: `#[NoAdminRequired]` with no per-object guard. It was right, and the finding is not the one the gate's name suggests. 🔴 THE ENDPOINT WAS A USER-EXISTENCE ORACLE. Any authenticated user could POST a uid and read the status code as an answer — 201 for a real account, 404 for an invented one. And it would have been a NEW oracle: Nextcloud governs enumeration through its own sharing settings, so an endpoint that answers around them has removed a control rather than added a feature. I introduced that while adding an existence check for a good reason (a pending request naming nobody can never be answered, so it would sit until it expired while its requester waited for a prompt no account could receive). Both facts now return ONE response. "Not someone you may ask" is all a requester needs and all they are entitled to; the difference between "no such person" and "not in your organisation" is exactly what an oracle is built out of. A test asserts the two are indistinguishable down to the uid the caller already supplied. ORGANISATION is the boundary because it is the fleet's tenancy unit — the same one scoping every register, schema and run. A delegation that crossed it would let one tenant's user request rights inside another's. An administrator is exempt: answering across tenants is what they are for, and that exemption is its own test, so a controller that refused every cross-organisation request could not pass this file. An unreadable organisation list REFUSES. Treating it as "no restriction" would re-open the enumeration surface wholesale, and it is the fail-open shape this subsystem has already been bitten by twice. One test note worth keeping: `Organisation::getUsers()` is a MAGIC method served by Entity::__call, so createMock() cannot stub it — it answers "Method name is not configured". The fixture uses a real entity with setUsers(). 16 tests / 29 assertions green; gate-7 clean on the file; phpcs, phpmd, psalm and phpstan clean; 18/18 Playwright still green live.
…n-runtime-enforcement
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 175/175 | |||
| npm | ✅ | ✅ 545/545 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-26 14:40 UTC
Download the full PDF report from the workflow artifacts.
…in e2e Two of the three things this change had recorded as open, closed. Both were recorded honestly and both turned out to be smaller than the note implied. 🔴 THE PROMPT NOW RENDERS AS A PERSON, NOT A TOKEN. `setRichSubject` / `setRichMessage` carry the requester as a `user` parameter, so a client renders a real user reference — display name, avatar, semantics a screen reader can announce as a person — and `setParsedSubject` / `setParsedMessage` keep the plain-text sentence every other surface reads. Both, and neither optional: NC's own isValidParsed() refuses a notification with only rich text, and a client without rich rendering shows nothing without the parsed half. A test asserts the two say the SAME sentence, because a rich subject reading differently from its fallback would mean two people looking at one security decision on different clients were answering different questions. A uid is an identifier, not a name. Asking somebody to grant rights to `j.devries3` when the person they know is "Jan de Vries" is harder for every reader and materially harder for one who cannot glance at a face beside it, so the display name is resolved and asserted — with the uid kept as the rich parameter's `id`, which is how a client resolves the avatar. The action labels stay "Allow" and "Deny" rather than becoming OK/Cancel. Out of context — which is how a screen reader reaches a button list — "OK" is meaningless and "Allow" is not. The message states the consequence BEFORE the controls, because a reader meets them in reading order.⚠️ I WAS WRONG THAT THE HARNESS COULD NOT DRIVE THIS. I recorded `awaiting_consent` as untestable in e2e because "the api-direct harness cannot drive a schedule fire or a cron sweep". `flow-schedule.spec.ts` has been doing exactly that since it was written — `occ background-job:execute` through the dev container, skipping when unreachable. delegation-parking.spec.ts follows that pattern: park on an unanswered request, assert the run says WHO it waits on, then answer and assert the sweep releases it into work that actually runs. Also fixes PRE-EXISTING DEBT the same file revealed: flow-schedule.spec.ts matched `Cron\FlowScheduleWorker`, and #2870 moved that job to `BackgroundJob\`. A matcher pinned to a namespace stops finding its job after a move — which does not FAIL the spec, it SKIPS it, so the whole scheduled-trigger path would have gone unverified with nothing red to say so. Both specs now match the class basename, which survives a namespace move.⚠️ NOT RUN LIVE. Docker is down on this box, and resolveContainer() refuses the shared `nextcloud` container by design, so this spec skips here regardless. The path it covers was verified by hand earlier today against a live instance — grant, save, revoke, re-request, schedule tick, park, answer, sweep, executed — and the spec encodes that sequence. Said plainly rather than reported as green. Dutch translations for all three new strings. 20 tests / 42 assertions in the Notifier suite; phpcs, phpmd, phpstan and the l10n parity check clean.
Ticked as DECIDED rather than left open. An open box invites somebody to 'finish' it by making the grant an object — and that is exactly the circularity task 2.2 exists to prevent: x-openregister-lifecycle and x-openregister-notifications are properties of a register schema evaluated by the object layer, so a grant declaring either would be read through object RBAC, and resolving a delegation would require the delegation. The two tasks cannot both be satisfied. 2.2 is the one holding a security property, so it wins, permanently — this is not a deferral and the box should not read like one. The notification half IS delivered, by 4.1, and the same argument explains why it is imperative rather than declarative: the dialect fires on OBJECT lifecycle events and a grant has none. or-delegation-grants now has no unticked task.
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ⏭️ | ||||
| phpcs | ⏭️ | ||||
| phpmd | ⏭️ | ||||
| psalm | ⏭️ | ||||
| phpstan | ⏭️ | ||||
| phpmetrics | ⏭️ | ||||
| eslint | ⏭️ | ||||
| stylelint | ⏭️ | ||||
| build | ⏭️ | ||||
| composer | ⏭️ | ⏭️ | |||
| npm | ⏭️ | ⏭️ | |||
| app:check-code | ⏭️ | ||||
| info.xml | ⏭️ | ||||
| REUSE | ⏭️ | ||||
| PHPUnit | ❌ | ||||
| Newman | ❌ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-26 20:57 UTC
Download the full PDF report from the workflow artifacts.
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 175/175 | |||
| npm | ✅ | ✅ 545/545 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-26 22:33 UTC
Download the full PDF report from the workflow artifacts.
* style(notifier): fix two phpcs errors that landed on development #2864 merged while its `PHP Quality (phpcs)` cell was red, so `development` now carries both: * an inline IF — the display-name fallback ternary * a positional call to `displayName()` Neither is a behaviour change; both block every subsequent PR's phpcs cell until they are gone, which is why this is its own small PR rather than a follow-up. 🔑 I had reported this file clean. The run was real and described an earlier version of it: I ran phpcs, then made two more edits to localise the rich strings, and never re-ran. A check that ran before the last edit is not a check on what shipped. Reproduced, fixed, re-verified: phpcs, phpmd and phpstan clean on lib/Notification; 20 tests / 42 assertions green. * test(notifier): cover the two displayName branches the ratchet found The coverage ratchet failed this PR on a 0.09% drop, and it was right. Turning the fallback ternary into an explicit early return took the file from 182 statements to 184, and one of the two new ones — `return \$uid` when the display name is blank — had nothing exercising it. Two branches now do: * a BLANK display name falls back to the uid. Nextcloud permits an empty display name, and an empty one here reads as " asks to act on your behalf" — a security decision with no subject in it. The uid is poorer copy and it is always a name for somebody. * NO user manager at all. The collaborator is nullable so the existing hand-built construction keeps binding, and that is only safe if the null path is exercised — an untested optional dependency is one whose absence is discovered in production. A 0.09% drop is small enough to wave through and was a real gap. 12 tests / 28 assertions. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
Closes
or-delegation-grantstasks 3.1–3.3. The grant record and its resolver landed in #2851 and nothing consulted them — this is the half that refuses.What changed
DelegationService::runAsDelegated()— the guarded form of "act as this user".Deliberately not on
ObjectService, despite ADR-099 naming it there.runAs()is the primitive (hand it anIUser, it narrows); the grant check is the authorization layer above it. Folding the check into the primitive makes it unusable by callers that legitimately have no delegation to check — a request handler running as its own authenticated user, a job replaying its recorded actor — and the usual answer to that is a$skipCheckflag, i.e. a security check with an off switch. There is still exactly one identity-switch primitive; this calls it.Save time —
FlowTriggerValidatorrefuses a schedule trigger naming a user the saver holds no grant for. Naming yourself stays free. A permitted delegation is stampedrunAsDeclaredBy: <saver>, server-written on every save, never read from the request body, and stripped whenever the trigger names its own saver — so a forged value cannot stand in for a grant.Fire time — without that stamp, 3.3 is not implementable: a schedule fires unattended, so at 03:00 there is no principal to check a grant against, and the only candidate left would be
flow.owner— the fallback ADR-099 removed.FlowDelegationCheckre-resolves at queue time, because save-time passing is not standing authorization: revoking is supposed to stop the next firing, and treating the stored trigger as proof would make revocation cosmetic for exactly the runs nobody is watching.Two decisions worth reading
🔴 The schedule is left ENABLED on a delegation refusal, unlike the unattributed case. A flow naming nobody cannot fix itself without an edit, so leaving it "on" would be a switch that lies. A revoked delegation is the opposite — it becomes valid again the moment the grant does, with no edit at all, and disabling would silently convert a temporary revocation into a permanent one that only a human re-enabling could undo, with nothing telling them to.
Migration posture: refuse until granted, no grandfathering. Warranted by the task-1.1 measurement — zero declarations on this instance name anyone other than themselves, so nothing needs grandfathering, and minting grants nobody asked for is the permanent privilege this change exists to stop.
Fail-closed, and bounded
An unreachable delegation store refuses only saves and runs that are asserting a delegation. Every other flow save and every non-delegating schedule is untouched — the tests prove that with the same unresolvable container, so the blast radius of an infrastructure fault is exactly the work whose authorization cannot be established.
Verification
Flow+Delegation)--warning-severity=0)delegated-identity.spec.tslocalhost:8080Every refusal assertion is paired with a positive control — the same trigger with one field changed — because a validator that rejects everything satisfies all four refusal tests and is an outage. The e2e refusal uses a uid that resolves (
ddauth-alice), not a ghost: refusing an unknown account was already true, and only a real colleague isolates the rule this change adds.Three tests target the stamp specifically, including a client-supplied
runAsDeclaredByon a self-named trigger — the nastier variant, because that path never consults the delegation service at all.Known limits, stated
AuditTrailMapperis object-scoped and a delegation is not an object mutation; inventing a second trail is a bigger decision than this task.suspendedpicks its identity back up from the run row without re-resolving. Recorded against task 5.1, where run states are being touched.Refactor
FlowDelegationCheckis split out ofFlowRunServicerather than added to it: the methods pushed that class past both the PHPMD length (1078 > 1000) and complexity (53 > 50) ceilings — the same reason every otherFlow*collaborator in that directory exists.Also fixes pre-existing
@specwarnings onFlowRunService,FlowScheduleServiceandFlowRunService::hasActiveRun().🤖 Generated with Claude Code