fix(rbac): correct the read-visibility diagnosis and delete the dead gate - #2252
Merged
Merged
Conversation
…gate The proposal merged in #2251 was wrong on both counts, and Phase 0 disproved it. Recording the correction rather than quietly dropping it. What it claimed: - the object read path gates on `create` - OpenRegister cannot express "any authenticated user" What is actually true: - Listing is filtered in SQL by MagicRbacHandler::buildRbacConditionsSql(), which asks for `read`. Correct already. - filterObjectsForPermissions() does gate on `create` — and has NO production caller. Repo-wide the only reference invokes it with _rbac:false, so the check never ran. Dead code that reads as the live gate. - The `authenticated` pseudo-group already exists in all three handlers and is specified in openspec/specs/rbac-scopes/spec.md:178. No new sentinel. Also withdrawn: the concern that filterUuidsForPermissions() gates on `delete`. Its only caller is deleteObjects(), so `delete` is right. The real cause of the openbuild blackout is a non-empty authorization block with no `read` key, which fails closed to owner-only rows by design (MagicRbacHandler:1031). That is an openbuild-side data fix, not an OR change. This commit therefore makes no behavioural change: - deletes filterObjectsForPermissions() and its test, leaving a note at the site so it is not reintroduced - repoints docs/features/organisation-roles.md Read+List at the SQL gate; that doc line is part of why the dead function looked live - rewrites the OpenSpec change, and adds design.md recording both the anomaly's answer and the reasoning error that made it necessary Gates: phpcs clean; phpmd at identical paths goes 3 findings -> 2 (the removal drops TooManyMethods 26->25), none added.
rubenvdlinde
added a commit
to ConductionNL/openbuild
that referenced
this pull request
Aug 1, 2026
) Every openbuild schema declared "authorization": { "create": ["admin"], "update": ["admin"], "delete": ["admin"] } — non-empty, and with no `read` key. OpenRegister treats that as fail-closed: `MagicRbacHandler::buildRbacConditionsSql()` bypasses filtering only for an EMPTY block; a populated block with no `read` rule falls through to the owner condition alone, so every non-admin caller saw zero rows. Not a bug in OR — deliberate, and commented as such at MagicRbacHandler:1031. That is the whole of #76. An owner could grant a colleague editor or viewer on an app and they still saw an empty list, because OR filtered the objects out one layer below openbuild's own permission check. Adds `"read": ["authenticated"]` to all 15 schemas — 6 in the monolith and 9 across the register.d fragments, which were missed by the first pass and would have left business rules, automations, component blocks and the agent workspace owner-only. `authenticated` requires $userId !== null (MagicRbacHandler:414), so anonymous callers are NOT granted. This is intentionally the coarse layer: appinfo/routes.php already documents that OR's schema read rule is a group ACL, not a row filter, and that the per-app `permissions` block is enforced by /api/applications. Both layers verified live. Measured on the disposable instance after a FORCED re-import: caller OR object API openbuild /api/applications admin 21 21 rbac-editor 0 -> 21 1 (granted editor on pw-verchain) rbac-viewer 0 -> 21 1 (granted viewer on pw-verchain) rbac-outsider 0 -> 21 0 (no grant) anonymous 0 401 Diagnosis trail: ConductionNL/openregister#2252. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 174/174 | |||
| npm | ✅ | ✅ 616/616 | |||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-08-01 21:28 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
to ConductionNL/openbuild
that referenced
this pull request
Aug 1, 2026
…n the RBAC suite (#84) * fix(rbac): grant authenticated read on every openbuild schema (#76) Every openbuild schema declared "authorization": { "create": ["admin"], "update": ["admin"], "delete": ["admin"] } — non-empty, and with no `read` key. OpenRegister treats that as fail-closed: `MagicRbacHandler::buildRbacConditionsSql()` bypasses filtering only for an EMPTY block; a populated block with no `read` rule falls through to the owner condition alone, so every non-admin caller saw zero rows. Not a bug in OR — deliberate, and commented as such at MagicRbacHandler:1031. That is the whole of #76. An owner could grant a colleague editor or viewer on an app and they still saw an empty list, because OR filtered the objects out one layer below openbuild's own permission check. Adds `"read": ["authenticated"]` to all 15 schemas — 6 in the monolith and 9 across the register.d fragments, which were missed by the first pass and would have left business rules, automations, component blocks and the agent workspace owner-only. `authenticated` requires $userId !== null (MagicRbacHandler:414), so anonymous callers are NOT granted. This is intentionally the coarse layer: appinfo/routes.php already documents that OR's schema read rule is a group ACL, not a row filter, and that the per-app `permissions` block is enforced by /api/applications. Both layers verified live. Measured on the disposable instance after a FORCED re-import: caller OR object API openbuild /api/applications admin 21 21 rbac-editor 0 -> 21 1 (granted editor on pw-verchain) rbac-viewer 0 -> 21 1 (granted viewer on pw-verchain) rbac-outsider 0 -> 21 0 (no grant) anonymous 0 401 Diagnosis trail: ConductionNL/openregister#2252. * test(e2e): un-skip versionRouting 9.2 and record the measured blockers on the RBAC suite 9.2 — ENABLED, four scenarios replacing one that asserted almost nothing. Two things unblocked it. globalSetup now provisions the rbac-* fixture users and mints one storageState each, so no spec form-logs-in (four consecutive logins is exactly what trips Nextcloud's brute-force throttle) — `loginAs` is gone from this file. And openbuild#76: until every schema carried a `read` rule, non-admins saw ZERO objects, so "viewer gets 404" passed for the wrong reason and no 200 assertion was reachable at all. The old body could not fail meaningfully. It located the schema list with `.ob-schema-list` / `[data-testid="schema-list"]`, neither of which exists in src/, so "must NOT be visible" held on any page including a correct one; and it downgraded a missing not-found UI to a console.warn. Now asserted: - viewer + staging -> 404 with the body pinned EXACTLY, plus a regex check that the envelope names no authorisation reason - non-member + staging -> byte-identical to the viewer's, and identical again to an unknown version slug (that indistinguishability IS REQ-OBVR-003) - editor + staging -> 200 with a manifest. The positive control: without it a broken fixture, a missing chain or a blanket denial all look like a pass - viewer UI -> `.openbuild-schema-list` (the REAL selector) absent, no stack trace Setup grants the roles via grantAppRoles(), which had been merged with no caller — so the editor control now actually exercises an editor. schema-access-scopes-rbac — STAYS SKIPPED, with honest blockers. Its three recorded blockers are all resolved (fixture users exist; the version chain is seeded; the feature and its copy are real — the warning is an NcNoteCard sibling of .openbuild-access-editor, and `.note-stub` never existed). A fourth, found by driving it rather than reading it, is not: the schema designer is unreachable for a non-admin. rbac-editor lands on the first-time setup wizard with `.openbuild-schema-list` count 0, because /api/setup/status is admin-only and useSetupStatus read its 403 as "nothing done". Fixed upstream in ConductionNL/nextcloud-vue#574; unblocks on a published bump. The comment also records a defect found while measuring, filed as #83: availableGroups feeds the dropdown `group:`-prefixed values while authorLockedOut compares bare gids, so the lock-out warning fires for members too — exactly the REQ-OBDSA-004 scenario. Noted so this suite is not simply un-skipped and declared green once the wizard blocker lifts. --------- Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #2251, which this corrects.
The short version
#2251 claimed the object read path gates on
create, and that OpenRegister had no way to express "any authenticated user". Phase 0 disproved both.createMagicRbacHandler::buildRbacConditionsSql(action: 'read'). Correct already.filterObjectsForPermissions()is the list filter_rbac: false, so the check never ran.filterUuidsForPermissions()has the same bug (delete)deleteObjects().deleteis correct. Withdrawn.authenticatedalready exists in all three handlers and is specified inopenspec/specs/rbac-scopes/spec.md:178.What actually caused the openbuild blackout
A non-empty authorization block that omits
readfails closed to owner-only rows — deliberately, and commented as such atMagicRbacHandler:1031. Every openbuild schema carries{create,update,delete: ["admin"]}with noread, so every non-admin saw zero.That is an openbuild-side data fix (
read: ["authenticated"]), not a change to OpenRegister.It also explains the anomaly #2251 filed as an unresolved open question: adding
read: ['rbac-editors']flipped 0 → 21 because it supplied the missing rule to the SQL gate. Nothing consulted thecreategate.What this PR changes
No behavioural change.
filterObjectsForPermissions()and its test, leaving a note at the site so it is not reintroduced.docs/features/organisation-roles.mdRead + List at the SQL gate. That doc line is part of why the dead function looked live.design.mdrecording the anomaly's answer, the cross-handler contract table, and the reasoning error — a code path was assumed live because it was public, documented,@spec-annotated and unit-tested. Onegrepfor callers would have caught it.@specwarnings in the touched file.Verification
phpcsclean at gate settings.phpmdcompared at identical paths: 3 findings → 2. The removal dropsTooManyMethods(26 → 25); none added.🤖 Generated with Claude Code