fix(auth): scope the contact-person read-outs to the caller's organisation (#459) - #466
Merged
Merged
Conversation
…ation (#459) `GET /api/contactpersonen/organisation/{organisationId}` was `@NoAdminRequired` with "is somebody logged in" as its only guard. `$organisationId` is a path parameter and was never compared to the caller's own organisation, so any authenticated user could read the contact persons of any organisation — and the response carries each contact's Nextcloud username, group membership and enabled/disabled state. Every sibling on the same controller that touches that data (`getUserInfo`, `getBulkUserInfo`, `updateUserGroups`, `disableUser`, `enableUser`) already refuses it to non-admins. - `checkOrganisationReadPermission()`: instance admins may read any organisation; everybody else only the organisation their own contactpersoon belongs to; a caller whose organisation cannot be resolved is refused. This mirrors `verifyCrossTenantScope()`, which already fails closed for writes. - The same guard is applied to the sibling route `getContactPersonsWithUserDetailsForOrganization`, named in the issue as having the same shape. - The per-record organisation is re-checked in PHP. The search filter is a bare top-level `organisation` key; whether OpenRegister reads that as an object property, as `@self` metadata, or ignores it is not visible from the call site, and an ignored filter returns an UNSCOPED result set that looks exactly like a scoped one. A record with no resolvable organisation is not served. - `resolveContactOrganisation()` now normalises the stored value. `organisatie` is declared as a related object in the register, so it can arrive as a nested envelope; comparing that raw against a plain UUID read as "different tenant" and denied legitimate members. - The enrichment loop reuses `buildUserInfoData()`, the shape the admin-gated siblings already return — three catalog group memberships rather than every GID the account holds. - `total` now counts what is actually returned instead of the unfiltered server-side total. Can-fail proof: reverting the controller to `origin/development` turns 4 of the 7 new tests red, including the item-level assertion — `victim@b.example` and `org-uuid-B` appear in the response body. The other 3 assert the legitimate surface still works and pass in both directions by design. phpcs lib/: 0 errors / 87 warnings, identical to origin/development. phpmd, psalm, phpstan clean. Unit suite 519 tests green.
Contributor
Quality Report — ConductionNL/softwarecatalog @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| check-vue-demi | ✅ | ||||
| test-l10n | ✅ | ||||
| composer | ✅ | ✅ 128/128 | |||
| npm | ✅ | ✅ 718/718 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-09 14:19 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
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.
Closes #459.
The defect
GET /api/contactpersonen/organisation/{organisationId}is@NoAdminRequiredand its only guard was "is somebody logged in".$organisationIdis a path parameter and was never compared to the caller's own organisation, so any authenticated user could read the contact persons of any organisation — and the response carries each contact's Nextcloud username, group membership and enabled/disabled state.Every sibling on the same controller that touches that data already refuses it to non-admins:
getUserInfo,getBulkUserInfo,updateUserGroups,disableUser,enableUser. This one did not.The fix
verifyCrossTenantScope()already applies to writesApplied to
getContactpersonenand to the siblinggetContactPersonsWithUserDetailsForOrganization, which #459 names as having the same shape.The second, independent problem in #459
The search filter is a bare top-level
'organisation' => $organisationId. Whether OpenRegister reads that as an object-property filter, as@selfmetadata, or ignores it entirely is not visible from the call site — and an ignored filter returns an UNSCOPED result set that looks exactly like a scoped one. Thecontactpersoonschema inlib/Settings/softwarecatalogus_register.jsondeclaresorganisatie, notorganisation, so the filter is at best not the key the schema uses.Rather than guess at the query semantics with no data to measure against (the dev instance holds zero OpenRegister objects), the organisation of every returned record is re-checked in PHP. A filter that fails to scope therefore cannot leak, and a record carrying no resolvable organisation is not served at all.
totalnow counts what is actually returned rather than the unfiltered server-side total.resolveContactOrganisation()also now normalises the stored value:organisatieis a related object, so it can arrive as a nested envelope, and comparing that raw against a plain UUID read as "different tenant" and denied legitimate members.The enrichment loop now reuses
buildUserInfoData()— the shape the admin-gated siblings already return. That reports the three software-catalog group memberships instead of every GID the account holds, so even an authorised read exposes less.Can-fail proof
Reverting only
lib/Controller/ContactpersonenController.phptoorigin/developmentand re-running the new tests:The item-level assertion is on the item, not the envelope: the foreign organisation's username must be absent from the body. The remaining 3 tests assert the legitimate surface still works (own org allowed, admin allowed, nested related-object reference resolved) and pass in both directions by design — that is what makes them a regression net rather than a mirror of the fix.
Measurements
phpcs lib/: 0 errors / 87 warnings — byte-identical toorigin/development.phpmd(with the repo baseline): clean. The first draft trippedTooManyMethods(26) andExcessiveMethodLength(106); both were resolved by removing code — folding two helpers into existing ones and reusingbuildUserInfoData()— not by suppression.psalm: no errors.phpstan: no errors.48c88ba1e0d049f8f38538c33e790d3e603c55d0, full-repo: 13 gates fail, identical toorigin/developmentgate-for-gate, except gate-25 contract-coverage 44 → 43. No count regressed.