DutchCodingCompany / filament-socialite

Add OAuth login through Laravel Socialite to Filament.
MIT License
137 stars 37 forks source link

wrong user logged in when using multiple models #73

Closed iotron closed 4 months ago

iotron commented 4 months ago

https://github.com/DutchCodingCompany/filament-socialite/blob/353346233cb37f022bad80665a3e8519e4679ca4/src/Models/SocialiteUser.php#L27 Since its a belongsTo relationship instead of a morphTo relation, when I use the same social to login to different panels with different user class, It fetches the user_id but not the class type hence logs the user into a different account. (Huge security issue). The model class should also be used while fetching users.

       ->plugin(
                FilamentSocialitePlugin::make()

                    ->setProviders([
                        'google' => [
                            'label' => 'Google',
                            // Custom icon requires an additional package, see below.
                            // 'icon' => 'fab-github',
                            // (optional) Button color override, default: 'gray'.
                            'color' => 'danger',
                            // (optional) Button style override, default: true (outlined).
                            'outlined' => true,
                        ],
                        'facebook' => [
                            'label' => 'Facebook',
                            // Custom icon requires an additional package, see below.
                            // 'icon' => 'fab-github',
                            // (optional) Button color override, default: 'gray'.
                            'color' => 'info',
                            // (optional) Button style override, default: true (outlined).
                            'outlined' => true,
                        ],
                    ])
                    // (optional) Enable or disable registration from OAuth.
                    ->setRegistrationEnabled(true)
                    // (optional) Change the associated model class.
                    ->setUserModelClass(\App\Models\User::class)
            )
       ->plugin(
                FilamentSocialitePlugin::make()

                    ->setProviders([
                        'google' => [
                            'label' => 'Google',
                            // Custom icon requires an additional package, see below.
                            // 'icon' => 'fab-github',
                            // (optional) Button color override, default: 'gray'.
                            'color' => 'danger',
                            // (optional) Button style override, default: true (outlined).
                            'outlined' => true,
                        ],
                        'facebook' => [
                            'label' => 'Facebook',
                            // Custom icon requires an additional package, see below.
                            // 'icon' => 'fab-github',
                            // (optional) Button color override, default: 'gray'.
                            'color' => 'info',
                            // (optional) Button style override, default: true (outlined).
                            'outlined' => true,
                        ],
                    ])
                    // (optional) Enable or disable registration from OAuth.
                    ->setRegistrationEnabled(true)
                    // (optional) Change the associated model class.
                    ->setUserModelClass(\App\Models\Host::class)
            )
iotron commented 4 months ago

created a PR for this issue https://github.com/DutchCodingCompany/filament-socialite/pull/74

bert-w commented 4 months ago

In v1.3.0 you should be able to provide your own resolution logic by creating a custom SocialiteUser class.

See this new section in the readme: https://github.com/DutchCodingCompany/filament-socialite?tab=readme-ov-file#change-how-a-socialite-user-is-created-or-retrieved

iotron commented 4 months ago

In v1.3.0 you should be able to provide your own resolution logic by creating a custom SocialiteUser class.

See this new section in the readme: https://github.com/DutchCodingCompany/filament-socialite?tab=readme-ov-file#change-how-a-socialite-user-is-created-or-retrieved

Please provide code example for a multiguard/multipanel implementation. I have tried it on my application and it cannot fetch the right user for login. The solution is here https://github.com/DutchCodingCompany/filament-socialite/pull/77. Resolve the user before everything else because multiple guards/user models can have same provider and provider_id.

<?php

namespace App\Models;

use DutchCodingCompany\FilamentSocialite\Models\Contracts\FilamentSocialiteUser as FilamentSocialiteUserContract;
use Illuminate\Contracts\Auth\Authenticatable;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\MorphTo;
use Laravel\Socialite\Contracts\User as SocialiteUserContract;

class SocialiteUser extends Model implements FilamentSocialiteUserContract
{
    protected $fillable = [
        'provider',
        'provider_id',
    ];

    public function user(): MorphTo
    {
        return $this->morphTo();
    }

    public function getUser(): Authenticatable
    {
        return $this->user;
    }

    public static function findForProvider(string $provider, SocialiteUserContract $oauthUser): ?self
    {
        return self::query()
            ->where('provider', $provider)
            ->where('provider_id', $oauthUser->getId())
            ->first();
    }

    public static function createForProvider(
        string $provider,
        SocialiteUserContract $oauthUser,
        Authenticatable $user
    ): self {
        return $user->socials()->updateOrCreate(
            ['provider' => $provider],
            ['provider_id' => $oauthUser->getId()]
        );
    }
}
bert-w commented 4 months ago

Just backtracking from your own PR, cant you use something like this:

public static function findForProvider(string $provider, SocialiteUserContract $oauthUser): ?self
    {
        $user = app()->call(FilamentSocialite::getUserResolver(), ['provider' => $provider, 'oauthUser' => $oauthUser]);

        return $user->socials()
            ->where('provider', $provider)
            ->where('provider_id', $oauthUser->getId())
            ->first();
    }

The reason why we cannot implement your morphTo suggestion by default in the migration, is because it is a major breaking change and requires everyone to update their apps. If there's a method to allow for this functionality non-invasively then that is always preferred.

The second issue we should not implement is moving around the logic in the SocialiteLoginController such that the $user object is retrieved first. This is counter-intuitive, since we want the flow to be:

oauth-login
-> process callback: fetch socialite user
-> login user THROUGH socialite user (= end of flow)
-> fall-back to an existing authenticatable
-> create missing records for user/socialite-user (= end of flow)
iotron commented 4 months ago

Just backtracking from your own PR, cant you use something like this:

public static function findForProvider(string $provider, SocialiteUserContract $oauthUser): ?self
    {
        $user = app()->call(FilamentSocialite::getUserResolver(), ['provider' => $provider, 'oauthUser' => $oauthUser]);

        return $user->socials()
            ->where('provider', $provider)
            ->where('provider_id', $oauthUser->getId())
            ->first();
    }

The reason why we cannot implement your morphTo suggestion by default in the migration, is because it is a major breaking change and requires everyone to update their apps. If there's a method to allow for this functionality non-invasively then that is always preferred.

The second issue we should not implement is moving around the logic in the SocialiteLoginController such that the $user object is retrieved first. This is counter-intuitive, since we want the flow to be:

oauth-login
-> process callback: fetch socialite user
-> login user THROUGH socialite user (= end of flow)
-> fall-back to an existing authenticatable
-> create missing records for user/socialite-user (= end of flow)

I have implemented the multiguard as an opt in feature here. I think this feature should have first party support as well because Filament supports multiple panels/guards. Kindly check it out. https://github.com/DutchCodingCompany/filament-socialite/pull/84

The login flow for multi guard will be slightly different.

 oauth-login
 -> process callback: fetch oauth user 
 -> fetch user because same provider and id may exist for different models
 -> fetch socialite users for user with provider and id
 -> login user THROUGH socialite user (= end of flow)
 -> fall-back to an existing authenticatable
 -> create missing records for user/socialite-user (= end of flow)