diff --git a/apps/web/supabase/migrations/20221215192558_schema.sql b/apps/web/supabase/migrations/20221215192558_schema.sql index 1490f27aa..2af623960 100644 --- a/apps/web/supabase/migrations/20221215192558_schema.sql +++ b/apps/web/supabase/migrations/20221215192558_schema.sql @@ -686,10 +686,11 @@ create policy roles_read on public.roles or public.has_role_on_account(account_id) ); + -- Function to check if a user can perform management actions on an account member create or replace function public.can_action_account_member(target_team_account_id uuid, - user_id uuid) + target_user_id uuid) returns boolean as $$ declare @@ -697,64 +698,83 @@ declare target_user_hierarchy_level int; current_user_hierarchy_level int; is_account_owner boolean; + target_user_role varchar(50); begin - select public.is_account_owner(target_team_account_id) into is_account_owner; - - -- a primary owner of the account can never be actioned - if is_account_owner and user_id = auth.uid() then - raise exception 'You cannot action the primary owner of the account'; + if target_user_id = auth.uid() then + raise exception 'You cannot update your own account membership with this function'; end if; -- an account owner can action any member of the account - if is_account_owner then - return true; + if public.is_account_owner(target_team_account_id) then + return true; end if; + -- check the target user is the primary owner of the account + select + exists ( + select + 1 + from + public.accounts + where + id = target_team_account_id + and primary_owner_user_id = target_user_id) into is_account_owner; + + + if is_account_owner then + raise exception 'The primary account owner cannot be actioned'; + end if; + + -- validate the auth user has the required permission on the account - -- to manage members of the account + -- to manage members of the account select - public.has_permission(auth.uid(), target_team_account_id, - 'members.manage'::app_permissions) into - permission_granted; + public.has_permission(auth.uid(), target_team_account_id, + 'members.manage'::app_permissions) into + permission_granted; + -- if the user does not have the required permission, raise an exception if not permission_granted then - raise exception 'You do not have permission to action a member from this account'; - - end if; - -- users cannot remove themselves from the account with this function - if can_action_account_member.user_id = auth.uid() then - raise exception 'You cannot update your own account membership with this function'; + raise exception 'You do not have permission to action a member from this account'; end if; + -- get the role of the target user select - hierarchy_level into target_user_hierarchy_level + am.account_role, + r.hierarchy_level from - public.roles + public.accounts_memberships as am + join + public.roles as r on am.account_role = r.name where - name = target_user_role; + am.account_id = target_team_account_id + and am.user_id = target_user_id + into target_user_role, target_user_hierarchy_level; + -- get the hierarchy level of the current user select - hierarchy_level into current_user_hierarchy_level + r.hierarchy_level into current_user_hierarchy_level from - public.roles + public.roles as r + join + public.accounts_memberships as am on r.name = am.account_role where - name =( - select - account_role - from - public.accounts_memberships - where - account_id = target_team_account_id - and user_id = auth.uid()); - - -- check if the current user has a higher hierarchy level than the - -- target user. Lower hierarchy levels have higher permissions than higher hierarchy levels - if current_user_hierarchy_level <= target_user_hierarchy_level then - raise exception 'You do not have permission to action this user'; + am.account_id = target_team_account_id + and am.user_id = auth.uid(); + if target_user_role is null then + raise exception 'The target user does not have a role on the account'; + end if; + + if current_user_hierarchy_level is null then + raise exception 'The current user does not have a role on the account'; + end if; + + -- check the current user has a higher role than the target user + if current_user_hierarchy_level >= target_user_hierarchy_level then + raise exception 'You do not have permission to action a member from this account'; end if; - -- return true if the user has the required permission return true; end; @@ -782,14 +802,16 @@ create policy accounts_read_team on public.accounts for select to authenticated using (has_role_on_account(id)); --- DELETE: Users can remove themselves from an account +-- DELETE: Users can remove themselves from an account unless they are the primary owner create policy accounts_memberships_delete_self on public.accounts_memberships - for delete to authenticated + for delete + to authenticated using (user_id = auth.uid()); -- DELETE: Users with the required role can remove members from an account create policy accounts_memberships_delete on public.accounts_memberships - for delete to authenticated + for delete + to authenticated using (public.can_action_account_member(account_id, user_id)); -- SELECT (public.accounts): Team members can read accounts of the team diff --git a/apps/web/supabase/tests/database/00000-makerkit-helpers.sql b/apps/web/supabase/tests/database/00000-makerkit-helpers.sql index 981e1775f..aa331d647 100644 --- a/apps/web/supabase/tests/database/00000-makerkit-helpers.sql +++ b/apps/web/supabase/tests/database/00000-makerkit-helpers.sql @@ -15,6 +15,8 @@ create or replace function makerkit.set_identifier( user_email text ) returns text + security definer + set search_path = auth, pg_temp as $$ begin update auth.users set raw_user_meta_data = jsonb_build_object('test_identifier', identifier) @@ -65,25 +67,6 @@ end; $$ language PLPGSQL; -create or replace function makerkit.get_user_id( - user_email text -) - returns uuid - as $$ -begin - - return - (select - primary_owner_user_id - from - accounts - where - email = user_email); - -end; - -$$ language PLPGSQL; - begin; select plan(1); diff --git a/apps/web/supabase/tests/database/account-permissions.test.sql b/apps/web/supabase/tests/database/account-permissions.test.sql index 5543d644f..605abade6 100644 --- a/apps/web/supabase/tests/database/account-permissions.test.sql +++ b/apps/web/supabase/tests/database/account-permissions.test.sql @@ -75,7 +75,7 @@ select throws_ok( update public.accounts_memberships set account_role = 'custom-role' where account_id = makerkit.get_account_id_by_slug('test') - and user_id = makerkit.get_user_id('test1@test.com'); + and user_id = tests.get_supabase_uid('test1'); set local role postgres; diff --git a/apps/web/supabase/tests/database/delete-membership.test.sql b/apps/web/supabase/tests/database/delete-membership.test.sql new file mode 100644 index 000000000..0f819b143 --- /dev/null +++ b/apps/web/supabase/tests/database/delete-membership.test.sql @@ -0,0 +1,94 @@ +begin; +create extension "basejump-supabase_test_helpers" version '0.0.6'; + +select no_plan(); + +select makerkit.set_identifier('primary_owner', 'test@makerkit.dev'); +select makerkit.set_identifier('owner', 'owner@makerkit.dev'); +select makerkit.set_identifier('member', 'member@makerkit.dev'); +select makerkit.set_identifier('custom', 'custom@makerkit.dev'); + +-- another user not in the team +select tests.create_supabase_user('test', 'test@supabase.com'); + +-- an owner cannot remove the primary owner +select tests.authenticate_as('owner'); + +select throws_ok( + $$ delete from public.accounts_memberships + where account_id = makerkit.get_account_id_by_slug('makerkit') + and user_id = '31a03e74-1639-45b6-bfa7-77447f1a4762' $$, + 'The primary account owner cannot be actioned' +); + +-- an owner can remove accounts with lower roles +select lives_ok( + $$ delete from public.accounts_memberships + where account_id = makerkit.get_account_id_by_slug('makerkit') + and user_id = '6b83d656-e4ab-48e3-a062-c0c54a427368' $$, + 'Owner should be able to remove a member' +); + +-- a member cannot remove a member with a higher role +select tests.authenticate_as('member'); + +-- delete a membership record where the user is a higher role than the current user +select throws_ok( + $$ delete from public.accounts_memberships + where account_id = makerkit.get_account_id_by_slug('makerkit') + and user_id = '5c064f1b-78ee-4e1c-ac3b-e99aa97c99bf' $$, + 'You do not have permission to action a member from this account' +); + +-- an primary_owner cannot remove themselves +select tests.authenticate_as('primary_owner'); + +select throws_ok( + $$ delete from public.accounts_memberships + where account_id = makerkit.get_account_id_by_slug('makerkit') + and user_id = '31a03e74-1639-45b6-bfa7-77447f1a4762' $$, + 'The primary account owner cannot be removed from the account membership list' +); + +-- a primary_owner can remove another member +select lives_ok( + $$ delete from public.accounts_memberships + where account_id = makerkit.get_account_id_by_slug('makerkit') + and user_id = 'b73eb03e-fb7a-424d-84ff-18e2791ce0b4'; $$, + 'Primary owner should be able to remove another member' +); + +-- foreigners + +-- a user not in the account cannot remove a member + +select tests.authenticate_as('test'); + +select throws_ok( + $$ delete from public.accounts_memberships + where account_id = '5deaa894-2094-4da3-b4fd-1fada0809d1c' + and user_id = tests.get_supabase_uid('owner'); $$, + 'You do not have permission to action a member from this account' + ); + +select tests.authenticate_as('owner'); + +select isnt_empty( + $$ select 1 from public.accounts_memberships + where account_id = '5deaa894-2094-4da3-b4fd-1fada0809d1c' + and user_id = tests.get_supabase_uid('owner'); $$, + 'Foreigners should not be able to remove members'); + +select tests.authenticate_as('test'); + +-- a user not in the account cannot remove themselves +select throws_ok( + $$ delete from public.accounts_memberships + where account_id = makerkit.get_account_id_by_slug('makerkit') + and user_id = auth.uid(); $$, + 'You do not have permission to action a member from this account' +); + +select * from finish(); + +rollback; \ No newline at end of file diff --git a/apps/web/supabase/tests/database/remove-membership.test.sql b/apps/web/supabase/tests/database/remove-membership.test.sql deleted file mode 100644 index cfd705036..000000000 --- a/apps/web/supabase/tests/database/remove-membership.test.sql +++ /dev/null @@ -1,10 +0,0 @@ -begin; -create extension "basejump-supabase_test_helpers" version '0.0.6'; - -select no_plan(); - --- test - -select * from finish(); - -rollback; \ No newline at end of file diff --git a/apps/web/supabase/tests/database/transfer-ownership.test.sql b/apps/web/supabase/tests/database/transfer-ownership.test.sql index c3c38194f..8359eda71 100644 --- a/apps/web/supabase/tests/database/transfer-ownership.test.sql +++ b/apps/web/supabase/tests/database/transfer-ownership.test.sql @@ -18,7 +18,7 @@ select tests.authenticate_as('primary_owner'); select throws_ok( $$ select public.transfer_team_account_ownership( makerkit.get_account_id_by_slug('makerkit'), - makerkit.get_user_id('custom@makerkit.dev') + tests.get_supabase_uid('custom') ) $$, 'permission denied for function transfer_team_account_ownership' ); @@ -29,7 +29,7 @@ set local role service_role; select throws_ok( $$ select public.transfer_team_account_ownership( makerkit.get_account_id_by_slug('makerkit'), - makerkit.get_user_id('test@supabase.com') + tests.get_supabase_uid('test') ) $$, 'The new owner must be a member of the account' ); @@ -38,14 +38,14 @@ select throws_ok( select lives_ok( $$ select public.transfer_team_account_ownership( makerkit.get_account_id_by_slug('makerkit'), - makerkit.get_user_id('owner@makerkit.dev') + tests.get_supabase_uid('owner') ) $$ ); -- check the account owner has been updated select row_eq( $$ select primary_owner_user_id from public.accounts where id = makerkit.get_account_id_by_slug('makerkit') $$, - row(makerkit.get_user_id('owner@makerkit.dev')), + row(tests.get_supabase_uid('owner')), 'The account owner should be updated' ); @@ -54,7 +54,7 @@ select row_eq( select lives_ok( $$ select public.transfer_team_account_ownership( makerkit.get_account_id_by_slug('makerkit'), - makerkit.get_user_id('member@makerkit.dev') + tests.get_supabase_uid('member') ) $$ ); @@ -62,20 +62,12 @@ select lives_ok( select row_eq( $$ select account_role from public.accounts_memberships where account_id = makerkit.get_account_id_by_slug('makerkit') - and user_id = makerkit.get_user_id('member@makerkit.dev'); + and user_id = tests.get_supabase_uid('member'); $$, row('owner'::varchar), 'The account owner should be updated' ); --- rollback -select lives_ok( - $$ select public.transfer_team_account_ownership( - makerkit.get_account_id_by_slug('makerkit'), - makerkit.get_user_id('test@makerkit.dev') - ) $$ -); - select * from finish(); rollback; \ No newline at end of file diff --git a/apps/web/supabase/tests/database/update-membership.test.sql b/apps/web/supabase/tests/database/update-membership.test.sql new file mode 100644 index 000000000..ed0991ce7 --- /dev/null +++ b/apps/web/supabase/tests/database/update-membership.test.sql @@ -0,0 +1,27 @@ +begin; +create extension "basejump-supabase_test_helpers" version '0.0.6'; + +select no_plan(); + +select makerkit.set_identifier('primary_owner', 'test@makerkit.dev'); +select makerkit.set_identifier('owner', 'owner@makerkit.dev'); +select makerkit.set_identifier('member', 'member@makerkit.dev'); +select makerkit.set_identifier('custom', 'custom@makerkit.dev'); + +-- another user not in the team +select tests.create_supabase_user('test', 'test@supabase.com'); + +select tests.authenticate_as('member'); + +-- run an update query +update public.accounts_memberships set account_role = 'owner' where user_id = auth.uid() and account_id = makerkit.get_account_id_by_slug('makerkit'); + +select row_eq( + $$ select account_role from public.accounts_memberships where user_id = auth.uid() and account_id = makerkit.get_account_id_by_slug('makerkit'); $$, + row('member'::varchar), + 'Updates fail silently to any field of the accounts_membership table' +); + +select * from finish(); + +rollback; \ No newline at end of file diff --git a/apps/web/supabase/tests/database/update-role.test.sql b/apps/web/supabase/tests/database/update-role.test.sql deleted file mode 100644 index cfd705036..000000000 --- a/apps/web/supabase/tests/database/update-role.test.sql +++ /dev/null @@ -1,10 +0,0 @@ -begin; -create extension "basejump-supabase_test_helpers" version '0.0.6'; - -select no_plan(); - --- test - -select * from finish(); - -rollback; \ No newline at end of file