Skip to content
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

SAML2 compatibility #101

Closed
juliangums opened this issue Jun 20, 2024 · 10 comments
Closed

SAML2 compatibility #101

juliangums opened this issue Jun 20, 2024 · 10 comments

Comments

@juliangums
Copy link
Contributor

I tried to implement a simple integration of the SAML2 provider: https://socialiteproviders.com/Saml2/
I can see the button
Screenshot 2024-06-20 at 14 00 03
but it takes me to /admin/oauth/saml2 which results in a 404. I don't even understand where the admin part comes from as that isn't my panel url. Did I miss anything setup-wise? I installed this package and the socialite driver and set up the provider config. I then simply added this to the panel:

            ->plugin(
                FilamentSocialitePlugin::make()
                    ->providers([
                        Provider::make('saml2')
                            ->label('SSO'),
                    ])
                    ->registration(false)
            );

and this to my EventServiceProvider

        SocialiteWasCalled::class => [
            Saml2ExtendSocialite::class.'@handle',
        ],

Are there any routes I need to set up? I was hoping this package handles that.

@bert-w
Copy link
Contributor

bert-w commented Jun 20, 2024

Can you do some debugging using php artisan route:list? The package should add the GET /$slug/oauth/{provider} route for you which handles the authentication flow.

@juliangums
Copy link
Contributor Author

This is the only one I can find: admin/oauth/{provider}

socialite.filament.admin.oauth.redirect › DutchCodingCompany\FilamentSocialite › SocialiteLoginController@redirectToProvider

@bert-w
Copy link
Contributor

bert-w commented Jun 20, 2024

Yes,so the route seems to be defined. Please check carefully where the 404 comes from because this route will redirect you to the external provider. (btw the "admin" prefix comes from your Filament Panel ID if im correct; it can be changed with the ->slug() function on the plugin).

@juliangums
Copy link
Contributor Author

juliangums commented Jun 20, 2024

@bert-w, thanks for your help. I changed the slug to "auth" to test it out, but I was still having the same issue.

The route is set up correctly. The package recognises that the saml2 provider is registered, otherwise, it would throw an error indicating it's not configured.

After some debugging, I found that the issue occurs here: https://github.com/DutchCodingCompany/filament-socialite/blob/main/src/Http/Controllers/SocialiteLoginController.php#L36-L42. I commented out ->with() and ->scopes(), so I knew it seems to be the ->redirect() that's causing the 404 error.

To rule out any issues with the driver, I tried a few things and I worked out that it works if I add one of these two routes in my web routes file:

Route::get('/auth/callback', function () {
    $user = Socialite::driver('saml2')->user();
});

Route::post('/auth/callback', function () {
    $user = Socialite::driver('saml2')->user();
});

Now, I'm not getting a 404 anymore, but I'm encountering an incompatible type hint error:

DutchCodingCompany\FilamentSocialite\Http\Controllers\SocialiteLoginController::redirectToProvider(): Return value must be of type Illuminate\Http\RedirectResponse, Symfony\Component\HttpFoundation\RedirectResponse returned

After removing the return type here https://github.com/DutchCodingCompany/filament-socialite/blob/main/src/Http/Controllers/SocialiteLoginController.php#L27 , I finally get redirected to the identity provider. But I still have two questions:

  1. I'm still unsure why I have to define /auth/callback, as I thought this package handles it. Do I need to write my own route and own logic like here: https://laravel.com/docs/11.x/socialite#authentication-and-storage?

  2. Is the type hint wrong in this package or the saml2 socialite provider?

@juliangums
Copy link
Contributor Author

juliangums commented Jun 20, 2024

I guess for the return type as one extends the other and it seems to be working with the wider one, I could submit a PR for that if you like if it isn't breaking anything.

@bramr94
Copy link
Collaborator

bramr94 commented Jun 21, 2024

Maybe the redirect URL is incorrect, can you verify it is the same as this:

https://example.com/oauth/callback/saml2

Since you are getting a 404 the redirect URL might be incorrect.

@juliangums
Copy link
Contributor Author

juliangums commented Jun 21, 2024

@bramr94 I see. I played around with the routes again. If I register something at /auth/callback as described above like this it works:

Route::get('/auth/callback', function () {
    $user = Socialite::driver('saml2')->user();
});

But if I change it to /oauth/callback/saml2 it does not. Where do I set this up through?

@juliangums
Copy link
Contributor Author

I can see it's hardcoded in this package so it can't be changed. I'll check if I find something in the saml2 driver package
https://github.com/DutchCodingCompany/filament-socialite/blob/main/routes/web.php#L28

@juliangums
Copy link
Contributor Author

Got it now. I had to define it with the correct sp_acs value

    'saml2' => [
        'metadata' => '...',
        'sp_acs' => '/oauth/callback/saml2',
    ],

The return type is still an issue though. Do you think it should be changed here or on the SAML2 socialite provider?

@juliangums
Copy link
Contributor Author

I can't see how I can change it in the SAML2 driver repo, but I also think it could be a breaking change. Someone else might be checking for the parent class. So I think it would be best to change it in this package. Are you ok with that? I prepared a pull request for that: #103

@bert-w bert-w closed this as completed Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants