fix(secrets): honor ops.secrets.read permission for reveal/list
A regular member granted reveal access either via the ops.secrets.read role permission or a per-secret ops_acl entry could not actually see or reveal secrets: revealSecret hard-gated on built-in role admin/board (ignoring both the permission and the ACL), and secrets_select RLS never consulted the permission, and the Ops page short-circuited the secrets fetch by built-in role. Migration 031 additively adds an OR user_has_permission(..., 'ops.secrets.read') branch to secrets_select; the admin/board and per-secret ops_acl branches are unchanged so deployment is access-neutral unless an admin has granted the permission. revealSecret now resolves the member and lets RLS be the boundary (a denied row simply does not return). The Ops page always queries secrets (RLS filters the set) and shows the section to admin/board, a permission holder, or anyone with an ACL-granted secret. Write paths (create/update/delete) are unchanged. Mirrored in schema.sql; migration docs and HANDOFF updated.
This commit is contained in:
Moheeb Zara
@@ -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 |
|
||||
|
||||
---
|
||||
|
||||
|
||||
+15
-9
@@ -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 = [
|
||||
|
||||
@@ -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`.
|
||||
|
||||
|
||||
+2
-1
@@ -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.
|
||||
|
||||
---
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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))
|
||||
)
|
||||
);
|
||||
@@ -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'
|
||||
|
||||
Reference in New Issue
Block a user