adamwathan / eloquent-oauth

Braindead simple social login with Laravel and Eloquent.
370 stars 44 forks source link

Allow overriding user by returning a user from the callback #79

Closed adamwathan closed 9 years ago

adamwathan commented 9 years ago

This PR allows overriding the user to be logged in by returning a user from the optional closure passed to login().

For example, if you supported multiple social login methods and wanted to match users based on email to prevent duplicate accounts, you could do something like this:

SocialAuth::login($provider, function ($user, $details) {
    $existingUser = User::where('email', $details->email)->first();

    if ($existingUser) {
        return $existingUser;
    }

    $user->email = $details->email;
});

If you manually return a different user than the package finds/builds for you, no additional user will get created in the database.

I still believe matching users based on email is a significant security vulnerability and I wouldn't recommend it, but I'm a libertarian, so do what you will :)

adamwathan commented 9 years ago

This addresses some discussion in #41. It's a manual process but I think that's as far as I'd like to take it.

dusterio commented 8 years ago

Does this actually work? I don't think it does, look at this part of Authenticator.php (lines 19+):

        if ($callback) {
            $callback($user, $userDetails);
        }

Returned values from callback function are not used at all at the moment. I tested – an empty user will be created in case code like above is used.

If you do something like this it starts working:

    public function login($providerAlias, $userDetails, $callback = false)
    {
        $user = $this->getUser($providerAlias, $userDetails);
        if ($callback) {
            $existingUser = $callback($user, $userDetails);

            if (!is_null($existingUser)) $user = $existingUser;
        }
        $this->updateUser($user, $providerAlias, $userDetails);
        $this->auth->login($user);
    }

Would appreciate if you could update this in the future release. Don't want to fork it because of just a few lines of code ;-)

adamwathan commented 8 years ago

@dusterio: The code is there, I just hadn't tagged a release yet.

https://github.com/adamwathan/eloquent-oauth/blob/master/src/Authenticator.php#L32-L37

Tagged now, so if you update to 0.4.0 of eloquent-oauth-l5 and 8.1 for eloquent-oauth, you'll get the new code.

dusterio commented 8 years ago

@adamwathan Oh you are right, thanks!