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

New user joining existing orgs #979

Merged
merged 22 commits into from
Feb 14, 2023
Merged

Conversation

bryce-fitzsimons
Copy link
Member

@bryce-fitzsimons bryce-fitzsimons commented Feb 6, 2023

Features and Changes

Adds a new flow for Cloud users who register using a company email address (verified via SSO). When we detect a potential matching pre-existing organization, we recommend it for the user to join. This should hopefully significantly cut down on the number of duplicate Cloud organizations going forward.

The logic for this flow is as follows:

  1. When a new Cloud user joins GrowthBook via SSO, try to determine if they are using a company email address (and not a free email domain) using free-email-domains-typescript.
  2. If using a company email, try to recommend an appropriate organization rather than defaulting to creating a new organization.
    a. An appropriate organization has a verified domain matching the SSO user's domain. Currently we must rely on the organization's owner email, finding the corresponding user, and making sure they also used a verified method to register (SSO).
    b. If no matches, skip recommendation.
    c. If match(es), choose the one with the most members.
  3. Introduce a new holding tank for these pending members.
    a. Give the admins tooling to approve or reject pending members.
    b. Give the admins a toggle to allow for auto approval (for verified emails).
  4. Send emails to admins and pending members for relevant events.

Testing

Testing is a bit tricky. You'll need to either simulate Cloud on your dev machine (including SSO) or implement a bunch of overrides. To quickly test via these in-code overrides, you can check out the branch and revert this commit 78def93e680ac6ef4b2ee88bdd7b76f511b87c88.

Then, try different combinations of adding verified: true or verified: false to the users mongo docs for both the owner and the joining user. Normally, this field is set when using a verified registration method (SSO).

Screenshots

image

image

image

image

image

@github-actions
Copy link

github-actions bot commented Feb 6, 2023

Your preview environment pr-979-bttf has been deployed.

Preview environment endpoints are available at:

@bryce-fitzsimons bryce-fitzsimons marked this pull request as ready for review February 11, 2023 07:25
Copy link
Contributor

@bttf bttf left a comment

Choose a reason for hiding this comment

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

UX-wise this makes a lot of sense and I think it's a good way to handle it.

I'm curious about how we are handling 'pending' members from a data modeling perspective - we are adding an additional pendingMembers array to the Organization, however an alternative that screams out to me is the addition of a status-like field to members, and having it flip between approved / pending (and in the future perhaps archived) ... Allowing us to keep the single members array on the Org.

We are duplicating some code, both in the Org model (see one of my comments) and in the putMember controller method - there is some overlap between addMemberToOrg and addPendingMemberToOrg ... (Although it seems the latter includes name and email ... I guess members don't have names but maybe we should add that now.)

@bryce-fitzsimons
Copy link
Member Author

Hey @bttf , thanks for the review. I addressed some of the easier DRY / formatting issues, but not 100% sure how we want to proceed with the data model concerns. I think it would be useful to get @jdorn's advice on this as well.

On the one hand, I agree that having a single members array with various status and optional fields is more concise (although it becomes less obvious when we should expect which optional fields for different statuses). On the other hand, we already have the precedent of delineating invites from members, as well as some concern about back-end auth and permission checks that are not set up to handle different statuses for members (pending, invited, etc). I could go either way on this, but would be good to get some additional input...

@bryce-fitzsimons
Copy link
Member Author

bryce-fitzsimons commented Feb 13, 2023

Update here. Spoke with @jdorn and we'll introduce a new domain field to the org model (verifiedDomain). We can check on this instead of doing regex lookups using owner email. We'll need to run a migration script on Cloud to make this work; merging this PR should be safe regardless of order.

We also decided to keep the members and pendingMembers as separate models to avoid the auth/permission complications.

bttf
bttf previously approved these changes Feb 14, 2023
Copy link
Contributor

@bttf bttf left a comment

Choose a reason for hiding this comment

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

Looks good, just one comment

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.

2 participants