Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-04-05] [$500] Anonymous user can access the settings page #38163

Closed
1 of 6 tasks
m-natarajan opened this issue Mar 12, 2024 · 45 comments
Closed
1 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@m-natarajan
Copy link

m-natarajan commented Mar 12, 2024

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: 1.4.50-5
Reproducible in staging?: y
Reproducible in production?: y
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
Expensify/Expensify Issue URL:
Issue reported by: @trjExpensify
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1710244751238109

Action Performed:

  1. As a member of a public room type a bunch of settings page URLs in a public room. E.g https://staging.new.expensify.com/settings/profile, https://staging.new.expensify.com/settings/wallet, https://staging.new.expensify.com/settings/preferences, https://staging.new.expensify.com/settings/security
  2. Sign-out of any instance of Expensify you have on that device
  3. Enter the URL of the public room containing those links (e.g https://staging.new.expensify.com/r/xxxxxxxxxxxx)
  4. Click on the links to the settings pages as an anonymous user in the chat
  5. Observe you can access the settings pages

Expected Result:

The sign-in prompt should appear in the RHP just like clicking the "settings" bottom tab or search icon.

Actual Result:

The pages are accessible via the deep links.

Workaround:

unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Recording.2848.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012cb619021a94beb4
  • Upwork Job ID: 1768801922139336704
  • Last Price Increase: 2024-03-16
Issue OwnerCurrent Issue Owner: @mkhutornyi
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 12, 2024
Copy link

melvin-bot bot commented Mar 12, 2024

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

@mkhutornyi
Copy link
Contributor

As a bug reporter, can I take this as C+ as I already have context?

@allgandalf
Copy link
Contributor

Proposal

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

Anonymous user can access the settings page

What is the root cause of that problem?

We do not check if the user is Anonymous in InitialSettingsPage.

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

Add a check such that if user is not signed in then call signOutAndRedirectToSignIn from Session

            if (Session.isAnonymousUser() ) {
                Navigation.isNavigationReady().then(() => {
                    Session.signOutAndRedirectToSignIn();
                });
            }
            

Put the above check inside the useEffect:

useEffect(() => {
Wallet.openInitialSettingsPage();
}, []);

What alternative solutions did you explore? (Optional)

N/A

@alexpensify
Copy link
Contributor

@puneetlath would your accountID-based project cover this one? If no, then I'll pitch it to the vsb room. Thanks for checking!

@bernhardoj
Copy link
Contributor

Proposal

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

Anonymous user can access restricted page such as settings page by clicking the link from the chat.

What is the root cause of that problem?

We don't prevent the user to directly navigate to a page from a link in the chat.

// If we are handling a New Expensify link then we will assume this should be opened by the app internally. This ensures that the links are opened internally via react-navigation
// instead of in a new tab or with a page refresh (which is the default behavior of an anchor tag)
if (internalNewExpensifyPath && hasSameOrigin) {
Navigation.navigate(internalNewExpensifyPath as Route);
return;
}

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

We can check if it's a page that is accessible for anonymous user only. If it's not accesible, we redirect the user to the sign in modal.

if (Session.isAnonymousUser() && !Session.canAnonymousUserAccessRoute(internalNewExpensifyPath)) {
    Session.signOutAndRedirectToSignIn()
    return;
}
Navigation.navigate(internalNewExpensifyPath as Route);

What alternative solutions did you explore? (Optional)

We can do the anonymous check inside Navigation.navigate, but this would require more changes as there would be cyclic dependency between Session and Navigation.

@DylanDylann
Copy link
Contributor

This is dupe of #25482

@trjExpensify
Copy link
Contributor

As a #37421 (comment), can I take this as C+ as I already have context?

Yep makes sense!

This is dupe of #25482

I don't think it's strictly a dupe per se. That's about a task title/description, whereas this is about settings pages -- importantly here, things like starting the Plaid flow to enable the wallet is possible. I agree both are valid though and we should prompt for sign-in.

@youssef-lr coming from that other issue, I think we fix this, so we close this loop on being able to start that enable wallet flow without being signed-in. (Check the OP vid here for that).

@mkhutornyi
Copy link
Contributor

I agree with @bernhardoj's proposal.
As a general solution, it fixes both issues at once.

@DylanDylann
Copy link
Contributor

I think we should reopen the previous issue because there are some pending proposals

@mkhutornyi
Copy link
Contributor

Oh it looks like @DylanDylann proposed same solution in the past.
So to be fair, I think we should assign @DylanDylann in either issue.

@alexpensify
Copy link
Contributor

alexpensify commented Mar 13, 2024

I'm double-checking with the team on the assignment plan and confirming if we should run with this GH or open the dupe. I should have an update by tomorrow. Thanks!

@puneetlath
Copy link
Contributor

@alexpensify no my changes are related to mentions, which it doesn't seem like this issue is related to.

@alexpensify
Copy link
Contributor

Cool, thanks for confirming!

@melvin-bot melvin-bot bot added the Overdue label Mar 15, 2024
@alexpensify alexpensify removed their assignment Mar 16, 2024
@melvin-bot melvin-bot bot removed the Overdue label Mar 16, 2024
@alexpensify alexpensify added External Added to denote the issue can be worked on by a contributor Overdue labels Mar 16, 2024
@melvin-bot melvin-bot bot changed the title Anonymous user can access the settings page [$500] Anonymous user can access the settings page Mar 16, 2024
Copy link

melvin-bot bot commented Mar 16, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 16, 2024
Copy link

melvin-bot bot commented Mar 16, 2024

Current assignees @mkhutornyi and @DylanDylann are eligible for the External assigner, not assigning anyone new.

@alexpensify
Copy link
Contributor

Ok, instead of opening the other GH, I chatted with the team and let's work here. @mkhutornyi let's go with @DylanDylann's proposal. Next week, I'll figure out the wave or VIP for this one.

@melvin-bot melvin-bot bot removed the Overdue label Mar 16, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 29, 2024
Copy link

melvin-bot bot commented Mar 29, 2024

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

If no regressions arise, payment will be issued on 2024-04-05. 🎊

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

  • @mkhutornyi requires payment (Needs manual offer from BZ)
  • @DylanDylann requires payment (Needs manual offer from BZ)

Copy link

melvin-bot bot commented Mar 29, 2024

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:

  • [@DylanDylann] The PR that introduced the bug has been identified. Link to the PR:
  • [@DylanDylann] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@DylanDylann] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@DylanDylann] Determine if we should create a regression test for this bug.
  • [@DylanDylann] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@alexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@alexpensify
Copy link
Contributor

Tomorrow, I will prepare the payment summary but please accept the invites here:

https://www.upwork.com/jobs/~012cb619021a94beb4

@DylanDylann and @mkhutornyi -- Thanks!

Copy link

melvin-bot bot commented Apr 5, 2024

Payment Summary

Upwork Job

BugZero Checklist (@alexpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1768801922139336704/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@alexpensify
Copy link
Contributor

@DylanDylann - I've paid you via Upwork

@mkhutornyi - Please accept the invite and I can complete the process. Thanks!

@alexpensify
Copy link
Contributor

Update: Waiting for @mkhutornyi to accept the Upwork invite to complete the payment process there.

@alexpensify
Copy link
Contributor

@mkhutornyi please accept the Upwork invite and then I can complete the payment process. Thanks!

@alexpensify
Copy link
Contributor

No update here, still waiting for @mkhutornyi to accept the Upwork invite:

https://www.upwork.com/jobs/~012cb619021a94beb4

@alexpensify
Copy link
Contributor

I believe that @mkhutornyi is OOO right now. Heads up, I will be offline starting on Wednesday until Tuesday, April 23. I'll check again one more time before I go OOO to see if I can complete the payment process.

@alexpensify

This comment was marked as outdated.

@alexpensify
Copy link
Contributor

Catching up from being OOO and going to remove @mallenexpensify. I checked the Upwork job and we are still waiting for @mkhutornyi to accept.

@alexpensify
Copy link
Contributor

I've asked the team for feedback since this one has been pending payment for over a month - https://expensify.slack.com/archives/C01SKUP7QR0/p1714516012720699

@alexpensify alexpensify added Monthly KSv2 and removed Weekly KSv2 labels May 1, 2024
@alexpensify
Copy link
Contributor

@mkhutornyi - I'm going to move this to monthly. Please let me know when you are back online and we can complete the payment process. Thanks!

@alexpensify
Copy link
Contributor

Heads up, I will be offline until Tuesday, May 28, 2024, and will not actively watch over this GitHub during that period.

If anything urgent is needed here, please ask for help in the #expensify-open-source Slack Room-- thanks!

@alexpensify
Copy link
Contributor

alexpensify commented May 31, 2024

Monthly check-in - no update here. Setting a reminder to review in a month.

@mallenexpensify mallenexpensify added Weekly KSv2 and removed Monthly KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors labels Jun 6, 2024
@mallenexpensify
Copy link
Contributor

@mkhutornyi is back! can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~019112f681ac31089d

@alexpensify , please finish off payment once it's ready.

@mallenexpensify mallenexpensify added Daily KSv2 and removed Weekly KSv2 labels Jun 6, 2024
@alexpensify
Copy link
Contributor

Awesome, thanks for the update!

@mkhutornyi
Copy link
Contributor

Accepted offer. Thanks

@alexpensify
Copy link
Contributor

Done, I've completed the payment process via Upwork and closed the jobs there. Welcome back @mkhutornyi!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Archived in project
Development

No branches or pull requests

10 participants