From 2b94931666184dd0a92c282509d10445b1e9b949 Mon Sep 17 00:00:00 2001 From: Joseph Bielawski Date: Fri, 16 Feb 2024 16:34:34 +0100 Subject: [PATCH] Replace custom authenticator passport with custom badge usage (#1978) --- .../RedirectToServiceController.php | 25 +--- .../Factory/OAuthAuthenticatorFactory.php | 5 +- src/Resources/config/oauth.php | 4 + src/Security/Core/User/EntityUserProvider.php | 3 +- .../AuthenticationFailureHandler.php | 81 +++++++++++ .../Http/Authenticator/OAuthAuthenticator.php | 39 ++--- .../Http/EntryPoint/OAuthEntryPoint.php | 19 +-- .../RefreshAccessTokenListenerOld.php | 4 + src/Security/Http/ResourceOwnerMap.php | 17 +-- src/Security/OAuthErrorHandler.php | 12 +- src/Security/OAuthUtils.php | 27 +--- .../AuthenticationFailureHandlerTest.php | 136 ++++++++++++++++++ tests/Security/Http/RefreshOnExpireTest.php | 1 + 13 files changed, 267 insertions(+), 106 deletions(-) create mode 100644 src/Security/Http/Authentication/AuthenticationFailureHandler.php create mode 100644 tests/Security/Http/Authentication/AuthenticationFailureHandlerTest.php diff --git a/src/Controller/RedirectToServiceController.php b/src/Controller/RedirectToServiceController.php index 099d7dfd0..06585acc0 100644 --- a/src/Controller/RedirectToServiceController.php +++ b/src/Controller/RedirectToServiceController.php @@ -27,27 +27,14 @@ */ final class RedirectToServiceController { - private OAuthUtils $oauthUtils; - private DomainWhitelist $domainWhitelist; - private ResourceOwnerMapLocator $resourceOwnerMapLocator; - private ?string $targetPathParameter; - private bool $failedUseReferer; - private bool $useReferer; - public function __construct( - OAuthUtils $oauthUtils, - DomainWhitelist $domainWhitelist, - ResourceOwnerMapLocator $resourceOwnerMapLocator, - ?string $targetPathParameter, - bool $failedUseReferer, - bool $useReferer + private readonly OAuthUtils $oauthUtils, + private readonly DomainWhitelist $domainWhitelist, + private readonly ResourceOwnerMapLocator $resourceOwnerMapLocator, + private readonly ?string $targetPathParameter, + private readonly bool $failedUseReferer, + private readonly bool $useReferer ) { - $this->oauthUtils = $oauthUtils; - $this->domainWhitelist = $domainWhitelist; - $this->resourceOwnerMapLocator = $resourceOwnerMapLocator; - $this->targetPathParameter = $targetPathParameter; - $this->failedUseReferer = $failedUseReferer; - $this->useReferer = $useReferer; } /** diff --git a/src/DependencyInjection/Security/Factory/OAuthAuthenticatorFactory.php b/src/DependencyInjection/Security/Factory/OAuthAuthenticatorFactory.php index 5feeff307..91903d200 100644 --- a/src/DependencyInjection/Security/Factory/OAuthAuthenticatorFactory.php +++ b/src/DependencyInjection/Security/Factory/OAuthAuthenticatorFactory.php @@ -31,11 +31,8 @@ */ final class OAuthAuthenticatorFactory extends AbstractFactory implements AuthenticatorFactoryInterface, FirewallListenerFactoryInterface { - private \ArrayIterator $firewallNames; - - public function __construct(\ArrayIterator $firewallNames) + public function __construct(private \ArrayIterator $firewallNames) { - $this->firewallNames = $firewallNames; } /** diff --git a/src/Resources/config/oauth.php b/src/Resources/config/oauth.php index 69f7cf89c..0bd01b1f4 100644 --- a/src/Resources/config/oauth.php +++ b/src/Resources/config/oauth.php @@ -17,6 +17,7 @@ use HWI\Bundle\OAuthBundle\Security\Core\Authentication\Provider\OAuthProvider; use HWI\Bundle\OAuthBundle\Security\Core\User\EntityUserProvider; use HWI\Bundle\OAuthBundle\Security\Core\User\OAuthUserProvider; +use HWI\Bundle\OAuthBundle\Security\Http\Authentication\AuthenticationFailureHandler; use HWI\Bundle\OAuthBundle\Security\Http\EntryPoint\OAuthEntryPoint; use HWI\Bundle\OAuthBundle\Security\Http\Firewall\AbstractRefreshAccessTokenListener; use HWI\Bundle\OAuthBundle\Security\Http\Firewall\OAuthListener; @@ -59,4 +60,7 @@ '%hwi_oauth.connect%', '%hwi_oauth.grant_rule%', ]); + + $services->set('hwi_oauth.authentication.failure_handler', AuthenticationFailureHandler::class) + ->arg('$connect', '%hwi_oauth.connect%'); }; diff --git a/src/Security/Core/User/EntityUserProvider.php b/src/Security/Core/User/EntityUserProvider.php index 74a90f6f8..33b5882fe 100644 --- a/src/Security/Core/User/EntityUserProvider.php +++ b/src/Security/Core/User/EntityUserProvider.php @@ -15,6 +15,7 @@ use Doctrine\Persistence\ObjectManager; use Doctrine\Persistence\ObjectRepository; use HWI\Bundle\OAuthBundle\OAuth\Response\UserResponseInterface; +use HWI\Bundle\OAuthBundle\Security\Core\Exception\AccountNotLinkedException; use Symfony\Component\PropertyAccess\PropertyAccess; use Symfony\Component\Security\Core\Exception\UnsupportedUserException; use Symfony\Component\Security\Core\Exception\UserNotFoundException; @@ -125,7 +126,7 @@ private function findUser(array $criteria): ?UserInterface private function createUserNotFoundException(string $username, string $message): UserNotFoundException { - $exception = new UserNotFoundException($message); + $exception = new AccountNotLinkedException($message); $exception->setUserIdentifier($username); return $exception; diff --git a/src/Security/Http/Authentication/AuthenticationFailureHandler.php b/src/Security/Http/Authentication/AuthenticationFailureHandler.php new file mode 100644 index 000000000..165ce84da --- /dev/null +++ b/src/Security/Http/Authentication/AuthenticationFailureHandler.php @@ -0,0 +1,81 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +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; +use Symfony\Component\HttpFoundation\Session\SessionInterface; +use Symfony\Component\Routing\Generator\UrlGeneratorInterface; +use Symfony\Component\Routing\RouterInterface; +use Symfony\Component\Security\Core\Exception\AuthenticationException; +use Symfony\Component\Security\Http\Authentication\AuthenticationFailureHandlerInterface; +use Twig\Environment; + +final class AuthenticationFailureHandler implements AuthenticationFailureHandlerInterface +{ + public function __construct( + private readonly RequestStack $requestStack, + private readonly RouterInterface $router, + private readonly Environment $twig, + private readonly bool $connect + ) { + } + + public function onAuthenticationFailure(Request $request, AuthenticationException $exception): Response + { + $error = $exception->getPrevious(); + + if ($this->connect && $error instanceof AccountNotLinkedException) { + $key = time(); + $session = $request->hasSession() ? $request->getSession() : $this->getSession(); + if ($session) { + if (!$session->isStarted()) { + $session->start(); + } + + $session->set('_hwi_oauth.registration_error.'.$key, $error); + } + + return new RedirectResponse( + $this->router->generate('hwi_oauth_connect_registration', ['key' => $key], UrlGeneratorInterface::ABSOLUTE_PATH) + ); + } + + if ($error instanceof AuthenticationException) { + $error = $error->getMessageKey(); + } else { + $error = $exception->getMessageKey(); + } + + return new Response( + $this->twig->render('@HWIOAuth/Connect/login.html.twig', ['error' => $error]) + ); + } + + private function getSession(): ?SessionInterface + { + if (method_exists($this->requestStack, 'getSession')) { + return $this->requestStack->getSession(); + } + + if ((null !== $request = $this->requestStack->getCurrentRequest()) && $request->hasSession()) { + return $request->getSession(); + } + + return null; + } +} diff --git a/src/Security/Http/Authenticator/OAuthAuthenticator.php b/src/Security/Http/Authenticator/OAuthAuthenticator.php index 7c431d726..8a566bda3 100644 --- a/src/Security/Http/Authenticator/OAuthAuthenticator.php +++ b/src/Security/Http/Authenticator/OAuthAuthenticator.php @@ -43,38 +43,19 @@ */ final class OAuthAuthenticator implements AuthenticatorInterface, AuthenticationEntryPointInterface, InteractiveAuthenticatorInterface { - private HttpUtils $httpUtils; - private OAuthAwareUserProviderInterface $userProvider; - private ResourceOwnerMapInterface $resourceOwnerMap; - private AuthenticationSuccessHandlerInterface $successHandler; - private AuthenticationFailureHandlerInterface $failureHandler; - private HttpKernelInterface $httpKernel; - /** - * @var string[] + * @param string[] $checkPaths */ - private array $checkPaths; - - private array $options; - public function __construct( - HttpUtils $httpUtils, - OAuthAwareUserProviderInterface $userProvider, - ResourceOwnerMapInterface $resourceOwnerMap, - array $checkPaths, - AuthenticationSuccessHandlerInterface $successHandler, - AuthenticationFailureHandlerInterface $failureHandler, - HttpKernelInterface $kernel, - array $options + private readonly HttpUtils $httpUtils, + private readonly OAuthAwareUserProviderInterface $userProvider, + private readonly ResourceOwnerMapInterface $resourceOwnerMap, + private readonly array $checkPaths, + private readonly AuthenticationSuccessHandlerInterface $successHandler, + private readonly AuthenticationFailureHandlerInterface $failureHandler, + private readonly HttpKernelInterface $kernel, + private readonly array $options ) { - $this->failureHandler = $failureHandler; - $this->successHandler = $successHandler; - $this->checkPaths = $checkPaths; - $this->resourceOwnerMap = $resourceOwnerMap; - $this->userProvider = $userProvider; - $this->httpUtils = $httpUtils; - $this->httpKernel = $kernel; - $this->options = $options; } public function supports(Request $request): bool @@ -96,7 +77,7 @@ public function start(Request $request, ?AuthenticationException $authException $iterator = $request->query->getIterator(); $subRequest->query->add(iterator_to_array($iterator)); - $response = $this->httpKernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST); + $response = $this->kernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST); if (200 === $response->getStatusCode()) { $response->headers->set('X-Status-Code', '401'); } diff --git a/src/Security/Http/EntryPoint/OAuthEntryPoint.php b/src/Security/Http/EntryPoint/OAuthEntryPoint.php index 275e107bb..296fe5e61 100644 --- a/src/Security/Http/EntryPoint/OAuthEntryPoint.php +++ b/src/Security/Http/EntryPoint/OAuthEntryPoint.php @@ -28,17 +28,12 @@ */ final class OAuthEntryPoint implements AuthenticationEntryPointInterface { - private HttpKernelInterface $httpKernel; - private HttpUtils $httpUtils; - private string $loginPath; - private bool $useForward; - - public function __construct(HttpKernelInterface $kernel, HttpUtils $httpUtils, string $loginPath, bool $useForward = false) - { - $this->httpKernel = $kernel; - $this->httpUtils = $httpUtils; - $this->loginPath = $loginPath; - $this->useForward = $useForward; + public function __construct( + private readonly HttpKernelInterface $kernel, + private readonly HttpUtils $httpUtils, + private readonly string $loginPath, + private readonly bool $useForward = false + ) { } /** @@ -53,7 +48,7 @@ public function start(Request $request, ?AuthenticationException $authException $iterator = $request->query->getIterator(); $subRequest->query->add($iterator->getArrayCopy()); - $response = $this->httpKernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST); + $response = $this->kernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST); if (200 === $response->getStatusCode()) { $response->headers->set('X-Status-Code', '401'); } diff --git a/src/Security/Http/Firewall/RefreshAccessTokenListenerOld.php b/src/Security/Http/Firewall/RefreshAccessTokenListenerOld.php index e8d49da0d..477fd3870 100644 --- a/src/Security/Http/Firewall/RefreshAccessTokenListenerOld.php +++ b/src/Security/Http/Firewall/RefreshAccessTokenListenerOld.php @@ -16,9 +16,13 @@ class RefreshAccessTokenListenerOld extends RefreshAccessTokenListener { + /** + * @phpstan-ignore-next-line + */ private AuthenticationProviderInterface $oAuthProvider; public function __construct( + /* @phpstan-ignore-next-line */ AuthenticationProviderInterface $oAuthProvider ) { $this->oAuthProvider = $oAuthProvider; diff --git a/src/Security/Http/ResourceOwnerMap.php b/src/Security/Http/ResourceOwnerMap.php index f1e2ac32f..76154bdfe 100644 --- a/src/Security/Http/ResourceOwnerMap.php +++ b/src/Security/Http/ResourceOwnerMap.php @@ -26,25 +26,16 @@ */ final class ResourceOwnerMap implements ResourceOwnerMapInterface { - private HttpUtils $httpUtils; - private array $resourceOwners; - private array $possibleResourceOwners; - private ServiceLocator $locator; - /** * @param array $possibleResourceOwners array with possible resource owners names * @param array $resourceOwners array with configured resource owners */ public function __construct( - HttpUtils $httpUtils, - array $possibleResourceOwners, - array $resourceOwners, - ServiceLocator $locator + private readonly HttpUtils $httpUtils, + private readonly array $possibleResourceOwners, + private readonly array $resourceOwners, + private readonly ServiceLocator $locator ) { - $this->httpUtils = $httpUtils; - $this->possibleResourceOwners = $possibleResourceOwners; - $this->resourceOwners = $resourceOwners; - $this->locator = $locator; } /** diff --git a/src/Security/OAuthErrorHandler.php b/src/Security/OAuthErrorHandler.php index 2ce7a1c24..ed10b6386 100644 --- a/src/Security/OAuthErrorHandler.php +++ b/src/Security/OAuthErrorHandler.php @@ -19,9 +19,9 @@ final class OAuthErrorHandler /** * "translated" OAuth errors to human readable format. * - * @var array + * @var array */ - private static $translatedOAuthErrors = [ + private static array $translatedOAuthErrors = [ 'access_denied' => 'You have refused access for this site.', 'authorization_expired' => 'Authorization expired.', 'bad_verification_code' => 'Bad verification code.', @@ -36,7 +36,7 @@ final class OAuthErrorHandler /** * @throws AuthenticationException */ - public static function handleOAuthError(Request $request) + public static function handleOAuthError(Request $request): void { $error = null; @@ -61,11 +61,7 @@ public static function handleOAuthError(Request $request) } if (null !== $error) { - if (isset(static::$translatedOAuthErrors[$error])) { - $error = static::$translatedOAuthErrors[$error]; - } else { - $error = sprintf('Unknown OAuth error: "%s".', $error); - } + $error = self::$translatedOAuthErrors[$error] ?? sprintf('Unknown OAuth error: "%s".', $error); throw new AuthenticationException($error); } diff --git a/src/Security/OAuthUtils.php b/src/Security/OAuthUtils.php index 007358c37..28e6aeac3 100644 --- a/src/Security/OAuthUtils.php +++ b/src/Security/OAuthUtils.php @@ -30,29 +30,18 @@ final class OAuthUtils public const SIGNATURE_METHOD_RSA = 'RSA-SHA1'; public const SIGNATURE_METHOD_PLAINTEXT = 'PLAINTEXT'; - private bool $connect; - private string $grantRule; - private HttpUtils $httpUtils; - private AuthorizationCheckerInterface $authorizationChecker; - private FirewallMap $firewallMap; - /** * @var array */ private array $ownerMaps = []; public function __construct( - HttpUtils $httpUtils, - AuthorizationCheckerInterface $authorizationChecker, - FirewallMap $firewallMap, - bool $connect, - string $grantRule + private readonly HttpUtils $httpUtils, + private readonly AuthorizationCheckerInterface $authorizationChecker, + private readonly FirewallMap $firewallMap, + private readonly bool $connect, + private readonly string $grantRule ) { - $this->httpUtils = $httpUtils; - $this->authorizationChecker = $authorizationChecker; - $this->firewallMap = $firewallMap; - $this->connect = $connect; - $this->grantRule = $grantRule; } public function addResourceOwnerMap($firewallName, ResourceOwnerMapInterface $ownerMap): void @@ -217,9 +206,7 @@ public static function signRequest( $signature = false; openssl_sign($baseString, $signature, $privateKey); - if (\PHP_VERSION_ID < 80000) { - openssl_free_key($privateKey); - } + break; case self::SIGNATURE_METHOD_PLAINTEXT: @@ -276,7 +263,7 @@ private function getResourceOwnerCheckPath(string $name, ?string $firewallName = * @param string|array|null $queryParameter The query parameter to parse and add to the State * @param ResourceOwnerInterface $resourceOwner The resource owner holding the state to be added to */ - private function addQueryParameterToState($queryParameter, ResourceOwnerInterface $resourceOwner): void + private function addQueryParameterToState(array|string|null $queryParameter, ResourceOwnerInterface $resourceOwner): void { if (null === $queryParameter) { return; diff --git a/tests/Security/Http/Authentication/AuthenticationFailureHandlerTest.php b/tests/Security/Http/Authentication/AuthenticationFailureHandlerTest.php new file mode 100644 index 000000000..18b865dbe --- /dev/null +++ b/tests/Security/Http/Authentication/AuthenticationFailureHandlerTest.php @@ -0,0 +1,136 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace HWI\Bundle\OAuthBundle\Tests\Security\Http\Authentication; + +use HWI\Bundle\OAuthBundle\Security\Core\Exception\AccountNotLinkedException; +use HWI\Bundle\OAuthBundle\Security\Http\Authentication\AuthenticationFailureHandler; +use PHPUnit\Framework\TestCase; +use Symfony\Component\HttpFoundation\RedirectResponse; +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\RouterInterface; +use Symfony\Component\Security\Core\Exception\AuthenticationException; +use Twig\Environment; + +final class AuthenticationFailureHandlerTest extends TestCase +{ + public function testRendersLoginPageByDefault() + { + $request = Request::create('/login'); + $requestStack = new RequestStack(); + $requestStack->push($request); + + $handler = new AuthenticationFailureHandler( + $requestStack, + $router = $this->createMock(RouterInterface::class), + $twig = $this->createMock(Environment::class), + false + ); + + $router->expects($this->never()) + ->method('generate'); + + $twig->expects($this->once()) + ->method('render') + ->with('@HWIOAuth/Connect/login.html.twig', ['error' => 'An authentication exception occurred.']); + + $this->assertInstanceOf( + Response::class, + $handler->onAuthenticationFailure($request, new AuthenticationException()) + ); + } + + public function testDoesNothingWhenConnectIsDisabled() + { + $request = Request::create('/login'); + $requestStack = new RequestStack(); + $requestStack->push($request); + + $handler = new AuthenticationFailureHandler( + $requestStack, + $router = $this->createMock(RouterInterface::class), + $twig = $this->createMock(Environment::class), + false + ); + + $router->expects($this->never()) + ->method('generate'); + + $twig->expects($this->once()) + ->method('render') + ->with('@HWIOAuth/Connect/login.html.twig', ['error' => 'An authentication exception occurred.']); + + $this->assertInstanceOf( + Response::class, + $handler->onAuthenticationFailure($request, new AuthenticationException()) + ); + } + + public function testDoesNothingWhenExceptionIsNotOAuthOne() + { + $request = Request::create('/login'); + $requestStack = new RequestStack(); + $requestStack->push($request); + + $handler = new AuthenticationFailureHandler( + $requestStack, + $router = $this->createMock(RouterInterface::class), + $twig = $this->createMock(Environment::class), + true + ); + + $router->expects($this->never()) + ->method('generate'); + + $twig->expects($this->once()) + ->method('render') + ->with('@HWIOAuth/Connect/login.html.twig', ['error' => 'An authentication exception occurred.']); + + $this->assertInstanceOf( + Response::class, + $handler->onAuthenticationFailure($request, new AuthenticationException()) + ); + } + + public function testRedirectsToRegistrationPage() + { + $request = Request::create('/login'); + $request->setSession($this->createMock(SessionInterface::class)); + + $requestStack = new RequestStack(); + $requestStack->push($request); + + $handler = new AuthenticationFailureHandler( + $requestStack, + $router = $this->createMock(RouterInterface::class), + $twig = $this->createMock(Environment::class), + true + ); + + $router->expects($this->once()) + ->method('generate') + ->with('hwi_oauth_connect_registration', ['key' => $key = time()]) + ->willReturn('https://localhost/register/'.$key); + + $twig->expects($this->never()) + ->method('render'); + + $this->assertInstanceOf( + RedirectResponse::class, + $handler->onAuthenticationFailure($request, new AuthenticationException(previous: new AccountNotLinkedException())) + ); + } +} diff --git a/tests/Security/Http/RefreshOnExpireTest.php b/tests/Security/Http/RefreshOnExpireTest.php index f96a48038..c54c3099f 100644 --- a/tests/Security/Http/RefreshOnExpireTest.php +++ b/tests/Security/Http/RefreshOnExpireTest.php @@ -181,6 +181,7 @@ private function loginUserWithToken(array $tokenData): CustomOAuthToken $token = new CustomOAuthToken($tokenData); $token->setResourceOwnerName('google'); + /* @phpstan-ignore-next-line */ if (method_exists($token, 'setAuthenticated')) { $token->setAuthenticated(true, false); }