diff --git a/DB_SCHEMA_MAP.md b/DB_SCHEMA_MAP.md index 1ff54fb..43b14ec 100644 --- a/DB_SCHEMA_MAP.md +++ b/DB_SCHEMA_MAP.md @@ -396,6 +396,7 @@ Default channels created by trigger on space INSERT: `general`, `announcements`, | 028 | Form slug unique PER SPACE (`UNIQUE(space_id, slug)`, drops global `UNIQUE(slug)`); public URL `/f/[space]/[slug]` | | 029 | `space_invites.role` (member_role enum, default `member`); invites can grant a role; admin-granting invites are admin-only (app-enforced) | | 030 | `certifications`, `member_certifications` + `certifications.manage` / `certifications.grant` permissions (the latter = Instructor). Additive default-deny RLS; grants soft-revoked + immutable (no DELETE policy); expiry snapshotted at grant; no anonymous path | +| 031 | `secrets_select` additively also honors `ops.secrets.read` (previously admin/board OR per-secret `ops_acl` only). Reveal/list gates let RLS decide; writes unchanged. Access-neutral unless the permission is granted | --- diff --git a/app/(app)/ops/page.tsx b/app/(app)/ops/page.tsx index 8ed391c..ec28945 100644 --- a/app/(app)/ops/page.tsx +++ b/app/(app)/ops/page.tsx @@ -11,22 +11,28 @@ export default async function OpsPage() { .from('space_members').select('space_id, role, display_name, user_id').eq('user_id', user.id).in('status', ['current', 'unverified', 'late']).single() if (!member) return null - const canSeeSecrets = member.role === 'admin' || member.role === 'board' - const canManageAcl = member.role === 'admin' || member.role === 'board' + const isAdminBoard = member.role === 'admin' || member.role === 'board' + // Writing/managing secrets and the ACL stays admin/board only. + const canManageAcl = isAdminBoard - const [{ data: kbEntries }, { data: areaLeads }, secretsResult, { data: customRoles }, { data: acl }] = await Promise.all([ + // The secrets list is always queried; secrets_select RLS filters it to the + // rows this member may see (admin/board, OR the ops.secrets.read role + // permission, OR a per-secret ops_acl entry). Only metadata is selected + // here; plaintext is fetched on demand via revealSecret(). + const [{ data: kbEntries }, { data: areaLeads }, secretsResult, { data: customRoles }, { data: acl }, { data: canReadSecretsPerm }] = await Promise.all([ supabase.from('knowledge_base').select('*').eq('space_id', member.space_id).order('is_pinned', { ascending: false }).order('created_at', { ascending: false }), supabase.from('area_leads').select('*').eq('space_id', member.space_id).order('area_name'), - canSeeSecrets - // CRITICAL: never select `value` or `encrypted_value` here. The list - // endpoint only returns metadata; the plaintext is fetched on demand - // through the revealSecret() server action. - ? supabase.from('secrets').select('id, title, area, created_at, label, description, icon, space_id, created_by, updated_at, category, notes, encryption_version').eq('space_id', member.space_id) - : Promise.resolve({ data: [] as Tables<'secrets'>[] }), + supabase.from('secrets').select('id, title, area, created_at, label, description, icon, space_id, created_by, updated_at, category, notes, encryption_version').eq('space_id', member.space_id), supabase.from('space_custom_roles').select('slug, name').eq('space_id', member.space_id), supabase.from('ops_acl').select('entity_type, entity_id, role').eq('space_id', member.space_id), + supabase.rpc('user_has_permission', { uid: user.id, sid: member.space_id, perm: 'ops.secrets.read' }), ]) + // Show the Secrets section to admin/board, to a holder of the + // ops.secrets.read permission, or to anyone RLS returned at least one + // (ACL-granted) secret for. + const canSeeSecrets = isAdminBoard || !!canReadSecretsPerm || (secretsResult.data?.length ?? 0) > 0 + // Selectable ACL roles: built-in roles, custom-role slugs, and area-lead // sentinels labelled by area name. const aclRoleOptions = [ diff --git a/docs/DATABASE_SCHEMA.md b/docs/DATABASE_SCHEMA.md index 0a808e0..0b375bb 100644 --- a/docs/DATABASE_SCHEMA.md +++ b/docs/DATABASE_SCHEMA.md @@ -686,7 +686,7 @@ CREATE INDEX idx_activity_space ON activity_log(space_id, created_at); --- -## Migrations 014-030 (additions since the 13-table baseline) +## Migrations 014-031 (additions since the 13-table baseline) `scripts/schema.sql` is the canonical idempotent schema; each numbered migration is mirrored as a section in it. Tables/columns added: @@ -710,6 +710,7 @@ migration is mirrored as a section in it. Tables/columns added: | 028 | Form slug is unique PER SPACE: drops the global `UNIQUE(forms.slug)`, adds `UNIQUE(space_id, slug)`. Public form URL is `/f/[space]/[slug]` | | 029 | `space_invites.role` (member_role enum, default `member`) so an invite can grant a chosen role; legacy invites and the spaces.invite_code path keep granting `member`. Who may create an admin-granting invite is enforced in-app (only an admin) | | 030 | `certifications` (per-space cert types; optional `validity_months`; `is_active` archive) + `member_certifications` (per-member grants; `expires_at` snapshotted at grant time; soft `revoked_at`/`revoked_by`; partial unique on one active grant per member+cert). RLS additive + default-deny: certifications SELECT = any space member, write = `certifications.manage`; member_certifications SELECT = `certifications.manage`/`certifications.grant` (all) or member (own), INSERT/UPDATE = `certifications.grant`, NO DELETE policy (history immutable). New `certifications.manage` + `certifications.grant` permissions (the latter = the Instructor capability) seeded to board + backfilled. No anonymous path | +| 031 | `secrets_select` additively also honors the `ops.secrets.read` role permission (was admin/board OR per-secret `ops_acl` only; the permission was never consulted). Access-neutral unless an admin has granted the permission. Reveal/list server-side gates updated to let RLS be the boundary; write paths unchanged | New enum: `comment_entity_type` = `forum_thread | proposal | incident | policy`. diff --git a/docs/HANDOFF.md b/docs/HANDOFF.md index 522c4db..67a9b66 100644 --- a/docs/HANDOFF.md +++ b/docs/HANDOFF.md @@ -24,7 +24,8 @@ Built phase-by-phase, each gated (vitest + `pnpm build` read in-run) + its own c ### Open / not done - Certifications is LOCAL (6 commits ahead). Awaiting deploy approval. After push: confirm `030` applied, board has both perms, exercise create-type -> grant -> revoke -> renew -> `/me` -> non-admin-instructor path; nothing is browser-verified (no live browser this session). - Pure logic is unit-tested; action *orchestration* (RLS/visibility/grant uniqueness) is NOT (no Supabase mock harness — same systemic gap as forms). -- Queued by user, NOT yet started: (a) verify secrets reveal works via BOTH `ops.secrets.read` role perm AND direct `ops_acl` entry (read-only audit under the RLS guardrail); (b) KB + processes should render markdown with working in-document anchor links. +- Secrets-permission audit (user-requested) FOUND A REAL BUG and FIXED IT (user-approved, LOCAL): `revealSecret` hard-gated on built-in role `['admin','board']` and `secrets_select` never consulted `ops.secrets.read`, so the permission was inert and even an ACL-granted member was pre-blocked by the action; the Ops page also short-circuited the secrets fetch by built-in role. Fix (additive, guarded-surface, approved): migration `031` adds `OR user_has_permission(...,'ops.secrets.read')` to `secrets_select` (admin/board + per-secret `ops_acl` branches unchanged, access-neutral); `revealSecret` now `requireMember` + RLS-bounded read (RLS is the boundary); Ops page always queries secrets (RLS filters) and shows the section for admin/board OR the perm OR any ACL-returned row. Writes unchanged. Mirrored in schema.sql + migration docs. NOT browser-verified. +- Queued by user, NOT yet started: KB + processes should render markdown with working in-document anchor links. - STILL OPEN: production incident-INSERT RLS bug — raised to user this pass; awaiting a prod retry to diagnose (#1 SSR auth.uid() vs #3 data drift). Do not touch the permissions/RLS surface without that diagnosis + approval. --- diff --git a/lib/actions/secrets.ts b/lib/actions/secrets.ts index f5f5090..e3af728 100644 --- a/lib/actions/secrets.ts +++ b/lib/actions/secrets.ts @@ -2,7 +2,7 @@ import { revalidatePath } from 'next/cache' import { createClient } from '@/lib/supabase/server' -import { requireMemberWithRole, parseInput } from '@/lib/auth-helpers' +import { requireMember, requireMemberWithRole, parseInput } from '@/lib/auth-helpers' import { ADMIN_ROLES } from '@/lib/permissions' import { createSecretSchema, updateSecretSchema, uuidSchema } from '@/lib/validations' import { encryptSecret, decryptSecret, encryptionAvailable } from '@/lib/secrets/crypto' @@ -110,7 +110,11 @@ export async function revealSecret(secretId: string) { if (!idCheck.ok) return { error: 'Invalid secret ID' } const supabase = await createClient() - const auth = await requireMemberWithRole(supabase, SECRETS_RW_ROLES, 'Board or admin access required') + // RLS (secrets_select) is the authority: admin/board, OR the + // ops.secrets.read role permission, OR a per-secret ops_acl entry. We no + // longer pre-block on built-in role here, because that ignored both the + // permission and the ACL path. A denied row simply does not come back. + const auth = await requireMember(supabase) if (!auth.ok) return { error: auth.error } const { member } = auth const { data: { user } } = await supabase.auth.getUser() @@ -120,9 +124,10 @@ export async function revealSecret(secretId: string) { .select('id, encryption_version, encrypted_value, value, title') .eq('id', idCheck.data) .eq('space_id', member.space_id) - .single() + .maybeSingle() - if (error || !data) return { error: error?.message ?? 'Not found' } + if (error) return { error: error.message } + if (!data) return { error: 'You do not have access to this secret.' } // A row marked encrypted (version > 0) MUST have ciphertext. If it does // not, the row is corrupt: do not silently fall back to the (empty/stale) diff --git a/scripts/031_secrets_permission_select.sql b/scripts/031_secrets_permission_select.sql new file mode 100644 index 0000000..7d0a543 --- /dev/null +++ b/scripts/031_secrets_permission_select.sql @@ -0,0 +1,41 @@ +-- ============================================================================= +-- 031: secrets SELECT also honors the ops.secrets.read role permission +-- ============================================================================= +-- Before this migration, secrets_select (from 023) allowed a row only to +-- admin/board OR a subject named in a per-secret ops_acl entry. The role +-- permission ops.secrets.read ("Reveal secrets" in the Customize matrix) was +-- NEVER consulted for secrets, so granting it did nothing and the matrix was +-- misleading. This adds the missing additive branch. +-- +-- This is purely ADDITIVE and access-neutral for existing deployments: +-- * The admin/board branch is unchanged. +-- * The per-secret ops_acl branch is unchanged. +-- * A new OR branch grants SELECT to a member whose effective role holds +-- ops.secrets.read in space_role_permissions (admin implicitly holds all, +-- already covered by user_has_permission). With no such grant present the +-- behavior is exactly as before, so no space loses or gains access by +-- deploying this unless an admin has explicitly granted the permission. +-- * Still space-scoped: user_has_permission is per (uid, space_id), so this +-- cannot widen access across tenants. +-- +-- The reveal/list server actions are updated in the same change to let RLS be +-- the boundary instead of a hard built-in-role pre-check (lib/actions/ +-- secrets.ts). Write paths (create/update/delete) are intentionally NOT +-- widened. +-- +-- Idempotent: DROP POLICY IF EXISTS then CREATE. The policy body is the +-- 023/030 text verbatim plus one OR branch. +-- ============================================================================= + +DROP POLICY IF EXISTS secrets_select ON public.secrets; +CREATE POLICY secrets_select ON public.secrets FOR SELECT + USING ( + public.user_has_role_in_space(auth.uid(), space_id, ARRAY['admin','board']) + OR public.user_has_permission(auth.uid(), space_id, 'ops.secrets.read') + OR EXISTS ( + SELECT 1 FROM public.ops_acl a + WHERE a.space_id = secrets.space_id AND a.entity_type = 'secret' + AND a.entity_id = secrets.id + AND a.role IN (SELECT public.user_effective_roles(auth.uid(), secrets.space_id)) + ) + ); diff --git a/scripts/schema.sql b/scripts/schema.sql index c39f834..1ed9e8b 100644 --- a/scripts/schema.sql +++ b/scripts/schema.sql @@ -1782,9 +1782,13 @@ CREATE POLICY ops_acl_delete ON public.ops_acl FOR DELETE USING (public.user_has_role_in_space(auth.uid(), space_id, ARRAY['admin','board'])); DROP POLICY IF EXISTS secrets_select ON public.secrets; +-- secrets_select also honors the ops.secrets.read role permission +-- (equivalent to scripts/031_secrets_permission_select.sql). Additive: the +-- admin/board and per-secret ops_acl branches are unchanged. CREATE POLICY secrets_select ON public.secrets FOR SELECT USING ( public.user_has_role_in_space(auth.uid(), space_id, ARRAY['admin','board']) + OR public.user_has_permission(auth.uid(), space_id, 'ops.secrets.read') OR EXISTS ( SELECT 1 FROM public.ops_acl a WHERE a.space_id = secrets.space_id AND a.entity_type = 'secret'