From 5ec04d33c621f201db40eb25af34e1084b6c2b09 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Fri, 25 Dec 2020 17:47:36 -0500 Subject: [PATCH] Add backend functionality to rename users --- lib/Database.php | 14 ++++++++ lib/User.php | 27 ++++++++++++++++ lib/User/Driver.php | 9 +++++- lib/User/Internal/Driver.php | 23 ++++++++++--- tests/cases/Database/SeriesUser.php | 25 +++++++++++++++ tests/cases/User/TestInternal.php | 24 +++++++++++++- tests/cases/User/TestUser.php | 50 +++++++++++++++++++++++++++-- 7 files changed, 164 insertions(+), 8 deletions(-) diff --git a/lib/Database.php b/lib/Database.php index 1d5405a..95ccc61 100644 --- a/lib/Database.php +++ b/lib/Database.php @@ -273,6 +273,20 @@ class Database { return true; } + public function userRename(string $user, string $name): bool { + if ($user === $name) { + return false; + } + try { + if (!$this->db->prepare("UPDATE arsse_users set id = ? where id = ?", "str", "str")->run($name, $user)->changes()) { + throw new User\ExceptionConflict("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); + } + } catch (Db\ExceptionInput $e) { + throw new User\ExceptionConflict("alreadyExists", ["action" => __FUNCTION__, "user" => $name], $e); + } + return true; + } + /** Removes a user from the database */ public function userRemove(string $user): bool { if ($this->db->prepare("DELETE from arsse_users where id = ?", "str")->run($user)->changes() < 1) { diff --git a/lib/User.php b/lib/User.php index df9a49d..1c7979b 100644 --- a/lib/User.php +++ b/lib/User.php @@ -42,6 +42,21 @@ class User { return (string) $this->id; } + public function begin(): Db\Transaction { + /* TODO: A proper implementation of this would return a meta-transaction + object which would contain both a user-manager transaction (when + applicable) and a database transaction, and commit or roll back both + as the situation calls. + + In theory, an external user driver would probably have to implement its + own approximation of atomic transactions and rollback. In practice the + only driver is the internal one, which is always backed by an ACID + database; the added complexity is thus being deferred until such time + as it is actually needed for a concrete implementation. + */ + return Arsse::$db->begin(); + } + public function auth(string $user, string $password): bool { $prevUser = $this->id; $this->id = $user; @@ -89,6 +104,18 @@ class User { return $out; } + public function rename(string $user, string $newName): bool { + if ($this->u->userRename($user, $newName)) { + if (!Arsse::$db->userExists($user)) { + Arsse::$db->userAdd($newName, null); + return true; + } else { + return Arsse::$db->userRename($user, $newName); + } + } + return false; + } + public function remove(string $user): bool { try { $out = $this->u->userRemove($user); diff --git a/lib/User/Driver.php b/lib/User/Driver.php index e0d949c..d4b7370 100644 --- a/lib/User/Driver.php +++ b/lib/User/Driver.php @@ -27,6 +27,13 @@ interface Driver { */ public function userAdd(string $user, string $password = null): ?string; + /** Renames a user + * + * The implementation must retain all user metadata as well as the + * user's password + */ + public function userRename(string $user, string $newName): bool; + /** Removes a user */ public function userRemove(string $user): bool; @@ -44,7 +51,7 @@ interface Driver { * @param string|null $password The cleartext password to assign to the user, or null to generate a random password * @param string|null $oldPassword The user's previous password, if known */ - public function userPasswordSet(string $user, ?string $newPassword, string $oldPassword = null); + public function userPasswordSet(string $user, ?string $newPassword, string $oldPassword = null): ?string; /** Removes a user's password; this makes authentication fail unconditionally * diff --git a/lib/User/Internal/Driver.php b/lib/User/Internal/Driver.php index 27486fb..80f16bb 100644 --- a/lib/User/Internal/Driver.php +++ b/lib/User/Internal/Driver.php @@ -40,6 +40,16 @@ class Driver implements \JKingWeb\Arsse\User\Driver { return $password; } + public function userRename(string $user, string $newName): bool { + // do nothing: the internal database is updated regardless of what the driver does (assuming it does not throw an exception) + // throw an exception if the user does not exist + if (!$this->userExists($user)) { + throw new ExceptionConflict("doesNotExist", ['action' => __FUNCTION__, 'user' => $user]); + } else { + return !($user === $newName); + } + } + public function userRemove(string $user): bool { return Arsse::$db->userRemove($user); } @@ -50,14 +60,19 @@ class Driver implements \JKingWeb\Arsse\User\Driver { public function userPasswordSet(string $user, ?string $newPassword, string $oldPassword = null): ?string { // do nothing: the internal database is updated regardless of what the driver does (assuming it does not throw an exception) - return $newPassword; + // throw an exception if the user does not exist + if (!$this->userExists($user)) { + throw new ExceptionConflict("doesNotExist", ['action' => __FUNCTION__, 'user' => $user]); + } else { + return $newPassword; + } } public function userPasswordUnset(string $user, string $oldPassword = null): bool { // do nothing: the internal database is updated regardless of what the driver does (assuming it does not throw an exception) // throw an exception if the user does not exist if (!$this->userExists($user)) { - throw new ExceptionConflict("doesNotExist", ['action' => "userPasswordUnset", 'user' => $user]); + throw new ExceptionConflict("doesNotExist", ['action' => __FUNCTION__, 'user' => $user]); } else { return true; } @@ -74,7 +89,7 @@ class Driver implements \JKingWeb\Arsse\User\Driver { public function userPropertiesGet(string $user, bool $includeLarge = true): array { // do nothing: the internal database will retrieve everything for us if (!$this->userExists($user)) { - throw new ExceptionConflict("doesNotExist", ['action' => "userPasswordUnset", 'user' => $user]); + throw new ExceptionConflict("doesNotExist", ['action' => __FUNCTION__, 'user' => $user]); } else { return []; } @@ -83,7 +98,7 @@ class Driver implements \JKingWeb\Arsse\User\Driver { public function userPropertiesSet(string $user, array $data): array { // do nothing: the internal database will set everything for us if (!$this->userExists($user)) { - throw new ExceptionConflict("doesNotExist", ['action' => "userPasswordUnset", 'user' => $user]); + throw new ExceptionConflict("doesNotExist", ['action' => __FUNCTION__, 'user' => $user]); } else { return $data; } diff --git a/tests/cases/Database/SeriesUser.php b/tests/cases/Database/SeriesUser.php index af591d4..0cd4ffb 100644 --- a/tests/cases/Database/SeriesUser.php +++ b/tests/cases/Database/SeriesUser.php @@ -180,4 +180,29 @@ trait SeriesUser { $this->assertException("doesNotExist", "User", "ExceptionConflict"); Arsse::$db->userLookup(2112); } + + public function testRenameAUser(): void { + $this->assertTrue(Arsse::$db->userRename("john.doe@example.com", "juan.doe@example.com")); + $state = $this->primeExpectations($this->data, [ + 'arsse_users' => ['id', 'num'], + 'arsse_user_meta' => ["owner", "key", "value"] + ]); + $state['arsse_users']['rows'][2][0] = "juan.doe@example.com"; + $state['arsse_user_meta']['rows'][6][0] = "juan.doe@example.com"; + $this->compareExpectations(static::$drv, $state); + } + + public function testRenameAUserToTheSameName(): void { + $this->assertFalse(Arsse::$db->userRename("john.doe@example.com", "john.doe@example.com")); + } + + public function testRenameAMissingUser(): void { + $this->assertException("doesNotExist", "User", "ExceptionConflict"); + Arsse::$db->userRename("juan.doe@example.com", "john.doe@example.com"); + } + + public function testRenameAUserToADuplicateName(): void { + $this->assertException("alreadyExists", "User", "ExceptionConflict"); + Arsse::$db->userRename("john.doe@example.com", "jane.doe@example.com"); + } } diff --git a/tests/cases/User/TestInternal.php b/tests/cases/User/TestInternal.php index c703835..858a876 100644 --- a/tests/cases/User/TestInternal.php +++ b/tests/cases/User/TestInternal.php @@ -9,6 +9,7 @@ namespace JKingWeb\Arsse\TestCase\User; use JKingWeb\Arsse\Arsse; use JKingWeb\Arsse\Database; use JKingWeb\Arsse\User\Driver as DriverInterface; +use JKingWeb\Arsse\User\ExceptionConflict; use JKingWeb\Arsse\User\Internal\Driver; /** @covers \JKingWeb\Arsse\User\Internal\Driver */ @@ -88,6 +89,21 @@ class TestInternal extends \JKingWeb\Arsse\Test\AbstractTest { \Phake::verify(Arsse::$db)->userAdd; } + public function testRenameAUser(): void { + $john = "john.doe@example.com"; + \Phake::when(Arsse::$db)->userExists->thenReturn(true); + $this->assertTrue((new Driver)->userRename($john, "jane.doe@example.com")); + $this->assertFalse((new Driver)->userRename($john, $john)); + \Phake::verify(Arsse::$db, \Phake::times(2))->userExists($john); + } + + public function testRenameAMissingUser(): void { + $john = "john.doe@example.com"; + \Phake::when(Arsse::$db)->userExists->thenReturn(false); + $this->assertException("doesNotExist", "User", "ExceptionConflict"); + (new Driver)->userRename($john, "jane.doe@example.com"); + } + public function testRemoveAUser(): void { $john = "john.doe@example.com"; \Phake::when(Arsse::$db)->userRemove->thenReturn(true)->thenThrow(new \JKingWeb\Arsse\User\ExceptionConflict("doesNotExist")); @@ -104,12 +120,18 @@ class TestInternal extends \JKingWeb\Arsse\Test\AbstractTest { public function testSetAPassword(): void { $john = "john.doe@example.com"; - \Phake::verifyNoFurtherInteraction(Arsse::$db); + \Phake::when(Arsse::$db)->userExists->thenReturn(true); $this->assertSame("superman", (new Driver)->userPasswordSet($john, "superman")); $this->assertSame(null, (new Driver)->userPasswordSet($john, null)); \Phake::verify(Arsse::$db, \Phake::times(0))->userPasswordSet; } + public function testSetAPasswordForAMssingUser(): void { + \Phake::when(Arsse::$db)->userExists->thenReturn(false); + $this->assertException("doesNotExist", "User", "ExceptionConflict"); + (new Driver)->userPasswordSet("john.doe@example.com", "secret"); + } + public function testUnsetAPassword(): void { \Phake::when(Arsse::$db)->userExists->thenReturn(true); $this->assertTrue((new Driver)->userPasswordUnset("john.doe@example.com")); diff --git a/tests/cases/User/TestUser.php b/tests/cases/User/TestUser.php index 597a158..e42832e 100644 --- a/tests/cases/User/TestUser.php +++ b/tests/cases/User/TestUser.php @@ -9,6 +9,7 @@ namespace JKingWeb\Arsse\TestCase\User; use JKingWeb\Arsse\Arsse; use JKingWeb\Arsse\Database; use JKingWeb\Arsse\User; +use JKingWeb\Arsse\Db\Transaction; use JKingWeb\Arsse\User\ExceptionConflict; use JKingWeb\Arsse\User\ExceptionInput; use JKingWeb\Arsse\User\Driver; @@ -43,6 +44,13 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { $this->assertSame("", (string) $u); } + public function testStartATransaction(): void { + \Phake::when(Arsse::$db)->begin->thenReturn(\Phake::mock(Transaction::class)); + $u = new User($this->drv); + $this->assertInstanceOf(Transaction::class, $u->begin()); + \Phake::verify(Arsse::$db)->begin(); + } + public function testGeneratePasswords(): void { $u = new User($this->drv); $pass1 = $u->generatePassword(); @@ -174,9 +182,48 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { \Phake::verify(Arsse::$db)->userExists($user); } + public function testRenameAUser(): void { + \Phake::when(Arsse::$db)->userExists->thenReturn(true); + \Phake::when(Arsse::$db)->userAdd->thenReturn(true); + \Phake::when(Arsse::$db)->userRename->thenReturn(true); + \Phake::when($this->drv)->userRename->thenReturn(true); + $u = new User($this->drv); + $old = "john.doe@example.com"; + $new = "jane.doe@example.com"; + $this->assertTrue($u->rename($old, $new)); + \Phake::verify($this->drv)->userRename($old, $new); + \Phake::verify(Arsse::$db)->userExists($old); + \Phake::verify(Arsse::$db)->userRename($old, $new); + } + + public function testRenameAUserWeDoNotKnow(): void { + \Phake::when(Arsse::$db)->userExists->thenReturn(false); + \Phake::when(Arsse::$db)->userAdd->thenReturn(true); + \Phake::when(Arsse::$db)->userRename->thenReturn(true); + \Phake::when($this->drv)->userRename->thenReturn(true); + $u = new User($this->drv); + $old = "john.doe@example.com"; + $new = "jane.doe@example.com"; + $this->assertTrue($u->rename($old, $new)); + \Phake::verify($this->drv)->userRename($old, $new); + \Phake::verify(Arsse::$db)->userExists($old); + \Phake::verify(Arsse::$db)->userAdd($new, null); + } + + public function testRenameAUserWithoutEffect(): void { + \Phake::when(Arsse::$db)->userExists->thenReturn(false); + \Phake::when(Arsse::$db)->userAdd->thenReturn(true); + \Phake::when(Arsse::$db)->userRename->thenReturn(true); + \Phake::when($this->drv)->userRename->thenReturn(false); + $u = new User($this->drv); + $old = "john.doe@example.com"; + $new = "jane.doe@example.com"; + $this->assertFalse($u->rename($old, $old)); + \Phake::verify($this->drv)->userRename($old, $old); + } + 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); @@ -188,7 +235,6 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { 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);