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: add sso enforcement for invite signup #25808

Merged
merged 26 commits into from
Oct 31, 2024

Conversation

zlwaterfield
Copy link
Contributor

@zlwaterfield zlwaterfield commented Oct 24, 2024

Problem

Right now, SSO enforcement is not upheld on the invite signup page. An invited user can login with password or one of our social SSO options (Google, Github, Gitlab) and there is no SAML option.

Here is what it looks like:
Screenshot 2024-10-24 at 5 30 12 PM

If the domain of the invite has SSO enforcement on then we want to show the correct button (SAML, Google, etc.) like we do on the login page. And we need to make sure we pass the invite ID through so it can accept the invite as well.

Changes

This PR adds the precheck call to check on the invite signup page using the domain of the email being invited. If it's turned on it then will hide all other fields (password, name, etc.) as well as the social SSO buttons along the bottom and show the SSO login button instead.

It also adds an actionText to the SSO login button props so we can show "Continue with.." instead of "Login with".

Note: with this change we will no login take in the Role field. We could look into adding it into the support fields for the SSO extra fields so it's processed on the SSO callback.

Here is what is would look like:
Screenshot 2024-10-24 at 5 27 43 PM

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

Need to do some more manual testing on this to confirm the invite gets accepted correctly. Here is where it happens: https://github.com/PostHog/posthog/blob/master/posthog/api/signup.py#L517-L519

Copy link
Contributor

github-actions bot commented Oct 24, 2024

Size Change: 0 B

Total Size: 1.15 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.15 MB

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

10 snapshot changes in total. 0 added, 10 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@raquelmsmith
Copy link
Member

I haven't reviewed the code yet but saw there are no changes to the backend, there should prob be some validation there?

@zlwaterfield
Copy link
Contributor Author

I haven't reviewed the code yet but saw there are no changes to the backend, there should prob be some validation there?

What sorta validation would you expect? I haven't changed any forms so it's using all of the same logic. The only thing is pushing to SSO if it's enforced.

@timgl
Copy link
Collaborator

timgl commented Oct 25, 2024

@zlwaterfield You shouldn't be able to use the API to sign up with username/password when SSO is enforced.

@zlwaterfield
Copy link
Contributor Author

@zlwaterfield You shouldn't be able to use the API to sign up with username/password when SSO is enforced.

Ah yes, I can add that in.

@zlwaterfield
Copy link
Contributor Author

@raquelmsmith added with tests.

@zlwaterfield
Copy link
Contributor Author

Looking into why snapshots changed because they shouldn't have.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

4 snapshot changes in total. 0 added, 4 modified, 0 deleted:

  • chromium: 0 added, 4 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

6 snapshot changes in total. 0 added, 6 modified, 0 deleted:

  • chromium: 0 added, 6 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Member

@raquelmsmith raquelmsmith left a comment

Choose a reason for hiding this comment

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

Just verifying the conditionals are correct, otherwise looks good 👍

>
Continue
</LemonButton>
{precheckResponse.status === 'pending' || !precheckResponse.sso_enforcement ? (
Copy link
Member

Choose a reason for hiding this comment

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

Are these conditionals correct? If the response status is pending do we want to show the "continue" button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will test a few things and confirm / improve.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@zlwaterfield zlwaterfield merged commit 40a602a into master Oct 31, 2024
96 checks passed
@zlwaterfield zlwaterfield deleted the zach/add-sso-enforcement-invite-signup branch October 31, 2024 14:02
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.

4 participants