fix(rbac): namespace user-UID vs group-GID principals in permissions (closes #37)#52
Merged
Merged
Conversation
…37) Closes #37. Before: `ApplicationsController::collectAuthorisedGroups()` flattened `permissions.{owners,editors,viewers}` into a single deduplicated string list, which `requirePermission()` intersected with the caller's group-GID list (`getUserGroupIds()`). Both lived in the same string namespace, so an Application whose `permissions.owners` contained the literal username `"admin"` was — by coincidence — also satisfied by anyone in the `admin` group GID. The audited admin-bypass branch (REQ-OBRBAC-006) was never reached for that app, and the `openbuilt-rbac` Newman suite's `rbac.admin_bypass` audit row never got written. After: principal entries now encode the namespace in the string itself. `user:<uid>` — matches when the caller's UID equals `<uid>`. `group:<gid>` — matches when one of the caller's group GIDs equals `<gid>`. `<value>` — back-compat: still treated as a group GID. Seeded `owners: ["admin"]` keeps matching the admin group (no behaviour change for existing data). `collectAuthorisedGroups()` returns `{ users: [...], groups: [...] }`; `requirePermission()` checks the caller's UID against `users` first, then the caller's GIDs against `groups`. Either match grants access; both buckets are now independent so a username and a same-named group no longer clash. Schema description in `lib/Settings/openbuilt_register.json` updated to document the three forms (user:uid, group:gid, bare-gid). `listMine()` got the same two-step check at line ~480 so the listing filter stays consistent with the per-app authz gate. PHPCS strict + PHPStan + Psalm all clean on the changed file.
Contributor
Quality Report — ConductionNL/openbuilt @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 432/432 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Coverage: 0% (0/19 statements)
Quality workflow — 2026-05-13 08:50 UTC
Download the full PDF report from the workflow artifacts.
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.
Why
ApplicationsController::collectAuthorisedGroups()flattenedpermissions.{owners,editors,viewers}into a single deduplicated string list, whichrequirePermission()intersected with the caller's group-GID list (getUserGroupIds()). Both lived in the same string namespace, so an Application whosepermissions.ownerscontained the literal username"admin"was — by coincidence — also satisfied by anyone in theadmingroup GID. The audited admin-bypass branch (REQ-OBRBAC-006) was never reached for that app, and theopenbuilt-rbacNewman suite'srbac.admin_bypassaudit row never got written.What changed
Principal entries now encode the namespace in the string itself:
user:<uid><uid>group:<gid><gid><value>owners: ["admin"]keeps matching the admin group (zero behaviour change for existing data)collectAuthorisedGroups()returns{ users: [...], groups: [...] }(was: singlearray<string>).requirePermission()checks the caller's UID againstusersfirst, then the caller's GIDs againstgroups. Either match grants access.listMine()got the same two-step check so the listing filter stays consistent with the per-app authz gate.lib/Settings/openbuilt_register.jsonupdated to document the three forms.Backwards compat
owners: ["admin"](seeded)user:adminowners: ["team-alpha"](typical)owners: ["user:alice"](new)user:alice(nonsense)alice(new capability)The coincidental-match path is gone. An admin-group member trying to access an app whose
permissions.ownersis["admin"]still gets in via the admin-group match — they ALSO go through the audited admin-bypass branch only when neither bucket matches, exactly as the spec intended.Quality gates
vendor/bin/phpcs --standard=phpcs.xml lib/Controller/ApplicationsController.php✓vendor/bin/phpstan analyse lib/Controller/ApplicationsController.php✓vendor/bin/psalm lib/Controller/ApplicationsController.php✓ (no errors)🤖 Generated with Claude Code