From f7fe67f7f7fcde615c48094c1ebf3d33dd5dc9e3 Mon Sep 17 00:00:00 2001 From: giancarlo Date: Sat, 20 Apr 2024 23:22:18 +0800 Subject: [PATCH] Update test cases and improve account actioning This commit refactors Supabase test cases to reflect the updated account actioning mechanism. The "makerkit.get_user_id" function calls were replaced with the new "tests.get_supabase_uid" function, aligning with the testing structure update. It also introduces new policies which further refine user role actions with more precise checks, replacing the old 'delete' policy with the more comprehensive 'can_action_account_member' function. New test cases for updating memberships and deleting memberships have also been added. --- .../migrations/20221215192558_schema.sql | 102 +++++++++++------- .../tests/database/00000-makerkit-helpers.sql | 21 +--- .../database/account-permissions.test.sql | 2 +- .../tests/database/delete-membership.test.sql | 94 ++++++++++++++++ .../tests/database/remove-membership.test.sql | 10 -- .../database/transfer-ownership.test.sql | 20 ++-- .../tests/database/update-membership.test.sql | 27 +++++ .../tests/database/update-role.test.sql | 10 -- 8 files changed, 192 insertions(+), 94 deletions(-) create mode 100644 apps/web/supabase/tests/database/delete-membership.test.sql delete mode 100644 apps/web/supabase/tests/database/remove-membership.test.sql create mode 100644 apps/web/supabase/tests/database/update-membership.test.sql delete mode 100644 apps/web/supabase/tests/database/update-role.test.sql 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