-
Notifications
You must be signed in to change notification settings - Fork 809
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
OAuth login / Socialite #214
Comments
I really like this idea! But we should refrain from using terms such as |
Why? As I see it the only people that will get offended by those words, should calm down & understand that the words aren't racist at all. |
That's not a reason why we should change the words. The article just describes how it's a symbiotic change. The 'issue' is an issue that doesn't exist. |
I closed #84 before but I'll bring this up with Taylor. |
Sorry, Allowlist is better indeed. I've updated it, so can we please stay on track ;) @driesvints Thank you. #84 is the other way around though I think? This ticket was meant being able to login using Socialite, not provide an OAuth server. |
@barryvdh you're right 👍 |
I have submitted this kind of issue and actively replied that I might offer to port the package to Jetstream, no answer tho: #161 Might be useful on building the backend. |
Is anyone working on this integration? Happy to help on it if so |
+1 for Socialite integration. - Starting a new project and would like to use jetstream but need to integrate social logins as well. |
For those who want to take a look, I've started development on this in a feature branched on my forked version of Jetstream. 👀 It's a bit all over the place at the minute, but the gist of it is:
There's still a bit to do, such as ensuring that Socialite is pulled in if the user has specified @taylorotwell are you and the team working on this actively? Or is this something you guys would be up for giving me some pointers on? Happy to do the work, as I said previously, but it is my first time contributing to a project with this much adoption and community presence, so any tips etc. are appreciated! Branch comparison → master...joelbutcher:feature-jetstream-socialite ✌🏼 |
Would be awesome to have google, facebook, github login support. If the email exists during oauth step.. it should offer to connect any previous manually created account. |
I'm now at a point where the nuts and bolts are all done. (Took some time as I also work full time on other stuff...). Looking at the brand guidelines for a lot of OAuth providers, the button wording can change quite a bit:
So I'm considering just buttons just with icons, seems like it would make the views be less cluttered - what are people's thoughts? I'm curious to know whether people would want button components for each provider that Socialite supports by default, or a 'social button' component with a massive if block, determining the button contents by a prop? |
+1 for this feature, hope this progresses soon enough for my next project |
@joelbutcher I think you'd be safe with translated text in a view published during install. <!-- resources/views/auth/socialite/buttons/facebook.blade.php -->
<a href="{{ route('socialite.facebook') }}" class="flex">
<svg class="w-5 h-5 mr-2" role="img" viewBox="0 0 24 24" xmlns="https://www.w3.org/2000/svg"><title>Facebook icon</title><path d="M23.9981 11.9991C23.9981 5.37216 18.626 0 11.9991 0C5.37216 0 0 5.37216 0 11.9991C0 17.9882 4.38789 22.9522 10.1242 23.8524V15.4676H7.07758V11.9991H10.1242V9.35553C10.1242 6.34826 11.9156 4.68714 14.6564 4.68714C15.9692 4.68714 17.3424 4.92149 17.3424 4.92149V7.87439H15.8294C14.3388 7.87439 13.8739 8.79933 13.8739 9.74824V11.9991H17.2018L16.6698 15.4676H13.8739V23.8524C19.6103 22.9522 23.9981 17.9882 23.9981 11.9991Z"/></svg>
<span>{{ __('Login Using Facebook') }}</span>
</a> and maybe a component in the login view that conditionally loads buttons <!-- resources/views/auth/socialite/providers.blade.php -->
@if(Jetstream::hasSocialiteFeatures())
<!-- if facebook / include button -->
<!-- if twitter / include button -->
<!-- if github / include button -->
@endif Although it would be cool if it looped over a config including 3rd party drivers: @if(Jetstream::hasSocialiteFeatures())
@foreach($providers as $provider)
@includeFirst([
"auth.socialite.buttons.$provider",
'auth.socialite.buttons.default'.
], ['route' => "socialite.$provider")
@endforeach
@endif Then people can just add |
@dillingham thanks for this, will look into adding something similar this week! Also, if you want to fork the branch and work on any bits yourself to help speed this along, please don't hold back! |
@joelbutcher really good progress on that branch. Keep it up 👍 |
@viezel Thanks 🤓 Anyone, feel free to hop on and help out - I'm struggling to find time to make headway! |
Good question, 🤔 Maybe if $password is null, require password reset to remove last connected account. If the user authenticated facebook but no longer has access to their fb email, could be problematic for resetting. A password step + email confirmation during signup (less ideal but I've seen it in oauth signup flows before) would avoid both. Ps. That looks really nice! Your effort on this is very appreciated 😄 |
@dillingham I think this could probably be something that developers would want to change? At the moment, I've just got a confirmation dialog as the password is null. I'd also hidden the 'remove button if you've only got connected account, to avoid any potential issues with passwords being null etc. @driesvints @taylorotwell if either of you guys could shed some light on ideal functionality here, that would be great! |
Do most social login services allow the same user to authenticate using multiple social providers? I don't feel like I've seen that before. For example if Spotify lets me authenticate with Facebook or Google, wouldn't those be two separate Spotify accounts? |
I think if another provider is used and has a unique email it would for sure create a new account if logged out. If it’s the same email or an attempt to connect while logged in.. it associates. For apps where you can perform behavior via multiple providers, that multiple provider interface is pretty nice to have (maybe also show ones not connected) youtube example: |
@joelbutcher it feels like if you have one connected account there should not be an option to remove it? If you want to end your account at that point you should use the "Delete Account" feature? |
If you use your verified email account, you should probably be able to disconnect it and use the 'email login' feature. If email login is disabled, you should have at least 1 login method. (Don't know if you fill the e-mail when registering with OAuth?) |
@taylorotwell @barryvdh Thanks for this - all good stuff to consider in the development of this feature! Just want to address some things you guys have touched upon:
I'm not 100% certain on 'most social login services', but I know Medium, for examples sakes, allows this. I think if you're using social accounts from different providers with the same email (i.e Facebook AND Google when the email on both is I know your Spotify example is different as the uniqueness of a user isn't on the email, but the username (which Spotify generates based on the name/email given by the connected provider during OAuth.
Yeh, this is my intended functionality. If there's just one connected account (i.e. I only use Facebook for OAuth) then the 'remove' button disappears. If I have Google AND Facebook accounts connected, then the 'remove' button is visible on both.
I'm reluctant to allow disabling of email login, what if I want to use your product, but don't have an account with any of the providers you've allowed? I guess there may well be some niche use-cases for this, but developers looking for this 'feature' can just alter the components if desired.
I am, so when you click an icon and authorize with the provider, I'm grabbing the email returned, resolving the user for the connected accounts provider id and the email address (a user can have multiple connected accounts). If no user is found, I then create the user with a null password and use the fortify pipelines. Hopefully, that's addressed all of the issues raised in the comments above, if you've got more, fire 'em away 😎 |
How does two factor authentication work with applications that use social login. I'm not sure I use social login with any applications that I have 2FA enabled on so I'm not sure what is customary. |
@taylorotwell in my experience MFA/2FA should be handled by the provider. For example, everywhere I use 'Continue as Joel' from Facebook in incognito (or a browser I've never logged into Facebook on before) Facebook challenges it and requires me to verify myself by either generating a code, or by me logging into a known and verified browser session and confirm that it was me who just logged in |
OK. It's probably easiest at this point if you just turn what you have into a finished PR so we can discuss actual code. Thanks. |
Just need to tidy up some bits, will complete this when I get home after work! |
Sorry guys, my first PR was a mess! Spent the whole evening testing livewire to find a number of issues. For simplicities sake, I've disabled the ability the email field on the update profile information form if the authenticated user has any connected accounts. |
Was just imagining if Pretty out of scope for this but kinda fun to think about. |
@dillingham does Socialite come with the If not the this would probably require a PR to that package in order to pull it in for this functionality. On that note, I was debating making a PR for Socialite to put a |
@joelbutcher looks like |
@joelbutcher This feature is a great idea. One small request would be to not lock the social account to a user, there are use cases where the oauth credentials need to apply to teams. Replacing the user_id field on connected_accounts table to be polymorphic would allow this. |
@iantasker Can you give me such a use case? |
@joelbutcher We are currently looking at linking teams to business stripe connect accounts, some businesses use non-person account (ie an integration account) so the integration doesn't need to re-setup if someone leaves. |
@iantasker Okay, that sounds really cool!! I'm hesitant to make the relation on this PR polymorphic for a few reasons:
|
@joelbutcher In response
|
@iantasker Extension of this implementation is possible, you would need to make the migration changes yourself and override the published If this gets merged and you REALLY want to make this change, then may I suggest you raise a PR at that point? I'm not planning on making the change in this PR I'm afraid |
@joelbutcher I'll check to see if the team socialite functionality is possible via an extension and let you know if there are any blockers, hopefully, anything non-polymorphic that is a blocker could be implemented? |
@iantasker The branch & PR for this feature is now at a stage where I don't want to be making drastic changes to the underlying functionality. It really needs Taylor to take a proper look at it so we can get this in for Jetstream v2, any changes would require another look over from him and he's not got the time to keep revisiting one PR. When/if it gets merged, you could open an issue and @ mention me in the description/reference our discussion here and we can revisit it once this PR is merged? |
@barryvdh Taylor has closed my PR for this issue, but I'm looking into developing a package for this to compliment Jetstream... |
@joelbutcher I am happy to help turn this PR into a compatible plugin, how can I help? |
Besides normal login and two-factor, it might be nice to add OAuth login. Because Socialite is a first-party package, this could make sense to add by default.
Ideas:
WhitelistAllowlist registration for certain domains (eg. let everyone inside the company register, nobody else)What would be needed
The text was updated successfully, but these errors were encountered: