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

feat(users): bulk lookup users by email #3720

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

cdriesler
Copy link
Member

Description & motivation

  • The invites flow reaches a point where it knows:
    • A collection of emails
    • Where it would like to invite them to
  • The current lookup allows searching one query => many user matches, but not many queries => any matches

Changes:

  • Adds usersByEmail query modeled after users query
  • Extracts common logic to lookupUsersBaseQuery
  • Adds tests for both lookups

@cdriesler cdriesler requested a review from alemagio December 18, 2024 19:04
@iainsproat
Copy link
Contributor

@hasServerRole(role: SERVER_GUEST)
@hasScopes(scopes: ["users:read", "profile:read"])

Can you remind me what this does; do they need to be logged in to use this?

Is there a limit on the number of emails which can be passed in? (I see there's a limit of 100 on the results)

What is to stop someone dumping in a list of known or guessed email addresses to app.speckle.systems and using that to confirm whether they are users or not? With the status quo of a single look up, they might hit up against our (generous) rate limits (as a fairly weak source of friction against abuse), but does this make it easier to abuse the system?

Do we need some sort of lookup limit or rate limit?

Copy link
Contributor

@iainsproat iainsproat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like a clearer description in the PR of the security/confidentiality implications before we merge.

@cdriesler cdriesler removed the request for review from alemagio January 7, 2025 01:04
@cdriesler
Copy link
Member Author

The PR is parity with existing data egress if you know/guess a user's email. (You must be signed in/have a token.) The ability to do this in bulk does speed things up, in theory. I've lowered the limit to 20 instead of 100 (cc @Mikehrn it sounded like this would be plenty for invite use cases?)

If we want to add additional rate limiting I think that can happen in a follow-up so we unblock Mike

@cdriesler cdriesler requested a review from iainsproat January 7, 2025 17:15
@Mikehrn
Copy link
Contributor

Mikehrn commented Jan 7, 2025

The PR is parity with existing data egress if you know/guess a user's email. (You must be signed in/have a token.) The ability to do this in bulk does speed things up, in theory. I've lowered the limit to 20 instead of 100 (cc @Mikehrn it sounded like this would be plenty for invite use cases?)

If we want to add additional rate limiting I think that can happen in a follow-up so we unblock Mike

Nice :) Yeah 20 is totally fine, I will limit it 20 as well in the FE, multiple invites at one are are not super common, and if people invite multiple people it's usually 2-3 max

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

Successfully merging this pull request may close these issues.

3 participants