From 931fe3b585e3d0b4b9f15698cc32e7f189c7f216 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Fri, 2 Nov 2018 11:52:55 -0400 Subject: [PATCH] Move password generation to the User class This allows user drivers which wish to generate their own passwords to do so, and those which do not to defer to the built-in generator --- lib/Database.php | 27 +++++++------------------ lib/User.php | 12 +++++++---- lib/User/Driver.php | 4 ++-- lib/User/Internal/Driver.php | 13 ++++++++---- tests/lib/Database/SeriesUser.php | 33 +++---------------------------- 5 files changed, 29 insertions(+), 60 deletions(-) diff --git a/lib/Database.php b/lib/Database.php index 03300502..7182c6f7 100644 --- a/lib/Database.php +++ b/lib/Database.php @@ -6,7 +6,6 @@ declare(strict_types=1); namespace JKingWeb\Arsse; -use PasswordGenerator\Generator as PassGen; use JKingWeb\DrUUID\UUID; use JKingWeb\Arsse\Misc\Query; use JKingWeb\Arsse\Misc\Context; @@ -83,7 +82,7 @@ class Database { return $out; } - protected function generateIn(array $values, string $type) { + protected function generateIn(array $values, string $type): array { $out = [ [], // query clause [], // binding types @@ -122,21 +121,15 @@ class Database { return (bool) $this->db->prepare("SELECT count(*) from arsse_users where id = ?", "str")->run($user)->getValue(); } - public function userAdd(string $user, string $password = null): string { + public function userAdd(string $user, string $password): bool { if (!Arsse::$user->authorize($user, __FUNCTION__)) { throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); } elseif ($this->userExists($user)) { throw new User\Exception("alreadyExists", ["action" => __FUNCTION__, "user" => $user]); } - if ($password===null) { - $password = (new PassGen)->length(Arsse::$conf->userTempPasswordLength)->get(); - } - $hash = ""; - if (strlen($password) > 0) { - $hash = password_hash($password, \PASSWORD_DEFAULT); - } + $hash = (strlen($password) > 0) ? password_hash($password, \PASSWORD_DEFAULT) : ""; $this->db->prepare("INSERT INTO arsse_users(id,password) values(?,?)", "str", "str")->runArray([$user,$hash]); - return $password; + return true; } public function userRemove(string $user): bool { @@ -169,21 +162,15 @@ class Database { return (string) $this->db->prepare("SELECT password from arsse_users where id = ?", "str")->run($user)->getValue(); } - public function userPasswordSet(string $user, string $password = null): string { + public function userPasswordSet(string $user, string $password): 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]); } - if ($password===null) { - $password = (new PassGen)->length(Arsse::$conf->userTempPasswordLength)->get(); - } - $hash = ""; - if (strlen($password) > 0) { - $hash = password_hash($password, \PASSWORD_DEFAULT); - } + $hash = (strlen($password) > 0) ? password_hash($password, \PASSWORD_DEFAULT) : ""; $this->db->prepare("UPDATE arsse_users set password = ? where id = ?", "str", "str")->run($hash, $user); - return $password; + return true; } public function sessionCreate(string $user): string { diff --git a/lib/User.php b/lib/User.php index 6e474645..b0b05bcd 100644 --- a/lib/User.php +++ b/lib/User.php @@ -6,7 +6,7 @@ declare(strict_types=1); namespace JKingWeb\Arsse; -use JKingWeb\Arsse\User\Internal\Driver as InternalDriver; +use PasswordGenerator\Generator as PassGen; class User { @@ -80,7 +80,7 @@ class User { if (!$this->authorize($user, $func)) { throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); } - return $this->u->userAdd($user, $password); + return $this->u->userAdd($user, $password) ?? $this->u->userAdd($user, $this->generatePassword()); } public function remove(string $user): bool { @@ -103,11 +103,15 @@ class User { if (!$this->authorize($user, $func)) { throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); } - $out = $this->u->userPasswordSet($user, $newPassword, $oldPassword); - if (!$this->u instanceof InternalDriver && Arsse::$db->userExists($user)) { + $out = $this->u->userPasswordSet($user, $newPassword, $oldPassword) ?? $this->u->userPasswordSet($user, $this->generatePassword(), $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, $out); } return $out; } + + protected function generatePassword(): string { + return (new PassGen)->length(Arsse::$conf->userTempPasswordLength)->get(); + } } diff --git a/lib/User/Driver.php b/lib/User/Driver.php index 91aeaa0e..50ef8f3b 100644 --- a/lib/User/Driver.php +++ b/lib/User/Driver.php @@ -22,11 +22,11 @@ interface Driver { // checks whether a user exists public function userExists(string $user): bool; // adds a user - public function userAdd(string $user, string $password = null): string; + public function userAdd(string $user, string $password = null); // removes a user public function userRemove(string $user): bool; // lists all users 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): string; + public function userPasswordSet(string $user, string $newPassword = null, string $oldPassword = null); } diff --git a/lib/User/Internal/Driver.php b/lib/User/Internal/Driver.php index 5d65b764..b6dfe96c 100644 --- a/lib/User/Internal/Driver.php +++ b/lib/User/Internal/Driver.php @@ -34,8 +34,12 @@ class Driver implements \JKingWeb\Arsse\User\Driver { return Arsse::$db->userExists($user); } - public function userAdd(string $user, string $password = null): string { - return Arsse::$db->userAdd($user, $password); + public function userAdd(string $user, string $password = null) { + if (isset($password)) { + // only add the user if the password is not null; the user manager will retry with a generated password if null is returned + Arsse::$db->userAdd($user, $password); + } + return $password; } public function userRemove(string $user): bool { @@ -46,7 +50,8 @@ class Driver implements \JKingWeb\Arsse\User\Driver { return Arsse::$db->userList(); } - public function userPasswordSet(string $user, string $newPassword = null, string $oldPassword = null): string { - return Arsse::$db->userPasswordSet($user, $newPassword); + public function userPasswordSet(string $user, string $newPassword = null, string $oldPassword = null) { + // do nothing: the internal database is updated regardless of what the driver does (assuming it does not throw an exception) + return $newPassword; } } diff --git a/tests/lib/Database/SeriesUser.php b/tests/lib/Database/SeriesUser.php index ef704069..78d1f81b 100644 --- a/tests/lib/Database/SeriesUser.php +++ b/tests/lib/Database/SeriesUser.php @@ -60,35 +60,13 @@ trait SeriesUser { } public function testAddANewUser() { - $this->assertSame("", Arsse::$db->userAdd("john.doe@example.org", "")); + $this->assertTrue(Arsse::$db->userAdd("john.doe@example.org", "")); Phake::verify(Arsse::$user)->authorize("john.doe@example.org", "userAdd"); $state = $this->primeExpectations($this->data, ['arsse_users' => ['id','name','rights']]); $state['arsse_users']['rows'][] = ["john.doe@example.org", null, 0]; $this->compareExpectations($state); } - /** - * @depends testGetAPassword - * @depends testAddANewUser - */ - public function testAddANewUserWithARandomPassword() { - $user1 = "john.doe@example.org"; - $user2 = "john.doe@example.net"; - $pass1 = Arsse::$db->userAdd($user1); - $pass2 = Arsse::$db->userAdd($user2); - $this->assertSame(Arsse::$conf->userTempPasswordLength, strlen($pass1)); - $this->assertSame(Arsse::$conf->userTempPasswordLength, strlen($pass2)); - $this->assertNotEquals($pass1, $pass2); - $hash1 = Arsse::$db->userPasswordGet($user1); - $hash2 = Arsse::$db->userPasswordGet($user2); - Phake::verify(Arsse::$user)->authorize($user1, "userAdd"); - Phake::verify(Arsse::$user)->authorize($user2, "userAdd"); - Phake::verify(Arsse::$user)->authorize($user1, "userPasswordGet"); - Phake::verify(Arsse::$user)->authorize($user2, "userPasswordGet"); - $this->assertTrue(password_verify($pass1, $hash1), "Failed verifying password of $user1 '$pass1' against hash '$hash1'."); - $this->assertTrue(password_verify($pass2, $hash2), "Failed verifying password of $user2 '$pass2' against hash '$hash2'."); - } - public function testAddAnExistingUser() { $this->assertException("alreadyExists", "User"); Arsse::$db->userAdd("john.doe@example.com", ""); @@ -136,19 +114,14 @@ trait SeriesUser { */ public function testSetAPassword() { $user = "john.doe@example.com"; + $pass = "secret"; $this->assertEquals("", Arsse::$db->userPasswordGet($user)); - $pass = Arsse::$db->userPasswordSet($user, "secret"); + $this->assertTrue(Arsse::$db->userPasswordSet($user, $pass)); $hash = Arsse::$db->userPasswordGet($user); $this->assertNotEquals("", $hash); Phake::verify(Arsse::$user)->authorize($user, "userPasswordSet"); $this->assertTrue(password_verify($pass, $hash), "Failed verifying password of $user '$pass' against hash '$hash'."); } - public function testSetARandomPassword() { - $user = "john.doe@example.com"; - $this->assertEquals("", Arsse::$db->userPasswordGet($user)); - $pass = Arsse::$db->userPasswordSet($user); - $hash = Arsse::$db->userPasswordGet($user); - } public function testSetThePasswordOfAMissingUser() { $this->assertException("doesNotExist", "User");