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 <myrtajxheni@gmail.com>
phpList · Jun 21, 2018 · 9ad7af3 · 9ad7af3
1 parent f910bde
commit 9ad7af3
Show file tree

Hide file tree

Showing 5 changed files with 31 additions and 6 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
      */