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(third-party-auth): Don't show Google/Apple login buttons in the Sync flow #15443

Merged
merged 1 commit into from
Jun 14, 2023

Conversation

LZoog
Copy link
Contributor

@LZoog LZoog commented Jun 13, 2023

Because:

  • Users have to have an FxA account to register/login to Sync so we should not allow them to login with third-party auth in this flow

This commit:

  • Hides these buttons if the flow is Sync

Fixes FXA-7655


Test locally @ here before and after patch. Looked better to me to leave the "firefox family" text when third party auth buttons are hidden - there's a nicer way to do this in Backbone/Mustache than have two sets of #firefox-family-services references in the mustache file (like setting a shouldShowThirdPartyAuth method) but we're refactoring this all soon enough so 🤷‍♀️ as-is seems fine to me.

…ync flow

Because:
* Users have to have an FxA account to register/login to Sync so we should not allow them to login with third-party auth in this flow

This commit:
* Hides these buttons if the flow is Sync

Fixes FXA-7655
@LZoog LZoog requested a review from a team as a code owner June 13, 2023 18:34
@vpomerleau
Copy link
Contributor

Tried this out locally with a fresh profile. Looks good with the test link, but if I remove &service=sync third party auth buttons are still hidden. Is that expected?

@LZoog
Copy link
Contributor Author

LZoog commented Jun 13, 2023

Tried this out locally with a fresh profile. Looks good with the test link, but if I remove &service=sync third party auth buttons are still hidden. Is that expected?

Yes, you'll still be in the sync flow because of that context param you pick up. There's a bunch of logic in app-start that selects the correct relier, this one hits here.

Copy link
Contributor

@vbudhram vbudhram left a comment

Choose a reason for hiding this comment

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

I dig it 👍🏽

Screenshot 2023-06-14 at 11 35 25 AM

@LZoog LZoog merged commit 4b3a68d into main Jun 14, 2023
@LZoog LZoog deleted the FXA-7655 branch June 14, 2023 16:19
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