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

fix: duplicate SSO login buttons when SSO provider or SAML login is enforced #17333

Merged
merged 7 commits into from
Sep 13, 2023

Conversation

maxbelm
Copy link
Contributor

@maxbelm maxbelm commented Sep 6, 2023

Problem

This PR attempts to address #17324

In writing up this PR a couple assumptions were made:

  1. That we should not display the SocialLoginButton component if an SSO provider is enforced
  2. That we should not display the SocialLoginButton component if SAML is available

The thinking behind those assumptions is: a user will have a clearer, more streamlined login experience when they only see the enforced options for login.

Please let me know if any of the assumptions are incorrect, happy to adjust the work as needed!

Changes

SSO Enforcement (Google):
image

SAML available:
image

No SSO enforcement or SAML:
image

How did you test this code?

Since this is a frontend UI change and there are stories present for the login component, I went with a manual testing approach. Happy to adjust that approach if we'd like!

Testing explanation:

Previously there was one value that influenced the SocialLoginButton component's display:

  • preflight.available_social_auth_providers

As of this PR, three values will influence the component's display:

  • precheckResponse.saml_available
  • precheckResponse.sso_enforcement
  • preflight.available_social_auth_providers

In order to test this, I modified these values inline to observe that the UI behavior was as expected.

The Google SSO enforced picture involved setting the values to the following:

  • precheckResponse.saml_available = false
  • precheckResponse.sso_enforcement = 'google-oauth2'
  • preflight.available_social_auth_providers = { github: true, gitlab: true, 'google-oauth2': true }

The SAML available picture involved setting the values to the following:

  • precheckResponse.saml_available = true
  • precheckResponse.sso_enforcement = null
  • preflight.available_social_auth_providers = { github: true, gitlab: true, 'google-oauth2': true }

The social login link picture involved setting the values to the following:

  • precheckResponse.saml_available = false
  • precheckResponse.sso_enforcement = null
  • preflight.available_social_auth_providers = { github: true, gitlab: true, 'google-oauth2': true }

Note: The StoryBook stories for the Login component look correct as well.

@raquelmsmith
Copy link
Member

This looks great, thanks for the detailed PR @maxbelm!

@raquelmsmith raquelmsmith enabled auto-merge (squash) September 6, 2023 18:28
@raquelmsmith raquelmsmith changed the title Fix duplicate SSO login buttons when SSO provider or SAML login enforced fix: duplicate SSO login buttons when SSO provider or SAML login enforced Sep 6, 2023
auto-merge was automatically disabled September 6, 2023 18:31

Head branch was pushed to by a user without write access

@maxbelm maxbelm force-pushed the fix/dupe-sso-login-btn branch from 88d48f0 to 2fdda1c Compare September 6, 2023 18:31
@maxbelm maxbelm changed the title fix: duplicate SSO login buttons when SSO provider or SAML login enforced fix: duplicate SSO login buttons when SSO provider or SAML login is enforced Sep 6, 2023
@raquelmsmith raquelmsmith enabled auto-merge (squash) September 6, 2023 18:32
@maxbelm
Copy link
Contributor Author

maxbelm commented Sep 6, 2023

@raquelmsmith I really appreciate the quick feedback and review!

It looks like E2E required tests and docker image build are erroring out:

Unable to exchange open-source pull request OIDC token for temporary Depot token: Error: OIDC auth challenge ... timed out 
...
Error: failed with: Error: missing API token, please run `depot login`

I'm not too familiar with this, is this something the team has encountered before?

@raquelmsmith
Copy link
Member

Hm, that's because your fork is technically outside of our repository (as is expected). Let me check with the rest of the team to see what to do here.

github-actions bot and others added 6 commits September 7, 2023 23:45
# Conflicts:
#	frontend/__snapshots__/scenes-app-recordings--recordings-play-list-no-pinned-recordings.png
# Conflicts:
#	frontend/__snapshots__/scenes-app-recordings--recordings-play-list-no-pinned-recordings.png
@raquelmsmith raquelmsmith merged commit 8dcea1f into PostHog:master Sep 13, 2023
60 checks passed
pauldambra pushed a commit that referenced this pull request Sep 14, 2023
…nforced (#17333)

fix: Hide social login links when SAML is available or SSO provider is enforced
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