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

HIGH: Prevent DMs from appearing as "Hidden" #32613

Closed
quinthar opened this issue Dec 7, 2023 · 8 comments
Closed

HIGH: Prevent DMs from appearing as "Hidden" #32613

quinthar opened this issue Dec 7, 2023 · 8 comments
Assignees
Labels

Comments

@quinthar
Copy link
Contributor

quinthar commented Dec 7, 2023

Background:

  • we created a function in Auth to determine whether userA knows userB. The criteria are:
    • if userB has ever requested money or sent an expense report to userA
    • if userB has ever sent a message in a DM or groupDM that userA is a part of
    • if userA and userB share access to at least one non-public workspace room or domain room
  • that function is heavy, so it’s not fast to run over a large group of users (e.g. everyone in your LHN or every participant in a public report)
  • When you sign in, we run a big query to get the personalDetails of a bunch of people (people you share policies with, people you share chats with, etc)
    • For the people that you share chats with but not policies with (aka DMs), we don’t check if you know them because it would be too expensive to check them all. So you are only returned their public personal details upon sign-in
    • When you go to their profile or open a report (e.g. your DM) with them, we do check if you know them and you are returned their full details if you do

Sequence of events:

  1. first, we made it so that we don’t return the login when you don’t know someone
    a. this means if you go directly to their profile, for example, by clicking on their avatar from a post in a public room, you don’t get their login info
  2. then, we made it so that when you go to someone’s profile, we don’t return their email address as their display name
    a. this is because accountIDs are sequential and we didn’t want to make it possible to get a ton of email addresses by looping through new.expensify.com/a/1
    b. at this point, all the other flows (e.g. OpenReport, sign in) would still return the email address as the display name
  3. then, a contributor reported the security issue that you could get email addresses by going to any profile (e.g. new.expensify.com/a/1) and clicking “Message hidden”.
    a. This is because OpenReport would still return the email address as the display name if a display name wasn’t set
    b. So we made it so that we never return the display name if we aren’t confident that you know the user

Current state:
This leaves us with two kind of weird “bugs” right now:

  1. When you sign in, in your LHN, on DMs with people that you do know but who don’t share any policies with you, you’ll see “hidden” for their display name (if they don’t have one set) until you open the chat. Once you open the chat, you’ll get the full details and see their email address as their display name
    This is because the “known user” check is slow, so we don’t do it for all your DMs on sign-in
  2. When you create a new DM with someone who you don’t “know” (because they’ve never chatted you before) you see their display name as “hidden” even though you just created the chat using their email address.
    This is because we are no longer returning the email address as the display name for unknown users. And by our current definition, you don’t know this user until they message you back.

Slack convo: https://expensify.slack.com/archives/C066HJM2CAZ/p1701193182994409

@quinthar quinthar converted this from a draft issue Dec 7, 2023
@quinthar quinthar changed the title HIGH Prevent DMs from appearing as "Hidden" HIGH: Prevent DMs from appearing as "Hidden" Dec 7, 2023
@melvin-bot melvin-bot bot added the Monthly KSv2 label Dec 11, 2023
@puneetlath
Copy link
Contributor

@quinthar I laid out the current problems above.

So far what we have done:

  1. made it so that if you are the inviter for someone, then they are considered as "known" to you (convo)

Potentially changes still in discussion:

  1. Optimistically set the personalDetails when creating a new DM and don't overwrite the email with the server response if you don't know them (convo)
  2. In focus mode, only get the personalDetails of those needed for your LHN. Get everyone else's asynchronously via a Bedrock job (convo)
  3. In non-focus mode, since we don't have pagination yet, only get up to X personal details. Get everyone else's asynchronously via a Bedrock job (convo)
  4. make the "isKnownUser" queries faster somehow

@puneetlath puneetlath added Weekly KSv2 and removed Monthly KSv2 labels Dec 11, 2023
@puneetlath
Copy link
Contributor

Did some query timing on the existing queries here. They are not fast in the worst scenarios, but I think they are fine for us to go forward with the planned solution and see if we have any problems before optimizing further.

@puneetlath
Copy link
Contributor

I’m planning to break this into 4-5 PRs:

  1. Update the OpenApp Auth command to fetch personal details for just reports being returned instead of all chats when in Focus mode
  2. Create a GetPersonalDetailsListAsync Auth command to get personalDetails for all chats, including whether or not the other users are “known”
  3. Web PR to update openApp API call to use the personalDetails from 1 and trigger a bedrock job to do 2 instead of using a separate Auth command to get all personal details as it does today
  4. Update the Auth command to take a similar approach for non-focus-mode (after a little time to make sure things are ok with the above)
  5. (maybe) try to make the queries faster

I’ve started a PR for 1: https://github.com/Expensify/Auth/pull/9414

@quinthar quinthar moved this from HIGH to CRITICAL in [#whatsnext] #vip-vsb Dec 19, 2023
@quinthar quinthar moved this from CRITICAL to HIGH in [#whatsnext] #vip-vsb Dec 19, 2023
@melvin-bot melvin-bot bot added the Overdue label Dec 22, 2023
@quinthar
Copy link
Contributor Author

I think this is a good update, but I think this should actually be handled in #vip-split, as #vip-vsb doesn't really deal with DMs.

@puneetlath
Copy link
Contributor

Web PR is ready, just an unrelated failing test: https://github.com/Expensify/Web-Expensify/pull/40276

@melvin-bot melvin-bot bot removed the Overdue label Dec 30, 2023
@puneetlath
Copy link
Contributor

This is done. To summarize what we did:

  1. We moved the fetching of personal details into the OpenApp auth command, instead of being in a separate auth command
  2. We kept the fetching of details for fellow workspace members, domain members, and delegates the same (all of which we automatically mark as "known" to you)
  3. We updated the fetching of details for people you've chatted with to be just for the chats we're returning in OpenApp instead of everyone you've ever chatted with
  4. We'll do the "isKnownUser" check for up to 50 DMs (we only need to do the check for DMs with people you don't share policies with)

That's all for now. We'll see how it goes. In the future, we can potentially look into "prefetching" other personal details that we think might be important to make available to you locally.

This comment was marked as resolved.

@puneetlath
Copy link
Contributor

This is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants