Skip to content

Per-space viewer controls#1795

Open
richiemcilroy wants to merge 18 commits intomainfrom
spaces-sharing
Open

Per-space viewer controls#1795
richiemcilroy wants to merge 18 commits intomainfrom
spaces-sharing

Conversation

@richiemcilroy
Copy link
Copy Markdown
Member

@richiemcilroy richiemcilroy commented May 10, 2026

This PR adds per-space viewer controls (summary, captions, chapters, reactions, transcript, comments) and optional space-level password protection, then merges those rules with video and org settings so the dashboard, share page, and embed player reflect the effective policy.

Greptile Summary

This PR introduces per-space viewer controls (summary, captions, chapters, reactions, transcript, comments) and optional space-level password protection, then merges those rules with video and org settings so the dashboard, share page, and embed player all reflect the effective policy. The new resolveEffectiveVideoRules utility is well-structured and the migration/schema changes are clean.

  • EffectiveVideoRules: Clean new module that merges video, org, and space settings into a single Required<ViewerSettings> result; space settings take precedence, then video, then org defaults.
  • Password extension: VideosPolicy.buildCanView now collects space-level password hashes alongside the video password; the test suite explicitly asserts that space members must also satisfy the space password check — worth confirming this is the desired UX since members won't be prompted for a password they may not know.
  • Dashboard/embed pages: org settings are now properly fetched and passed to resolveEffectiveVideoRules, closing the organizationSettings: null gap noted in previous reviews.

Confidence Score: 3/5

Merging carries risk: explicit space/org members who haven't manually entered their space's password will be blocked from accessing videos in that space with no graceful recovery path.

The core access-control change in VideosPolicy.buildCanView now passes space password hashes to verifyPasswordCandidates even for confirmed org/space members. A member without the x-cap-password cookie is denied access to every video in a password-protected space, and there is no UI path to prompt them for that password once already authenticated.

packages/web-backend/src/Videos/VideosPolicy.ts — specifically the member branch that calls verifyPasswordCandidates with the full passwordHashes array

Important Files Changed

Filename Overview
packages/web-backend/src/Videos/EffectiveVideoRules.ts New module: clean implementation of viewer-setting merging and password-hash collection; logic is correct and well-structured
packages/web-backend/src/Videos/VideosPolicy.ts Space password hashes are now included in verifyPasswordCandidates for org/space members, which locks out members who haven't manually entered the space password; intentional per tests but a sharp UX regression
apps/web/actions/organization/update-space.ts Auth tightened to Admin-only; password management added; missing empty-name validation that create-space.ts has
apps/web/actions/organization/space-settings.ts New shared constants/helpers for space settings; preserveProSpaceSettings freezes previously-set pro settings even if the user wants to disable them after downgrading
apps/web/actions/videos/password.ts Extends password verification to check space passwords in addition to the video password; logic and cookie-setting are correct
packages/database/schema.ts Adds settings JSON and encrypted password columns to the spaces table; matches the migration SQL

Comments Outside Diff (1)

  1. packages/web-backend/src/Videos/VideosPolicy.ts, line 82-91 (link)

    P1 Space members now blocked by space passwords they may not know

    buildCanView now calls verifyPasswordCandidates(video, passwordHashes) for org/space members where passwordHashes includes every space-level password hash. Because the only way to satisfy this check is via the x-cap-password cookie (set only through the verifyVideoPassword action), a user who is an explicit member of a space but has never manually entered that space's password will receive a VerifyVideoPasswordError and be denied access to any video inside that password-protected space.

    The test "requires a space password for space members" confirms this is the intentional design, but it is a sharp behavioral change: previously, having org/space membership granted access to unpassworded videos unconditionally. Now even members must go through the password-entry flow if their space has a password set, but they may never be shown that UI prompt. If the design intent is that space passwords are exclusively for external viewers, consider only passing the video-level password hash to verifyPasswordCandidates for the member branch, or skipping the call entirely for confirmed members.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: packages/web-backend/src/Videos/VideosPolicy.ts
    Line: 82-91
    
    Comment:
    **Space members now blocked by space passwords they may not know**
    
    `buildCanView` now calls `verifyPasswordCandidates(video, passwordHashes)` for org/space members where `passwordHashes` includes every space-level password hash. Because the only way to satisfy this check is via the `x-cap-password` cookie (set only through the `verifyVideoPassword` action), a user who is an explicit member of a space but has never manually entered that space's password will receive a `VerifyVideoPasswordError` and be denied access to any video inside that password-protected space.
    
    The test `"requires a space password for space members"` confirms this is the intentional design, but it is a sharp behavioral change: previously, having org/space membership granted access to unpassworded videos unconditionally. Now even members must go through the password-entry flow if their space has a password set, but they may never be shown that UI prompt. If the design intent is that space passwords are exclusively for external viewers, consider only passing the video-level password hash to `verifyPasswordCandidates` for the member branch, or skipping the call entirely for confirmed members.
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
packages/web-backend/src/Videos/VideosPolicy.ts:82-91
**Space members now blocked by space passwords they may not know**

`buildCanView` now calls `verifyPasswordCandidates(video, passwordHashes)` for org/space members where `passwordHashes` includes every space-level password hash. Because the only way to satisfy this check is via the `x-cap-password` cookie (set only through the `verifyVideoPassword` action), a user who is an explicit member of a space but has never manually entered that space's password will receive a `VerifyVideoPasswordError` and be denied access to any video inside that password-protected space.

The test `"requires a space password for space members"` confirms this is the intentional design, but it is a sharp behavioral change: previously, having org/space membership granted access to unpassworded videos unconditionally. Now even members must go through the password-entry flow if their space has a password set, but they may never be shown that UI prompt. If the design intent is that space passwords are exclusively for external viewers, consider only passing the video-level password hash to `verifyPasswordCandidates` for the member branch, or skipping the call entirely for confirmed members.

### Issue 2 of 3
apps/web/actions/organization/space-settings.ts:28-36
**`preserveProSpaceSettings` silently locks in pro settings after downgrade**

When a non-pro user edits a space previously configured by a pro user (e.g. `disableSummary: true`), the pro keys are always sourced from `existingSettings ?? false`. A downgraded admin can never re-enable summaries/chapters/transcripts — they're permanently frozen. If the intent is to let anyone remove a pro-gated restriction (just not add new ones), the helper should only preserve `true` values.

```suggestion
export const preserveProSpaceSettings = (
	submittedSettings: Record<SpaceSettingKey, boolean>,
	existingSettings: SpaceSettings | null | undefined,
) => ({
	...submittedSettings,
	...Object.fromEntries(
		proSpaceSettingKeys.map((key) => [
			key,
			existingSettings?.[key] === true ? true : submittedSettings[key],
		]),
	),
});
```

### Issue 3 of 3
apps/web/actions/organization/update-space.ts:66-67
**No validation that `name` is non-empty on update**

`create-space.ts` guards against an empty name, but `update-space.ts` omits that check. A direct API call with an empty `name` would silently clear the space name in the database.

```suggestion
	if (!name?.trim()) {
		return { success: false, error: "Space name is required" };
	}

	const submittedSettings = getSpaceSettingsFromFormData(formData);
	const canUseProFeatures = userIsPro(user);
```

Reviews (2): Last reviewed commit: "fix: restore typecheck setup" | Re-trigger Greptile

@brin-security-scanner brin-security-scanner Bot added contributor:verified Contributor passed trust analysis. pr:verified PR passed security analysis. labels May 10, 2026
@paragon-review
Copy link
Copy Markdown

Paragon Review Skipped

Hi @richiemcilroy! Your Polarity credit balance is insufficient to complete this review.

Please visit https://app.paragon.run to finish your review.

Comment thread apps/web/lib/folder.ts
Comment on lines 262 to +270
const processedVideoData = yield* Effect.all(
videoData.map(
Effect.fn(function* (video) {
const sharedSpaces = sharedSpacesMap[video.id] ?? [];
const rules = resolveEffectiveVideoRules({
videoSettings: video.settings,
organizationSettings: null,
spaces: sharedSpaces.filter((space) => !space.isOrg),
});
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.

P2 Org settings not passed to resolveEffectiveVideoRules

organizationSettings is hardcoded to null here (and identically in spaces/[spaceId]/page.tsx). If an org has disabled, say, disableCaptions at the org level, the dashboard card will compute settings.disableCaptions = false — showing a different state than what viewers actually see on the share/embed pages, which correctly receive org settings. An owner could be misled about effective settings for their video.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/lib/folder.ts
Line: 262-270

Comment:
**Org settings not passed to `resolveEffectiveVideoRules`**

`organizationSettings` is hardcoded to `null` here (and identically in `spaces/[spaceId]/page.tsx`). If an org has disabled, say, `disableCaptions` at the org level, the dashboard card will compute `settings.disableCaptions = false` — showing a different state than what viewers actually see on the share/embed pages, which correctly receive org settings. An owner could be misled about effective settings for their video.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +22 to +40
const settingKeys = [
"disableSummary",
"disableCaptions",
"disableChapters",
"disableReactions",
"disableTranscript",
"disableComments",
] as const;

const getSettingsFromFormData = (formData: FormData) =>
Object.fromEntries(
settingKeys.map((key) => [key, formData.get(key) === "true"]),
);

const proSettingKeys = [
"disableSummary",
"disableChapters",
"disableTranscript",
] as const;
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.

P2 Duplicate setting-key arrays silently diverge from create-space.ts

settingKeys and proSettingKeys are defined verbatim in both create-space.ts and update-space.ts. If a new viewer-control key (e.g. disableDownload) is added to one file but missed in the other, create will honour it while update silently drops it (or vice-versa). Consider extracting these into a shared constants module so both actions stay in sync automatically.

Suggested change
const settingKeys = [
"disableSummary",
"disableCaptions",
"disableChapters",
"disableReactions",
"disableTranscript",
"disableComments",
] as const;
const getSettingsFromFormData = (formData: FormData) =>
Object.fromEntries(
settingKeys.map((key) => [key, formData.get(key) === "true"]),
);
const proSettingKeys = [
"disableSummary",
"disableChapters",
"disableTranscript",
] as const;
// TODO: extract these into a shared constants module (e.g. lib/space-settings.ts)
// that both create-space.ts and update-space.ts import, so adding a new key
// only needs to happen in one place.
const settingKeys = [
"disableSummary",
"disableCaptions",
"disableChapters",
"disableReactions",
"disableTranscript",
"disableComments",
] as const;
const getSettingsFromFormData = (formData: FormData) =>
Object.fromEntries(
settingKeys.map((key) => [key, formData.get(key) === "true"]),
);
const proSettingKeys = [
"disableSummary",
"disableChapters",
"disableTranscript",
] as const;
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/actions/organization/update-space.ts
Line: 22-40

Comment:
**Duplicate setting-key arrays silently diverge from `create-space.ts`**

`settingKeys` and `proSettingKeys` are defined verbatim in both `create-space.ts` and `update-space.ts`. If a new viewer-control key (e.g. `disableDownload`) is added to one file but missed in the other, create will honour it while update silently drops it (or vice-versa). Consider extracting these into a shared constants module so both actions stay in sync automatically.

```suggestion
// TODO: extract these into a shared constants module (e.g. lib/space-settings.ts)
// that both create-space.ts and update-space.ts import, so adding a new key
// only needs to happen in one place.
const settingKeys = [
	"disableSummary",
	"disableCaptions",
	"disableChapters",
	"disableReactions",
	"disableTranscript",
	"disableComments",
] as const;

const getSettingsFromFormData = (formData: FormData) =>
	Object.fromEntries(
		settingKeys.map((key) => [key, formData.get(key) === "true"]),
	);

const proSettingKeys = [
	"disableSummary",
	"disableChapters",
	"disableTranscript",
] as const;
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines 87 to 90

if (!isCreator && !membership) {
if (!isCreator && membership?.role !== "Admin") {
return { success: false, error: "Unauthorized" };
}
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.

P2 Authorization tightened from "any member" to "Admin-role only"

Previously any space member could submit the update form. This PR changes the guard to membership?.role !== "Admin", so regular (non-admin) members can no longer rename a space, change viewer settings, or manage the member list. This is a sensible security improvement, but it's a silent behaviour change that could break existing UX flows where regular members were previously able to update the space. Worth documenting in the PR or confirming it's intentional.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/actions/organization/update-space.ts
Line: 87-90

Comment:
**Authorization tightened from "any member" to "Admin-role only"**

Previously any space member could submit the update form. This PR changes the guard to `membership?.role !== "Admin"`, so regular (non-admin) members can no longer rename a space, change viewer settings, or manage the member list. This is a sensible security improvement, but it's a silent behaviour change that could break existing UX flows where regular members were previously able to update the space. Worth documenting in the PR or confirming it's intentional.

How can I resolve this? If you propose a fix, please make it concise.

@richiemcilroy
Copy link
Copy Markdown
Member Author

hey! please re-review the pr @greptileai

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

Labels

contributor:verified Contributor passed trust analysis. pr:verified PR passed security analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant