More work on user management

CHANGELOG

@@ -4,6 +4,10 @@ Version 0.9.0 (????-??-??)
 Bug fixes:
 - Use icons specified in Atom feeds when available
 
+Changes:
+- Explicitly forbid U+003A COLON in usernames, for compatibility with HTTP
+  Basic authentication
+
 Version 0.8.5 (2020-10-27)
 ==========================
 

lib/AbstractException.php

@@ -75,6 +75,7 @@ abstract class AbstractException extends \Exception {
         "User/ExceptionSession.invalid"               => 10431,
         "User/ExceptionInput.invalidTimezone"         => 10441,
         "User/ExceptionInput.invalidBoolean"          => 10442,
+        "User/ExceptionInput.invalidUsername"         => 10443,
         "Feed/Exception.internalError"                => 10500,
         "Feed/Exception.invalidCertificate"           => 10501,
         "Feed/Exception.invalidUrl"                   => 10502,

lib/User.php

@@ -7,6 +7,7 @@ declare(strict_types=1);
 namespace JKingWeb\Arsse;
 
 use JKingWeb\Arsse\Misc\ValueInfo as V;
+use JKingWeb\Arsse\User\ExceptionConflict as Conflict;
 use PasswordGenerator\Generator as PassGen;
 
 class User {
@@ -49,26 +50,41 @@ class User {
     }
 
     public function add(string $user, ?string $password = null): string {
+        // ensure the user name does not contain any U+003A COLON characters, as
+        // this is incompatible with HTTP Basic authentication
+        if (strpos($user, ":") !== false) {
+            throw new User\ExceptionInput("invalidUsername", "U+003A COLON");
+        }
         try {
             $out = $this->u->userAdd($user, $password) ?? $this->u->userAdd($user, $this->generatePassword());
-        } finally {
+        } catch (Conflict $e) {
-            // synchronize the internal database
             if (!Arsse::$db->userExists($user)) {
-                Arsse::$db->userAdd($user, $out ?? null);
+                Arsse::$db->userAdd($user, null);
             }
+            throw $e;
+        }
+        // synchronize the internal database
+        if (!Arsse::$db->userExists($user)) {
+            Arsse::$db->userAdd($user, $out);
         }
         return $out;
     }
     
+
     public function remove(string $user): bool {
         try {
-            return $this->u->userRemove($user);
+            $out = $this->u->userRemove($user);
-        } finally { // @codeCoverageIgnore
+        } catch (Conflict $e) {
             if (Arsse::$db->userExists($user)) {
-                // if the user was removed and we (still) have it in the internal database, remove it there
                 Arsse::$db->userRemove($user);
             }
+            throw $e;
         }
+        if (Arsse::$db->userExists($user)) {
+            // if the user was removed and we (still) have it in the internal database, remove it there
+            Arsse::$db->userRemove($user);
+        }
+        return $out;
     }
 
     public function passwordSet(string $user, ?string $newPassword, $oldPassword = null): string {

locale/en.php

@@ -139,6 +139,7 @@ return [
     'Exception.JKingWeb/Arsse/User/Exception.authMissing'                  => 'Please log in to proceed',
     'Exception.JKingWeb/Arsse/User/Exception.authFailed'                   => 'Authentication failed',
     'Exception.JKingWeb/Arsse/User/ExceptionSession.invalid'               => 'Session with ID {0} does not exist',
+    'Exception.JKingWeb/Arsse/User/ExceptionInput.invalidUsername'         => 'User names may not contain the Unicode character {0}',
     'Exception.JKingWeb/Arsse/Feed/Exception.internalError'                => 'Could not download feed "{url}" because of an internal error which is probably a bug',
     'Exception.JKingWeb/Arsse/Feed/Exception.invalidCertificate'           => 'Could not download feed "{url}" because its server is serving an invalid SSL certificate',
     'Exception.JKingWeb/Arsse/Feed/Exception.invalidUrl'                   => 'Feed URL "{url}" is invalid',

tests/cases/User/TestUser.php

@@ -11,6 +11,7 @@ use JKingWeb\Arsse\Database;
 use JKingWeb\Arsse\User;
 use JKingWeb\Arsse\AbstractException as Exception;
 use JKingWeb\Arsse\User\ExceptionConflict;
+use JKingWeb\Arsse\User\ExceptionInput;
 use JKingWeb\Arsse\User\Driver;
 
 /** @covers \JKingWeb\Arsse\User */
@@ -84,7 +85,7 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest {
     }
 
     public function testAddAUser(): void {
-        $user = "ohn.doe@example.com";
+        $user = "john.doe@example.com";
         $pass = "secret";
         $u = new User($this->drv);
         \Phake::when($this->drv)->userAdd->thenReturn($pass);
@@ -95,7 +96,7 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest {
     }
 
     public function testAddAUserWeDoNotKnow(): void {
-        $user = "ohn.doe@example.com";
+        $user = "john.doe@example.com";
         $pass = "secret";
         $u = new User($this->drv);
         \Phake::when($this->drv)->userAdd->thenReturn($pass);
@@ -107,7 +108,7 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest {
     }
 
     public function testAddADuplicateUser(): void {
-        $user = "ohn.doe@example.com";
+        $user = "john.doe@example.com";
         $pass = "secret";
         $u = new User($this->drv);
         \Phake::when($this->drv)->userAdd->thenThrow(new ExceptionConflict("alreadyExists"));
@@ -122,7 +123,7 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest {
     }
 
     public function testAddADuplicateUserWeDoNotKnow(): void {
-        $user = "ohn.doe@example.com";
+        $user = "john.doe@example.com";
         $pass = "secret";
         $u = new User($this->drv);
         \Phake::when($this->drv)->userAdd->thenThrow(new ExceptionConflict("alreadyExists"));
@@ -136,4 +137,110 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest {
             \Phake::verify($this->drv)->userAdd($user, $pass);
         }
     }
+
+    public function testAddAnInvalidUser(): void {
+        $user = "john:doe@example.com";
+        $pass = "secret";
+        $u = new User($this->drv);
+        \Phake::when($this->drv)->userAdd->thenThrow(new ExceptionInput("invalidUsername"));
+        $this->assertException("invalidUsername", "User", "ExceptionInput");
+        $u->add($user, $pass);
+    }
+
+    public function testAddAUserWithARandomPassword(): void {
+        $user = "john.doe@example.com";
+        $pass = "random password";
+        $u = \Phake::partialMock(User::class, $this->drv);
+        \Phake::when($u)->generatePassword->thenReturn($pass);
+        \Phake::when($this->drv)->userAdd->thenReturn(null)->thenReturn($pass);
+        \Phake::when(Arsse::$db)->userExists->thenReturn(true);
+        $this->assertSame($pass, $u->add($user));
+        \Phake::verify($this->drv)->userAdd($user, null);
+        \Phake::verify($this->drv)->userAdd($user, $pass);
+        \Phake::verify(Arsse::$db)->userExists($user);
+    }
+    
+    public function testRemoveAUser(): void {
+        $user = "john.doe@example.com";
+        $pass = "secret";
+        $u = new User($this->drv);
+        \Phake::when($this->drv)->userRemove->thenReturn(true);
+        \Phake::when(Arsse::$db)->userExists->thenReturn(true);
+        $this->assertTrue($u->remove($user));
+        \Phake::verify(Arsse::$db)->userExists($user);
+        \Phake::verify(Arsse::$db)->userRemove($user);
+        \Phake::verify($this->drv)->userRemove($user);
+    }
+    
+    public function testRemoveAUserWeDoNotKnow(): void {
+        $user = "john.doe@example.com";
+        $pass = "secret";
+        $u = new User($this->drv);
+        \Phake::when($this->drv)->userRemove->thenReturn(true);
+        \Phake::when(Arsse::$db)->userExists->thenReturn(false);
+        $this->assertTrue($u->remove($user));
+        \Phake::verify(Arsse::$db)->userExists($user);
+        \Phake::verify($this->drv)->userRemove($user);
+    }
+    
+    public function testRemoveAMissingUser(): void {
+        $user = "john.doe@example.com";
+        $pass = "secret";
+        $u = new User($this->drv);
+        \Phake::when($this->drv)->userRemove->thenThrow(new ExceptionConflict("doesNotExist"));
+        \Phake::when(Arsse::$db)->userExists->thenReturn(true);
+        $this->assertException("doesNotExist", "User", "ExceptionConflict");
+        try {
+            $u->remove($user);
+        } finally {
+            \Phake::verify(Arsse::$db)->userExists($user);
+            \Phake::verify(Arsse::$db)->userRemove($user);
+            \Phake::verify($this->drv)->userRemove($user);
+        }
+    }
+    
+    public function testRemoveAMissingUserWeDoNotKnow(): void {
+        $user = "john.doe@example.com";
+        $pass = "secret";
+        $u = new User($this->drv);
+        \Phake::when($this->drv)->userRemove->thenThrow(new ExceptionConflict("doesNotExist"));
+        \Phake::when(Arsse::$db)->userExists->thenReturn(false);
+        $this->assertException("doesNotExist", "User", "ExceptionConflict");
+        try {
+            $u->remove($user);
+        } finally {
+            \Phake::verify(Arsse::$db)->userExists($user);
+            \Phake::verify($this->drv)->userRemove($user);
+        }
+    }
+
+    public function testSetAPassword(): void {
+        $user = "john.doe@example.com";
+        $pass = "secret";
+        $u = new User($this->drv);
+        \Phake::when($this->drv)->userPasswordSet->thenReturn($pass);
+        \Phake::when(Arsse::$db)->userPasswordSet->thenReturn($pass);
+        \Phake::when(Arsse::$db)->userExists->thenReturn(true);
+        $this->assertSame($pass, $u->passwordSet($user, $pass));
+        \Phake::verify($this->drv)->userPasswordSet($user, $pass, null);
+        \Phake::verify(Arsse::$db)->userPasswordSet($user, $pass, null);
+        \Phake::verify(Arsse::$db)->sessionDestroy($user);
+        \Phake::verify(Arsse::$db)->userExists($user);
+    }
+
+    public function testSetARandomPassword(): void {
+        $user = "john.doe@example.com";
+        $pass = "random password";
+        $u = \Phake::partialMock(User::class, $this->drv);
+        \Phake::when($u)->generatePassword->thenReturn($pass);
+        \Phake::when($this->drv)->userPasswordSet->thenReturn(null)->thenReturn($pass);
+        \Phake::when(Arsse::$db)->userPasswordSet->thenReturn($pass);
+        \Phake::when(Arsse::$db)->userExists->thenReturn(true);
+        $this->assertSame($pass, $u->passwordSet($user, null));
+        \Phake::verify($this->drv)->userPasswordSet($user, null, null);
+        \Phake::verify($this->drv)->userPasswordSet($user, $pass, null);
+        \Phake::verify(Arsse::$db)->userPasswordSet($user, $pass, null);
+        \Phake::verify(Arsse::$db)->sessionDestroy($user);
+        \Phake::verify(Arsse::$db)->userExists($user);
+    }
 }