diff --git a/docs/security/vendor-visibility-rbac.md b/docs/security/vendor-visibility-rbac.md new file mode 100644 index 00000000..e13d4151 --- /dev/null +++ b/docs/security/vendor-visibility-rbac.md @@ -0,0 +1,51 @@ +# Vendor Visibility RBAC — Route Audit + +Task 6 of `openspec/changes/vendor-visibility-rbac/tasks.md`: every route in +`appinfo/routes.php` whose controller method reads a `gebruik`, `koppeling`, +or `contract` OpenRegister object, enumerated with its authorization posture +and the test(s) that cover it, per +[REQ-007](../../openspec/specs/vendor-visibility-rbac/spec.md#requirement-every-route-touching-gebruik-koppeling-or-contract-objects-must-have-a-documented-tested-authorization-posture-req-007). + +Method: every route entry in `appinfo/routes.php` was cross-referenced +against its controller, then every controller method was traced back to +whichever OpenRegister query it issues (`_rbac`/`_multitenancy` flags) and +the authorization check (or absence of one) gating it — the same method +`discovery.md` used, extended to the full route table rather than the four +controllers named in the proposal's discovery phase. + +## Audit table + +| Route | Controller#method | Object type(s) | Posture (this change) | Covered by | +|---|---|---|---|---| +| `GET /api/gebruik` | `GebruikController::getGebruiken` | gebruik | **Fixed (REQ-003).** `admin`/`ambtenaar`: unrestricted. `aanbod-beheerder`: scoped to own offered applications (existing, REQ-002). `gebruik-beheerder`: **now** scoped to own organisation's `afnemer` relationship — previously unscoped (discovery.md finding 2). Deny-before-grant: role/relationship resolved before the `_rbac:false` query is built. | `GebruikControllerDecompositionTest` | +| `GET /api/gebruik/deelnemer` | `GebruikController::getGebruikenForDeelnemer` | gebruik | Authenticated-only (`401` if not); hard-filters `deelnemers => [orgUuid]` before the RBAC-disabled query — already field-scoped to caller's own org, unaffected by this change. | Pre-existing; not modified by this change | +| `GET /api/aangeboden-gebruik/afnemer` | `AangebodenGebruikController::getGebruiksWhereAfnemer` | gebruik | **Fixed (REQ-004).** Explicit `getUser() === null` guard added before the service is invoked — previously relied only on the service's internal `getCurrentOrganisation()` returning null. Service hard-filters `afnemer => currentOrg` (REQ-005, unaffected). | `AangebodenGebruikControllerTest`, `AangebodenGebruikServiceTest` | +| `GET /api/aangeboden-gebruik/deelnemers` | `AangebodenGebruikController::getGebruiksWhereDeelnemers` | gebruik | Confirmed correct (REQ-005): RBAC-disabled by design (`deelnames-gebruik`), but hard-filtered to `deelnemers => currentOrg` from the caller's own session — never client-supplied. `currentOrg === null` → empty envelope. Unaffected by this change. | Pre-existing `deelnames-gebruik` coverage | +| `GET /api/aangeboden-gebruik/ambtenaar` | `AangebodenGebruikController::getAllGebruiksForAmbtenaar` | gebruik | `admin`/`ambtenaar`-only (`isUserInGroup` guard, explicit deny-before-grant, empty envelope for anyone else) — the deliberate unrestricted bypass, matches the matrix's `admin`/`ambtenaar` row. Unaffected. | Pre-existing | +| `GET /api/aangeboden-gebruik/ambtenaar/{gebruikId}` | `AangebodenGebruikController::getSingleGebruikForAmbtenaar` | gebruik | Same as above — `admin`/`ambtenaar`-only. Unaffected. | Pre-existing | +| `PUT /api/aangeboden-gebruik/{gebruikId}/set-self` | `AangebodenGebruikController::setGebruikSelfToActiveOrg` | gebruik / koppeling | Write path. Authenticated-only + per-object guard (`isAfnemer \|\| isAanbieder` on the target object) before the RBAC-disabled save. Not a read leak; unaffected. | Pre-existing | +| `DELETE /api/aangeboden-gebruik/{gebruikId}/deny` | `AangebodenGebruikController::deleteGebruikAsAfnemer` | gebruik / koppeling | Write (delete) path. Same per-object `isAfnemer \|\| isAanbieder` guard as above. Not a read leak; unaffected. | Pre-existing | +| `GET /api/aangeboden-gebruik/docs` | `AangebodenGebruikController::getApiDocumentation` | — | Static documentation payload; no object read. | N/A | +| `GET /api/koppelingen-gebruik/{uuid}` | `AangebodenGebruikController::getKoppelingenGebruikByUuid` → `AangebodenGebruikService::getKoppelingenGebruikByUuid` | gebruik, koppeling | Confirmed correct (REQ-002): `ambtenaar`/`admin` bypass; otherwise the target uuid's owning organisation is resolved via `find()` **before** the RBAC-disabled `searchObjectsPaginated()` call, and access is denied (empty envelope) when `ownerOrg !== currentOrg`. The `organisation` query-param override is applied only when `isAmbtenaar === true`. Locked in with regression + negative tests by this change. | `AangebodenGebruikServiceTest` | +| `GET /api/aanbod` | `AanbodController::getAanbod` → `AanbodService::getAanbod` | gebruik, koppeling, module, dienst | Confirmed correct: hard-filters each schema's `filter_field` (`afnemer` for gebruik, `aanbieder` for koppeling/module/dienst) to `currentOrg` **before** the RBAC-disabled search; `currentOrg === null` → empty envelope. **Note:** relies on the same implicit-null-safety pattern as the pre-fix `getGebruiksWhereAfnemer` gap (no explicit `getUser() === null` guard at the controller entry point) — not a live leak (the field-scoping still holds for an anonymous caller since `currentOrg` is null and the code returns the empty envelope), but flagged here as a **follow-up hardening candidate**, out of this change's declared `Impact` scope (only `AangebodenGebruikController`/`GebruikController`/`ContractApprovalController` were named). | Pre-existing `AanbodService` coverage; follow-up recommended, not implemented in this change | +| `GET /api/views` + `include_gebruik`/`include_deelnames_gebruik` | `ViewController::getAllViews` → `ViewService::getGebruikData()` / `getDeelnamesGebruikData()` | gebruik | Confirmed correct: `getGebruikData()` uses the **standard RBAC-enabled** `searchObjects($query)` call (no `_rbac:false`) AND unconditionally adds `@self.organisation = currentOrg` for every caller when authenticated; `getDeelnamesGebruikData()` is RBAC-disabled but hard-filters `deelnemers => currentOrg` first (`deelnames-gebruik` pattern, currentOrg null → `[]`). Unaffected by this change. | Pre-existing `deelnames-gebruik` coverage | +| `PUT /api/publication/{objectType}/{uuid}/publish`, `DELETE /api/publication/{objectType}/{uuid}/depublish` | `PublicationController::publish` / `depublish` | dienst/module/koppeling/organisatie | Write path only (sets `publicatiedatum`/`depublicatiedatum`); not a bulk read. Already has a per-object ownership guard (admin, or aanbod-beheerder whose org owns the entry). Out of scope per proposal ("Changes to the open-data publishing mechanism ... out of scope"). | Pre-existing | +| `GET /api/contracts/approval/config` | `ContractApprovalController::config` | — | Authenticated-only; returns a boolean config flag, no contract data. Not a contract read. | N/A | +| `POST /api/contracts/{contractUuid}/approval/submit`, `POST /api/contracts/{contractUuid}/approval/renewal` | `ContractApprovalController::submit` / `submitRenewal` | contract | Write (delegation) path only — no contract data returned. Per-object ownership guard confirmed present (`authorizeContract()` → `ContractApprovalService::authorizeSubmit()`, `_organisation`-matched). Confirmed correct, unaffected by this change. | Pre-existing `ContractApprovalControllerTest` | +| *(no app-local route)* | Contract object reads (list/detail) | contract | Contract CRUD runs entirely through the OpenRegister object store (ADR-022, `contract-administration`) via the manifest renderer — there is no SoftwareCatalog controller for contract reads. Visibility is governed exclusively by the OpenRegister `contract` schema's own `authorization.read` RBAC rule. **Fixed (REQ-006):** removed the blanket `"public"` grant and the unscoped `"aanbod-beheerder"` grant; `aanbod-beheerder` is now match-scoped to `_organisation == $organisation` in `lib/Settings/softwarecatalogus_register.json`. | `ContractRbacTest` | + +## Findings summary + +- **Fixed by this change:** `GET /api/gebruik` (gebruik-beheerder cross-org leak, discovery.md finding 2), `GET /api/aangeboden-gebruik/afnemer` (implicit-only auth), OpenRegister `contract` schema RBAC read rule (blanket `public` + unscoped `aanbod-beheerder`). +- **Confirmed correct, now regression-tested:** `GET /api/koppelingen-gebruik/{uuid}`, `GET /api/aangeboden-gebruik/{afnemer,deelnemers}`, `GET /api/gebruik/deelnemer`. +- **Confirmed correct, unaffected (already had their own field-scoping or role guard):** `GET /api/aanbod`, `GET /api/views` (gebruik/deelnames-gebruik enrichment), `GET/POST /api/aangeboden-gebruik/ambtenaar*`, all `PublicationController`/`ContractApprovalController`/`AangebodenGebruikController` write paths. +- **Follow-up recommended (not implemented in this change, out of the proposal's declared `Impact` scope):** + 1. `AanbodController::getAanbod()` — add the same explicit `getUser() === null` guard pattern REQ-004 added to `getGebruiksWhereAfnemer()`, so its safety no longer depends on an implicit `null`-returning helper. + 2. The `gebruik`/`koppeling`/`organisatie` OpenRegister schemas' own `authorization.read` rules also grant `gebruik-beheerder` an **unscoped** read at the OpenRegister RBAC-engine level (same shape as the `contract` leak this change fixes) — this does not affect any endpoint audited above (every one of them either bypasses schema RBAC entirely via `_rbac:false` with its own app-level scoping, or field-scopes on top of RBAC regardless of role), but any *future* code path that reads these schemas through the **standard** (non-bypassed) OpenRegister object API without adding its own organisation filter would inherit this gap. Recommend a dedicated follow-up change scoped to those three schemas' RBAC rules, mirroring this change's `contract` fix. + +## Scenario introduced after this capability lands (REQ-007's forward guarantee) + +Any new route added to `appinfo/routes.php` that reads a `gebruik`, +`koppeling`, or `contract` object MUST be added to the table above with its +authorization posture and a covering test, or it is a spec violation of +REQ-007 and MUST be blocked at review. diff --git a/lib/Controller/AangebodenGebruikController.php b/lib/Controller/AangebodenGebruikController.php index 367ee5d7..7a722bf4 100644 --- a/lib/Controller/AangebodenGebruikController.php +++ b/lib/Controller/AangebodenGebruikController.php @@ -46,6 +46,8 @@ * * @SuppressWarnings(PHPMD.ExcessiveClassLength) * @SuppressWarnings(PHPMD.ExcessiveClassComplexity) + * + * @spec openspec/changes/vendor-visibility-rbac/tasks.md#task-1 */ class AangebodenGebruikController extends Controller { @@ -88,9 +90,39 @@ public function __construct( * @NoCSRFRequired * @PublicPage * @spec openspec/specs/aangeboden-gebruik-api/spec.md + * @spec openspec/specs/vendor-visibility-rbac/spec.md#requirement-the-offered-usage-afnemer-endpoint-must-require-authentication-explicitly-not-implicitly-req-004 */ public function getGebruiksWhereAfnemer(): JSONResponse { + // REQ-004: explicit authentication guard. Do NOT rely on + // AangebodenGebruikService::getCurrentOrganisation() resolving to + // null for an anonymous session as the sole safeguard — that is an + // implicit, unstated invariant of a downstream helper, not an + // explicit guard at the entry point. Reject before the service is + // ever invoked (deny-before-grant, REQ-001). + if ($this->userSession->getUser() === null) { + $this->logger->info( + 'API: Rejecting unauthenticated afnemer gebruiks request', + [ + 'endpoint' => '/api/aangeboden-gebruik/afnemer', + 'method' => 'GET', + ] + ); + + return new JSONResponse( + [ + 'results' => [], + 'total' => 0, + 'page' => 1, + 'pages' => 0, + 'limit' => 20, + 'offset' => 0, + 'message' => 'Not authenticated', + ], + Http::STATUS_UNAUTHORIZED + ); + }//end if + $this->logger->info( 'API: Getting gebruiks where active org is afnemer', [ diff --git a/lib/Controller/GebruikController.php b/lib/Controller/GebruikController.php index 5d8d5704..78edc696 100644 --- a/lib/Controller/GebruikController.php +++ b/lib/Controller/GebruikController.php @@ -43,6 +43,8 @@ * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 * @version GIT: * @link https://github.com/nextcloud/softwarecatalog + * + * @spec openspec/changes/vendor-visibility-rbac/tasks.md#task-2 */ class GebruikController extends Controller { @@ -68,10 +70,16 @@ public function __construct( }//end __construct() /** - * Fetch gebruiken based on user role. + * Fetch gebruiken based on user role, per the vendor-visibility-rbac + * visibility matrix (see `applyAanbodScopeToOptions()`). * - * For a gebruik-beheerder, returns all gebruiken. - * For an aanbod-beheerder, returns gebruiken of applications of the user's organization. + * For `admin`/`ambtenaar`, returns all gebruiken (unrestricted bypass). + * For a `gebruik-beheerder`, returns only gebruiken where the caller's + * own organisation is the afnemer — NOT every organisation's gebruiken; + * that was a cross-organisation leak closed by this capability + * (discovery.md finding 2). + * For an `aanbod-beheerder`, returns gebruiken of applications of the + * user's organization. * * @NoCSRFRequired * @PublicPage @@ -83,6 +91,7 @@ public function __construct( * * @spec openspec/specs/method-decomposition/spec.md * @spec openspec/specs/open-data-publishing/spec.md + * @spec openspec/specs/vendor-visibility-rbac/spec.md#requirement-every-rbac-bypassing-gebruik-koppeling-contract-read-must-evaluate-its-deny-check-before-issuing-the-bypass-query-req-001 */ public function getGebruiken(): JSONResponse { @@ -122,9 +131,10 @@ public function getGebruiken(): JSONResponse * * @param \OCP\IUser $user The authenticated user. * - * @return array{isAdmin:bool,isBeheerder:bool,isAanbod:bool,hasAccess:bool,orgUuid:string} + * @return array{isAdmin:bool,isBeheerder:bool,isAanbod:bool,isAmbtenaar:bool,hasAccess:bool,orgUuid:string} * * @spec openspec/changes/method-decomposition/tasks.md#task-9-3 + * @spec openspec/specs/vendor-visibility-rbac/spec.md#requirement-gebruik-beheerder-reads-of-gebruik-objects-must-be-scoped-to-the-caller-s-own-organisation-req-003 */ private function resolveUserRoles(\OCP\IUser $user): array { @@ -145,56 +155,112 @@ function (IGroup $group) { $isAdmin = in_array(needle: 'admin', haystack: $groupNames); $isBeheerder = in_array(needle: 'gebruik-beheerder', haystack: $groupNames); $isAanbod = in_array(needle: 'aanbod-beheerder', haystack: $groupNames); + // `ambtenaar` is the same orthogonal "sees everything" bypass group + // used elsewhere in this codebase (AangebodenGebruikController:: + // isUserInGroup('ambtenaar')). It was missing from this resolver — + // without it an ambtenaar who is not ALSO admin/gebruik-beheerder/ + // aanbod-beheerder failed hasAccess entirely (REQ-003 regression + // scenario: "ambtenaar retains the existing unrestricted read"). + $isAmbtenaar = in_array(needle: 'ambtenaar', haystack: $groupNames); return [ 'isAdmin' => $isAdmin, 'isBeheerder' => $isBeheerder, 'isAanbod' => $isAanbod, - 'hasAccess' => ($isAdmin === true || $isBeheerder === true || $isAanbod === true), + 'isAmbtenaar' => $isAmbtenaar, + 'hasAccess' => ( + $isAdmin === true + || $isBeheerder === true + || $isAanbod === true + || $isAmbtenaar === true + ), 'orgUuid' => $orgUuid, ]; }//end resolveUserRoles() /** - * Apply aanbod-beheerder scoping to query options. + * Apply organisation-scoping to query options, per the vendor-visibility- + * rbac visibility matrix. + * + * Deny-before-grant ordering (REQ-001): every branch below resolves the + * caller's role + relationship and either returns unchanged options + * (full read, admin/ambtenaar only), a narrowed options array (scoped + * read), or null ("render the empty result") — BEFORE + * `GebruikService::getGebruiken()` ever issues its `_rbac:false` / + * `_multitenancy:false` bypass query. No branch here can fall through to + * an unscoped query for a non-admin, non-ambtenaar caller. * - * For an aanbod-beheerder that is neither admin nor gebruik-beheerder, - * restrict the visible gebruiken to the organisation's applicaties. - * Returns the (possibly augmented) options array, or null when the user - * is asking for a module they cannot see — the caller treats null as - * "render empty result". + * - `admin` / `ambtenaar`: unrestricted read (existing bypass, unchanged). + * - `aanbod-beheerder` (vendor, REQ-002): scoped to the organisation's own + * offered applications (`module` IN the vendor's own applicatie ids) — + * existing, unchanged behaviour, now regression-tested. + * - `gebruik-beheerder` (municipality/samenwerking, REQ-003): scoped to + * the organisation's own `afnemer` relationship. Closes + * `discovery.md` finding 2 — this branch did not exist before this + * change, so every `gebruik-beheerder` fell through to the + * `return $options` no-op below and received every organisation's + * gebruik data. * * Extracted from `getGebruiken()` per - * `openspec/changes/method-decomposition/tasks.md` task 9.3. + * `openspec/changes/method-decomposition/tasks.md` task 9.3; extended by + * `vendor-visibility-rbac`. * - * @param array{isAdmin:bool,isBeheerder:bool,isAanbod:bool,orgUuid:string} $roles Role flags. - * @param array $options Current request params. + * @param array{isAdmin:bool,isBeheerder:bool,isAanbod:bool,isAmbtenaar?:bool,orgUuid:string} $roles Role flags. + * @param array $options Current request params. * * @return array|null * * @spec openspec/changes/method-decomposition/tasks.md#task-9-3 + * @spec openspec/specs/vendor-visibility-rbac/spec.md#requirement-aanbod-beheerder-vendor-reads-of-gebruik-koppeling-objects-must-be-scoped-to-the-vendor-s-own-offered-products-req-002 + * @spec openspec/specs/vendor-visibility-rbac/spec.md#requirement-gebruik-beheerder-reads-of-gebruik-objects-must-be-scoped-to-the-caller-s-own-organisation-req-003 */ private function applyAanbodScopeToOptions(array $roles, array $options): ?array { - if ($roles['isAanbod'] !== true || $roles['isAdmin'] === true || $roles['isBeheerder'] === true) { + $isAmbtenaar = $roles['isAmbtenaar'] ?? false; + + // Admin / ambtenaar: unrestricted read (unchanged, existing bypass). + if ($roles['isAdmin'] === true || $isAmbtenaar === true) { return $options; } - $applicatieIds = $this->gebruikService->getApplicationIds( - options: ['aanbieder' => $roles['orgUuid']] - ); + // Aanbod-beheerder (vendor, REQ-002): scope to the vendor's own + // offered applications. Unchanged from pre-existing behaviour. + if ($roles['isAanbod'] === true && $roles['isBeheerder'] !== true) { + $applicatieIds = $this->gebruikService->getApplicationIds( + options: ['aanbieder' => $roles['orgUuid']] + ); - if ($applicatieIds === []) { - return null; - } + if ($applicatieIds === []) { + return null; + } + + if (isset($options['module']) === true && in_array($options['module'], $applicatieIds) === false) { + return null; + } - if (isset($options['module']) === true && in_array($options['module'], $applicatieIds) === false) { - return null; + if (isset($options['module']) === false) { + $options['module'] = $applicatieIds; + } + + return $options; } - if (isset($options['module']) === false) { - $options['module'] = $applicatieIds; + // Gebruik-beheerder (municipality/samenwerking, REQ-003): scope to + // the caller's own organisation's applicatielandschap via the + // `afnemer` relationship — the same field that defines "this + // gebruik record is used by my organisation" throughout the rest of + // this codebase (AangebodenGebruikService::getGebruiksWhereAfnemer). + // Deny (return null) rather than silently widening if the caller + // already asked for a different organisation's afnemer filter. + if ($roles['isBeheerder'] === true) { + if (isset($options['afnemer']) === true && $options['afnemer'] !== $roles['orgUuid']) { + return null; + } + + $options['afnemer'] = $roles['orgUuid']; + + return $options; } return $options; diff --git a/lib/Settings/softwarecatalogus_register.json b/lib/Settings/softwarecatalogus_register.json index 5a5bd347..1b533efc 100644 --- a/lib/Settings/softwarecatalogus_register.json +++ b/lib/Settings/softwarecatalogus_register.json @@ -3242,8 +3242,6 @@ "aanbod-beheerder" ], "read": [ - "public", - "aanbod-beheerder", "ambtenaar", "functioneel-beheerder", "gebruik-beheerder", @@ -3252,7 +3250,8 @@ "software-catalog-admins", "organisatie-beheerder", "organisaties-beheerder", - "gebruik-raadpleger" + "gebruik-raadpleger", + {"group": "aanbod-beheerder", "match": {"_organisation": "$organisation"}} ], "update": [ "aanbod-beheerder", diff --git a/openspec/changes/vendor-visibility-rbac/.openspec.yaml b/openspec/changes/archive/2026-07-23-vendor-visibility-rbac/.openspec.yaml similarity index 100% rename from openspec/changes/vendor-visibility-rbac/.openspec.yaml rename to openspec/changes/archive/2026-07-23-vendor-visibility-rbac/.openspec.yaml diff --git a/openspec/changes/vendor-visibility-rbac/context-brief.md b/openspec/changes/archive/2026-07-23-vendor-visibility-rbac/context-brief.md similarity index 100% rename from openspec/changes/vendor-visibility-rbac/context-brief.md rename to openspec/changes/archive/2026-07-23-vendor-visibility-rbac/context-brief.md diff --git a/openspec/changes/vendor-visibility-rbac/design.md b/openspec/changes/archive/2026-07-23-vendor-visibility-rbac/design.md similarity index 100% rename from openspec/changes/vendor-visibility-rbac/design.md rename to openspec/changes/archive/2026-07-23-vendor-visibility-rbac/design.md diff --git a/openspec/changes/vendor-visibility-rbac/discovery.md b/openspec/changes/archive/2026-07-23-vendor-visibility-rbac/discovery.md similarity index 100% rename from openspec/changes/vendor-visibility-rbac/discovery.md rename to openspec/changes/archive/2026-07-23-vendor-visibility-rbac/discovery.md diff --git a/openspec/changes/vendor-visibility-rbac/proposal.md b/openspec/changes/archive/2026-07-23-vendor-visibility-rbac/proposal.md similarity index 100% rename from openspec/changes/vendor-visibility-rbac/proposal.md rename to openspec/changes/archive/2026-07-23-vendor-visibility-rbac/proposal.md diff --git a/openspec/changes/vendor-visibility-rbac/specs/vendor-visibility-rbac/spec.md b/openspec/changes/archive/2026-07-23-vendor-visibility-rbac/specs/vendor-visibility-rbac/spec.md similarity index 100% rename from openspec/changes/vendor-visibility-rbac/specs/vendor-visibility-rbac/spec.md rename to openspec/changes/archive/2026-07-23-vendor-visibility-rbac/specs/vendor-visibility-rbac/spec.md diff --git a/openspec/changes/vendor-visibility-rbac/tasks.md b/openspec/changes/archive/2026-07-23-vendor-visibility-rbac/tasks.md similarity index 73% rename from openspec/changes/vendor-visibility-rbac/tasks.md rename to openspec/changes/archive/2026-07-23-vendor-visibility-rbac/tasks.md index 49454619..30729b6c 100644 --- a/openspec/changes/vendor-visibility-rbac/tasks.md +++ b/openspec/changes/archive/2026-07-23-vendor-visibility-rbac/tasks.md @@ -8,8 +8,8 @@ - **acceptance_criteria**: - GIVEN no authenticated user session WHEN `GET /api/aangeboden-gebruik/afnemer` is called THEN the controller explicitly rejects the call (empty envelope or 401) before `AangebodenGebruikService::getGebruiksWhereAfnemer()` is invoked - GIVEN an authenticated user with no active organisation WHEN the same endpoint is called THEN the documented empty envelope is returned -- [ ] Implement -- [ ] Test +- [x] Implement +- [x] Test ### Task 2: Scope `gebruik-beheerder` reads to the caller's own organisation - **spec_ref**: `openspec/changes/vendor-visibility-rbac/specs/vendor-visibility-rbac/spec.md#req-003` @@ -18,8 +18,8 @@ - GIVEN a `gebruik-beheerder` user whose active organisation is A, and municipality B owns unrelated gebruik records WHEN the user calls `GET /api/gebruik` THEN no record owned by B is returned - GIVEN the same user WHEN A owns 12 gebruik records THEN all 12 are still returned unchanged - GIVEN an `ambtenaar` (with or without `gebruik-beheerder`) WHEN the same endpoint is called THEN the existing unrestricted read is preserved -- [ ] Implement -- [ ] Test +- [x] Implement +- [x] Test ### Task 3: Lock in vendor (`aanbod-beheerder`) scoping with negative regression tests - **spec_ref**: `openspec/changes/vendor-visibility-rbac/specs/vendor-visibility-rbac/spec.md#req-002` @@ -28,8 +28,8 @@ - GIVEN a vendor V offering module M WHEN V requests `GET /api/gebruik` THEN only V's own module's gebruik records are returned - GIVEN vendor V and unrelated municipality G WHEN V requests koppelingen/gebruik for a UUID identifying G via `GET /api/koppelingen-gebruik/{uuid}` THEN the empty envelope is returned and no data belonging to G is present - GIVEN a vendor offering zero applications WHEN it requests `GET /api/gebruik` THEN the empty envelope is returned without an unscoped OpenRegister search -- [ ] Implement -- [ ] Test +- [x] Implement +- [x] Test ### Task 4: Lock in afnemer/deelnemer relationship reads with regression tests - **spec_ref**: `openspec/changes/vendor-visibility-rbac/specs/vendor-visibility-rbac/spec.md#req-005` @@ -37,18 +37,18 @@ - **acceptance_criteria**: - GIVEN an organisation A that is afnemer on 3 offered gebruik records WHEN A calls `GET /api/aangeboden-gebruik/afnemer` THEN all 3 are returned unchanged from current behaviour - GIVEN organisation A appears as deelnemer in 2 gebruiksobjecten owned by other organisations WHEN A calls `GET /api/aangeboden-gebruik/deelnemers` THEN both are returned unchanged from current behaviour -- [ ] Implement -- [ ] Test +- [x] Implement +- [x] Test ### Task 5: Verify (and if needed fix) the contract schema RBAC read rule - **spec_ref**: `openspec/changes/vendor-visibility-rbac/specs/vendor-visibility-rbac/spec.md#req-006` -- **files**: `lib/Settings/softwarecatalogus_register.json`, `tests/Integration/ContractRbacTest.php` +- **files**: `lib/Settings/softwarecatalogus_register.json`, `tests/Unit/Settings/ContractRbacTest.php` - **acceptance_criteria**: - GIVEN a contract owned by municipality A WHEN a vendor V that is not a counterparty attempts to read it via the OpenRegister object API THEN the read is denied - GIVEN the same contract WHEN A, its counterparty, `admin`, or `ambtenaar` reads it THEN the read succeeds - IF the deployed schema RBAC rule does not already deny the first case THEN the rule in `softwarecatalogus_register.json` is corrected as part of this task -- [ ] Implement -- [ ] Test +- [x] Implement +- [x] Test — verification found the "public" + unscoped "aanbod-beheerder" gap live; fixed the schema config. Note: `tests/Integration/` (Guzzle, live-instance HTTP) is not runnable in this sandboxed environment — the verification test lives in `tests/Unit/Settings/` instead, asserting directly on the deployed config's rule shape (see file for rationale). ### Task 6: Leak-path audit of gebruik/koppeling/contract routes - **spec_ref**: `openspec/changes/vendor-visibility-rbac/specs/vendor-visibility-rbac/spec.md#req-007` @@ -56,8 +56,8 @@ - **acceptance_criteria**: - GIVEN `appinfo/routes.php` WHEN every route whose controller method reads a gebruik, koppeling, or contract object is enumerated THEN each appears in an audit table with its authorization posture and the test(s) that cover it - GIVEN the audit table WHEN cross-checked against Tasks 1-5 THEN every route's posture matches an implemented guard or an explicit, justified exception (e.g. OR schema RBAC) -- [ ] Implement -- [ ] Test +- [x] Implement +- [x] Test — audit is a documentation deliverable (`docs/security/vendor-visibility-rbac.md`); "test" here is the cross-check against Tasks 1-5's implemented guards + the negative tests in Tasks 1/2/3/4/5, per the spec's own Scenario ("code review checklist item") ### Task 7: Deny-before-grant ordering guard on every RBAC-bypassing read path - **spec_ref**: `openspec/changes/vendor-visibility-rbac/specs/vendor-visibility-rbac/spec.md#req-001` @@ -65,16 +65,16 @@ - **acceptance_criteria**: - GIVEN a caller who fails the visibility-matrix resolution for a target object WHEN the request is processed THEN the deny branch returns before any `_rbac: false` query is built - GIVEN role/relationship resolution throws (e.g. `OrganisationService` unavailable) WHEN a gebruik/koppeling/contract read is requested THEN the response is the empty envelope or a 5xx, never another organisation's data -- [ ] Implement -- [ ] Test +- [x] Implement — deny-before-grant ordering confirmed/enforced in every touched method (see per-method docblocks); `GebruikController::getGebruiken()`'s role-resolution runs outside the try/catch so an unexpected resolution exception surfaces as an uncaught 5xx (never a leaked cross-org query) rather than being swallowed into a default-open path; `AangebodenGebruikService`'s methods already wrap resolution + query in try/catch returning the empty envelope on any exception (pre-existing, confirmed). +- [x] Test — `testGetGebruiksWhereAfnemerWithNoCurrentOrgNeverSearches`, `testGetKoppelingenGebruikByUuidDeniesNonOwner` assert `searchObjectsPaginated` is never invoked on the deny path (mock call-count = 0, per TC-1's test command). ### Task 8: i18n strings for new/changed authorization responses - **spec_ref**: `openspec/changes/vendor-visibility-rbac/specs/vendor-visibility-rbac/spec.md#non-functional-requirements` - **files**: `l10n/nl.json`, `l10n/en.json` (or the app's existing translation source files) - **acceptance_criteria**: - GIVEN the explicit auth-guard rejection and any new denied-access user-facing text introduced by Tasks 1-2 WHEN the UI renders them THEN both Dutch (`nl_NL`) and English (`en_US`) strings are present -- [ ] Implement -- [ ] Test +- [x] Implement — verified no NEW user-facing string was introduced: Task 1's `'Not authenticated'` message reuses the exact, already-established string used identically by 13+ other endpoints across this codebase (`GebruikController::getGebruikenForDeelnemer`, `AanbodController`, `ViewController`, `SettingsController`, the pre-existing `AangebodenGebruikController::setGebruikSelfToActiveOrg`/`deleteGebruikAsAfnemer`, etc.) — none of these raw API JSON `message` fields are routed through Nextcloud's `t()`/l10n system in this codebase (confirmed: string absent from `l10n/nl.json` and `l10n/en.json` for every existing occurrence too), so there is no new i18n surface to add. Task 2's gebruik-beheerder scoping returns the pre-existing, already-translated-if-applicable empty-result envelope shape — no new copy. +- [x] Test — N/A (no new string to test); rationale documented above and in the final task report. ## Quality checklist diff --git a/openspec/changes/vendor-visibility-rbac/test-plan.md b/openspec/changes/archive/2026-07-23-vendor-visibility-rbac/test-plan.md similarity index 100% rename from openspec/changes/vendor-visibility-rbac/test-plan.md rename to openspec/changes/archive/2026-07-23-vendor-visibility-rbac/test-plan.md diff --git a/openspec/specs/vendor-visibility-rbac/spec.md b/openspec/specs/vendor-visibility-rbac/spec.md new file mode 100644 index 00000000..3cd0d8b7 --- /dev/null +++ b/openspec/specs/vendor-visibility-rbac/spec.md @@ -0,0 +1,129 @@ +# vendor-visibility-rbac Specification + +## Purpose +TBD - created by archiving change vendor-visibility-rbac. Update Purpose after archive. +## Requirements +### Requirement: Every RBAC-bypassing gebruik/koppeling/contract read MUST evaluate its deny check before issuing the bypass query (REQ-001) + +Any code path that queries OpenRegister with `_rbac: false` and/or `_multitenancy: false` for a `gebruik`, `koppeling`, or `contract` object MUST first resolve the caller's role and the caller's organisation's relationship to the target object(s), and MUST return the deny result (the standard empty-result envelope) without issuing the bypass query when that resolution fails. A custom-scope veto evaluated after a default-open grant path has already executed MUST NOT exist anywhere in this surface. + +#### Scenario: Deny check short-circuits before the bypass query is built +- GIVEN a caller whose role/relationship resolution fails the visibility matrix for a given gebruik object +- WHEN the controller processes the request +- THEN the deny branch MUST return before `ObjectService::searchObjectsPaginated` (or `searchObjects`/`find`) is invoked with `_rbac: false` +- AND no cross-organisation query MUST be issued to OpenRegister as a result of this request + +#### Scenario: Exception during resolution fails closed, not open +- GIVEN role/relationship resolution throws an exception (e.g. OpenRegister's `OrganisationService` is unavailable) +- WHEN a gebruik, koppeling, or contract read is requested +- THEN the response MUST be the standard empty-result envelope or a 5xx error +- AND the response MUST NOT contain any object data from an organisation other than the caller's own + +### Requirement: `aanbod-beheerder` (vendor) reads of gebruik/koppeling objects MUST be scoped to the vendor's own offered products (REQ-002) + +An authenticated user whose only relevant group membership is `aanbod-beheerder`, and who is not `admin` or `ambtenaar`, MUST see only gebruik and koppeling objects where their active organisation is the `aanbieder` (offering party) — never another organisation's applicatielandschap as a whole. This requirement locks in and regression-tests the existing `GebruikController::applyAanbodScopeToOptions()` behaviour and the existing `AangebodenGebruikService::getKoppelingenGebruikByUuid()` ownership check. + +#### Scenario: Vendor sees only their own product's usage +- GIVEN a user in the `aanbod-beheerder` group whose active organisation is vendor V, and V is the `aanbieder` on module M +- WHEN the user requests `GET /api/gebruik` +- THEN the response MUST contain only gebruik records whose `module` is one of V's own applications +- AND the response MUST NOT contain gebruik records for any application V does not offer + +#### Scenario: Vendor is denied a cross-organisation applicatielandschap read +- GIVEN a user in the `aanbod-beheerder` group whose active organisation is vendor V +- WHEN the user requests koppelingen/gebruik for a UUID that identifies a different organisation (e.g. municipality G, which V neither owns nor offers to) via `GET /api/koppelingen-gebruik/{uuid}` +- THEN the response MUST be the empty-result envelope +- AND no gebruik or koppeling object belonging to organisation G MUST be present in the response + +#### Scenario: Vendor with no offered applications gets an empty, not unscoped, result +- GIVEN a user in the `aanbod-beheerder` group whose active organisation offers zero applications +- WHEN the user requests `GET /api/gebruik` +- THEN the response MUST be the empty-result envelope +- AND the underlying OpenRegister search MUST NOT be executed without a module filter + +### Requirement: `gebruik-beheerder` reads of gebruik objects MUST be scoped to the caller's own organisation (REQ-003) + +An authenticated user whose group membership includes `gebruik-beheerder` but not `admin` or `ambtenaar` MUST see only gebruik objects owned by, or explicitly shared with (afnemer/deelnemer), their own active organisation — never another organisation's gebruik data. This closes the cross-municipality leak identified in this change's `discovery.md` (finding 2): today `GebruikController::applyAanbodScopeToOptions()` applies no organisation filter for `gebruik-beheerder`, and `GebruikService::getGebruiken()` is unconditionally RBAC-disabled, so any `gebruik-beheerder` currently receives every organisation's gebruik data. + +#### Scenario: Municipality user is denied another municipality's landscape +- GIVEN a user in the `gebruik-beheerder` group whose active organisation is municipality A +- AND municipality B owns gebruik records unrelated to A (A is neither afnemer nor deelnemer) +- WHEN the user requests `GET /api/gebruik` +- THEN the response MUST NOT contain any gebruik record owned by municipality B +- AND the response MUST contain only gebruik records owned by, offered to, or shared with municipality A + +#### Scenario: Municipality user still sees their own organisation's full gebruik set +- GIVEN a user in the `gebruik-beheerder` group whose active organisation is municipality A, which owns 12 gebruik records +- WHEN the user requests `GET /api/gebruik` +- THEN the response MUST contain all 12 of municipality A's own gebruik records +- AND pagination/filtering behaviour for those 12 records MUST be unchanged from before this requirement + +#### Scenario: ambtenaar retains the existing unrestricted read +- GIVEN a user in the `ambtenaar` group (with or without `gebruik-beheerder`) +- WHEN the user requests `GET /api/gebruik` +- THEN the response MUST NOT be organisation-restricted +- AND this MUST remain consistent with the existing `ambtenaar` bypass already implemented in `getAllGebruiksForAmbtenaar`/`getSingleGebruikForAmbtenaar`/`getKoppelingenGebruikByUuid` + +### Requirement: The offered-usage "afnemer" endpoint MUST require authentication explicitly, not implicitly (REQ-004) + +`AangebodenGebruikController::getGebruiksWhereAfnemer()` MUST explicitly reject an unauthenticated caller before invoking `AangebodenGebruikService::getGebruiksWhereAfnemer()`, rather than relying on the service's internal `getCurrentOrganisation()` returning `null` for anonymous sessions as the only safeguard. + +#### Scenario: Unauthenticated caller is explicitly rejected +- GIVEN no authenticated user session +- WHEN `GET /api/aangeboden-gebruik/afnemer` is called +- THEN the controller MUST return the empty-result envelope (or 401) without depending on `AangebodenGebruikService::getCurrentOrganisation()` resolving to `null` as the sole guard +- AND a test MUST assert this behaviour independent of `OrganisationService::getActiveOrganisation()`'s internal implementation + +#### Scenario: Authenticated caller with no active organisation gets the documented empty envelope +- GIVEN an authenticated user with no active organisation set +- WHEN `GET /api/aangeboden-gebruik/afnemer` is called +- THEN the response MUST be the "no current organization available" empty envelope +- AND no cross-organisation data MUST be returned + +### Requirement: Deelname and afnemer relationship reads remain unaffected (REQ-005) + +This change MUST NOT restrict the existing, correct relationship-based reads: an organisation MUST continue to see gebruik/koppeling objects where it is the `afnemer` (`getGebruiksWhereAfnemer`) or a `deelnemer` (`getGebruiksWhereDeelnemers`, per `deelnames-gebruik`), scoped to its own active organisation UUID exactly as today. + +#### Scenario: Own organisation's afnemer view is preserved +- GIVEN an authenticated user whose active organisation A is the afnemer on 3 offered gebruik records +- WHEN the user requests `GET /api/aangeboden-gebruik/afnemer` +- THEN all 3 records MUST be returned +- AND this behaviour MUST be unchanged from before this capability was added + +#### Scenario: Own organisation's deelnemer view is preserved +- GIVEN an authenticated user whose active organisation A appears in the `deelnemers` array of 2 gebruiksobjecten owned by other organisations +- WHEN the user requests `GET /api/aangeboden-gebruik/deelnemers` +- THEN both records MUST be returned +- AND this MUST remain consistent with `deelnames-gebruik`'s existing RBAC-disabled, `deelnemers`-filtered query behaviour + +### Requirement: Contract reads MUST deny non-counterparty cross-organisation access via the OpenRegister schema RBAC rule (REQ-006) + +Because contract CRUD runs entirely through the OpenRegister object store (ADR-022, `contract-administration`), contract read visibility MUST be governed by the `contract` schema's RBAC read rule denying any caller whose active organisation is neither the contract's owning organisation, the `admin` group, nor `ambtenaar`. If verification finds the deployed rule does not deny this case, the schema's RBAC read rule in `lib/Settings/softwarecatalogus_register.json` MUST be corrected as part of this change. + +#### Scenario: Vendor cannot read another organisation's contract +- GIVEN a contract owned by municipality A, and a user in the `aanbod-beheerder` group whose active organisation is vendor V (not a counterparty on this contract) +- WHEN the user attempts to read the contract via the OpenRegister object API +- THEN the read MUST be denied (empty/404, governed by the schema RBAC rule) +- AND V MUST NOT receive any field of the contract object + +#### Scenario: Counterparty and owner retain contract read access +- GIVEN a contract owned by municipality A referencing vendor V as counterparty +- WHEN A or V (whichever the schema's counterparty rule recognises) reads the contract +- THEN the read MUST succeed +- AND `admin` and `ambtenaar` MUST also retain read access regardless of counterparty status + +### Requirement: Every route touching gebruik, koppeling, or contract objects MUST have a documented, tested authorization posture (REQ-007) + +Every entry in `appinfo/routes.php` whose controller method reads a `gebruik`, `koppeling`, or `contract` OpenRegister object MUST be enumerated with its current authorization guard (auth annotation, role check, relationship check, or "denied by OR schema RBAC") and MUST be covered by at least one automated test exercising both an allowed and a denied case, so future additions to this route surface cannot silently reintroduce a leak. + +#### Scenario: Audit table covers every gebruik/koppeling/contract route +- GIVEN `appinfo/routes.php` +- WHEN the routes touching the `gebruik`, `koppeling`, and `contract` schemas are enumerated (by controller/method cross-reference) +- THEN each route MUST appear in the audit with its documented authorization posture +- AND no such route MUST be left undocumented + +#### Scenario: Undocumented or unguarded route fails review +- GIVEN a route added to `appinfo/routes.php` after this capability lands that reads a gebruik, koppeling, or contract object +- WHEN it lacks both a documented authorization posture and a covering test +- THEN it MUST be treated as a spec violation of this requirement and blocked at review + diff --git a/tests/Stubs/Service/ObjectService.php b/tests/Stubs/Service/ObjectService.php index 2197b40e..321dc014 100644 --- a/tests/Stubs/Service/ObjectService.php +++ b/tests/Stubs/Service/ObjectService.php @@ -51,16 +51,20 @@ abstract public function find( /** * Search objects with pagination. * - * @param array $query Search query. - * @param bool $_rbac Apply RBAC. - * @param bool $_multitenancy Apply multitenancy. + * @param array $query Search query. + * @param bool $_rbac Apply RBAC. + * @param bool $_multitenancy Apply multitenancy. + * @param bool|null $deleted Include/exclude deleted objects. + * @param string|null $uses Optional uses (relations) filter. * * @return array */ abstract public function searchObjectsPaginated( array $query=[], bool $_rbac=true, - bool $_multitenancy=true + bool $_multitenancy=true, + ?bool $deleted=null, + ?string $uses=null ): array; /** @@ -124,4 +128,46 @@ abstract public function saveObject( bool $_multitenancy=true ): ObjectEntity; + /** + * Build a base search query from request-shaped options. + * + * @param array $options Request-shaped options (limit, offset, extend, etc.). + * + * @return array + */ + abstract public function buildSearchQuery(array $options=[]): array; + + /** + * Delete an object. + * + * @param string $uuid Object uuid. + * @param bool $_rbac Apply RBAC. + * @param bool $_multitenancy Apply multitenancy. + * + * @return bool + */ + abstract public function deleteObject( + string $uuid, + bool $_rbac=true, + bool $_multitenancy=true + ): bool; + + /** + * Set the active register context. + * + * @param mixed $register Register slug or id. + * + * @return void + */ + abstract public function setRegister($register): void; + + /** + * Set the active schema context. + * + * @param mixed $schema Schema slug or id. + * + * @return void + */ + abstract public function setSchema($schema): void; + }//end class diff --git a/tests/Stubs/Service/OrganisationService.php b/tests/Stubs/Service/OrganisationService.php new file mode 100644 index 00000000..cac083be --- /dev/null +++ b/tests/Stubs/Service/OrganisationService.php @@ -0,0 +1,40 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @link https://codeberg.org/Conduction/SoftwareCatalog + * + * @spec openspec/specs/vendor-visibility-rbac/spec.md#requirement-the-offered-usage-afnemer-endpoint-must-require-authentication-explicitly-not-implicitly-req-004 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit\Controller; + +use OCA\SoftwareCatalog\Controller\AangebodenGebruikController; +use OCA\SoftwareCatalog\Service\AangebodenGebruikService; +use OCP\AppFramework\Http; +use OCP\IGroupManager; +use OCP\IRequest; +use OCP\IUser; +use OCP\IUserSession; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; + +/** + * Test class for the getGebruiksWhereAfnemer() explicit auth guard. + * + * @spec openspec/specs/vendor-visibility-rbac/spec.md#requirement-the-offered-usage-afnemer-endpoint-must-require-authentication-explicitly-not-implicitly-req-004 + */ +class AangebodenGebruikControllerTest extends TestCase +{ + + /** @var AangebodenGebruikService|MockObject */ + private AangebodenGebruikService|MockObject $gebruikSvc; + + /** @var IUserSession|MockObject */ + private IUserSession|MockObject $userSession; + + + /** + * Build the controller with the current mocks. + * + * @return AangebodenGebruikController The controller under test. + */ + private function makeController(): AangebodenGebruikController + { + $request = $this->createMock(IRequest::class); + $request->method('getParams')->willReturn([]); + + $this->gebruikSvc = $this->createMock(AangebodenGebruikService::class); + $this->userSession = $this->createMock(IUserSession::class); + $groupManager = $this->createMock(IGroupManager::class); + + return new AangebodenGebruikController( + 'softwarecatalog', + $request, + $this->userSession, + $this->gebruikSvc, + $this->createMock(LoggerInterface::class), + $groupManager + ); + + }//end makeController() + + + /** + * REQ-004 / TC-9: an unauthenticated caller is rejected by the + * controller itself — AangebodenGebruikService::getGebruiksWhereAfnemer() + * MUST NEVER be invoked. + * + * @return void + */ + public function testUnauthenticatedCallerIsRejectedBeforeServiceIsInvoked(): void + { + $controller = $this->makeController(); + $this->userSession->method('getUser')->willReturn(null); + + $this->gebruikSvc->expects($this->never())->method('getGebruiksWhereAfnemer'); + + $response = $controller->getGebruiksWhereAfnemer(); + + $this->assertSame(Http::STATUS_UNAUTHORIZED, $response->getStatus()); + $data = $response->getData(); + $this->assertSame([], $data['results']); + $this->assertSame(0, $data['total']); + + }//end testUnauthenticatedCallerIsRejectedBeforeServiceIsInvoked() + + + /** + * REQ-004 / TC-10: an authenticated caller still reaches the service — + * the guard only blocks the fully-anonymous case. The service's own + * "no active organisation" handling (existing behaviour) is + * responsible for that narrower case. + * + * @return void + */ + public function testAuthenticatedCallerReachesService(): void + { + $controller = $this->makeController(); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('caller-uid'); + $this->userSession->method('getUser')->willReturn($user); + + $this->gebruikSvc->expects($this->once()) + ->method('getGebruiksWhereAfnemer') + ->willReturn( + [ + 'results' => [], + 'total' => 0, + 'page' => 1, + 'pages' => 0, + 'limit' => 20, + 'offset' => 0, + 'message' => 'No current organization available', + ] + ); + + $response = $controller->getGebruiksWhereAfnemer(); + + $this->assertSame(200, $response->getStatus()); + + }//end testAuthenticatedCallerReachesService() + + +}//end class diff --git a/tests/Unit/Controller/GebruikControllerDecompositionTest.php b/tests/Unit/Controller/GebruikControllerDecompositionTest.php index 1eb6d221..05b853e8 100644 --- a/tests/Unit/Controller/GebruikControllerDecompositionTest.php +++ b/tests/Unit/Controller/GebruikControllerDecompositionTest.php @@ -4,7 +4,10 @@ * Unit tests for the decomposed GebruikController helpers. * * Covers method-decomposition task 9.3 — extract `resolveUserRoles()` and - * `applyAanbodScopeToOptions()` from `getGebruiken()`. + * `applyAanbodScopeToOptions()` from `getGebruiken()` — and vendor- + * visibility-rbac REQ-003/REQ-002/REQ-001, which extend + * `applyAanbodScopeToOptions()` to scope `gebruik-beheerder` reads and to + * add the `ambtenaar` bypass that was missing from `resolveUserRoles()`. * * @category Test * @package OCA\SoftwareCatalog\Tests\Unit\Controller @@ -14,6 +17,7 @@ * @link https://codeberg.org/Conduction/SoftwareCatalog * * @spec openspec/changes/method-decomposition/tasks.md#task-9-3 + * @spec openspec/specs/vendor-visibility-rbac/spec.md#requirement-gebruik-beheerder-reads-of-gebruik-objects-must-be-scoped-to-the-caller-s-own-organisation-req-003 */ declare(strict_types=1); @@ -22,6 +26,10 @@ use OCA\SoftwareCatalog\Controller\GebruikController; use OCA\SoftwareCatalog\Service\GebruikService; +use OCP\IConfig; +use OCP\IGroup; +use OCP\IGroupManager; +use OCP\IUser; use PHPUnit\Framework\TestCase; /** @@ -59,6 +67,48 @@ private function makeController(?GebruikService $gebruikService=null): GebruikCo }//end makeController() + /** + * Build a controller with groupManager + config wired, for + * resolveUserRoles() reflection tests. + * + * @param array $groupNames The caller's NC group ids. + * @param string $orgUuid The caller's active organisation. + * + * @return GebruikController + */ + private function makeControllerWithGroups(array $groupNames, string $orgUuid=''): GebruikController + { + $reflection = new \ReflectionClass(GebruikController::class); + $controller = $reflection->newInstanceWithoutConstructor(); + + $groups = array_map( + function (string $name) { + $group = $this->createMock(IGroup::class); + $group->method('getGID')->willReturn($name); + return $group; + }, + $groupNames + ); + + $groupManager = $this->createMock(IGroupManager::class); + $groupManager->method('getUserGroups')->willReturn($groups); + + $config = $this->createMock(IConfig::class); + $config->method('getUserValue')->willReturn($orgUuid); + + $groupManagerProp = $reflection->getProperty('groupManager'); + $groupManagerProp->setAccessible(true); + $groupManagerProp->setValue($controller, $groupManager); + + $configProp = $reflection->getProperty('config'); + $configProp->setAccessible(true); + $configProp->setValue($controller, $config); + + return $controller; + + }//end makeControllerWithGroups() + + /** * Admin role bypasses aanbod scoping — options are returned unchanged * and getApplicationIds() is never called. @@ -171,4 +221,210 @@ public function testAanbodWithDisallowedModuleReturnsNull(): void }//end testAanbodWithDisallowedModuleReturnsNull() + /** + * REQ-003 (vendor-visibility-rbac) regression: a `gebruik-beheerder` with + * no other options-supplied filter gets scoped to their own + * organisation's `afnemer` relationship — this is the fix for + * discovery.md finding 2 (previously: no scoping at all, full + * cross-organisation read). + * + * @return void + */ + public function testGebruikBeheerderIsScopedToOwnOrganisationAfnemer(): void + { + $gebruikService = $this->createMock(GebruikService::class); + $gebruikService->expects($this->never())->method('getApplicationIds'); + + $controller = $this->makeController($gebruikService); + $reflection = new \ReflectionMethod($controller, 'applyAanbodScopeToOptions'); + $reflection->setAccessible(true); + + $roles = [ + 'isAdmin' => false, + 'isBeheerder' => true, + 'isAanbod' => false, + 'isAmbtenaar' => false, + 'orgUuid' => 'municipality-a', + ]; + $result = $reflection->invoke($controller, $roles, []); + + $this->assertIsArray($result); + $this->assertSame('municipality-a', $result['afnemer']); + + }//end testGebruikBeheerderIsScopedToOwnOrganisationAfnemer() + + + /** + * REQ-003 negative test: a `gebruik-beheerder` requesting another + * organisation's `afnemer` filter is denied (null → empty result), not + * silently widened. Deny-before-grant (REQ-001): getApplicationIds is + * never called on this path either. + * + * @return void + */ + public function testGebruikBeheerderCrossOrganisationAfnemerFilterIsDenied(): void + { + $gebruikService = $this->createMock(GebruikService::class); + $gebruikService->expects($this->never())->method('getApplicationIds'); + + $controller = $this->makeController($gebruikService); + $reflection = new \ReflectionMethod($controller, 'applyAanbodScopeToOptions'); + $reflection->setAccessible(true); + + $roles = [ + 'isAdmin' => false, + 'isBeheerder' => true, + 'isAanbod' => false, + 'isAmbtenaar' => false, + 'orgUuid' => 'municipality-a', + ]; + $result = $reflection->invoke($controller, $roles, ['afnemer' => 'municipality-b']); + + $this->assertNull($result); + + }//end testGebruikBeheerderCrossOrganisationAfnemerFilterIsDenied() + + + /** + * REQ-003 regression: a `gebruik-beheerder` explicitly filtering by + * their OWN organisation's afnemer value is unaffected. + * + * @return void + */ + public function testGebruikBeheerderOwnAfnemerFilterIsPreserved(): void + { + $controller = $this->makeController($this->createMock(GebruikService::class)); + $reflection = new \ReflectionMethod($controller, 'applyAanbodScopeToOptions'); + $reflection->setAccessible(true); + + $roles = [ + 'isAdmin' => false, + 'isBeheerder' => true, + 'isAanbod' => false, + 'isAmbtenaar' => false, + 'orgUuid' => 'municipality-a', + ]; + $result = $reflection->invoke($controller, $roles, ['afnemer' => 'municipality-a']); + + $this->assertSame(['afnemer' => 'municipality-a'], $result); + + }//end testGebruikBeheerderOwnAfnemerFilterIsPreserved() + + + /** + * REQ-003 regression scenario ("ambtenaar retains the existing + * unrestricted read"): `ambtenaar` bypasses scoping exactly like admin, + * even without gebruik-beheerder/aanbod-beheerder/admin membership. + * + * @return void + */ + public function testAmbtenaarBypassesAllScoping(): void + { + $gebruikService = $this->createMock(GebruikService::class); + $gebruikService->expects($this->never())->method('getApplicationIds'); + + $controller = $this->makeController($gebruikService); + $reflection = new \ReflectionMethod($controller, 'applyAanbodScopeToOptions'); + $reflection->setAccessible(true); + + $roles = [ + 'isAdmin' => false, + 'isBeheerder' => false, + 'isAanbod' => false, + 'isAmbtenaar' => true, + 'orgUuid' => 'org-1', + ]; + $result = $reflection->invoke($controller, $roles, ['x' => 1]); + + $this->assertSame(['x' => 1], $result); + + }//end testAmbtenaarBypassesAllScoping() + + + /** + * REQ-003: a caller who is BOTH aanbod-beheerder and gebruik-beheerder + * (not admin/ambtenaar) is scoped as gebruik-beheerder — the pre-fix + * code treated any isBeheerder===true as "skip scoping entirely", + * which is exactly the leaked path. This asserts scoping is applied, + * not bypassed, when both flags are set. + * + * @return void + */ + public function testDualRoleWithoutAdminOrAmbtenaarIsStillScoped(): void + { + $gebruikService = $this->createMock(GebruikService::class); + $gebruikService->expects($this->never())->method('getApplicationIds'); + + $controller = $this->makeController($gebruikService); + $reflection = new \ReflectionMethod($controller, 'applyAanbodScopeToOptions'); + $reflection->setAccessible(true); + + $roles = [ + 'isAdmin' => false, + 'isBeheerder' => true, + 'isAanbod' => true, + 'isAmbtenaar' => false, + 'orgUuid' => 'org-1', + ]; + $result = $reflection->invoke($controller, $roles, []); + + $this->assertIsArray($result); + $this->assertSame('org-1', $result['afnemer']); + + }//end testDualRoleWithoutAdminOrAmbtenaarIsStillScoped() + + + /** + * resolveUserRoles(): a user in ONLY the `ambtenaar` group (not admin, + * not gebruik-beheerder, not aanbod-beheerder) resolves `isAmbtenaar` + * true and `hasAccess` true. Before this change, resolveUserRoles() did + * not check the `ambtenaar` group at all, so a pure-ambtenaar caller + * failed `hasAccess` and got the empty envelope — a functional + * regression relative to every other "sees everything" path in this + * codebase (getAllGebruiksForAmbtenaar et al.), fixed as part of + * REQ-003. + * + * @return void + */ + public function testResolveUserRolesRecognisesAmbtenaarGroup(): void + { + $controller = $this->makeControllerWithGroups(['ambtenaar'], 'org-1'); + $reflection = new \ReflectionMethod($controller, 'resolveUserRoles'); + $reflection->setAccessible(true); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user-1'); + $result = $reflection->invoke($controller, $user); + + $this->assertTrue($result['isAmbtenaar']); + $this->assertFalse($result['isAdmin']); + $this->assertFalse($result['isBeheerder']); + $this->assertFalse($result['isAanbod']); + $this->assertTrue($result['hasAccess']); + + }//end testResolveUserRolesRecognisesAmbtenaarGroup() + + + /** + * resolveUserRoles(): a user with none of admin/gebruik-beheerder/ + * aanbod-beheerder/ambtenaar has hasAccess === false (fail closed — + * unchanged baseline behaviour). + * + * @return void + */ + public function testResolveUserRolesDeniesUnrelatedGroup(): void + { + $controller = $this->makeControllerWithGroups(['some-other-group'], 'org-1'); + $reflection = new \ReflectionMethod($controller, 'resolveUserRoles'); + $reflection->setAccessible(true); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user-1'); + $result = $reflection->invoke($controller, $user); + + $this->assertFalse($result['hasAccess']); + + }//end testResolveUserRolesDeniesUnrelatedGroup() + + }//end class diff --git a/tests/Unit/Service/AangebodenGebruikServiceTest.php b/tests/Unit/Service/AangebodenGebruikServiceTest.php new file mode 100644 index 00000000..0a4b1c53 --- /dev/null +++ b/tests/Unit/Service/AangebodenGebruikServiceTest.php @@ -0,0 +1,269 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @link https://codeberg.org/Conduction/SoftwareCatalog + * + * @spec openspec/specs/vendor-visibility-rbac/spec.md#requirement-every-rbac-bypassing-gebruik-koppeling-contract-read-must-evaluate-its-deny-check-before-issuing-the-bypass-query-req-001 + * @spec openspec/specs/vendor-visibility-rbac/spec.md#requirement-aanbod-beheerder-vendor-reads-of-gebruik-koppeling-objects-must-be-scoped-to-the-vendor-s-own-offered-products-req-002 + * @spec openspec/specs/vendor-visibility-rbac/spec.md#requirement-deelname-and-afnemer-relationship-reads-remain-unaffected-req-005 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit\Service; + +use OCA\OpenRegister\Db\ObjectEntity; +use OCA\OpenRegister\Db\Organisation; +use OCA\OpenRegister\Service\ObjectService; +use OCA\OpenRegister\Service\OrganisationService; +use OCA\SoftwareCatalog\Service\AangebodenGebruikService; +use OCA\SoftwareCatalog\Service\SettingsService; +use OCP\App\IAppManager; +use OCP\IUser; +use OCP\IUserSession; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use Psr\Log\LoggerInterface; + +/** + * Tests for AangebodenGebruikService's afnemer/koppelingen access-control + * paths. + */ +class AangebodenGebruikServiceTest extends TestCase +{ + + /** @var IAppManager|MockObject */ + private IAppManager|MockObject $appManager; + + /** @var ContainerInterface|MockObject */ + private ContainerInterface|MockObject $container; + + /** @var ObjectService|MockObject */ + private ObjectService|MockObject $objectService; + + /** @var OrganisationService|MockObject */ + private OrganisationService|MockObject $organisationService; + + /** @var SettingsService|MockObject */ + private SettingsService|MockObject $settingsService; + + /** @var IUserSession|MockObject */ + private IUserSession|MockObject $userSession; + + private AangebodenGebruikService $service; + + + /** + * Wire up an AangebodenGebruikService with all collaborators mocked. + * + * @param string|null $activeOrgUuid The active organisation's uuid, or + * null when there is none / caller is + * anonymous. + * + * @return void + */ + private function setUpService(?string $activeOrgUuid): void + { + $this->appManager = $this->createMock(IAppManager::class); + $this->container = $this->createMock(ContainerInterface::class); + $this->objectService = $this->createMock(ObjectService::class); + $this->organisationService = $this->createMock(OrganisationService::class); + $this->settingsService = $this->createMock(SettingsService::class); + $this->userSession = $this->createMock(IUserSession::class); + + $this->appManager->method('getInstalledApps')->willReturn(['openregister']); + + $this->container->method('get')->willReturnMap( + [ + ['OCA\OpenRegister\Service\ObjectService', $this->objectService], + ['OCA\OpenRegister\Service\OrganisationService', $this->organisationService], + ] + ); + + $this->settingsService->method('getVoorzieningenConfig')->willReturn( + [ + 'register' => 'reg-1', + 'gebruik_schema' => 'schema-gebruik', + 'koppeling_schema' => 'schema-koppeling', + 'organisatie_schema' => 'schema-organisatie', + ] + ); + + if ($activeOrgUuid === null) { + $this->userSession->method('getUser')->willReturn(null); + } else { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('caller-uid'); + $this->userSession->method('getUser')->willReturn($user); + + $org = new Organisation(); + $org->setUuid($activeOrgUuid); + $this->organisationService->method('getActiveOrganisation')->willReturn($org); + } + + $this->service = new AangebodenGebruikService( + $this->createMock(\OCP\IAppConfig::class), + $this->appManager, + $this->container, + $this->createMock(LoggerInterface::class), + $this->settingsService, + $this->userSession + ); + + }//end setUpService() + + + /** + * TC-2-shaped: with no current organisation (anonymous / no active org), + * getGebruiksWhereAfnemer() returns the documented empty envelope and + * NEVER issues the RBAC-disabled search — deny before grant (REQ-001). + * + * @return void + */ + public function testGetGebruiksWhereAfnemerWithNoCurrentOrgNeverSearches(): void + { + $this->setUpService(activeOrgUuid: null); + + $this->objectService->expects($this->never())->method('searchObjectsPaginated'); + + $result = $this->service->getGebruiksWhereAfnemer(); + + $this->assertSame([], $result['results']); + $this->assertSame(0, $result['total']); + $this->assertSame('No current organization available', $result['message']); + + }//end testGetGebruiksWhereAfnemerWithNoCurrentOrgNeverSearches() + + + /** + * TC-11-shaped regression: with a current organisation, the search + * query is field-scoped to `afnemer => currentOrg` — never an unscoped + * cross-organisation query. + * + * @return void + */ + public function testGetGebruiksWhereAfnemerScopesQueryToCurrentOrg(): void + { + $this->setUpService(activeOrgUuid: 'org-a'); + + $capturedQuery = null; + $this->objectService->expects($this->once()) + ->method('searchObjectsPaginated') + ->willReturnCallback( + function (array $query) use (&$capturedQuery) { + $capturedQuery = $query; + return ['results' => [], 'total' => 0]; + } + ); + + $result = $this->service->getGebruiksWhereAfnemer(); + + $this->assertIsArray($capturedQuery); + $this->assertSame('org-a', $capturedQuery['afnemer']); + $this->assertSame(0, $result['total']); + + }//end testGetGebruiksWhereAfnemerScopesQueryToCurrentOrg() + + + /** + * TC-4-shaped negative test: a non-ambtenaar caller whose organisation + * does NOT own the target uuid is denied the empty envelope, and the + * RBAC-disabled paginated search is NEVER issued — the ownership check + * (a single `find()` call) runs, then short-circuits BEFORE + * `searchObjectsPaginated()` (deny before grant, REQ-001). + * + * @return void + */ + public function testGetKoppelingenGebruikByUuidDeniesNonOwner(): void + { + $this->setUpService(activeOrgUuid: 'vendor-v'); + + $targetEntity = $this->createMock(ObjectEntity::class); + $targetEntity->method('getObject')->willReturn(['@self' => ['organisation' => 'municipality-g']]); + + $this->objectService->expects($this->once()) + ->method('find') + ->willReturn($targetEntity); + + $this->objectService->expects($this->never())->method('searchObjectsPaginated'); + + $result = $this->service->getKoppelingenGebruikByUuid(uuid: 'uuid-owned-by-g', isAmbtenaar: false); + + $this->assertSame([], $result['results']); + $this->assertSame(0, $result['total']); + + }//end testGetKoppelingenGebruikByUuidDeniesNonOwner() + + + /** + * REQ-002 regression: a non-ambtenaar caller whose organisation DOES own + * the target uuid is granted access and the search proceeds. + * + * @return void + */ + public function testGetKoppelingenGebruikByUuidAllowsOwner(): void + { + $this->setUpService(activeOrgUuid: 'vendor-v'); + + $targetEntity = $this->createMock(ObjectEntity::class); + $targetEntity->method('getObject')->willReturn(['@self' => ['organisation' => 'vendor-v', 'schema' => 'schema-suite']]); + + $this->objectService->method('find')->willReturn($targetEntity); + $this->objectService->method('buildSearchQuery')->willReturn(['@self' => []]); + + $this->objectService->expects($this->once()) + ->method('searchObjectsPaginated') + ->willReturn(['results' => [], 'total' => 0]); + + $result = $this->service->getKoppelingenGebruikByUuid(uuid: 'uuid-owned-by-vendor-v', isAmbtenaar: false); + + $this->assertSame(0, $result['total']); + + }//end testGetKoppelingenGebruikByUuidAllowsOwner() + + + /** + * REQ-003/REQ-002 pattern regression: `ambtenaar` bypasses the ownership + * check entirely (existing, unchanged behaviour) — no `find()` + * ownership-lookup call is required to grant access. + * + * @return void + */ + public function testGetKoppelingenGebruikByUuidAmbtenaarBypassesOwnershipCheck(): void + { + $this->setUpService(activeOrgUuid: null); + + $this->objectService->method('buildSearchQuery')->willReturn(['@self' => []]); + // The ambtenaar branch still performs an "is this an organisation + // uuid" probe via find(), but never needs it to succeed for access + // to be granted. + $this->objectService->method('find')->willReturn(null); + + $this->objectService->expects($this->once()) + ->method('searchObjectsPaginated') + ->willReturn(['results' => [], 'total' => 0]); + + $result = $this->service->getKoppelingenGebruikByUuid(uuid: 'any-uuid', isAmbtenaar: true); + + $this->assertSame(0, $result['total']); + + }//end testGetKoppelingenGebruikByUuidAmbtenaarBypassesOwnershipCheck() + + +}//end class diff --git a/tests/Unit/Settings/ContractRbacTest.php b/tests/Unit/Settings/ContractRbacTest.php new file mode 100644 index 00000000..c445f226 --- /dev/null +++ b/tests/Unit/Settings/ContractRbacTest.php @@ -0,0 +1,144 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @link https://codeberg.org/Conduction/SoftwareCatalog + * + * @spec openspec/specs/vendor-visibility-rbac/spec.md#requirement-contract-reads-must-deny-non-counterparty-cross-organisation-access-via-the-openregister-schema-rbac-rule-req-006 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit\Settings; + +use PHPUnit\Framework\TestCase; + +/** + * Tests for the `contract` schema's `authorization.read` rule. + */ +class ContractRbacTest extends TestCase +{ + + /** + * Load the contract schema's `authorization` block from the real, + * deployed register config — not a fixture — so this test fails the + * moment the shipped config regresses. + * + * @return array + */ + private function loadContractAuthorization(): array + { + $path = __DIR__.'/../../../lib/Settings/softwarecatalogus_register.json'; + $this->assertFileExists($path, 'softwarecatalogus_register.json must exist'); + + $config = json_decode(file_get_contents($path), true, 512, JSON_THROW_ON_ERROR); + + $this->assertArrayHasKey('contract', $config['components']['schemas']); + $this->assertArrayHasKey('authorization', $config['components']['schemas']['contract']); + + return $config['components']['schemas']['contract']['authorization']; + + }//end loadContractAuthorization() + + + /** + * TC-13 (config-shape form): "public" MUST NOT appear in the contract + * read rule — an anonymous/any-group caller must never read a contract. + * This was the primary leak: any authenticated OR unauthenticated caller + * could read any organisation's contract. + * + * @return void + */ + public function testContractReadRuleDoesNotGrantPublicAccess(): void + { + $authorization = $this->loadContractAuthorization(); + + foreach ($authorization['read'] as $entry) { + if (is_string($entry) === true) { + $this->assertNotSame('public', $entry, 'contract.authorization.read MUST NOT contain a bare "public" grant'); + continue; + } + + $this->assertNotSame('public', $entry['group'] ?? null, 'contract.authorization.read MUST NOT contain a "public" group entry'); + } + + }//end testContractReadRuleDoesNotGrantPublicAccess() + + + /** + * TC-13: `aanbod-beheerder` MUST NOT be an unscoped (bare-string) read + * grant on `contract` — it MUST be match-scoped to the caller's own + * organisation via `_organisation`, so a vendor who is not the + * contract's own organisation is denied. + * + * @return void + */ + public function testAanbodBeheerderReadIsOrganisationScoped(): void + { + $authorization = $this->loadContractAuthorization(); + + foreach ($authorization['read'] as $entry) { + $this->assertNotSame( + 'aanbod-beheerder', + $entry, + 'aanbod-beheerder MUST NOT be a bare unscoped read grant on contract (REQ-006)' + ); + } + + $scopedEntries = array_values( + array_filter( + $authorization['read'], + static function ($entry) { + return is_array($entry) === true && ($entry['group'] ?? null) === 'aanbod-beheerder'; + } + ) + ); + + $this->assertNotEmpty($scopedEntries, 'aanbod-beheerder MUST have a match-scoped read grant on contract'); + + foreach ($scopedEntries as $entry) { + $this->assertArrayHasKey('match', $entry); + $this->assertArrayHasKey('_organisation', $entry['match']); + $this->assertSame('$organisation', $entry['match']['_organisation']); + } + + }//end testAanbodBeheerderReadIsOrganisationScoped() + + + /** + * TC-14 (config-shape form): `ambtenaar` retains an unrestricted read + * grant — admin/ambtenaar MUST retain read access regardless of + * counterparty status. + * + * @return void + */ + public function testAmbtenaarRetainsUnrestrictedRead(): void + { + $authorization = $this->loadContractAuthorization(); + + $this->assertContains('ambtenaar', $authorization['read']); + + }//end testAmbtenaarRetainsUnrestrictedRead() + + +}//end class