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] [$1000] Welcome Message Visibility Bug: User A Can See Welcome Message Intended for User B #20990

Closed
1 of 6 tasks
kavimuru opened this issue Jun 18, 2023 · 69 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Jun 18, 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. Go to web chrome and login with User A (mobile number as primary account) and create a Workspace
  2. Now create a room with the same workspace in public mode (everyone)
  3. Go to Room details > settings > welcome message > write message
  4. Go to chat and write the message
  5. Go to Room details >share code > copy code > send the code to User B
  6. Open the room with User B and you will be able to see the welcome message which says “only visible to You (User B)”
  7. Reply to the welcome message as User B
  8. Reply to the chat room as User B
  9. From User A, open the chat room and you will be able to see the welcome message intended for User B saying its 'ONLY visible to User B'
    Expected Result:
    Message intended for User B should NOT be visible to User A (or “message ONLY visible to User B should be removed”)
    Actual Result:
    Welcome message was visible to User A with a notation ‘only visible to User B’

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?

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

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

welcome.message.vulnerability.mp4
Recording.1014.mp4

Expensify/Expensify Issue URL:
Issue reported by: @avi-shek-jha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686355332887919

View all open jobs on GitHub

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

melvin-bot bot commented Jun 18, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 18, 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

@dukenv0307
Copy link
Contributor

Proposal

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

Welcome Message Visibility Bug: User A Can See Welcome Message Intended for User B

What is the root cause of that problem?

In ReportActionItem, we don't check if the message is whispered and only visible to a user, we will hide it from other users' side

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

We could add a check !isWhisper || isWhisperOnlyVisibleByUser here to hide the message item if the message only visible to a user

<PressableWithSecondaryInteraction

or add this check above here to only hide the content of message
{renderReportActionItem(hovered, isWhisper)}

What alternative solutions did you explore? (Optional)

@NicMendonca NicMendonca added the External Added to denote the issue can be worked on by a contributor label Jun 19, 2023
@melvin-bot melvin-bot bot changed the title Welcome Message Visibility Bug: User A Can See Welcome Message Intended for User B [$1000] Welcome Message Visibility Bug: User A Can See Welcome Message Intended for User B Jun 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 19, 2023

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

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

melvin-bot bot commented Jun 19, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 19, 2023

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

@melvin-bot melvin-bot bot added the Overdue label Jun 20, 2023
@NicMendonca
Copy link
Contributor

@aimane-chnaif bump on the above proposal ^^

@melvin-bot melvin-bot bot removed the Overdue label Jun 20, 2023
@wildan-m
Copy link
Contributor

Proposal

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

In my opinion, this is expected behavior if the user is a workspace owner, so the owner can re-visit the welcome message chat thread from room chat.

I didn't see the chat message from a third account that is not a workspace owner.

The actual issue here is the text Only visible to doesn't mention anyone

image

Please correct me if I'm mistaken.

What is the root cause of that problem?

I've noticed that props.personalDetails always show an empty object in ReportActionItem.

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

We can change it to props.personalDetailsList here

const whisperedToPersonalDetails = isWhisper ? _.filter(props.personalDetails, (details) => _.includes(whisperedTo, details.login)) : [];

like what we did here.

To make it make more sense, then we can add the workspace owner to the whisperedTo array, so the text will be Only visible to you and newuser@email.com it will require backend change to add the owner as whisperedTo item.

What alternative solutions did you explore? (Optional)

N/A

@NicMendonca NicMendonca removed the Bug Something is broken. Auto assigns a BugZero manager. label Jun 23, 2023
@melvin-bot melvin-bot bot added the Overdue label Jun 23, 2023
@NicMendonca NicMendonca removed their assignment Jun 23, 2023
@melvin-bot melvin-bot bot removed the Overdue label Jun 23, 2023
@NicMendonca NicMendonca added Overdue Bug Something is broken. Auto assigns a BugZero manager. labels Jun 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 23, 2023

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

@melvin-bot

This comment was marked as duplicate.

@NicMendonca
Copy link
Contributor

@conorpendergrast I am going OOO until Wednesday. Can you watch this while I am away? I'll unassign you when I am back. Thank you!

@NicMendonca NicMendonca self-assigned this Jun 23, 2023
@conorpendergrast
Copy link
Contributor

Of course! Toodles

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

Thanks for the discussion here! Given that this will be addressed in the #21822 we should close this out. Are there any other actions we need to take here?

@aimane-chnaif
Copy link
Contributor

@robertjchen I reviewed that PR. I think it's better to handle compensation here. And also @avi-shek-jha might be eligible for bug reporting bonus.

@pecanoro
Copy link
Contributor

pecanoro commented Jun 29, 2023

@robertjchen You need to leave it open until we merge the PR to pay the reporting bonus or any review bonus

@robertjchen
Copy link
Contributor

robertjchen commented Jun 29, 2023

Got it, thanks for the clarification! Since @pecanoro handled all the questions here, I'll re-assign for credit! 🙇

@robertjchen robertjchen removed their assignment Jun 29, 2023
@thienlnam thienlnam added the Internal Requires API changes or must be handled by Expensify staff label Jun 29, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 29, 2023

Current assignee @aimane-chnaif is eligible for the Internal assigner, not assigning anyone new.

@pecanoro pecanoro added the Reviewing Has a PR in review label Jun 29, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 7, 2023

@puneetlath, @pecanoro, @NicMendonca, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@puneetlath
Copy link
Contributor

I believe this was fixed via #21822 which was deployed to production 5 days ago. So @NicMendonca I think you just need to pay out the issue reporter and then this is good to go.

@tranvantoan-qn
Copy link

FYI @puneetlath
I just saw the issue again on staging, so it may not have been fixed or deployed yet. The dev team should probably recheck it.

image

@aimane-chnaif
Copy link
Contributor

@tranvantoan-qn from your screenshot, I am not sure what bug it is. Can you please elaborate more with repro step?

@tranvantoan-qn
Copy link

tranvantoan-qn commented Jul 11, 2023

@aimane-chnaif As the bug's title mentioned User A Can See Welcome Message Intended for User B

In this case, I'm user A, and user B is the one I'm highlighted

Since it's the whisperer message sent by the system, it should say:

  • "Only visible to you" not Only visible to {User B}
  • OR It shouldn't show for me

I didn't test this case intentionally, but It happened to me when I join this room

@aimane-chnaif
Copy link
Contributor

@tranvantoan-qn user A is admin right? It's not bug but expected. Please read through full discussion in this GH.

@tranvantoan-qn
Copy link

tranvantoan-qn commented Jul 11, 2023

@aimane-chnaif
No - the user A - which is me is just a participant in this public room - SNH to be specific:

@tranvantoan-qn
Copy link

Please look at the following image, now it looks OK - but the message I show in the previous image (at the time when the issue happens) is GONE:

image

@pecanoro
Copy link
Contributor

@tranvantoan-qn You need to provide some reproduction steps

@pecanoro
Copy link
Contributor

@NicMendonca Friendly reminder to pay the reporting bonus to @avi-shek-jha so we can close this 🙏

@aimane-chnaif
Copy link
Contributor

Also #20990 (comment)

@pecanoro
Copy link
Contributor

Ahh you are right, @NicMendonca We also need to pay @aimane-chnaif for internal review!!

@NicMendonca
Copy link
Contributor

@avi-shek-jha @aimane-chnaif can you please apply to the job? https://www.upwork.com/jobs/~01d954ce545b1fc7fc

@NicMendonca NicMendonca changed the title [$1000] Welcome Message Visibility Bug: User A Can See Welcome Message Intended for User B [Hold for payment] [$1000] Welcome Message Visibility Bug: User A Can See Welcome Message Intended for User B Jul 13, 2023
@avi-shek-jha
Copy link

Thanks. Applied.

@NicMendonca
Copy link
Contributor

@aimane-chnaif sent you the offer

@NicMendonca
Copy link
Contributor

@aimane-chnaif paid!

@@avi-shek-jha just need you to accept the offer, thanks!

@NicMendonca
Copy link
Contributor

everyone has been paid! 🎉 thank all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests