[hold for payment 2023-08-30] [$2000] Security - Copy button is displayed without any secret key when the 2FA step 2 page is accessed directly through URL #19496

kbecciv · 2023-05-23T21:55:12Z

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

Action Performed:

Got to https://staging.new.expensify.com/settings/security/two-factor-auth/verify on an account where you have not previously copied or downloaded 2FA secret keys.
Sign in if not already.
Notice you are not shown the 2FA secret key when the page loads. Nor if you go back, can you see the recovery codes on the previous page.

Expected Result:

Here are how we want to handle all of the various cases related to this issue:

2FA is not enabled, and the user loads:

new.expensify.com/settings/security/two-factor-auth/codes > we show codes.
new.expensify.com/settings/security/two-factor-auth/verify > we redirect to step 1 (codes).
new.expensify.com/settings/security/two-factor-auth/success > we redirect to step 1 (codes).
new.expensify.com/settings/security/two-factor-auth/enabled > we redirect to step 1 (codes).
http://new.expensify.com/settings/security/two-factor-auth/disable > we redirect to step 1 (codes).

2FA is enabled, and the user loads:

new.expensify.com/settings/security/two-factor-auth/codes > we redirect to new.expensify.com/settings/security/two-factor-auth/enabled
new.expensify.com/settings/security/two-factor-auth/verify > we redirect to [new.expensify.com/settings/security/two-factor-auth/enabled (http://new.expensify.com/settings/security/two-factor-auth/enabled)
new.expensify.com/settings/security/two-factor-auth/success > we redirect to new.expensify.com/settings/security/two-factor-auth/enabled.
new.expensify.com/settings/security/two-factor-auth/enabled > they land on enabled.
http://new.expensify.com/settings/security/two-factor-auth/disable > they land on http://new.expensify.com/settings/security/two-factor-auth/enabled .

Actual Result:

Copy button is there without any secret key shown.

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.17.0

Reproducible in staging?: Yes

Reproducible in production?: yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-05-24.at.2.28.55.AM.1.mov

Expensify/Expensify Issue URL:

Issue reported by: @adeel0202

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1684420754597459

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~012f90e1d072b364da
Upwork Job ID: 1661463527223799808
Last Price Increase: 2023-06-14

melvin-bot · 2023-05-23T21:55:17Z

Triggered auto assignment to @joekaufmanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot · 2023-05-23T21:55:19Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

huzaifa-99 · 2023-05-23T23:52:00Z

Proposal

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

We want to regenerate recovery codes when the user visits the security codes page from verify codes page after refreshing the screen no the verify codes page.

What is the root cause of that problem?

Currently the recovery codes are only regenerating in the security settings screen when the 'two factor authentication' option is clicked.

App/src/pages/settings/Security/SecuritySettingsPage.js

Line 43 in a25396f

Session.toggleTwoFactorAuth(true);

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

We should move the Session.toggleTwoFactorAuth(true); in SecuritySettingsPage.js#L43 before this line in CodesPage.js

Additionally, we can check if the recovery code is already there and fetch new one accordingly

if (!(props.account.recoveryCodes && props.account.twoFactorAuthSecretKey)) {
    Session.toggleTwoFactorAuth(true);
}

What alternative solutions did you explore? (Optional)

N/A

joekaufmanexpensify · 2023-05-24T20:05:39Z

I can reproduce this on web. If I had to suspect why this happening, it's because typically when enabling 2FA, we require you to download or copy the recovery codes, before proceeding the step that has the secret key. Whereas by loading https://staging.new.expensify.com/settings/security/two-factor-auth/verify , the user is bypassing this required first step.

IMO, the best solution would be to redirect the user to the first page of the 2FA flow (to copy/download their recovery codes), if they try and load https://staging.new.expensify.com/settings/security/two-factor-auth/verify directly, and have niot previously copied or downloaded their recovery codes.

2023-05-24_15-58-57.mp4

melvin-bot · 2023-05-24T20:05:50Z

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

melvin-bot · 2023-05-24T20:05:53Z

Current assignee @joekaufmanexpensify is eligible for the External assigner, not assigning anyone new.

melvin-bot · 2023-05-24T20:05:59Z

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

melvin-bot · 2023-05-24T20:06:03Z

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

kushu7 · 2023-05-24T21:19:28Z

Proposal

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

Copy button is displayed without any secret key when the 2FA step 2 page is accessed directly through URL

What is the root cause of that problem?

Its happening because user directly accessing settings/security/two-factor-auth/verify page. we are missing recoveryCodes and twoFactorAuthSecretKey.

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

I digged more into this.
And find out it will require more refactoring as direct access is available for all pages.
My observation:

settings/security/two-factor-auth/enabled : We should only allow direct access when props.account.requiresTwoFactorAuth is true. otherwise redirect to 2FA CODES page.
settings/security/two-factor-auth/disable: We should only allow direct access when props.account.requiresTwoFactorAuth is true. otherwise redirect to 2FA CODES page.
settings/security/two-factor-auth/codes: We should only allow direct access when props.account.requiresTwoFactorAuth is false. otherwise redirect to 2FA ENABLED page.
settings/security/two-factor-auth/verify: We should not allow direct access.if props.account.requiresTwoFactorAuth is false then redirect it to 2FA CODES.
settings/security/two-factor-auth/success:We should only allow direct access when props.account.requiresTwoFactorAuth is true. if false redirect to 2FA CODES.

We can create action like this.

function checkAccessAndRedirectIfRequired() {
    const currentRoute = ReportUtils.getRouteFromLink(Navigation.getActiveRoute()).substring(1);
    const connectionID = Onyx.connect({
        key: ONYXKEYS.ACCOUNT,
        waitForCollectionCallback: true,
        callback: (account) => {
            Onyx.disconnect(connectionID);
            
            if (!account) {
                return;
            }
            const is2faEnabled = account.requiresTwoFactorAuth;
            if (currentRoute === ROUTES.SETTINGS_2FA_CODES) {
                if (is2faEnabled) return Navigation.navigate(ROUTES.SETTINGS_2FA_IS_ENABLED)
                clearTwoFactorAuthData();
                Session.toggleTwoFactorAuth(true);
                return;
            }
            // just redirect to CODES page direct access should not be allowed
            if (currentRoute === ROUTES.SETTINGS_2FA_VERIFY || currentRoute === ROUTES.SETTINGS_2FA_IS_ENABLED) {
                if (!is2faEnabled) Navigation.navigate(ROUTES.SETTINGS_2FA_CODES)
            }
            ... others handlers for page
        }
    });

}

And have to use in ALL 2FA pages. will handle all edge cases.

OLD understanding.

We should redirect user to 1st step and toggle2FA.

App/src/pages/settings/Security/TwoFactorAuth/VerifyPage.js

Lines 54 to 59 in 6c23b16

    
           useEffect(() => { 
        
               if (props.account.requiresTwoFactorAuth) { 
        
                   Navigation.navigate(ROUTES.SETTINGS_TWO_FACTOR_SUCCESS); 
        
               } 
        
           }, [props.account.requiresTwoFactorAuth]);

Can add below this function.

    useEffect(() => {
        if (props.account.recoveryCodes) {
            return;
        }
        Navigation.navigate(ROUTES.SETTINGS_2FA_CODES)
    }, [props.account.recoveryCodes])

and in CodePage we can add this

Session.toggleTwoFactorAuth(true); // we can use this in `useEffect`

We are doing this so it will solve our #19628 Issue when user directly open this page.

and can safely remove this line from here

App/src/pages/settings/Security/SecuritySettingsPage.js

Line 43 in aaefecf

Session.toggleTwoFactorAuth(true);

What alternative solutions did you explore? (Optional)

None

Video

newvid.mov

melvin-bot · 2023-05-24T21:19:30Z

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

hoangzinh · 2023-05-25T09:20:25Z

Proposal

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

Security - Copy button is displayed without any secret key when the 2FA step 2 page is accessed directly through URL

What is the root cause of that problem?

We only generate recoveryCodes whenever user clicks on menu item Two-factor authentication in the Security page
Whenever the 1st 2FA page is unmount, we clear 2FA auth data.

=> That's reason why if we reload the Verify 2FA page or go to it by direct link, there is no recoveryCodes there.

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

First of all, we need to update the way generating recovery codes + clean up it:
- In the 1st page (CodesPage):
  - We need to move the code to generate 2FA data from here to this userEffect method of the 1st page of 2FA flow => we will check if account.recoverCodes is empty, we will call API to new 2FA data, otherwise do nothing. (preserve the code when user go back and forth)
  - We also need to remove clean up code here out of return function of useEffect. Otherwise, when we go to the Verify page first, then back to this 1st page, then go to the Verify page => this LOC will trigger and clear all 2FA auth data.
- In the VerifyPage:
  - Before navigate to success page, we need to clear the 2FA auth data for security concern.
Secondly, we need to check and redirect with condition listed here and here
- I'm imaging we will write a HOC like this (I prefer to use HOC because we can prevent unnecessary code trigger in 2FA steps/pages (componentDidMount/Update) when we're checking whether we need to redirect to any step or can stay at the current step). Which will check:
  - If OpenApp api is fetching => show full screen loading page (with key ONYXKEYS.IS_LOADING_REPORT_DATA)
  - In componentDidUpdate lifecycle (or useEffect for functional component), we will check if OpenApp api is done, then we will check:
    - if 2FA is enabled but current route is not IsEnabled page => navigate to IsEnable page.
    - Otherwise, if 2FA is not enabled but current route is IsEnabled page => navigate to CodesPage page (1st page).
    - Otherwise, if the recoveryCodes is not exists and current route is not the CodePage => navigate to CodePage.
- Use this HOC in every steps of 2FA flow

joekaufmanexpensify · 2023-05-25T19:00:32Z

@aimane-chnaif thoughts on these proposals?

melvin-bot · 2023-05-26T18:34:50Z

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

joekaufmanexpensify · 2023-05-26T18:35:39Z

Adding an additional assignee as I'm OOO next week (until June 5th). Next steps here is to make sure we get these proposals reviewed, and hopefully select one!

joekaufmanexpensify · 2023-07-17T19:32:34Z

I'd expect that you'd go back to the codes page. Do you agree @aimane-chnaif, @deetergp ?

aimane-chnaif · 2023-07-17T19:38:34Z

We originally discussed to go back to Settings when individual routes.
@koko57 when click back button on success page, which page are you showing?

koko57 · 2023-07-18T06:06:20Z

So I've implemented it this way:
success page -> back to settings
verify page -> back to codes
but I can change that easily, if necessary

aimane-chnaif · 2023-07-18T06:07:56Z

Works for me

koko57 · 2023-07-18T15:17:15Z

I've opened the draft PR in the morning, it was reviewed by my teammates. It's almost ready to be opened, but I have some problems with my android emulator and can't record videos and retest it once again, but it should be working (as there's no android specific changes here). I'll try to fix it and hopefully I'll be able to attach the videos tomorrow

koko57 · 2023-07-19T09:21:34Z

Fixed the issue and added recordings for Android. PR opened: #23060

koko57 · 2023-08-02T13:07:06Z

@aimane-chnaif bumping for re-review 🙂

joekaufmanexpensify · 2023-08-21T13:32:49Z

PR got merged last week!

joekaufmanexpensify · 2023-08-28T13:46:22Z

Payment due here on 2023-08-30!

aimane-chnaif · 2023-08-29T11:00:44Z

Coming from #25518:
@joekaufmanexpensify I accepted offer from https://www.upwork.com/jobs/~01f029092225d9427e, so let's use this contract.

joekaufmanexpensify · 2023-08-29T12:09:21Z

@aimane-chnaif just to clarify, you mean i'll take over payment for the contract you accepted in the above issue, and just handle payment in this issue using that contract? If so, sounds good to me!

aimane-chnaif · 2023-08-29T12:11:53Z

@aimane-chnaif just to clarify, you mean i'll take over payment for the contract you accepted in the above issue, and just handle payment in this issue using that contract? If so, sounds good to me!

yes exactly. Thanks

joekaufmanexpensify · 2023-08-29T19:13:39Z

Sounds good!

joekaufmanexpensify · 2023-08-30T20:24:05Z

It took a while to get this one merged, but we discussed here, and given the amount of effort involved in this issue, there will be no penalty. This means we need to issue the following payments:

@aimane-chnaif - $2,000 for C+ review (bounty was increase in May to try and find proposals). Paid via upwork.
@adeel0202 - $250 for reporting bonus. Paid via Upwork.
@koko57 - payment handled separately via agency.

joekaufmanexpensify · 2023-08-31T20:49:57Z

Original upwork job closed, so I created a new one here to pay @adeel0202 .

Separately, @sakluger got assigned a duplicate issue to pay @aimane-chnaif for their review in this issue and already hired them there, so we're going to use that job for the purposes of paying @aimane-chnaif.

joekaufmanexpensify · 2023-08-31T20:51:26Z

@adeel0202 offer sent for $250!

adeel0202 · 2023-08-31T20:53:07Z

@adeel0202 offer sent for $250!

Accepted, thanks.

joekaufmanexpensify · 2023-08-31T20:54:45Z

@adeel0202 $250 sent and contract ended!

joekaufmanexpensify · 2023-08-31T21:01:45Z

@aimane-chnaif $2,000 sent for the upwork contract you accepted in this issue, and contract ended.

joekaufmanexpensify · 2023-08-31T21:01:59Z

Closing as this one's all set. Thanks everyone!

kbecciv added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels May 23, 2023

melvin-bot bot assigned joekaufmanexpensify May 23, 2023

joekaufmanexpensify added External Added to denote the issue can be worked on by a contributor and removed Needs Reproduction Reproducible steps needed labels May 24, 2023

melvin-bot bot changed the title ~~Security - Copy button is displayed without any secret key when the 2FA step 2 page is accessed directly through URL~~ [$1000] Security - Copy button is displayed without any secret key when the 2FA step 2 page is accessed directly through URL May 24, 2023

melvin-bot bot assigned aimane-chnaif May 24, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 24, 2023

melvin-bot bot assigned deetergp May 24, 2023

kushu7 mentioned this issue May 26, 2023

Recovery codes does not show if user uses the url(https://staging.new.expensify.com/settings/security/two-factor-auth/codes) #19628

Closed

6 tasks

joekaufmanexpensify removed their assignment May 26, 2023

joekaufmanexpensify added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels May 26, 2023

melvin-bot bot assigned Christinadobrzyn May 26, 2023

This comment was marked as outdated.

Sign in to view

koko57 mentioned this issue Jul 18, 2023

Fix/19496 2fa steps refactor #23060

Merged

56 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 labels Jul 19, 2023

bernhardoj mentioned this issue Jul 20, 2023

Web - User can access Two-factor authentication (Step 2: Verify) without copying the code, resulting in an empty code being displayed with copy button #23198

Closed

6 tasks

joekaufmanexpensify removed the Daily KSv2 label Jul 20, 2023

aimane-chnaif mentioned this issue Aug 29, 2023

[HOLD for payment 2023-08-30] Payment issue for E/App PR#23060 #25518

Closed

joekaufmanexpensify added Daily KSv2 and removed Weekly KSv2 labels Aug 30, 2023

joekaufmanexpensify closed this as completed Aug 31, 2023

[hold for payment 2023-08-30] [$2000] Security - Copy button is displayed without any secret key when the 2FA step 2 page is accessed directly through URL #19496

[hold for payment 2023-08-30] [$2000] Security - Copy button is displayed without any secret key when the 2FA step 2 page is accessed directly through URL #19496

Comments

kbecciv commented May 23, 2023 • edited by joekaufmanexpensify Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented May 23, 2023

melvin-bot bot commented May 23, 2023 • edited by joekaufmanexpensify Loading

Bug0 Triage Checklist (Main S/O)

huzaifa-99 commented May 23, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

joekaufmanexpensify commented May 24, 2023 • edited Loading

melvin-bot bot commented May 24, 2023

melvin-bot bot commented May 24, 2023

melvin-bot bot commented May 24, 2023

melvin-bot bot commented May 24, 2023

kushu7 commented May 24, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

OLD understanding.

What alternative solutions did you explore? (Optional)

Video

melvin-bot bot commented May 24, 2023

hoangzinh commented May 25, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

joekaufmanexpensify commented May 25, 2023

melvin-bot bot commented May 26, 2023

This comment was marked as outdated.

joekaufmanexpensify commented May 26, 2023

joekaufmanexpensify commented Jul 17, 2023

aimane-chnaif commented Jul 17, 2023

koko57 commented Jul 18, 2023 • edited Loading

aimane-chnaif commented Jul 18, 2023

koko57 commented Jul 18, 2023

koko57 commented Jul 19, 2023

koko57 commented Aug 2, 2023

joekaufmanexpensify commented Aug 21, 2023

joekaufmanexpensify commented Aug 28, 2023

aimane-chnaif commented Aug 29, 2023

joekaufmanexpensify commented Aug 29, 2023

aimane-chnaif commented Aug 29, 2023

joekaufmanexpensify commented Aug 29, 2023

joekaufmanexpensify commented Aug 30, 2023

joekaufmanexpensify commented Aug 31, 2023

joekaufmanexpensify commented Aug 31, 2023

adeel0202 commented Aug 31, 2023

joekaufmanexpensify commented Aug 31, 2023

joekaufmanexpensify commented Aug 31, 2023

joekaufmanexpensify commented Aug 31, 2023

kbecciv commented May 23, 2023 •

edited by joekaufmanexpensify

Loading

melvin-bot bot commented May 23, 2023 •

edited by joekaufmanexpensify

Loading

huzaifa-99 commented May 23, 2023 •

edited

Loading

joekaufmanexpensify commented May 24, 2023 •

edited

Loading

kushu7 commented May 24, 2023 •

edited

Loading

hoangzinh commented May 25, 2023 •

edited

Loading

koko57 commented Jul 18, 2023 •

edited

Loading