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

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6319789_1702998210569.20231219_191222.mp4

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]

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

[melvin-bot]

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

[melvin-bot]

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]

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

[bernhardoj]

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).

App/src/pages/tasks/TaskAssigneeSelectorModal.js

Lines 208 to 209 in b3ecf32

	const canModifyTask = Task.canModifyTask(report, props.currentUserPersonalDetails.accountID, lodashGet(props.rootParentReportPolicy, 'role', ''));
	const isTaskNonEditable = ReportUtils.isTaskReport(report) && (!canModifyTask || !isOpen);

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.

App/src/pages/tasks/TaskAssigneeSelectorModal.js

Lines 249 to 252 in b3ecf32

	withOnyx({
	reports: {
	key: ONYXKEYS.COLLECTION.REPORT,
	},

App/src/pages/tasks/TaskAssigneeSelectorModal.js

Lines 262 to 268 in b3ecf32

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

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.

App/src/pages/tasks/TaskAssigneeSelectorModal.js

Lines 129 to 134 in b3ecf32

	const report = useMemo(() => {
	if (!props.route.params || !props.route.params.reportID) {
	return null;
	}
	return props.reports[`${ONYXKEYS.COLLECTION.REPORT}${props.route.params.reportID}`];
	}, [props.reports, props.route.params]);

[dukenv0307]

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]

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]

Proposal from @dukenv0307 looks good to me #33296 (comment)
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]

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

[bernhardoj]

@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]

@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]

@dukenv0307's proposal LGTM 👍

[melvin-bot]

📣 @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]

Taking this over.

[dukenv0307]

@narefyev91 this PR is ready for review.

[melvin-bot]

📣 @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]

@c3024 you're up!

[c3024]

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

[melvin-bot]

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

[garrettmknight]

Reassigning since @lakchote is out for parental leave.

[robertjchen]

Merged!

[melvin-bot]

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

[melvin-bot]

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

• Task assignee list opens with deep link when user has no access to edit task #33564

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

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]

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]

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: Fix/31863: Don't allow member to edit task in room #32165
• [@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: de9e6d4#r136920736
• [@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
• [@c3024] Determine if we should create a regression test for this bug. Yes
• [@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]

Regression test proposal

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

👍 or 👎

[garrettmknight]

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