diff --git a/appinfo/routes.php b/appinfo/routes.php index 21eb6272..c0605f99 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -200,11 +200,20 @@ // until an admin approves). Anti-spam rate-limited. ['name' => 'intake#submit', 'url' => '/api/intake/register', 'verb' => 'POST'], - // REGISTRATION MODERATION / APPROVAL QUEUE — admin-gated (isAdmin guard). + // REGISTRATION / REVIEW MODERATION / APPROVAL QUEUE — admin-gated + // (AuthorizedAdminSetting). Selects organisatie (default) or + // beoordeeling via the `type` query param — one generalised + // mechanism, see ModerationService. ['name' => 'moderation#pending', 'url' => '/api/moderation/pending', 'verb' => 'GET'], ['name' => 'moderation#approve', 'url' => '/api/moderation/{uuid}/approve', 'verb' => 'POST'], ['name' => 'moderation#reject', 'url' => '/api/moderation/{uuid}/reject', 'verb' => 'POST'], + // CATALOG RATINGS (softwarecatalog#375) — authenticated review + // submission (author/status always server-stamped, never from the + // client) + public approved-only aggregate for module/dienst detail. + ['name' => 'review#submit', 'url' => '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/api/reviews', 'verb' => 'POST'], + ['name' => 'review#aggregate', 'url' => '/api/reviews/aggregate', 'verb' => 'GET'], + // ORGANISATION MERGE (gemeentelijke herindeling / leveranciersovername) — // admin-gated (isAdmin guard in the controller body, no-admin-idor safe). // @spec openspec/specs/organisation-merge/spec.md#requirement-both-merge-endpoints-must-be-admin-only-with-an-explicit-per-object-authorization-guard diff --git a/docs/features/catalog-ratings.md b/docs/features/catalog-ratings.md new file mode 100644 index 00000000..1a9d05af --- /dev/null +++ b/docs/features/catalog-ratings.md @@ -0,0 +1,114 @@ + + +# Catalog ratings + +Turns the previously dormant `beoordeeling` (review) schema into a working, +**moderated** ratings-and-testimonials feature for modules and services — +and closes the authorization hole it shipped with (world-readable, no +create/update/delete rules, no attributable author). See +[VNG Softwarecatalogus issue #49](https://github.com/VNG-Realisatie/Softwarecatalogus/issues/49) +and softwarecatalog#375. + +Specification: [`openspec/specs/catalog-ratings/spec.md`](../../openspec/specs/catalog-ratings/spec.md). + +## Why it existed but didn't work + +The `beoordeeling` schema was part of the published VNG data model but had +never been wired up: `authorization` was `{"read": ["public"]}` with **no** +create/update/delete rules at all, and no author or owning-organisation +binding. Shipping a ratings UI on top of that as-is would have been +world-readable with undefined write rules and no accountability. This +change fixes the schema first, then builds the feature on top of the fixed +schema. + +## Submitting a review + +From a module's detail page, a signed-in catalog user clicks **"Write a +review"** (`ReviewsPanel.vue`, a body widget on `ModuleDetail`), which opens +`SubmitReviewModal.vue`: a title, a 1-10 rating, and a testimonial. There is +**no "your name" field** — the author is always the authenticated Nextcloud +session, bound server-side by `ReviewService`; anything the client sends for +`auteur` is discarded. + +``` +POST /apps/softwarecatalog/api/reviews +{ "review": {"naam": "Solid intake flow", "waardering": 9, "beschrijvingLang": "..."}, + "subjectType": "module", "subjectId": "" } +``` + +Every submission lands `status: "pending"` — it is not yet visible to +anyone outside the catalog's internal groups. + +## Moderation + +Reviews are approved/rejected through the **same** `ModerationQueue.vue` +component already used for anonymous organisation registration, now +parameterised by a `type` prop (`organisatie`, default, or `beoordeeling`). +A second instance renders in **Settings → Review moderation**, backed by the +same admin-gated `ModerationController`/`ModerationService` +(`#[AuthorizedAdminSetting]`), selected via `?type=beoordeeling`. Approving +sets `status: "approved"`; rejecting sets `status: "rejected"` and the +review stays hidden. + +## Fail-closed public read + +`beoordeeling.authorization.read` is no longer an unconditional `["public"]` +grant. It is `[{"group":"public","match":{"status":"approved"}}, ]` — unauthenticated readers only ever see `approved` +reviews; `pending`/`rejected` reviews are invisible to them. This is +declared in a new **fragment**, `lib/Settings/register.d/catalog-ratings.json` +(ADR-037) — the shipped monolith `softwarecatalogus_register.json` is never +edited directly (an edit there is a silent no-op on installed instances). + +A subtlety in the fragment merge itself was fixed as part of closing this +hole: the generic register-fragment merge concatenates list values (correct +for most schema properties), which would have left the dangerous bare +`"public"` entry in place even after the fragment "added" a narrower rule. +`SettingsService::deepMergeConfig()` now replaces (rather than +concatenates) list values within any `authorization` block specifically, so +the fragment genuinely removes the wide-open base rule. + +## Aggregate rating + +Module (and, once a `DienstDetail` page exists — see Known gaps below, +dienst) detail pages show an average rating + review count, computed by +`ReviewAggregateService` from **approved reviews only**. A module with zero +approved reviews shows a null average / zero count rather than an error. + +## Authorization summary + +| Action | Who | +|---|---| +| Read approved reviews | Anyone (public) | +| Read pending/rejected reviews | Internal catalog groups + the review's own author (owner privilege) | +| Create | Authenticated catalog-user groups (never anonymous) | +| Update | The review's author (owner privilege) or an org-scoped admin group | +| Delete | `software-catalog-admins` only, or the review's author (owner privilege) | + +## Known gaps / follow-ups + +- **No `DienstDetail` page yet.** The submit/aggregate backend is + subject-type-agnostic (`module` or `dienst`), and `beoordeeling` already + supports a `diensten` relation, but the softwarecatalog manifest has no + `/diensten/:id` detail route today (`Diensten` is a `type: custom` faceted + index with no per-row detail page) — that is a pre-existing gap unrelated + to the authorization fix this change makes. Filed as a follow-up to wire + `ReviewsPanel` onto that page once it exists. +- **Residual direct-API risk.** A user already in an authorized `create` + group could bypass `ReviewController` and call OpenRegister's generic + object API directly, setting `auteur`/`status` themselves on that path. + This is an existing, accepted trust boundary shared by every other schema + in this app; the public read gate is enforced independently of which path + wrote the object. + +## Screenshots + +Not captured in this change — per this repo's convention (see +`organisation-merge.md`), Playwright screenshot capture against a live +instance was out of bounds for this session (no live Nextcloud instance +without touching the shared dev environment). Follow-up: capture the +"Write a review" flow, the aggregate rating panel, and the review moderation +queue per ADR-010 once verified against a running instance. diff --git a/l10n/en_US.js b/l10n/en_US.js index c1939645..852a4b57 100644 --- a/l10n/en_US.js +++ b/l10n/en_US.js @@ -369,7 +369,42 @@ OC.L10N.register( "Attach the applications that make up this suite. Only applications already in the catalogue can be attached — creating a new application is not part of this wizard." : "Attach the applications that make up this suite. Only applications already in the catalogue can be attached — creating a new application is not part of this wizard.", "Could not load applications. Please try again." : "Could not load applications. Please try again.", "Applications ({count})" : "Applications ({count})", - "No applications attached yet." : "No applications attached yet." + "No applications attached yet." : "No applications attached yet.", + "Approve" : "Approve", + "Reject" : "Reject", + "Nothing to moderate" : "Nothing to moderate", + "Refresh queue" : "Refresh queue", + "Registration moderation" : "Registration moderation", + "Review anonymous catalog registrations. Approving an entry publishes it; rejecting leaves it hidden." : "Review anonymous catalog registrations. Approving an entry publishes it; rejecting leaves it hidden.", + "Loading pending registrations…" : "Loading pending registrations…", + "There are no pending registrations right now." : "There are no pending registrations right now.", + "Could not load the moderation queue" : "Could not load the moderation queue", + "{label} approved and published" : "{label} approved and published", + "{label} rejected" : "{label} rejected", + "{label} has no identifier" : "{label} has no identifier", + "Could not update the {label}" : "Could not update the {label}", + "Review moderation" : "Review moderation", + "Review pending ratings and testimonials. Approving a review publishes it; rejecting leaves it hidden." : "Review pending ratings and testimonials. Approving a review publishes it; rejecting leaves it hidden.", + "Loading pending reviews…" : "Loading pending reviews…", + "There are no pending reviews right now." : "There are no pending reviews right now.", + "Ratings & reviews" : "Ratings & reviews", + "Loading reviews" : "Loading reviews", + "Could not load reviews" : "Could not load reviews", + "Write a review" : "Write a review", + "1 review" : "1 review", + "{count} reviews" : "{count} reviews", + "No reviews yet" : "No reviews yet", + "Be the first to share your experience." : "Be the first to share your experience.", + "Your review will be visible to other municipalities once an administrator approves it." : "Your review will be visible to other municipalities once an administrator approves it.", + "Title" : "Title", + "Summarise your experience in a few words" : "Summarise your experience in a few words", + "Rating (1-10)" : "Rating (1-10)", + "Select a rating" : "Select a rating", + "Testimonial" : "Testimonial", + "What was your experience with this software?" : "What was your experience with this software?", + "Submit review" : "Submit review", + "Thank you — your review was submitted for moderation" : "Thank you — your review was submitted for moderation", + "Could not submit your review" : "Could not submit your review" }, "nplurals=2; plural=(n != 1);" ); diff --git a/l10n/en_US.json b/l10n/en_US.json index f66c35ca..dc284eff 100644 --- a/l10n/en_US.json +++ b/l10n/en_US.json @@ -428,6 +428,41 @@ "Attach the applications that make up this suite. Only applications already in the catalogue can be attached — creating a new application is not part of this wizard.": "Attach the applications that make up this suite. Only applications already in the catalogue can be attached — creating a new application is not part of this wizard.", "Could not load applications. Please try again.": "Could not load applications. Please try again.", "Applications ({count})": "Applications ({count})", - "No applications attached yet.": "No applications attached yet." + "No applications attached yet.": "No applications attached yet.", + "Approve": "Approve", + "Reject": "Reject", + "Nothing to moderate": "Nothing to moderate", + "Refresh queue": "Refresh queue", + "Registration moderation": "Registration moderation", + "Review anonymous catalog registrations. Approving an entry publishes it; rejecting leaves it hidden.": "Review anonymous catalog registrations. Approving an entry publishes it; rejecting leaves it hidden.", + "Loading pending registrations…": "Loading pending registrations…", + "There are no pending registrations right now.": "There are no pending registrations right now.", + "Could not load the moderation queue": "Could not load the moderation queue", + "{label} approved and published": "{label} approved and published", + "{label} rejected": "{label} rejected", + "{label} has no identifier": "{label} has no identifier", + "Could not update the {label}": "Could not update the {label}", + "Review moderation": "Review moderation", + "Review pending ratings and testimonials. Approving a review publishes it; rejecting leaves it hidden.": "Review pending ratings and testimonials. Approving a review publishes it; rejecting leaves it hidden.", + "Loading pending reviews…": "Loading pending reviews…", + "There are no pending reviews right now.": "There are no pending reviews right now.", + "Ratings & reviews": "Ratings & reviews", + "Loading reviews": "Loading reviews", + "Could not load reviews": "Could not load reviews", + "Write a review": "Write a review", + "1 review": "1 review", + "{count} reviews": "{count} reviews", + "No reviews yet": "No reviews yet", + "Be the first to share your experience.": "Be the first to share your experience.", + "Your review will be visible to other municipalities once an administrator approves it.": "Your review will be visible to other municipalities once an administrator approves it.", + "Title": "Title", + "Summarise your experience in a few words": "Summarise your experience in a few words", + "Rating (1-10)": "Rating (1-10)", + "Select a rating": "Select a rating", + "Testimonial": "Testimonial", + "What was your experience with this software?": "What was your experience with this software?", + "Submit review": "Submit review", + "Thank you — your review was submitted for moderation": "Thank you — your review was submitted for moderation", + "Could not submit your review": "Could not submit your review" } } diff --git a/l10n/nl.js b/l10n/nl.js index c2ec3d38..dc6a3624 100644 --- a/l10n/nl.js +++ b/l10n/nl.js @@ -406,7 +406,42 @@ OC.L10N.register( "Attach the applications that make up this suite. Only applications already in the catalogue can be attached — creating a new application is not part of this wizard." : "Koppel de applicaties waaruit deze suite bestaat. Alleen applicaties die al in de catalogus staan kunnen worden gekoppeld — het aanmaken van een nieuwe applicatie maakt geen deel uit van deze wizard.", "Could not load applications. Please try again." : "Kon de applicaties niet laden. Probeer het opnieuw.", "Applications ({count})" : "Applicaties ({count})", - "No applications attached yet." : "Nog geen applicaties gekoppeld." + "No applications attached yet." : "Nog geen applicaties gekoppeld.", + "Approve" : "Goedkeuren", + "Reject" : "Afwijzen", + "Nothing to moderate" : "Niets te modereren", + "Refresh queue" : "Wachtrij vernieuwen", + "Registration moderation" : "Registratiemoderatie", + "Review anonymous catalog registrations. Approving an entry publishes it; rejecting leaves it hidden." : "Beoordeel anonieme catalogusregistraties. Goedkeuren publiceert een item; afwijzen houdt het verborgen.", + "Loading pending registrations…" : "Openstaande registraties laden…", + "There are no pending registrations right now." : "Er zijn op dit moment geen openstaande registraties.", + "Could not load the moderation queue" : "Kon de moderatiewachtrij niet laden", + "{label} approved and published" : "{label} goedgekeurd en gepubliceerd", + "{label} rejected" : "{label} afgewezen", + "{label} has no identifier" : "{label} heeft geen identificatie", + "Could not update the {label}" : "Kon {label} niet bijwerken", + "Review moderation" : "Beoordelingsmoderatie", + "Review pending ratings and testimonials. Approving a review publishes it; rejecting leaves it hidden." : "Beoordeel openstaande waarderingen en testimonials. Goedkeuren publiceert een beoordeling; afwijzen houdt deze verborgen.", + "Loading pending reviews…" : "Openstaande beoordelingen laden…", + "There are no pending reviews right now." : "Er zijn op dit moment geen openstaande beoordelingen.", + "Ratings & reviews" : "Waarderingen & beoordelingen", + "Loading reviews" : "Beoordelingen laden", + "Could not load reviews" : "Kon beoordelingen niet laden", + "Write a review" : "Schrijf een beoordeling", + "1 review" : "1 beoordeling", + "{count} reviews" : "{count} beoordelingen", + "No reviews yet" : "Nog geen beoordelingen", + "Be the first to share your experience." : "Wees de eerste die een ervaring deelt.", + "Your review will be visible to other municipalities once an administrator approves it." : "Uw beoordeling is zichtbaar voor andere gemeenten zodra een beheerder deze goedkeurt.", + "Title" : "Titel", + "Summarise your experience in a few words" : "Vat uw ervaring in een paar woorden samen", + "Rating (1-10)" : "Waardering (1-10)", + "Select a rating" : "Selecteer een waardering", + "Testimonial" : "Testimonial", + "What was your experience with this software?" : "Wat was uw ervaring met deze software?", + "Submit review" : "Beoordeling indienen", + "Thank you — your review was submitted for moderation" : "Bedankt — uw beoordeling is ingediend ter moderatie", + "Could not submit your review" : "Kon uw beoordeling niet indienen" }, "nplurals=2; plural=(n != 1);" ); diff --git a/l10n/nl.json b/l10n/nl.json index 5a624464..99f5152f 100644 --- a/l10n/nl.json +++ b/l10n/nl.json @@ -572,6 +572,41 @@ "Attach the applications that make up this suite. Only applications already in the catalogue can be attached — creating a new application is not part of this wizard.": "Koppel de applicaties waaruit deze suite bestaat. Alleen applicaties die al in de catalogus staan kunnen worden gekoppeld — het aanmaken van een nieuwe applicatie maakt geen deel uit van deze wizard.", "Could not load applications. Please try again.": "Kon de applicaties niet laden. Probeer het opnieuw.", "Applications ({count})": "Applicaties ({count})", - "No applications attached yet.": "Nog geen applicaties gekoppeld." + "No applications attached yet.": "Nog geen applicaties gekoppeld.", + "Approve": "Goedkeuren", + "Reject": "Afwijzen", + "Nothing to moderate": "Niets te modereren", + "Refresh queue": "Wachtrij vernieuwen", + "Registration moderation": "Registratiemoderatie", + "Review anonymous catalog registrations. Approving an entry publishes it; rejecting leaves it hidden.": "Beoordeel anonieme catalogusregistraties. Goedkeuren publiceert een item; afwijzen houdt het verborgen.", + "Loading pending registrations…": "Openstaande registraties laden…", + "There are no pending registrations right now.": "Er zijn op dit moment geen openstaande registraties.", + "Could not load the moderation queue": "Kon de moderatiewachtrij niet laden", + "{label} approved and published": "{label} goedgekeurd en gepubliceerd", + "{label} rejected": "{label} afgewezen", + "{label} has no identifier": "{label} heeft geen identificatie", + "Could not update the {label}": "Kon {label} niet bijwerken", + "Review moderation": "Beoordelingsmoderatie", + "Review pending ratings and testimonials. Approving a review publishes it; rejecting leaves it hidden.": "Beoordeel openstaande waarderingen en testimonials. Goedkeuren publiceert een beoordeling; afwijzen houdt deze verborgen.", + "Loading pending reviews…": "Openstaande beoordelingen laden…", + "There are no pending reviews right now.": "Er zijn op dit moment geen openstaande beoordelingen.", + "Ratings & reviews": "Waarderingen & beoordelingen", + "Loading reviews": "Beoordelingen laden", + "Could not load reviews": "Kon beoordelingen niet laden", + "Write a review": "Schrijf een beoordeling", + "1 review": "1 beoordeling", + "{count} reviews": "{count} beoordelingen", + "No reviews yet": "Nog geen beoordelingen", + "Be the first to share your experience.": "Wees de eerste die een ervaring deelt.", + "Your review will be visible to other municipalities once an administrator approves it.": "Uw beoordeling is zichtbaar voor andere gemeenten zodra een beheerder deze goedkeurt.", + "Title": "Titel", + "Summarise your experience in a few words": "Vat uw ervaring in een paar woorden samen", + "Rating (1-10)": "Waardering (1-10)", + "Select a rating": "Selecteer een waardering", + "Testimonial": "Testimonial", + "What was your experience with this software?": "Wat was uw ervaring met deze software?", + "Submit review": "Beoordeling indienen", + "Thank you — your review was submitted for moderation": "Bedankt — uw beoordeling is ingediend ter moderatie", + "Could not submit your review": "Kon uw beoordeling niet indienen" } } diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index bf53779a..86abee56 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -107,6 +107,8 @@ * @link https://codeberg.org/Conduction/SoftwareCatalog * * @SuppressWarnings(PHPMD.CouplingBetweenObjects) + * + * @spec openspec/specs/settings-service/spec.md */ class Application extends App implements IBootstrap { @@ -129,6 +131,8 @@ public function __construct() * @param IRegistrationContext $context Registration context * * @return void + * + * @spec openspec/specs/settings-service/spec.md */ public function register(IRegistrationContext $context): void { @@ -413,7 +417,8 @@ function ($container) { } ); - // Register the registration moderation/approval-queue service. + // Register the registration/review moderation/approval-queue service + // (generalised to also moderate beoordeeling — softwarecatalog#375). $context->registerService( \OCA\SoftwareCatalog\Service\ModerationService::class, function ($container) { @@ -425,6 +430,35 @@ function ($container) { } ); + // Register the authenticated review-submission service (catalog-ratings, + // softwarecatalog#375). Author identity comes from IUserSession, never + // from client input. + $context->registerService( + \OCA\SoftwareCatalog\Service\ReviewService::class, + function ($container) { + return new \OCA\SoftwareCatalog\Service\ReviewService( + container: $container, + settingsService: $container->get(SettingsService::class), + userSession: $container->get(\OCP\IUserSession::class), + logger: $container->get('Psr\Log\LoggerInterface') + ); + } + ); + + // Register the public approved-only review aggregate/read service + // (catalog-ratings, softwarecatalog#375) — split from ReviewService + // to keep each class under the complexity budget. + $context->registerService( + \OCA\SoftwareCatalog\Service\ReviewAggregateService::class, + function ($container) { + return new \OCA\SoftwareCatalog\Service\ReviewAggregateService( + container: $container, + settingsService: $container->get(SettingsService::class), + logger: $container->get('Psr\Log\LoggerInterface') + ); + } + ); + // Register module version service (creates default 1.0.0 version for new modules). $context->registerService( ModuleVersionService::class, diff --git a/lib/Controller/ModerationController.php b/lib/Controller/ModerationController.php index fcc01628..3c6e5ae4 100644 --- a/lib/Controller/ModerationController.php +++ b/lib/Controller/ModerationController.php @@ -1,12 +1,16 @@ * SPDX-License-Identifier: EUPL-1.2 @@ -42,7 +47,10 @@ use OCP\IRequest; /** - * Admin-gated registration moderation queue. + * Admin-gated registration / review moderation queue. + * + * @spec openspec/specs/open-data-publishing/spec.md + * @spec openspec/specs/catalog-ratings/spec.md */ class ModerationController extends Controller { @@ -60,17 +68,20 @@ public function __construct( }//end __construct() /** - * List the pending anonymous registrations awaiting moderation. + * List the pending entries (of `type`) awaiting moderation. + * + * @param string $type The moderated object type ('organisatie', default, or 'beoordeeling'). * * @return JSONResponse `{ok, items}` or a 400. * * @AuthorizedAdminSetting(settings=OCA\SoftwareCatalog\Settings\SoftwareCatalogAdmin) * @spec openspec/specs/open-data-publishing/spec.md + * @spec openspec/specs/catalog-ratings/spec.md#requirement-review-moderation-must-reuse-the-existing-moderation-queue-mechanism-not-a-second-one */ #[AuthorizedAdminSetting(settings: SoftwareCatalogAdmin::class)] - public function pending(): JSONResponse + public function pending(string $type=ModerationService::MODERATED_TYPE): JSONResponse { - $result = $this->moderation->listPending(); + $result = $this->moderation->listPending(type: $type); if ($result['ok'] === false) { return new JSONResponse(data: ['message' => $result['reason']], statusCode: Http::STATUS_BAD_REQUEST); } @@ -79,19 +90,21 @@ public function pending(): JSONResponse }//end pending() /** - * Approve a pending registration (active + publish). + * Approve a pending entry (of `type`). * - * @param string $uuid The registration uuid. + * @param string $uuid The entry uuid. + * @param string $type The moderated object type ('organisatie', default, or 'beoordeeling'). * * @return JSONResponse `{ok, status}` or a 400. * * @AuthorizedAdminSetting(settings=OCA\SoftwareCatalog\Settings\SoftwareCatalogAdmin) * @spec openspec/specs/open-data-publishing/spec.md + * @spec openspec/specs/catalog-ratings/spec.md#requirement-a-newly-submitted-review-must-require-moderation-approval-before-becoming-public */ #[AuthorizedAdminSetting(settings: SoftwareCatalogAdmin::class)] - public function approve(string $uuid): JSONResponse + public function approve(string $uuid, string $type=ModerationService::MODERATED_TYPE): JSONResponse { - $result = $this->moderation->approve($uuid); + $result = $this->moderation->approve($uuid, type: $type); if ($result['ok'] === false) { return new JSONResponse(data: ['message' => $result['reason']], statusCode: Http::STATUS_BAD_REQUEST); } @@ -100,19 +113,21 @@ public function approve(string $uuid): JSONResponse }//end approve() /** - * Reject a pending registration. + * Reject a pending entry (of `type`). * - * @param string $uuid The registration uuid. + * @param string $uuid The entry uuid. + * @param string $type The moderated object type ('organisatie', default, or 'beoordeeling'). * * @return JSONResponse `{ok, status}` or a 400. * * @AuthorizedAdminSetting(settings=OCA\SoftwareCatalog\Settings\SoftwareCatalogAdmin) * @spec openspec/specs/open-data-publishing/spec.md + * @spec openspec/specs/catalog-ratings/spec.md#requirement-a-newly-submitted-review-must-require-moderation-approval-before-becoming-public */ #[AuthorizedAdminSetting(settings: SoftwareCatalogAdmin::class)] - public function reject(string $uuid): JSONResponse + public function reject(string $uuid, string $type=ModerationService::MODERATED_TYPE): JSONResponse { - $result = $this->moderation->reject($uuid); + $result = $this->moderation->reject($uuid, type: $type); if ($result['ok'] === false) { return new JSONResponse(data: ['message' => $result['reason']], statusCode: Http::STATUS_BAD_REQUEST); } diff --git a/lib/Controller/ReviewController.php b/lib/Controller/ReviewController.php new file mode 100644 index 00000000..ed4c7e27 --- /dev/null +++ b/lib/Controller/ReviewController.php @@ -0,0 +1,131 @@ + + * @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/catalog-ratings/spec.md + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Controller; + +use OCA\SoftwareCatalog\AppInfo\Application; +use OCA\SoftwareCatalog\Service\ReviewAggregateService; +use OCA\SoftwareCatalog\Service\ReviewService; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http; +use OCP\AppFramework\Http\Attribute\NoAdminRequired; +use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\PublicPage; +use OCP\AppFramework\Http\JSONResponse; +use OCP\IRequest; + +/** + * Authenticated review submission + public approved-only aggregate. + * + * @spec openspec/specs/catalog-ratings/spec.md + */ +class ReviewController extends Controller +{ + /** + * Constructor. + * + * @param IRequest $request The request. + * @param ReviewService $reviews The review submission service. + * @param ReviewAggregateService $aggregate The review aggregate/read service. + */ + public function __construct( + IRequest $request, + private readonly ReviewService $reviews, + private readonly ReviewAggregateService $aggregate, + ) { + parent::__construct(appName: Application::APP_ID, request: $request); + }//end __construct() + + /** + * Submit an authenticated review into the moderation queue. + * + * @param array $review The review payload (naam, waardering, beschrijvingKort/Lang). + * @param string $subjectType 'module' or 'dienst'. + * @param string $subjectId The uuid of the module/dienst being reviewed. + * + * @return JSONResponse `{ok, uuid, status}` (202 Accepted) or a 400/401. + * + * @NoAdminRequired + * @spec openspec/specs/catalog-ratings/spec.md#requirement-the-submitting-users-identity-must-be-bound-server-side-and-must-not-be-accepted-from-client-input + */ + #[NoAdminRequired] + public function submit(array $review=[], string $subjectType='', string $subjectId=''): JSONResponse + { + $result = $this->reviews->submit(payload: $review, subjectType: $subjectType, subjectId: $subjectId); + if ($result['ok'] === false) { + $statusCode = Http::STATUS_BAD_REQUEST; + if ($result['reason'] === 'not authenticated') { + $statusCode = Http::STATUS_UNAUTHORIZED; + } + + return new JSONResponse(data: ['message' => $result['reason']], statusCode: $statusCode); + } + + return new JSONResponse( + data: [ + 'ok' => true, + 'uuid' => $result['uuid'], + 'status' => $result['status'], + 'message' => 'Review received and queued for moderation', + ], + statusCode: Http::STATUS_ACCEPTED + ); + }//end submit() + + /** + * The approved-only aggregate (average + count) and a bounded list of + * approved reviews for a module or dienst. + * + * @param string $subjectType 'module' or 'dienst'. + * @param string $subjectId The uuid of the module/dienst. + * + * @return JSONResponse `{average, count, items}` or a 400. + * + * @PublicPage + * @NoCSRFRequired + * @spec openspec/specs/catalog-ratings/spec.md#requirement-module-and-dienst-detail-pages-must-display-an-aggregate-rating-computed-only-from-approved-reviews + */ + #[PublicPage] + #[NoCSRFRequired] + public function aggregate(string $subjectType='', string $subjectId=''): JSONResponse + { + $result = $this->aggregate->getAggregate(subjectType: $subjectType, subjectId: $subjectId); + if ($result['ok'] === false) { + return new JSONResponse(data: ['message' => $result['reason']], statusCode: Http::STATUS_BAD_REQUEST); + } + + return new JSONResponse( + data: [ + 'average' => $result['average'], + 'count' => $result['count'], + 'items' => $result['items'], + ] + ); + }//end aggregate() +}//end class diff --git a/lib/Service/ModerationService.php b/lib/Service/ModerationService.php index 76d29198..c5588de3 100644 --- a/lib/Service/ModerationService.php +++ b/lib/Service/ModerationService.php @@ -1,14 +1,26 @@ * SPDX-License-Identifier: EUPL-1.2 @@ -35,27 +48,40 @@ use Psr\Log\LoggerInterface; /** - * Lists + decides the anonymous-registration moderation queue. + * Lists + decides the organisatie / beoordeeling moderation queues. + * + * @spec openspec/specs/open-data-publishing/spec.md + * @spec openspec/specs/catalog-ratings/spec.md */ class ModerationService { /** - * The moderated catalog object type. + * The moderated catalog object type (default / legacy — organisatie). */ public const MODERATED_TYPE = 'organisatie'; /** - * Pending (awaiting moderation) state. + * The review moderated catalog object type. + */ + public const MODERATED_TYPE_REVIEW = 'beoordeeling'; + + /** + * Pending (awaiting moderation) state — shared field value across types. */ public const STATUS_PENDING = 'pending'; /** - * Approved (active, publishable) state. + * Approved (active, publishable) state for organisatie. */ public const STATUS_ACTIVE = 'active'; /** - * Rejected state. + * Approved (publicly visible) state for beoordeeling. + */ + public const STATUS_APPROVED = 'approved'; + + /** + * Rejected state — shared field value across types. */ public const STATUS_REJECTED = 'rejected'; @@ -74,15 +100,19 @@ public function __construct( }//end __construct() /** - * List the pending anonymous registrations awaiting moderation. + * List the pending entries (of the given type) awaiting moderation. + * + * @param string $type The moderated object type (default: organisatie). * * @return array{ok:bool, reason:string, items:array>} * * @spec openspec/specs/open-data-publishing/spec.md + * @spec openspec/specs/catalog-ratings/spec.md#requirement-review-moderation-must-reuse-the-existing-moderation-queue-mechanism-not-a-second-one */ - public function listPending(): array + public function listPending(string $type=self::MODERATED_TYPE): array { - $target = $this->resolveTarget(); + $config = $this->typeConfig(type: $type); + $target = $this->resolveTarget(type: $type); if ($target === null) { return ['ok' => false, 'reason' => 'register/schema not configured', 'items' => []]; } @@ -95,15 +125,15 @@ public function listPending(): array try { $objects = $objectService->searchObjects( query: [ - '@self' => ['register' => $target['register'], 'schema' => $target['schema']], - 'registratiestatus' => self::STATUS_PENDING, - '_limit' => 500, + '@self' => ['register' => $target['register'], 'schema' => $target['schema']], + $config['statusField'] => self::STATUS_PENDING, + '_limit' => 500, ], _rbac: false, _multitenancy: false ); } catch (\Throwable $e) { - $this->logger->error('ModerationService: listPending failed', ['error' => $e->getMessage()]); + $this->logger->error('ModerationService: listPending failed', ['type' => $type, 'error' => $e->getMessage()]); return ['ok' => false, 'reason' => 'query failed', 'items' => []]; } @@ -121,47 +151,62 @@ public function listPending(): array }//end listPending() /** - * Approve a pending registration: set it active AND publish it (set - * `publicatiedatum`), making it anonymously visible via the RBAC gate. + * Approve a pending entry: set it to the type's "approved" value and, for + * organisatie only, publish it (`publicatiedatum = now`) so the public + * RBAC read gate makes it anonymously visible. * - * @param string $uuid The registration uuid. + * @param string $uuid The entry uuid. + * @param string $type The moderated object type (default: organisatie). * * @return array{ok:bool, reason:string, status:?string} Result. * * @spec openspec/specs/open-data-publishing/spec.md + * @spec openspec/specs/catalog-ratings/spec.md#requirement-a-newly-submitted-review-must-require-moderation-approval-before-becoming-public */ - public function approve(string $uuid): array + public function approve(string $uuid, string $type=self::MODERATED_TYPE): array { + $config = $this->typeConfig(type: $type); return $this->decide( uuid: $uuid, - mutator: static function (array $data): array { - $data['registratiestatus'] = self::STATUS_ACTIVE; - $data['publicatiedatum'] = gmdate('Y-m-d\TH:i:sP'); - $data['depublicatiedatum'] = null; + type: $type, + mutator: static function (array $data) use ($config): array { + $data[$config['statusField']] = $config['approvedValue']; + if ($config['stampPublication'] === true) { + $data['publicatiedatum'] = gmdate('Y-m-d\TH:i:sP'); + $data['depublicatiedatum'] = null; + } + return $data; }, - status: self::STATUS_ACTIVE, + status: $config['approvedValue'], action: 'approved' ); }//end approve() /** - * Reject a pending registration: set it rejected and never give it a - * `publicatiedatum` (it stays invisible to anonymous readers). + * Reject a pending entry: set it to the type's "rejected" value; for + * organisatie, never give it a `publicatiedatum` (stays invisible). * - * @param string $uuid The registration uuid. + * @param string $uuid The entry uuid. + * @param string $type The moderated object type (default: organisatie). * * @return array{ok:bool, reason:string, status:?string} Result. * * @spec openspec/specs/open-data-publishing/spec.md + * @spec openspec/specs/catalog-ratings/spec.md#requirement-a-newly-submitted-review-must-require-moderation-approval-before-becoming-public */ - public function reject(string $uuid): array + public function reject(string $uuid, string $type=self::MODERATED_TYPE): array { + $config = $this->typeConfig(type: $type); return $this->decide( uuid: $uuid, - mutator: static function (array $data): array { - $data['registratiestatus'] = self::STATUS_REJECTED; - $data['publicatiedatum'] = null; + type: $type, + mutator: static function (array $data) use ($config): array { + $data[$config['statusField']] = self::STATUS_REJECTED; + if ($config['stampPublication'] === true) { + $data['publicatiedatum'] = null; + } + return $data; }, status: self::STATUS_REJECTED, @@ -170,22 +215,44 @@ public function reject(string $uuid): array }//end reject() /** - * Apply a moderation decision to a pending registration. + * Per-type moderation configuration: which field carries the moderation + * state, what its "approved" value is, and whether approval also stamps + * `publicatiedatum` (organisatie only — beoordeeling's public visibility + * is governed entirely by its own `status` field via the schema RBAC + * rule, no publication date involved). * - * Only a registration that is currently `pending` may be decided — this - * keeps the action idempotent and prevents re-deciding an already-approved - * entry (which would re-stamp its publicatiedatum). + * @param string $type The moderated object type. * - * @param string $uuid The registration uuid. + * @return array{statusField:string, approvedValue:string, stampPublication:bool} The config. + */ + private function typeConfig(string $type): array + { + if ($type === self::MODERATED_TYPE_REVIEW) { + return ['statusField' => 'status', 'approvedValue' => self::STATUS_APPROVED, 'stampPublication' => false]; + } + + return ['statusField' => 'registratiestatus', 'approvedValue' => self::STATUS_ACTIVE, 'stampPublication' => true]; + }//end typeConfig() + + /** + * Apply a moderation decision to a pending entry. + * + * Only an entry that is currently `pending` may be decided — this keeps + * the action idempotent and prevents re-deciding an already-approved + * entry (which would re-stamp its publicatiedatum for organisatie). + * + * @param string $uuid The entry uuid. + * @param string $type The moderated object type. * @param callable(array):array $mutator The state mutation. * @param string $status The resulting status. * @param string $action Log label. * * @return array{ok:bool, reason:string, status:?string} Result. */ - private function decide(string $uuid, callable $mutator, string $status, string $action): array + private function decide(string $uuid, string $type, callable $mutator, string $status, string $action): array { - $target = $this->resolveTarget(); + $config = $this->typeConfig(type: $type); + $target = $this->resolveTarget(type: $type); if ($target === null) { return ['ok' => false, 'reason' => 'register/schema not configured', 'status' => null]; } @@ -204,11 +271,11 @@ private function decide(string $uuid, callable $mutator, string $status, string _multitenancy: false ); } catch (\Throwable $e) { - return ['ok' => false, 'reason' => 'registration not found', 'status' => null]; + return ['ok' => false, 'reason' => 'entry not found', 'status' => null]; } if ($entity === null) { - return ['ok' => false, 'reason' => 'registration not found', 'status' => null]; + return ['ok' => false, 'reason' => 'entry not found', 'status' => null]; } $data = $this->toDataBag(object: $entity); @@ -218,8 +285,8 @@ private function decide(string $uuid, callable $mutator, string $status, string return ['ok' => false, 'reason' => 'peer-sourced entries cannot be moderated locally', 'status' => null]; } - if (($data['registratiestatus'] ?? null) !== self::STATUS_PENDING) { - return ['ok' => false, 'reason' => 'registration is not pending moderation', 'status' => null]; + if (($data[$config['statusField']] ?? null) !== self::STATUS_PENDING) { + return ['ok' => false, 'reason' => 'entry is not pending moderation', 'status' => null]; } $data = $mutator($data); @@ -233,23 +300,25 @@ private function decide(string $uuid, callable $mutator, string $status, string uuid: $uuid ); } catch (\Throwable $e) { - $this->logger->error('ModerationService: '.$action.' failed', ['uuid' => $uuid, 'error' => $e->getMessage()]); - return ['ok' => false, 'reason' => 'could not update registration', 'status' => null]; + $this->logger->error('ModerationService: '.$action.' failed', ['type' => $type, 'uuid' => $uuid, 'error' => $e->getMessage()]); + return ['ok' => false, 'reason' => 'could not update entry', 'status' => null]; } - $this->logger->info('ModerationService: registration '.$action, ['uuid' => $uuid, 'status' => $status]); + $this->logger->info('ModerationService: entry '.$action, ['type' => $type, 'uuid' => $uuid, 'status' => $status]); return ['ok' => true, 'reason' => $action, 'status' => $status]; }//end decide() /** - * Resolve the moderated register/schema. + * Resolve the moderated register/schema for a given type. + * + * @param string $type The moderated object type. * * @return array{register:int, schema:int}|null The target, or null. */ - private function resolveTarget(): ?array + private function resolveTarget(string $type): ?array { - $register = $this->settingsService->getRegisterIdForObjectType(self::MODERATED_TYPE); - $schema = $this->settingsService->getSchemaIdForObjectType(self::MODERATED_TYPE); + $register = $this->settingsService->getRegisterIdForObjectType($type); + $schema = $this->settingsService->getSchemaIdForObjectType($type); if ($register === null || $schema === null) { return null; } diff --git a/lib/Service/ReviewAggregateService.php b/lib/Service/ReviewAggregateService.php new file mode 100644 index 00000000..58470c17 --- /dev/null +++ b/lib/Service/ReviewAggregateService.php @@ -0,0 +1,307 @@ + + * @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/catalog-ratings/spec.md#requirement-module-and-dienst-detail-pages-must-display-an-aggregate-rating-computed-only-from-approved-reviews + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Service; + +use Psr\Container\ContainerInterface; +use Psr\Log\LoggerInterface; + +/** + * Approved-only aggregate (average + count) + a bounded review list. + * + * @spec openspec/specs/catalog-ratings/spec.md#requirement-module-and-dienst-detail-pages-must-display-an-aggregate-rating-computed-only-from-approved-reviews + */ +class ReviewAggregateService +{ + /** + * The catalog object type reviews live on. + */ + public const REVIEW_TYPE = 'beoordeeling'; + + /** + * Public-visible moderation state. + */ + public const STATUS_APPROVED = 'approved'; + + /** + * Subject types a review may be attached to. + * + * @var array + */ + public const SUBJECT_TYPES = ['module', 'dienst']; + + /** + * Constructor. + * + * @param ContainerInterface $container The DI container (lazy OR lookup). + * @param SettingsService $settingsService Resolves register/schema ids. + * @param LoggerInterface $logger Logger. + */ + public function __construct( + private readonly ContainerInterface $container, + private readonly SettingsService $settingsService, + private readonly LoggerInterface $logger + ) { + }//end __construct() + + /** + * The approved-only aggregate (average + count) and a bounded list of + * approved reviews for a module or dienst. + * + * @param string $subjectType 'module' or 'dienst'. + * @param string $subjectId The uuid of the module/dienst. + * + * @return array{ok:bool, reason:string, average:?float, count:int, items:array>} Result. + * + * @spec openspec/specs/catalog-ratings/spec.md#requirement-module-and-dienst-detail-pages-must-display-an-aggregate-rating-computed-only-from-approved-reviews + */ + public function getAggregate(string $subjectType, string $subjectId): array + { + if (in_array($subjectType, self::SUBJECT_TYPES, true) === false) { + return ['ok' => false, 'reason' => 'invalid subject type', 'average' => null, 'count' => 0, 'items' => []]; + } + + $approved = $this->fetchApprovedReviews(); + if ($approved['ok'] === false) { + return ['ok' => false, 'reason' => $approved['reason'], 'average' => null, 'count' => 0, 'items' => []]; + } + + $relationField = 'diensten'; + if ($subjectType === 'module') { + $relationField = 'modules'; + } + + $matched = $this->filterApprovedForSubject( + approvedReviews: $approved['items'], + relationField: $relationField, + subjectId: $subjectId + ); + + return [ + 'ok' => true, + 'reason' => 'ok', + 'average' => $this->averageRating(reviews: $matched), + 'count' => count($matched), + 'items' => array_slice($matched, 0, 10), + ]; + }//end getAggregate() + + /** + * Query every approved review (bounded, RBAC-bypassed — this method + * self-applies the `status = approved` predicate, which is exactly what + * the schema's own public RBAC rule allows anonymous readers to see). + * + * @return array{ok:bool, reason:string, items:array>} Result. + */ + private function fetchApprovedReviews(): array + { + $target = $this->resolveTarget(); + if ($target === null) { + return ['ok' => false, 'reason' => 'review register/schema not configured', 'items' => []]; + } + + $objectService = $this->getObjectService(); + if ($objectService === null) { + return ['ok' => false, 'reason' => 'ObjectService unavailable', 'items' => []]; + } + + try { + $objects = $objectService->searchObjects( + query: [ + '@self' => ['register' => $target['register'], 'schema' => $target['schema']], + 'status' => self::STATUS_APPROVED, + '_limit' => 1000, + ], + _rbac: false, + _multitenancy: false + ); + } catch (\Throwable $e) { + $this->logger->error('ReviewAggregateService: aggregate query failed', ['error' => $e->getMessage()]); + return ['ok' => false, 'reason' => 'query failed', 'items' => []]; + } + + $objectList = []; + if (is_array($objects) === true) { + $objectList = $objects; + } + + // Re-check `status` in PHP even though it was also passed as a query + // predicate above: `_rbac: false` bypasses OpenRegister's own + // enforcement of that predicate on some ObjectService implementations, + // so this is the actual enforcement point, not defensive redundancy. + $items = []; + foreach ($objectList as $object) { + $data = $this->toDataBag(object: $object); + if (($data['status'] ?? null) === self::STATUS_APPROVED) { + $items[] = $data; + } + } + + return ['ok' => true, 'reason' => 'ok', 'items' => $items]; + }//end fetchApprovedReviews() + + /** + * Narrow an already-approved review list down to the ones referencing + * the given subject. + * + * @param array> $approvedReviews The approved reviews. + * @param string $relationField 'modules' or 'diensten'. + * @param string $subjectId The uuid being looked for. + * + * @return array> The matching reviews. + */ + private function filterApprovedForSubject(array $approvedReviews, string $relationField, string $subjectId): array + { + $matched = []; + foreach ($approvedReviews as $data) { + $relationValue = ($data[$relationField] ?? null); + if ($this->relationContainsSubject(relationValue: $relationValue, subjectId: $subjectId) === true) { + $matched[] = $data; + } + } + + return $matched; + }//end filterApprovedForSubject() + + /** + * Whether a related-object array/scalar value references the given + * subject id. Tolerates both plain-uuid-array and nested-object-array + * storage shapes (`[""]` or `[{"id":""}]`) since this app has + * no single confirmed convention for a `related-object` array property. + * + * @param mixed $relationValue The raw property value. + * @param string $subjectId The uuid being looked for. + * + * @return bool True when the subject is referenced. + */ + private function relationContainsSubject(mixed $relationValue, string $subjectId): bool + { + if (is_array($relationValue) === false) { + return false; + } + + foreach ($relationValue as $entry) { + if (is_string($entry) === true && $entry === $subjectId) { + return true; + } + + if (is_array($entry) === true) { + $entryId = $entry['id'] ?? $entry['uuid'] ?? null; + if (is_string($entryId) === true && $entryId === $subjectId) { + return true; + } + } + } + + return false; + }//end relationContainsSubject() + + /** + * The average `waardering` across a review list, or null when empty. + * + * @param array> $reviews The reviews to average. + * + * @return float|null The rounded average, or null when $reviews is empty. + */ + private function averageRating(array $reviews): ?float + { + $count = count($reviews); + if ($count === 0) { + return null; + } + + $sum = 0.0; + foreach ($reviews as $review) { + $sum += (float) ($review['waardering'] ?? 0); + } + + return round($sum / $count, 2); + }//end averageRating() + + /** + * Resolve the register/schema reviews live in. + * + * @return array{register:int, schema:int}|null The target, or null. + */ + private function resolveTarget(): ?array + { + $register = $this->settingsService->getRegisterIdForObjectType(self::REVIEW_TYPE); + $schema = $this->settingsService->getSchemaIdForObjectType(self::REVIEW_TYPE); + if ($register === null || $schema === null) { + return null; + } + + return ['register' => (int) $register, 'schema' => (int) $schema]; + }//end resolveTarget() + + /** + * Normalise an ObjectService result item to a data bag (with its uuid). + * + * @param mixed $object The result item (ObjectEntity or array). + * + * @return array The data bag. + */ + private function toDataBag(mixed $object): array + { + if (is_array($object) === true) { + return $object; + } + + if (is_object($object) === true && method_exists($object, 'getObject') === true) { + $data = $object->getObject(); + if (method_exists($object, 'getUuid') === true && empty($data['id']) === true) { + $data['id'] = $object->getUuid(); + } + + if (is_array($data) === true) { + return $data; + } + + return []; + } + + return []; + }//end toDataBag() + + /** + * Get the OpenRegister ObjectService from the DI container. + * + * @return object|null The object service, or null when OR is absent. + */ + private function getObjectService(): ?object + { + try { + return $this->container->get('OCA\\OpenRegister\\Service\\ObjectService'); + } catch (\Throwable $e) { + $this->logger->error('ReviewAggregateService: ObjectService unavailable', ['error' => $e->getMessage()]); + return null; + } + }//end getObjectService() +}//end class diff --git a/lib/Service/ReviewService.php b/lib/Service/ReviewService.php new file mode 100644 index 00000000..1a2bd51c --- /dev/null +++ b/lib/Service/ReviewService.php @@ -0,0 +1,406 @@ +getDisplayName()` before the object is + * persisted, and every submission is forced to `status = pending` — only an + * admin moderation decision (`ModerationService`, reusing the existing + * organisatie moderation pattern) may change that. + * + * @category Service + * @package OCA\SoftwareCatalog\Service + * @author Conduction b.v. + * @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/catalog-ratings/spec.md + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Service; + +use OCP\IUser; +use OCP\IUserSession; +use Psr\Container\ContainerInterface; +use Psr\Log\LoggerInterface; + +/** + * Authenticated submission of a beoordeeling (review). + * + * @spec openspec/specs/catalog-ratings/spec.md + */ +class ReviewService +{ + /** + * The catalog object type reviews live on. + */ + public const REVIEW_TYPE = 'beoordeeling'; + + /** + * Moderation state of a freshly-submitted review — mirrors + * `ModerationService::STATUS_PENDING` but the two are intentionally + * decoupled constants (different schemas, different field names). + */ + public const STATUS_PENDING = 'pending'; + + /** + * Subject types a review may be attached to. + * + * @var array + */ + public const SUBJECT_TYPES = ['module', 'dienst']; + + /** + * Required fields on a review submission payload. + * + * @var array + */ + public const REQUIRED_FIELDS = ['naam', 'waardering']; + + /** + * Maximum number of fields accepted on a submission (anti-abuse). + */ + public const MAX_FIELDS = 30; + + /** + * Maximum length of any single string value (anti-abuse). + */ + public const MAX_FIELD_LENGTH = 5000; + + /** + * Caller-controlled keys that MUST be stripped from a submission — the + * author, moderation state, identifiers, ownership, and provenance are + * always server-controlled, never client-supplied. + * + * @var array + */ + public const FORBIDDEN_KEYS = [ + 'auteur', + 'status', + 'id', + 'uuid', + '_owner', + '_organisation', + '_source', + 'modules', + 'diensten', + 'koppelingen', + 'gebruik', + ]; + + /** + * Constructor. + * + * @param ContainerInterface $container The DI container (lazy OR lookup). + * @param SettingsService $settingsService Resolves register/schema ids. + * @param IUserSession $userSession The Nextcloud user session. + * @param LoggerInterface $logger Logger. + */ + public function __construct( + private readonly ContainerInterface $container, + private readonly SettingsService $settingsService, + private readonly IUserSession $userSession, + private readonly LoggerInterface $logger + ) { + }//end __construct() + + /** + * Submit an authenticated review into the moderation queue. + * + * Requires an active session (never anonymous). Strips every + * client-controlled privileged key, re-derives `auteur` from the + * session, binds the subject (`modules`/`diensten`), and forces + * `status = pending`. + * + * @param array $payload The review payload + * (naam, waardering, + * beschrijvingKort/Lang). + * @param string $subjectType 'module' or 'dienst'. + * @param string $subjectId The uuid of the module/dienst being reviewed. + * + * @return array{ok:bool, reason:string, uuid:?string, status:?string} Result. + * + * @spec openspec/specs/catalog-ratings/spec.md#requirement-the-submitting-users-identity-must-be-bound-server-side-and-must-not-be-accepted-from-client-input + */ + public function submit(array $payload, string $subjectType, string $subjectId): array + { + $user = $this->userSession->getUser(); + $reason = $this->guardSubmission(user: $user, subjectType: $subjectType, subjectId: $subjectId, payload: $payload); + if ($reason !== null) { + return ['ok' => false, 'reason' => $reason, 'uuid' => null, 'status' => null]; + } + + $target = $this->resolveTarget(); + if ($target === null) { + return ['ok' => false, 'reason' => 'review register/schema not configured', 'uuid' => null, 'status' => null]; + } + + $objectService = $this->getObjectService(); + if ($objectService === null) { + return ['ok' => false, 'reason' => 'ObjectService unavailable', 'uuid' => null, 'status' => null]; + } + + // $user is guaranteed non-null past guardSubmission()'s "not authenticated" check. + $clean = $this->buildSubmissionObject(payload: $payload, user: $user, subjectType: $subjectType, subjectId: $subjectId); + + try { + $entity = $objectService->saveObject( + object: $clean, + register: $target['register'], + schema: $target['schema'] + ); + } catch (\Throwable $e) { + $this->logger->error('ReviewService: submit failed', ['error' => $e->getMessage()]); + return ['ok' => false, 'reason' => 'could not store review', 'uuid' => null, 'status' => null]; + } + + $uuid = $this->entityUuid(entity: $entity); + $this->logger->info( + 'ReviewService: review queued (pending)', + ['uuid' => $uuid, 'auteur' => $clean['auteur'], 'subjectType' => $subjectType, 'subjectId' => $subjectId] + ); + + return ['ok' => true, 'reason' => 'queued for moderation', 'uuid' => $uuid, 'status' => self::STATUS_PENDING]; + }//end submit() + + /** + * Pre-flight guards for a submission: authentication, subject shape, and + * payload validation. Split out of submit() to keep both methods under + * the cyclomatic-complexity budget. + * + * @param IUser|null $user The authenticated user, or null. + * @param string $subjectType 'module' or 'dienst'. + * @param string $subjectId The uuid of the module/dienst. + * @param array $payload The raw review payload. + * + * @return string|null The rejection reason, or null when the submission may proceed. + */ + private function guardSubmission(?IUser $user, string $subjectType, string $subjectId, array $payload): ?string + { + if ($user === null) { + return 'not authenticated'; + } + + if (in_array($subjectType, self::SUBJECT_TYPES, true) === false) { + return 'invalid subject type'; + } + + if (trim($subjectId) === '') { + return 'subject id is required'; + } + + return $this->validate(payload: $payload); + }//end guardSubmission() + + /** + * Build the object to persist: sanitised payload + server-derived + * author, forced pending status, and the subject binding. + * + * @param array $payload The raw review payload. + * @param IUser $user The authenticated user. + * @param string $subjectType 'module' or 'dienst'. + * @param string $subjectId The uuid of the module/dienst. + * + * @return array The object ready for ObjectService::saveObject(). + * + * @spec openspec/specs/catalog-ratings/spec.md#requirement-the-submitting-users-identity-must-be-bound-server-side-and-must-not-be-accepted-from-client-input + */ + private function buildSubmissionObject(array $payload, IUser $user, string $subjectType, string $subjectId): array + { + $clean = $this->sanitise(payload: $payload); + + // Author identity is ALWAYS re-derived from the session — a + // client-supplied `auteur` was already stripped above and is never + // read back from $payload. + $displayName = trim($user->getDisplayName()); + $authorName = $user->getUID(); + if ($displayName !== '') { + $authorName = $displayName; + } + + $clean['auteur'] = $authorName; + $clean['status'] = self::STATUS_PENDING; + + $relationField = 'diensten'; + if ($subjectType === 'module') { + $relationField = 'modules'; + } + + $clean[$relationField] = [$subjectId]; + + return $clean; + }//end buildSubmissionObject() + + /** + * Validate a review payload (anti-abuse + required fields). + * + * @param array $payload The payload. + * + * @return string|null The rejection reason, or null when valid. + */ + private function validate(array $payload): ?string + { + if ($payload === []) { + return 'empty payload'; + } + + if (count($payload) > self::MAX_FIELDS) { + return 'too many fields'; + } + + $reason = $this->validateRequiredFields(payload: $payload); + if ($reason !== null) { + return $reason; + } + + $reason = $this->validateRating(payload: $payload); + if ($reason !== null) { + return $reason; + } + + return $this->validateFieldSizes(payload: $payload); + }//end validate() + + /** + * Every field in REQUIRED_FIELDS is present and non-empty. + * + * @param array $payload The payload. + * + * @return string|null The rejection reason, or null when valid. + */ + private function validateRequiredFields(array $payload): ?string + { + foreach (self::REQUIRED_FIELDS as $field) { + if (array_key_exists($field, $payload) === false || $payload[$field] === null || $payload[$field] === '') { + return 'missing required field: '.$field; + } + } + + return null; + }//end validateRequiredFields() + + /** + * `waardering` is numeric and within the 1-10 range. + * + * @param array $payload The payload (REQUIRED_FIELDS already confirmed present). + * + * @return string|null The rejection reason, or null when valid. + */ + private function validateRating(array $payload): ?string + { + $rating = $payload['waardering']; + if (is_numeric($rating) === false || (int) $rating < 1 || (int) $rating > 10) { + return 'waardering must be between 1 and 10'; + } + + return null; + }//end validateRating() + + /** + * No string value exceeds MAX_FIELD_LENGTH (anti-abuse). + * + * @param array $payload The payload. + * + * @return string|null The rejection reason, or null when valid. + */ + private function validateFieldSizes(array $payload): ?string + { + foreach ($payload as $value) { + if (is_string($value) === true && strlen($value) > self::MAX_FIELD_LENGTH) { + return 'field value exceeds the maximum length'; + } + } + + return null; + }//end validateFieldSizes() + + /** + * Strip caller-controlled / privileged keys from a submission payload. + * + * @param array $payload The raw payload. + * + * @return array The sanitised payload. + */ + private function sanitise(array $payload): array + { + foreach (self::FORBIDDEN_KEYS as $key) { + unset($payload[$key]); + } + + return $payload; + }//end sanitise() + + /** + * Resolve the register/schema reviews live in. + * + * @return array{register:int, schema:int}|null The target, or null. + */ + private function resolveTarget(): ?array + { + $register = $this->settingsService->getRegisterIdForObjectType(self::REVIEW_TYPE); + $schema = $this->settingsService->getSchemaIdForObjectType(self::REVIEW_TYPE); + if ($register === null || $schema === null) { + return null; + } + + return ['register' => (int) $register, 'schema' => (int) $schema]; + }//end resolveTarget() + + /** + * The uuid of a saved entity (handles entity or array result shapes). + * + * @param mixed $entity The saveObject result. + * + * @return string|null The uuid, or null. + */ + private function entityUuid(mixed $entity): ?string + { + if (is_object($entity) === true && method_exists($entity, 'getUuid') === true) { + $uuid = $entity->getUuid(); + if (is_string($uuid) === true) { + return $uuid; + } + + return null; + } + + if (is_array($entity) === true) { + $uuid = $entity['id'] ?? $entity['uuid'] ?? null; + if (is_string($uuid) === true) { + return $uuid; + } + + return null; + } + + return null; + }//end entityUuid() + + /** + * Get the OpenRegister ObjectService from the DI container. + * + * @return object|null The object service, or null when OR is absent. + */ + private function getObjectService(): ?object + { + try { + return $this->container->get('OCA\\OpenRegister\\Service\\ObjectService'); + } catch (\Throwable $e) { + $this->logger->error('ReviewService: ObjectService unavailable', ['error' => $e->getMessage()]); + return null; + } + }//end getObjectService() +}//end class diff --git a/lib/Service/SettingsService.php b/lib/Service/SettingsService.php index 66dba0c6..a3aa467e 100644 --- a/lib/Service/SettingsService.php +++ b/lib/Service/SettingsService.php @@ -64,6 +64,8 @@ * @SuppressWarnings(PHPMD.Superglobals) * @SuppressWarnings(PHPMD.CamelCaseVariableName) * @SuppressWarnings(PHPMD.CamelCaseParameterName) + * + * @spec openspec/specs/settings-service/spec.md */ class SettingsService { @@ -154,6 +156,8 @@ public function isOpenRegisterInstalled(?string $minVersion=self::MIN_OPENREGIST * Checks if OpenRegister is enabled * * @return bool True if OpenRegister is enabled + * + * @spec openspec/specs/settings-service/spec.md */ public function isOpenRegisterEnabled(): bool { @@ -2286,6 +2290,8 @@ public function getAllGroups(): array * @param array $settings Raw settings from getEmailSettings(). * * @return array The settings, safe to return over HTTP. + * + * @spec openspec/specs/settings-service/spec.md */ public function redactEmailSecrets(array $settings): array { @@ -6304,6 +6310,8 @@ function ($result) { * Get catalog location * * @return string The catalog location URL + * + * @spec openspec/specs/settings-service/spec.md */ public function getCatalogLocation(): string { @@ -6316,6 +6324,8 @@ public function getCatalogLocation(): string * @param string $location The catalog location URL. * * @return void + * + * @spec openspec/specs/settings-service/spec.md */ public function setCatalogLocation(string $location): void { @@ -7144,14 +7154,33 @@ public function setEolSyncStatus(array $status): void * merged by key union (recursing on shared keys); list arrays are concatenated; * scalars in the fragment overwrite the base. Disjoint fragments never collide. * - * @param array $base The accumulated config. - * @param array $overlay The fragment to merge in. + * EXCEPTION (catalog-ratings, softwarecatalog#375): any key literally named + * `authorization` switches its entire subtree to REPLACE semantics for list + * values, instead of the general concatenation above. Concatenating an RBAC + * rule list is a fail-OPEN trap: if the base already carries an unconditional + * entry such as `read: ["public"]`, concatenating a narrower overlay rule onto + * it produces `["public", {...}]` — the dangerous unconditional entry is still + * present, so the schema stays fully world-readable no matter what the overlay + * adds (the same class of bug as OR's veto-after-grant trap, or#2025, one layer + * up in the config-merge step). A fragment narrowing a schema's authorization + * MUST be able to remove a dangerous base entry outright, so `authorization` + * lists are replaced wholesale. This is scoped to that one key name — every + * other merge (including every fragment that predates this one) is unaffected. + * + * @param array $base The accumulated config. + * @param array $overlay The fragment to merge in. + * @param bool $replaceLists Whether list values in this subtree replace + * (true, inside an `authorization` block) + * rather than concatenate (false, the general + * case). * * @return array The merged config. */ - private static function deepMergeConfig(array $base, array $overlay): array + private static function deepMergeConfig(array $base, array $overlay, bool $replaceLists=false): array { foreach ($overlay as $key => $value) { + $childReplaceLists = ($replaceLists === true || $key === 'authorization'); + if (is_array($value) === true && isset($base[$key]) === true && is_array($base[$key]) === true @@ -7159,14 +7188,18 @@ private static function deepMergeConfig(array $base, array $overlay): array $baseIsList = ($base[$key] === [] || array_keys($base[$key]) === range(0, (count($base[$key]) - 1))); $overlayIsList = ($value === [] || array_keys($value) === range(0, (count($value) - 1))); if ($baseIsList === true && $overlayIsList === true) { - $base[$key] = array_merge($base[$key], $value); + if ($childReplaceLists === true) { + $base[$key] = $value; + } else { + $base[$key] = array_merge($base[$key], $value); + } } else { - $base[$key] = self::deepMergeConfig(base: $base[$key], overlay: $value); + $base[$key] = self::deepMergeConfig(base: $base[$key], overlay: $value, replaceLists: $childReplaceLists); } } else { $base[$key] = $value; } - } + }//end foreach return $base; diff --git a/lib/Settings/register.d/catalog-ratings.json b/lib/Settings/register.d/catalog-ratings.json new file mode 100644 index 00000000..bb5dd896 --- /dev/null +++ b/lib/Settings/register.d/catalog-ratings.json @@ -0,0 +1,90 @@ +{ + "components": { + "schemas": { + "beoordeeling": { + "properties": { + "auteur": { + "type": "string", + "description": "Display name of the submitting user. Stamped server-side by ReviewService from the authenticated Nextcloud session at submission time — a client-supplied value is always discarded and never persisted.", + "visible": true, + "facetable": false, + "title": "Auteur", + "order": 10, + "example": "Bijvoorbeeld: Jan Jansen" + }, + "status": { + "type": "string", + "enum": [ + "pending", + "approved", + "rejected" + ], + "default": "pending", + "description": "Moderation status. Every new review is forced to 'pending' server-side by ReviewService regardless of client input; only an admin approval/rejection decision (ModerationService, reusing the organisatie moderation pattern) may transition it. The public RBAC read rule below only ever matches 'approved'.", + "visible": true, + "facetable": true, + "title": "Status", + "order": 11, + "example": "Bijvoorbeeld: pending" + } + }, + "authorization": { + "create": [ + "vng-raadpleger", + "software-catalog-users", + "software-catalog-admins", + "organisaties-beheerder", + "organisatie-beheerder", + "gebruik-raadpleger", + "gebruik-beheerder", + "functioneel-beheerder", + "ambtenaar", + "aanbod-beheerder" + ], + "read": [ + { + "group": "public", + "match": { + "status": "approved" + } + }, + "aanbod-beheerder", + "ambtenaar", + "functioneel-beheerder", + "gebruik-beheerder", + "vng-raadpleger", + "software-catalog-users", + "software-catalog-admins", + "organisatie-beheerder", + "organisaties-beheerder", + "gebruik-raadpleger" + ], + "update": [ + "software-catalog-admins", + { + "group": "organisatie-beheerder", + "match": { + "_organisation": "$organisation" + } + }, + { + "group": "organisaties-beheerder", + "match": { + "_organisation": "$organisation" + } + }, + { + "group": "functioneel-beheerder", + "match": { + "_organisation": "$organisation" + } + } + ], + "delete": [ + "software-catalog-admins" + ] + } + } + } + } +} 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 `