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

@@ -525,6 +525,23 @@ UPDATE auth.users SET raw_app_meta_data = raw_app_meta_data || '{"role": "super-
 
 Please replace `<user_id>` 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 <name>` 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 <name>` 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
 

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.is_set('enable_team_accounts')
-        public.has_permission(auth.uid(), account_id, 'invites.manage'::app_permissions)
+        and public.has_permission(auth.uid(), account_id, 'invites.manage'::app_permissions)
-        and public.has_same_role_hierarchy_level(
+        and public.has_same_role_hierarchy_level(auth.uid(), account_id, role));
-        auth.uid(), account_id, role));
 
 -- UPDATE: Users can update invitations to users of an account they are
 --   a member of

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;

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();
 

packages/features/team-accounts/src/components/members/account-members-table.tsx

@@ -75,7 +75,8 @@ export function AccountMembersTable({
     currentRoleHierarchy: userRoleHierarchy,
   });
 
-  const filteredMembers = members.filter((member) => {
+  const filteredMembers = members
+    .filter((member) => {
       const searchString = search.toLowerCase();
       const displayName = member.name ?? member.email.split('@')[0];
 
@@ -83,6 +84,17 @@ export function AccountMembersTable({
         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 (

packages/features/team-accounts/src/components/members/role-badge.tsx

@@ -5,13 +5,15 @@ import { Trans } from '@kit/ui/trans';
 
 type Role = string;
 
-const roleClassNameBuilder = cva('font-medium capitalize shadow-none', {
+const roles = {
-  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',
-    },
+};
+
+const roleClassNameBuilder = cva('font-medium capitalize shadow-none', {
+  variants: {
+    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 (
-    <Badge className={className}>
+    <Badge className={className} variant={isCustom ? 'outline' : 'default'}>
       <span data-test={'member-role-badge'}>
         <Trans i18nKey={`common:roles.${role}.label`} defaults={role} />
       </span>

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(
+  const adminClient = getSupabaseServerActionClient({ admin: true });
-    getSupabaseServerActionClient({ admin: true }),
-  );
 
-  await service.transferOwnership({
+  await service.transferOwnership(
+    {
       accountId,
       userId,
       confirmation: params.confirmation,
-  });
+    },
+    adminClient,
+  );
 
   // revalidate all pages that depend on the account
   revalidatePath('/home/[account]', 'layout');

packages/features/team-accounts/src/server/services/account-members.service.ts

@@ -125,6 +125,7 @@ export class AccountMembersService {
 
   async transferOwnership(
     params: z.infer<typeof TransferOwnershipConfirmationSchema>,
+    adminClient: SupabaseClient<Database>,
   ) {
     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,