Skip to content

Commit

Permalink
enhancement(Expense): reduce default permissions for collective admins
Browse files Browse the repository at this point in the history
  • Loading branch information
@Betree
Betree committed May 7, 2024
1 parent ce2f317 commit 176f0c0
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 52 deletions.
97 changes: 77 additions & 20 deletions server/graphql/common/expenses.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
Expand Down Expand Up @@ -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
]);
};

Expand Down Expand Up @@ -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,
);
}
};

Expand Down Expand Up @@ -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);
};

/**
Expand Down
27 changes: 20 additions & 7 deletions test/server/graphql/common/expenses.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -104,6 +105,7 @@ describe('server/graphql/common/expenses', () => {
await limitedHostAdmin.update({ data: { features: { ALL: false } } });

return {
name,
expense,
collective,
host: collective.host,
Expand All @@ -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 */
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
8 changes: 0 additions & 8 deletions test/server/graphql/v2/mutation/ExpenseMutations-legacy.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -689,20 +689,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');
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',
Expand All @@ -711,7 +705,6 @@ describe('server/graphql/v2/mutation/ExpenseMutations', () => {

for (const [user, accountingCategory] of [
[expense.User, null],
[collectiveAdmin, category2],
[hostAdmin, category3],
]) {
const result = await graphqlQueryV2(
Expand All @@ -734,7 +727,6 @@ describe('server/graphql/v2/mutation/ExpenseMutations', () => {
await expense.reload();
expect(expense.data.valuesByRole).to.containSubset({
submitter: { accountingCategory: null },
collectiveAdmin: { accountingCategory: { id: category2.id } },
hostAdmin: { accountingCategory: { id: category3.id } },
});
});
Expand Down
23 changes: 8 additions & 15 deletions test/server/graphql/v2/mutation/ExpenseMutations.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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(
Expand All @@ -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');

Expand Down Expand Up @@ -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',
Expand All @@ -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(
Expand All @@ -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 } },
});
});
Expand Down Expand Up @@ -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: [
Expand Down Expand Up @@ -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 }),
Expand Down
4 changes: 2 additions & 2 deletions test/server/graphql/v2/query/ExpenseQuery.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ describe('server/graphql/v2/query/ExpenseQuery', () => {
});
});

it('can only see Payout method data if owner, collective admin, or host admin/accountant', async () => {
it('can only see Payout method data if owner or host admin/accountant', async () => {
// Query
const queryParams = { id: expense.id };
const resultUnauthenticated = await graphqlQueryV2(expenseQuery, queryParams);
Expand All @@ -114,7 +114,7 @@ describe('server/graphql/v2/query/ExpenseQuery', () => {
// Check results
expect(resultUnauthenticated.data.expense.payoutMethod.data).to.be.null;
expect(resultAsRandomUser.data.expense.payoutMethod.data).to.be.null;
expect(resultAsCollectiveAdmin.data.expense.payoutMethod.data).to.deep.equal(payoutMethod.data);
expect(resultAsCollectiveAdmin.data.expense.payoutMethod.data).to.be.null;
expect(resultAsOwner.data.expense.payoutMethod.data).to.deep.equal(payoutMethod.data);
expect(resultAsHostAdmin.data.expense.payoutMethod.data).to.deep.equal(payoutMethod.data);
expect(resultAsHostAccountant.data.expense.payoutMethod.data).to.deep.equal(payoutMethod.data);
Expand Down

0 comments on commit 176f0c0

Please sign in to comment.