From e45ba3f0eaa398a159eeac278d9491c6ad11f08a Mon Sep 17 00:00:00 2001 From: "J. King" Date: Sun, 24 Mar 2019 14:42:23 -0400 Subject: [PATCH] Add means of unsetting a password in the backend --- lib/Database.php | 10 +++--- lib/User.php | 13 ++++++++ lib/User/Driver.php | 2 ++ lib/User/Internal/Driver.php | 15 ++++++++- tests/cases/Database/SeriesUser.php | 7 ++++ tests/cases/User/TestInternal.php | 52 ++++++++++++++++++++--------- tests/cases/User/TestUser.php | 38 +++++++++++++++++++++ 7 files changed, 115 insertions(+), 22 deletions(-) diff --git a/lib/Database.php b/lib/Database.php index df614b5..bfbcfee 100644 --- a/lib/Database.php +++ b/lib/Database.php @@ -289,27 +289,27 @@ class Database { } /** Retrieves the hashed password of a user */ - public function userPasswordGet(string $user): string { + public function userPasswordGet(string $user) { if (!Arsse::$user->authorize($user, __FUNCTION__)) { throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); } elseif (!$this->userExists($user)) { throw new User\Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); } - return (string) $this->db->prepare("SELECT password from arsse_users where id = ?", "str")->run($user)->getValue(); + return $this->db->prepare("SELECT password from arsse_users where id = ?", "str")->run($user)->getValue(); } /** Sets the password of an existing user * * @param string $user The user for whom to set the password - * @param string $password The new password, in cleartext. The password will be stored hashed + * @param string $password The new password, in cleartext. The password will be stored hashed. If null is passed, the password is unset and authentication not possible */ - public function userPasswordSet(string $user, string $password): bool { + public function userPasswordSet(string $user, string $password = null): bool { if (!Arsse::$user->authorize($user, __FUNCTION__)) { throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); } elseif (!$this->userExists($user)) { throw new User\Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); } - $hash = (strlen($password) > 0) ? password_hash($password, \PASSWORD_DEFAULT) : ""; + $hash = (strlen($password ?? "") > 0) ? password_hash($password, \PASSWORD_DEFAULT) : $password; $this->db->prepare("UPDATE arsse_users set password = ? where id = ?", "str", "str")->run($hash, $user); return true; } diff --git a/lib/User.php b/lib/User.php index 82e8d3d..4f52980 100644 --- a/lib/User.php +++ b/lib/User.php @@ -114,6 +114,19 @@ class User { return $out; } + public function passwordUnset(string $user, $oldPassword = null): bool { + $func = "userPasswordUnset"; + if (!$this->authorize($user, $func)) { + throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); + } + $out = $this->u->userPasswordUnset($user, $oldPassword); + if (Arsse::$db->userExists($user)) { + // if the password change was successful and the user exists, set the internal password to the same value + Arsse::$db->userPasswordSet($user, null); + } + return $out; + } + public function generatePassword(): string { return (new PassGen)->length(Arsse::$conf->userTempPasswordLength)->get(); } diff --git a/lib/User/Driver.php b/lib/User/Driver.php index 50ef8f3..b5657ac 100644 --- a/lib/User/Driver.php +++ b/lib/User/Driver.php @@ -29,4 +29,6 @@ interface Driver { public function userList(): array; // sets a user's password; if the driver does not require the old password, it may be ignored public function userPasswordSet(string $user, string $newPassword = null, string $oldPassword = null); + // removes a user's password; this makes authentication fail unconditionally + public function userPasswordUnset(string $user, string $oldPassword = null): bool; } diff --git a/lib/User/Internal/Driver.php b/lib/User/Internal/Driver.php index 4c73025..d50777a 100644 --- a/lib/User/Internal/Driver.php +++ b/lib/User/Internal/Driver.php @@ -20,6 +20,9 @@ class Driver implements \JKingWeb\Arsse\User\Driver { public function auth(string $user, string $password): bool { try { $hash = $this->userPasswordGet($user); + if (is_null($hash)) { + return false; + } } catch (Exception $e) { return false; } @@ -58,7 +61,17 @@ class Driver implements \JKingWeb\Arsse\User\Driver { return $newPassword; } - protected function userPasswordGet(string $user): string { + 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 Exception("doesNotExist", ['action' => "userPasswordUnset", 'user' => $user]); + } else { + return true; + } + } + + protected function userPasswordGet(string $user) { return Arsse::$db->userPasswordGet($user); } } diff --git a/tests/cases/Database/SeriesUser.php b/tests/cases/Database/SeriesUser.php index 991577a..8036bee 100644 --- a/tests/cases/Database/SeriesUser.php +++ b/tests/cases/Database/SeriesUser.php @@ -127,6 +127,13 @@ trait SeriesUser { $this->assertTrue(password_verify($pass, $hash), "Failed verifying password of $user '$pass' against hash '$hash'."); } + public function testUnsetAPassword() { + $user = "john.doe@example.com"; + $this->assertEquals("", Arsse::$db->userPasswordGet($user)); + $this->assertTrue(Arsse::$db->userPasswordSet($user, null)); + $this->assertNull(Arsse::$db->userPasswordGet($user)); + } + public function testSetThePasswordOfAMissingUser() { $this->assertException("doesNotExist", "User"); Arsse::$db->userPasswordSet("john.doe@example.org", "secret"); diff --git a/tests/cases/User/TestInternal.php b/tests/cases/User/TestInternal.php index f7f042d..29d9923 100644 --- a/tests/cases/User/TestInternal.php +++ b/tests/cases/User/TestInternal.php @@ -37,12 +37,13 @@ class TestInternal extends \JKingWeb\Arsse\Test\AbstractTest { * @dataProvider provideAuthentication * @group slow */ - public function testAuthenticateAUser(bool $authorized, string $user, string $password, bool $exp) { + public function testAuthenticateAUser(bool $authorized, string $user, $password, bool $exp) { if ($authorized) { Phake::when(Arsse::$db)->userPasswordGet("john.doe@example.com")->thenReturn('$2y$10$1zbqRJhxM8uUjeSBPp4IhO90xrqK0XjEh9Z16iIYEFRV4U.zeAFom'); // hash of "secret" Phake::when(Arsse::$db)->userPasswordGet("jane.doe@example.com")->thenReturn('$2y$10$bK1ljXfTSyc2D.NYvT.Eq..OpehLRXVbglW.23ihVuyhgwJCd.7Im'); // hash of "superman" Phake::when(Arsse::$db)->userPasswordGet("owen.hardy@example.com")->thenReturn(""); Phake::when(Arsse::$db)->userPasswordGet("kira.nerys@example.com")->thenThrow(new \JKingWeb\Arsse\User\Exception("doesNotExist")); + Phake::when(Arsse::$db)->userPasswordGet("007@example.com")->thenReturn(null); } else { Phake::when(Arsse::$db)->userPasswordGet->thenThrow(new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")); } @@ -54,22 +55,26 @@ class TestInternal extends \JKingWeb\Arsse\Test\AbstractTest { $jane = "jane.doe@example.com"; $owen = "owen.hardy@example.com"; $kira = "kira.nerys@example.com"; + $bond = "007@example.com"; return [ - [false, $john, "secret", false], - [false, $jane, "superman", false], - [false, $owen, "", false], - [false, $kira, "ashalla", false], - [true, $john, "secret", true], - [true, $jane, "superman", true], - [true, $owen, "", true], - [true, $kira, "ashalla", false], - [true, $john, "top secret", false], - [true, $jane, "clark kent", false], - [true, $owen, "watchmaker", false], - [true, $kira, "singha", false], - [true, $john, "", false], - [true, $jane, "", false], - [true, $kira, "", false], + [false, $john, "secret", false], + [false, $jane, "superman", false], + [false, $owen, "", false], + [false, $kira, "ashalla", false], + [false, $bond, "", false], + [true, $john, "secret", true], + [true, $jane, "superman", true], + [true, $owen, "", true], + [true, $kira, "ashalla", false], + [true, $john, "top secret", false], + [true, $jane, "clark kent", false], + [true, $owen, "watchmaker", false], + [true, $kira, "singha", false], + [true, $john, "", false], + [true, $jane, "", false], + [true, $kira, "", false], + [true, $bond, "for England", false], + [true, $bond, "", false], ]; } @@ -133,4 +138,19 @@ class TestInternal extends \JKingWeb\Arsse\Test\AbstractTest { $this->assertSame("superman", (new Driver)->userPasswordSet($john, "superman")); $this->assertSame(null, (new Driver)->userPasswordSet($john, null)); } + + public function testUnsetAPassword() { + $drv = \Phake::partialMock(Driver::class); + \Phake::when($drv)->userExists->thenReturn(true); + Phake::verifyNoFurtherInteraction(Arsse::$db); + $this->assertTrue($drv->userPasswordUnset("john.doe@example.com")); + } + + public function testUnsetAPasswordForAMssingUser() { + $drv = \Phake::partialMock(Driver::class); + \Phake::when($drv)->userExists->thenReturn(false); + Phake::verifyNoFurtherInteraction(Arsse::$db); + $this->assertException("doesNotExist", "User"); + $drv->userPasswordUnset("john.doe@example.com"); + } } diff --git a/tests/cases/User/TestUser.php b/tests/cases/User/TestUser.php index 9496c41..3584f1e 100644 --- a/tests/cases/User/TestUser.php +++ b/tests/cases/User/TestUser.php @@ -297,4 +297,42 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { [true, $jane, "secret", true, new \JKingWeb\Arsse\User\Exception("doesNotExist")], ]; } + + /** @dataProvider providePasswordClearings */ + public function testClearAPassword(bool $authorized, bool $exists, string $user, $exp) { + Phake::when($this->drv)->authorize->thenReturn($authorized); + Phake::when($this->drv)->userPasswordUnset->thenReturn(true); + Phake::when($this->drv)->userPasswordUnset("jane.doe@example.net", null)->thenThrow(new \JKingWeb\Arsse\User\Exception("doesNotExist")); + Phake::when(Arsse::$db)->userExists->thenReturn($exists); + $u = new User($this->drv); + try { + if ($exp instanceof \JKingWeb\Arsse\AbstractException) { + $this->assertException($exp); + $u->passwordUnset($user); + } else { + $this->assertSame($exp, $u->passwordUnset($user)); + } + } finally { + Phake::verify(Arsse::$db, Phake::times((int) ($authorized && $exists && is_bool($exp))))->userPasswordSet($user, null); + } + } + + public function providePasswordClearings() { + $forbidden = new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized"); + $missing = new \JKingWeb\Arsse\User\Exception("doesNotExist"); + return [ + [false, true, "jane.doe@example.com", $forbidden], + [false, true, "john.doe@example.com", $forbidden], + [false, true, "jane.doe@example.net", $forbidden], + [false, false, "jane.doe@example.com", $forbidden], + [false, false, "john.doe@example.com", $forbidden], + [false, false, "jane.doe@example.net", $forbidden], + [true, true, "jane.doe@example.com", true], + [true, true, "john.doe@example.com", true], + [true, true, "jane.doe@example.net", $missing], + [true, false, "jane.doe@example.com", true], + [true, false, "john.doe@example.com", true], + [true, false, "jane.doe@example.net", $missing], + ]; + } }