From 27d9c046d53de240206840a1147088a97ef2cce9 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Mon, 16 Nov 2020 00:11:19 -0500 Subject: [PATCH] More work on user management --- CHANGELOG | 4 ++ lib/AbstractException.php | 1 + lib/User.php | 28 +++++++-- locale/en.php | 1 + tests/cases/User/TestUser.php | 115 ++++++++++++++++++++++++++++++++-- 5 files changed, 139 insertions(+), 10 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 730e60a..3b65066 100644 --- a/CHANGELOG +++ b/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) ========================== diff --git a/lib/AbstractException.php b/lib/AbstractException.php index 22b6eb5..d26b3cd 100644 --- a/lib/AbstractException.php +++ b/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, diff --git a/lib/User.php b/lib/User.php index ffb6d4a..2fec130 100644 --- a/lib/User.php +++ b/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 { - // synchronize the internal database + } catch (Conflict $e) { 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); - } finally { // @codeCoverageIgnore + $out = $this->u->userRemove($user); + } 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 { diff --git a/locale/en.php b/locale/en.php index e5ada18..2791c5b 100644 --- a/locale/en.php +++ b/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', diff --git a/tests/cases/User/TestUser.php b/tests/cases/User/TestUser.php index 97a9352..9e34a2e 100644 --- a/tests/cases/User/TestUser.php +++ b/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); + } }