enhancement(Expense): reduce default permissions for collective admins

server/graphql/common/expenses.ts

@@ -181,24 +181,53 @@ const isCollectiveAdmin = async (req: express.Request, expense: Expense): Promis
 export const isHostAdmin = async (req: express.Request, expense: Expense): Promise<boolean> => {
   if (!req.remoteUser) {
     return false;
+  } else if (expense.HostCollectiveId) {
+    return req.remoteUser.isAdmin(expense.HostCollectiveId);
+  } else if (!expense.collective) {
+    expense.collective = await req.loaders.Collective.byId.load(expense.CollectiveId);
+    if (!expense.collective) {
+      return false;
+    }
   }
 
-  if (!expense.collective) {
-    expense.collective = await req.loaders.Collective.byId.load(expense.CollectiveId);
+  return req.remoteUser.isAdmin(expense.collective.HostCollectiveId) && expense.collective.isActive;
+};
+
+const isAdminOrAccountantOfHostWhoPaidExpense = async (req: express.Request, expense: Expense): Promise<boolean> => {
+  if (!req.remoteUser) {
+    return false;
   }
+  return expense.HostCollectiveId && req.remoteUser.isAdmin(expense.HostCollectiveId);
+};
 
-  if (!expense.collective) {
+const isAdminOfCollectiveWithLooseEditPermissions = async (
+  req: express.Request,
+  expense: Expense,
+): Promise<boolean> => {
+  if (!req.remoteUser || !(await isCollectiveAdmin(req, expense))) {
     return false;
   }
 
-  return req.remoteUser.isAdmin(expense.collective.HostCollectiveId) && expense.collective.isActive;
+  // Collective already loaded by `isCollectiveAdmin`, we need to load the host
+  if (expense.collective && !expense.collective.host) {
+    expense.collective.host = await req.loaders.Collective.byId.load(expense.collective.HostCollectiveId);
+  }
+
+  // Host must have a special `settings.allowCollectiveAdminsToEditPrivateExpenseData` flag
+  return Boolean(expense.collective?.host?.settings?.allowCollectiveAdminsToEditPrivateExpenseData);
 };
 
-const isAdminOrAccountantOfHostWhoPaidExpense = async (req: express.Request, expense: Expense): Promise<boolean> => {
+const isAdminOfCollectiveAndExpenseIsAVirtualCard = async (
+  req: express.Request,
+  expense: Expense,
+): Promise<boolean> => {
   if (!req.remoteUser) {
     return false;
+  } else if (expense.type !== EXPENSE_TYPE.CHARGE) {
+    return false;
+  } else {
+    return isCollectiveAdmin(req, expense);
   }
-  return expense.HostCollectiveId && req.remoteUser.isAdmin(expense.HostCollectiveId);
 };
 
 export type ExpensePermissionEvaluator = (
@@ -262,7 +291,8 @@ export const canSeeExpensePayoutMethod: ExpensePermissionEvaluator = async (req,
     isHostAdmin,
     isHostAccountant,
     isAdminOrAccountantOfHostWhoPaidExpense,
-    isCollectiveAdmin, // Some fiscal hosts rely on the collective admins to do some verifications on the payout method
+    isAdminOfCollectiveWithLooseEditPermissions, // Some fiscal hosts rely on the collective admins to do some verifications on the payout method
+    isAdminOfCollectiveAndExpenseIsAVirtualCard, // Virtual cards are created by the collective admins
   ]);
 };
 
@@ -382,7 +412,12 @@ export const canEditExpense: ExpensePermissionEvaluator = async (
     }
     return false;
   } else {
-    return remoteUserMeetsOneCondition(req, expense, [isOwner, isHostAdmin, isCollectiveAdmin], options);
+    return remoteUserMeetsOneCondition(
+      req,
+      expense,
+      [isOwner, isHostAdmin, isAdminOfCollectiveAndExpenseIsAVirtualCard, isAdminOfCollectiveWithLooseEditPermissions],
+      options,
+    );
   }
 };
 
@@ -643,18 +678,40 @@ export const canEditExpenseAccountingCategory = async (
   expense: Expense,
   options = { throw: false },
 ): Promise<boolean> => {
-  return remoteUserMeetsOneCondition(
-    req,
-    expense,
-    [
-      // Host admins and accountants can always change the accounting category.
-      isHostAdmin,
-      isHostAccountant,
-      // Otherwise, we fallback on the default edit permissions (must be a collective/fromCollective admin)
-      canEditExpense,
-    ],
-    options,
-  );
+  if (!canUseFeature(req.remoteUser, FEATURE.USE_EXPENSES)) {
+    if (options?.throw) {
+      throw new Forbidden(
+        'User cannot edit accounting categories for expenses',
+        EXPENSE_PERMISSION_ERROR_CODES.UNSUPPORTED_USER_FEATURE,
+      );
+    }
+    return false;
+  }
+
+  // Host admins and accountants can always change the accounting category.
+  if (await remoteUserMeetsOneCondition(req, expense, [isHostAdmin, isHostAccountant])) {
+    return true;
+  }
+
+  // Other roles can only change the accounting category if the expense is not paid yet
+  const nonEditableStatuses = ['PAID', 'PROCESSING', 'SCHEDULED_FOR_PAYMENT', 'CANCELED'];
+  if (nonEditableStatuses.includes(expense.status)) {
+    if (options?.throw) {
+      throw new Forbidden(
+        'Can not change accounting category in current status',
+        EXPENSE_PERMISSION_ERROR_CODES.UNSUPPORTED_STATUS,
+      );
+    }
+    return false;
+  }
+
+  // Always allow for collective admins
+  if (await isCollectiveAdmin(req, expense)) {
+    return true;
+  }
+
+  // Otherwise, fallback to the default edit expense permissions
+  return canEditExpense(req, expense, options);
 };
 
 /**

test/server/graphql/common/expenses.test.js

@@ -67,9 +67,10 @@ describe('server/graphql/common/expenses', () => {
     normal: cloneDeep(contextShape),
     selfHosted: cloneDeep(contextShape),
     virtualCard: cloneDeep(contextShape),
+    hostWithSpecialExpensePermissions: cloneDeep(contextShape),
   };
 
-  const prepareContext = async ({ host = undefined, collective = undefined } = {}) => {
+  const prepareContext = async ({ host = undefined, collective = undefined, name } = {}) => {
     const randomUser = await fakeUser();
     const collectiveAdmin = await fakeUser();
     const collectiveAccountant = await fakeUser();
@@ -104,6 +105,7 @@ describe('server/graphql/common/expenses', () => {
     await limitedHostAdmin.update({ data: { features: { ALL: false } } });
 
     return {
+      name,
       expense,
       collective,
       host: collective.host,
@@ -124,17 +126,28 @@ describe('server/graphql/common/expenses', () => {
 
   before(async () => {
     // The most common pattern: a collective + fiscal host
-    contexts.normal = await prepareContext();
+    contexts.normal = await prepareContext({ name: 'normal' });
 
     // Virtual card
-    contexts.virtualCard = await prepareContext();
+    contexts.virtualCard = await prepareContext({ name: 'virtualCard' });
     await contexts.virtualCard.expense.update({ type: 'CHARGE' });
 
     // A self-hosted collective
     const selfHostedCollective = await fakeCollective({ isHostAccount: true, isActive: true, HostCollectiveId: null });
     await selfHostedCollective.update({ HostCollectiveId: selfHostedCollective.id });
     selfHostedCollective.host = selfHostedCollective;
-    contexts.selfHosted = await prepareContext({ host: selfHostedCollective, collective: selfHostedCollective });
+    contexts.selfHosted = await prepareContext({
+      name: 'selfHosted',
+      host: selfHostedCollective,
+      collective: selfHostedCollective,
+    });
+
+    // A host with loose expense permissions
+    contexts.hostWithSpecialExpensePermissions = await prepareContext({ name: 'hostWithSpecialExpensePermissions' });
+    const updatedHostSettings = { allowCollectiveAdminsToEditPrivateExpenseData: true };
+    const updatedHost = await contexts.hostWithSpecialExpensePermissions.host.update({ settings: updatedHostSettings });
+    contexts.hostWithSpecialExpensePermissions.collective.host = updatedHost;
+    contexts.hostWithSpecialExpensePermissions.expense.collective.host = updatedHost;
   });
 
   /** A helper to run the same test on all contexts, to make sure they behave the same way */
@@ -184,8 +197,8 @@ describe('server/graphql/common/expenses', () => {
         expect(await checkAllPermissions(canSeeExpensePayoutMethod, context)).to.deep.equal({
           public: false,
           randomUser: false,
-          collectiveAdmin: true,
-          collectiveAccountant: context.isSelfHosted ? true : false,
+          collectiveAdmin: ['hostWithSpecialExpensePermissions', 'selfHosted', 'virtualCard'].includes(context.name),
+          collectiveAccountant: context.name === 'selfHosted',
           hostAdmin: true,
           hostAccountant: true,
           expenseOwner: true,
@@ -317,7 +330,7 @@ describe('server/graphql/common/expenses', () => {
         expect(await checkAllPermissions(canEditExpense, contexts.normal)).to.deep.equal({
           public: false,
           randomUser: false,
-          collectiveAdmin: true,
+          collectiveAdmin: false,
           collectiveAccountant: false,
           hostAdmin: true,
           hostAccountant: false,

test/server/graphql/v2/mutation/ExpenseMutations-legacy.test.js

@@ -210,7 +210,7 @@ const convertExpenseItemId = item => {
   return item?.id ? { ...item, id: idEncode(item.id, IDENTIFIER_TYPES.EXPENSE_ITEM) } : item;
 };
 
-describe('server/graphql/v2/mutation/ExpenseMutations', () => {
+describe('server/graphql/v2/mutation/ExpenseMutations (legacy)', () => {
   before(async () => {
     // It seems that a previous test doesn't free the sendMessage stub. This corrects it
     await resetTestDB();
@@ -689,10 +689,8 @@ describe('server/graphql/v2/mutation/ExpenseMutations', () => {
       });
 
       it('record a breakdown of the values by role (when editing multiple fields)', async () => {
-        const collectiveAdmin = await fakeUser();
         const hostAdmin = await fakeUser();
         const expense = await fakeExpense();
-        await expense.collective.addUserWithRole(collectiveAdmin, 'ADMIN');
         await expense.collective.host.addUserWithRole(hostAdmin, 'ADMIN');
 
         const initialCategory = await fakeAccountingCategory({
@@ -711,7 +709,6 @@ describe('server/graphql/v2/mutation/ExpenseMutations', () => {
 
         for (const [user, accountingCategory] of [
           [expense.User, null],
-          [collectiveAdmin, category2],
           [hostAdmin, category3],
         ]) {
           const result = await graphqlQueryV2(

test/server/graphql/v2/mutation/ExpenseMutations-legacy.test.js.snap

@@ -1,5 +1,59 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`server/graphql/v2/mutation/ExpenseMutations (legacy) processExpense MARK_AS_UNPAID Taxes with VAT 1`] = `
+"
+| type   | isRefund | To         | amount   | tax     | netAmountInCollectiveCurrency | currency | amountInHostCurrency | hostCurrency | hostCurrencyFxRate |
+| ------ | -------- | ---------- | -------- | ------- | ----------------------------- | -------- | -------------------- | ------------ | ------------------ |
+| DEBIT  | false    | Collective | -€100.00 | -€20.00 | -€120.00                      | EUR      | -$110.00             | USD          | 1.1                |
+| CREDIT | false    | Payee      | €120.00  | -€20.00 | €100.00                       | EUR      | $132.00              | USD          | 1.1                |
+| DEBIT  | true     | Payee      | -€120.00 | €20.00  | -€100.00                      | EUR      | -$132.00             | USD          | 1.1                |
+| CREDIT | true     | Collective | €100.00  | €20.00  | €120.00                       | EUR      | $110.00              | USD          | 1.1                |"
+`;
+
+exports[`server/graphql/v2/mutation/ExpenseMutations (legacy) processExpense PAY Multi-currency expense Pays the expense manually 1`] = `
+"
+| type   | amount | netAmountInCollectiveCurrency | paymentFee | currency | hostCurrency | hostCurrencyFxRate | To    | From  | Host | isRefund |
+| ------ | ------ | ----------------------------- | ---------- | -------- | ------------ | ------------------ | ----- | ----- | ---- | -------- |
+| CREDIT | 1700   | 1600                          | -100       | USD      | USD          | 1                  | Payee | Babel | NULL | false    |
+| DEBIT  | -1600  | -1700                         | -100       | USD      | USD          | 1                  | Babel | Payee | OSC  | false    |"
+`;
+
+exports[`server/graphql/v2/mutation/ExpenseMutations (legacy) processExpense PAY Multi-currency expense Records a manual payment with an active account 1`] = `
+"
+| type   | amount | netAmountInCollectiveCurrency | paymentFee | currency | hostCurrency | hostCurrencyFxRate | To    | From  | Host      | isRefund |
+| ------ | ------ | ----------------------------- | ---------- | -------- | ------------ | ------------------ | ----- | ----- | --------- | -------- |
+| CREDIT | 1700   | 1600                          | -110       | USD      | NZD          | 1.1                | Payee | Babel | PayeeHost | false    |
+| DEBIT  | -1600  | -1700                         | -100       | USD      | USD          | 1                  | Babel | Payee | OSC       | false    |"
+`;
+
+exports[`server/graphql/v2/mutation/ExpenseMutations (legacy) processExpense PAY Taxes with VAT (manual payment) 1`] = `
+"
+| type   | isRefund | To         | amount   | tax     | netAmountInCollectiveCurrency | currency | amountInHostCurrency | hostCurrency | hostCurrencyFxRate |
+| ------ | -------- | ---------- | -------- | ------- | ----------------------------- | -------- | -------------------- | ------------ | ------------------ |
+| DEBIT  | false    | Collective | -€100.00 | -€20.00 | -€120.00                      | EUR      | -$110.00             | USD          | 1.1                |
+| CREDIT | false    | User       | €120.00  | -€20.00 | €100.00                       | EUR      | $132.00              | USD          | 1.1                |"
+`;
+
+exports[`server/graphql/v2/mutation/ExpenseMutations (legacy) processExpense PAY pays 100% of the balance by putting the fees on the payee 1`] = `
+"
+| type   | kind    | isRefund | To       | From     | amount | amountInHostCurrency | paymentFee | netAmountInCollectiveCurrency |
+| ------ | ------- | -------- | -------- | -------- | ------ | -------------------- | ---------- | ----------------------------- |
+| DEBIT  | EXPENSE | false    | Webpack  | Facebook | -9425  | -9425                | -575       | -10000                        |
+| CREDIT | EXPENSE | false    | Facebook | Webpack  | 10000  | 10000                | -575       | 9425                          |
+| DEBIT  | EXPENSE | true     | Facebook | Webpack  | -10000 | -10000               | 575        | -9425                         |
+| CREDIT | EXPENSE | true     | Webpack  | Facebook | 9425   | 9425                 | 575        | 10000                         |"
+`;
+
+exports[`server/graphql/v2/mutation/ExpenseMutations (legacy) processExpense PAY pays 100% of the balance by putting the fees on the payee but do not refund processor fees 1`] = `
+"
+| type   | kind    | isRefund | To       | From     | amount | amountInHostCurrency | paymentFee | netAmountInCollectiveCurrency |
+| ------ | ------- | -------- | -------- | -------- | ------ | -------------------- | ---------- | ----------------------------- |
+| DEBIT  | EXPENSE | false    | Webpack  | Facebook | -9425  | -9425                | -575       | -10000                        |
+| CREDIT | EXPENSE | false    | Facebook | Webpack  | 10000  | 10000                | -575       | 9425                          |
+| DEBIT  | EXPENSE | true     | Facebook | Webpack  | -9425  | -9425                | 0          | -9425                         |
+| CREDIT | EXPENSE | true     | Webpack  | Facebook | 9425   | 9425                 | 0          | 9425                          |"
+`;
+
 exports[`server/graphql/v2/mutation/ExpenseMutations processExpense MARK_AS_UNPAID Taxes with VAT 1`] = `
 "
 | type   | isRefund | To         | amount   | tax     | netAmountInCollectiveCurrency | currency | amountInHostCurrency | hostCurrency | hostCurrencyFxRate |

test/server/graphql/v2/mutation/ExpenseMutations.test.js

@@ -1005,7 +1005,7 @@ describe('server/graphql/v2/mutation/ExpenseMutations', () => {
 
     describe('Accounting category', () => {
       it('fails if the accounting category is invalid', async () => {
-        const expense = await fakeExpense();
+        const expense = await fakeExpense({ status: 'APPROVED' });
         const callMutation = accountingCategory =>
           graphqlQueryV2(
             editExpenseMutation,
@@ -1033,7 +1033,7 @@ describe('server/graphql/v2/mutation/ExpenseMutations', () => {
       });
 
       it('fails if the accounting category has invalid kind', async () => {
-        const expense = await fakeExpense();
+        const expense = await fakeExpense({ status: 'APPROVED' });
         const accountingCategory = await fakeAccountingCategory({
           CollectiveId: expense.collective.HostCollectiveId,
           kind: 'CONTRIBUTION',
@@ -1089,7 +1089,7 @@ describe('server/graphql/v2/mutation/ExpenseMutations', () => {
       });
 
       it('can reset the accounting category by passing null', async () => {
-        const expense = await fakeExpense();
+        const expense = await fakeExpense({ status: 'APPROVED' });
         const accountingCategory = await fakeAccountingCategory({ CollectiveId: expense.collective.HostCollectiveId });
         await expense.update({ AccountingCategoryId: accountingCategory.id });
         const result = await graphqlQueryV2(
@@ -1107,7 +1107,7 @@ describe('server/graphql/v2/mutation/ExpenseMutations', () => {
       it('record a breakdown of the values by role (when editing only the category)', async () => {
         const collectiveAdmin = await fakeUser();
         const hostAdmin = await fakeUser();
-        const expense = await fakeExpense();
+        const expense = await fakeExpense({ status: 'APPROVED' });
         await expense.collective.addUserWithRole(collectiveAdmin, 'ADMIN');
         await expense.collective.host.addUserWithRole(hostAdmin, 'ADMIN');
 
@@ -1161,20 +1161,14 @@ describe('server/graphql/v2/mutation/ExpenseMutations', () => {
       });
 
       it('record a breakdown of the values by role (when editing multiple fields)', async () => {
-        const collectiveAdmin = await fakeUser();
         const hostAdmin = await fakeUser();
-        const expense = await fakeExpense();
-        await expense.collective.addUserWithRole(collectiveAdmin, 'ADMIN');
+        const expense = await fakeExpense({ status: 'APPROVED' });
         await expense.collective.host.addUserWithRole(hostAdmin, 'ADMIN');
 
         const initialCategory = await fakeAccountingCategory({
           CollectiveId: expense.collective.HostCollectiveId,
           kind: 'EXPENSE',
         });
-        const category2 = await fakeAccountingCategory({
-          CollectiveId: expense.collective.HostCollectiveId,
-          kind: 'EXPENSE',
-        });
         const category3 = await fakeAccountingCategory({
           CollectiveId: expense.collective.HostCollectiveId,
           kind: 'EXPENSE',
@@ -1183,7 +1177,6 @@ describe('server/graphql/v2/mutation/ExpenseMutations', () => {
 
         for (const [user, accountingCategory] of [
           [expense.User, null],
-          [collectiveAdmin, category2],
           [hostAdmin, category3],
         ]) {
           const result = await graphqlQueryV2(
@@ -1200,13 +1193,13 @@ describe('server/graphql/v2/mutation/ExpenseMutations', () => {
             user,
           );
 
+          result.errors && console.error(result.errors);
           expect(result.errors).to.not.exist;
         }
 
         await expense.reload();
         expect(expense.data.valuesByRole).to.containSubset({
           submitter: { accountingCategory: null },
-          collectiveAdmin: { accountingCategory: { id: category2.id } },
           hostAdmin: { accountingCategory: { id: category3.id } },
         });
       });
@@ -1264,7 +1257,7 @@ describe('server/graphql/v2/mutation/ExpenseMutations', () => {
     });
 
     it('replaces expense items', async () => {
-      const expense = await fakeExpense({ amount: 3000 });
+      const expense = await fakeExpense({ status: 'APPROVED', amount: 3000 });
       const expenseUpdateData = {
         id: idEncode(expense.id, IDENTIFIER_TYPES.EXPENSE),
         items: [
@@ -1294,7 +1287,7 @@ describe('server/graphql/v2/mutation/ExpenseMutations', () => {
     });
 
     it('updates the items', async () => {
-      const expense = await fakeExpense({ amount: 10000, items: [] });
+      const expense = await fakeExpense({ status: 'APPROVED', amount: 10000, items: [] });
       const initialItems = (
         await Promise.all([
           fakeExpenseItem({ ExpenseId: expense.id, amount: 2000 }),