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.49k stars 2.84k forks source link

Add a step to the Issue New Card form that collects a magic code #50667

Open tgolen opened 1 week ago

tgolen commented 1 week ago

Problem

Someone can issue a physical or virtual Expensify card without verifying they are the owner of the account. This relates to an internal security issue.

Why this is important to solve

This is a security vulnerability which can be taken advantage of if an account is compromised.

Solution

Collect a magic code when issuing physical or virtual Expensify cards. In a little more detail:

Issue OwnerCurrent Issue Owner: @hungvu193
melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

nkdengineer commented 1 week ago

Proposal

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

Add a step to the Issue New Card form that collects a magic code

What is the root cause of that problem?

This is a new feature request

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

https://github.com/Expensify/App/blob/cff3e862e3b7623d9ade431a64bac55645964a2a/src/pages/workspace/expensifyCard/issueNew/IssueNewCardPage.tsx#L33

https://github.com/Expensify/App/blob/cff3e862e3b7623d9ade431a64bac55645964a2a/src/libs/actions/Card.ts#L628

The details can be discussed in the PR phrase. I can create a test branch if we need

What alternative solutions did you explore? (Optional)

NA

allgandalf commented 1 week ago

Should we work on this issue under Workspace Feed project instead @trjExpensify ?

tgolen commented 1 week ago

@nkdengineer thanks! I was thinking the magic code should probably be the first step. My thinking is that there isn't much sense in letting the user fill out the rest of the form if they can't get past the magic code.

nkdengineer commented 1 week ago

@tgolen If we do that we need an API to help the user verrify before moving to the next step.

twilight2294 commented 1 week ago

@tgolen I am working on a simpler proposal for this one, please wait assignment for few minutes

twilight2294 commented 1 week ago

Proposal

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

Add a step to the Issue New Card form that collects a magic code

What is the root cause of that problem?

This is a feature request.

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

We can follow the same pattern we used for delegates here, we already have the requestValidationCode api command:

https://github.com/Expensify/App/blob/9490328c0fec06128e8cb48b34fa4b8dd6a02ff7/src/libs/actions/Delegate.ts#L177-L179

NOTE: We always ask for validation code at the confirm step (Like delegates), so to match the app existing approach, we should ask it at the last.

  1. When we click on confirm button, we need to trigger requestValidationCode.
  2. Then like we do with delgates, we should create a RHP page to take form input like here. We need to create a base form logic (much can be reused from there) and then we need to create a separate file for android paltform.
  3. Then like we do in delegates, we can send the validation code with the api: https://github.com/Expensify/App/blob/4c90d62a6a56ff0000031eceeb9195d30a041a8a/src/pages/settings/Security/AddDelegate/ValidateCodeForm/BaseValidateCodeForm.tsx#L152

    
    if (cardType === CONST.EXPENSIFY_CARD.CARD_TYPE.PHYSICAL) {
        API.write(WRITE_COMMANDS.CREATE_EXPENSIFY_CARD, {...parameters, feedCountry, validateCode});
        return;
    }
    
    const domainAccountID = PolicyUtils.getWorkspaceAccountID(policyID);
    
    // eslint-disable-next-line rulesdir/no-multiple-api-calls
    API.write(WRITE_COMMANDS.CREATE_ADMIN_ISSUED_VIRTUAL_CARD, {...parameters, domainAccountID, validateCode});

    Or we can include that code in the parameters itself. We ned to add a validateCode prop to issueExpensifyCard, if the code validation fails we already have the checks in the BaseValidateCodeForm so we can use most of the logic from there

NOTE: we also need to make changes to the number of steps back to routes etc. all that can be done in the PR phase

What alternative solutions did you explore? (Optional)

nkdengineer commented 1 week ago
  1. It had the logic to call requestValidateCodeAction API when it is rendered
  2. It had a separate file for the Android platform
  3. It's also used in other places of the app

If this step is after the confirmation step, we should move the logic call API to the ValidationStep

To create ValidationStep, we can use ValidateCodeActionModal with isVisible as true. The way to use it as we do here

twilight2294 commented 1 week ago
hungvu193 commented 1 week ago
  • Using ValidateCodeActionModal is useful because
  1. It had the logic to call requestValidateCodeAction API when it is rendered
  2. It had a separate file for the Android platform
  3. It's also used in other places of the app

If this step is after the confirmation step, we should move the logic call API to the ValidationStep

To create ValidationStep, we can use ValidateCodeActionModal with isVisible as true. The way to use it as we do here

  • I notice that the idea of the last proposal is similar to what I proposed
  • If we don't want to create a new step we can add ValidateCodeActionModal in the confirmation page and will open this modal when we click on the confirmation button

I also agree with @nkdengineer at this point. We also have a PR to reuse ValidateCodeModal as much as possible here. (https://github.com/Expensify/App/pull/49445)

tgolen commented 1 week ago

Yeah, that's right, we should reuse those existing components as much as possible. It sounds like this should work:

If we don't want to create a new step we can add ValidateCodeActionModal in the confirmation page and will open this modal when we click on the confirmation button

hungvu193 commented 6 days ago

Yeah. I think we can go with @nkdengineer 's proposal here

mountiny commented 5 days ago

I want to highlight that @getusha and @hungvu193 are working on making the component for the magic code easier to reuse https://github.com/Expensify/App/pull/49445, but it has been proving tricky and taking a bit of time

tgolen commented 5 days ago

Thanks for the heads up @mountiny. Do you want to hold off on doing anything here until that's settled? Or is there something that we can do in this PR that maybe helps the progress of making it easier to reuse?

mountiny commented 4 days ago

I think it would make sense to wait on the refactor, or then we have to double the work. I hope the PR can be wrapped up soon. I have started a thread for this here to hopefully get to faster completion.