feat(reviews): catalog-ratings — moderated ratings + close the beoordeeling authorization hole (#375) - #399
Merged
Merged
Conversation
Full OpenSpec cycle for closing the beoordeeling (review) schema's authorization hole and shipping moderated ratings/testimonials (softwarecatalog#375). Archives the change and lands the canonical capability spec at openspec/specs/catalog-ratings/spec.md (10 requirements, each with positive + negative scenarios).
…#375)
beoordeeling shipped with authorization: {"read": ["public"]} and NO
create/update/delete rules at all, no author binding, and no owning-
organisation binding — world-readable with undefined write rules and
no accountability.
Adds lib/Settings/register.d/catalog-ratings.json (ADR-037 fragment,
never editing the monolith): auteur + status properties, and explicit
create/update/delete authorization. Public read is now conditioned on
status=approved only (fail-closed); update is narrowed to admin/org-
scoped groups (owner-privilege covers the author's own review); delete
is restricted to catalog-admins only.
Fixes a merge-time trap this depends on: SettingsService::deepMergeConfig()
concatenates list values by default (correct for e.g. extending
`required`), which would have left the base's bare "public" read entry
in place no matter what the fragment added — the same class of bug as
OR's veto-after-grant trap (or#2025), one layer up in the config-merge
step. Any key literally named `authorization` now replaces list values
instead of concatenating them, scoped narrowly so the one pre-existing
fragment (contracts-to-decidesk.json, which never touches
`authorization`) is unaffected.
Tests: DeepMergeAuthorizationTest covers the replace-vs-concatenate
fix, the real fragment leaving no bare "public" entry, and the narrow
update/delete authorization lists.
…eration reuse ReviewController/ReviewService (new): POST /api/reviews requires an authenticated session (#[NoAdminRequired], normal CSRF — never anonymous, unlike IntakeController). Author identity is ALWAYS re-derived from IUserSession server-side; auteur/status/id/_owner/ _organisation/_source/modules/diensten/koppelingen/gebruik are stripped from the client payload before persisting. Every submission is forced to status=pending. ReviewAggregateService (new, split out of ReviewService to stay under the ExcessiveClassComplexity budget): GET /api/reviews/aggregate is #[PublicPage] and computes the approved-only average+count and a bounded review list in PHP against ObjectService::searchObjects() results, rather than a declarative manifest stat-widget filter — this app has no confirmed precedent that the aggregation backend's filter supports array-containment matching on beoordeeling.modules/diensten (an array-of-related-object property), so the safer, fully unit-testable path was chosen over an unverified one. ModerationService/ModerationController generalised (not duplicated) to a second moderated type: type=beoordeeling (field `status`, approved/ rejected) alongside the existing type=organisatie default (registratiestatus, active/rejected) — every parameter defaults to the exact prior organisatie behavior, so IntakeModerationTest's existing assertions (none of which pass a type) are unaffected; added type=beoordeeling coverage (approve/reject/listPending/non-pending guard) alongside them. Tests: ReviewServiceTest (unauthenticated refusal, client-supplied author ignored, forced-pending, server-controlled subject binding, validation) + ReviewAggregateServiceTest (approved-only, cross-subject exclusion, zero-reviews case, dienst subject matching).
…eview moderation ReviewsPanel.vue (new): ModuleDetail bodyWidget (same escape-hatch mechanism as ContractApprovalPanel) showing the approved-only aggregate rating + a bounded review list + a "Write a review" action. SubmitReviewModal.vue (new, own file per ADR-012): title + 1-10 rating (NcSelect with inputLabel) + testimonial. Deliberately has no "your name" field — the server always derives the author from the session. reviewForm.js / reviewAggregate.js (new): pure helpers extracted out of the two components so the review-payload shape (critically: never including auteur/status) and the aggregate query/response handling are unit-testable without mounting NcDialog-based components, matching this repo's established convention (contractCost.js, moderationItem.js). ModerationQueue.vue generalised via `type`/`entityLabel`/copy props so the beoordeeling review queue reuses the exact same component as the organisatie registration queue (no second moderation UI); every prop defaults to the original organisatie copy so the existing settings instance is visually unchanged. A second instance (type="beoordeeling") is wired into SoftwareCatalogSettings.vue as "Review moderation". Fixes the manifest's dead Reviews-index columns: titel/auteur/score/ datum (only "auteur" ever matched a description; none matched the schema) replaced with the real properties naam/auteur/waardering/status. Tests: reviewForm.spec.js (payload never contains auteur/status/id/ _owner), reviewAggregate.spec.js (query building + response normalisation, incl. malformed-response defaults).
Covers every new user-facing string introduced by the catalog-ratings
feature (submit-review modal, aggregate panel, generalised moderation
queue copy) in both l10n/nl.{js,json} and l10n/en_US.{js,json}.
Documents the authorization hole that existed, the submit/moderation/ aggregate flows, the authorization summary table, and known gaps (no DienstDetail page yet; residual direct-API risk for already- authorized groups). Screenshots not captured — no live instance available without touching the shared dev environment, per this repo's existing organisation-merge.md precedent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #375. Evidence: VNG issue #49 (peer municipalities want experiences/ratings when selecting software).
🔴 Security hole closed (worth shipping on its own)
beoordeelingshipped withauthorization: {"read": ["public"]}— no create/update/delete rules, no author, no owning organisation, and a schema description admitting it was never actually used. Now:register.d/catalog-ratings.jsonfragment (monolith untouched) addsauteur/status+ explicit create/update/delete rules.readis now{group: public, match: {status: approved}}— fail closed.IUserSession;auteur/status/_ownerare stripped from client payloads. Update is narrowed to admin/org-scoped groups; delete is admin-only.🐛 Found a real bug in the fragment-merge mechanism itself
SettingsService::deepMergeConfig()concatenates list values, so a fragment could never have overridden the base's bare"public"read entry — the fail-closed intent would have been silently defeated. Fixed so a key namedauthorizationreplaces list values rather than concatenating, scoped narrowly so the one pre-existing fragment is unaffected. Same class as the veto-after-grant trap (or#2025), one layer up in config merge.Feature
Generalised the existing
ModerationService/ModerationQueue.vuevia atypeparam (type=beoordeeling) rather than building a second moderation mechanism — every default preserves the originalorganisatiebehaviour byte-for-byte.ReviewAggregateServicecomputes an approved-only average+count in PHP rather than trusting a declarative filter over an array-of-relations. Fixed the deadtitel/auteur/score/datumcolumns on the Reviews index.Tests: 460 PHPUnit (32 new/changed) + 210 vitest (9 new), only the pre-existing #393 CSV error. Negative cases covered: unauthenticated submission refused, client-supplied author ignored, pending/rejected excluded from the public aggregate.
openspec validate --specs --strict53/53. Archived (10 requirements).Documented residual: a user already in an authorised create-group could still write via OpenRegister's generic object API, bypassing
ReviewController. That risk is shared by every schema in this app and is recorded in the proposal and feature doc rather than silently ignored.🤖 Generated with Claude Code