Repository: hwi/HWIOAuthBundle (php)

Issue:
Since 2.2.0 firewalls failure_handler gets overriden
| Q                 | A
| ------------------| ---
| Bug?              | yes
| New Feature?      | no
| Support question? | yes
| Version           | 2.2.0


#### Actual Behavior

What is the actual behavior?
Our custom firewall `failure_handler` gets replaced. This makes it impossible to have any control over SSO behavior, like custom return domains.

We need to control the domain part of the redirect URL (multi tenant react app).
How can I do that if the configured `failure_handler` gets ignored?

#### Expected Behavior

What is the behavior you expect?
When configured, the preferred failure handler should be used.

#### Steps to Reproduce

Configure `failure_handler` on firewall config, call login-check endpoint with empty code= parameter, observe custom handler being ignored and hwi_oauth.authentication.failure_handler being used.


For the time being, I had to overwrite hwi_oauth.authentication.failure_handler service with our own implementation, but this is neither documented, or supported for multiple firewalls.


Pull request:
Prevent overwriting `failure_path` in `AuthenticationFailureHandler`
Fixes #1989

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index e07e8d6..287c8d8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,9 @@
 Changelog
 =========
+## 2.2.1 (2024-03-xx)
+* Bugfix: Prevent overwriting `failure_path` in `AuthenticationFailureHandler` when connect functionality is not enabled,
+* Bugfix: Prevent overwriting `failure_handler` in security configuration if set,
+
 ## 2.2.0 (2024-02-28)
 * BC Break: Dropped support for PHP 7.4 & 8.0,
 * Added: Telegram resource owner,
diff --git a/src/DependencyInjection/Security/Factory/OAuthAuthenticatorFactory.php b/src/DependencyInjection/Security/Factory/OAuthAuthenticatorFactory.php
index 28a5b14..615988b 100644
--- a/src/DependencyInjection/Security/Factory/OAuthAuthenticatorFactory.php
+++ b/src/DependencyInjection/Security/Factory/OAuthAuthenticatorFactory.php
@@ -138,6 +138,10 @@ final class OAuthAuthenticatorFactory extends AbstractFactory implements Authent
     protected function createAuthenticationFailureHandler(ContainerBuilder $container, string $id, array $config): string
     {
         $id = $this->getFailureHandlerId($id);
+        if ($container->has($id)) {
+            return $id;
+        }
+
         $options = array_intersect_key($config, $this->defaultFailureHandlerOptions);
 
         $failureHandler = $container->setDefinition($id, new ChildDefinition('security.authentication.custom_failure_handler'));
diff --git a/src/Resources/config/oauth.php b/src/Resources/config/oauth.php
index 596079c..ab03d0a 100644
--- a/src/Resources/config/oauth.php
+++ b/src/Resources/config/oauth.php
@@ -69,6 +69,7 @@ return static function (ContainerConfigurator $containerConfigurator): void {
         ->args([
             service('request_stack'),
             service('router'),
+            service('security.http_utils'),
             '%hwi_oauth.connect%',
         ]);
 };
diff --git a/src/Security/Http/Authentication/AuthenticationFailureHandler.php b/src/Security/Http/Authentication/AuthenticationFailureHandler.php
index 73ea0e8..c192290 100644
--- a/src/Security/Http/Authentication/AuthenticationFailureHandler.php
+++ b/src/Security/Http/Authentication/AuthenticationFailureHandler.php
@@ -14,7 +14,6 @@ declare(strict_types=1);
 namespace HWI\Bundle\OAuthBundle\Security\Http\Authentication;
 
 use HWI\Bundle\OAuthBundle\Security\Core\Exception\AccountNotLinkedException;
-use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\RequestStack;
 use Symfony\Component\HttpFoundation\Response;
@@ -22,12 +21,13 @@ use Symfony\Component\HttpFoundation\Session\SessionInterface;
 use Symfony\Component\Routing\RouterInterface;
 use Symfony\Component\Security\Core\Exception\AuthenticationException;
 use Symfony\Component\Security\Http\Authentication\AuthenticationFailureHandlerInterface;
+use Symfony\Component\Security\Http\HttpUtils;
 use Symfony\Component\Security\Http\ParameterBagUtils;
 
 final class AuthenticationFailureHandler implements AuthenticationFailureHandlerInterface
 {
     private array $defaultOptions = [
-        'failure_path' => 'hwi_oauth_connect_registration',
+        'failure_path' => null,
         'failure_forward' => false,
         'login_path' => '/login',
         'failure_path_parameter' => '_failure_path',
@@ -36,8 +36,11 @@ final class AuthenticationFailureHandler implements AuthenticationFailureHandler
     public function __construct(
         private readonly RequestStack $requestStack,
         private readonly RouterInterface $router,
+        private readonly HttpUtils $httpUtils,
         private readonly bool $connect,
+        array $options = [],
     ) {
+        $this->setOptions($options);
     }
 
     public function setOptions(array $options): void
@@ -58,7 +61,7 @@ final class AuthenticationFailureHandler implements AuthenticationFailureHandler
         $options['failure_path'] ??= $options['login_path'];
 
         $error = $exception->getPrevious();
-
+        $key = null;
         if ($this->connect && $error instanceof AccountNotLinkedException) {
             $key = time();
             $session = $request->hasSession() ? $request->getSession() : $this->getSession();
@@ -69,29 +72,17 @@ final class AuthenticationFailureHandler implements AuthenticationFailureHandler
 
                 $session->set('_hwi_oauth.registration_error.'.$key, $error);
             }
-
-            if ('/' === $options['failure_path'][0]) {
-                $failurePath = $request->getUriForPath($options['failure_path'][0]);
-            } else {
-                $failurePath = $this->router->generate($options['failure_path'], ['key' => $key]);
-            }
-
-            return new RedirectResponse($failurePath);
-        }
-
-        if ($error instanceof AuthenticationException) {
-            $error = $error->getMessageKey();
-        } else {
-            $error = $exception->getMessageKey();
         }
 
-        if ('/' === $options['login_path'][0]) {
-            $loginPath = $request->getUriForPath($options['login_path'][0]);
+        if (null !== $key) {
+            $failurePath = $this->router->generate($options['failure_path'], ['key' => $key]);
+        } elseif ('/' === $options['failure_path'][0]) {
+            $failurePath = $options['failure_path'];
         } else {
-            $loginPath = $this->router->generate($options['login_path'], ['error' => $error]);
+            $failurePath = $this->router->generate($options['failure_path']);
         }
 
-        return new RedirectResponse($loginPath);
+        return $this->httpUtils->createRedirectResponse($request, $failurePath);
     }
 
     private function getSession(): ?SessionInterface
diff --git a/src/Security/Http/Authenticator/OAuthAuthenticator.php b/src/Security/Http/Authenticator/OAuthAuthenticator.php
index 8a566bd..0036950 100644
--- a/src/Security/Http/Authenticator/OAuthAuthenticator.php
+++ b/src/Security/Http/Authenticator/OAuthAuthenticator.php
@@ -128,6 +128,7 @@ final class OAuthAuthenticator implements AuthenticatorInterface, Authentication
 
         $userBadge = class_exists(UserBadge::class)
             ? new UserBadge(
+                /* @phpstan-ignore-next-line */
                 $user ? method_exists($user, 'getUserIdentifier') ? $user->getUserIdentifier() : $user->getUsername() : null,
                 static function () use ($user) { return $user; }
             )


Test patch:
diff --git a/tests/Security/Http/Authentication/AuthenticationFailureHandlerTest.php b/tests/Security/Http/Authentication/AuthenticationFailureHandlerTest.php
index 33e4afc..260fa59 100644
--- a/tests/Security/Http/Authentication/AuthenticationFailureHandlerTest.php
+++ b/tests/Security/Http/Authentication/AuthenticationFailureHandlerTest.php
@@ -21,8 +21,10 @@ use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\RequestStack;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpFoundation\Session\SessionInterface;
+use Symfony\Component\Routing\Matcher\UrlMatcherInterface;
 use Symfony\Component\Routing\RouterInterface;
 use Symfony\Component\Security\Core\Exception\AuthenticationException;
+use Symfony\Component\Security\Http\HttpUtils;
 
 final class AuthenticationFailureHandlerTest extends TestCase
 {
@@ -35,6 +37,7 @@ final class AuthenticationFailureHandlerTest extends TestCase
         $handler = new AuthenticationFailureHandler(
             $requestStack,
             $router = $this->createMock(RouterInterface::class),
+            new HttpUtils($router, $this->createMock(UrlMatcherInterface::class)),
             false
         );
 
@@ -56,13 +59,14 @@ final class AuthenticationFailureHandlerTest extends TestCase
         $handler = new AuthenticationFailureHandler(
             $requestStack,
             $router = $this->createMock(RouterInterface::class),
+            new HttpUtils($router, $this->createMock(UrlMatcherInterface::class)),
             false,
         );
         $handler->setOptions(['login_path' => 'route_name']);
 
         $router->expects($this->once())
             ->method('generate')
-            ->with('route_name', ['error' => 'An authentication exception occurred.'], 1)
+            ->with('route_name', [], 1)
             ->willReturn('/path');
 
         $this->assertInstanceOf(
@@ -80,6 +84,7 @@ final class AuthenticationFailureHandlerTest extends TestCase
         $handler = new AuthenticationFailureHandler(
             $requestStack,
             $router = $this->createMock(RouterInterface::class),
+            new HttpUtils($router, $this->createMock(UrlMatcherInterface::class)),
             false
         );
 
@@ -101,6 +106,7 @@ final class AuthenticationFailureHandlerTest extends TestCase
         $handler = new AuthenticationFailureHandler(
             $requestStack,
             $router = $this->createMock(RouterInterface::class),
+            new HttpUtils($router, $this->createMock(UrlMatcherInterface::class)),
             true
         );
 
@@ -124,7 +130,9 @@ final class AuthenticationFailureHandlerTest extends TestCase
         $handler = new AuthenticationFailureHandler(
             $requestStack,
             $router = $this->createMock(RouterInterface::class),
-            true
+            new HttpUtils($router, $this->createMock(UrlMatcherInterface::class)),
+            true,
+            ['failure_path' => 'hwi_oauth_connect_registration']
         );
 
         $router->expects($this->once())