From a55655a61affe32e02daed98875078880f5de80c Mon Sep 17 00:00:00 2001 From: giancarlo Date: Sat, 20 Apr 2024 20:33:19 +0800 Subject: [PATCH] Improve owner transfer process and member sorting Extended the account ownership transfer tests and implemented several updates. This includes transferring the ownership only to an existing account member, sorting team members based on role hierarchy and whether a member is the primary owner. In the permissions check, prevented non-members from creating invitations and enhanced the styling of role badges depending on if they are custom or not. --- README.md | 17 +++++ .../migrations/20221215192558_schema.sql | 32 ++++++-- .../tests/database/invitations.test.sql | 17 +++++ .../database/transfer-ownership.test.sql | 73 ++++++++++++++++++- .../members/account-members-table.tsx | 28 +++++-- .../src/components/members/role-badge.tsx | 15 ++-- .../actions/team-members-server-actions.ts | 19 +++-- .../services/account-members.service.ts | 3 +- 8 files changed, 172 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index 4b7338186..45bd5ec25 100644 --- a/README.md +++ b/README.md @@ -525,6 +525,23 @@ UPDATE auth.users SET raw_app_meta_data = raw_app_meta_data || '{"role": "super- Please replace `` with the user ID you want to assign as a super admin. +## How to approach Local Development + +Supabase's hosted Studio is pretty great - but I don't think it should be used to perform schema changes. Instead, I recommend using your local Supabase Studio to make the changes and then generate the migration file. Then, you can push the migration to the remote Supabase instance. + +At this point, you have two options: + +1. create a migration with `pnpm --filter web supabase migration new ` and update the code manually +2. or, use the local Supabase Studio to make the changes and then run `pnpm --filter web supabase db diff -f ` which will generate the migration file for you. DOUBLY CHECK THE FILE! + +Once you've tested it all and are happy with your local changes, push the migration to the remote Supabase instance with `pnpm --filter web supabase db push`. + +Doing the opposite is also okay - but: +1. You're making changes against the production database - which is risky +2. You're not testing the changes locally - which is risky +3. You need to pull the changes from the remote Supabase instance to your local instance so they are in sync + +My two cents - but you do you - anything that works for you is good. ## Going to Production diff --git a/apps/web/supabase/migrations/20221215192558_schema.sql b/apps/web/supabase/migrations/20221215192558_schema.sql index 9258004d0..1490f27aa 100644 --- a/apps/web/supabase/migrations/20221215192558_schema.sql +++ b/apps/web/supabase/migrations/20221215192558_schema.sql @@ -344,8 +344,20 @@ create or replace function begin if current_user not in('service_role') then raise exception 'You do not have permission to transfer account ownership'; - end if; + + -- verify the user is already a member of the account + if not exists( + select + 1 + from + public.accounts_memberships + where + target_account_id = account_id + and user_id = new_owner_id) then + raise exception 'The new owner must be a member of the account'; + end if; + -- update the primary owner of the account update public.accounts @@ -354,12 +366,13 @@ begin where id = target_account_id and is_personal_account = false; + -- update membership assigning it the hierarchy role update public.accounts_memberships set account_role =( - kit.get_upper_system_role()) + public.get_upper_system_role()) where target_account_id = account_id and user_id = new_owner_id; @@ -416,7 +429,7 @@ create trigger protect_account_fields before update on public.accounts for each row execute function kit.protect_account_fields(); -create or replace function kit.get_upper_system_role() +create or replace function public.get_upper_system_role() returns varchar as $$ declare @@ -431,6 +444,9 @@ end; $$ language plpgsql; +grant execute on function public.get_upper_system_role() to + service_role; + create or replace function kit.add_current_user_to_new_account() returns trigger language plpgsql @@ -446,7 +462,7 @@ begin values( new.id, auth.uid(), - kit.get_upper_system_role()); + public.get_upper_system_role()); end if; @@ -955,6 +971,7 @@ begin account_id = target_account_id and target_user_id = user_id); + -- If the user does not have a role in the account, they cannot perform the action if user_role_hierarchy_level is null then return false; end if; @@ -1077,10 +1094,9 @@ create policy invitations_read_self on public.invitations create policy invitations_create_self on public.invitations for insert to authenticated with check ( - public.is_set('enable_team_accounts') and - public.has_permission(auth.uid(), account_id, 'invites.manage'::app_permissions) - and public.has_same_role_hierarchy_level( - auth.uid(), account_id, role)); + public.is_set('enable_team_accounts') + and public.has_permission(auth.uid(), account_id, 'invites.manage'::app_permissions) + and public.has_same_role_hierarchy_level(auth.uid(), account_id, role)); -- UPDATE: Users can update invitations to users of an account they are -- a member of diff --git a/apps/web/supabase/tests/database/invitations.test.sql b/apps/web/supabase/tests/database/invitations.test.sql index ea15000d0..16cb0bdcd 100644 --- a/apps/web/supabase/tests/database/invitations.test.sql +++ b/apps/web/supabase/tests/database/invitations.test.sql @@ -58,6 +58,23 @@ select lives_ok( 'custom role should be able to create invitations' ); +-- Foreigners should not be able to create invitations + +select tests.create_supabase_user('user'); + +select tests.authenticate_as('user'); + +-- it will fail because the user is not a member of the account +select throws_ok( + $$ insert into public.invitations (email, invited_by, account_id, role, invite_token) values ('invite4@makerkit.dev', auth.uid(), makerkit.get_account_id_by_slug('makerkit'), 'member', gen_random_uuid()) $$, + 'new row violates row-level security policy for table "invitations"' +); + +select is_empty($$ + select * from public.invitations where account_id = makerkit.get_account_id_by_slug('makerkit') $$, + 'no invitations should be listed' +); + 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 cfd705036..c3c38194f 100644 --- a/apps/web/supabase/tests/database/transfer-ownership.test.sql +++ b/apps/web/supabase/tests/database/transfer-ownership.test.sql @@ -3,7 +3,78 @@ create extension "basejump-supabase_test_helpers" version '0.0.6'; select no_plan(); --- test +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'); + +-- auth as a primary owner +select tests.authenticate_as('primary_owner'); + +-- only the service role can transfer ownership +select throws_ok( + $$ select public.transfer_team_account_ownership( + makerkit.get_account_id_by_slug('makerkit'), + makerkit.get_user_id('custom@makerkit.dev') + ) $$, + 'permission denied for function transfer_team_account_ownership' +); + +set local role service_role; + +-- the new owner must be a member of the account so this should fail +select throws_ok( + $$ select public.transfer_team_account_ownership( + makerkit.get_account_id_by_slug('makerkit'), + makerkit.get_user_id('test@supabase.com') + ) $$, + 'The new owner must be a member of the account' +); + +-- this should work because the user is a member of the account +select lives_ok( + $$ select public.transfer_team_account_ownership( + makerkit.get_account_id_by_slug('makerkit'), + makerkit.get_user_id('owner@makerkit.dev') + ) $$ +); + +-- 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')), + 'The account owner should be updated' +); + +-- when transferring ownership to an account with a lower role +-- the account will also be updated to the new role +select lives_ok( + $$ select public.transfer_team_account_ownership( + makerkit.get_account_id_by_slug('makerkit'), + makerkit.get_user_id('member@makerkit.dev') + ) $$ +); + +-- check the account owner has been updated +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'); + $$, + 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(); diff --git a/packages/features/team-accounts/src/components/members/account-members-table.tsx b/packages/features/team-accounts/src/components/members/account-members-table.tsx index 9668cd144..3bf6c40e6 100644 --- a/packages/features/team-accounts/src/components/members/account-members-table.tsx +++ b/packages/features/team-accounts/src/components/members/account-members-table.tsx @@ -75,15 +75,27 @@ export function AccountMembersTable({ currentRoleHierarchy: userRoleHierarchy, }); - const filteredMembers = members.filter((member) => { - const searchString = search.toLowerCase(); - const displayName = member.name ?? member.email.split('@')[0]; + const filteredMembers = members + .filter((member) => { + const searchString = search.toLowerCase(); + const displayName = member.name ?? member.email.split('@')[0]; - return ( - displayName.includes(searchString) || - member.role.toLowerCase().includes(searchString) - ); - }); + return ( + displayName.includes(searchString) || + member.role.toLowerCase().includes(searchString) + ); + }) + .sort((prev, next) => { + if (prev.primary_owner_user_id === prev.user_id) { + return -1; + } + + if (prev.role_hierarchy_level < next.role_hierarchy_level) { + return -1; + } + + return 1; + }); return (
diff --git a/packages/features/team-accounts/src/components/members/role-badge.tsx b/packages/features/team-accounts/src/components/members/role-badge.tsx index e52b290f2..3853b4b4b 100644 --- a/packages/features/team-accounts/src/components/members/role-badge.tsx +++ b/packages/features/team-accounts/src/components/members/role-badge.tsx @@ -5,13 +5,15 @@ import { Trans } from '@kit/ui/trans'; type Role = string; +const roles = { + owner: '', + member: + 'bg-blue-50 hover:bg-blue-50 text-blue-500 dark:bg-blue-500/10 dark:hover:bg-blue-500/10', +}; + const roleClassNameBuilder = cva('font-medium capitalize shadow-none', { variants: { - role: { - owner: '', - member: - 'bg-blue-50 hover:bg-blue-50 text-blue-500 dark:bg-blue-500/10 dark:hover:bg-blue-500/10', - }, + role: roles, }, }); @@ -20,9 +22,10 @@ export const RoleBadge: React.FC<{ }> = ({ role }) => { // @ts-expect-error: hard to type this since users can add custom roles const className = roleClassNameBuilder({ role }); + const isCustom = !(role in roles); return ( - + diff --git a/packages/features/team-accounts/src/server/actions/team-members-server-actions.ts b/packages/features/team-accounts/src/server/actions/team-members-server-actions.ts index d215f4263..d65454b7c 100644 --- a/packages/features/team-accounts/src/server/actions/team-members-server-actions.ts +++ b/packages/features/team-accounts/src/server/actions/team-members-server-actions.ts @@ -87,17 +87,20 @@ export async function transferOwnershipAction( ); } + const service = new AccountMembersService(client); + // at this point, the user is authenticated and is the owner of the account // so we proceed with the transfer of ownership with admin privileges - const service = new AccountMembersService( - getSupabaseServerActionClient({ admin: true }), - ); + const adminClient = getSupabaseServerActionClient({ admin: true }); - await service.transferOwnership({ - accountId, - userId, - confirmation: params.confirmation, - }); + await service.transferOwnership( + { + accountId, + userId, + confirmation: params.confirmation, + }, + adminClient, + ); // revalidate all pages that depend on the account revalidatePath('/home/[account]', 'layout'); diff --git a/packages/features/team-accounts/src/server/services/account-members.service.ts b/packages/features/team-accounts/src/server/services/account-members.service.ts index 504741d46..758f63738 100644 --- a/packages/features/team-accounts/src/server/services/account-members.service.ts +++ b/packages/features/team-accounts/src/server/services/account-members.service.ts @@ -125,6 +125,7 @@ export class AccountMembersService { async transferOwnership( params: z.infer, + adminClient: SupabaseClient, ) { const logger = await getLogger(); @@ -135,7 +136,7 @@ export class AccountMembersService { logger.info(ctx, `Transferring ownership of account...`); - const { data, error } = await this.client.rpc( + const { data, error } = await adminClient.rpc( 'transfer_team_account_ownership', { target_account_id: params.accountId,