fix(web): require an explicit share to download, matching the view policy

[DPS0340]

What

canUserDownloadVideo no longer treats the video's own organization as a share target. Download permission now requires the same thing view permission requires: ownership, an explicit sharedVideos row for an org the user belongs to, or an explicit spaceVideos row for a space they belong to.

Closes #2037.

The problem

const orgIds = [orgId, ...sharedOrgs.map((org) => org.organizationId)];
//              ^^^^^ video.orgId, unconditionally

orgId is passed from video.orgId at the call site in actions/videos/download.ts. That is the org the video belongs to, not one it was shared with — so membership of the owner's org was enough to download anything that org owns, and the sharedVideos lookup above it was redundant for that org.

The view policy next door is stricter. buildCanView in packages/web-backend/src/Videos/VideosPolicy.ts goes through orgsRepo.membershipForVideo, which joins through sharedVideos and requires the share row to exist:

.where(Dz.and(
  Dz.eq(Db.organizationMembers.userId, userId),
  Dz.eq(Db.sharedVideos.videoId, videoId),   // the share must exist
))

There, video.orgId is used only to look up the allowed email domain, never as a grant.

So the two paths disagreed: a colleague in the owner's org who cannot view an unshared private video could still download it. The space branch of the same function was already written the stricter way — it derives space ids purely from spaceVideos rows and returns false when there are none — which is part of why the org branch reads as an oversight.

The change

The org membership lookup now runs only when sharedVideos returned rows, and only against those org ids. As a side effect it skips a query entirely when the video has no org shares, which is the common case for a personal recording.

orgId becomes unused, so it is dropped from the signature and from the single call site.

Verification

I want to be precise about what I did and did not run.

What I ran. I could not install the full monorepo toolchain, so I ran the committed test file, unmodified, against the real video-download-permissions.ts in an isolated vitest project with the same @/lib alias this repo's vitest.config.ts defines. Six tests, all passing:

• the owner is allowed without any query at all
• a colleague in the owner's org is refused when the video was never shared — the regression, and it also asserts only 2 queries run, not 4
• a member of an org the video was shared with is allowed
• shared to an org the user is not in is refused
• a member of a space the video was shared into is allowed
• a space the user is not in is refused

Revert check. I restored the original implementation (with orgId and the unconditional orgIds) and re-ran the same tests: exactly 2 fail. The other 4 pass under both implementations, so they confirm the change did not simply make the function refuse more things.

Direct proof of the old behaviour. With the original code, an unshared private video plus a colleague who is a member of the owner's org returns:

CURRENT code, unshared private video, colleague in owner org -> download allowed? true

What I did not do. I have not run this against a live Cap instance with a seeded database, so the query-shape reasoning is from reading the drizzle chains rather than from observed SQL. video-download-permissions.ts had no test file before this, so nothing pinned either interpretation.

If video.orgId was meant to be a grant and the view policy is deliberately the stricter of the two, then this is a docs gap rather than a bug, and I would much rather be told that than have it merged. The test file is written so it documents whichever rule you land on.

[DPS0340]

@tembo please review

[DPS0340]

Note on the red check so it isn't read as a build break: Vercel failed with

Authorization required to deploy.

That is the preview-deploy authorization gate for an outside contributor, not a compile or test failure. My other open PRs on this repo show the identical status. Contributor trust and both Socket Security checks pass.

The verification I ran locally on this exact diff is in the PR body, including the revert check confirming the 2 new behavioural tests fail against the current implementation and the other 4 pass under both.

[tembo]

Looks good overall. One thing: this signature change still needs a call-site update in apps/web/app/s/[videoId]/page.tsx (it still passes orgId: video.orgId), otherwise this will fail typecheck/build.

[DPS0340]

@tembo please review

[DPS0340]

Good catch, and it would have broken the build — fixed in b988753.

I checked the call sites with grep -rn canUserDownloadVideo apps/web when I made the change and only acted on actions/videos/download.ts. app/s/[videoId]/page.tsx was in that output and I missed it. Now verified exhaustively against the tracked file list:

$ git ls-files -z '*.ts' '*.tsx' | xargs -0 grep -ln canUserDownloadVideo
apps/web/actions/videos/download.ts
apps/web/app/s/[videoId]/page.tsx
apps/web/lib/video-download-permissions.ts

Both call sites now pass only { userId, ownerId, videoId }.

One thing I confirmed before removing the line: video.orgId is still used elsewhere in that page (the organizations join at line 792), so dropping it from the call does not leave an unused binding.

The behavioural point matters more here than in the server action, actually — page.tsx is what decides whether the download button renders for a viewer, so before this change a colleague in the owner's org saw a download button on a video they could not view. Now the button follows the same rule as access.

@tembo please re-review.

[DPS0340]

Closing this one first, and it is the clearest cut of the four.

#2038 closes #2037 — but I filed #2037 myself. Every other PR I am keeping closes an issue a real user reported (@geo-chen, @rlnt, @pulgueta, @shreyastaware). This one is me reporting a thing and then fixing my own report, which is exactly the kind of item that should not be taking up space in your review queue ahead of theirs.

The underlying observation still stands as far as I can tell: download.ts grants the owner's entire organisation, while viewing the same video requires an explicit share, so download is the looser of the two. But that is a policy call, not a bug — you may well intend org members to be able to download. It needs your decision before it needs my patch, and it is 149 lines across 4 files to review for something that might be working as designed.

I have left #2037 open as the question. If you say "yes, download should match view", I will reopen this against current main.

Trimming my open PRs from nine to five; the other three closures are for the same reason (queue size, no defect found).