From f4e238d3d50ea86886e372dbe8cd6337240a5fa6 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Fri, 24 Jul 2026 15:08:58 +0200 Subject: [PATCH 1/6] =?UTF-8?q?docs(openspec):=20catalog-ratings=20proposa?= =?UTF-8?q?l,=20design,=20specs=20=E2=80=94=20archived?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Full OpenSpec cycle for closing the beoordeeling (review) schema's authorization hole and shipping moderated ratings/testimonials (softwarecatalog#375). Archives the change and lands the canonical capability spec at openspec/specs/catalog-ratings/spec.md (10 requirements, each with positive + negative scenarios). --- .../2026-07-24-catalog-ratings/.openspec.yaml | 2 + .../context-brief.md | 38 +++ .../2026-07-24-catalog-ratings/design.md | 221 +++++++++++++++++ .../2026-07-24-catalog-ratings/proposal.md | 166 +++++++++++++ .../specs/catalog-ratings/spec.md | 224 ++++++++++++++++++ .../2026-07-24-catalog-ratings/tasks.md | 84 +++++++ .../2026-07-24-catalog-ratings/test-plan.md | 145 ++++++++++++ openspec/specs/catalog-ratings/spec.md | 193 +++++++++++++++ 8 files changed, 1073 insertions(+) create mode 100644 openspec/changes/archive/2026-07-24-catalog-ratings/.openspec.yaml create mode 100644 openspec/changes/archive/2026-07-24-catalog-ratings/context-brief.md create mode 100644 openspec/changes/archive/2026-07-24-catalog-ratings/design.md create mode 100644 openspec/changes/archive/2026-07-24-catalog-ratings/proposal.md create mode 100644 openspec/changes/archive/2026-07-24-catalog-ratings/specs/catalog-ratings/spec.md create mode 100644 openspec/changes/archive/2026-07-24-catalog-ratings/tasks.md create mode 100644 openspec/changes/archive/2026-07-24-catalog-ratings/test-plan.md create mode 100644 openspec/specs/catalog-ratings/spec.md diff --git a/openspec/changes/archive/2026-07-24-catalog-ratings/.openspec.yaml b/openspec/changes/archive/2026-07-24-catalog-ratings/.openspec.yaml new file mode 100644 index 00000000..f4dd94eb --- /dev/null +++ b/openspec/changes/archive/2026-07-24-catalog-ratings/.openspec.yaml @@ -0,0 +1,2 @@ +schema: conduction +created: 2026-07-24 diff --git a/openspec/changes/archive/2026-07-24-catalog-ratings/context-brief.md b/openspec/changes/archive/2026-07-24-catalog-ratings/context-brief.md new file mode 100644 index 00000000..e9f9b878 --- /dev/null +++ b/openspec/changes/archive/2026-07-24-catalog-ratings/context-brief.md @@ -0,0 +1,38 @@ +# Context Brief: catalog-ratings + +## What +Turn the dormant `beoordeeling` (review) schema into a working, **moderated** ratings-and-testimonials feature — and close the authorization hole it currently ships with. Closes softwarecatalog#375. + +## Why (evidence) +- VNG Softwarecatalogus issue **#49** — peer municipalities want experiences/ratings when selecting software. Selecting software on peer experience is a core catalog job-to-be-done. +- Competitive: peer-comparison is exactly the value the centralised GEMMA registry cannot easily offer. + +## 🔴 Security gap to close (found 2026-07-24 — do this even if the feature is trimmed) +The `beoordeeling` schema today has: +- `authorization: {"read": ["public"]}` — **no create / update / delete rules at all** +- **no author field and no owner/organisation field** +- its own description says it is *"onderdeel van het vastgestelde datamodel maar wordt niet daadwerkelijk in de applicatie gebruikt"* + +So reviews are world-readable with undefined write rules and no attributable author. Shipping a ratings UI on top of that without fixing it would be irresponsible. Properties today: `naam`, `beschrijvingKort`, `beschrijvingLang`, `waardering` (the rating), `modules`, `diensten`, `koppelingen`, `gebruik`. + +Also latent: the manifest's `Reviews` index declares an **`auteur` column that does not exist on the schema** — a dead column to fix. + +## Scope +IN: +- Schema: add author binding (the submitting user) + owning organisation; explicit `authorization` create/update/delete rules (author or org-admin may edit their own; deletion restricted); keep public read ONLY for **approved** reviews. +- A `status`/moderation field with an approval workflow, reusing the existing `ModerationQueue.vue` pattern already built for anonymous organisation registration — do not invent a second moderation mechanism. +- Submit-a-review flow (rating + testimonial) from a module/dienst detail page. +- Aggregate rating display (average + count) on module/dienst detail, and the `auteur` column fixed on the Reviews index. +- i18n (EN keys + nl + en_US), unit tests, docs. + +OUT: cross-organisation reputation scoring; review replies/threads; notifying vendors of new reviews (a notification rule already exists for reviews — reuse, don't extend); anonymous public review submission. + +## Design constraints +- **Register changes go in a NEW `lib/Settings/register.d/catalog-ratings.json` FRAGMENT — never edit the monolith.** Per ADR-037 (`lib/Settings/register.d/README.md`). The import version is computed from `info.version` + a hash of the `register.d/*.json` fragments, so **a monolith edit is a silent no-op on every installed instance** (softwarecatalog#391). +- **Fail closed**: unapproved reviews must not be publicly readable. Known OR trap (or#2025) — a veto evaluated AFTER a default-open grant is dead code, so deny before any grant. `publish` is RBAC, not a self-serve flag. +- Author identity must come from the server session, never from client-supplied input (else anyone can forge an author). +- ADR-001 OpenRegister storage only; ADR-008 layering; ADR-012 Cn components (modals in their own file; `NcSelect` needs `inputLabel`). +- 🔑 Register object types in the store by **schema SLUG** against `voorzieningenConfig.register` (the `useSelfFetchList.js` pattern) — several `voorzieningen_config._schema` keys are never populated; that exact mistake made the portfolio-report org picker dead (sc#392). +- Security change ⇒ `hydra-gate-security-change-has-tests` requires tests; include NEGATIVE tests (unapproved review not publicly readable; non-author cannot edit someone else's review). +- Spec deltas: `### Requirement: ` headers; MUST/SHALL on the FIRST physical line; no angle brackets in requirement bodies; `#### Scenario:` GIVEN/WHEN/THEN per MUST/SHALL. +- `@spec` anchors → canonical `openspec/specs//spec.md#requirement-`, NEVER a change dir. diff --git a/openspec/changes/archive/2026-07-24-catalog-ratings/design.md b/openspec/changes/archive/2026-07-24-catalog-ratings/design.md new file mode 100644 index 00000000..e734199c --- /dev/null +++ b/openspec/changes/archive/2026-07-24-catalog-ratings/design.md @@ -0,0 +1,221 @@ +# Design: catalog-ratings + +## Architecture Overview +`beoordeeling` becomes a moderated, authored object type on the existing +`voorzieningen` OpenRegister register — no new register, no new database +table (ADR-001). Two new thin backend seams are added on top of the generic +`ObjectService` path softwarecatalog otherwise uses directly from the +frontend, mirroring the two existing precedents for security-sensitive +writes/reads (`IntakeService`/`IntakeController` for anonymous intake, +`ModerationService`/`ModerationController` for the approval queue): + +- `ReviewService`/`ReviewController` — authenticated submit (author stamped + from session, status forced to `pending`) + public approved-only read + + aggregate. +- `ModerationService`/`ModerationController` — generalised (not duplicated) + to also moderate `beoordeeling`, alongside its existing `organisatie` path. + +``` +Vue (ModuleDetail bodyWidget: ReviewsPanel.vue) + │ GET /api/reviews?type=module&id= (public, approved-only + aggregate) + │ POST /api/reviews (auth session, author/status stamped) + ▼ +ReviewController → ReviewService → OpenRegister ObjectService (register.d/catalog-ratings.json RBAC) + +Vue (SoftwareCatalogSettings.vue: ModerationQueue type="beoordeeling") + │ GET /api/moderation/pending?type=beoordeeling + │ POST /api/moderation/{uuid}/approve?type=beoordeeling + │ POST /api/moderation/{uuid}/reject?type=beoordeeling + ▼ +ModerationController → ModerationService (generalised) → OpenRegister ObjectService +``` + +## API Design + +### `GET /api/reviews` +Public, read-only. Query params `type` (`module`|`dienst`), `id` (subject +uuid). Returns approved reviews for that subject plus the aggregate. + +**Response:** +```json +{ + "average": 8.25, + "count": 4, + "items": [ + { "id": "…", "naam": "Solid intake flow", "waardering": 9, "auteur": "Jan Jansen", "beschrijvingLang": "…" } + ] +} +``` + +### `POST /api/reviews` +Authenticated (`#[NoAdminRequired]`). Body: `naam`, `waardering` (1-10), +`beschrijvingKort`/`beschrijvingLang` (testimonial), `subjectType` +(`module`|`dienst`), `subjectId` (uuid). `auteur`, `status`, `id`, `uuid`, +`_owner`, `_organisation`, `_source` are stripped from the payload server-side +before validation (mirrors `IntakeService::FORBIDDEN_KEYS`) and `auteur` is +set from `IUserSession::getUser()->getDisplayName()`, `status` forced to +`pending`. + +**Response (202):** +```json +{ "ok": true, "uuid": "…", "status": "pending", "message": "Review received and queued for moderation" } +``` + +### `GET /api/moderation/pending?type=beoordeeling` +### `POST /api/moderation/{uuid}/approve?type=beoordeeling` +### `POST /api/moderation/{uuid}/reject?type=beoordeeling` +Admin-gated (`#[AuthorizedAdminSetting(SoftwareCatalogAdmin::class)]`), +identical contract to the existing `organisatie` moderation endpoints; `type` +defaults to `organisatie` for backward compatibility with the existing +`ModerationQueue.vue` instance and its tests. + +## Database Changes +None — no Nextcloud migration class. All state lives in OpenRegister objects +governed by the `beoordeeling` JSON schema, extended via +`lib/Settings/register.d/catalog-ratings.json` (ADR-037), never by editing +`lib/Settings/softwarecatalogus_register.json`. + +## Nextcloud Integration +- Controllers: `ReviewController` (new), `ModerationController` (extended) +- Services: `ReviewService` (new), `ModerationService` (extended) +- Mappers/Entities: none — all persistence via OpenRegister's `ObjectService` +- Events/Hooks: none new — the schema's existing + `x-openregister-notifications.review-submitted` rule (already present, + unused today) starts firing once objects are actually created; reused + as-is per the proposal's Out-of-Scope + +## Security Considerations +This IS the security-critical part of the change; see also +`context-brief.md`. + +1. **Fail-closed public read.** `beoordeeling.authorization.read` changes + from an unconditional `["public"]` to + `[{"group":"public","match":{"status":"approved"}}, ]`. + Per or#2025 (veto-after-grant is dead code), the fix must ensure the + dangerous bare `"public"` entry is fully REMOVED, not additionally + guarded — appending a narrower rule after an unconditional one is a no-op + because OpenRegister's rule evaluation is a first-match/any-match OR, not + a most-specific-wins evaluation. + +2. **The register.d merge trap.** `SettingsService::deepMergeConfig()` + concatenates list-valued overlay keys onto the base (documented, + intentional, and correct for e.g. extending a `required` array). Applied + naively to `authorization.read`, concatenating my new list onto the + existing `["public"]` base produces `["public", {...}]` — `"public"` is + still present, so the schema would still be unconditionally + world-readable and the whole point of this change would silently not + ship. **Decision:** teach `deepMergeConfig` that any key literally named + `authorization` is replaced wholesale (list values included) rather than + concatenated, for that key's entire subtree. This is scoped to the + `authorization` key only — every other merge behavior (including the one + existing fragment, `contracts-to-decidesk.json`, which never touches + `authorization`) is unaffected. Alternative considered: express the + fragment's `read` array as `["public+conditional-only"]` and rely on some + later filter — rejected, no such conditional-suppression mechanism exists + in the RBAC evaluator (confirmed against + `openregister/openspec/specs/auth-system/spec.md`); replacing is + the only construct that actually removes the base entry. + +3. **Author identity never from client input.** `ReviewController::submit()` + strips `auteur` (and `status`/`id`/`uuid`/`_owner`/`_organisation`/ + `_source`) from the request body before it ever reaches `ObjectService`, + then sets `auteur` itself from the authenticated `IUserSession`. This + mirrors `IntakeService::FORBIDDEN_KEYS` exactly (same class of problem — + different trust boundary: anonymous vs. authenticated-but-untrusted + client payload). + +4. **Ownership-scoped edit.** `beoordeeling` gets no bespoke "is this the + author" check in application code: OpenRegister's own role hierarchy + (`admin > object owner > named groups > authenticated > public`, per + `auth-system` spec REQ "role hierarchy") already grants the creating + user (`_owner`, auto-stamped by `ObjectService::saveObject()` from the + session at create time — no application code needed) full CRUD on their + own review regardless of the schema's named-group `update`/`delete` + lists. The schema's own `update`/`delete` lists are therefore + deliberately narrow (admin + org-scoped org-admin groups only, no broad + "all catalog users" entry) — that narrowness is what makes "non-author + cannot edit another's review" true; owner override is what makes + "author can edit their own" true, without needing to duplicate that + check in `ReviewService`. + +5. **Deletion restricted.** Per the brief, `delete` is intentionally not + granted to the broad staff-role list every other schema in this register + uses — only `software-catalog-admins`, plus the owner override above. + +6. **CSRF/rate-limiting.** `POST /api/reviews` is a normal authenticated, + CSRF-protected (Nextcloud default) endpoint — no `#[PublicPage]`, no + `#[NoCSRFRequired]`, unlike `IntakeController` (which is deliberately + anonymous + rate-limited). `GET /api/reviews` and the moderation + list/decide endpoints follow the exact existing precedent + (`FacetController`/`ModerationController`). + +7. **Residual risk (accepted, documented in the proposal).** A user already + in an authorized `create` group could still call OpenRegister's generic + object API directly instead of `ReviewController`, and set `auteur`/ + `status` themselves on that path. This is an existing, accepted trust + boundary shared by every other schema in this app (frontend talks to OR + directly); the public read gate is enforced independently of which path + wrote the object, so this does not reproduce the brief's "no + authorization at all" hole. + +## NL Design System +`ReviewsPanel.vue`/`SubmitReviewModal.vue` use `NcButton`, `NcTextField`, +`NcTextArea`, `NcNoteCard`/`NcEmptyContent` from `@nextcloud/vue` and NC CSS +variables only (no hardcoded colors, ADR-003). A star/numeric rating input +component: a simple 1-10 `NcSelect` (`inputLabel` set, ADR-012/hydra-gate- +nc-input-labels) rather than inventing a bespoke star-rating widget the +design system doesn't provide. + +## File Structure +``` +lib/ + Settings/register.d/catalog-ratings.json (new fragment) + Controller/ReviewController.php (new) + Service/ReviewService.php (new) + Controller/ModerationController.php (generalised: +type param) + Service/ModerationService.php (generalised: +type param) + Service/SettingsService.php (deepMergeConfig authorization fix) +src/ + components/reviews/ReviewsPanel.vue (new body widget) + modals/SubmitReviewModal.vue (new, own file per ADR-012) + views/settings/sections/ModerationQueue.vue (parameterised: type/labels props) + views/settings/SoftwareCatalogSettings.vue (second ModerationQueue instance) + utils/adminApi.js (reused as-is for the new endpoints) + customComponents.js (register ReviewsPanel) + manifest.json (ModuleDetail bodyWidgets; Reviews index columns) +appinfo/routes.php (new /api/reviews* routes) +tests/Unit/Service/ReviewServiceTest.php (new, incl. negative security tests) +tests/Unit/Service/DeepMergeAuthorizationTest.php (new) +tests/Unit/Service/IntakeModerationTest.php (extended: type=beoordeeling coverage) +tests/vitest/reviewsPanel.spec.js (new) +l10n/nl.js, l10n/nl.json, l10n/en_US.js, l10n/en_US.json (new keys) +docs/features/catalog-ratings.md (new, with screenshot) +``` + +## Seed Data +No seed data is added by this change. `beoordeeling` remains empty on a +fresh install (as it is today); the moderation queue and ratings panel both +render correctly on zero rows (`NcEmptyContent`, `count: 0` / `average: null` +handled explicitly). Reviews are created only by real user submission +through `ReviewController`. + +## Trade-offs +- **Custom PHP aggregate vs. declarative manifest `stat` widget.** The + existing `stat` widget type (used by `rv-score`/`ct-value`) is + attractive for consistency, but its `filter` semantics for an + array-of-related-object property (`beoordeeling.modules`) are unverified + in this codebase (no existing usage filters an array-of-relations field; + `TimeseriesRequestValidator` only confirms the aggregated *field* must be + a declared schema property, not that array-containment filtering works). + Given the "orphaned capability" failure mode already observed elsewhere in + this fleet (spec-says-done ≠ feature runs), the aggregate is computed in + `ReviewService` against `ObjectService::searchObjects()` results in PHP + instead — slightly more code, but deterministic and fully unit-testable + without depending on unverified filter behavior. +- **Generalising `ModerationService`/`ModerationQueue.vue` vs. a parallel + review-specific moderation stack.** The brief is explicit: reuse the + pattern, don't invent a second mechanism. Generalising risks the + well-tested `organisatie` path; mitigated by keeping every new parameter + defaulted to the exact current `organisatie`/`registratiestatus`/`active` + behavior, so `IntakeModerationTest.php`'s existing assertions (which never + pass a `type`) continue to exercise the unchanged default path. diff --git a/openspec/changes/archive/2026-07-24-catalog-ratings/proposal.md b/openspec/changes/archive/2026-07-24-catalog-ratings/proposal.md new file mode 100644 index 00000000..5d6711b3 --- /dev/null +++ b/openspec/changes/archive/2026-07-24-catalog-ratings/proposal.md @@ -0,0 +1,166 @@ +# Proposal: catalog-ratings + +## Summary +Turns the dormant `beoordeeling` (review) schema into a working, moderated +ratings-and-testimonials feature and closes the authorization hole it ships +with today: `beoordeeling` currently has `authorization: {"read": ["public"]}` +with no create/update/delete rules and no author or owning-organisation +binding, so any review would be world-readable with undefined write rules and +no attributable author. This change adds explicit create/update/delete +authorization, a server-stamped author + owning organisation, a +pending/approved/rejected moderation status reusing the existing +`ModerationQueue.vue` approval pattern, a submit-a-review flow (rating + +testimonial) from the module detail page, an aggregate rating (average + +count) on module detail, and fixes the manifest's dead `auteur` Reviews-index +column. Closes softwarecatalog#375. + +## Motivation +VNG Softwarecatalogus issue #49 — peer municipalities want to see peer +experience/ratings when selecting software; this is a core catalog +job-to-be-done and a point of differentiation versus the centralised GEMMA +registry, which does not offer peer review. The schema for this already +exists in the published data model but has never been wired up, and shipping +it as-is would expose an unauthenticated write-anything, read-everything +surface with no accountability — worse than not having the feature at all. +This change makes the feature real while closing that hole first. + +## Affected Projects +- [x] Project: `softwarecatalog` — schema fragment (author/org binding + + authorization + status), submit/list/aggregate endpoints, moderation reuse, + module-detail ratings panel, Reviews-index column fix, i18n, tests. + +## Scope + +### In Scope +- Schema: add `auteur` (server-stamped author display name) and `status` + (`pending`/`approved`/`rejected`, default `pending`) properties to + `beoordeeling` via a new `lib/Settings/register.d/catalog-ratings.json` + fragment (never editing the monolith). Owning organisation uses + OpenRegister's existing `_organisation` system field (the same convention + already used by `contactpersoon`/`gebruik`/`koppeling` in this register) — + no new schema property needed for it. +- Explicit `authorization.create/update/delete` on `beoordeeling` (today + entirely absent), and a `read` rule that is public ONLY for + `status: approved` reviews, replacing the current unconditional + `["public"]` grant with a genuinely fail-closed rule. +- A fix to `SettingsService::deepMergeConfig()` (pre-existing register- + fragment merge helper) so that a fragment overlaying a schema's + `authorization` block REPLACES rule lists instead of concatenating them — + the existing list-concatenation behavior is correct for ordinary schema + properties but is a fail-OPEN trap for authorization arrays: concatenating + a narrower overlay onto a base list that still contains bare `"public"` + would leave the dangerous wide-open base entry in place no matter what the + overlay adds. This is the same class of bug as OR's veto-after-grant trap + (or#2025), one layer up in the config-merge step. +- Moderation: reuse `ModerationController`/`ModerationService`/ + `ModerationQueue.vue` by generalising them to a second moderated type + (`beoordeeling`, field `status`, approved value `approved`) rather than + building a second admin queue mechanism. The existing `organisatie` + moderation path (registratiestatus/active) keeps its exact current + behavior as the default. +- Submit-a-review flow: a new `ReviewController`/`ReviewService` (mirroring + `IntakeService`'s pattern) so a logged-in user's author identity is always + taken from the Nextcloud session and never from client-supplied input, and + new submissions are always forced to `status: pending`. A `SubmitReviewModal.vue` + (own file, ADR-012) reachable from the module detail page, plus an + average+count aggregate, computed server-side (not via a declarative + manifest stat widget, to avoid depending on unverified array-containment + filter semantics for `beoordeeling.modules`). +- Fix the manifest's `Reviews` index (`src/manifest.json`): replace the + dead `titel`/`auteur`/`score`/`datum` columns (none of which exist on the + schema) with the real properties `naam`/`auteur`/`waardering`/`status`. +- i18n (English keys + `l10n/nl.js`/`l10n/nl.json` + + `l10n/en_US.js`/`l10n/en_US.json`), PHPUnit + vitest tests (including the + mandated negative security tests), and feature docs. + +### Out of Scope +- Cross-organisation reputation scoring. +- Review replies/threads. +- Notifying vendors of new reviews (the `review-submitted` + `x-openregister-notifications` rule already declared on `beoordeeling` is + reused as-is, not extended). +- Anonymous public review submission — review authorship requires an + authenticated session; this is the opposite of the `IntakeService` anonymous + path and is why submission is NOT wired through `IntakeService` itself. +- A dedicated `DienstDetail` page. `beoordeeling` already supports rating a + `dienst` via its `diensten` relation and the aggregate/submit backend is + subject-type-agnostic (`module` or `dienst`), but the softwarecatalog + manifest today has no `/diensten/:id` detail route at all (`Diensten` is a + `type: custom` faceted index with no row-level detail page) — adding one is + a pre-existing gap unrelated to the authorization hole this change closes. + A follow-up issue is filed to wire the ratings panel there once that page + exists. + +## Approach +Add a register.d fragment binding `beoordeeling` to a server-enforced +moderation lifecycle and fail-closed RBAC; add two small backend +controller/service pairs (review submission + review moderation, the latter +generalising the existing organisatie moderation code rather than +duplicating it); add a small custom `ReviewsPanel.vue` body-widget on +`ModuleDetail` (the existing `bodyWidgets`/`component` manifest escape hatch, +same mechanism as `ContractApprovalPanel`) that shows the aggregate + a +submit button; wire a second `ModerationQueue.vue` instance into the admin +settings page for review moderation, parameterised by `type` prop. + +## New Dependencies +None. + +## Impact +- `lib/Settings/register.d/catalog-ratings.json` (new fragment) +- `lib/Service/SettingsService.php` (`deepMergeConfig` authorization-replace fix) +- `lib/Controller/ReviewController.php`, `lib/Service/ReviewService.php` (new) +- `lib/Controller/ModerationController.php`, `lib/Service/ModerationService.php` (generalised to a second type) +- `src/views/settings/sections/ModerationQueue.vue` (parameterised by `type`/labels) +- `src/views/settings/SoftwareCatalogSettings.vue` (second `ModerationQueue` instance) +- `src/components/reviews/ReviewsPanel.vue`, `src/modals/SubmitReviewModal.vue` (new) +- `src/customComponents.js`, `src/manifest.json` (`ModuleDetail` bodyWidgets, `Reviews` index columns) +- `src/utils/moderationItem.js` (already generic — no change expected, verified during implementation) +- `appinfo/routes.php` (new `/api/reviews*` endpoints) +- i18n: `l10n/nl.js`, `l10n/nl.json`, `l10n/en_US.js`, `l10n/en_US.json` +- Tests: `tests/Unit/Service/ReviewServiceTest.php`, + `tests/Unit/Service/DeepMergeAuthorizationTest.php`, + `tests/vitest/*` for the new Vue logic. + +## Cross-Project Dependencies +None. `beoordeeling` is entirely internal to the `voorzieningen` register +owned by softwarecatalog; no other Conduction app reads or writes it. + +## Risks + +### Risk 1: A user in an authorized `create` group can bypass `ReviewController` and POST to OpenRegister's generic object API directly, setting `auteur`/`status` themselves +**Severity:** Medium — **Mitigation:** This is an accepted, pre-existing +architecture trade-off shared by every schema in this app (softwarecatalog's +frontend talks to OpenRegister directly for all other schemas; write access +is gated purely by group membership, not by a bespoke controller). The +`ReviewController` path is the one the shipped UI uses and is what closes the +brief's named hole (world-readable + no authorization at all). The public +`read` gate (`status: approved` only) is enforced by OpenRegister's own RBAC +filter regardless of which path was used to write the object, so a +self-approved forged review is still not the "no authorization at all" +situation described in the brief — it is a residual risk equivalent to what +every other schema already accepts for its trusted internal groups. Noted as +a follow-up rather than blocking this change. + +### Risk 2: `deepMergeConfig` behavior change could alter existing fragments +**Severity:** Low — **Mitigation:** The only key affected is one literally +named `authorization`; the sole existing fragment +(`register.d/contracts-to-decidesk.json`) never touches that key, so its +merged output is byte-for-byte unchanged. Covered by a new unit test +asserting both the old (non-authorization) concatenation behavior and the +new (authorization) replace behavior. + +## Rollback Strategy +Revert the commits on `wip/catalog-ratings`. The register fragment is +additive and isolated (`register.d/catalog-ratings.json`); deleting it +reverts `beoordeeling` to its pre-change (dormant, unauthorized) shape on the +next settings reload. No data migration is introduced, so no destructive +rollback step is needed; any reviews already submitted remain valid +OpenRegister objects and can be manually pruned if desired. + +## Open Questions +None outstanding — the aggregate-filter uncertainty (whether OpenRegister's +declarative stat-widget filter supports array-containment on +`beoordeeling.modules`) was resolved by not depending on it: the aggregate is +computed by `ReviewService` in PHP against `ObjectService::searchObjects()` +results, which is fully unit-testable regardless of that filter's actual +semantics. diff --git a/openspec/changes/archive/2026-07-24-catalog-ratings/specs/catalog-ratings/spec.md b/openspec/changes/archive/2026-07-24-catalog-ratings/specs/catalog-ratings/spec.md new file mode 100644 index 00000000..3332d74d --- /dev/null +++ b/openspec/changes/archive/2026-07-24-catalog-ratings/specs/catalog-ratings/spec.md @@ -0,0 +1,224 @@ +# catalog-ratings Specification + +**Status**: in-progress +**Scope**: softwarecatalog +**OpenSpec changes**: +- catalog-ratings + +## Purpose +Turns the dormant `beoordeeling` (review) schema into a working, moderated +ratings-and-testimonials feature for modules and services, while closing the +authorization hole it shipped with (world-readable, no create/update/delete +rules, no attributable author). Reviews are submitted by authenticated +catalog users, land pending, and only become publicly visible once approved +through the same admin moderation pattern already used for anonymous +organisation registration. + +## ADDED Requirements + +### Requirement: Public read access to a review MUST be restricted to approved reviews +The `beoordeeling` schema MUST grant the `public` (unauthenticated) group +read access only to objects whose `status` property equals `approved`. The +schema MUST NOT grant an unconditional `public` read rule. + +#### Scenario: An approved review is publicly readable +- **GIVEN** a `beoordeeling` object with `status: "approved"` +- **WHEN** an unauthenticated client reads it (directly, or via the module/dienst aggregate endpoint) +- **THEN** the review is returned + +#### Scenario: A pending review is not publicly readable +- **GIVEN** a `beoordeeling` object with `status: "pending"` +- **WHEN** an unauthenticated client attempts to read it, either directly or via a list request +- **THEN** the review is absent from list responses and a direct fetch returns not-found or forbidden + +#### Scenario: A rejected review is not publicly readable +- **GIVEN** a `beoordeeling` object with `status: "rejected"` +- **WHEN** an unauthenticated client attempts to read it +- **THEN** the review is absent from list responses and a direct fetch returns not-found or forbidden + +### Requirement: The register fragment merge MUST replace authorization rule lists, not concatenate them +`SettingsService::deepMergeConfig()` MUST treat any key literally named +`authorization` as replace-on-merge for its entire subtree (including list +values), rather than the general-purpose list-concatenation behavior used +for every other key. A register fragment narrowing a schema's authorization +MUST fully remove a dangerous base entry (such as a bare `"public"` read +grant), not append a narrower rule alongside it. + +#### Scenario: An authorization list in a fragment replaces the base list +- **GIVEN** a base schema authorization block with `read: ["public"]` +- **AND** a register fragment overlaying that schema's `authorization.read` with `[{"group":"public","match":{"status":"approved"}}]` +- **WHEN** `deepMergeConfig()` merges the fragment onto the base +- **THEN** the merged `authorization.read` MUST be exactly `[{"group":"public","match":{"status":"approved"}}]` +- **AND** MUST NOT contain the bare string `"public"` + +#### Scenario: Non-authorization list keys still concatenate (unchanged regression) +- **GIVEN** a base schema with `required: ["naam"]` +- **AND** a register fragment overlaying that schema's `required` with `["waardering"]` +- **WHEN** `deepMergeConfig()` merges the fragment onto the base +- **THEN** the merged `required` MUST be `["naam", "waardering"]` (concatenated, not replaced) + +### Requirement: Creating, updating, or deleting a review MUST be governed by explicit authorization rules +The `beoordeeling` schema MUST declare explicit `authorization.create`, +`authorization.update`, and `authorization.delete` rules. `create` MUST be +limited to authenticated catalog-user groups (never `public`). `update` and +`delete` MUST NOT grant the full breadth of catalog-user groups; `delete` +MUST be restricted to catalog-admin groups only. + +#### Scenario: An authenticated catalog user can submit a review +- **GIVEN** a user in the `software-catalog-users` group +- **WHEN** the user submits a review through `POST /api/reviews` +- **THEN** the review is created with `status: "pending"` + +#### Scenario: An unauthenticated request cannot create a review +- **GIVEN** no active Nextcloud session +- **WHEN** a request is made to `POST /api/reviews` +- **THEN** the request is rejected (401/403) and no `beoordeeling` object is created + +### Requirement: The submitting user's identity MUST be bound server-side and MUST NOT be accepted from client input +`ReviewService::submit()` MUST discard any client-supplied `auteur`, +`status`, `id`, `uuid`, `_owner`, `_organisation`, and `_source` keys before +persisting, and MUST set `auteur` from the authenticated +`IUserSession::getUser()` display name. + +#### Scenario: The stored review carries the authenticated user's name +- **GIVEN** a user "Jan Jansen" is authenticated +- **WHEN** they submit a review with no `auteur` field in the payload +- **THEN** the persisted object's `auteur` equals "Jan Jansen" + +#### Scenario: A client-supplied author is ignored +- **GIVEN** a user "Jan Jansen" is authenticated +- **WHEN** they submit a review with `auteur: "Someone Else"` in the payload +- **THEN** the persisted object's `auteur` equals "Jan Jansen", not "Someone Else" + +### Requirement: Only the review's author or an organisation-scoped admin MAY update it; unrelated users MUST be refused +A user MUST be able to update their own review by virtue of OpenRegister's +object-owner privilege (no bespoke authorization-code check required). A +user who is neither the review's owner nor a member of a group granted in +`beoordeeling.authorization.update` MUST NOT be able to update the review. + +#### Scenario: The author edits their own review +- **GIVEN** a review created by user "Jan Jansen" (`_owner: "jan.jansen"`) +- **WHEN** "jan.jansen" updates the review's `beschrijvingLang` +- **THEN** the update succeeds + +#### Scenario: A non-author, non-admin user cannot edit another user's review +- **GIVEN** a review created by user "Jan Jansen" (`_owner: "jan.jansen"`) +- **AND** user "Piet Peters" is authenticated, is not the owner, and is not in any `beoordeeling.authorization.update` group +- **WHEN** "piet.peters" attempts to update the review +- **THEN** the update is refused (403) + +### Requirement: Review deletion MUST be restricted to catalog admins (plus the owner) +`beoordeeling.authorization.delete` MUST NOT include the broad catalog-user +groups other schemas in this register grant delete to; only catalog-admin +groups are listed (owner deletion remains available via the OpenRegister +owner privilege, independent of this list). + +#### Scenario: A regular catalog user cannot delete another user's review +- **GIVEN** a review created by user "Jan Jansen" +- **AND** user "Piet Peters" is in `software-catalog-users` but not `software-catalog-admins` and is not the owner +- **WHEN** "piet.peters" attempts to delete the review +- **THEN** the delete is refused (403) + +### Requirement: A newly submitted review MUST require moderation approval before becoming public +Every review created through `ReviewService::submit()` MUST be created with +`status: "pending"`, regardless of any client-supplied value. Only an +explicit admin approval decision MAY transition it to `status: "approved"`. + +#### Scenario: A submission lands pending and is not yet public +- **WHEN** an authenticated user submits a valid review +- **THEN** the stored object has `status: "pending"` +- **AND** it is not returned to unauthenticated readers + +#### Scenario: Admin approval makes the review public +- **GIVEN** a review with `status: "pending"` +- **WHEN** an admin approves it through the moderation queue +- **THEN** the review's `status` becomes `"approved"` +- **AND** it is now returned to unauthenticated readers + +#### Scenario: Admin rejection keeps the review hidden +- **GIVEN** a review with `status: "pending"` +- **WHEN** an admin rejects it through the moderation queue +- **THEN** the review's `status` becomes `"rejected"` +- **AND** it remains absent from unauthenticated read results + +### Requirement: Review moderation MUST reuse the existing moderation queue mechanism, not a second one +`ModerationService`/`ModerationController` MUST support moderating +`beoordeeling` objects (`status` field, `approved`/`rejected` values) through +the same `listPending()`/`approve()`/`reject()` methods and the same +admin-gated endpoints already used for `organisatie` (`registratiestatus` +field, `active`/`rejected` values), selected by an explicit type parameter +that defaults to the existing `organisatie` behavior. The existing +`ModerationQueue.vue` component MUST be reused (parameterised), not +duplicated, for the review moderation UI. + +#### Scenario: An admin moderates pending reviews through the existing queue UI +- **GIVEN** at least one `beoordeeling` object with `status: "pending"` +- **WHEN** an admin opens the review moderation section in Settings +- **THEN** the pending review appears in a `ModerationQueue.vue` instance with Approve/Reject actions + +#### Scenario: A non-admin cannot reach the review moderation endpoints +- **GIVEN** a user who is not a Nextcloud admin +- **WHEN** they call `GET /api/moderation/pending?type=beoordeeling` +- **THEN** the request is rejected before the controller body runs (Nextcloud's `AuthorizedAdminSetting` middleware) + +#### Scenario: The default (unparameterised) organisatie moderation path is unchanged +- **GIVEN** an admin approves a pending `organisatie` registration via `POST /api/moderation/{uuid}/approve` with no `type` query parameter +- **WHEN** the request is processed +- **THEN** the behavior is identical to the pre-existing `organisatie`/`registratiestatus`/`active` flow (unaffected by the `beoordeeling` generalisation) + +### Requirement: Module and dienst detail pages MUST display an aggregate rating computed only from approved reviews +The aggregate (average `waardering` and count) MUST be computed only from +`beoordeeling` objects with `status: "approved"` for the given module or +dienst. When there are zero approved reviews for the subject, the aggregate +MUST report a count of `0` and a null average rather than erroring. + +#### Scenario: Aggregate reflects only approved reviews +- **GIVEN** a module has one approved review (`waardering: 8`) and one pending review (`waardering: 2`) +- **WHEN** the aggregate is requested for that module +- **THEN** the average is `8` and the count is `1` + +#### Scenario: Aggregate with no approved reviews +- **GIVEN** a module has zero approved reviews +- **WHEN** the aggregate is requested for that module +- **THEN** the average is `null` and the count is `0` + +### Requirement: The Reviews index MUST display columns that exist on the beoordeeling schema +The `Reviews` index page's `columns` configuration MUST reference only +properties actually declared on the `beoordeeling` schema (`src/manifest.json`). + +#### Scenario: Every configured column resolves to a real schema property +- **GIVEN** the `Reviews` index page's `config.columns` array +- **WHEN** each column name is checked against `beoordeeling`'s declared properties +- **THEN** every column name (`naam`, `auteur`, `waardering`, `status`) is a real property +- **AND** none of the previously dead column names (`titel`, `score`, `datum`) remain + +## Non-Functional Requirements + +- **Performance:** The aggregate endpoint MUST bound its underlying query + (`_limit`) so a module/dienst detail page load never issues an unbounded + scan of the `beoordeeling` collection. +- **Accessibility:** The rating input in `SubmitReviewModal.vue` MUST use an + `NcSelect` with `inputLabel` set (WCAG 2.1 AA 1.3.1/4.1.2, ADR-012). +- **Internationalization:** All new user-facing strings MUST be added in + Dutch and English (ADR-005): `l10n/nl.js`/`l10n/nl.json` and + `l10n/en_US.js`/`l10n/en_US.json`. + +## Acceptance Criteria + +- [ ] `beoordeeling.authorization` has no unconditional `public` entry in any of `read`/`create`/`update`/`delete` +- [ ] A pending or rejected review is not returned to an unauthenticated reader (negative test passing) +- [ ] A client-supplied `auteur` value is never persisted (negative test passing) +- [ ] A non-author, non-admin cannot update another user's review (negative test passing) +- [ ] Review moderation is reachable through the existing `ModerationQueue.vue` component, parameterised, not a new component +- [ ] The module detail page shows an aggregate rating and a working submit-review flow +- [ ] The `Reviews` index no longer references the dead `auteur`/`titel`/`score`/`datum` columns + +## Notes +- `_organisation` (owning organisation) uses OpenRegister's existing system + field and matching convention already used by `contactpersoon`/`gebruik`/ + `koppeling` in this register — no new schema property was needed for it. +- A `DienstDetail` page does not exist yet in this manifest; the + `dienst`-subject path of the submit/aggregate API is implemented and + tested, but the UI wiring is deferred to a follow-up (see proposal + Out-of-Scope) since adding the missing detail page itself is unrelated in + scope to closing the authorization hole. diff --git a/openspec/changes/archive/2026-07-24-catalog-ratings/tasks.md b/openspec/changes/archive/2026-07-24-catalog-ratings/tasks.md new file mode 100644 index 00000000..ec8a3bd6 --- /dev/null +++ b/openspec/changes/archive/2026-07-24-catalog-ratings/tasks.md @@ -0,0 +1,84 @@ +# Tasks: catalog-ratings + +## Implementation Tasks + +### Task 1: Add the catalog-ratings register fragment (author/org binding + fail-closed authorization + status) +- **spec_ref**: `openspec/specs/catalog-ratings/spec.md#requirement-public-read-access-to-a-review-must-be-restricted-to-approved-reviews` +- **files**: `lib/Settings/register.d/catalog-ratings.json` +- **acceptance_criteria**: + - GIVEN the fragment is merged WHEN `beoordeeling` is loaded THEN it has `auteur` and `status` properties, and `authorization.create/update/delete` are all present and non-empty + - GIVEN the merged config WHEN `authorization.read` is inspected THEN it contains no bare `"public"` entry, only a `status: approved`-conditioned one +- [x] Implement +- [x] Test + +### Task 2: Fix deepMergeConfig to replace (not concatenate) authorization lists +- **spec_ref**: `openspec/specs/catalog-ratings/spec.md#requirement-the-register-fragment-merge-must-replace-authorization-rule-lists-not-concatenate-them` +- **files**: `lib/Service/SettingsService.php`, `tests/Unit/Service/DeepMergeAuthorizationTest.php` +- **acceptance_criteria**: + - GIVEN a base `authorization.read` of `["public"]` and an overlay of `[{"group":"public","match":{"status":"approved"}}]` WHEN merged THEN the result is exactly the overlay + - GIVEN a base `required` of `["naam"]` and an overlay of `["waardering"]` WHEN merged THEN the result is `["naam","waardering"]` (concatenated, unchanged behavior) +- [x] Implement +- [x] Test + +### Task 3: ReviewService + ReviewController (submit, approved-only read, aggregate) +- **spec_ref**: `openspec/specs/catalog-ratings/spec.md#requirement-the-submitting-users-identity-must-be-bound-server-side-and-must-not-be-accepted-from-client-input` +- **files**: `lib/Service/ReviewService.php`, `lib/Controller/ReviewController.php`, `appinfo/routes.php`, `tests/Unit/Service/ReviewServiceTest.php` +- **acceptance_criteria**: + - GIVEN an authenticated user submits a review with a forged `auteur` WHEN it is persisted THEN the stored `auteur` is the session user's display name, not the forged value + - GIVEN an unauthenticated request to `POST /api/reviews` WHEN processed THEN it is rejected and no object is created + - GIVEN a module with one approved and one pending review WHEN the aggregate is requested THEN only the approved review counts + - GIVEN a module with zero approved reviews WHEN the aggregate is requested THEN average is null and count is 0 +- [x] Implement +- [x] Test + +### Task 4: Generalise ModerationService/ModerationController to a second moderated type (beoordeeling) +- **spec_ref**: `openspec/specs/catalog-ratings/spec.md#requirement-review-moderation-must-reuse-the-existing-moderation-queue-mechanism-not-a-second-one` +- **files**: `lib/Service/ModerationService.php`, `lib/Controller/ModerationController.php`, `tests/Unit/Service/IntakeModerationTest.php` +- **acceptance_criteria**: + - GIVEN `type=beoordeeling` WHEN `listPending()`/`approve()`/`reject()` are called THEN they operate on the `beoordeeling` register/schema using the `status` field and `approved`/`rejected` values + - GIVEN no `type` parameter (existing callers) WHEN the same methods are called THEN behavior is byte-for-byte identical to the pre-existing `organisatie`/`registratiestatus` path (existing test assertions keep passing unmodified) +- [x] Implement +- [x] Test + +### Task 5: Parameterise ModerationQueue.vue and add the review moderation section to Settings +- **spec_ref**: `openspec/specs/catalog-ratings/spec.md#requirement-review-moderation-must-reuse-the-existing-moderation-queue-mechanism-not-a-second-one` +- **files**: `src/views/settings/sections/ModerationQueue.vue`, `src/views/settings/SoftwareCatalogSettings.vue`, `tests/vitest/moderationItem.spec.js` +- **acceptance_criteria**: + - GIVEN the settings page WHEN it renders THEN a second `ModerationQueue` instance (`type="beoordeeling"`) appears alongside the existing organisation-registration one, with its own title/description +- [x] Implement +- [x] Test + +### Task 6: SubmitReviewModal.vue + ReviewsPanel.vue (submit flow + aggregate display) wired onto ModuleDetail +- **spec_ref**: `openspec/specs/catalog-ratings/spec.md#requirement-module-and-dienst-detail-pages-must-display-an-aggregate-rating-computed-only-from-approved-reviews` +- **files**: `src/modals/SubmitReviewModal.vue`, `src/components/reviews/ReviewsPanel.vue`, `src/customComponents.js`, `src/manifest.json`, `tests/vitest/reviewsPanel.spec.js` +- **acceptance_criteria**: + - GIVEN a module detail page WHEN it loads THEN it shows the aggregate rating (average + count) and a "Write a review" action opening `SubmitReviewModal.vue` + - GIVEN the rating input WHEN rendered THEN it is an `NcSelect` with `inputLabel` set (no bare `