From 78762b52f3dadaf4676e745a6d48a7b1732492af Mon Sep 17 00:00:00 2001 From: Joseph Bielawski Date: Fri, 1 Mar 2024 17:00:30 +0100 Subject: [PATCH 1/2] Prevent overwriting `failure_path` in `AuthenticationFailureHandler` --- CHANGELOG.md | 3 ++ src/Resources/config/oauth.php | 1 + .../AuthenticationFailureHandler.php | 33 +++++++------------ .../Http/Authenticator/OAuthAuthenticator.php | 1 + .../AuthenticationFailureHandlerTest.php | 12 +++++-- 5 files changed, 27 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e07e8d60d..c261e980e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ Changelog ========= +## 2.2.1 (2024-03-xx) +* Bugfix: Prevent overwriting `failure_path` in `AuthenticationFailureHandler` when connect functionality is not enabled + ## 2.2.0 (2024-02-28) * BC Break: Dropped support for PHP 7.4 & 8.0, * Added: Telegram resource owner, diff --git a/src/Resources/config/oauth.php b/src/Resources/config/oauth.php index 596079c70..ab03d0a60 100644 --- a/src/Resources/config/oauth.php +++ b/src/Resources/config/oauth.php @@ -69,6 +69,7 @@ ->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 73ea0e8a2..c1922907b 100644 --- a/src/Security/Http/Authentication/AuthenticationFailureHandler.php +++ b/src/Security/Http/Authentication/AuthenticationFailureHandler.php @@ -14,7 +14,6 @@ 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\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 @@ public function onAuthenticationFailure(Request $request, AuthenticationExceptio $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 @@ public function onAuthenticationFailure(Request $request, AuthenticationExceptio $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 8a566bda3..00369504f 100644 --- a/src/Security/Http/Authenticator/OAuthAuthenticator.php +++ b/src/Security/Http/Authenticator/OAuthAuthenticator.php @@ -128,6 +128,7 @@ public function authenticate(Request $request): Passport $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; } ) diff --git a/tests/Security/Http/Authentication/AuthenticationFailureHandlerTest.php b/tests/Security/Http/Authentication/AuthenticationFailureHandlerTest.php index 33e4afc25..260fa5988 100644 --- a/tests/Security/Http/Authentication/AuthenticationFailureHandlerTest.php +++ b/tests/Security/Http/Authentication/AuthenticationFailureHandlerTest.php @@ -21,8 +21,10 @@ 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 @@ public function testRedirectsToLoginPathWhenNotSet() $handler = new AuthenticationFailureHandler( $requestStack, $router = $this->createMock(RouterInterface::class), + new HttpUtils($router, $this->createMock(UrlMatcherInterface::class)), false ); @@ -56,13 +59,14 @@ public function testRedirectsToLoginRoute() $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 @@ public function testDoesNothingWhenConnectIsDisabled() $handler = new AuthenticationFailureHandler( $requestStack, $router = $this->createMock(RouterInterface::class), + new HttpUtils($router, $this->createMock(UrlMatcherInterface::class)), false ); @@ -101,6 +106,7 @@ public function testDoesNothingWhenExceptionIsNotOAuthOne() $handler = new AuthenticationFailureHandler( $requestStack, $router = $this->createMock(RouterInterface::class), + new HttpUtils($router, $this->createMock(UrlMatcherInterface::class)), true ); @@ -124,7 +130,9 @@ public function testRedirectsToRegistrationPage() $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()) From caf319abc65b6194c4eb17553d65157a988d3a98 Mon Sep 17 00:00:00 2001 From: Joseph Bielawski Date: Fri, 1 Mar 2024 17:25:01 +0100 Subject: [PATCH 2/2] Prevent overwriting `failure_handler` in security configuration if set --- CHANGELOG.md | 3 ++- .../Security/Factory/OAuthAuthenticatorFactory.php | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c261e980e..287c8d8b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,8 @@ Changelog ========= ## 2.2.1 (2024-03-xx) -* Bugfix: Prevent overwriting `failure_path` in `AuthenticationFailureHandler` when connect functionality is not enabled +* 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, diff --git a/src/DependencyInjection/Security/Factory/OAuthAuthenticatorFactory.php b/src/DependencyInjection/Security/Factory/OAuthAuthenticatorFactory.php index 28a5b1419..615988b46 100644 --- a/src/DependencyInjection/Security/Factory/OAuthAuthenticatorFactory.php +++ b/src/DependencyInjection/Security/Factory/OAuthAuthenticatorFactory.php @@ -138,6 +138,10 @@ public function getFirewallNames(): \ArrayIterator 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'));