Prevent sharing a diagram with yourself
RLS is the real guard (diagram_collaborators_insert/update now reject user_id = the diagram's owner, regardless of who's performing the write — covers a Super Admin acting on someone else's diagram too, not just the normal owner path); the client-side check in SupabaseDiagramCollaboratorRepository.add is just there to surface a friendly message instead of the raw 42501. Verified: tsc -b and oxlint clean; supabase db reset + 53/53 pgTAP tests pass (1 new test).
This commit is contained in:
@@ -1,6 +1,6 @@
|
|||||||
import { useEffect, useState } from 'react'
|
import { useEffect, useState } from 'react'
|
||||||
import type { CollaboratorPermission } from '../../data/DiagramCollaboratorRepository'
|
import type { CollaboratorPermission } from '../../data/DiagramCollaboratorRepository'
|
||||||
import { UnknownUsernameError } from '../../data/DiagramCollaboratorRepository'
|
import { SelfCollaboratorError, UnknownUsernameError } from '../../data/DiagramCollaboratorRepository'
|
||||||
import { useDiagramCollaboratorStore } from '../../state/diagramCollaboratorStore'
|
import { useDiagramCollaboratorStore } from '../../state/diagramCollaboratorStore'
|
||||||
import { useProjectStore } from '../../state/projectStore'
|
import { useProjectStore } from '../../state/projectStore'
|
||||||
import Modal from '../common/Modal'
|
import Modal from '../common/Modal'
|
||||||
@@ -36,7 +36,11 @@ export default function DiagramSharingModal({ onClose }: { onClose: () => void }
|
|||||||
await add(diagramId, trimmed, permission)
|
await add(diagramId, trimmed, permission)
|
||||||
setUsername('')
|
setUsername('')
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
setError(err instanceof UnknownUsernameError ? err.message : 'Could not add that person. Try again.')
|
setError(
|
||||||
|
err instanceof UnknownUsernameError || err instanceof SelfCollaboratorError
|
||||||
|
? err.message
|
||||||
|
: 'Could not add that person. Try again.',
|
||||||
|
)
|
||||||
} finally {
|
} finally {
|
||||||
setBusy(false)
|
setBusy(false)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -11,6 +11,11 @@ export interface DiagramCollaborator {
|
|||||||
* not a system failure. */
|
* not a system failure. */
|
||||||
export class UnknownUsernameError extends Error {}
|
export class UnknownUsernameError extends Error {}
|
||||||
|
|
||||||
|
/** Thrown by `add` when the resolved username is the diagram owner's own
|
||||||
|
* account — RLS blocks this at the database level too (the real
|
||||||
|
* enforcement), this is just a friendlier message than the raw 42501. */
|
||||||
|
export class SelfCollaboratorError extends Error {}
|
||||||
|
|
||||||
/** Storage abstraction for a diagram's collaborator list, per
|
/** Storage abstraction for a diagram's collaborator list, per
|
||||||
* organized-ideas.md §8: the owner shares with specific people by
|
* organized-ideas.md §8: the owner shares with specific people by
|
||||||
* username, choosing view or edit access per person. RLS restricts
|
* username, choosing view or edit access per person. RLS restricts
|
||||||
|
|||||||
@@ -3,7 +3,7 @@ import type {
|
|||||||
DiagramCollaborator,
|
DiagramCollaborator,
|
||||||
DiagramCollaboratorRepository,
|
DiagramCollaboratorRepository,
|
||||||
} from './DiagramCollaboratorRepository'
|
} from './DiagramCollaboratorRepository'
|
||||||
import { UnknownUsernameError } from './DiagramCollaboratorRepository'
|
import { SelfCollaboratorError, UnknownUsernameError } from './DiagramCollaboratorRepository'
|
||||||
import { supabase } from './supabaseClient'
|
import { supabase } from './supabaseClient'
|
||||||
|
|
||||||
interface CollaboratorRow {
|
interface CollaboratorRow {
|
||||||
@@ -43,6 +43,12 @@ export class SupabaseDiagramCollaboratorRepository implements DiagramCollaborato
|
|||||||
if (!userId) {
|
if (!userId) {
|
||||||
throw new UnknownUsernameError(`No account found for username "${username}".`)
|
throw new UnknownUsernameError(`No account found for username "${username}".`)
|
||||||
}
|
}
|
||||||
|
const {
|
||||||
|
data: { user: currentUser },
|
||||||
|
} = await supabase.auth.getUser()
|
||||||
|
if (currentUser && userId === currentUser.id) {
|
||||||
|
throw new SelfCollaboratorError("You already have full access to your own diagram — you can't share it with yourself.")
|
||||||
|
}
|
||||||
const { error } = await supabase.from('diagram_collaborators').insert({ diagram_id: diagramId, user_id: userId, permission })
|
const { error } = await supabase.from('diagram_collaborators').insert({ diagram_id: diagramId, user_id: userId, permission })
|
||||||
if (error) {
|
if (error) {
|
||||||
console.error('Failed to add collaborator in Supabase', error)
|
console.error('Failed to add collaborator in Supabase', error)
|
||||||
|
|||||||
@@ -0,0 +1,29 @@
|
|||||||
|
-- Prevents a diagram's owner from ending up as their own collaborator row
|
||||||
|
-- (found via manual testing: the UI let you "share" a diagram with
|
||||||
|
-- yourself). Ownership already implies full access, so a self-collaborator
|
||||||
|
-- row is never meaningful — enforced here, not just in the client, since
|
||||||
|
-- the client-side check alone wouldn't stop a Super Admin's "add on behalf
|
||||||
|
-- of the owner" path or any other direct API access from creating one.
|
||||||
|
|
||||||
|
drop policy "diagram_collaborators_insert" on public.diagram_collaborators;
|
||||||
|
|
||||||
|
create policy "diagram_collaborators_insert" on public.diagram_collaborators for insert
|
||||||
|
with check (
|
||||||
|
(public.is_super_admin() or public.diagram_owner_id(diagram_id) = auth.uid())
|
||||||
|
and user_id != public.diagram_owner_id(diagram_id)
|
||||||
|
);
|
||||||
|
|
||||||
|
-- The existing update policy had no WITH CHECK at all (only USING) — added
|
||||||
|
-- here too, defensively, in case user_id (part of the primary key) is ever
|
||||||
|
-- changed via UPDATE rather than delete+insert.
|
||||||
|
drop policy "diagram_collaborators_update" on public.diagram_collaborators;
|
||||||
|
|
||||||
|
create policy "diagram_collaborators_update" on public.diagram_collaborators for update
|
||||||
|
using (
|
||||||
|
public.is_super_admin()
|
||||||
|
or public.diagram_owner_id(diagram_id) = auth.uid()
|
||||||
|
)
|
||||||
|
with check (
|
||||||
|
(public.is_super_admin() or public.diagram_owner_id(diagram_id) = auth.uid())
|
||||||
|
and user_id != public.diagram_owner_id(diagram_id)
|
||||||
|
);
|
||||||
@@ -25,7 +25,7 @@ begin;
|
|||||||
|
|
||||||
create extension if not exists pgtap with schema extensions;
|
create extension if not exists pgtap with schema extensions;
|
||||||
|
|
||||||
select plan(52);
|
select plan(53);
|
||||||
|
|
||||||
-- ----------------------------------------------------------------------
|
-- ----------------------------------------------------------------------
|
||||||
-- Fixtures (as postgres — RLS does not apply)
|
-- Fixtures (as postgres — RLS does not apply)
|
||||||
@@ -138,6 +138,13 @@ select throws_ok(
|
|||||||
|
|
||||||
select set_config('request.jwt.claim.sub', '11111111-1111-1111-1111-111111111111', true);
|
select set_config('request.jwt.claim.sub', '11111111-1111-1111-1111-111111111111', true);
|
||||||
|
|
||||||
|
select throws_ok(
|
||||||
|
$$ insert into public.diagram_collaborators (diagram_id, user_id, permission)
|
||||||
|
values ('b0000000-0000-0000-0000-000000000001', '11111111-1111-1111-1111-111111111111', 'edit') $$,
|
||||||
|
'42501'::char(5), null,
|
||||||
|
'alice (owner) cannot add herself as a collaborator on her own diagram'
|
||||||
|
);
|
||||||
|
|
||||||
select lives_ok(
|
select lives_ok(
|
||||||
$$ insert into public.diagram_collaborators (diagram_id, user_id, permission)
|
$$ insert into public.diagram_collaborators (diagram_id, user_id, permission)
|
||||||
values ('b0000000-0000-0000-0000-000000000001', '22222222-2222-2222-2222-222222222222', 'view') $$,
|
values ('b0000000-0000-0000-0000-000000000001', '22222222-2222-2222-2222-222222222222', 'view') $$,
|
||||||
|
|||||||
Reference in New Issue
Block a user