From 0f5fb066d19f7b15ea447a1588362d5543c31edc Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 29 Sep 2018 08:16:40 +0200 Subject: [PATCH] Converted AuthenticationpluginManager in a plain plugin manager and encasulated in new service adding extra behavior --- module/Rest/config/auth.config.php | 5 +- .../AuthenticationPluginManager.php | 48 +---------- .../AuthenticationPluginManagerInterface.php | 11 +-- .../RequestToHttpAuthPlugin.php | 64 ++++++++++++++ .../RequestToHttpAuthPluginInterface.php | 17 ++++ .../Middleware/AuthenticationMiddleware.php | 19 ++--- ...AuthenticationPluginManagerFactoryTest.php | 33 ++++++++ .../AuthorizationHeaderPluginTest.php | 2 +- .../RequestToAuthPluginTest.php | 83 +++++++++++++++++++ .../AuthenticationMiddlewareTest.php | 29 +++---- 10 files changed, 226 insertions(+), 85 deletions(-) create mode 100644 module/Rest/src/Authentication/RequestToHttpAuthPlugin.php create mode 100644 module/Rest/src/Authentication/RequestToHttpAuthPluginInterface.php create mode 100644 module/Rest/test/Authentication/AuthenticationPluginManagerFactoryTest.php rename module/Rest/test/Authentication/{ => Plugin}/AuthorizationHeaderPluginTest.php (98%) create mode 100644 module/Rest/test/Authentication/RequestToAuthPluginTest.php diff --git a/module/Rest/config/auth.config.php b/module/Rest/config/auth.config.php index cd63447d..94e853be 100644 --- a/module/Rest/config/auth.config.php +++ b/module/Rest/config/auth.config.php @@ -31,6 +31,7 @@ return [ 'factories' => [ Authentication\AuthenticationPluginManager::class => Authentication\AuthenticationPluginManagerFactory::class, + Authentication\RequestToHttpAuthPlugin::class => ConfigAbstractFactory::class, Middleware\AuthenticationMiddleware::class => ConfigAbstractFactory::class, ], @@ -39,8 +40,10 @@ return [ ConfigAbstractFactory::class => [ Authentication\Plugin\AuthorizationHeaderPlugin::class => [Authentication\JWTService::class, 'translator'], + Authentication\RequestToHttpAuthPlugin::class => [Authentication\AuthenticationPluginManager::class], + Middleware\AuthenticationMiddleware::class => [ - Authentication\AuthenticationPluginManager::class, + Authentication\RequestToHttpAuthPlugin::class, 'translator', 'config.auth.routes_whitelist', 'Logger_Shlink', diff --git a/module/Rest/src/Authentication/AuthenticationPluginManager.php b/module/Rest/src/Authentication/AuthenticationPluginManager.php index da8121e8..1837dcc6 100644 --- a/module/Rest/src/Authentication/AuthenticationPluginManager.php +++ b/module/Rest/src/Authentication/AuthenticationPluginManager.php @@ -3,55 +3,9 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\Authentication; -use Psr\Container\ContainerExceptionInterface; -use Psr\Http\Message\ServerRequestInterface; -use Shlinkio\Shlink\Rest\Authentication\Plugin\ApiKeyHeaderPlugin; -use Shlinkio\Shlink\Rest\Authentication\Plugin\AuthorizationHeaderPlugin; -use Shlinkio\Shlink\Rest\Exception\NoAuthenticationException; use Zend\ServiceManager\AbstractPluginManager; -use function array_filter; -use function array_reduce; -use function array_shift; class AuthenticationPluginManager extends AbstractPluginManager implements AuthenticationPluginManagerInterface { - // Headers here have to be defined in order of priority. - // When more than one is matched, the first one will take precedence - public const SUPPORTED_AUTH_HEADERS = [ - ApiKeyHeaderPlugin::HEADER_NAME, - AuthorizationHeaderPlugin::HEADER_NAME, - ]; - - /** - * @throws ContainerExceptionInterface - * @throws NoAuthenticationException - */ - public function fromRequest(ServerRequestInterface $request): Plugin\AuthenticationPluginInterface - { - if (! $this->hasAnySupportedHeader($request)) { - throw NoAuthenticationException::fromExpectedTypes([ - ApiKeyHeaderPlugin::HEADER_NAME, - AuthorizationHeaderPlugin::HEADER_NAME, - ]); - } - - return $this->get($this->getFirstAvailableHeader($request)); - } - - private function hasAnySupportedHeader(ServerRequestInterface $request): bool - { - return array_reduce( - self::SUPPORTED_AUTH_HEADERS, - function (bool $carry, string $header) use ($request) { - return $carry || $request->hasHeader($header); - }, - false - ); - } - - private function getFirstAvailableHeader(ServerRequestInterface $request): string - { - $foundHeaders = array_filter(self::SUPPORTED_AUTH_HEADERS, [$request, 'hasHeader']); - return array_shift($foundHeaders) ?? ''; - } + protected $instanceOf = Plugin\AuthenticationPluginInterface::class; } diff --git a/module/Rest/src/Authentication/AuthenticationPluginManagerInterface.php b/module/Rest/src/Authentication/AuthenticationPluginManagerInterface.php index 11ae49ca..ae8c1abf 100644 --- a/module/Rest/src/Authentication/AuthenticationPluginManagerInterface.php +++ b/module/Rest/src/Authentication/AuthenticationPluginManagerInterface.php @@ -3,15 +3,8 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\Authentication; -use Psr\Container; -use Psr\Http\Message\ServerRequestInterface; -use Shlinkio\Shlink\Rest\Exception\NoAuthenticationException; +use Psr\Container\ContainerInterface; -interface AuthenticationPluginManagerInterface extends Container\ContainerInterface +interface AuthenticationPluginManagerInterface extends ContainerInterface { - /** - * @throws Container\ContainerExceptionInterface - * @throws NoAuthenticationException - */ - public function fromRequest(ServerRequestInterface $request): Plugin\AuthenticationPluginInterface; } diff --git a/module/Rest/src/Authentication/RequestToHttpAuthPlugin.php b/module/Rest/src/Authentication/RequestToHttpAuthPlugin.php new file mode 100644 index 00000000..3be4b60a --- /dev/null +++ b/module/Rest/src/Authentication/RequestToHttpAuthPlugin.php @@ -0,0 +1,64 @@ +authPluginManager = $authPluginManager; + } + + /** + * @throws Container\ContainerExceptionInterface + * @throws NoAuthenticationException + */ + public function fromRequest(ServerRequestInterface $request): Plugin\AuthenticationPluginInterface + { + if (! $this->hasAnySupportedHeader($request)) { + throw NoAuthenticationException::fromExpectedTypes([ + Plugin\ApiKeyHeaderPlugin::HEADER_NAME, + Plugin\AuthorizationHeaderPlugin::HEADER_NAME, + ]); + } + + return $this->authPluginManager->get($this->getFirstAvailableHeader($request)); + } + + private function hasAnySupportedHeader(ServerRequestInterface $request): bool + { + return array_reduce( + self::SUPPORTED_AUTH_HEADERS, + function (bool $carry, string $header) use ($request) { + return $carry || $request->hasHeader($header); + }, + false + ); + } + + private function getFirstAvailableHeader(ServerRequestInterface $request): string + { + $foundHeaders = array_filter(self::SUPPORTED_AUTH_HEADERS, [$request, 'hasHeader']); + return array_shift($foundHeaders) ?? ''; + } +} diff --git a/module/Rest/src/Authentication/RequestToHttpAuthPluginInterface.php b/module/Rest/src/Authentication/RequestToHttpAuthPluginInterface.php new file mode 100644 index 00000000..a05f3274 --- /dev/null +++ b/module/Rest/src/Authentication/RequestToHttpAuthPluginInterface.php @@ -0,0 +1,17 @@ +translator = $translator; $this->routesWhitelist = $routesWhitelist; $this->logger = $logger ?: new NullLogger(); - $this->authPluginManager = $authPluginManager; + $this->requestToAuthPlugin = $requestToAuthPlugin; } /** @@ -81,12 +78,12 @@ class AuthenticationMiddleware implements MiddlewareInterface, StatusCodeInterfa } try { - $plugin = $this->authPluginManager->fromRequest($request); + $plugin = $this->requestToAuthPlugin->fromRequest($request); } catch (ContainerExceptionInterface | NoAuthenticationException $e) { $this->logger->warning('Invalid or no authentication provided.' . PHP_EOL . $e); return $this->createErrorResponse(sprintf($this->translator->translate( 'Expected one of the following authentication headers, but none were provided, ["%s"]' - ), implode('", "', AuthenticationPluginManager::SUPPORTED_AUTH_HEADERS))); + ), implode('", "', RequestToHttpAuthPlugin::SUPPORTED_AUTH_HEADERS))); } try { diff --git a/module/Rest/test/Authentication/AuthenticationPluginManagerFactoryTest.php b/module/Rest/test/Authentication/AuthenticationPluginManagerFactoryTest.php new file mode 100644 index 00000000..0a1b3993 --- /dev/null +++ b/module/Rest/test/Authentication/AuthenticationPluginManagerFactoryTest.php @@ -0,0 +1,33 @@ +factory = new AuthenticationPluginManagerFactory(); + } + + /** + * @test + */ + public function serviceIsProperlyCreated() + { + $instance = $this->factory->__invoke(new ServiceManager(['services' => [ + 'config' => [], + ]]), ''); + $this->assertInstanceOf(AuthenticationPluginManager::class, $instance); + } +} diff --git a/module/Rest/test/Authentication/AuthorizationHeaderPluginTest.php b/module/Rest/test/Authentication/Plugin/AuthorizationHeaderPluginTest.php similarity index 98% rename from module/Rest/test/Authentication/AuthorizationHeaderPluginTest.php rename to module/Rest/test/Authentication/Plugin/AuthorizationHeaderPluginTest.php index 2b85f989..31590c53 100644 --- a/module/Rest/test/Authentication/AuthorizationHeaderPluginTest.php +++ b/module/Rest/test/Authentication/Plugin/AuthorizationHeaderPluginTest.php @@ -1,7 +1,7 @@ pluginManager = $this->prophesize(AuthenticationPluginManagerInterface::class); + $this->requestToPlugin = new RequestToHttpAuthPlugin($this->pluginManager->reveal()); + } + + /** + * @test + */ + public function exceptionIsFoundWhenNoneOfTheSupportedMethodsIsFound() + { + $request = ServerRequestFactory::fromGlobals(); + + $this->expectException(NoAuthenticationException::class); + $this->expectExceptionMessage(sprintf( + 'None of the valid authentication mechanisms where provided. Expected one of ["%s"]', + implode('", "', RequestToHttpAuthPlugin::SUPPORTED_AUTH_HEADERS) + )); + + $this->requestToPlugin->fromRequest($request); + } + + /** + * @test + * @dataProvider provideHeaders + */ + public function properPluginIsFetchedWhenAnyAuthTypeIsFound(array $headers, string $expectedHeader) + { + $request = ServerRequestFactory::fromGlobals(); + foreach ($headers as $header => $value) { + $request = $request->withHeader($header, $value); + } + + $plugin = $this->prophesize(AuthenticationPluginInterface::class); + $getPlugin = $this->pluginManager->get($expectedHeader)->willReturn($plugin->reveal()); + + $this->requestToPlugin->fromRequest($request); + + $getPlugin->shouldHaveBeenCalledTimes(1); + } + + public function provideHeaders(): array + { + return [ + 'API key header only' => [[ + ApiKeyHeaderPlugin::HEADER_NAME => 'foobar', + ], ApiKeyHeaderPlugin::HEADER_NAME], + 'Authorization header only' => [[ + AuthorizationHeaderPlugin::HEADER_NAME => 'foobar', + ], AuthorizationHeaderPlugin::HEADER_NAME], + 'Both headers' => [[ + AuthorizationHeaderPlugin::HEADER_NAME => 'foobar', + ApiKeyHeaderPlugin::HEADER_NAME => 'foobar', + ], ApiKeyHeaderPlugin::HEADER_NAME], + ]; + } +} diff --git a/module/Rest/test/Middleware/AuthenticationMiddlewareTest.php b/module/Rest/test/Middleware/AuthenticationMiddlewareTest.php index acdbf9d9..7886133a 100644 --- a/module/Rest/test/Middleware/AuthenticationMiddlewareTest.php +++ b/module/Rest/test/Middleware/AuthenticationMiddlewareTest.php @@ -14,9 +14,9 @@ use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Rest\Action\AuthenticateAction; -use Shlinkio\Shlink\Rest\Authentication\AuthenticationPluginManager; -use Shlinkio\Shlink\Rest\Authentication\AuthenticationPluginManagerInterface; use Shlinkio\Shlink\Rest\Authentication\Plugin\AuthenticationPluginInterface; +use Shlinkio\Shlink\Rest\Authentication\RequestToHttpAuthPlugin; +use Shlinkio\Shlink\Rest\Authentication\RequestToHttpAuthPluginInterface; use Shlinkio\Shlink\Rest\Exception\NoAuthenticationException; use Shlinkio\Shlink\Rest\Exception\VerifyAuthenticationException; use Shlinkio\Shlink\Rest\Middleware\AuthenticationMiddleware; @@ -39,7 +39,7 @@ class AuthenticationMiddlewareTest extends TestCase /** * @var ObjectProphecy */ - protected $pluginManager; + protected $requestToPlugin; /** * @var callable @@ -48,8 +48,8 @@ class AuthenticationMiddlewareTest extends TestCase public function setUp() { - $this->pluginManager = $this->prophesize(AuthenticationPluginManagerInterface::class); - $this->middleware = new AuthenticationMiddleware($this->pluginManager->reveal(), Translator::factory([]), [ + $this->requestToPlugin = $this->prophesize(RequestToHttpAuthPluginInterface::class); + $this->middleware = new AuthenticationMiddleware($this->requestToPlugin->reveal(), Translator::factory([]), [ AuthenticateAction::class, ]); } @@ -62,7 +62,7 @@ class AuthenticationMiddlewareTest extends TestCase { $handler = $this->prophesize(RequestHandlerInterface::class); $handle = $handler->handle($request)->willReturn(new Response()); - $fromRequest = $this->pluginManager->fromRequest(Argument::any())->willReturn( + $fromRequest = $this->requestToPlugin->fromRequest(Argument::any())->willReturn( $this->prophesize(AuthenticationPluginInterface::class)->reveal() ); @@ -101,12 +101,11 @@ class AuthenticationMiddlewareTest extends TestCase */ public function errorIsReturnedWhenNoValidAuthIsProvided($e) { - $authToken = 'ABC-abc'; $request = ServerRequestFactory::fromGlobals()->withAttribute( RouteResult::class, RouteResult::fromRoute(new Route('bar', $this->getDummyMiddleware()), []) - )->withHeader(AuthenticationMiddleware::AUTHORIZATION_HEADER, $authToken); - $fromRequest = $this->pluginManager->fromRequest(Argument::any())->willThrow($e); + ); + $fromRequest = $this->requestToPlugin->fromRequest(Argument::any())->willThrow($e); /** @var Response\JsonResponse $response */ $response = $this->middleware->process($request, $this->prophesize(RequestHandlerInterface::class)->reveal()); @@ -115,7 +114,7 @@ class AuthenticationMiddlewareTest extends TestCase $this->assertEquals(RestUtils::INVALID_AUTHORIZATION_ERROR, $payload['error']); $this->assertEquals(sprintf( 'Expected one of the following authentication headers, but none were provided, ["%s"]', - implode('", "', AuthenticationPluginManager::SUPPORTED_AUTH_HEADERS) + implode('", "', RequestToHttpAuthPlugin::SUPPORTED_AUTH_HEADERS) ), $payload['message']); $fromRequest->shouldHaveBeenCalledTimes(1); } @@ -134,17 +133,16 @@ class AuthenticationMiddlewareTest extends TestCase */ public function errorIsReturnedWhenVerificationFails() { - $authToken = 'ABC-abc'; $request = ServerRequestFactory::fromGlobals()->withAttribute( RouteResult::class, RouteResult::fromRoute(new Route('bar', $this->getDummyMiddleware()), []) - )->withHeader(AuthenticationMiddleware::AUTHORIZATION_HEADER, $authToken); + ); $plugin = $this->prophesize(AuthenticationPluginInterface::class); $verify = $plugin->verify($request)->willThrow( VerifyAuthenticationException::withError('the_error', 'the_message') ); - $fromRequest = $this->pluginManager->fromRequest(Argument::any())->willReturn($plugin->reveal()); + $fromRequest = $this->requestToPlugin->fromRequest(Argument::any())->willReturn($plugin->reveal()); /** @var Response\JsonResponse $response */ $response = $this->middleware->process($request, $this->prophesize(RequestHandlerInterface::class)->reveal()); @@ -161,18 +159,17 @@ class AuthenticationMiddlewareTest extends TestCase */ public function updatedResponseIsReturnedWhenVerificationPasses() { - $authToken = 'ABC-abc'; $newResponse = new Response(); $request = ServerRequestFactory::fromGlobals()->withAttribute( RouteResult::class, RouteResult::fromRoute(new Route('bar', $this->getDummyMiddleware()), []) - )->withHeader(AuthenticationMiddleware::AUTHORIZATION_HEADER, $authToken); + ); $plugin = $this->prophesize(AuthenticationPluginInterface::class); $verify = $plugin->verify($request)->will(function () { }); $update = $plugin->update($request, Argument::type(ResponseInterface::class))->willReturn($newResponse); - $fromRequest = $this->pluginManager->fromRequest(Argument::any())->willReturn($plugin->reveal()); + $fromRequest = $this->requestToPlugin->fromRequest(Argument::any())->willReturn($plugin->reveal()); $handler = $this->prophesize(RequestHandlerInterface::class); $handle = $handler->handle($request)->willReturn(new Response());