[$250] Copilot - Copilot with removed access can still edit profile details

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.55-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5140765&group_by=cases:section_id&group_id=229064&group_order=asc
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

1. User A is a main account
2. User B is a copilot with full copilot access
   Steps:
3. As User A navigate to Settings > Security
4. Remove Copilot access for User B
5. As User B try to make some changes in profile section (Add display name, status..)

Expected Result:

Copilot with removed access should not be able to make changes in main account

Actual Result:

Copilot with removed access can still edit profile details to main account

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6649468_1730215937183.Recording__4438.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021854642791837871459
• Upwork Job ID: 1854642791837871459
• Last Price Increase: 2024-11-07
• Automatic offers:
• nkdengineer | Contributor | 104995797

[melvin-bot]

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

@alexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[nkdengineer]

Edited by proposal-police: This proposal was edited at 2024-11-08 03:01:39 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Copilot with removed access can still edit profile details to main account

What is the root cause of that problem?

The API returns an error as expected but we don't revert the display name or status in failure data

App/src/libs/actions/User.ts

Lines 1262 to 1266 in 3edc346

	API.write(WRITE_COMMANDS.UPDATE_STATUS, parameters, {
	optimisticData,
	});
	}

What changes do you think we should make in order to solve the problem?

When the access is removed, the account data will be changed like this. So we can use the account data to check whether delegate access has been removed.

1. In AuthScreen, we can add a useEffect to disconnect the delegate access and display a modal

useEffect(() => {
    const isActingAsDelegate = !!account?.delegatedAccess?.delegate;
    const delegates = account?.delegatedAccess?.delegates ?? [];
    const isAccessRemoved = delegates.findIndex((delegate) => delegate.email === session?.email) === -1;

    if (!isActingAsDelegate || !isAccessRemoved) {
        return;
    }
    disconnect();
    // add logic display the modal here

}, [account]);

2. Regarding the modal, we can reuse DelegateNoAccessModal with a bit of customization to change the title/subtitle if necessary. Or we can create a new one based on DelegateNoAccessModal. The design team can confirm the content.

Then add the modal to AuthScreen here

App/src/libs/Navigation/AppNavigator/AuthScreens.tsx

Line 570 in 2fe1a06

<SearchRouterModal />

(the pusher data that the BE should send is something that the contributor would suggest part of his proposal)

3. If these checks aren't enough to check the delegate is removed or not, BE can send a pusher data for account data with a field in delegatedAccess like isCurrentDelegateAccessRemoved. And we can use this field to detect the access is removed.

{
  delegatedAccess: {
      isCurrentDelegateAccessRemoved: true
  }
}

const isActingAsDelegate = !!account?.delegatedAccess?.delegate ?? false;
const delegators = account?.delegatedAccess?.delegators ?? [];

[alexpensify]

Adding to my testing list, I'll review this one soon.

[alexpensify]

No update yet

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~021854642791837871459

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt (External)

[alexpensify]

@s77rt - can you please confirm if this proposal will fix this issue?

Heads up, I will be offline until Tuesday, November 12, 2024, and will not actively watch over this GitHub during that period.

If this GitHub requires an urgent update, please ask for help in the #expensify-open-source Slack Room. If it can wait, I'll continue the review process when I return online. Thanks!

[s77rt]

@nkdengineer Thanks for the proposal! I think this is design related. I have asked in Slack https://expensify.slack.com/archives/C01GTK53T8Q/p1731059122077419

[s77rt]

🎀 👀 🎀 Needs BE changes

[melvin-bot]

Triggered auto assignment to @chiragsalian, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[s77rt]

@chiragsalian This may need some BE changes (send pusher events) please check the linked Slack thread above

[melvin-bot]

@alexpensify, @chiragsalian, @s77rt Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[s77rt]

Discussing on slack. We have a plan, now we need the BE changes and the design

[melvin-bot]

@alexpensify @chiragsalian @s77rt this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[alexpensify]

@s77rt can you share the Slack thread link here? Thanks!

[nkdengineer]

@s77rt this PR is ready for review

[nkdengineer]

@s77rt the current modal content like this. How should we change it?

[s77rt]

@Expensify/design may be able to help here

[shawnborton]

I feel like we had this conversation somewhere else too. Basically if UserB is the copilot and suddenly they have their access removed, we should just refresh the page and send UserB back to their own account with an alert modal explaining what happened. Does that sound familiar?

[dannymcclain]

Yeah I definitely remember having a conversation somewhere and arriving at that solution. Not sure where, but I think that's the route we should take for sure.

[s77rt]

Yeah that's it. We are looking for the copy confirmation or should we ask on slack?

Btw, currently the BE revoke access to the current user and we can't switch back to our account. We will ask the user to logout for now. cc @chiragsalian you may want to look into this if we want to switch account instead of logging the user out

[shawnborton]

I would ask on Slack or let's see what @jamesdeanexpensify thinks.

[jamesdeanexpensify]

Would this work?

Not so fast...
You need to be a copilot to access this page. Sorry!

And then maybe link on "copilot" to the copilot HelpDot page?

[shawnborton]

I think we might actually need copy for when you get booted out of the Copilot account and land suddenly back in your own. Does that sound right @s77rt ?

[s77rt]

That was the initial plan but now it seems we can't land the user back on his own account. Instead we want to force user to logout (clicking the button will log you out), and still inform them that their delegate access has been removed.

[jamesdeanexpensify]

Something like this then?

Copilot access removed
You've been removed as a copilot for [email/phone] so you no longer have access to their account. Sorry!

[shawnborton]

That copy works for me 👍

[chiragsalian]

Hmm from a product perspective, i think investing time to force the copiloted user to logout is a wrong design choice.

Even if its more complicated we should figure out whats needed so that the copilot user, gets booted out from the copilot account, and back into their original account instead of a logout, and they see the message james provided. Others can weigh in if they feel differently. Unless technically the former is too hard to achieve.

[shawnborton]

I agree with that Chirag.

[s77rt]

True! The backend invalidates our session and we can't do anything at this point (from frontend perspective). This requires backend changes: If user A is using another account and his access is removed, then sign user A back into his account.

Would you be able to look into this @chiragsalian? Internally I suppose we can just call the signin function but send the onyx data via the pusher

[chiragsalian]

Would you be able to look into this @chiragsalian?

Sure i can, but full transparency, i may not be able to this week because of more pressing items. I should be able to investigate next week.

[s77rt]

@chiragsalian Any updates on this?

[chiragsalian]

Sorry, unfortunately not yet. Been busy with higher priority items. I'm hoping to get to this soon.

[alexpensify]

Weekly Update: Waiting for an internal investigation

[alexpensify]

@chiragsalian - any update if you can review this one soon? Thanks!

[chiragsalian]

i don't think there is anything to review here. I basically need to make some backend changes. After that I'm unsure if some external changes would still be necessary. The issue is unfortunately low on my priority list.

[alexpensify]

@chiragsalian - I forgot to ask but should we make this a Monthly?

[chiragsalian]

i think since this affects users it should remain a weekly. I feel like if we set it to monthly it would just be forgotten and closed.

[nkdengineer]

@chiragsalian any update here? Thanks!