Skip to content

Feat/auth image collections [OWTR #1] - #548

Open
JohanHjelsethStorstad wants to merge 87 commits into
mainfrom
feat/auth-image-collections
Open

Feat/auth image collections [OWTR #1]#548
JohanHjelsethStorstad wants to merge 87 commits into
mainfrom
feat/auth-image-collections

Conversation

@JohanHjelsethStorstad

@JohanHjelsethStorstad JohanHjelsethStorstad commented Jul 27, 2026

Copy link
Copy Markdown
Member

Image system rewrite: special/dynamic collections + double-level visibility

This is PR # 1 in the one week till realese series. These should be merged sequentially

Rewrites the image service from a single flat "images" service into a
sub-service with per-domain implementations, and puts every dynamic image
collection behind a two-level visibility system (who may see it, who may
administrate it) with a full admin UI.

main..HEAD is 82 commits / 374 files. This is a large refactor — the sections
below are ordered roughly by how much reviewer attention they need.


1. Image service: one service → sub-service + implementations

The old src/services/images/{actions,operations,schemas,auth,types,collections/}
is deleted and replaced by:

Path Role
images/subservice/ The generic image/collection sub-operations every implementer builds on (uploadImage, updateCollection, destroyCollection, readPageOfImagesInCollection, …)
images/subservice/special/implement.ts implementSpecialCollection() — how a domain service claims one SpecialCollection and gets typed operations for it
images/dynamic/ Ordinary user-created collections (the ones with visibility)
images/standard/ The STANDARDIMAGES collection + readStandardImage / regeneration-from-config
images/specialPanels/ Assembles every special collection's panel operations for the frontend

Domain services now own their own image collection rather than reaching into a
shared one — users (PROFILE_IMAGES), ombul (OMBULCOVERS), committees
(COMMITTEELOGOS), flairs (FLAIRIMAGES).

Schema: Flair.image and Committee.logoImage now point at Image instead
of CmsImage. Article/ArticleCategory/NewsArticle moved out of the CMS
schema file to reflect that they are implementations, not CMS primitives.

Standard images are no longer seeded rows that can drift: each is declared in
StandardImageConfig with a source file in the new top-level standard_store/,
and readStandardImage regenerates it from config if it is missing or has
escaped the standard collection. The SpecialCmsImage enum shrank accordingly
(FRONTPAGE_LOGO, NOT_FOUND, LOADER_IMAGE, nav/footer buttons … are gone —
those are standard images now).

Store (src/services/store/src/lib/store/) became a factory,
implementStore(), so each service gets a namespaced store with its own allowed
extensions — and deleting an image now actually deletes the files.

2. Double-level visibility

Visibility is attached twice to every ImageCollection
(visibilityRegularId / visibilityAdminId).
implementDoubleLevelVisibilityOperations() gives an owning service a
readDoubleLevelMatrix plus updateRegularLevel / updateAdminLevel, each with
its own authorizer and an ownership check that a passed visibilityId really is
that owner's level.

New authorizer RequireLevelFromDoubleLevelVisibility (level: REGULAR | ADMIN,
with an optional bypass permission — IMAGE_ADMIN for images).

Invariant: the admin level must always be a sub-visibility of the regular
level; an administrator who cannot see what they administrate is a broken state.
Enforced via isSubVisibility on the matrix the update would produce, so it is
checked before anything is written and either level can still be updated alone.

Note one non-obvious consequence: an empty admin level means everyone
administrates, which is not a subset of a narrowed regular level. So on a fresh
collection the admin level must be narrowed before the regular level can be.

beforeRun (framework): enforcing this needed a hook, so .implement() gained
an optional beforeRun?: BeforeRun<…> taking the same args as authorizer /
ownershipCheck. It runs after auth and ownership, before the operation, and
throws to abort. This is the general place for implementer invariants a
sub-operation cannot state on its own — ownershipCheck stays for "does this
implementer own the resource".

Bug fix: visibilityOperations.update called omegaOrderOperations.readCurrent({})
without bypassing auth. That order is only a placeholder stored on ACTIVE
conditions, but requiring the caller to also hold OMEGA_ORDER_READ — not a
default or membership permission — meant essentially nobody could save a
visibility change
. Now { bypassAuth: true }, matching every other internal
readCurrent call.

3. Frontend

  • VisibilityAdmin — new editor for one matrix: requirements (ANDed) each
    holding conditions (ORed), ACTIVE vs ORDER per condition, group/order pickers.
    CollectionAdmin mounts it twice, once per level.
  • DoubleLevelVisibilityDescription — human-readable "Kan se / Kan administrere",
    shown on the collection page. Generic over any double-level service, grouped with
    VisibilityAdmin under _components/Visibility/.
  • CollectionAdmin runs one authorizer per action (upload-one, upload-many,
    update, destroy, update-regular, update-admin) instead of gating the whole panel
    on updateCollection. The count of auth checks now matches the count of actions.
  • The visibility matrix is fetched server-side in page.tsx and threaded down. If
    that read fails it becomes null and the visibility button is simply hidden
    (rather than 404-ing the page); the remaining checks fall closed against an
    unsatisfiable placeholder, so the IMAGE_ADMIN bypass still works.
  • ImageListImagePanel, which serves both special and dynamic collections;
    CollectionCard no longer forces being a link (new CollectionCardLink);
    new StandardImageServer / StandardImageClient; new ClientData provider
    replaces DefaultPermissions and the image-selection contexts.
  • ImageUploader is now just a Form. Callers decide on popup vs inline —
    committee logos and ombul covers render it beside the current image, EditOverlay
    is no longer used for special-collection uploads, and title is caller-supplied.
  • Flair image editing moved out of the reusable Flair component into a button in
    the /admin/flairs list. (Flair had briefly become a client component receiving
    a Session class instance as a prop, which crashes RSC serialization.)
  • Fixes: invisible "add part" buttons on the article editor (secondary matched the
    page background), and collection cards collapsing horizontally in the CMS image
    editor (missing flex-shrink: 0).

4. Shared utils

Pulled out of services into src/lib/groups/ so they can be reused and so none of
them throw:

  • inferGroupName, checkGroupValidity, groupOptions (orderOptions / findGroup).
  • checkGroupValidity no longer throws — it returns
    { valid: true, group } | { valid: false }. The throwing behaviour lives in
    assertGroupValidity in the service layer, which all 5 existing call sites use.
  • orderOptions deduplicates the identical order-range logic that VisibilityAdmin
    and UserList each had.
  • describeMatrix moved to auth/visibility/ next to checkVisibility /
    isSubVisibility.

5. Seeding

Migrated to defineSeedOperation (context-aware, so seeders stop hand-threading
prisma/session). Fixes a standard-image race on seed, stops logging expected
NOT-FOUNDs during upsert, and makes the OmegaWeb migration use the new image system.

6. Tests

New tests/services/visibility.test.ts (29 tests) and
tests/services/dynamicImages.test.ts (21 tests) — 50 total, all passing.

  • Unit coverage of checkVisibility (AND across requirements, OR within one,
    ACTIVE vs ORDER, empty = everyone) and isSubVisibility.
  • visibilityOperations create/update/destroy: update replaces rather than
    appends, ORDER conditions keep their order, cascade on destroy.
  • A standalone double-level implementation built directly on
    implementDoubleLevelVisibilityOperations with no owning domain model — its
    implementationParams are just the two visibility ids. Covers both level updates,
    the per-level authorizers, the ownership check (wrong level / another owner's
    visibility → DISSALLOWED), and the sub/super invariant in both directions.
  • Dynamic images, focused on authorization: regular level gates reading, admin level
    gates updating/destroying, IMAGE_ADMIN bypasses both, the paging filter hides
    collections the session may not see, showOnlyCollectionsSessionAdministrates
    filters on the admin level, and special collections stay unreachable through the
    dynamic service.

JohanHjelsethStorstad and others added 30 commits April 16, 2026 11:30
…tter reflect that they are not cms but implementation, just like /services folder does

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@JohanHjelsethStorstad JohanHjelsethStorstad changed the title Feat/auth image collections Feat/auth image collections [OWTR #1] Jul 27, 2026
@JohanHjelsethStorstad

Copy link
Copy Markdown
Member Author

I suggest someone to user-test this that the visibility authoring makes sense

@JohanHjelsethStorstad

Copy link
Copy Markdown
Member Author

@theodorklauritzen . Can you look at this?

@theodorklauritzen theodorklauritzen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Written by Claude (Opus 5) with minimal oversight from Theodor. This review was produced by the compound-engineering ce-code-review skill: ten reviewer agents over the full diff, merged and deduplicated, then put through an independent validation pass (4 of 15 findings were dropped there — see Coverage). Treat the findings as a starting point, not a verdict from a human reviewer. Line references are against head 445e6999.

Code Review -- PR #548 "Feat/auth image collections [OWTR #1]"

Scope: pr:548 (vevcom/projectNext), base main, head feat/auth-image-collections @ 445e6999, merge-base 854dbe05. 402 files, +8299 / -5533, 82 commits. Scope mode: PR-remote (local tree is on an unrelated branch; all inspection ran against the fetched PR head). No untracked files excluded.

Intent (from the PR body, explicit): split the flat images service into a sub-service plus per-domain implementations, put every dynamic collection behind a two-level visibility model (see vs administrate), and move standard images from seeded rows to config-declared files -- without weakening authorization, losing stored files, or breaking existing image consumers across the CmsImage -> Image move.

Reviewer team: correctness, project-standards (repo-root CLAUDE.md governs every changed path), security (new authorizer + per-level checks), adversarial (auth + persistence writes + file deletion), testing (two new suites, large behavior change), maintainability (13k executable changed lines, new abstractions), data-migration (Prisma schema retargets Flair.image/Committee.logoImage, SpecialCmsImage shrinks), reliability (store deletion, seed races), performance (paging + visibility filters), previous-comments (PR already has a review and two comments).

Triage Groups

Group Findings Context Preferred Resolution Why
Nested calls re-run authorizers (apply queue) #4, #7 An operation calls another operation internally; bypassAuth defaults to false, so the inner authorizer is enforced against the end user. Fix #7 first (it blocks a normal member from changing their own profile picture), then #4's four readCurrent({}) sites. Both are the same one-line pattern: pass bypassAuth: true / use an internal reader. Same root cause, same fix shape; the PR body already documents this exact bug class as "fixed" for visibilityOperations.update.
Empty visibility level authorizes everyone (decision gate, then apply) #5, #8 checkVisibility returns true for an empty requirement list. Two independent ways to reach that state: a fresh/seeded/migrated collection (#8) and a failed visibility save (#5). Decide #8 first: make an empty ADMIN matrix unsatisfiable in RequireLevelFromDoubleLevelVisibility. That closes #8 for existing collections too and caps #5's blast radius. Then wrap the update in a transaction (#5). One design choice resolves both; fixing only #5 leaves seeded and migrated collections open.
Image files leak from the store (apply queue) #10, #11 Deleting a collection never unlinks files; deleting one image unlinks inside a DB transaction and aborts on the first missing file. One refactor: split destroyImage into a DB part returning the fsLocations and a post-commit cleanup step that ignores ENOENT, then reuse it in destroyCollection. Do #10 first; #11 consumes it. Shared fix path; committeeOperations.destroy already has the correct pattern to mirror.
Uncached repeat reads per request (apply queue) #9, #14 The same rows are re-fetched several times per request in the standard-image path and the visibility-matrix path. Wrap both read paths in React's per-request cache(). #9 first -- it is on every page load. Identical mechanism and fix; no request-scoped cache exists anywhere under src/services today.

P0 -- Critical

#4 -- omegaOrderOperations.readCurrent({}) called without bypassAuth at four sites -- src/services/users/operations.ts:38 (also groups/committees/operations.ts:261, groups/interestGroups/operations.ts:16, omegaOrder/operations.ts:20)

  • Why it matters: Creating a user (invitation flow), creating a committee, creating an interest group, and incrementing the omega order now throw UNAUTHORIZED for any session that holds the operation's own permission but not OMEGA_ORDER_READ -- an admin permission, not a default or membership one. All four previously called the unauthenticated helper readCurrentOmegaOrder(), deleted in this PR.
  • Fix (mechanical): add bypassAuth: true to each of the four calls, matching omegaOrder/operations.ts:64 in this same diff.
  • Confidence: 100, validator-confirmed (bypassAuth resolves to false for nested calls at serviceOperation.ts:250). Single reviewer (project-standards).
  • Note the irony: the PR body describes fixing exactly this bug for visibilityOperations.update, while introducing it at four more sites.

#5 -- Visibility update deletes all requirements outside a transaction, so a failed save opens the collection to everyone -- src/services/visibility/operations.ts:23

  • Why it matters: visibilityOperations.update runs visibilityRequirement.deleteMany and then a separate visibility.update re-create, with no transaction and no opensTransaction. Any failure in between -- a non-existent groupId/order (both real FKs), or two conditions colliding on @@unique([visibilityRequirementId, groupId, order]) -- leaves zero requirements, and checkVisibility treats an empty list as "everyone". It is reachable directly through updateDynamicImageCollectionRegularLevel/AdminLevelVisibilityAction.
  • Fix (mechanical): set opensTransaction: true and wrap both statements in one prisma.$transaction, mirroring imageOperations.destroyCollection. Optionally de-duplicate conditions by (groupId, order) before the write so the normal UI path never trips the unique index.
  • Confidence: 75, validator-confirmed. Corroborated by two independent reviewers (security, adversarial).

#7 -- Special-collection panel authorizer re-runs on internal calls, blocking self-service uploads -- src/services/images/subservice/special/implement.ts:84

  • Why it matters: A normal member cannot change their own profile picture. The settings page renders the uploader (userAuth.updateProfileImage passes on username match), but the server call fails: uploadImage internally calls readCollection({}), whose authorizer is profileImagesImagePanelAuth = RequirePermission('USERS_UPDATE'). internalCall neuters only the sub-operation's own authorizer, not the nested one. The same mismatch hits committeeOperations.updateLogo, committeeOperations.create, and ombulOperations.create.
  • Fix (mechanical): give implementSpecialCollection an internal reader (or call readCollection({ bypassAuth: true })) for uploadImage, destroyImage, and readPageOfImagesInCollection; keep the panel authorizer only on the externally exposed specialCollectionPanelOperations.readCollection.
  • Confidence: 75, validator-confirmed. Single reviewer (correctness).

P1 -- High

#8 -- New collections start with an empty admin level, so anyone can administrate them -- src/services/images/dynamic/operations.ts:114 (decision gate -- do not auto-apply)

  • Why it matters: createCollection mints two Visibility rows with zero requirements; checkVisibility is vacuously true for an empty list; RequireLevelFromDoubleLevelVisibility is declared USER_NOT_REQUIERED_FOR_AUTHORIZED. So destroyDynamicImageCollectionAction succeeds with no session at all. This is not only a transient window on new collections: seedImages creates the permanent "seeded cms images" collection the same way, and migrateImageCollections gives every OmegaWeb collection visibilityAdmin: { create: {} } under its own //TODO: not everyone should be able to update this.....
  • The decision (two options, different scope):
    • (a) Seed visibilityAdmin from the creating session inside createCollection's transaction. Fixes new collections only.
    • (b) Make RequireLevelFromDoubleLevelVisibility treat an empty matrix as unsatisfiable when level === 'ADMIN'. Also closes seeded and migrated collections; IMAGE_ADMIN stays the recovery path. This also removes the ordering constraint the PR body documents ("the admin level must be narrowed before the regular level").
    • (b) is the broader fix; (a) alone leaves seeded and migrated collections open.
  • Confidence: 100, validator-confirmed. Corroborated by three independent reviewers (correctness, security, adversarial) -- the strongest agreement in this review.

#9 -- Standard-image lookups fan out into ~30 uncached DB round trips on every page -- src/services/images/standard/operations.ts:106

  • Why it matters: src/app/layout.tsx:47 awaits readAllStandardImagesAction() on every request. That loops all 14 StandardImage members; each does a findUnique plus a standardImagesImagePanelOperations.readCollection({}) that re-resolves the same STANDARDIMAGES collection every iteration. NavBar and Footer then render their own StandardImageServer calls on top. No request-scoped cache exists anywhere under src/services.
  • Fix (mechanical): wrap the collection lookup (and ideally readStandardImage) in React's cache().
  • Confidence: 100, validator-confirmed. Single reviewer (performance).

#10 -- destroyImage unlinks files inside the DB transaction and aborts on the first missing file -- src/services/images/subservice/operations.ts:181

  • Why it matters: destroyImage deletes the row, then unlinks four files. Callers (updateProfileImage, committee updateLogo) invoke it with prisma: tx inside prisma.$transaction(..., { timeout: 20000 }). Filesystem writes do not roll back, so a later failure restores the Image row with its files already gone. Separately, implementStore.destroyFile throws NOT FOUND on ENOENT, so one already-missing file aborts the remaining three unlinks.
  • Fix (mechanical): split into a DB-only part returning the four fsLocations plus a post-commit cleanup step; make destroyFile swallow ENOENT; use Promise.allSettled for the four unlinks.
  • Confidence: 100, validator-confirmed. Corroborated by two independent reviewers (correctness, adversarial).

#11 -- Destroying an image collection deletes the rows but leaves every file in the store -- src/services/images/subservice/operations.ts:33

  • Why it matters: The confirmation dialog says "Dette vil ogsa slette alle bilder i salingen", but destroyCollection deletes only the ImageCollection row and relies on onDelete: Cascade for the Image rows. It never calls imageStore.destroyFile, and no caller compensates. Every deletion orphans four files per image with no DB row left to find them by. Store-volume usage grows unbounded. committeeOperations.destroy (committees/operations.ts:214-218) already shows the correct pattern.
  • Fix (mechanical): read the collection's fsLocations before the transaction, unlink them after it commits (ignoring ENOENT), and add a test asserting the store has no residue.
  • Confidence: 100, validator-confirmed. Corroborated by four independent reviewers (correctness, testing, reliability, adversarial).

#12 -- Collection cover image accepts any imageId without an ownership check -- src/services/images/subservice/operations.ts:57

  • Why it matters: updateCollection connects coverImage to a caller-supplied coverImageId with no check that the image belongs to a collection the session administrates. Reading the collection back returns the full Image row (coverImage: true in the includer), whose fsLocation* values map onto the unauthenticated /store/images/<uuid> URLs. Iterating image IDs turns this into readout of restricted collections. The bar is low: IMAGE_COLLECTION_CREATE is a committee permission and a self-created collection makes the caller its administrator. This PR already added the equivalent guard on the sibling path (cmsImageOperations.update refuses an imageId unless sessionAdministratesCollectionOfImage).
  • Fix (mechanical): reuse sessionAdministratesCollectionOfImage (extract it from cms/images/operations.ts:23), or require the image to belong to the collection being updated and throw Smorekopp('UNAUTHORIZED', ...) otherwise.
  • Confidence: 75, validator-confirmed. Single reviewer (security).

#13 -- Image encoding and file writes run inside the DB transaction, papered over with a 20s timeout -- src/services/users/operations.ts:580

  • Why it matters: uploadImage resizes to three sizes, avif-encodes each, and writes four files -- all inside prisma.$transaction. The author's own comment says this is "comfortably slower than the default 5000ms interactive transaction timeout under load", and the response was to raise every affected transaction to 20s. A pool connection and any held locks stay checked out across CPU-bound encoding and filesystem I/O. Same pattern in flairs.updateImage, committees.create/updateLogo, and ombul.updateCoverImage.
  • Fix (design-shaped but concrete): two phases -- do the encode/store work outside the transaction, then open a short transaction that only writes the already-produced fsLocation/ext values.
  • Confidence: 75, validator-confirmed. Single reviewer (reliability).

P2 -- Moderate

#14 -- Double-level visibility matrix is re-fetched 2-3x per authorized request -- src/services/visibility/implement.ts:113

  • Why it matters: readDoubleLevelMatrix reads the matrix in the authorizer, then the operation body discards it and reads it again. updateRegularLevel/updateAdminLevel read it three times (authorizer, ownershipCheck, beforeRun). Every collection detail page pays for this.
  • Fix (mechanical): memoize readDoubleLevelMatrixInternal with cache(), or add a fourth AuthorizerFactory type parameter (as RequireVisibilityFilter already has) so the hooks receive the matrix the authorizer already computed.
  • Confidence: 100, validator-confirmed. Single reviewer (performance).

#15 -- Creating a collection navigates to a route that does not exist -- src/app/image-collections/MakeNewCollection.tsx:21

  • Why it matters: Post-create navigation goes to /image-collections/${collection.id}, but the only routes under src/app/image-collections are dynamic/[name] and special/[specialName]. Creating a collection 404s.
  • Fix (mechanical): navigate by name, matching CollectionCardLink.collectionHref: /image-collections/dynamic/${encodeURIComponent(collection.name)}.
  • Confidence: 75, validator-confirmed. Single reviewer (adversarial).

Pre-existing (does not count toward the verdict)

  • src/prisma/seeder/src/dobbelOmega/migrateImages.ts:118 -- the OmegaWeb image migration is still hard-capped at 10 images by a literal .slice(0, 10), after the limits filter has already trimmed the set. P2. If a debug cap is wanted, drive it from limits.

Coverage

  • 10 reviewers dispatched, 10 returned. No failures.
  • Cross-model adversarial pass: not run -- the reviewed head is not the working tree (pr-remote), where reviewers must inspect fetched refs. The adversarial lens ran via the in-process adversarial-reviewer fallback, as the routing rule requires. Its agreement with the session-model reviewers is therefore same-family corroboration, not cross-model.
  • Validation: one batch, all 15 merged findings validated (no shortcut skips -- no cross-model corroboration existed to license one). 11 confirmed, 4 dropped as validated:false, all from data-migration:
    • Flair/Committee/Ombul FK retarget CmsImage -> Image with no id remap (was P0)
    • visibilityReadId -> visibilityRegularId rename without @map (was P0)
    • Ombul.paragraphId newly required with no backfill (was P0)
    • SpecialCmsImage dropping enum members still referenced by live rows (was P0)
    • Common reason: the harm requires an incremental-DDL path that does not exist in this repo. prisma.config.ts sets migrations.path: '' with a TODO: Add migrations before production; production's Dockerfile/compose run only build+start with no db push or migrate; the sole schema-application script is seed (prisma db push --force-reset). The validator also confirmed migrateOmbul.ts now creates the paragraph via paragraph: { create: {} }, and that no code at head still references the dropped enum members. See the first residual risk below -- the underlying concern is real, it just is not this PR's defect.
  • Suppressed by the confidence gate: 3 findings at anchor 50 (concurrent level-update races, concurrent standard-image regeneration, special-collection regeneration leaking Visibility rows) -- all preserved as residual risks below.
  • Demoted to soft buckets: 5 single-reviewer P2 advisories (2 testing-coverage, 1 maintainability, 2 reliability/adversarial).
  • Quote-the-line gate: 0 findings demoted for a missing first_evidence.
  • Untracked files: none excluded. Plan discovery: no plan found under docs/plans/, so settlement suppression was not evaluated.
  • No learnings corpus (docs/solutions/ is empty), so learnings-researcher did not run. agent-native and deployment-verification were not selected.

Residual risks

  1. No forward-migration mechanism at all. prisma.config.ts has migrations.path: '' with TODO: Add migrations before production; the only schema-apply command is prisma db push --force-reset (full wipe + reseed). That is what invalidated the four schema findings above -- but it also means this PR's destructive DDL (an FK retarget, a required-column addition, an enum narrowing, a column rename) has no defined path to a populated production database. Pre-existing, and the highest-blast-radius item in this PR series. Worth an explicit deploy decision before OWTR #1 ships.
  2. Image bytes are not behind the new visibility model. nginx serves /store/images/<uuid>.<ext> with no authorization, so visibility hides listings and metadata only. Anyone who obtains an fsLocation keeps access to a restricted image forever. Pre-existing, but this PR is what makes visibility look like an access-control boundary for image content.
  3. standardStoreRoot is derived from import.meta.url (src/lib/standardStore/files.ts:10). Proven for the unbundled seeder scripts; unconfirmed once images/standard/operations.ts pulls it into the Next.js production bundle, where the module sits at a chunk-dependent depth under .next/server/. If the three .. segments miss, every runtime standard-image self-heal throws ENOENT on a public path. Needs a prod build to settle.
  4. readStandardImage is RequireNothing but performs writes (delete + re-upload) when a standard image is missing or has escaped the standard collection. In a degraded state that is a repeatable unauthenticated write/disk-consumption path, and the delete bypasses destroyImage, orphaning the old files. Concurrent regeneration can also collide on the standardImage unique index.
  5. uploadImage writes four files before prisma.image.create; a failed create strands them (the mirror image of #11, also reachable via uploadManyImages).
  6. Concurrent updateRegularLevel + updateAdminLevel each validate the sub-visibility invariant against a stale peer level, so two concurrent saves can leave the pair violating it.
  7. implementStore joins an unsanitized dynamicStorePrefix into the path, and destroyFile never normalizes fsLocation. No current caller passes user input, so this is latent, not exploitable today (call-site coverage is grep-only).
  8. A failed visibility read collapses to the same null as "not permitted" in image-collections/dynamic/[name]/page.tsx; an admin cannot tell a transient error from a denial, and nothing is logged.
  9. dynamicImageOperations.ownershipCheck calls the full readCollection rather than an internal call, adding an undocumented REGULAR-visibility requirement to every dynamic image mutation and surfacing NOT FOUND instead of DISSALLOWED for special collections.
  10. Special collections are created with two blank visibilities; they currently fall closed only because readCollection's findFirstOrThrow throws inside ownershipCheck -- an exception, not an authorization decision.
  11. expandImageCollection falls back to images[0] with take: 1 and no orderBy, so a collection's implicit cover can change between reads.
  12. destroyImage on an image referenced by Ombul.coverImage cascades the Ombul row away (onDelete: Cascade), while Flair.image and Committee.logoImage are Restrict. No reachable exploit constructed, but the asymmetry deserves a deliberate call.
  13. The double-level matrix (group ids and omega orders for both levels) is serialized to the client for every session that can read a collection, exposing the composition of restricted groups to regular viewers.
  14. uploadAsStandardImage is baked into the generic uploadImage sub-operation, forcing uploadAsStandardImage: null at three unrelated call sites (maintainability, advisory).
  15. readDefaultCollectionCover has no fallback if readStandardImage fails, unlike StandardImageServer which degrades gracefully.
  16. Seed timeouts were raised 30s -> 60s with maxWorkers: CI ? 2 : undefined, justified by sharp/avif CPU contention. Reasonable, but nothing regression-checks seed duration, so future slowdowns eat the new budget silently.
  17. implementDoubleLevelVisibilityOperations has exactly one production consumer today; its generality is unproven until a second domain adopts it.
  18. The only formal review on the PR is a Copilot bot review stating it could not review because the PR exceeds 300 files -- so no automated review coverage exists on this PR today. The one substantive human comment ("I suggest someone to user-test this that the visibility authoring makes sense") is unresolved and is a UX-validation ask, not a code change.
  19. Verified clean (not a risk): the old VisiblityAdmin typo directory, ImageList, src/app/images/, the duplicated standard_store, and ombul/ConfigVars.ts are all fully deleted at the PR head -- clean renames, no leftovers.

Testing gaps

  • No test asserts the store is empty after destroyCollection -- the gap that hid #11.
  • No test drives visibilityOperations.update through a failing re-create to assert the matrix is unchanged rather than emptied (#5).
  • No test asserts what an empty admin level authorizes; the suite arranges that state with Session.empty() but never pins the resulting exposure (#8).
  • No test covers a non-privileged user changing their own profile image end-to-end (#7).
  • No test covers updateCollection with a coverImageId from a collection the session cannot read (#12).
  • No test covers the updateRegularLevel/updateAdminLevel ownership check with another owner's visibilityId (implement.ts:128-132, :150-154 are unexercised).
  • No test covers readStandardImage's two regeneration branches, or implementSpecialCollection's auto-create-from-config path and its cross-collection destroyImage check.
  • No test exercises rollback of an image-update transaction to check whether replacement files are cleaned up.
  • No test asserts DB query counts for layout.tsx / NavBar / Footer, so #9 and #14 would silently regress after a fix.
  • No test covers the dobbelOmega CmsImage -> Image migration paths, or applies the schema diff to an already-populated database (PrismaTestEnvironment always starts from empty).
  • The beforeRun framework hook is exercised only indirectly through its one caller; no direct test of its ordering/abort behavior.

Verdict: Not ready

Three P0s block merge, and all three are small, mechanical fixes: #7 (a normal member cannot change their own profile picture), #4 (user/committee/interest-group creation throws UNAUTHORIZED without OMEGA_ORDER_READ), and #5 (a failed visibility save empties the matrix, which means "everyone"). Fix those first -- they are one-line-per-site changes.

Then make the one design call: #8, whether an empty ADMIN visibility level should authorize everyone. Choosing option (b) -- treat an empty matrix as unsatisfiable for ADMIN -- also closes seeded and migrated collections and removes the level-ordering constraint the PR body documents as a known wart.

Everything else is mechanical: the store file leaks (#10, #11), the cover-image ownership check (#12), the per-request cache (#9, #14), the transaction split (#13), and the broken post-create redirect (#15).

Separately, and outside this PR's diff: the repo has no forward-migration mechanism, and this PR is the first to ship genuinely destructive DDL. That is what cleared the four schema findings, but it needs a deploy decision before OWTR #1 ships.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants