From a9cd00c37a08b1dc5dc0ba95db2f4808e7733094 Mon Sep 17 00:00:00 2001 From: Igor Scheller Date: Sat, 28 Jan 2023 00:41:29 +0100 Subject: [PATCH] Authenticator: Improve auth methods handling, esp. for api endpoints --- .gitlab-ci.yml | 2 +- includes/controller/shifts_controller.php | 11 +- includes/pages/user_atom.php | 10 +- includes/pages/user_ical.php | 11 +- src/Helpers/Authenticator.php | 92 +++++++-- src/Middleware/RequestHandler.php | 2 + src/Middleware/SessionHandler.php | 6 +- src/helpers.php | 15 +- tests/Unit/Helpers/AuthenticatorTest.php | 200 +++++++++++++------ tests/Unit/Middleware/RequestHandlerTest.php | 3 + tests/Unit/Middleware/SessionHandlerTest.php | 9 +- 11 files changed, 242 insertions(+), 119 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 65a7e589..d7c41ed2 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -217,7 +217,7 @@ test: - ./bin/migrate script: - >- - php -d pcov.enabled=1 -d pcov.directory=. vendor/bin/phpunit -vvv --colors=never + php -d memory_limit=1024M -d pcov.enabled=1 -d pcov.directory=. vendor/bin/phpunit -vvv --colors=never --coverage-text --coverage-html "${HOMEDIR}/coverage/" --log-junit "${HOMEDIR}/unittests.xml" after_script: diff --git a/includes/controller/shifts_controller.php b/includes/controller/shifts_controller.php index 157485c2..442b9327 100644 --- a/includes/controller/shifts_controller.php +++ b/includes/controller/shifts_controller.php @@ -386,15 +386,10 @@ function shift_next_controller() */ function shifts_json_export_controller() { - $request = request(); - $user = auth()->apiUser('key'); + $user = auth()->userFromApi(); - if ( - !$request->has('key') - || !$request->input('key') - || !$user - ) { - throw new HttpForbidden('{"error":"Missing or invalid key"}', ['content-type' => 'application/json']); + if (!$user) { + throw new HttpForbidden('{"error":"Missing or invalid ?key="}', ['content-type' => 'application/json']); } if (!auth()->can('shifts_json_export')) { diff --git a/includes/pages/user_atom.php b/includes/pages/user_atom.php index 71505d81..a4926d99 100644 --- a/includes/pages/user_atom.php +++ b/includes/pages/user_atom.php @@ -11,14 +11,10 @@ use Illuminate\Support\Collection as SupportCollection; function user_atom() { $request = request(); - $user = auth()->apiUser('key'); + $user = auth()->userFromApi(); - if ( - !$request->has('key') - || !$request->input('key') - || empty($user) - ) { - throw new HttpForbidden('Missing or invalid key', ['content-type' => 'text/text']); + if (!$user) { + throw new HttpForbidden('Missing or invalid ?key=', ['content-type' => 'text/text']); } if (!auth()->can('atom')) { diff --git a/includes/pages/user_ical.php b/includes/pages/user_ical.php index 132abe76..dbb44672 100644 --- a/includes/pages/user_ical.php +++ b/includes/pages/user_ical.php @@ -9,15 +9,10 @@ use Illuminate\Support\Collection; */ function user_ical() { - $request = request(); - $user = auth()->apiUser('key'); + $user = auth()->userFromApi(); - if ( - !$request->has('key') - || !$request->input('key') - || !$user - ) { - throw new HttpForbidden('Missing or invalid key', ['content-type' => 'text/text']); + if (!$user) { + throw new HttpForbidden('Missing or invalid ?key=', ['content-type' => 'text/text']); } if (!auth()->can('ical')) { diff --git a/src/Helpers/Authenticator.php b/src/Helpers/Authenticator.php index e86ac938..c738c19e 100644 --- a/src/Helpers/Authenticator.php +++ b/src/Helpers/Authenticator.php @@ -6,6 +6,7 @@ use Carbon\Carbon; use Engelsystem\Models\Group; use Engelsystem\Models\User\User; use Engelsystem\Models\User\User as UserRepository; +use Illuminate\Support\Str; use Psr\Http\Message\ServerRequestInterface; use Symfony\Component\HttpFoundation\Session\Session; @@ -30,7 +31,7 @@ class Authenticator } /** - * Load the user from session + * Load the user from session or api auth */ public function user(): ?User { @@ -38,47 +39,50 @@ class Authenticator return $this->user; } + $this->user = $this->userFromSession(); + if (!$this->user && request()->getAttribute('route-api', false)) { + $this->user = $this->userFromApi(); + } + + return $this->user; + } + + /** + * Load the user from session + */ + public function userFromSession(): ?User + { + if ($this->user) { + return $this->user; + } + $userId = $this->session->get('user_id'); if (!$userId) { return null; } - $user = $this + $this->user = $this ->userRepository ->find($userId); - if (!$user) { - return null; - } - - $this->user = $user; return $this->user; } /** - * Get the user by his api key + * Get the user by its api key */ - public function apiUser(string $parameter = 'api_key'): ?User + public function userFromApi(): ?User { if ($this->user) { return $this->user; } - $params = $this->request->getQueryParams(); - if (!isset($params[$parameter])) { - return null; + $this->user = $this->userByHeaders(); + if ($this->user) { + return $this->user; } - /** @var User|null $user */ - $user = $this - ->userRepository - ->whereApiKey($params[$parameter]) - ->first(); - if (!$user) { - return $this->user(); - } - - $this->user = $user; + $this->user = $this->userByQueryParam(); return $this->user; } @@ -150,6 +154,50 @@ class Authenticator return true; } + /** + * Get the user by authorization bearer or x-api-key headers + */ + protected function userByHeaders(): ?User + { + $header = $this->request->getHeader('authorization'); + if (!empty($header) && Str::startsWith(Str::lower($header[0]), 'bearer ')) { + return $this->userByApiKey(Str::substr($header[0], 7)); + } + + $header = $this->request->getHeader('x-api-key'); + if (!empty($header)) { + return $this->userByApiKey($header[0]); + } + + return null; + } + + /** + * Get the user by query parameters + */ + protected function userByQueryParam(): ?User + { + $params = $this->request->getQueryParams(); + if (!empty($params['key'])) { + $this->user = $this->userByApiKey($params['key']); + } + + return $this->user; + } + + /** + * Get the user by its api key + */ + protected function userByApiKey(string $key): ?User + { + $this->user = $this + ->userRepository + ->whereApiKey($key) + ->first(); + + return $this->user; + } + public function setPassword(User $user, string $password): void { $user->password = password_hash($password, $this->passwordAlgorithm); diff --git a/src/Middleware/RequestHandler.php b/src/Middleware/RequestHandler.php index cef15e8e..6f7a188c 100644 --- a/src/Middleware/RequestHandler.php +++ b/src/Middleware/RequestHandler.php @@ -26,6 +26,8 @@ class RequestHandler implements MiddlewareInterface public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { $requestHandler = $request->getAttribute('route-request-handler'); + $this->container->instance(ServerRequestInterface::class, $request); + $this->container->instance('request', $request); /** @var CallableHandler|MiddlewareInterface|RequestHandlerInterface $requestHandler */ $requestHandler = $this->resolveRequestHandler($requestHandler); diff --git a/src/Middleware/SessionHandler.php b/src/Middleware/SessionHandler.php index 0421abb5..28a697d7 100644 --- a/src/Middleware/SessionHandler.php +++ b/src/Middleware/SessionHandler.php @@ -18,13 +18,15 @@ class SessionHandler implements MiddlewareInterface public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { $requestPath = $request->getAttribute('route-request-path'); + $isApi = in_array($requestPath, $this->paths); + $request = $request->withAttribute('route-api', $isApi); $return = $handler->handle($request); $cookies = $request->getCookieParams(); if ( - $this->session instanceof NativeSessionStorage - && in_array($requestPath, $this->paths) + $isApi + && $this->session instanceof NativeSessionStorage && !isset($cookies[$this->session->getName()]) ) { $this->destroyNative(); diff --git a/src/helpers.php b/src/helpers.php index 3f58fdf2..f672f7ee 100644 --- a/src/helpers.php +++ b/src/helpers.php @@ -14,6 +14,8 @@ use Symfony\Component\HttpFoundation\Session\SessionInterface; /** * Get the global app instance + * @return mixed|Application + * @phpcsSuppress SlevomatCodingStandard.TypeHints.ReturnTypeHint.UselessAnnotation */ function app(string $id = null): mixed { @@ -44,6 +46,8 @@ function back(int $status = 302, array $headers = []): Response /** * Get or set config values + * @return mixed|Config + * @phpcsSuppress SlevomatCodingStandard.TypeHints.ReturnTypeHint.UselessAnnotation */ function config(string|array $key = null, mixed $default = null): mixed { @@ -87,6 +91,10 @@ function redirect(string $path, int $status = 302, array $headers = []): Respons return $redirect->to($path, $status, $headers); } +/** + * @return mixed|Request + * @phpcsSuppress SlevomatCodingStandard.TypeHints.ReturnTypeHint.UselessAnnotation + */ function request(string $key = null, mixed $default = null): mixed { /** @var Request $request */ @@ -114,6 +122,10 @@ function response(mixed $content = '', int $status = 200, array $headers = []): return $response; } +/** + * @return mixed|SessionInterface + * @phpcsSuppress SlevomatCodingStandard.TypeHints.ReturnTypeHint.UselessAnnotation + */ function session(string $key = null, mixed $default = null): mixed { /** @var SessionInterface $session */ @@ -175,9 +187,6 @@ function url(string $path = null, array $parameters = []): UrlGeneratorInterface return $urlGenerator->to($path, $parameters); } -/** - * @param mixed[] $data - */ function view(string $template = null, array $data = []): Renderer|string { /** @var Renderer $renderer */ diff --git a/tests/Unit/Helpers/AuthenticatorTest.php b/tests/Unit/Helpers/AuthenticatorTest.php index e5cc8b87..3ed80fd3 100644 --- a/tests/Unit/Helpers/AuthenticatorTest.php +++ b/tests/Unit/Helpers/AuthenticatorTest.php @@ -3,6 +3,7 @@ namespace Engelsystem\Test\Unit\Helpers; use Engelsystem\Helpers\Authenticator; +use Engelsystem\Http\Request; use Engelsystem\Models\Group; use Engelsystem\Models\Privilege; use Engelsystem\Models\User\User; @@ -12,97 +13,164 @@ use Engelsystem\Test\Unit\ServiceProviderTest; use PHPUnit\Framework\MockObject\MockObject; use Psr\Http\Message\ServerRequestInterface; use Symfony\Component\HttpFoundation\Session\Session; +use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage; class AuthenticatorTest extends ServiceProviderTest { use HasDatabase; /** - * @covers \Engelsystem\Helpers\Authenticator::__construct * @covers \Engelsystem\Helpers\Authenticator::user + * @covers \Engelsystem\Helpers\Authenticator::__construct */ - public function testUser(): void + public function testUserNotAuthorized(): void { - /** @var ServerRequestInterface|MockObject $request */ - $request = $this->getMockForAbstractClass(ServerRequestInterface::class); - /** @var Session|MockObject $session */ - $session = $this->createMock(Session::class); + $request = new Request(); + $session = new Session(new MockArraySessionStorage()); /** @var UserModelImplementation|MockObject $userRepository */ $userRepository = new UserModelImplementation(); - /** @var User|MockObject $user */ - $user = $this->createMock(User::class); - - $session->expects($this->exactly(3)) - ->method('get') - ->with('user_id') - ->willReturnOnConsecutiveCalls( - null, - 42, - 1337 - ); + $this->app->instance('request', $request); $auth = new Authenticator($request, $session, $userRepository); + $user = $auth->user(); - // Not in session - $this->assertNull($auth->user()); - - // Unknown user - UserModelImplementation::$id = 42; - $this->assertNull($auth->user()); - - // User found - UserModelImplementation::$id = 1337; - UserModelImplementation::$user = $user; - $this->assertEquals($user, $auth->user()); - - // User cached - UserModelImplementation::$id = null; - UserModelImplementation::$user = null; - $this->assertEquals($user, $auth->user()); + $this->assertNull($user); } /** - * @covers \Engelsystem\Helpers\Authenticator::apiUser + * @covers \Engelsystem\Helpers\Authenticator::user + * @covers \Engelsystem\Helpers\Authenticator::userFromSession */ - public function testApiUser(): void + public function testUserViaFromSession(): void { - /** @var ServerRequestInterface|MockObject $request */ - $request = $this->getMockForAbstractClass(ServerRequestInterface::class); - /** @var Session|MockObject $session */ - $session = $this->createMock(Session::class); - /** @var UserModelImplementation|MockObject $userRepository */ - $userRepository = new UserModelImplementation(); - /** @var User|MockObject $user */ - $user = $this->createMock(User::class); + $this->initDatabase(); - $request->expects($this->exactly(3)) - ->method('getQueryParams') - ->with() - ->willReturnOnConsecutiveCalls( - [], - ['api_key' => 'iMaNot3xiSt1nGAp1Key!'], - ['foo_key' => 'SomeSecretApiKey'] - ); + $request = new Request(); + $session = new Session(new MockArraySessionStorage()); - /** @var Authenticator|MockObject $auth */ - $auth = new Authenticator($request, $session, $userRepository); + $session->set('user_id', 42); + User::factory()->create(['id' => 42]); - // No key - $this->assertNull($auth->apiUser()); + $auth = new Authenticator($request, $session, new User()); + $user = $auth->user(); - // Unknown user - UserModelImplementation::$apiKey = 'iMaNot3xiSt1nGAp1Key!'; - $this->assertNull($auth->apiUser()); + $this->assertInstanceOf(User::class, $user); + $this->assertEquals(42, $user->id); - // User found - UserModelImplementation::$apiKey = 'SomeSecretApiKey'; - UserModelImplementation::$user = $user; - $this->assertEquals($user, $auth->apiUser('foo_key')); + // Cached in user() + $user2 = $auth->user(); + $this->assertEquals($user, $user2); - // User cached - UserModelImplementation::$apiKey = null; - UserModelImplementation::$user = null; - $this->assertEquals($user, $auth->apiUser()); + // Cached in userFromSession() + $user3 = $auth->userFromSession(); + $this->assertEquals($user, $user3); + } + + /** + * @covers \Engelsystem\Helpers\Authenticator::user + * @covers \Engelsystem\Helpers\Authenticator::userFromApi + * @covers \Engelsystem\Helpers\Authenticator::userByHeaders + */ + public function testUserViaFromApi(): void + { + $this->initDatabase(); + + $request = new Request(); + $session = new Session(new MockArraySessionStorage()); + + $request = $request->withHeader('Authorization', 'Bearer F00Bar'); + $request = $request->withAttribute('route-api', true); + $this->app->instance('request', $request); + User::factory()->create(['api_key' => 'F00Bar']); + + $auth = new Authenticator($request, $session, new User()); + $user = $auth->user(); + + $this->assertInstanceOf(User::class, $user); + $this->assertEquals('F00Bar', $user->api_key); + + // Cached in userFromApi() + $user2 = $auth->userFromApi(); + $this->assertEquals($user, $user2); + } + + /** + * @covers \Engelsystem\Helpers\Authenticator::userFromSession + */ + public function testUserFromSessionNotFound(): void + { + $this->initDatabase(); + + $request = new Request(); + $session = new Session(new MockArraySessionStorage()); + + $auth = new Authenticator($request, $session, new User()); + + $user = $auth->userFromSession(); + $this->assertNull($user); + + $session->set('user_id', 42); + $user2 = $auth->userFromSession(); + $this->assertNull($user2); + } + + /** + * @covers \Engelsystem\Helpers\Authenticator::userFromApi + * @covers \Engelsystem\Helpers\Authenticator::userByQueryParam + * @covers \Engelsystem\Helpers\Authenticator::userByApiKey + */ + public function testUserFromApiByQueryParam(): void + { + $this->initDatabase(); + + $request = new Request(); + $session = new Session(new MockArraySessionStorage()); + + $request = $request->withQueryParams(['key' => 'F00Bar']); + + $auth = new Authenticator($request, $session, new User()); + + // User not found + $user = $auth->userFromApi(); + $this->assertNull($user); + + // User exists + User::factory()->create(['api_key' => 'F00Bar']); + $user2 = $auth->userFromApi(); + $this->assertInstanceOf(User::class, $user2); + $this->assertEquals('F00Bar', $user2->api_key); + } + + /** + * @covers \Engelsystem\Helpers\Authenticator::userByHeaders + */ + public function testUserByHeaders(): void + { + $this->initDatabase(); + + $request = new Request(); + $request = $request->withAttribute('route-api', true); + $session = new Session(new MockArraySessionStorage()); + $this->app->instance('request', $request); + + $auth = new Authenticator($request, $session, new User()); + + // Header not set + $user = $auth->userFromApi(); + $this->assertNull($user); + + // User not found + $request = $request->withHeader('x-api-key', 'SomeWrongKey'); + $auth = new Authenticator($request, $session, new User()); + $user = $auth->userFromApi(); + $this->assertNull($user); + + $request = $request->withHeader('x-api-key', 'F00Bar'); + $auth = new Authenticator($request, $session, new User()); + User::factory()->create(['api_key' => 'F00Bar']); + $user = $auth->user(); + $this->assertInstanceOf(User::class, $user); + $this->assertEquals('F00Bar', $user->api_key); } /** diff --git a/tests/Unit/Middleware/RequestHandlerTest.php b/tests/Unit/Middleware/RequestHandlerTest.php index a1785e06..cc4b3410 100644 --- a/tests/Unit/Middleware/RequestHandlerTest.php +++ b/tests/Unit/Middleware/RequestHandlerTest.php @@ -130,6 +130,9 @@ class RequestHandlerTest extends TestCase ->method('make') ->with($className) ->willReturn($middlewareInterface); + $container->expects($this->exactly(2)) + ->method('instance') + ->withConsecutive([ServerRequestInterface::class, $request], ['request', $request]); $return = $middleware->process($request, $handler); $this->assertEquals($return, $response); diff --git a/tests/Unit/Middleware/SessionHandlerTest.php b/tests/Unit/Middleware/SessionHandlerTest.php index 45a22c44..2230434b 100644 --- a/tests/Unit/Middleware/SessionHandlerTest.php +++ b/tests/Unit/Middleware/SessionHandlerTest.php @@ -39,9 +39,14 @@ class SessionHandlerTest extends TestCase $request->expects($this->exactly(2)) ->method('getAttribute') ->with('route-request-path') - ->willReturn('/foo'); + ->willReturnOnConsecutiveCalls('/foo', '/lorem'); - $sessionStorage->expects($this->exactly(2)) + $request->expects($this->exactly(2)) + ->method('withAttribute') + ->withConsecutive(['route-api', true], ['route-api', false]) + ->willReturn($request); + + $sessionStorage->expects($this->once()) ->method('getName') ->willReturn('SESSION');