[HOLD for payment 2024-01-11] [$500] Task - Task assignee list opens with deep link when user has no access to edit task

kbecciv commented 9 months 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: 1.4.14.0 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: Applause - Internal Team Slack conversation:

Action Performed:

Precondition:

User is a member of the #announce workspace room in which a task is created and assigned to other user.
1. Log in as member of the workspace.
2. Go to #announce room.
3. Click on the task.
4. Add /assignee to the end of the link.
5. Select a different assignee.

Expected Result:

Not here page opens.

Actual Result:

Task assignee list opens.

Workaround:

Unknown

Platforms:

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

[ ] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/032635f8-42b0-445b-9c4f-bb89e19327b9

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01c0cdfb5367dcebe3
Upwork Job ID: 1737132419534086144
Last Price Increase: 2023-12-19
Automatic offers:

dukenv0307 | Contributor | 28067679
c3024 | Contributor | 28072736

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

bernhardoj commented 9 months ago

Proposal

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

We can access task assignee page even though we don't have access to.

What is the root cause of that problem?

We already have a condition here to disable it if the user doesn't have the permission (canModifyTask). https://github.com/Expensify/App/blob/b3ecf327c585ca323a628405f4ffc70877c601a0/src/pages/tasks/TaskAssigneeSelectorModal.js#L208-L209

But the problem here is that the rootParentReportPolicy is always empty. If we look at how we get that data, we can see that it depends on report onyx data, but we never have the report data, we only subscribe to reports (the whole collection) data. https://github.com/Expensify/App/blob/b3ecf327c585ca323a628405f4ffc70877c601a0/src/pages/tasks/TaskAssigneeSelectorModal.js#L249-L252 https://github.com/Expensify/App/blob/b3ecf327c585ca323a628405f4ffc70877c601a0/src/pages/tasks/TaskAssigneeSelectorModal.js#L262-L268

So, report is always empty.

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

Add a new Onyx subscription to get the current report, just like we do in other places. (put it before rootParentReportPolicy)

report: {
    key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${lodashGet(route, 'params.reportID')}`,
},

With this change, we can now remove this code and replace all of its usage with the report from the props. https://github.com/Expensify/App/blob/b3ecf327c585ca323a628405f4ffc70877c601a0/src/pages/tasks/TaskAssigneeSelectorModal.js#L129-L134

dukenv0307 commented 9 months ago

Proposal

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

Task assignee list opens.

What is the root cause of that problem?

In here, we're relying on the report in order to get the rootParentReportPolicy, however, on that page we're not getting the report, we get the full reports collection as can be seen here. This is because this selector modal is used in both create and edit case, when used in edit case, it has the reportID, when used in create case, it does not.

This leads to the report here to always be empty, the rootParentReportPolicy will be empty and the canModifyTask here is true, which is incorrect. This causes the user to be able to access that page.

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

In here, we should use the reports instead, and get the report via the route.params.reportID similar to here to use to get the rootParentReportPolicy.

I believe it's intentional to not connect to the report key in Onyx since the data is already available in reports and we don't want extra (double) rerendering of the assignee page due to that redundant key.

Optionally, since both this change and this existing code are getting the report via reportID and reports, we can modify this util to accept a second reports param (which defaults to the allReports) and use it in both places for DRY.

What alternative solutions did you explore? (Optional)

NA

yh-0218 commented 9 months ago

Proposal

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

Task - Task assignee list opens with deep link when user has no access to edit task

What is the root cause of that problem?

We did get report as props of parent on TaskAssigneeSelectorModal so props.rootParentReportPolicy is empty on TaskAssigneeSelectorModal

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

We can passed from reportID from param.

rootParentReportPolicy: {
            key: ({route}) => {
                const report = ReportUtils.getReport(route.params.reportID)
                const rootParentReport = ReportUtils.getRootParentReport(report);
                return `${ONYXKEYS.COLLECTION.POLICY}${rootParentReport ? rootParentReport.policyID : '0'}`;
            },
            selector: (policy) => _.pick(policy, ['role']),
},

What alternative solutions did you explore? (Optional)

narefyev91 commented 9 months ago

Proposal from @dukenv0307 looks good to me https://github.com/Expensify/App/issues/33296#issuecomment-1863051968 In case that we already have all reports at this page - no needs to have additional subscription to specific report from onyx Let's just work with reports and reportId 🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 9 months ago

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

bernhardoj commented 9 months ago

@narefyev91 I think having a different way to get the report object (to optimize it) is something that can be discussed on the PR stage itself.

dukenv0307 commented 9 months ago

@narefyev91 I think having a different way to get the report object (to optimize it) is something that can be discussed on the PR stage itself.

I think this issue is straightforward so an optimized solution that doesn't degrade performance should be preferred before we get to the PR, where we might or might not realize the issue and could have the bad code merged.

lakchote commented 9 months ago

@dukenv0307's proposal LGTM 👍

melvin-bot[bot] commented 9 months ago

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

c3024 commented 9 months ago

Taking this over.

dukenv0307 commented 8 months ago

@narefyev91 this PR is ready for review.

melvin-bot[bot] commented 8 months ago

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

garrettmknight commented 8 months ago

@c3024 you're up!

c3024 commented 8 months ago

PR review done. Awaiting final approval for merge by @lakchote

melvin-bot[bot] commented 8 months ago

Triggered auto assignment to @robertjchen (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

garrettmknight commented 8 months ago

Reassigning since @lakchote is out for parental leave.

robertjchen commented 8 months ago

Merged!

melvin-bot[bot] commented 8 months ago

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

melvin-bot[bot] commented 8 months ago

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

https://github.com/Expensify/App/pull/33564

If no regressions arise, payment will be issued on 2024-01-11. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

[ ] External issue reporter
[ ] Contributor that fixed the issue
[ ] Contributor+ that helped on the issue and/or PR

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

@c3024 requires payment automatic offer (Contributor)
@dukenv0307 requires payment automatic offer (Contributor)

melvin-bot[bot] commented 8 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:

[ ] [@c3024] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@c3024] 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:
[ ] [@c3024] 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:
[ ] [@c3024] Determine if we should create a regression test for this bug.
[ ] [@c3024] 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.
[ ] [@garrettmknight] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

c3024 commented 8 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:

[x] [@c3024] The PR that introduced the bug has been identified. Link to the PR: #32165
[x] [@c3024] 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: https://github.com/Expensify/App/commit/de9e6d4be3a367d9947ba2e2167e74e6669a5df0#r136920736
[x] [@c3024] 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: NA
[x] [@c3024] Determine if we should create a regression test for this bug. Yes
[x] [@c3024] 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.

c3024 commented 8 months ago

Regression test proposal

In the #announce room of a workspace W, create a task as User A and assign it to User B
Log in as User C (who is also a member of the workspace W)
Go to #announce room of the workspace W
Click on the task created at step 1
Add /assignee to the end of the URL and visit the URL
Verify that "Not here" page is displayed.

👍 or 👎

garrettmknight commented 8 months ago

Everybody's paid and the QA issue is up. Closing!

Expensify / App