-
Notifications
You must be signed in to change notification settings - Fork 806
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
[2.x] Socialite Support #444
Conversation
stubs/livewire/resources/views/profile/update-profile-information-form.blade.php
Outdated
Show resolved
Hide resolved
stubs/livewire/resources/views/profile/remove-connected-accounts-form.blade.php
Outdated
Show resolved
Hide resolved
@joelbutcher why did you close this? |
@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 |
@driesvints I've addressed the changes requests in the latest. Once again apologies for the mess-up with Git! |
@joelbutcher no worries. This is a fantastic PR tbh 👍 |
@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! |
Have you considered instead of having a separate configuration item doing this: Features::socialite(['providers' => ['facebook', 'github']]) |
@taylorotwell I hadn't, but it wouldn't take too much to implement this. Edit: Never mind, just realised what you mean - similar to how teams can pass options. Will do this now! |
@taylorotwell Just pushed up that change 👍 |
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? |
Also, what if email verification is enabled for email based users? What happens to social users? |
And what about "reset password"? (maybe I missed the convo about that one) |
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) |
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. |
@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. |
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? |
@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
At the end of the day, I'm happy, willing, and able to implement whatever the general consensus is... |
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? |
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. |
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. |
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... |
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? |
@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
Login
Authenticated (connecting to an existing, logged-in user)
Feature RecapAll important logic for OAuth registration, login and connection can be found in
Update
|
@joelbutcher keep up the good work.. one thing I think is missing here is the email verification... @taylorotwell already mentioned this:
When creating a user I think the |
@irakan yup, all done ✅ |
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. |
@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. |
@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 😅 |
@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:
What are peoples thoughts? |
@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.
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. |
@driesvints Could you elaborate on this?
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. |
@FoksVHox 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. |
@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 |
@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. |
@driesvints fair point. In the event of the order not being exact, maybe the consumer can opt into a Just food for thought, Im sure adding a few components to views wouldn't be that complicated for users. |
@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 Thanks in advance! |
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. |
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 |
Great work on that @joelbutcher 👍 |
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! |
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 yourcomposer.json
to include the repository following and require the feature branch.You may then run the Jetstream install command, specifying socialite support:
You may remove support for Socialite by commenting out the feature in
jetstream.php
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
andregister
routes views:Existing users that have signed up using email and password can connect social accounts in their profile settings, as well as remove them: