joelbutcher / socialstream

OAuth for Laravel, simplified.
https://docs.socialstream.dev
MIT License
433 stars 66 forks source link

[5.x] Fix user being able to link account without loginOnRegistration() #332

Closed miguilimzero closed 10 months ago

miguilimzero commented 10 months ago

The refactoring from https://github.com/joelbutcher/socialstream/pull/325 flushed the OAuth logic again, making it very hard to understand how the authentication flow works. On top of that, it introduced a vulnerability/bug where the user can link a provider account to an existing account without the loginOnRegistration() feature enabled.

To reproduce, the system must have Features::globalLogin() and Features::createAccountOnFirstLogin() enabled, and the user needs to authenticate using a non-linked provider from a random route.

How can it be better organized? (From my point of view)

The first point I observed is that Oauth is using multiple points to create an account (or provider account) again. You can see that $this->createsConnectedAccounts->create() is used three times and $this->register() two times, opening gaps for problems like the one that was identified.

You may also see that $this->login() has the responsibility to "create the connected account", while it is being used on $this->register which already has this responsibility... It is very difficult to understand when creating the account will be used at each point.

We should keep the flow simple and reduce the times the code can create or link an account. This PR also aims to simplify this flow again.

miguilimzero commented 10 months ago

I think the name loginOnRegistration is also very confusing from what it does. I get lost sometimes when trying to understand what is happening. I think it should be something like createProviderAccountOnLogin.

miguilimzero commented 10 months ago

Another thing that caught my attention is the login failure and registered failure event and redirect. Aren't they inverted?

It doesn't seem so from the old version, but I preserved the same logic when they should be triggered while refactoring.