search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.42k stars 2.8k forks source link

[HOLD for payment 2024-07-22] [HOLD for payment 2024-07-17] [$250] Prevent requesting money from domain emails #44429

Closed techievivek closed 2 months ago

techievivek commented 3 months ago

Domain emails are in format +@<domain> like +@expensify.com.

Problem

We allow users to request money from domains in newDot. Update the code so we don't allow this action.

Screenshot 2024-06-26 at 1 10 52 PM Screenshot 2024-06-26 at 1 09 48 PM

Steps to reproduce:

  1. From the global create button, select Split expense.
  2. In the search field enter +@applause.expensifail.com' and confirm you are able to select that email as an option.

Solution

Update the code to prevent users from requesting money from domain emails.

Solution

Update the code to prevent users from requesting money from domain emails.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~014e28879d82637e16
  • Upwork Job ID: 1805872485147678962
  • Last Price Increase: 2024-06-26
  • Automatic offers:
    • rayane-djouah | Reviewer | 102895687
    • nkdengineer | Contributor | 102895688
Issue OwnerCurrent Issue Owner: @isabelastisser
melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @isabelastisser (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.

melvin-bot[bot] commented 3 months ago

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

nkdengineer commented 3 months ago

Proposal

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

We'd like to prevent requesting money from domain emails

What is the root cause of that problem?

There's no logic to exclude domain emails when getOptions

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

Add a param includeDomainEmail to getOptions https://github.com/Expensify/App/blob/57e5d0e7197a9c21e6f221389bed87870c90ef0a/src/libs/OptionsListUtils.ts#L1769, default to true

In https://github.com/Expensify/App/blob/57e5d0e7197a9c21e6f221389bed87870c90ef0a/src/libs/OptionsListUtils.ts#L1941, add

&& (includeDomainEmail || !Str.isDomainEmail(detail.login))

[Optional] In https://github.com/Expensify/App/blob/57e5d0e7197a9c21e6f221389bed87870c90ef0a/src/libs/OptionsListUtils.ts#L1915 add 

if (Str.isDomainEmail(login) && !includeDomainEmail) {
    return;
}

In the money request participants page, pass includeDomainEmail as false

Can include the same check in other places in getOptions if needed

What alternative solutions did you explore? (Optional)

includeDomainEmail=false can be used for other use cases that should not include the domain email. We can also default includeDomainEmail to false if we don't want to show this when getting options by default.

We can optionally update the message when not found to "You cannot submit expense to a domain email". It's clearer than "No results found" that's being shown to the user.

rayane-djouah commented 3 months ago

@nkdengineer's proposal looks good to me.

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 3 months ago

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

rayane-djouah commented 3 months ago

@techievivek - To confirm, Do we need to exclude domain email options in all IOU flows: Submit Expense, Split Expense, Pay Someone, Send Invoice? Do we also need to exclude them in the Start chat and assign task flows?

Edit: based on this issue https://github.com/Expensify/App/issues/44485, we need to exclude the domain email option for the Start chat flow. So, I think all flows should be considered.

techievivek commented 3 months ago

So, I think all flows should be considered.

Yeah, we want to exclude them from all the flows as as they are not real users.

melvin-bot[bot] commented 3 months ago

📣 @rayane-djouah 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 3 months ago

📣 @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 📖

iwiznia commented 3 months ago

Whatever we do here, we will ALSO need to update the backend @techievivek

nkdengineer commented 3 months ago

@rayane-djouah this PR is ready for preview

techievivek commented 3 months ago

@iwiznia Yeah, backend already handles this exception correctly.

Screenshot 2024-07-03 at 9 47 00 AM
trjExpensify commented 3 months ago

@nkdengineer @techievivek did the PR for this also take care of preventing starting a chat, a group, assigning a task, inviting to a workspace.. basically filtering the domain account out of the participant selector everywhere?

image
techievivek commented 2 months ago

@trjExpensify Yeah, we have handled this for all cases; C+ confirmed this here: https://github.com/Expensify/App/issues/44429#issuecomment-2192166488.

melvin-bot[bot] commented 2 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 2 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.5-13 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-07-17. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 2 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

trjExpensify commented 2 months ago

@trjExpensify Yeah, we have handled this for all cases; C+ confirmed this here: #44429 (comment).

Great stuff!

melvin-bot[bot] commented 2 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.6-8 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-07-22. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 2 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

isabelastisser commented 2 months ago

Bump @rayane-djouah to complete the BZ list, thanks!

isabelastisser commented 2 months ago

The payments were processed in Upwork.

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @adelekennedy (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.

isabelastisser commented 2 months ago

I will be OOO tomorrow and next week, so I am reassigning this until I return on July 29. Thanks, @adelekennedy!

Status:

rayane-djouah commented 2 months ago

Regression Test Proposal

  1. From the global create button, select one of the flows: Submit Expense, Split Expense, Pay Someone, Send Invoice, Start chat, or assign task...
  2. In the "Name, email, or phone number" field enter a domain email in this format: +@<domain>, like +@applause.expensifail.com.
  3. Verify that you aren't able to select the domain email as an option.

Do we agree 👍 or 👎

techievivek commented 2 months ago

The regression test looks good to me; @adelekennedy, can we please get that added? Thanks.

techievivek commented 2 months ago

Not overdue, completing the BZ list.

melvin-bot[bot] commented 2 months ago

@isabelastisser, @techievivek, @adelekennedy, @rayane-djouah, @nkdengineer Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

adelekennedy commented 2 months ago

https://github.com/Expensify/Expensify/issues/414452

isabelastisser commented 2 months ago

Thanks, @adelekennedy !