[HOLD for payment 2023-09-21] [$1000] App allows task title update for completed task using deep link and throws error on update

kavimuru commented 12 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!

Action Performed:

Open the app
Click on plus and click on 'Assign a task'
Write any title and assign task to anyone
In task report, click on task to open task title edit
Copy the URL and send it to any other report
Mark the task as done
Click on link sent in step 5, edit the title and click on save
Observe that task report in LHN now has red dot for error
Expected Result:

Once task is completed, app should not allow to edit task title by deep link

Actual Result:

App allows to edit task title using deep link even after task is marked as completed and if edited using the link, app displays red dot in LHN on task report

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

[x] Android / native
[x] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[x] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: 1.3.38-3 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

https://github.com/Expensify/App/assets/43996225/54d911ae-54de-4062-8083-3be2cb7aa2b2

https://github.com/Expensify/App/assets/43996225/e299c4b5-9b41-4c50-bffa-47e19f426929

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1688742180722969

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01df4c0b6543fe49a8
Upwork Job ID: 1678539598526873600
Last Price Increase: 2023-08-28
Automatic offers:

BhuvaneshPatil | Contributor | 26456675
dhanashree-sawant | Reporter | 26456679

melvin-bot[bot] commented 12 months ago

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

melvin-bot[bot] commented 12 months ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] 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
[x] 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.
[x] 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.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

hoangzinh commented 12 months ago

Proposal

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

App allows task title update for completed task using deep link and throws error on update

What is the root cause of that problem?

In the TaskTitlePage component, we haven't have logic to check whether the task is complete/cancelled before showing the page.

This issue is also reproducible in edit Task description, edit Task assignee and edit Task share destination pages.

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

We should add logic into the TaskTitlePage component so that if the Task is not open (means is completed or cancelled), then we can either navigate back or show not show page. We can put those logic into an HOC/hook to reuse it in other 3 pages above. I imagine it almost same with this existing HOC withReportOrNotFound

BhuvaneshPatil commented 12 months ago

Proposal

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

App allows task title update for completed task using deep link and throws error on update

What is the root cause of that problem?

We are not having any check (whether task is open) when we are opening TaskTitlePage using link. That's causing this issue. Same error is happening with TaskDesscriptionPage

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

We can wrap the TaskTitlePage with FullPageNotFoundView component. For shouldShow prop, we can add the required checks -

If the report/task is open or not

For this we are using this method -

https://github.com/Expensify/App/blob/6a5b1a4dba08b6850a24ca75347dfa2e6cbfd342/src/components/ReportActionItem/TaskView.js#L50

This can be used in shouldShow prop for FullPageNotFoundView.

This has advantage over withReportOrNotFound HOC. We can add custom message to show in two scenarios described above. We can pass custom subtitleKey ( that shows why page can't be viewed, example - Can't edit the Completed Task) to FullPageNotFoundView. It will make the end user understand why description is not changeable

We can also use early return in case the Task is completed and render nothing at all, with this approach we are not being verbose to user that why we are not showing the page.

UPDATE🚨 Many proposals are here mentioning about access check for user while opening these pages - It was supposed to be handle in this issue - https://github.com/Expensify/App/issues/21580. But that was closed in favour of this issue (the changes will be added to this issue) and proposal(that was selected) is - https://github.com/Expensify/App/issues/21580#issuecomment-1607810113

What alternative solutions did you explore? (Optional)

If we don't want to wrap the FullPageNotFoundView, we can use withReportOrNotFound here we can add additional condition to check if report is open or not.

report.stateNum === CONST.REPORT.STATE_NUM.OPEN && report.statusNum === CONST.REPORT.STATUS.OPEN;

By using this solution we can't provide detailed info to user why he/she can't access the page.

twisterdotcom commented 12 months ago

Wow, good find.

melvin-bot[bot] commented 12 months ago

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

melvin-bot[bot] commented 12 months ago

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

melvin-bot[bot] commented 12 months ago

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

Nodebrute commented 11 months ago

Proposal

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

Web - User able to add or edit assignee even if task was completed

What is the root cause of that problem?

We are not adding any check on who can edit assignee.

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

First we need to add check if isCompletedTaskReport but what is task is not completed yet? User shouldn't be able to access that page if Task is completed we have few other cases here too. We can use Task.canModifyTask to stop users who aren't allowed to edit the task Additionally we can also check for isCancelledTaskreport.

Task is not completed yet and users who are not allowed to edit assignee can access that page and can edit Task. Only 3 users are allowed to edit task. Task Assignee Task Assigner Policy Admin

We need to add other checks here too

!Task.isTaskAssigneeOrTaskOwner ---To check if current user is task assignee or task owner if not then user will not be able to access that page. !PolicyUtils.isPolicyAdmin----To check if current user is policy admin if not then user will not be able to access that page ReportUtils.isCompletedTaskReport---To check if task is completed then no one should be able to access that page.

After adding these checks we can either dismiss modal, show hmmm it's not here page or return early from editTaskAndNavigate.

What alternative solutions did you explore? (Optional)

We can do this for title and description pages too.

dukenv0307 commented 11 months ago

Proposal

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

user able to add or edit assignee even if task was completed

What is the root cause of that problem?

We don't have any checks to dismiss task assignee modal or do nothing in editTaskAndNavigate if the task is completed.

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

After we change the permission, we can display not found page for TaskTitlePage, TaskDescriptionPage, and TaskAssigneeSelectorModal if we are go to the task pages in editing mode and the task is completed or the user cannot modify the task or the task is canceled

We can create a function to check if the task can edited or not and then use this to display not found page
For TaskAssigneeSelectorModal page, because the page is also used when we create a new task, so we should also check if we are open the page in a task report or not !_.isEmpty(report) && (ReportUtils.isCompletedTaskReport || ReportUtils.isCanceledTaskReport || !Task.canModifyTask)
For TaskDescriptionPage and TaskTitlePage pages, we can simply add the check (ReportUtils.isCompletedTaskReport || ReportUtils.isCanceledTaskReport || !Task.canModifyTask) to display not found page

Because we display not found page for these page, we also should subscribe isLoadingReportData to display loading page if the data is loading when we open these page by deeplink before login to preventing not found page display briefly after logging or use withReportorNotFound HOC or create new HOC for task report to take care this.

What alternative solutions did you explore? (Optional)

We also can add the check ReportUtils.isCompletedTaskReport || !Task.canModifyTaskeditTaskAndNavigate` here to do nothing if the task is completed

if (ReportUtils.isCompletedTaskReport || !Task.canModifyTask) {
    return;
}

https://github.com/Expensify/App/blob/38890e2e3c32b52dad11a7dfc09494924519946b/src/libs/actions/Task.js#L308

twisterdotcom commented 11 months ago

@eVoloshchak - any of these proposals look good?

DanutGavrus commented 11 months ago

Proposal

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

App allows task title update for completed task using deep link and throws error on update

What is the root cause of that problem?

We allow the Sub-Report deep link routes to be handled by their respective component pages, which means that users will be able to access pages which they shouldn't: https://github.com/Expensify/App/blob/2d05d2e77fcac41767ec0946a00911be1a0ad4f1/src/libs/ReportUtils.js#L2292-L2300

While coming from a deeplink, or manually entering the link in the search bar and press enter, the following happens: Report.openReportFromDeepLink(url, isAuthenticated) triggers -> const reportID = ReportUtils.getReportIDFromLink(url); triggers -> return ''; triggers which lets the openReportFromDeepLink continue with no reportID -> thus giving users access to those sub-pages.

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

I think we should prevent users from navigating to pages which they shouldn't be allowed to(such as navigating to /login while already logged in - a general example). To do that, we should:

Modify getReportIDFromLink as to return {reportID, isSubReportPageRoute} without setting it to '' before;

Update openReportFromDeepLink as follows:

let route = ReportUtils.getRouteFromLink(url);
let { reportID, isSubReportPageRoute } = ReportUtils.getReportIDFromLink(url);
if (isSubReportPageRoute) {
// Prevent Sub-Report deep link to routes with no access
const report = ReportUtils.getReport(reportID);
if (ReportUtils.isCompletedTaskReport(report) && route.endsWith('/assignee')) {
    route = route.slice(0, -9);
} else {
    // We allow the Sub-Report deep link to routes with access to be handled by their respective component pages
    reportID = ''
}
}

Previous code is a draft, we should add more conditions in our if for access verification, add more sub-routes, replace the hard-coded sub-route string with consts, -9 with those consts' lengths, etc.
This is the result:

https://github.com/Expensify/App/assets/56603839/35561a01-42ba-4670-9511-3efdc40df2a2

twisterdotcom commented 11 months ago

Bump @eVoloshchak. I'll reassign to another C+ tomorrow if you're a bit busy, don't worry.

melvin-bot[bot] commented 11 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

eVoloshchak commented 11 months ago

Thank you for the proposals, everyone! The first four proposals are quite similar (they can't be that different, since all we need to do is conditionally display an error)

I think we should proceed with @Nodebrute's proposal, it is adding an important check if user has permission to edit/view the task title/description in addition to checking if task is completed.

We can use FullPageNotFoundView, there seem to be two cases:

The task is completed -> show FullPageNotFoundView with something like 'Can't edit task when it's completed'
User isn't Task Assignee, Task Assigner or Policy Admin (can't see or edit the task) -> show FullPageNotFoundView with 'You can't access this task'

🎀👀🎀 C+ reviewed!

UPD: there's already an issue User able to add or edit assignee even if task was completed, we would be resolving that too with this proposal

melvin-bot[bot] commented 11 months ago

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

BhuvaneshPatil commented 11 months ago

@eVoloshchak @twisterdotcom

https://github.com/Expensify/App/issues/21580 - This is issue that is being worked on right now. Please have a look. https://github.com/Expensify/App/issues/21579

Nodebrute commented 11 months ago

@eVoloshchak @joelbettner @twisterdotcom they are handling it from backend right now. It is marked as internal and no previous proposal has mentioned an effective way to handle it in frontend. We can deal with frontend in this issue.

twisterdotcom commented 11 months ago

Just waiting on your input @joelbettner!

melvin-bot[bot] commented 11 months ago

@eVoloshchak @twisterdotcom @joelbettner this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 11 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

twisterdotcom commented 11 months ago

@joelbettner bump on this please.

BhuvaneshPatil commented 11 months ago

@Nodebrute Have a look at this comment - https://github.com/Expensify/App/issues/21580#issuecomment-1614907480

It was approved then moved to internal. And the proposal mentioned about adding the FullPageNotFoundView if user other than - PolicyAdmin, Assignee, Creator tries to access using link.

@eVoloshchak this bug involves check if task is completed or not, and the additional work of blocking the view for users was discussed in separate issue (linked above), please have a look on that as well.

cc - @abdulrahuman5196

alitoshmatov commented 11 months ago

Proposal

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

App allows task title update for completed task using deep link and throws error on update

What is the root cause of that problem?

We have following routes:

We are not checking if user has access to this pages and can edit the task. Also when we manually enter this routes in non-task reports this pages show up and changes effect the report(at least locally)

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

So we should add FullPageNotFoundView with following conditions: Check if report is task report !ReportUtils.isTaskReport(props.report) Check if report is completed ReportUtils.isCompletedTaskReport(props.report) Check if user has permission to edit task Task.isTaskAssigneeOrTaskOwner(props.report, props.session.accountID), PolicyUtils.isPolicyAdmin(policy)

In following places:

Task title page https://github.com/Expensify/App/blob/a32fa95627b8a5a9a093c8e05e79dd2facf9f8a9/src/pages/tasks/TaskTitlePage.js#L69-L70
Task description page, we should also add since it doesn't have access to the current report
```
report: {
        key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${getReportID(route)}`,
    },
```
https://github.com/Expensify/App/blob/a32fa95627b8a5a9a093c8e05e79dd2facf9f8a9/src/pages/tasks/TaskDescriptionPage.js#L58-L59
Task assignee is a bit different condition should be props.report && !ReportUtils.isTaskReport(props.report) since it can be opened for new tasks and props.report might be empty https://github.com/Expensify/App/blob/a656c82d0ebf1d3954575084555a004efc47035b/src/pages/tasks/TaskAssigneeSelectorModal.js#L183 Also we should add
```
report: {
        key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${getReportID(route)}`,
    },
```

What alternative solutions did you explore? (Optional)

alitoshmatov commented 11 months ago

Coming from here https://github.com/Expensify/App/issues/23473 where we have similar problem with report pages.

This issue is also present for non task reports:

We are not checking if user has access to this pages and can edit the task. Also when we manually enter this routes in non-task reports this pages show up and changes effect the report(at least locally)

That's why posted proposal with solution to that also

DanutGavrus commented 11 months ago

@joelbettner With this implementation we can easily create rule sets which prevent user accessing sub-pages he shouldn't. We'll have them in just 1 place for the whole app instead of fixing each deeplink in its own component one by one. Seems to also make it easy to prevent/fix future similar issues.

cc: @abdulrahuman5196 @thienlnam @stitesExpensify I'm curious to hear your thoughts about this approach as it seems different from other proposals. Maybe it's a good approach, maybe not. 🤔

joelbettner commented 11 months ago

@eVoloshchak @joelbettner @twisterdotcom they are handling it from backend right now. It is marked as internal and no previous proposal has mentioned an effective way to handle it in frontend. We can deal with frontend in this issue.

Can you link to the issue in which the backend checks are being worked on? Are the backend changes complete? Will the front end changes be affected by whatever is written for the backend?

If the backend changes are complete and we are ready to start working on the front end changes, @eVoloshchak I'm not entirely sure which proposal I'm supposed to be reviewing since several other proposals were recently submitted.

Let me know.

Nodebrute commented 11 months ago

@joelbettner C+ approved comment https://github.com/Expensify/App/issues/22451#issuecomment-1638973855. Here, we want to show the FullPageNotFoundView, and I don't think this frontend change will affect the backend.

BhuvaneshPatil commented 11 months ago

This is the issue that I've reported - https://github.com/Expensify/App/issues/21580.

For above issue, proposal was accepted by @abdulrahuman5196 there are lot of comments in that issue thread on approach to follow. And the time when issue was reported and proposal was accepted we didn't have the TaskView that we have now.

@thienlnam decided to work on this on backend. And issue was marked as internal.

The reason I added this comment is because while accepting proposal one of the reason was the accepted proposal has mentioned about adding other checks, and they were discussed in other issue way before and put on hold. So it would be fair to judge the proposals on the RCA (i.e. not having checks for task is completed or not). And if we want to add the user access check then we shall consider the comments on dedicated issue.

I have added the comment on https://github.com/Expensify/App/issues/21580 that we shall wrap the components (that change title, description and assignee) with FullPageNotFoundView because many contributors are suggesting the same.

melvin-bot[bot] commented 11 months ago

@eVoloshchak @twisterdotcom @joelbettner this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 11 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 11 months ago

@eVoloshchak, @twisterdotcom, @joelbettner Whoops! This issue is 2 days overdue. Let's get this updated quick!

joelbettner commented 11 months ago

I don't think this frontend change will affect the backend./

As long as that is the case then I'm good with the proposal @eVoloshchak.

BhuvaneshPatil commented 11 months ago

This is the issue that I've reported - #21580.

For above issue, proposal was accepted by @abdulrahuman5196 there are lot of comments in that issue thread on approach to follow. And the time when issue was reported and proposal was accepted we didn't have the TaskView that we have now.

@thienlnam decided to work on this on backend. And issue was marked as internal.

The reason I added this comment is because while accepting proposal one of the reason was the accepted proposal has mentioned about adding other checks, and they were discussed in other issue way before and put on hold. So it would be fair to judge the proposals on the RCA (i.e. not having checks for task is completed or not). And if we want to add the user access check then we shall consider the comments on dedicated issue.

I have added the comment on #21580 that we shall wrap the components (that change title, description and assignee) with FullPageNotFoundView because many contributors are suggesting the same.

@joelbettner @eVoloshchak please consider this comment as well, and share your view on the same.

thienlnam commented 11 months ago

Adding the same comment as over here, but let's wait until we update the permissions before moving forward with this change

twisterdotcom commented 11 months ago

Still held.

twisterdotcom commented 10 months ago

@thienlnam can we unhold this now?

thienlnam commented 10 months ago

Yup, this is ready to go

Nodebrute commented 10 months ago

@joelbettner @eVoloshchak we can start working on this issue

dukenv0307 commented 10 months ago

Updated proposal after the update permission.

Nodebrute commented 10 months ago

@eVoloshchak I've already updated the proposal 36 minutes ago.

melvin-bot[bot] commented 10 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

twisterdotcom commented 10 months ago

Apologies but I am finally going on Parental Leave. I am reassigning to another member of the team.

melvin-bot[bot] commented 10 months ago

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

BhuvaneshPatil commented 10 months ago

best wishes @twisterdotcom

melvin-bot[bot] commented 10 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

eVoloshchak commented 10 months ago

~~The expected behavior didn't change, this review still stands, I think we can proceed with @Nodebrute's proposal~~

cc: @joelbettner

BhuvaneshPatil commented 10 months ago

@eVoloshchak Please have a look at this comment https://github.com/Expensify/App/issues/22451#issuecomment-1654671183. And the access check are extra work that is not needed. Because now anyone who can view the task can modify it. So please consider the base changes(to check if task is completed or not) while selecting the proposal.

dukenv0307 commented 10 months ago

@eVoloshchak Please help to check again

Permission of the task is changed. Now everyone who can access the task report can also edit the task report
When we display not found page, we should also display loading page if the data is loading.

eVoloshchak commented 10 months ago

@BhuvaneshPatil, oh, I see, thank you for the explanation on this! It became a bit convoluted with multiple issues dedicated to the same flow.

While this issue was created for a specific case (accessing a task that is completed), we should cover all of the other cases where user doesn't have access to this page. This was first proposed by @BhuvaneshPatil in https://github.com/Expensify/App/issues/21580#issuecomment-1607810113, but that issue was closed (in favor of this one). I think it's fair to review proposals from both this issue and https://github.com/Expensify/App/issues/21580.

This proposal from @BhuvaneshPatil was posted before this issue was even created, so I think we should proceed with that, since this issue was extended to implement all of the changes

cc: @joelbettner

Expensify / App