chrisreedio / socialment

Socialite OAuth Support for Filament
MIT License
73 stars 10 forks source link

[Bug]: Canceled login request triggers an error #11

Closed ketchalegend closed 8 months ago

ketchalegend commented 9 months ago

What happened?

When a user tries to signup then later decides to return back by canceling the request for example when using twitter. He gets an error.

I fixed this by adding this line to the callback function in SocialmentController

        if (request()->has('denied')) {
            // Handle the denied access case here, such as showing an error message or redirecting
            return redirect()->route('filament.app.auth.login');
        }

I added this too to make it easier for users to create an account.

        if (! $connectedAccount->exists) {
            // Check if a user with the same email already exists
            $user = User::where('email', $socialUser->getEmail())->first();

            if (!$user) {
                // If the user doesn't exist, create a new one
                $user = User::create([
                    'name' => $socialUser->getName(),
                    'email' => $socialUser->getEmail(),
                    'company' => '',
                    // You can set other user attributes here as needed
                ]);
            }

            // Associate the user with the connected account
            $connectedAccount->user()->associate($user)->save();
        }

Also you could edit the documentation and tell the user to add routes to web.php because the ones in your package doesn't work, atleast i guess because i defined another path. Not everyone will know that he has to use /login/{provider}/callback because the standard is /auth/{provider}/callback which is important for the redirect url.

Route::get('/auth/{provider}', [SocialmentController::class, 'redirect'])->name('socialment.redirect');
Route::get('/auth/{provider}/callback', [SocialmentController::class, 'callback'])->name('socialment.callback');

Also you could mention that users have to add credentials to config/services.php or else it won't work.

How to reproduce the bug

I explained above how to reproduce the issue. I could not do a pull request. But its a great package.

Package Version

v3.0.0-beta

PHP Version

8.2.0

Laravel Version

10.0.0

Which operating systems does with happen with?

No response

Notes

No response

ketchalegend commented 9 months ago

Those are suggestions what to fix. I had to extend the controller with my changes.

chrisreedio commented 9 months ago

Thanks @ketchalegend !

I have updated the README to address some of your input/concerns. Let me know if you have any feedback on the new section.

I haven't had time to really review the code changes yet but wanted to get those documentation changes included.

Really appreciate you checking out my plugin and greatly appreciate the feedback!

I will review the code suggestions this week and get back to you!

chrisreedio commented 9 months ago

So I was just doing some work on of my projects that use Socialment and I ran into this exact thing. :D

What @ketchalegend proposed is definitely the right direction. Will probably work on this and get a new version out shortly that addresses this.

chrisreedio commented 9 months ago

@ketchalegend

I have implemented the existing user handling code in the latest v3.0.1-beta release.

I am going to give some more thought to the code that handles denied requests. I agree this needs to be improved but I want to make sure we find a solution that works for most (if not all) providers.

I usually only test with a small set of providers. If you (@ketchalegend) use a much larger variety of providers can you test and report back which providers you confirm working with the following code you provided?

        if (request()->has('denied')) {
            // Handle the denied access case here, such as showing an error message or redirecting
            return redirect()->route('filament.app.auth.login');
        }
ketchalegend commented 9 months ago

@chrisreedio I've taken a look at what you've done so far, and it looks good. I'll be conducting a more thorough review with additional providers and will keep you updated. Thanks for your efforts in developing this plugin! 🚀

I did come across one issue while testing it with multiple providers. On the login page, there doesn't seem to be a grid, which causes a problem when there are more than three providers. Instead of aligning neatly below the first three, it spills over the page. I'm not sure if this makes sense to you, but I wanted to bring it to your attention.

In providers-list.blade.php i did add this

<section class='filalite-panel mt-4'>
    <div class="relative flex items-center col-span-3 pt-3 pb-3" style="padding-bottom: 20px;">
        <div class="flex-grow border-t border-gray-400"></div>
        <span class="flex-shrink mx-4 text-gray-400 px-4 ">
            {{ config('socialment.view.prompt', 'Or Login Via') }}
        </span>
        <div class="flex-grow border-t border-gray-400"></div>
    </div>

    <div class="grid grid-cols-3 gap-6">
        @foreach ($providers as $providerName => $provider)
            <a class='ring-2 ring-slate-700/50 hover:ring-slate-600/70 transition-all rounded-lg px-4 py-3 flex gap-2 items-center'
                href='{{ route('socialment.redirect', $providerName) }}'>
                <x-icon name="{{ $provider['icon'] }}" class='w-8' />
                <span>{{ $provider['label'] }}</span>
            </a>
        @endforeach
    </div>
</section>
chrisreedio commented 8 months ago

A fix for this is in progress btw.

chrisreedio commented 8 months ago

@ketchalegend This should be fixed now.