From 63f7c7d1158468f91fef8cb266acc390daf00e59 Mon Sep 17 00:00:00 2001 From: "Cong Pham (via MelvinBot)" Date: Sun, 13 Sep 2026 02:45:36 +0000 Subject: [PATCH 1/2] Only let policy admins and managers edit expenses on expense reports Co-authored-by: Cong Pham --- src/libs/ReportUtils.ts | 12 ++++++-- tests/unit/ReportUtilsTest.ts | 57 +++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index 8f70f6bf3cd0..c6d758ab6229 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -5118,13 +5118,19 @@ function canEditMoneyRequest( } // This will be fixed as part of https://github.com/Expensify/Expensify/issues/507850 const reportPolicy = policy ?? getPolicy(moneyRequestReport?.policyID); - const isAdmin = reportPolicy?.role === CONST.POLICY.ROLE.ADMIN; - const isManager = deprecatedCurrentUserAccountID === moneyRequestReport?.managerID; + const isManagerOfReport = deprecatedCurrentUserAccountID === moneyRequestReport?.managerID; - if (isInvoiceReport(moneyRequestReport) && (isManager || isChatReportArchived)) { + if (isInvoiceReport(moneyRequestReport) && (isManagerOfReport || isChatReportArchived)) { return false; } + // Only an expense report confers admin/manager editing rights, matching canCurrentUserEditExpense. Without this + // guard an unreported expense (self-DM track expense) is weighed against the caller's policy, which is the viewer's + // own default workspace rather than the expense's, so anyone who admins any workspace could edit someone else's + // expense. A self-DM also has no managerID, so an unresolved account ID would otherwise match it. + const isAdmin = isExpenseReport(moneyRequestReport) && reportPolicy?.role === CONST.POLICY.ROLE.ADMIN; + const isManager = isExpenseReport(moneyRequestReport) && isManagerOfReport; + // Admin & managers can always edit coding fields such as tag, category, billable, etc. if (isAdmin || isManager) { return true; diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 0831ed30b66d..c586ef24b3a8 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -6105,6 +6105,63 @@ describe('ReportUtils', () => { expect(canEditRequest).toEqual(false); }); + it('should not let someone else edit an unreported expense just because they are an admin of their own workspace', async () => { + const otherUserAccountID = 99; + const selfDMReport: Report = { + reportID: '98099', + type: CONST.REPORT.TYPE.CHAT, + chatType: CONST.REPORT.CHAT_TYPE.SELF_DM, + ownerAccountID: otherUserAccountID, + stateNum: CONST.REPORT.STATE_NUM.OPEN, + statusNum: CONST.REPORT.STATUS_NUM.OPEN, + }; + // The workspace the viewer would move the expense to, which is their own - not the workspace the expense belongs to + const viewerOwnPolicy: Policy = { + ...createRandomPolicy(98099, CONST.POLICY.TYPE.TEAM), + role: CONST.POLICY.ROLE.ADMIN, + }; + const transaction = { + ...createRandomTransaction(98099), + reportID: CONST.REPORT.UNREPORTED_REPORT_ID, + // A managed card transaction is allowed through earlier on, so keep this a plain tracked expense + managedCard: false, + }; + const trackExpenseAction: ReportAction = { + ...createRandomReportAction(98099), + reportID: selfDMReport.reportID, + actionName: CONST.REPORT.ACTIONS.TYPE.IOU, + actorAccountID: otherUserAccountID, + message: [{type: CONST.REPORT.MESSAGE.TYPE.TEXT, text: ''}], + previousMessage: undefined, + originalMessage: { + IOUTransactionID: transaction.transactionID, + amount: 530, + currency: CONST.CURRENCY.USD, + type: CONST.IOU.REPORT_ACTION_TYPE.TRACK, + }, + }; + + const reportCollectionDataSet: ReportCollectionDataSet = { + [`${ONYXKEYS.COLLECTION.REPORT}${selfDMReport.reportID}`]: selfDMReport, + }; + const transactionCollectionDataSet: TransactionCollectionDataSet = { + [`${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`]: transaction, + }; + + await Onyx.multiSet({ + [ONYXKEYS.PERSONAL_DETAILS_LIST]: participantsPersonalDetails, + [ONYXKEYS.SESSION]: {email: currentUserEmail, accountID: currentUserAccountID}, + ...reportCollectionDataSet, + ...transactionCollectionDataSet, + }); + await waitForBatchedUpdates(); + + expect(canEditMoneyRequest(trackExpenseAction, transaction, undefined, false, selfDMReport, viewerOwnPolicy)).toBe(false); + + // The person who tracked the expense keeps their own edit rights + expect(canEditMoneyRequest({...trackExpenseAction, actorAccountID: currentUserAccountID}, transaction, undefined, false, selfDMReport, viewerOwnPolicy)).toBe(true); + }); + it('should use the passed reportActions to determine whether the report was forwarded since the last submit', async () => { const reportID = '89015'; const transactionID = '89015-transaction'; From a428b95c22548cf486ab75df3add2ab8235cfb80 Mon Sep 17 00:00:00 2001 From: "Cong Pham (via MelvinBot)" Date: Wed, 16 Sep 2026 17:33:25 +0000 Subject: [PATCH 2/2] Keep admin rights on invoice reports, not just expense reports Restricting the admin/manager shortcut to expense reports also stripped it from invoice reports, which pushed a policy admin past the Dynamic External Workflow guard and made canEditFieldOfMoneyRequestTest fail whenever createRandomPolicy happened to pick that approval mode. Gate on isFinancialReportsForBusinesses instead, which covers both workspace report types while still excluding self-DMs, and add a deterministic regression test. Co-authored-by: Cong Pham --- src/libs/ReportUtils.ts | 14 +++++---- tests/unit/ReportUtilsTest.ts | 53 +++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 6 deletions(-) diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index 882998b1200d..826429fce986 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -5133,12 +5133,14 @@ function canEditMoneyRequest( return false; } - // Only an expense report confers admin/manager editing rights, matching canCurrentUserEditExpense. Without this - // guard an unreported expense (self-DM track expense) is weighed against the caller's policy, which is the viewer's - // own default workspace rather than the expense's, so anyone who admins any workspace could edit someone else's - // expense. A self-DM also has no managerID, so an unresolved account ID would otherwise match it. - const isAdmin = isExpenseReport(moneyRequestReport) && reportPolicy?.role === CONST.POLICY.ROLE.ADMIN; - const isManager = isExpenseReport(moneyRequestReport) && isManagerOfReport; + // Admin/manager rights only apply when the expense actually sits on a workspace report. Without this guard an + // unreported expense (self-DM track expense) is weighed against the caller's policy, which is the viewer's own + // default workspace rather than the expense's, so anyone who admins any workspace could edit someone else's + // expense. A self-DM also has no managerID, so an unresolved account ID would otherwise match it. Invoice reports + // stay included so a policy admin keeps the rights they had before this guard existed. + const isReportOnAWorkspace = isFinancialReportsForBusinesses(moneyRequestReport); + const isAdmin = isReportOnAWorkspace && reportPolicy?.role === CONST.POLICY.ROLE.ADMIN; + const isManager = isReportOnAWorkspace && isManagerOfReport; // Admin & managers can always edit coding fields such as tag, category, billable, etc. if (isAdmin || isManager) { diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index d58adb892e83..c1695e403a96 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -6222,6 +6222,59 @@ describe('ReportUtils', () => { expect(canEditMoneyRequest({...trackExpenseAction, actorAccountID: currentUserAccountID}, transaction, undefined, false, selfDMReport, viewerOwnPolicy)).toBe(true); }); + it('should still let a policy admin edit an expense on an unapproved invoice report', async () => { + // Invoice reports are workspace reports, so restricting admin rights to reported expenses must not strip + // them here. Dynamic External Workflow is set deliberately because only the admin check short-circuits + // ahead of the guard that blocks non-admins on reports that are no longer open. + const invoicePolicy: Policy = { + ...createRandomPolicy(98100, CONST.POLICY.TYPE.TEAM), + role: CONST.POLICY.ROLE.ADMIN, + approvalMode: CONST.POLICY.APPROVAL_MODE.DYNAMICEXTERNAL, + }; + const invoiceReport: Report = { + reportID: '98100', + type: CONST.REPORT.TYPE.INVOICE, + policyID: invoicePolicy.id, + ownerAccountID: 99, + // A different account, so the invoice-manager early return below does not apply + managerID: 8723, + stateNum: CONST.REPORT.STATE_NUM.SUBMITTED, + statusNum: CONST.REPORT.STATUS_NUM.SUBMITTED, + }; + const transaction = {...createRandomTransaction(98100), reportID: invoiceReport.reportID}; + const invoiceAction: ReportAction = { + ...createRandomReportAction(98100), + reportID: invoiceReport.reportID, + actionName: CONST.REPORT.ACTIONS.TYPE.IOU, + actorAccountID: 99, + message: [{type: CONST.REPORT.MESSAGE.TYPE.TEXT, text: ''}], + previousMessage: undefined, + originalMessage: { + IOUTransactionID: transaction.transactionID, + amount: 530, + currency: CONST.CURRENCY.USD, + type: CONST.IOU.REPORT_ACTION_TYPE.CREATE, + }, + }; + + const reportCollectionDataSet: ReportCollectionDataSet = { + [`${ONYXKEYS.COLLECTION.REPORT}${invoiceReport.reportID}`]: invoiceReport, + }; + const transactionCollectionDataSet: TransactionCollectionDataSet = { + [`${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`]: transaction, + }; + + await Onyx.multiSet({ + [ONYXKEYS.PERSONAL_DETAILS_LIST]: participantsPersonalDetails, + [ONYXKEYS.SESSION]: {email: currentUserEmail, accountID: currentUserAccountID}, + ...reportCollectionDataSet, + ...transactionCollectionDataSet, + }); + await waitForBatchedUpdates(); + + expect(canEditMoneyRequest(invoiceAction, transaction, undefined, false, invoiceReport, invoicePolicy)).toBe(true); + }); + it('should use the passed reportActions to determine whether the report was forwarded since the last submit', async () => { const reportID = '89015'; const transactionID = '89015-transaction';