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,