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.55k stars 2.89k forks source link

[$250] Login - Incorrect page with an incorrect MC is shown when navigating to an invalid MC link #52124

Open lanitochka17 opened 1 week ago

lanitochka17 commented 1 week ago

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.58-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5190900 Email or phone of affected tester (no customers): https://expensify.testrail.io/index.php?/tests/view/5190900 Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify
  2. Enter an existing Gmail account on the login page
  3. Navigate to the email inbox of the account and open the validate email and copy the link
  4. Paste the link in an incognito tab (or another chrome account) and change it to staging
  5. Modify the last portion of the link by a character, i.e. If the original link is like this https://staging.new.expensify.com/v/7453760/123884, change a single character > https://staging.new.expensify.com/v/7453760/12388**8**
  6. Hit enter and navigate to the link

Expected Result:

The user is redirected the "Magic code expired" page

Actual Result:

"Here's your magic code" page appears with an incorrect magic code

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/a400680b-f60e-477a-8a7d-e071f306358a

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021854430009812006082
  • Upwork Job ID: 1854430009812006082
  • Last Price Increase: 2024-11-07
  • Automatic offers:
    • DylanDylann | Reviewer | 104849643
    • mkzie2 | Contributor | 104849645
Issue OwnerCurrent Issue Owner: @DylanDylann
melvin-bot[bot] commented 1 week ago

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

Christinadobrzyn commented 1 week ago

Oh interesting - this is reproducible, I'm going to move this to convert and set this external. It might need to be internal.

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

huult commented 1 week ago

Edited by proposal-police: This proposal was edited at 2024-11-07 09:10:58 UTC.

Proposal

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

Incorrect page with an incorrect MC is shown when navigating to an invalid MC link

What is the root cause of that problem?

New feature We haven't yet handled the case where the user changes the code in the parameters, so when the user modifies the code in the parameters, it will apply to the validate code page.

https://github.com/Expensify/App/blob/f05be952684549101854d0317dc649acf18c5f6e/src/pages/ValidateLoginPage/index.website.tsx#L77-L87

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

To resolve this ticket, we should validate the code to show ExpiredValidateCodeModal in cases of an invalid code (e.g., 88888888, 8939*282...), and show ValidateCodeModal if the code is valid. Something like this:

//.src/pages/ValidateLoginPage/index.website.tsx#L78
+    const isInvalidCode = !ValidationUtils.isValidValidateCode(validateCode);

    return (
        <>
-           {autoAuthStateWithDefault === CONST.AUTO_AUTH_STATE.FAILED && <ExpiredValidateCodeModal />}        
+            {(autoAuthStateWithDefault === CONST.AUTO_AUTH_STATE.FAILED || isInvalidCode) && <ExpiredValidateCodeModal />}
            {autoAuthStateWithDefault === CONST.AUTO_AUTH_STATE.JUST_SIGNED_IN && is2FARequired && !isSignedIn && !!login && <JustSignedInModal is2FARequired />}
            {autoAuthStateWithDefault === CONST.AUTO_AUTH_STATE.JUST_SIGNED_IN && isSignedIn && !exitTo && !!login && <JustSignedInModal is2FARequired={false} />}
            {/* If session.autoAuthState isn't available yet, we use shouldStartSignInWithValidateCode to conditionally render the component instead of local autoAuthState which contains a default value of NOT_STARTED */}
-           {(!autoAuthState ? !shouldStartSignInWithValidateCode : autoAuthStateWithDefault === CONST.AUTO_AUTH_STATE.NOT_STARTED) && !exitTo && (
+            {(!autoAuthState ? !shouldStartSignInWithValidateCode : autoAuthStateWithDefault === CONST.AUTO_AUTH_STATE.NOT_STARTED) && !exitTo && !isInvalidCode && (
                <ValidateCodeModal
                    accountID={Number(accountID)}
                    code={validateCode}
                />
            )}
            {(!autoAuthState ? shouldStartSignInWithValidateCode : autoAuthStateWithDefault === CONST.AUTO_AUTH_STATE.SIGNING_IN) && <FullScreenLoadingIndicator />}
        </>
    );

What alternative solutions did you explore? (Optional)

We should show a 'not found' page when the user tries to edit the code in the parameters.

// src/pages/ValidateLoginPage/index.website.tsx#L77
    const isInvalidCode = !ValidationUtils.isValidValidateCode(validateCode);

    if (isInvalidCode) {
        return <FullPageNotFoundView shouldShow />;
    }

Or, if we use this code in ValidateCodeModal, we will discuss it in the PR phrase, something like that:

POC https://github.com/user-attachments/assets/49b32f13-f196-4393-b1a8-96b8a2362438
mkzie2 commented 1 week ago

Edited by proposal-police: This proposal was edited at 2024-11-11 09:33:47 UTC.

Proposal

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

"Here's your magic code" page appears with an incorrect magic code

What is the root cause of that problem?

There is no Onyx data in the incognito tab. This causes shouldStartSignInWithValidateCode to be false, and signInWithValidateCode to ben't called, which causes the autoAuthState to ben't updated to CONST.AUTO_AUTH_STATE.FAILED.

https://github.com/Expensify/App/blob/27b4786cceb9f44fad1f065b06ee56dc046a9716/src/pages/ValidateLoginPage/index.website.tsx#L32

https://github.com/Expensify/App/blob/27b4786cceb9f44fad1f065b06ee56dc046a9716/src/pages/ValidateLoginPage/index.website.tsx#L44-L59

Finally ValidateCodeModal is shown.

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

If the magic code is invalid, we should display a helpful text to the user instead of displaying the expired page. In ValidateCodeModal, we can use isValidValidateCode function to check the validate code is valid or not. If it's invalid we will change the description here.

https://github.com/Expensify/App/blob/eff7bffb673fb4c2f1d89b1c53a9bc1e6a799c1b/src/components/ValidateCode/ValidateCodeModal.tsx#L47-L53

It looks like this and if we click on Go back to sign in here it will redirect to the login input form.

Screenshot 2024-11-07 at 21 43 00

What alternative solutions did you explore? (Optional)

We can display not found page in ValidateCodeModal if the magic code is invalid

const isInvalidCode = !ValidationUtils.isValidValidateCode(validateCode);
if (isInvalidCode) {
    return <NotFoundPage shouldShow />;
}

or we can redirect to the not found page in ValidateLoginPage if the validateCode is invalid

Christinadobrzyn commented 1 week ago

@DylanDylann can you check out these proposals when you have a moment? TY!

DylanDylann commented 5 days ago

@mkzie2's proposal looks good to me

Note for the PR phase: We can display a Not Found Page instead of designing a new description for this case. Because on the FE we can't validate exact the magic code, we only can check the shape of the magic code based on regex. So in cases where users try to edit the URL, I would prefer to display the Not Found Page

πŸŽ€ πŸ‘€ πŸŽ€ C+ Reviewed

melvin-bot[bot] commented 5 days ago

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

huult commented 5 days ago

@DylanDylann Can you double-check the expectations for this ticket? They want to redirect to the 'Magic code expired' page, so my proposal follows that approach. I considered changing the text to 'invalid' to display in ValidateCodeModal as well, but after reviewing the expectations, I didn't include it in my proposal. But if we want to display the 'inValid' text, we need to confirm with the design team before deciding.

DylanDylann commented 5 days ago

@huult Thanks for your feedback

DylanDylann commented 5 days ago

@Expensify/design Could you help to confirm the expect on this issue?

  1. The user is redirected to the "Magic code expired" page (as in OP)
  2. The user is redirected to the Not Found Page
  3. Creating a new page with a new design for this case like this
Screenshot 2024-11-09 at 17 36 30

IMO, because on the FE we can't validate exact the magic code, we only can check the shape of the magic code based on regex. So in cases where users try to edit the URL, I would prefer to display the Not Found Page

cc @dangrous

dubielzyk-expensify commented 4 days ago

Hmm. This seems like a bit of a odd use case, but it feels more right to show "Not found" or "Something went wrong" instead of saying the mc code has expired cause it technically hasn't right (the user just tampered)? I'd probably say 2 in this instance then given this doesn't seem like something we'll run into a lot.

Keen to hear what the other @Expensify/design 'ers think

shawnborton commented 3 days ago

Yeah, I tend to agree with that too.

It seems like super old magic code links still work, even though the magic code itself won't work. So I think I would just make it so that any URL that is accessed where we aren't using 6 digits in the last portion, we should just redirect that to a 404 not found page.

mkzie2 commented 3 days ago

@dangrous Since all design teams agree with option 2 (Display the Not Found Page), please help to review this comment https://github.com/Expensify/App/issues/52124#issuecomment-2466159514. I also updated the alternative solution with the expected.

huult commented 3 days ago

Proposal updated

dangrous commented 3 days ago

yep, this makes sense! Let's go with @mkzie2 (as the first updated proposal) and display that Not Found page. Thank you!

melvin-bot[bot] commented 3 days ago

πŸ“£ @DylanDylann πŸŽ‰ 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 days ago

πŸ“£ @mkzie2 πŸŽ‰ 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 πŸ“–

mkzie2 commented 2 days ago

@DylanDylann On native we don't display the validated code modal, should we display the not found page in the case of this issue or not?

DylanDylann commented 1 day ago

@mkzie2 If so, we should do nothing on native

mkzie2 commented 1 day ago

@DylanDylann The PR is ready.