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

OAuth login / Socialite #214

Closed
barryvdh opened this issue Sep 16, 2020 · 45 comments
Closed

OAuth login / Socialite #214

barryvdh opened this issue Sep 16, 2020 · 45 comments
Assignees

Comments

@barryvdh
Copy link

barryvdh commented Sep 16, 2020

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:

  • Configure socialite providers, eg Google, Facebook etc
  • Register with Socialite (define the fields)
  • Whitelist Allowlist registration for certain domains (eg. let everyone inside the company register, nobody else)
  • Login with Socialite (perhaps optionally disable normal login)
  • Connect/disconnect your account with other accounts on your profile page
  • Possibility to skip 2FA if coming from Socialite (because other accounts could handle it)

What would be needed

  • Additional table with socialite ids
  • UI for login/connect/register
  • Registering of socialite / mapping of fields
@abenerd
Copy link
Contributor

abenerd commented Sep 17, 2020

I really like this idea! But we should refrain from using terms such as whitelist and blacklist and use terms such as allowlist and blocklist or something similar.

@FoksVHox
Copy link

FoksVHox commented Sep 17, 2020

I really like this idea! But we should refrain from using terms such as whitelist and blacklist and use terms such as allowlist and blocklist or something similar.

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.

@abenerd
Copy link
Contributor

abenerd commented Sep 17, 2020

@FoksVHox https://www.zdnet.com/article/linux-team-approves-new-terminology-bans-terms-like-blacklist-and-slave

@FoksVHox
Copy link

@FoksVHox https://www.zdnet.com/article/linux-team-approves-new-terminology-bans-terms-like-blacklist-and-slave

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.

@driesvints
Copy link
Member

I closed #84 before but I'll bring this up with Taylor.

@barryvdh
Copy link
Author

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.

@driesvints
Copy link
Member

@barryvdh you're right 👍

@driesvints driesvints changed the title [Feature] OAuth login / Socialite OAuth login / Socialite Sep 18, 2020
@rennokki
Copy link
Contributor

rennokki commented Sep 25, 2020

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.

@joelbutcher
Copy link
Contributor

joelbutcher commented Oct 4, 2020

Is anyone working on this integration? Happy to help on it if so

@mberneis
Copy link

mberneis commented Oct 7, 2020

+1 for Socialite integration. - Starting a new project and would like to use jetstream but need to integrate social logins as well.

@joelbutcher
Copy link
Contributor

joelbutcher commented Oct 12, 2020

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:

  • Socialite as a Jetstream feature in the config,
  • Migration and abstract model for 'connected accounts', with events for created, updated and deleted.
  • Profile component to remove connected accounts (InertiaJS and Livewire).
  • [DELETE] /user/connected-accounts/{id} route for removing connected accounts with inertia js

There's still a bit to do, such as ensuring that Socialite is pulled in if the user has specified --socialite in the install command (also needs adding). Tests also need adding 😎

@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 ✌🏼

@dillingham
Copy link
Contributor

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.
Pretty commonly seen in the wild. Also usually with a email verification step.

@joelbutcher
Copy link
Contributor

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:

Sign in / Sign up with x
Continue with x
Login with x

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?

@arnarstef
Copy link

+1 for this feature, hope this progresses soon enough for my next project

@dillingham
Copy link
Contributor

dillingham commented Oct 18, 2020

@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 auth.socialite.buttons.gitlab view
And $route in a default view would equal socialite.gitlab
Maybe 3rd includeFirst option to fallback if default is deleted
'jetstream::auth.socialite.buttons.default'

@joelbutcher
Copy link
Contributor

joelbutcher commented Oct 19, 2020

@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!

@viezel
Copy link

viezel commented Nov 3, 2020

@joelbutcher really good progress on that branch. Keep it up 👍

@joelbutcher
Copy link
Contributor

@viezel Thanks 🤓

Anyone, feel free to hop on and help out - I'm struggling to find time to make headway!

@joelbutcher
Copy link
Contributor

Just to update you all on the progress - here are all the services currently supported by Socialite - I need a few people to pull this into a fresh livewire install and test/give feedback (if you're able!)

Screenshot 2020-11-08 at 00 49 40

@joelbutcher
Copy link
Contributor

Okay! Laravel Socialite is now integrated into Jetstream. A question for discussion, what should happen if you have only one connected provider? I've made password nullable on the users table

Screenshot 2020-11-08 at 16 54 45

@dillingham
Copy link
Contributor

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 😄

@joelbutcher
Copy link
Contributor

@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!

@taylorotwell
Copy link
Member

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?

@dillingham
Copy link
Contributor

dillingham commented Nov 9, 2020

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:

maxresdefault (1)

@taylorotwell
Copy link
Member

@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?

@barryvdh
Copy link
Author

barryvdh commented Nov 9, 2020

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?)

@joelbutcher
Copy link
Contributor

@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:

  1. Do most social login services allow the same user to authenticate using multiple social providers?

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 [email protected]) then I'd expect the app I'm logging in to, to resolve both?

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.

  1. it feels like if you have one connected account

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.

  1. If you want to end your account at that point you should use the "Delete Account" feature?

  2. If email login is disabled, you should have at least 1 login method.

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.

  1. Don't know if you fill the e-mail when registering with OAuth

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 😎

@taylorotwell
Copy link
Member

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.

@joelbutcher
Copy link
Contributor

joelbutcher commented Nov 10, 2020

@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

@taylorotwell
Copy link
Member

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.

@joelbutcher
Copy link
Contributor

Just need to tidy up some bits, will complete this when I get home after work!

This was referenced Nov 10, 2020
@joelbutcher
Copy link
Contributor

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.

@dillingham
Copy link
Contributor

dillingham commented Nov 13, 2020

Was just imagining if Socialite::drivers() produced an array like ['github', 'facebook'] .. and any package from https://socialiteproviders.com got automatically registered... and the Connected Accounts panel looped them... you could composer install any of them and they automatically appear on the settings page.

Pretty out of scope for this but kinda fun to think about.

@joelbutcher
Copy link
Contributor

@dillingham does Socialite come with the providers method?

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 providers key in it's config, what do ya'll think?

@dillingham
Copy link
Contributor

@joelbutcher looks like Illuminate\Support\Manager has a getDrivers method but I'm not sure if it registers providers in the way I was picturing above. When you call Socialite::driver('github') a github key => instance gets stored in $drivers, so Im guessing it only is available during a request where thats called not for all drivers "registered". Looks like the manager class of "SocialiteProviders" does something like what we are discussing. I think it would be a bonus but not essential

@iantasker
Copy link

@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.

@joelbutcher
Copy link
Contributor

@iantasker Can you give me such a use case?

@iantasker
Copy link

@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.

@joelbutcher
Copy link
Contributor

@iantasker Okay, that sounds really cool!! I'm hesitant to make the relation on this PR polymorphic for a few reasons:

  1. Your given use-case is extremely niche and unique to the project you're working on. (not to mention the fact that Socialite doesn't support Stripe as a provider).
  2. Jetstream is designed to be a starting point, so you're free to make whatever changes you need to your applications codebase in order to cater for your specific scenario.
  3. I'm generally not a fan of polymorphic relationships anyway. Especially when it comes to security credentials - IMO there's too many moving parts for that to be a valid solution to a use-case that isn't "the norm" for OAuth

@iantasker
Copy link

@joelbutcher In response

  1. The use case isn't limited to just stripe (which is supported via community plugins) it's mainly about the ability to extend what you are implementing. It would be good if the solution was flexible enough to support all these providers https://socialiteproviders.com/about/
  2. I agree Jeatsream is a starting point but it should be flexible, by not implementing a polymorphic relationship you are creating an additional burden to maintain forks for a straight forward extension that can't easily be added after the fact.
  3. I can understand your preference not to use them but like myself, there could be others that do have a preference to use them. Its a valid use case even if you don't think it's "the norm".

@joelbutcher
Copy link
Contributor

@iantasker Extension of this implementation is possible, you would need to make the migration changes yourself and override the published ConnectedAccount model. The SocialiteProviders package will already supported by this as the redirect and callback routes just required the driver string to replace the wildcard in the URI. Should work if you follow their installation & usage docs.

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

@iantasker
Copy link

@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?

@joelbutcher
Copy link
Contributor

@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?

@joelbutcher
Copy link
Contributor

@barryvdh Taylor has closed my PR for this issue, but I'm looking into developing a package for this to compliment Jetstream...

@iantasker
Copy link

@joelbutcher I am happy to help turn this PR into a compatible plugin, how can I help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests