feat(rbac): let a caller ask to be judged as nobody (WOO-578) - #3855
WilcoLouwerse wants to merge 5 commits into
Conversation
OpenCatalogi's public /api/search promises that a signed-in administrator sees exactly what an anonymous caller sees (SCH-PFTS-001). The runtime toggle that enforced this, `_rbacAsPublic`, was removed with the inheritFromPublic graft, and WOO-551 accepted the drift to keep the endpoint alive. This brings the guarantee back as a scope instead of a flag threaded through six layers. ObjectService::runAsAnonymous(callable) clears the session subject for the duration of the callable — the narrowing twin of runAs() — so every reader of IUserSession::getUser() in the RBAC and organisation layers sees no user: no admin bypass, no _owner grant, no group rules, no inheritFromPublic widening. The permission caches are keyed by UID and stay correct by construction. Clearing the subject alone would make the evaluation MORE permissive under occ or PHPUnit, because two guards trust a call without a user: the CLI bypass in the RBAC filters and SystemOperationContext. AnonymousEvaluationContext is the static marker that closes both doors while the scope is active; SystemOperationContext::isActive() yields to it, so narrowing wins over elevating everywhere the system scope is consulted. It is deliberately not a query key: nothing in a request can switch it on or off, and a test pins the class to exactly two static entry points. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| test-l10n-parity | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 170/170 | |||
| npm | ✅ | ✅ 654/654 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it on any branch from the Actions tab, or locally with npx playwright test. |
||||
| Hydra gates | ❌ |
Quality workflow — 2026-09-17 13:50 UTC
Download the full PDF report from the workflow artifacts.
Hydra gate-16 wants an @SPEC on every changed method; phpmd flags the static reads of AnonymousEvaluationContext, which are the point of an ambient marker — same note SystemOperationContext already carries at its other call sites. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| test-l10n-parity | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 170/170 | |||
| npm | ✅ | ✅ 654/654 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it on any branch from the Actions tab, or locally with npx playwright test. |
||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-17 14:15 UTC
Download the full PDF report from the workflow artifacts.
The scope did not work on a real request. `setVolatileActiveUser(null)`
looks like the inverse of what runAs() does, but in Session::getUser()
null is not "no user" — it is "not resolved yet":
if (is_null($this->activeUser)) {
$uid = $this->session->get('user_id'); // still the admin
$this->activeUser = $this->manager->get($uid);
}
So the next read re-hydrated the signed-in user from the PHP session and
the admin bypass fired as before. runAs() escapes this only because it
writes a non-null user. Under CLI there is no user_id, which is why the
suite was green and CI could not see it.
Incognito mode is what core itself uses to serve a public link while a
session exists (ShareController, PublicAuth, BearerAuth), and getUser()
checks it FIRST, before the fallback. The volatile clear stays so the
memoised copy does not survive either, and the previous incognito state
is restored rather than switched off, so nesting composes.
The old test could not catch this: a createMock(IUserSession) models
setUser() semantics — set null, get null. The new one reproduces core's
fallback and fails without the fix (verified by reverting it).
Two caches also leaked across the scope, which falsifies the "keyed by
UID, correct by construction" claim the docblock made:
- PermissionHandler keyed on the $userId ARGUMENT, so an admin call that
defaults to the current user and an anonymous call shared the key
`u_` with different verdicts. It now keys on the resolved subject,
which fixes the same latent defect for runAs().
- ConditionMatcher memoised the active organisation in a single field,
so `@organisation.uuid` could answer with an admin's tenant inside an
anonymous evaluation. Now memoised per subject, misses included.
Also from the review: a test pinning the organisation-handler gating
(three of the four sites had none), a note that the system scope yielding
is not uniformly narrowing — lifecycle-event suppression starts firing,
so do not open this scope inside a system operation — and a marker on the
second MultiTenancyTrait block, which is pre-existing dead code.
Found by the review pass on this PR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ❌ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| test-l10n-parity | ✅ | ||||
| format | ❌ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 170/170 | |||
| npm | ✅ | ✅ 654/654 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ❌ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it on any branch from the Actions tab, or locally with npx playwright test. |
||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-18 09:44 UTC
Download the full PDF report from the workflow artifacts.
Same reason as the phpstan ignore added with the fix: OC_User is a Nextcloud server legacy global, absent from nextcloud/ocp and with no OCP equivalent, but always present at runtime. Its incognito mode is the only switch Session::getUser() honours before its user_id fallback, which is what runAsAnonymous() needs (WOO-578). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| test-l10n-parity | ✅ | ||||
| format | ❌ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 170/170 | |||
| npm | ✅ | ✅ 654/654 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ❌ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it on any branch from the Actions tab, or locally with npx playwright test. |
||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-18 10:15 UTC
Download the full PDF report from the workflow artifacts.
WilcoLouwerse
left a comment
There was a problem hiding this comment.
Verdict: APPROVE (Strict) — self-review posted as COMMENT (GitHub blocks self-APPROVE)
A strict pass found one blocker that would have shipped a primitive that does nothing on a real request, plus two cache leaks that falsified this PR's own safety claim. All are fixed in 0b982272; the verdict reflects the branch as it stands.
🔴 The scope did not clear the subject on an HTTP request — fixed in 0b982272
setVolatileActiveUser(null) reads as the inverse of runAs(), and it is not. In core's Session::getUser(), null is not "no user" — it is "not resolved yet":
if (is_null($this->activeUser)) {
$uid = $this->session->get(user_id); // still the signed-in admin
$this->activeUser = $this->manager->get($uid);
}So the next read inside the scope re-hydrated the admin from the PHP session, MagicRbacHandler::applyRbacFilters() hit in_array('admin', $userGroups) and returned without applying any filter. runAs() escapes this only because it writes a non-null user.
The suite was green because PHPUnit runs under CLI, where there is no user_id — and because a createMock(IUserSession::class) models setUser() semantics: set null, get null. It structurally could not catch this.
The fix is what core itself uses to serve a public link while a session exists — OC_User incognito mode, which getUser() checks first, before the fallback (precedent: ShareController, PublicAuth, BearerAuth). The volatile clear stays so the memoised copy does not survive either, and the previous incognito state is restored rather than switched off, so nesting composes. The new test reproduces core's fallback and I confirmed it fails with the fix reverted and passes with it restored.
🟡 Two caches leaked across the scope — fixed in 0b982272
The docblock claimed the permission caches are "keyed by UID and stay correct by construction". That was false:
PermissionHandlerkeyed on the$userIdargument, which isnullfor the ordinary "use the current user" call — so an admin-defaulted call and an anonymous call shared the keyu_with different verdicts. Now keyed on the resolved subject, which fixes the same latent defect forrunAs().ConditionMatcher::$cachedActiveOrgwas a single field, so@organisation.uuidcould answer with an admin's tenant inside an anonymous evaluation — a cross-tenant widening on a public endpoint. Now memoised per subject, misses included.
I could not demonstrate a live pre-scope call site for the first on the /api/search path, so it was not exploitable today. It is public API with a false safety claim attached, which is enough.
🟡 Three of the four gating sites had no test — one added
MagicRbacHandlerAnonymousScopeTest genuinely pinned applyRbacFilters. MagicOrganizationHandlerAnonymousScopeTest now pins the organisation handler, and its observable is the returned scope itself rather than a mock expectation: SCOPE_ALL without the scope, not SCOPE_ALL inside it.
testInsideTheScopeAnAnonymousCallerHoldsNoStaffPermission passed for the wrong reason — hasPermission() has neither bypass, so it denies with or without the scope. Renamed and re-documented as the companion check it actually is, not as evidence for the gating.
🟢 Two things worth knowing, now written down
- The system scope yielding is not uniformly narrowing. Most consumers deny where they would have allowed, which is the intent. Two do the opposite:
MagicMapper::suppressLifecycleEvents()and SaveObjects' bulk dispatch use it to withhold work, so inside an anonymous scope they would start firing again — the per-object listener storm that suppression exists to prevent. Not reachable today (the only caller is a read-only public search, and nothing opens the scope internally), so it is a constraint on the next caller. Noted in the class docblock. - Under the web SAPI three of the four gatings are no-ops, because
PHP_SAPI === 'cli'is false there. In production the only effective narrowing fromAnonymousEvaluationContextisSystemOperationContext::isActive()returning false — which is exactly why the blocker above was fatal rather than partial. The secondMultiTenancyTraitblock is also unreachable (the earlier null-user block returns on every branch); its gating is kept for symmetry and marked as such rather than removed, since that is pre-existing dead code and not this PR's to clean up.
Verified clean
@spec openspec/specs/rbac-scopes/spec.md resolves. All four CLI bypasses are found and gated — grep -rn "PHP_SAPI" lib/ returns exactly those four plus two comments. Nothing request-side can reach AnonymousEvaluationContext::run(), and testTheScopeHasNoSettableSurface pins the public surface by reflection. ObjectGrantResolver and OrganisationService::$userOrgsCache are correctly keyed by uid; cachedInheritFromPublic is keyed by schema only and correctly so, since it never reads the subject. MultiTenancyTrait's session readers do not memoise. finally, nesting and throw behaviour are covered.
CI
Frontend Check (format), PHPUnit (NC stable35) and the aggregate Quality Report are red on development too — the MDTO archival tests, untouched here. Psalm was red on this branch and is fixed in 4068d3cf: like phpstan, it cannot resolve the OC_User legacy global, so it gets the same documented suppression.
🤖 Generated with Claude Code
…nonymous-evaluation
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| test-l10n-parity | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 170/170 | |||
| npm | ✅ | ✅ 654/654 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it on any branch from the Actions tab, or locally with npx playwright test. |
||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-18 15:09 UTC
Download the full PDF report from the workflow artifacts.
Why
OpenCatalogi's public
/api/searchpromises uniform visibility (SCH-PFTS-001, WOO-536): a signed-in administrator sees exactly what an anonymous caller sees. The runtime toggle that enforced this,_rbacAsPublic(#2855), was removed with theinheritFromPublicgraft (31687c6f3), and WOO-551 accepted the drift to keep the endpoint alive. This brings the guarantee back as a scope, not a flag threaded through six layers.Jira: WOO-578 (sub-task of WOO-572). OpenCatalogi side: ConductionNL/opencatalogi
feat/woo-578-anonymous-evaluation(guards onmethod_exists, so either side can land first).What
ObjectService::runAsAnonymous(callable)— the narrowing twin ofrunAs(). Clears the session subject (setVolatileActiveUser(null)) for the duration of the callable and restores it in afinally, so every reader ofIUserSession::getUser()in the RBAC and organisation layers sees no user: no admin bypass, no_ownergrant, no group rules, noinheritFromPublicwidening. Permission caches are keyed by UID and stay correct by construction.AnonymousEvaluationContext— a static marker. Clearing the subject alone would make the evaluation more permissive under occ/PHPUnit, because two guards trust a call without a user: the CLI bypass in the RBAC/tenancy filters (MagicRbacHandler,MultiTenancyTrait,MagicOrganizationHandler) andSystemOperationContext. Those now yield while the scope is active;SystemOperationContext::isActive()returns false inside it — narrowing wins over elevating.Tests
AnonymousEvaluationContextTest,ObjectServiceRunAsAnonymousTest(mirrorsObjectServiceRunAsTest),MagicRbacHandlerAnonymousScopeTest(CLI SAPI: same empty session bypasses without the scope, is filtered inside it). ExistingSystemOperationContextTest,ObjectServiceRunAsTest,MagicRbacHandlerTestunchanged and green. phpstan/phpcs clean on the touched files.Not in this PR
authorization.inheritFromPublicis untouched: it only widens authenticated callers, and inside the scope there are none.development(see the WOO-578 plan).🤖 Generated with Claude Code