From 9ad7af3f0a391c878d18f78a089b15b70f7b2c52 Mon Sep 17 00:00:00 2001 From: Xheni Myrtaj Date: Thu, 21 Jun 2018 17:21:37 +0200 Subject: [PATCH] Refuse API key if user is disabled While testing the REST API, I noticed that disabled administrators are still able to generate API keys as well as use their existing ones. This adjusts the authentication logic to block access for disabled administrators. Signed-off-by: Xheni Myrtaj --- src/Security/Authentication.php | 4 ++-- .../Repository/Fixtures/Administrator.csv | 5 ++-- .../AdministratorTokenWithAdministrator.csv | 1 + .../Identity/AdministratorRepositoryTest.php | 3 +-- .../Security/AuthenticationTest.php | 24 +++++++++++++++++++ 5 files changed, 31 insertions(+), 6 deletions(-) diff --git a/src/Security/Authentication.php b/src/Security/Authentication.php index 22c2694..b8bcfc2 100644 --- a/src/Security/Authentication.php +++ b/src/Security/Authentication.php @@ -58,10 +58,10 @@ public function authenticateByApiKey(Request $request) try { // This checks for cases where a super user created a session key and then got their super user - // privileges removed during the lifetime of the session key. + // privileges removed during the lifetime of the session key. Or an administrator got disabled. // In addition, this will load the lazy-loaded model from the database, // which will check that the model really exists in the database (i.e., it has not been deleted). - if (!$administrator->isSuperUser()) { + if (!$administrator->isSuperUser() || $administrator->isDisabled()) { $administrator = null; } } catch (EntityNotFoundException $exception) { diff --git a/tests/Integration/Domain/Repository/Fixtures/Administrator.csv b/tests/Integration/Domain/Repository/Fixtures/Administrator.csv index 18d178b..bacb62f 100644 --- a/tests/Integration/Domain/Repository/Fixtures/Administrator.csv +++ b/tests/Integration/Domain/Repository/Fixtures/Administrator.csv @@ -1,3 +1,4 @@ id,loginname,email,created,modified,password,passwordchanged,disabled,superuser -1,"john.doe","john@example.com","2017-06-22 15:01:17","2017-06-23 19:50:43","1491a3c7e7b23b9a6393323babbb095dee0d7d81b2199617b487bd0fb5236f3c","2017-06-28",1,1 -2,"max.doe","john@example.com","2017-06-22 15:01:17","2017-06-23 19:50:43","1491a3c7e7b23b9a6393323babbb095dee0d7d81b2199617b487bd0fb5236f3c","2017-06-28",1,0 +1,"john.doe","john@example.com","2017-06-22 15:01:17","2017-06-23 19:50:43","1491a3c7e7b23b9a6393323babbb095dee0d7d81b2199617b487bd0fb5236f3c","2017-06-28",0,1 +2,"max.doe","john@example.com","2017-06-22 15:01:17","2017-06-23 19:50:43","1491a3c7e7b23b9a6393323babbb095dee0d7d81b2199617b487bd0fb5236f3c","2017-06-28",0,0 +3,"disabled.admin","disabled.admin@example.com","2017-06-22 15:01:17","2017-06-23 19:50:43","1491a3c7e7b23b9a6393323babbb095dee0d7d81b2199617b487bd0fb5236f3c","2017-06-28",1,1 diff --git a/tests/Integration/Domain/Repository/Fixtures/AdministratorTokenWithAdministrator.csv b/tests/Integration/Domain/Repository/Fixtures/AdministratorTokenWithAdministrator.csv index bbad8a3..2a0522d 100644 --- a/tests/Integration/Domain/Repository/Fixtures/AdministratorTokenWithAdministrator.csv +++ b/tests/Integration/Domain/Repository/Fixtures/AdministratorTokenWithAdministrator.csv @@ -2,3 +2,4 @@ id,value,expires,adminid,entered 1,"cfdf64eecbbf336628b0f3071adba762","2027-06-22 16:43:29",1,1512582100 2,"cfdf64eecbbf336628b0f3071adba763","2027-06-22 16:43:29",999,1512582100 3,"cfdf64eecbbf336628b0f3071adba764","2027-06-22 16:43:29",2,1512582100 +4,"cfdf64eecbbf336628b0f3071adba765","2027-06-22 16:43:29",3,1512582100 diff --git a/tests/Integration/Domain/Repository/Identity/AdministratorRepositoryTest.php b/tests/Integration/Domain/Repository/Identity/AdministratorRepositoryTest.php index 11ad006..9ae477a 100644 --- a/tests/Integration/Domain/Repository/Identity/AdministratorRepositoryTest.php +++ b/tests/Integration/Domain/Repository/Identity/AdministratorRepositoryTest.php @@ -62,8 +62,7 @@ public function findReadsModelFromDatabase() static::assertEquals($modificationDate, $actualModel->getModificationDate()); static::assertSame($passwordHash, $actualModel->getPasswordHash()); static::assertEquals($passwordChangeDate, $actualModel->getPasswordChangeDate()); - static::assertTrue($actualModel->isDisabled()); - static::assertTrue($actualModel->isDisabled()); + static::assertFalse($actualModel->isDisabled()); } /** diff --git a/tests/Integration/Security/AuthenticationTest.php b/tests/Integration/Security/AuthenticationTest.php index 6e9364f..2e69f60 100644 --- a/tests/Integration/Security/AuthenticationTest.php +++ b/tests/Integration/Security/AuthenticationTest.php @@ -81,6 +81,30 @@ public function authenticateByApiKeyWithValidApiKeyReturnsMatchingAdministrator( static::assertSame(1, $result->getId()); } + /** + * @test + */ + public function authenticateByApiKeyWithValidApiKeyAndDisabledAdministratorReturnsNull() + { + $this->getDataSet()->addTable( + static::ADMINISTRATOR_TABLE_NAME, + __DIR__ . '/../Domain/Repository/Fixtures/Administrator.csv' + ); + $this->getDataSet()->addTable( + static::TOKEN_TABLE_NAME, + __DIR__ . '/../Domain/Repository/Fixtures/AdministratorTokenWithAdministrator.csv' + ); + $this->applyDatabaseChanges(); + + $apiKey = 'cfdf64eecbbf336628b0f3071adba765'; + $request = new Request(); + $request->headers->add(['php-auth-pw' => $apiKey]); + + $result = $this->subject->authenticateByApiKey($request); + + static::assertNull($result); + } + /** * @test */