[$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 contributors@expensify.com 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): applausetester+gm103221@applause.expensifail.com 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
• [x] Android: mWeb Chrome
• [ ] iOS: Standalone
• [ ] iOS: HybridApp
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/b2dee8bf-13e3-46fe-874c-3692f2815e65

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

Issue Owner

Current Issue Owner: @s77rt

[melvin-bot[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

https://github.com/Expensify/App/blob/3edc346319174eba90b494bc1f06c2f28056d4bd/src/libs/actions/User.ts#L1262-L1266

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

https://github.com/Expensify/App/blob/2fe1a064b38a9422dc58b6675fa51de6ef13bfc2/src/libs/Navigation/AppNavigator/AuthScreens.tsx#L570

(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[bot]]

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

[melvin-bot[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[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[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[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!

[s77rt]

@alexpensify https://expensify.slack.com/archives/C01GTK53T8Q/p1731059122077419

[melvin-bot[bot]]

@alexpensify, @chiragsalian, @s77rt Huh... This is 4 days overdue. Who can take care of this?

[s77rt]

Still discussing

[s77rt]

Okay, so we are going with the following solution:

1. After access is removed, the BE will send you pusher event
2. After you get the pusher event, you should be brought back to using your own account
3. A modal should be displayed informing you what just happened

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

[s77rt]

Now, we are looking for proposals...

cc @nkdengineer please update your proposal to reflect the new requirement

[nkdengineer]

@s77rt Updated proposal.

[s77rt]

@nkdengineer Thanks for the update!

When the access is removed, the account data will be changed like this

So the BE already sends us pusher data, no need for another data right? Can you double check the data before and after access is lost? We probably need to use usePrev hook so the effect only execute after you have lost access and not when you didn't have access at all

[nkdengineer]

@s77rt Here is the pusher data when we're delegating and the access is removed.

• Here is the pusher data

  

• Here is the data before the access is removed

  

• Here is the data after the access is removed

  

We probably need to use usePrev hook so the effect only execute after you have lost access and not when you didn't have access at all

isActingAsDelegate is only true if we're in delegating. After we disconnect, it will become false.

const isActingAsDelegate = !!account?.delegatedAccess?.delegate ?? false;

[s77rt]

@nkdengineer Looks good but I think we should check for delegate in delegates, instead of checking the length of delegates as if that account has multiple delegates and you are the only person who got removed the length check will fail.

[nkdengineer]

@s77rt Updated proposal.

[s77rt]

@nkdengineer Thank you! Overall looks good me.

🎀 👀 🎀 C+ reviewed Link to proposal

[melvin-bot[bot]]

Current assignee @chiragsalian is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[chiragsalian]

Here is the pusher data

Proposal LGTM. Feel free to begin working on the PR @nkdengineer . Regarding this part,

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
  }
}

I think before that we can update the push notification to set delegatedAccess.delegate to null right and then this this logic,

const isActingAsDelegate = !!account?.delegatedAccess?.delegate ?? false;

would because false and be better right? Cause right now with the push notification it looks like delegatedAccess.delegate wont change, correct me if I'm mistaken.

So it sounds like i don't need to make any immediate back end changes because the push notification of,

[{"key":"account","onyxMethod":"merge","value":{"delegatedAccess":{"delegates":[],"delegators":[]}}}]

should already be sent out. But if otherwise let me know.

[melvin-bot[bot]]

📣 @nkdengineer 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[s77rt]

@chiragsalian I think we should keep delegatedAccess.delegate because technically (from FE perspective) you are still connected to the other account. Another reason is that I think it's better to check if delegatedAccess.delegate is not in delegatedAccess.delegates rather than checking if session.email is not in delegatedAccess.delegates because if you have a secondary login the session.email would be different than delegatedAccess.delegate (or at least that's what I'm expecting). But either way, we don't need BE changes for now.

[nkdengineer]

@s77rt We have a problem here, after the access is removed and we call disconnect API, BE returns this error and we cannot switch to the current user.

[s77rt]

@nkdengineer Is there any action we can perform in that state or all return errors?

[nkdengineer]

I also tried with connect function with email the the current user but the error still the same.

[s77rt]

Hmm, okay but can we still show the modal and on user press we log him out? Not ideal but at least the user would be able to use the app again -- (will need to confirm this or find a solution to the session being expired)

[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