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

[2.x] Socialite Support #444

Closed
wants to merge 39 commits into from
Closed

[2.x] Socialite Support #444

wants to merge 39 commits into from

Conversation

joelbutcher
Copy link
Contributor

@joelbutcher joelbutcher commented Nov 10, 2020

This PR adds optional support for Laravel Socialite and all Socialite support providers, via feature flags. Support is available for both InertiaJS and Livewire. See #214

To get started with this PR, create a fresh Laravel project (without --jet) and update your composer.json to include the repository following and require the feature branch.

composer require laravel/jetstream:dev-feature-jetstream-support

You may then run the Jetstream install command, specifying socialite support:

php artisan jetstream:install {stack} --socialite

You may remove support for Socialite by commenting out the feature in jetstream.php

'features' => [
    // Features::socialite(['facebook' => true, 'github' => true]),
],

You'll need to follow the instructions for configuring socialite providers to ensure you're application will support this feature.

Once you're all setup, you should be able to see your providers in your login and register routes views:

Screenshot 2020-11-10 at 21 06 36

Existing users that have signed up using email and password can connect social accounts in their profile settings, as well as remove them:

Screenshot 2020-11-13 at 17 04 10

@joelbutcher joelbutcher changed the title Feature jetstream socialite [2.x] Socialite Support Nov 10, 2020
@driesvints
Copy link
Member

@joelbutcher why did you close this?

@joelbutcher
Copy link
Contributor Author

joelbutcher commented Nov 12, 2020

@driesvints Apologies, I rebased the branch and push it up, but from a different point in time - the git diff forced it to close for some reason. I've made a note of the requested changes and will re-open this PR. I have two machines, my fault entirely

@joelbutcher joelbutcher reopened this Nov 12, 2020
@joelbutcher
Copy link
Contributor Author

@driesvints I've addressed the changes requests in the latest. Once again apologies for the mess-up with Git!

@driesvints
Copy link
Member

@joelbutcher no worries. This is a fantastic PR tbh 👍

@joelbutcher
Copy link
Contributor Author

joelbutcher commented Nov 12, 2020

@driesvints Thanks!! I was debating whether or not to use Blade Icons for the SVG's 😉. But since there's only need for a handful, and I wanted to maintain the brand colors, I thought it best to just include the ones that were needed!

@taylorotwell
Copy link
Member

taylorotwell commented Nov 12, 2020

Have you considered instead of having a separate configuration item doing this:

Features::socialite(['providers' => ['facebook', 'github']])

@joelbutcher
Copy link
Contributor Author

joelbutcher commented Nov 12, 2020

@taylorotwell I hadn't, but it wouldn't take too much to implement this. I'm assuming developers would then specify this in their applications JetstreamServiceProvider

Edit: Never mind, just realised what you mean - similar to how teams can pass options. Will do this now!

@joelbutcher
Copy link
Contributor Author

@taylorotwell Just pushed up that change 👍

@taylorotwell
Copy link
Member

What does the "update password" feature do when the user is authenticated with a social provider? Is the two factor management pane hidden when the user is authenticated using a social provider?

@taylorotwell
Copy link
Member

Also, what if email verification is enabled for email based users? What happens to social users?

@driesvints
Copy link
Member

And what about "reset password"? (maybe I missed the convo about that one)

@joelbutcher
Copy link
Contributor Author

None of those features have been touched as I was unsure what the desired outcome should be?

Email verification, in my opinion, should behave in the same way regardless of whether the user registered via email or socialite (I use the email returned from socialite and populate the users email from this)

@taylorotwell
Copy link
Member

taylorotwell commented Nov 12, 2020

We should probably just change password reset to always return some message like "If an account exists with that email address you will receive an email soon..." etc... that goes for existing email users, social users, and even if the email doesn't exist.

Update password and two factor authentication management panels should be totally hidden if the user is authenticated socially.

@driesvints
Copy link
Member

@taylorotwell I'm not sure those two should be mutually exclusive. For example, on Laravel.io you can first register using Github but afterwards set a password if you want. Both allow you to login.

@taylorotwell
Copy link
Member

Is that how most social authed applications work? Should we just leave both update password and 2FA screens available? The update password screen would perhaps need some wording or something to indicate that if they set a password they will be able to login with both their connected accounts and their email? I dunno.

@joelbutcher said earlier that he thought 2FA should be the social service's responsibility. I'm not sure if this PR is setup to handle 2FA from a social authed account?

@joelbutcher
Copy link
Contributor Author

joelbutcher commented Nov 12, 2020

@taylorotwell I was going off my experience tbh, I don't think there's a defined way of doing 2FA. Regardless, the users email is always populated whether signing/logging in with email or via Socialite. The only issue there is the 2FA confirmation dialogue - the 2FA feature currently requires a user to enter password before being able to enable 2FA.

I've also found that there is an additional complication with hiding those panels as the logic becomes more complex. We've got three conditions we need to check for, making the @if statements look horrible 😂.

  1. App supports socialite & user signed up with email
  2. App supports socialite & user signed up with social provider
  3. App doesn't supports socialite

Screenshot 2020-11-12 at 16 01 06

At the end of the day, I'm happy, willing, and able to implement whatever the general consensus is...

@taylorotwell
Copy link
Member

taylorotwell commented Nov 12, 2020

Checking on GitLab... I sign in via GitHub...

They show me a password update screen but I don't even know what my current password would be so I have no way to confirm it? I'm not sure if this is a bug on their end. What would the user even type into that box?

They also allow me to enable / disable 2FA but do not require me to confirm my password to do so.

Also noting that GitLab seems to keep a separate copy of the email address for the social account. You can change your GitLab email address even if you authenticate socially. Not saying we have to support this just noting that behavior. I assume with this implementation we update the email address on every login? What if they change their email to an email that is already taken in the system and then try to login? Do all these social providers confirm your email address first so that I can't break into other people's accounts in a Jetstream app through that vector?

@driesvints
Copy link
Member

Is that how most social authed applications work?

I don't think there's a golden rule here. DigitalOcean allows only one login type: either email/password or one single social login (not multiple like this PR). Algolia allows a combination of email/password and social accounts.

Imho I think a combination makes most sense. You add different ways of logging in so you have an alternative if you ever get locked out of one.

Algolia requires your 2FA from within Algolia even if you login with a social account. I don't know the general consensus on this.

@taylorotwell
Copy link
Member

Fortify supports enabling 2FA without password confirmation via the feature flag option but the Jetstream mark-up would have to be changed to respect this I believe.

@joelbutcher
Copy link
Contributor Author

I've just tested with Medium. I already had an account through Facebook OAuth. But when signing up with email, using the same email address that's connected to Facebook, it created me a new account. Medium also don't require a password as login is all done via 'magic' email links...

@taylorotwell
Copy link
Member

It's a little odd to me to add a password to a socially connected account. Isn't that one of the main points of logging in socially? To not have to give the service your password? Does this PR allow a password authenticated user to connect a social account? I guess they would have to do that via the login screen and make sure the email on their social service matched their existing Jetstream email?

@joelbutcher
Copy link
Contributor Author

@taylorotwell I've just done quite a big refactor to the controller logic, taking into account all the above scenarios. Here's the lowdown:

Registration

  1. Registering for the first time with a provider that has an email address and a user for that email doesn't exist in the database, creates a new user and logs them in.
  2. Registering with a provider that already exists, returns to /register with the following error:

Screenshot 2020-12-17 at 11 39 41

  1. Registering with a provider with no email associated with the account returns to /register with the following error:

Screenshot 2020-12-17 at 11 39 28

Login

  1. Attempting to log in using a provider that doesn't exist for the account ID and provider name, returns to /login with the following error:

Screenshot 2020-12-17 at 11 39 53

  1. Attempting to log in using a provider that does exist for the account ID and provider name, fetches the user the account is associated with in the DB and logs them in, redirecting them to the fortify.home route.

Authenticated (connecting to an existing, logged-in user)

  1. Attempting to connect an account that is already associated with another user, redirects to the profile.show route with the following danger banner error message:

Screenshot 2020-12-17 at 11 40 53

Feature Recap

All important logic for OAuth registration, login and connection can be found in \Laravel\Jetstream\Http\Controllers\SocialiteController@handleProviderCallback (method name subject to change, if requested)

  • Alters the create_users_table migration to set the password field as nullable, if the Jetstream is installed with the --socialite option.
  • Registering via OAuth creates a new user using the email address on the provider account (provider accounts with no email address associated to them cannot register)
    • If Jetstream is installed with --socialite AND --teams options, registering also creates a personal team.
  • If you've registered via OAuth and NOT set a password, all forms that require a password in profile.show are hidden/disabled (Delete Account, 2FA, Log out Browser Sessions etc.).
  • A "Set Password" form replaces the "Update Password" form.
  • Once a user has set a password using the new "Set Password" form, all previously hidden/disabled forms are available (Delete Account, 2FA, Log out Browser Sessions etc.)

Update

  • Users can update their email at any time, regardless of whether they've sign up with an OAuth provider.

@irakan
Copy link

irakan commented Dec 17, 2020

@joelbutcher keep up the good work.. one thing I think is missing here is the email verification...

@taylorotwell already mentioned this:

Also, what if email verification is enabled for email based users? What happens to social users?

When creating a user I think the email_verified_at field should be filled since the social platform has verified it already for us...

@joelbutcher
Copy link
Contributor Author

@irakan yup, all done ✅

@taylorotwell
Copy link
Member

I just can't really take on the maintenance of this right now. Sorry - I may try to implement it myself for some future 3.x release.

@joelbutcher
Copy link
Contributor Author

@taylorotwell well I'd be lying if I said I wasn't a tad annoyed considering the number of people who have helped me get this PR to the stage it's in, the discussions taken place etc. But I understand the maintenance burden this also poses for you and the team.

If it's okay with you guys, I think I'll look into sticking this into a package to compliment Jetstream.

Thanks for taking the time out for the discussions on this, valuable questions asked and lessons learned whilst developing this.

@jwahdatehagh
Copy link

@joelbutcher - your patience in this was astounding. Hats off to you - and of course go ahead and stick it into a package - i wouldn't think you need anyones permission to do that 😅

@joelbutcher
Copy link
Contributor Author

@jwahdatehagh thanks! A lot of the patience was only face value - it is quite frustrating to work for a month on something that ends up not being used, or effectively being told the work isn't good enough.

A couple of changes I'm thinking of making before I release this as a package include:

  • Moving the controller logic to a published action.
  • Utilising Authorization exceptions better.

What are peoples thoughts?

@driesvints
Copy link
Member

driesvints commented Dec 21, 2020

@joelbutcher We're very sorry about this but as Taylor said this would place quite a big maintenance burden on us. The reason why it took this long was because we were genuinely considering this to be merged into the upcoming 2.x release until the very end.

it is quite frustrating to work for a month on something that ends up not being used, or effectively being told the work isn't good enough.

Unfortunately this is also part of open source. We as maintainers need to decide how much work we want to take upon us while also still being able to put in work into all our other projects and apps. This means making these tough decisions. Also: nobody said your PR isn't good enough. Imo this PR looks good and definitely could be released as a third party package.

As a closing note I want to say to everyone who's disappointed that some PRs aren't getting merged: Jetstream is completely open source. Feel free to fork the package and start a "Jetstream Extended" package yourself if you're willing to take up the maintenance work of all the extra features.

@FoksVHox
Copy link

@driesvints Could you elaborate on this?

this would place quite a big maintenance burden on us. The reason why it took this long was because we were genuinely considering this to be merged into the upcoming 2.x release until the very end.

Personally, right now I don't see how this could be "a big maintenance burden" as it's an integration with a 1st party package.

@driesvints
Copy link
Member

@FoksVHox this adds about 1900 extra lines of code.

@FoksVHox
Copy link

this adds about 1900 extra lines of code.

@driesvints While that being true, I doubt that those lines need to be changed frequently, but anyhow thanks for the reply.

@dillingham
Copy link
Contributor

@joelbutcher while Im sure it stung, it wasn't for nothing. Alot was discovered and I think this will be a very popular package. I for one am very looking forward to it and happy to contribute as best I can.

@taylorotwell Im not envious of having to make the hard call. But it does seem like tons of edge cases and might be rushed for an upcoming release. Tons of blow back potential on twitter if things werent smooth enough lol

@driesvints if jetstream was a little more extensible, might not need to maintain a fork. For example, I think the only thing this oAuth package would need is for Jetstream to have some blade @stack for views (laravel/ideas#2439) and a Jetstream.booting((Vue) => {}) to allow adding components to the Jetstream Vue instance (Like nova does). This would make it easy to displace the maintenance burden while also testing these features in the wild and discovering bugs before making them official.

@driesvints
Copy link
Member

@dillingham I'm not sure if that's a viable solution as you cannot guarantee the order of elements from the views you're stacking to/from. Also: it's not guaranteed that the source code from the parts you're extending from remains the same.

@dillingham
Copy link
Contributor

dillingham commented Dec 21, 2020

@driesvints fair point. In the event of the order not being exact, maybe the consumer can opt into a ->pushStackOrder method. To the second point, yeah would have to avoid getting too specific with the stacks. But login-form.after would always be there, as would profile-panels.after

Just food for thought, Im sure adding a few components to views wouldn't be that complicated for users.

@joelbutcher
Copy link
Contributor Author

@taylorotwell @driesvints do you guys have any plans to open GitHub Discussions on this repo? Would give a central location for everyone to talk about this feature.

Side note: I've done the Livewire version for Jetstream v1.6, but I'm getting an issue in Inertia stating that props is undefined when attempting to retrieve $page.props.jetstream and $page.props.user - any ideas?

Thanks in advance!

@driesvints
Copy link
Member

No plans for discussions sorry. We tested it on the framework repo but felt it wasn't exactly working in the way we'd like. We won't be rolling it out on other repos.

The inertia error is probably because you updated to a newer inertia version that 1.x doesn't supports. V2 will fix this.

@joelbutcher
Copy link
Contributor Author

Just issued the first pre-release version v0.0.1 of this package here.

I intend to ship v1 on Monday and work on v2 for Jetstream v2 after that. If you have any issues, hit me up in Discord or add the issue to the repo

@driesvints
Copy link
Member

Great work on that @joelbutcher 👍

@MohannadNaj
Copy link

Just sharing: I was listening to Taylor's Q&A at PHPKonf 2020 (the video was uploaded at Dec 28, 2020), where he referred to this PR -at 00:20:40- as potentially getting merged. The Socialstream package looks really great, can't wait to start using it.. Thank you @joelbutcher for the great work!

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.

None yet