diff --git a/src/components/WorkspaceMemberRoleList.tsx b/src/components/WorkspaceMemberRoleList.tsx index c27b37694980..b569350f1969 100644 --- a/src/components/WorkspaceMemberRoleList.tsx +++ b/src/components/WorkspaceMemberRoleList.tsx @@ -34,9 +34,12 @@ type WorkspaceMemberRoleListProps = { navigateBackTo?: Route; isLoading?: boolean; onSelectRole?: (value: ListItemType) => void; + + /** When provided, restricts the selectable roles to this set (e.g. an Authorized Payer may only be an Admin or Payments Admin) */ + allowedRoles?: Array>; }; -function WorkspaceMemberRoleList({role, policy, navigateBackTo = undefined, isLoading = false, onSelectRole = () => {}}: WorkspaceMemberRoleListProps) { +function WorkspaceMemberRoleList({role, policy, navigateBackTo = undefined, isLoading = false, onSelectRole = () => {}, allowedRoles = undefined}: WorkspaceMemberRoleListProps) { const {translate} = useLocalize(); const styles = useThemeStyles(); const {login: currentUserLogin = ''} = useCurrentUserPersonalDetails(); @@ -86,7 +89,9 @@ function WorkspaceMemberRoleList({role, policy, navigateBackTo = undefined, isLo }, ]; - const availableRoleItems: ListItemType[] = workspaceRoles.filter((item) => canMemberAssignRole(policy, currentUserLogin, item.value)); + const availableRoleItems: ListItemType[] = workspaceRoles.filter( + (item) => canMemberAssignRole(policy, currentUserLogin, item.value) && (!allowedRoles || allowedRoles.includes(item.value)), + ); return ( <> diff --git a/src/libs/PolicyUtils.ts b/src/libs/PolicyUtils.ts index 5b72267efafe..6e8c77156a19 100644 --- a/src/libs/PolicyUtils.ts +++ b/src/libs/PolicyUtils.ts @@ -697,6 +697,19 @@ function getReimburserEmail(policy: OnyxEntry): string | undefined { return policy.reimburser ?? policy.achAccount?.reimburser ?? (isManualReimbursement ? policy.owner : undefined); } +/** + * Whether the given role is allowed to pay (reimburse) on a workspace. + */ +function canRolePay(role: string | undefined): boolean { + return !!role && ROLE_PERMISSION_BUNDLES[role]?.[CONST.POLICY.POLICY_FEATURE.WORKFLOWS_PAYMENTS] === CONST.POLICY.POLICY_FEATURE_ACCESS.WRITE; +} + +/** + * The roles that are allowed to pay (reimburse) on a workspace, derived from the WORKFLOWS_PAYMENTS permission. The + * Authorized Payer (reimburser) must always hold one of these, so any role change for a payer is restricted to this set. + */ +const PAYER_ROLES = Object.values(CONST.POLICY.ROLE).filter(canRolePay); + function isPolicyPayer(policy: OnyxEntry, currentUserLogin: string | undefined): boolean { if (!policy) { return false; @@ -3174,6 +3187,8 @@ export { isPolicyMember, isPolicyPayer, getReimburserEmail, + PAYER_ROLES, + canRolePay, arePaymentsEnabled, isSubmitterAndApprover, isSubmitAndClose, diff --git a/src/pages/workspace/WorkspaceMembersPage.tsx b/src/pages/workspace/WorkspaceMembersPage.tsx index 4349da8316b0..6b414ff61ba7 100644 --- a/src/pages/workspace/WorkspaceMembersPage.tsx +++ b/src/pages/workspace/WorkspaceMembersPage.tsx @@ -589,7 +589,8 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers options.push(memberOption); } - if (hasAtLeastOneNonAdminRole && !hasAtLeastOnePayer && canAssignElevatedRoles) { + // Admin is a valid payer role, so the payer may be promoted to Admin (Admin and Payments Admin are the two roles that can pay). + if (hasAtLeastOneNonAdminRole && canAssignElevatedRoles) { options.push(adminOption); } @@ -611,7 +612,8 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers options.push(peopleAdminOption); } - if (hasAtLeastOneNonPaymentsAdminRole && isControlPolicy(policy) && !hasAtLeastOnePayer && canAssignElevatedRoles) { + // Payments Admin is a valid payer role, so the payer may be changed to Payments Admin (Admin and Payments Admin are the two roles that can pay). + if (hasAtLeastOneNonPaymentsAdminRole && isControlPolicy(policy) && canAssignElevatedRoles) { options.push(paymentsAdminOption); } diff --git a/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx b/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx index 1e6f423a0f77..9ba89e150ece 100644 --- a/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx +++ b/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx @@ -16,7 +16,6 @@ import {useCompanyCardFeedIcons} from '@hooks/useCompanyCardIcons'; import useConfirmModal from '@hooks/useConfirmModal'; import {useCurrencyListActions} from '@hooks/useCurrencyList'; import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'; -import useEnvironment from '@hooks/useEnvironment'; import useExpensifyCardFeeds from '@hooks/useExpensifyCardFeeds'; import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset'; import useLocalize from '@hooks/useLocalize'; @@ -41,6 +40,7 @@ import { getReimburserEmail, isControlPolicy, isPolicyApprover, + PAYER_ROLES, tryNavigateToSubmitWorkspaceUpgrade, } from '@libs/PolicyUtils'; import shouldRenderTransferOwnerButton from '@libs/shouldRenderTransferOwnerButton'; @@ -114,7 +114,6 @@ function WorkspaceMemberDetailsPage({personalDetails, policy, route}: WorkspaceM const illustrations = useThemeIllustrations(); const companyCardFeedIcons = useCompanyCardFeedIcons(); const {accountID: currentUserAccountID, login: currentUserLogin = ''} = useCurrentUserPersonalDetails(); - const {environmentURL} = useEnvironment(); const [cardFeeds] = useCardFeeds(policyID); const [cardList] = useOnyx(`${ONYXKEYS.COLLECTION.WORKSPACE_CARDS_LIST}`); const [customCardNames] = useOnyx(ONYXKEYS.NVP_EXPENSIFY_COMPANY_CARDS_CUSTOM_NAMES); @@ -142,12 +141,14 @@ function WorkspaceMemberDetailsPage({personalDetails, policy, route}: WorkspaceM const ownerDetails = personalDetails?.[policy?.ownerAccountID ?? CONST.DEFAULT_NUMBER_ID] ?? ({} as PersonalDetails); const policyOwnerDisplayName = temporaryGetDisplayNameOrDefault({passedPersonalDetails: ownerDetails, translate, formatPhoneNumber}) ?? policy?.owner ?? ''; const {cardList: assignableCards, ...workspaceCards} = getAllCardsForWorkspace(workspaceAccountID, cardList, cardFeeds, expensifyCardSettings); - const workspaceWorkflowsPageURL = `${environmentURL}/${ROUTES.WORKSPACE_WORKFLOWS.getRoute(policyID, CONST.TAB.WORKFLOWS.PAYMENTS)}`; const isSMSLogin = Str.isSMSLogin(memberLogin); const phoneNumber = getPhoneNumber(details); const reimburserEmail = getReimburserEmail(policy); const isReimburser = !!reimburserEmail && reimburserEmail === memberLogin; - const canEditSelectedMemberRole = !isSelectedMemberOwner && !isSelectedMemberCurrentUser && !isReimburser && canManageSelectedMemberRole; + // Only let the Authorized Payer change roles when there is another payer role they can actually move to. + const assignablePayerRoles = PAYER_ROLES.filter((payerRole) => canMemberAssignRole(policy, currentUserLogin, payerRole)); + const canReimburserChangeRole = assignablePayerRoles.some((payerRole) => payerRole !== member?.role); + const canEditSelectedMemberRole = !isSelectedMemberOwner && !isSelectedMemberCurrentUser && canManageSelectedMemberRole && (!isReimburser || canReimburserChangeRole); const {isAccountLocked} = useLockedAccountState(); const {showLockedAccountModal} = useLockedAccountActions(); @@ -401,8 +402,6 @@ function WorkspaceMemberDetailsPage({personalDetails, policy, route}: WorkspaceM } Navigation.navigate(ROUTES.WORKSPACE_MEMBER_DETAILS_ROLE.getRoute(policyID, accountID)); }} - hintText={isReimburser ? translate('common.roleCannotBeChanged', workspaceWorkflowsPageURL) : undefined} - shouldRenderHintAsHTML /> {isControlPolicy(policy) && ( <> diff --git a/src/pages/workspace/members/WorkspaceMemberDetailsRolePage.tsx b/src/pages/workspace/members/WorkspaceMemberDetailsRolePage.tsx index 2876830990d5..48c6ebb92d5a 100644 --- a/src/pages/workspace/members/WorkspaceMemberDetailsRolePage.tsx +++ b/src/pages/workspace/members/WorkspaceMemberDetailsRolePage.tsx @@ -11,7 +11,7 @@ import {isRuleBotEnforcingRules} from '@libs/AgentRulesUtils'; import Navigation from '@libs/Navigation/Navigation'; import type {PlatformStackScreenProps} from '@libs/Navigation/PlatformStackNavigation/types'; import type {SettingsNavigatorParamList} from '@libs/Navigation/types'; -import {canMemberAssignRole} from '@libs/PolicyUtils'; +import {canMemberAssignRole, canRolePay, getReimburserEmail, PAYER_ROLES} from '@libs/PolicyUtils'; import AccessOrNotFoundWrapper from '@pages/workspace/AccessOrNotFoundWrapper'; import withPolicyAndFullscreenLoading from '@pages/workspace/withPolicyAndFullscreenLoading'; @@ -40,6 +40,10 @@ function WorkspaceMemberDetailsRolePage({policy, personalDetails, route}: Worksp const memberLogin = personalDetails?.[accountID]?.login ?? ''; const member = policy?.employeeList?.[memberLogin]; const canManageSelectedMemberRole = canMemberAssignRole(policy, currentUserLogin, member?.role); + // The Authorized Payer (reimburser) must stay a valid payer, so restrict them to the roles that can pay (for example Admin or Payments Admin). + const reimburserEmail = getReimburserEmail(policy); + const isReimburser = !!reimburserEmail && reimburserEmail === memberLogin; + const allowedRoles = isReimburser ? [...PAYER_ROLES] : undefined; useRedirectSubmitWorkspaceFeatureUpgrade({ policy, backTo: ROUTES.WORKSPACE_MEMBER_DETAILS.getRoute(policyID, accountID), @@ -53,6 +57,10 @@ function WorkspaceMemberDetailsRolePage({policy, personalDetails, route}: Worksp if (!canMemberAssignRole(policy, currentUserLogin, value)) { return; } + // Guard the direct-navigation path: a reimburser must stay a valid payer, so reject any role that cannot pay. + if (isReimburser && !canRolePay(value)) { + return; + } if (value !== CONST.POLICY.ROLE.ADMIN && isRuleBotEnforcingRules(accountID, policy)) { showRuleBotGuardModal('changeRole', policyID); return; @@ -76,6 +84,7 @@ function WorkspaceMemberDetailsRolePage({policy, personalDetails, route}: Worksp role={member?.role} policy={policy} onSelectRole={changeRole} + allowedRoles={allowedRoles} navigateBackTo={ROUTES.WORKSPACE_MEMBER_DETAILS.getRoute(policyID, accountID)} /> diff --git a/tests/ui/WorkspaceMemberDetailsPageTest.tsx b/tests/ui/WorkspaceMemberDetailsPageTest.tsx index 68fa39d62ce3..c68a129e0f09 100644 --- a/tests/ui/WorkspaceMemberDetailsPageTest.tsx +++ b/tests/ui/WorkspaceMemberDetailsPageTest.tsx @@ -67,6 +67,8 @@ describe('WorkspaceMemberDetailsPage', () => { const primaryAccountID = 7777; const primaryEmail = 'primary@example.com'; const secondaryEmail = 'secondary@example.com'; + const adminPayerAccountID = 8888; + const adminPayerEmail = 'adminpayer@example.com'; const policy = { ...LHNTestUtils.getFakePolicy(), @@ -83,6 +85,7 @@ describe('WorkspaceMemberDetailsPage', () => { [invitedEmail]: {email: invitedEmail, role: CONST.POLICY.ROLE.USER}, [phoneLogin]: {email: phoneLogin, role: CONST.POLICY.ROLE.USER}, [primaryEmail]: {email: primaryEmail, role: CONST.POLICY.ROLE.USER}, + [adminPayerEmail]: {email: adminPayerEmail, role: CONST.POLICY.ROLE.ADMIN}, }, }; @@ -102,6 +105,7 @@ describe('WorkspaceMemberDetailsPage', () => { [invitedAccountID]: TestHelper.buildPersonalDetails(invitedEmail, invitedAccountID, 'Invited'), [phoneAccountID]: TestHelper.buildPersonalDetails(phoneLogin, phoneAccountID, 'Phone'), [primaryAccountID]: TestHelper.buildPersonalDetails(primaryEmail, primaryAccountID, 'Primary'), + [adminPayerAccountID]: TestHelper.buildPersonalDetails(adminPayerEmail, adminPayerAccountID, 'AdminPayer'), }); await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, policy); }); @@ -223,6 +227,80 @@ describe('WorkspaceMemberDetailsPage', () => { await waitForBatchedUpdatesWithAct(); }); + it('should not lock the Role field for a non-admin Authorized Payer so they can be promoted to Admin', async () => { + // Make the invited member (a plain USER) the workspace Authorized Payer. + await act(async () => { + await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, { + reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES, + reimburser: invitedEmail, + }); + }); + + const {unmount} = renderPage({policyID: policy.id, accountID: String(invitedAccountID)}); + await waitForBatchedUpdatesWithAct(); + + await waitFor(() => { + expect(screen.getByTestId('WorkspaceMemberDetailsPage')).toBeOnTheScreen(); + }); + + // The locked hint must NOT be shown — a non-admin payer can still be promoted to Admin. + expect(screen.queryByText(/Role can/)).not.toBeOnTheScreen(); + + unmount(); + await waitForBatchedUpdatesWithAct(); + }); + + it('should not lock the Role field for an Authorized Payer who is already an Admin so they can be changed to Payments Admin', async () => { + // The admin member is the workspace Authorized Payer — Payments Admin is also a valid payer, so a lateral change is allowed. + await act(async () => { + await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, { + reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES, + reimburser: adminPayerEmail, + }); + }); + + const {unmount} = renderPage({policyID: policy.id, accountID: String(adminPayerAccountID)}); + await waitForBatchedUpdatesWithAct(); + + await waitFor(() => { + expect(screen.getByTestId('WorkspaceMemberDetailsPage')).toBeOnTheScreen(); + }); + + // The locked hint must NOT be shown — an admin payer can still be changed to Payments Admin, another valid payer role. + expect(screen.queryByText(/Role can/)).not.toBeOnTheScreen(); + + unmount(); + await waitForBatchedUpdatesWithAct(); + }); + + it('should keep the Role field interactive for an admin Authorized Payer on a non-Control workspace because Editor also holds the payments permission', async () => { + // On a Team (non-Control) workspace, Payments Admin is not assignable, but Editor also holds the WORKFLOWS_PAYMENTS + // permission, so an admin payer still has another valid payer role to switch to. The Role row must stay interactive. + await act(async () => { + await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, { + type: CONST.POLICY.TYPE.TEAM, + reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES, + reimburser: adminPayerEmail, + }); + }); + + const {unmount} = renderPage({policyID: policy.id, accountID: String(adminPayerAccountID)}); + await waitForBatchedUpdatesWithAct(); + + await waitFor(() => { + expect(screen.getByTestId('WorkspaceMemberDetailsPage')).toBeOnTheScreen(); + }); + + const roleItem = await screen.findByTestId('member-role-menu-item'); + + // Editor is another payer role the admin payer can switch to, so the row is interactive with no lock hint. + expect(roleItem).not.toBeDisabled(); + expect(screen.queryByText(/Role can/)).not.toBeOnTheScreen(); + + unmount(); + await waitForBatchedUpdatesWithAct(); + }); + it('should show the not found page when the accountID matches no workspace member', async () => { const {unmount} = renderPage({policyID: policy.id, accountID: '999999'}); await waitForBatchedUpdatesWithAct(); diff --git a/tests/ui/WorkspaceMembersTest.tsx b/tests/ui/WorkspaceMembersTest.tsx index 8858c337d1b4..d7968922e029 100644 --- a/tests/ui/WorkspaceMembersTest.tsx +++ b/tests/ui/WorkspaceMembersTest.tsx @@ -429,11 +429,10 @@ describe('WorkspaceMembers', () => { await waitForBatchedUpdatesWithAct(); }); - it('should hide role-change options when the selected member is the Authorized Payer resolved via policy.reimburser', async () => { + it('should hide demotions but offer Make payments admin when the selected member is the Authorized Payer resolved via policy.reimburser', async () => { // Given a workspace whose Authorized Payer is an admin configured through policy.reimburser - // (the canonical resolution) rather than achAccount.reimburser. On the buggy code the guard - // only read achAccount.reimburser, so it failed to recognize this payer and wrongly offered - // the role-change options. + // (the canonical resolution) rather than achAccount.reimburser. Demotions to roles that cannot + // pay must stay hidden, but changing to Payments Admin (the other valid payer role) must be offered. await act(async () => { await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, { reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES, @@ -459,7 +458,7 @@ describe('WorkspaceMembers', () => { expect(screen.getByTestId(`PopoverMenuItem-${removeText}`)).toBeOnTheScreen(); }); - // ...and none of the role-change options are offered for the payer + // ...the demotions that would strip the payer of pay capability are hidden const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 1}); expect(screen.queryByTestId(`PopoverMenuItem-${makeMemberText}`)).not.toBeOnTheScreen(); @@ -469,16 +468,18 @@ describe('WorkspaceMembers', () => { const makeCardAdminText = TestHelper.translateLocal('workspace.people.makeCardAdmin', {count: 1}); expect(screen.queryByTestId(`PopoverMenuItem-${makeCardAdminText}`)).not.toBeOnTheScreen(); + // ...but Make payments admin IS offered — Payments Admin is a valid payer role + const makePaymentsAdminText = TestHelper.translateLocal('workspace.people.makePaymentsAdmin', {count: 1}); + expect(screen.getByTestId(`PopoverMenuItem-${makePaymentsAdminText}`)).toBeOnTheScreen(); + unmount(); await waitForBatchedUpdatesWithAct(); }); - it('should hide the Make workspace admin option when the selected member is a Payments Admin who is the Authorized Payer', async () => { - // Given a Payments Admin who is also the Authorized Payer. PAYMENTS_ADMIN is the only non-admin - // role with write access to WORKFLOWS_PAYMENTS, so it is the sole role that can hold the payer - // role without already being an admin — which makes it the only path that can reach the - // "Make workspace admin" option. Every other role-change option is already gated on the payer, - // but adminOption was not, so it was wrongly offered for this payer. + it('should offer Make workspace admin but hide demotions when the selected member is a Payments Admin who is the Authorized Payer', async () => { + // Given a Payments Admin who is also the Authorized Payer. Admin and Payments Admin are both valid + // payer roles, so promoting this payer to Admin keeps them a valid payer and must be offered. + // Every demotion to a role that cannot pay (Member, Auditor, Card Admin) stays gated on the payer. await act(async () => { await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, { reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES, @@ -507,9 +508,16 @@ describe('WorkspaceMembers', () => { expect(screen.getByTestId(`PopoverMenuItem-${removeText}`)).toBeOnTheScreen(); }); - // ...but "Make workspace admin" is hidden for the payer, even though their role is not admin + // ...and "Make workspace admin" IS offered — Admin is a valid payer role const makeAdminText = TestHelper.translateLocal('workspace.people.makeAdmin', {count: 1}); - expect(screen.queryByTestId(`PopoverMenuItem-${makeAdminText}`)).not.toBeOnTheScreen(); + expect(screen.getByTestId(`PopoverMenuItem-${makeAdminText}`)).toBeOnTheScreen(); + + // ...but the demotions that would strip the payer of pay capability stay hidden + const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 1}); + expect(screen.queryByTestId(`PopoverMenuItem-${makeMemberText}`)).not.toBeOnTheScreen(); + + const makeAuditorText = TestHelper.translateLocal('workspace.people.makeAuditor', {count: 1}); + expect(screen.queryByTestId(`PopoverMenuItem-${makeAuditorText}`)).not.toBeOnTheScreen(); unmount(); await waitForBatchedUpdatesWithAct();