Skip to content

feat(delegation): enforce grants at save and at fire - #2864

Merged
rubenvdlinde merged 9 commits into
developmentfrom
feat/delegation-runtime-enforcement
Aug 26, 2026
Merged

feat(delegation): enforce grants at save and at fire#2864
rubenvdlinde merged 9 commits into
developmentfrom
feat/delegation-runtime-enforcement

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

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.

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 an IUser, 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 $skipCheck flag, i.e. a security check with an off switch. There is still exactly one identity-switch primitive; this calls it.

Save timeFlowTriggerValidator refuses a schedule trigger naming a user the saver holds no grant for. Naming yourself stays free. A permitted delegation is stamped runAsDeclaredBy: <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. 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.

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

PHPUnit (Flow + Delegation) 665 passed, 1332 assertions
PHPCS (--warning-severity=0) clean
PHPMD (+ unused-params, baselined) clean
PHPStan no errors
Psalm no errors
Playwright delegated-identity.spec.ts 9/9 live against localhost:8080

Every 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 runAsDeclaredBy on a self-named trigger — the nastier variant, because that path never consults the delegation service at all.

Known limits, stated

  • The audit record is a structured log line, not a durable trail. AuditTrailMapper is object-scoped and a delegation is not an object mutation; inventing a second trail is a bigger decision than this task.
  • Resume-time is not yet re-checked — only queue time. A run already parked in suspended picks its identity back up from the run row without re-resolving. Recorded against task 5.1, where run states are being touched.
  • Task 2.3's declarative-lifecycle half is closed as in tension with 2.2 and 2.2 wins: a grant declaring its lifecycle in a register schema would be an object, and reading it would go through object RBAC — resolving a delegation would require the delegation.

Refactor

FlowDelegationCheck is split out of FlowRunService rather 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 other Flow* collaborator in that directory exists.

Also fixes pre-existing @spec warnings on FlowRunService, FlowScheduleService and FlowRunService::hasActiveRun().

🤖 Generated with Claude Code

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().
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 9b90c99

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.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 57d8346

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.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ b673dcd

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.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ aafa289

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".
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 86bba67

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.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 85c44d6

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.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 14bb214

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.

@rubenvdlinde
rubenvdlinde merged commit acdc3af into development Aug 26, 2026
56 of 58 checks passed
@rubenvdlinde
rubenvdlinde deleted the feat/delegation-runtime-enforcement branch August 26, 2026 20:57
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ a675844

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.

rubenvdlinde added a commit that referenced this pull request Aug 27, 2026
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant