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 2023-07-10] [$1000] Users are able to flag their own comment #21212

Closed
6 tasks done
kbecciv opened this issue Jun 21, 2023 · 55 comments
Closed
6 tasks done
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

@kbecciv
Copy link

kbecciv commented Jun 21, 2023

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 with 2 account (A and B)
  2. Create a group chat between them, send some messages from account A.
  3. From account B, right click and choose Flag as offensive, copy the link and paste to the browser that account A is logging in.
  4. The Flag As Offensive page opened and you can flag your own comment.

Expected Result:

Users shouldn't be able to flag their own message

Actual Result:

Users are able to flag their own comment.

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.27-6

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-14.at.18.22.57.3.mov
Screen.Recording.2023-06-14.at.18.22.57.2.mov

Expensify/Expensify Issue URL:

Issue reported by: @hungvu193

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686742370457539

Recording.802.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012f9f19f3eff4c785
  • Upwork Job ID: 1671569938552557568
  • Last Price Increase: 2023-06-21
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 21, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 21, 2023

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

@kbecciv
Copy link
Author

kbecciv commented Jun 21, 2023

Proposal

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

Users are able to flag their own comment.

What is the root cause of that problem?

We are not checking if current reportAction is your own comment inside FlagCommentPage.

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

We should check if current reportAction is your own reportAction then we can decide the next step (it's depend on expected behavior). We can show not found view or set severityMenuItems to empty and leave a placeholder or dismiss the modal.

    const [shouldShowBlockingView, setShowBlockingView] = React.useState(false)
    React.useEffect(() => {
        let reportToCheck;
        // handle for thread
        if (reportAction === undefined) {
            reportToCheck = ReportActionsUtils.getParentReportAction(props.report);
        } else {
            reportToCheck = reportAction
        }

        if (props.session.email === reportToCheck.actorEmail) {
            setShowBlockingView(true); // or we can dismiss this modal
        }
    }, [reportAction, props.report]);

    if (shouldShowBlockingView) {
        return <NotFoundPage />
    }

What alternative solutions did you explore? (Optional)

N/A

@dukenv0307
Copy link
Contributor

dukenv0307 commented Jun 21, 2023

Proposal

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

Users are able to flag their own comment

What is the root cause of that problem?

We don't prevent the user access to the page to flag their comment by URL flag/:reportID/:reportActionID

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

In FlagCommentPage, We should use FullPageNotFoundView with conditions like here

shouldShow: (type, reportAction, isArchivedRoom, betas, menuTarget, isChronosReport, reportID) =>
type === CONTEXT_MENU_TYPES.REPORT_ACTION &&
ReportUtils.canFlagReportAction(reportAction, reportID) &&
!isArchivedRoom &&
!isChronosReport &&
!ReportUtils.isConciergeChatReport(reportID) &&
reportAction.actorEmail !== CONST.EMAIL.CONCIERGE,

Also should use isLoadingReportData props from ONYX to check if the app is loading

<ScreenWrapper includeSafeAreaPaddingBottom={false}>
            {({safeAreaPaddingBottomStyle}) => (
                <FullPageNotFoundView shouldShow={
                       !isLoadingReportData && 
                       ReportUtils.canFlagReportAction(reportAction, reportID) &&
                       !isArchivedRoom &&
                       !isChronosReport &&
                       !ReportUtils.isConciergeChatReport(reportID) &&
                       reportAction.actorEmail !== CONST.EMAIL.CONCIERGE
               }>
                    <HeaderWithBackButton title={props.translate('reportActionContextMenu.flagAsOffensive')} />
                    <ScrollView
                     ....
                </FullPageNotFoundView>
            )}
        </ScreenWrapper>

We also should move this code to outer of FlagComment Function to handle thread case

if (reportAction === undefined) {
reportID = ReportUtils.getParentReport(props.report).reportID;
reportAction = ReportActionsUtils.getParentReportAction(props.report);
}

As this comment, In case, one account has 2 contact method (A and B), If we decide that A can't flag the comment of B we can update canEditReportAction function (because as I suggest above, we also use this function in FlagCommentPage)

App/src/libs/ReportUtils.js

Lines 201 to 210 in 5f5dd02

function canEditReportAction(reportAction) {
return (
reportAction.actorEmail === sessionEmail &&
reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT &&
!isReportMessageAttachment(lodashGet(reportAction, ['message', 0], {})) &&
!ReportActionsUtils.isDeletedAction(reportAction) &&
!ReportActionsUtils.isCreatedTaskReportAction(reportAction) &&
reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE
);
}

We can use props.loginList instead of only sessionEmail to check

Result

Screenshot 2023-06-21 at 22 11 39

@abekkala abekkala added the External Added to denote the issue can be worked on by a contributor label Jun 21, 2023
@melvin-bot melvin-bot bot changed the title Users are able to flag their own comment [$1000] Users are able to flag their own comment Jun 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 21, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 21, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 21, 2023

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

@ygshbht
Copy link
Contributor

ygshbht commented Jun 21, 2023

Proposal

Hello Expensify Team,

I'm Yogesh, a seasoned web developer with a robust track record on Upwork. My profile, which can be viewed here: https://www.upwork.com/freelancers/~01ca41cc1d47038084, showcases my 100% job success rate and a slew of positive reviews from satisfied clients. For a comprehensive look at the diverse projects I've contributed to, feel free to visit my portfolio at yogeshbhatt.com.

On to the matter at hand: users being able to flag their own comments. My initial approach to resolve this would be to implement a backend check that verifies whether the commenter and the flagger are the same person. The outcome of this check could then be relayed to the frontend, where an appropriate error message could be displayed if necessary.

However, it's important to note that the exact solution can only be ascertained after a thorough examination of your existing codebase and its architecture. As such, while the approach I've outlined seems plausible based on the information currently at hand, I would need to delve deeper into your code to confirm its viability and ensure its seamless integration with your current setup.

As an experienced contributor, I've familiarized myself thoroughly with your Contributor Guidelines and README documentation. If granted the opportunity, I'm ready to uphold the highest standards of coding and conduct as part of your team.

I look forward to the possibility of working together and helping to further enhance the user experience on your platform.

Best regards,
Yogesh

@abekkala
Copy link
Contributor

@ygshbht can you review the contributor guidelines here and ensure your proposal is in the correct format.

Thanks!

@ygshbht
Copy link
Contributor

ygshbht commented Jun 22, 2023

@abekkala Thank you for pointing that out. As I'm relatively new here, your guidance is much appreciated

Proposal

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

The problem is that users are able to flag their own comments, which could lead to misuse and an inconsistent user experience.

What is the root cause of that problem?

The issue arises due to a lack of validation to prevent users from flagging their own comments.

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

A potential fix involves introducing a check in the front-end, if data is available, to validate whether the flagger and the commenter are the same user. If they match, flagging should be prevented. However, once I start working, alternative solutions might emerge, depending on where the root cause lies

@melvin-bot melvin-bot bot added the Overdue label Jun 26, 2023
@abekkala
Copy link
Contributor

abekkala commented Jun 26, 2023

@mananjadhav have you had time to review the proposals?

@melvin-bot melvin-bot bot removed the Overdue label Jun 26, 2023
@mananjadhav
Copy link
Collaborator

@hungvu193 @dukenv0307 The RCA is correct for both the proposals, but could you explain how will you check if the comment is by the logged in user? Also consider the secondary login aspect.

@ygshbht Some bits of technical details on what will be changed, where, how will you put the validation would be helpful to review the proposal.

@hungvu193
Copy link
Contributor

hungvu193 commented Jun 27, 2023

props.session.email === reportToCheck.actorEmail

@mananjadhav We can always get the actorEmail, I think we already had something similar here:

function canFlagReportAction(reportAction) {
    return (
        reportAction.actorEmail !== sessionEmail &&
        reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT &&
        !ReportActionsUtils.isDeletedAction(reportAction) &&
        !ReportActionsUtils.isCreatedTaskReportAction(reportAction)
    );
}

And you're right, we should update the condition to compare it the loginList instead of only current logged email. When I changed the contact methods, it will also change the actorEmail in reportAction.

Should be something like this:

if (_.keys(props.loginList).includes(reportAction.actorEmail) {
  // then decide to do something here
}

We can also update the canFlagReportAction function.

@dukenv0307
Copy link
Contributor

@mananjadhav Thanks for your reminder, updated proposal

@mananjadhav
Copy link
Collaborator

While both the proposals identified the correct root cause, I think @dukenv0307's proposal is good, because we're using FullPageNotFoundView.

While I don't think it's a good idea to use so many conditions in shouldShow, we could move this to a util.

🎀 👀 🎀 C+ reviewed.

@melvin-bot
Copy link

melvin-bot bot commented Jun 27, 2023

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 27, 2023

📣 @dukenv0307 You have been assigned to this job by @tylerkaraszewski!
Please apply to this job in Upwork 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 📖

@tylerkaraszewski
Copy link
Contributor

Giving this one to @dukenv0307

@melvin-bot
Copy link

melvin-bot bot commented Jul 10, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 10, 2023

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

@mananjadhav
Copy link
Collaborator

@flaviadefaria @CortneyOfstad The regression isn't from the PR. The offending PR for this issue is here.

I've posted a comment here. @tylerkaraszewski I don't think any action is required here and I also don't think we'll need any regression test here? wdyt?

@mananjadhav
Copy link
Collaborator

@CortneyOfstad I'll raise a payout request for this one NewDot.

@dangrous
Copy link
Contributor

Technically we should probably block this on the backend as well, although it's more of an annoyance than anything else. cc @thienlnam and I guess @tylerkaraszewski as the internal teammember on this issue

@mananjadhav
Copy link
Collaborator

Technically we should probably block this on the backend as well

@dangrous Agreed. Do we want to create another issue in the Web repo? or we'll track using this one?

@CortneyOfstad I've raised a request for my side. Can you please help with the payout for @dukenv0307 ?

@dangrous
Copy link
Contributor

Yeah I can make a separate issue!

@CortneyOfstad
Copy link
Contributor

CortneyOfstad commented Jul 13, 2023

@mananjadhav sounds good and to confirm, there is a $500 bonus as well? Just reviewing the contract in Upwork and wanted to confirm for @dukenv0307. TIA!

@anmurali
Copy link

Paid Manan after confirming here.

@CortneyOfstad
Copy link
Contributor

@mananjadhav bump on the comment above!

@mananjadhav
Copy link
Collaborator

Yes @CortneyOfstad there's a bonus as well.

@melvin-bot melvin-bot bot added the Overdue label Jul 17, 2023
@CortneyOfstad
Copy link
Contributor

Tackling payment now 👍

@melvin-bot melvin-bot bot removed the Overdue label Jul 18, 2023
@CortneyOfstad
Copy link
Contributor

@dukenv0307 sent the contract to you in UpWork — let me know as soon as you accept it and I can get that paid to you ASAP. Thanks for your patience, and sorry for the delay!

@dukenv0307
Copy link
Contributor

dukenv0307 commented Jul 18, 2023

@CortneyOfstad accepted, thank you!

@CortneyOfstad
Copy link
Contributor

Contract paid and completed! Closing!

@hungvu193
Copy link
Contributor

Contract paid and completed! Closing!

@CortneyOfstad you forgot me 🥲

@CortneyOfstad
Copy link
Contributor

So I'm a bit confused. @mananjadhav can you provide some context?

@CortneyOfstad CortneyOfstad reopened this Jul 18, 2023
@hungvu193
Copy link
Contributor

Ah I'm the reporter

@CortneyOfstad
Copy link
Contributor

OMG, I am so sorry! It's been a while since I've had to do this, so I'm a bit rusty! 😂

Paying you now!! So sorry!!

@hungvu193
Copy link
Contributor

OMG, I am so sorry! It's been a while since I've had to do this, so I'm a bit rusty! 😂

Paying you now!! So sorry!!

No worries 🤣

@CortneyOfstad
Copy link
Contributor

All paid!

@mananjadhav
Copy link
Collaborator

Thanks @CortneyOfstad. Can we close this now?

@CortneyOfstad
Copy link
Contributor

Yep — closing now!

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
None yet
Development

No branches or pull requests