[HOLD for payment 2023-07-14] [$1000] Web - UserB can still create a task in room chat when UserA changes post permission to "Admins only"

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Login to UserA account
2. Navigates to the "New room" section
3. Creates a new room by entering a room name and select UserA's workspace
4. Set the visibility of the room to "Public"
5. Clicks on "Create room"
6. Go to the room details
7. Select "Share code" and copy the URL to the clipboard
8. Returns to the previous page and clicks on "Settings" > "Who can post"
9. In another browser, login to UserB account
10. Joins the room by pasting the copied URL
11. Go to the "Assign task" section in the room chat
12. Change the "Who can post" setting to "Admins only" from UserA account
13. Now you can create a task in the room chat

Expected Result:

When UserA changes the post permission to "Admins only," UserB should not be able to create a task in the room chat

Actual Result:

Even after UserA changes the post permission to "Admins only," UserB can still create a task in the room chat

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.32-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
Notes/Photos/Videos: Any additional supporting documentation

screen-recording-2023-06-26-at-122853-am_vxzqn8ht.1.mp4

Recording.931.mp4

Expensify/Expensify Issue URL:
Issue reported by: @ayazhussain79
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1687722646703389

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01fc548bc92bd6afb3
• Upwork Job ID: 1674304197984038912
• Last Price Increase: 2023-06-29

[dukenv0307]

Proposal

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

UserB can still create a task in room chat when UserA changes post permission to "Admins only"

What is the root cause of that problem?

We don't check to disable auto-set shareSomeWhere in NewTaskPage if the permission of parentReport is changed

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

1. We should add a check here to not set shareSomeWhere value if the permission of parentReport is changed by using ReportUtils.isAllowedToComment function

if (props.task.parentReportID && ReportUtils.isAllowedToComment(lodashGet(props.reports, `report_${props.task.parentReportID}`, {}))) {

App/src/pages/tasks/NewTaskPage.js

Line 98 in 09a7432

if (props.task.parentReportID) {

2. We should add a check here to show an error if shareSomeWhere is already set before the permission of parentReport is changed

if (!props.task.shareDestination || !ReportUtils.isAllowedToComment(lodashGet(props.reports, `report_${props.task.shareDestination}`, {})))

App/src/pages/tasks/NewTaskPage.js

Line 124 in 09a7432

if (!props.task.shareDestination) {

3. I think we also should exclude the report which has permission is admin only in TaskShareDestinationSelectorModal. To do this we could filter props.reports to exclude the report that has permission is admin only and then pass this filter reports into getShareDestinationOptions function instead of props.reports

const reportExcludeAdminOnly = useMemo(() => {
    let reportFilter = {}
    _.keys(props.reports).forEach((reportKey) => {
        if (ReportUtils.isAllowedToComment(props.reports[reportKey])) {
            reportFilter[reportKey] = props.reports[reportKey];
        }
    })
    return reportFilter;
}, [props.reports])
useEffect(() => {
  const {recentReports, personalDetails, userToInvite} = OptionsListUtils.getShareDestinationOptions(
      reportExcludeAdminOnly,
      ...

App/src/pages/tasks/TaskShareDestinationSelectorModal.js

Line 59 in b2526e5

props.reports,

What alternative solutions did you explore? (Optional)

We also could display not found page specifying user cannot access to assign task by adding FullPageNotFoundView in NewTaskPage with shouldShow prop is !ReportUtils.isAllowedToComment(lodashGet(props.reports, report_${props.task.shareDestination}, {}))

[melvin-bot]

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

[kadiealexander]

Repro'd:

2023-06-29_18-29-09.mp4

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[sobitneupane]

@dukenv0307 Thanks for your proposal.

Can we show a page similar to Not Found Page specifying user don't have access to assign task? It will inform user that he/she no longer have access.

[dukenv0307]

@sobitneupane Yes. We also use the check that mentioned in my proposal to display not found page.

[sobitneupane]

@dukenv0307 Can you please add an alternative solution in your proposal for #21791 (comment)

[dukenv0307]

@sobitneupane Updated proposal

[sobitneupane]

Alternative Solution from @dukenv0307's proposal looks good to me.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[dukenv0307]

I think we also should exclude the report which has permission is admin only in TaskShareDestinationSelectorModal. To do this we could filter props.reports to exclude the report that has permission is admin only and then pass this filter reports into getShareDestinationOptions function instead of props.reports

@sobitneupane Do you think we also should exclude the report in TaskShareDestinationSelectorModal

[sobitneupane]

Oh yes. That might be necessary when user tries to create task from LHN.

[melvin-bot]

❌ There was an error making the offer to sobitneupane. The BZ member will need to manually hire the contributor. cc @thienlnam

[melvin-bot]

❌ There was an error making the offer to dukenv0307. The BZ member will need to manually hire the contributor. cc @thienlnam

[melvin-bot]

📣 @ayazhussain79 You have been assigned to this job!
Please apply to this job in Upwork here and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

[cristipaval]

I think we should block the task creation in this case on the backend side as well 🤔

[melvin-bot]

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

[kadiealexander]

Thanks for the help Flavia!

[ayazhussain79]

@kadiealexander offer accepted, Thank you

[flaviadefaria]

@sobitneupane can you please answer the information here? Thanks!

[flaviadefaria]

Payment issued for @dukenv0307 and @ayazhussain79.

[ayazhussain79]

Thank you

[flaviadefaria]

@sobitneupane friendy bump here so that we can close this GH.

[sobitneupane]

Sorry for the delay @flaviadefaria. I will complete the checklist by tomorrow.

[flaviadefaria]

@sobitneupane are you able to complete this today?

[sobitneupane]

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:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

#17992

• [@sobitneupane] 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:

#17992 (comment)

• [@sobitneupane] 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:

It is an edge case. So, I don't think this could have been caught earlier in PR review.

• [@sobitneupane] Determine if we should create a regression test for this bug.

Yes.

• [@sobitneupane] 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.

[sobitneupane]

Regression Test Proposal:

1. Login with user A
2. On another device, login with user B
3. From user B, create a workspace and invite user A to the workspcae
4. From user A, go to announce chat of this workspace
5. Click on plus icon > Assign task
6. Enter title, description and go to next step
7. From user B, go to announce chat of this workspace
8. Change the setting of "Who can post" to "Admins only"
9. From user A, verify that not found page appears, and after clicking on the back button, the modal is dismissed

Do we agree 👍 or 👎

[sobitneupane]

Requested payment on newDot.

[flaviadefaria]

Cool once Anu pays you we can close this.

[flaviadefaria]

Requested payment on newDot.

@anmurali, just for your control this should have been $1500.

[JmillsExpensify]

Reviewed details for @sobitneupane. This is accurate based on summary from Business Reviewer and approved for payment in NewDot.

[flaviadefaria]

Great so closing this.