From 11747c93fd62f2a758a8a8d6583eedfe7ee89f38 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Sun, 28 Oct 2018 10:59:17 -0400 Subject: [PATCH 01/16] Strip out unused user management functionality Tests have been removed as well; new tests are forthcoming --- README.md | 3 +- lib/CLI.php | 2 - lib/Database.php | 72 +---- lib/REST.php | 2 +- lib/REST/NextCloudNews/V1_2.php | 16 - lib/User.php | 332 ++----------------- lib/User/Driver.php | 16 +- lib/User/Internal/Driver.php | 4 - lib/User/Internal/InternalFunctions.php | 20 +- locale/en.php | 5 +- tests/cases/REST/NextCloudNews/TestV1_2.php | 19 +- tests/cases/REST/TinyTinyRSS/TestAPI.php | 1 - tests/cases/User/TestAuthorization.php | 338 -------------------- tests/cases/User/TestMockExternal.php | 17 - tests/cases/User/TestMockInternal.php | 23 -- tests/cases/User/Testnternal.php | 20 -- tests/lib/Database/SeriesUser.php | 121 +------ tests/lib/User/CommonTests.php | 154 --------- tests/lib/User/Database.php | 133 -------- tests/lib/User/DriverExternalMock.php | 127 -------- tests/lib/User/DriverInternalMock.php | 56 ---- tests/lib/User/DriverSkeleton.php | 72 ----- tests/phpunit.xml | 6 +- 23 files changed, 54 insertions(+), 1505 deletions(-) delete mode 100644 tests/cases/User/TestAuthorization.php delete mode 100644 tests/cases/User/TestMockExternal.php delete mode 100644 tests/cases/User/TestMockInternal.php delete mode 100644 tests/cases/User/Testnternal.php delete mode 100644 tests/lib/User/CommonTests.php delete mode 100644 tests/lib/User/Database.php delete mode 100644 tests/lib/User/DriverExternalMock.php delete mode 100644 tests/lib/User/DriverInternalMock.php delete mode 100644 tests/lib/User/DriverSkeleton.php diff --git a/README.md b/README.md index 7fd86cef..54a87197 100644 --- a/README.md +++ b/README.md @@ -93,7 +93,8 @@ As a general rule, The Arsse should yield the same output as the reference imple - When marking articles as starred the feed ID is ignored, as they are not needed to establish uniqueness - The feed updater ignores the `userId` parameter: feeds in The Arsse are deduplicated, and have no owner - The `/feeds/all` route lists only feeds which should be checked for updates, and it also returns all `userId` attributes as empty strings: feeds in The Arsse are deduplicated, and have no owner -- The updater console commands mentioned in the protocol specification are not implemented, as The Arsse does not implement the required NextCloud subsystems +- The API's "updater" routes do not require administrator priviledges as The Arsse has no concept of user classes +- The "updater" console commands mentioned in the protocol specification are not implemented, as The Arsse does not implement the required NextCloud subsystems - The `lastLoginTimestamp` attribute of the user metadata is always the current time: The Arsse's implementation of the protocol is fully stateless #### Ambiguities diff --git a/lib/CLI.php b/lib/CLI.php index 1605efe4..b116d1a2 100644 --- a/lib/CLI.php +++ b/lib/CLI.php @@ -38,8 +38,6 @@ USAGE_TEXT; protected function loadConf(): bool { $conf = file_exists(BASE."config.php") ? new Conf(BASE."config.php") : new Conf; Arsse::load($conf); - // command-line operations will never respect authorization - Arsse::$user->authorizationEnabled(false); return true; } diff --git a/lib/Database.php b/lib/Database.php index b615a275..03300502 100644 --- a/lib/Database.php +++ b/lib/Database.php @@ -149,24 +149,13 @@ class Database { return true; } - public function userList(string $domain = null): array { + public function userList(): array { $out = []; - if ($domain !== null) { - if (!Arsse::$user->authorize("@".$domain, __FUNCTION__)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $domain]); - } - $domain = str_replace(["\\","%","_"], ["\\\\", "\\%", "\\_"], $domain); - $domain = "%@".$domain; - foreach ($this->db->prepare("SELECT id from arsse_users where id like ?", "str")->run($domain) as $user) { - $out[] = $user['id']; - } - } else { - if (!Arsse::$user->authorize("", __FUNCTION__)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => "global"]); - } - foreach ($this->db->query("SELECT id from arsse_users") as $user) { - $out[] = $user['id']; - } + if (!Arsse::$user->authorize("", __FUNCTION__)) { + throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => ""]); + } + foreach ($this->db->query("SELECT id from arsse_users") as $user) { + $out[] = $user['id']; } return $out; } @@ -197,52 +186,6 @@ class Database { return $password; } - public function userPropertiesGet(string $user): array { - if (!Arsse::$user->authorize($user, __FUNCTION__)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); - } - $prop = $this->db->prepare("SELECT name,rights from arsse_users where id = ?", "str")->run($user)->getRow(); - if (!$prop) { - throw new User\Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); - } - return $prop; - } - - public function userPropertiesSet(string $user, array $properties): array { - 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]); - } - $valid = [ // FIXME: add future properties - "name" => "str", - ]; - list($setClause, $setTypes, $setValues) = $this->generateSet($properties, $valid); - if (!$setClause) { - // if no changes would actually be applied, just return - return $this->userPropertiesGet($user); - } - $this->db->prepare("UPDATE arsse_users set $setClause where id = ?", $setTypes, "str")->run($setValues, $user); - return $this->userPropertiesGet($user); - } - - public function userRightsGet(string $user): int { - if (!Arsse::$user->authorize($user, __FUNCTION__)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); - } - return (int) $this->db->prepare("SELECT rights from arsse_users where id = ?", "str")->run($user)->getValue(); - } - - public function userRightsSet(string $user, int $rights): bool { - if (!Arsse::$user->authorize($user, __FUNCTION__, $rights)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); - } elseif (!$this->userExists($user)) { - throw new User\Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); - } - $this->db->prepare("UPDATE arsse_users set rights = ? where id = ?", "int", "str")->run($rights, $user); - return true; - } - public function sessionCreate(string $user): string { // If the user isn't authorized to perform this action then throw an exception. if (!Arsse::$user->authorize($user, __FUNCTION__)) { @@ -596,10 +539,7 @@ class Database { if (!ValueInfo::id($id)) { throw new Db\ExceptionInput("typeViolation", ["action" => __FUNCTION__, "field" => "feed", 'type' => "int > 0"]); } - // disable authorization checks for the list call - Arsse::$user->authorizationEnabled(false); $sub = $this->subscriptionList($user, null, true, (int) $id)->getRow(); - Arsse::$user->authorizationEnabled(true); if (!$sub) { throw new Db\ExceptionInput("subjectMissing", ["action" => __FUNCTION__, "field" => "feed", 'id' => $id]); } diff --git a/lib/REST.php b/lib/REST.php index 0dcd6ecd..ac527f1d 100644 --- a/lib/REST.php +++ b/lib/REST.php @@ -135,7 +135,7 @@ class REST { $user = $env['REMOTE_USER']; } if (strlen($user)) { - if (Arsse::$user->auth($user, $password)) { + if (Arsse::$user->auth((string) $user, (string) $password)) { $req = $req->withAttribute("authenticated", true); $req = $req->withAttribute("authenticatedUser", $user); } else { diff --git a/lib/REST/NextCloudNews/V1_2.php b/lib/REST/NextCloudNews/V1_2.php index eb78d969..0ae19792 100644 --- a/lib/REST/NextCloudNews/V1_2.php +++ b/lib/REST/NextCloudNews/V1_2.php @@ -365,10 +365,6 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler { // return list of feeds which should be refreshed protected function feedListStale(array $url, array $data): ResponseInterface { - // function requires admin rights per spec - if (Arsse::$user->rightsGet(Arsse::$user->id)==User::RIGHTS_NONE) { - return new EmptyResponse(403); - } // list stale feeds which should be checked for updates $feeds = Arsse::$db->feedListStale(); $out = []; @@ -381,10 +377,6 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler { // refresh a feed protected function feedUpdate(array $url, array $data): ResponseInterface { - // function requires admin rights per spec - if (Arsse::$user->rightsGet(Arsse::$user->id)==User::RIGHTS_NONE) { - return new EmptyResponse(403); - } try { Arsse::$db->feedUpdate($data['feedId']); } catch (ExceptionInput $e) { @@ -680,19 +672,11 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler { } protected function cleanupBefore(array $url, array $data): ResponseInterface { - // function requires admin rights per spec - if (Arsse::$user->rightsGet(Arsse::$user->id)==User::RIGHTS_NONE) { - return new EmptyResponse(403); - } Service::cleanupPre(); return new EmptyResponse(204); } protected function cleanupAfter(array $url, array $data): ResponseInterface { - // function requires admin rights per spec - if (Arsse::$user->rightsGet(Arsse::$user->id)==User::RIGHTS_NONE) { - return new EmptyResponse(403); - } Service::cleanupPost(); return new EmptyResponse(204); } diff --git a/lib/User.php b/lib/User.php index 2c2ad05a..2e130345 100644 --- a/lib/User.php +++ b/lib/User.php @@ -7,11 +7,6 @@ declare(strict_types=1); namespace JKingWeb\Arsse; class User { - const RIGHTS_NONE = 0; // normal user - const RIGHTS_DOMAIN_MANAGER = 25; // able to act for any normal users on same domain; cannot elevate other users - const RIGHTS_DOMAIN_ADMIN = 50; // able to act for any users on same domain not above themselves; may elevate users on same domain to domain manager or domain admin - const RIGHTS_GLOBAL_MANAGER = 75; // able to act for any normal users on any domain; cannot elevate other users - const RIGHTS_GLOBAL_ADMIN = 100; // is completely unrestricted public $id = null; @@ -19,9 +14,6 @@ class User { * @var User\Driver */ protected $u; - protected $authz = 0; - protected $authzSupported = 0; - protected $actor = []; public static function driverList(): array { $sep = \DIRECTORY_SEPARATOR; @@ -41,165 +33,59 @@ class User { } public function __toString() { - if ($this->id===null) { - $this->credentials(); - } return (string) $this->id; } - // checks whether the logged in user is authorized to act for the affected user (used especially when granting rights) - public function authorize(string $affectedUser, string $action, int $newRightsLevel = 0): bool { - // if authorization checks are disabled (either because we're running the installer or the background updater) just return true - if (!$this->authorizationEnabled()) { - return true; - } - // if we don't have a logged-in user, fetch credentials - if ($this->id===null) { - $this->credentials(); - } - // if the affected user is the actor and the actor is not trying to grant themselves rights, accept the request - if ($affectedUser==Arsse::$user->id && $action != "userRightsSet") { - return true; - } - // if we're authorizing something other than a user function and the affected user is not the actor, make sure the affected user exists - $this->authorizationEnabled(false); - if (Arsse::$user->id != $affectedUser && strpos($action, "user")!==0 && !$this->exists($affectedUser)) { - throw new User\Exception("doesNotExist", ["action" => $action, "user" => $affectedUser]); - } - $this->authorizationEnabled(true); - // get properties of actor if not already available - if (!sizeof($this->actor)) { - $this->actor = $this->propertiesGet(Arsse::$user->id); - } - $rights = $this->actor["rights"]; - // if actor is a global admin, accept the request - if ($rights==User\Driver::RIGHTS_GLOBAL_ADMIN) { - return true; - } - // if actor is a common user, deny the request - if ($rights==User\Driver::RIGHTS_NONE) { - return false; - } - // if actor is not some other sort of admin, deny the request - if (!in_array($rights, [User\Driver::RIGHTS_GLOBAL_MANAGER,User\Driver::RIGHTS_DOMAIN_MANAGER,User\Driver::RIGHTS_DOMAIN_ADMIN], true)) { - return false; - } - // if actor is a domain admin/manager and domains don't match, deny the request - if ($this->actor["domain"] && $rights != User\Driver::RIGHTS_GLOBAL_MANAGER) { - $test = "@".$this->actor["domain"]; - if (substr($affectedUser, -1*strlen($test)) != $test) { - return false; - } - } - // certain actions shouldn't check affected user's rights - if (in_array($action, ["userRightsGet","userExists","userList"], true)) { - return true; - } - if ($action=="userRightsSet") { - // setting rights above your own is not allowed - if ($newRightsLevel > $rights) { - return false; - } - // setting yourself to rights you already have is harmless and can be allowed - if ($this->id==$affectedUser && $newRightsLevel==$rights) { - return true; - } - // managers can only set their own rights, and only to normal user - if (in_array($rights, [User\Driver::RIGHTS_DOMAIN_MANAGER, User\Driver::RIGHTS_GLOBAL_MANAGER])) { - if ($this->id != $affectedUser || $newRightsLevel != User\Driver::RIGHTS_NONE) { - return false; - } - return true; - } - } - $affectedRights = $this->rightsGet($affectedUser); - // managers can only act on themselves (checked above) or regular users - if (in_array($rights, [User\Driver::RIGHTS_GLOBAL_MANAGER,User\Driver::RIGHTS_DOMAIN_MANAGER]) && $affectedRights != User\Driver::RIGHTS_NONE) { - return false; - } - // domain admins canot act above themselves - if (!in_array($affectedRights, [User\Driver::RIGHTS_NONE,User\Driver::RIGHTS_DOMAIN_MANAGER,User\Driver::RIGHTS_DOMAIN_ADMIN])) { - return false; - } + // at one time there was a complicated authorization system; it exists vestigially to support a later revival if desired + public function authorize(string $affectedUser, string $action): bool { return true; } - public function credentials(): array { - if ($_SERVER['PHP_AUTH_USER']) { - $out = ["user" => $_SERVER['PHP_AUTH_USER'], "password" => $_SERVER['PHP_AUTH_PW']]; - } elseif ($_SERVER['REMOTE_USER']) { - $out = ["user" => $_SERVER['REMOTE_USER'], "password" => ""]; - } else { - $out = ["user" => "", "password" => ""]; - } - $this->id = $out["user"]; - return $out; - } - - public function auth(string $user = null, string $password = null): bool { - if ($user===null) { - return $this->authHTTP(); - } else { - $prevUser = $this->id ?? null; - $this->id = $user; - $this->actor = []; - switch ($this->u->driverFunctions("auth")) { - case User\Driver::FUNC_EXTERNAL: - if (Arsse::$conf->userPreAuth) { - $out = true; - } else { - $out = $this->u->auth($user, $password); - } - if ($out && !Arsse::$db->userExists($user)) { + public function auth(string $user, string $password): bool { + $prevUser = $this->id ?? null; + $this->id = $user; + switch ($this->u->driverFunctions("auth")) { + case User\Driver::FUNC_EXTERNAL: + if (Arsse::$conf->userPreAuth) { + $out = true; + } else { + $out = $this->u->auth($user, $password); + } + if ($out && !Arsse::$db->userExists($user)) { + $this->autoProvision($user, $password); + } + break; + case User\Driver::FUNC_INTERNAL: + if (Arsse::$conf->userPreAuth) { + if (!Arsse::$db->userExists($user)) { $this->autoProvision($user, $password); } - break; - case User\Driver::FUNC_INTERNAL: - if (Arsse::$conf->userPreAuth) { - if (!Arsse::$db->userExists($user)) { - $this->autoProvision($user, $password); - } - $out = true; - } else { - $out = $this->u->auth($user, $password); - } - break; - case User\Driver::FUNCT_NOT_IMPLEMENTED: - $out = false; - break; - } - if (!$out) { - $this->id = $prevUser; - } - return $out; + $out = true; + } else { + $out = $this->u->auth($user, $password); + } + break; + case User\Driver::FUNCT_NOT_IMPLEMENTED: + $out = false; + break; } - } - - public function authHTTP(): bool { - $cred = $this->credentials(); - if (!$cred["user"]) { - return false; + if (!$out) { + $this->id = $prevUser; } - return $this->auth($cred["user"], $cred["password"]); + return $out; } public function driverFunctions(string $function = null) { return $this->u->driverFunctions($function); } - public function list(string $domain = null): array { + public function list(): array { $func = "userList"; switch ($this->u->driverFunctions($func)) { case User\Driver::FUNC_EXTERNAL: // we handle authorization checks for external drivers - if ($domain===null) { - if (!$this->authorize("@".$domain, $func)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $domain]); - } - } else { - if (!$this->authorize("", $func)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => "all users"]); - } + if (!$this->authorize("", $func)) { + throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => ""]); } // no break case User\Driver::FUNC_INTERNAL: @@ -210,17 +96,6 @@ class User { } } - public function authorizationEnabled(bool $setting = null): bool { - if (is_null($setting)) { - return !$this->authz; - } - $this->authz += ($setting ? -1 : 1); - if ($this->authz < 0) { - $this->authz = 0; - } - return !$this->authz; - } - public function exists(string $user): bool { $func = "userExists"; switch ($this->u->driverFunctions($func)) { @@ -314,147 +189,8 @@ class User { } } - public function propertiesGet(string $user, bool $withAvatar = false): array { - // prepare default values - $domain = null; - if (strrpos($user, "@")!==false) { - $domain = substr($user, strrpos($user, "@")+1); - } - $init = [ - "id" => $user, - "name" => $user, - "rights" => User\Driver::RIGHTS_NONE, - "domain" => $domain - ]; - $func = "userPropertiesGet"; - switch ($this->u->driverFunctions($func)) { - case User\Driver::FUNC_EXTERNAL: - // we handle authorization checks for external drivers - if (!$this->authorize($user, $func)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); - } - $out = array_merge($init, $this->u->userPropertiesGet($user)); - // remove password if it is return (not exhaustive, but...) - if (array_key_exists('password', $out)) { - unset($out['password']); - } - // if the user does not exist in the internal database, add it - if (!Arsse::$db->userExists($user)) { - $this->autoProvision($user, "", $out); - } - return $out; - case User\Driver::FUNC_INTERNAL: - // internal functions handle their own authorization - return array_merge($init, $this->u->userPropertiesGet($user)); - case User\Driver::FUNCT_NOT_IMPLEMENTED: - // we can return generic values if the function is not implemented - return $init; - } - } - - public function propertiesSet(string $user, array $properties): array { - // remove from the array any values which should be set specially - foreach (['id', 'domain', 'password', 'rights'] as $key) { - if (array_key_exists($key, $properties)) { - unset($properties[$key]); - } - } - $func = "userPropertiesSet"; - switch ($this->u->driverFunctions($func)) { - case User\Driver::FUNC_EXTERNAL: - // we handle authorization checks for external drivers - if (!$this->authorize($user, $func)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); - } - $out = $this->u->userPropertiesSet($user, $properties); - if (Arsse::$db->userExists($user)) { - // if the property change was successful and the user exists, set the internal properties to the same values - Arsse::$db->userPropertiesSet($user, $out); - } else { - // if the user does not exists in the internal database, create it - $this->autoProvision($user, "", $out); - } - return $out; - case User\Driver::FUNC_INTERNAL: - // internal functions handle their own authorization - return $this->u->userPropertiesSet($user, $properties); - case User\Driver::FUNCT_NOT_IMPLEMENTED: - throw new User\ExceptionNotImplemented("notImplemented", ["action" => $func, "user" => $user]); - } - } - - public function rightsGet(string $user): int { - $func = "userRightsGet"; - switch ($this->u->driverFunctions($func)) { - case User\Driver::FUNC_EXTERNAL: - // we handle authorization checks for external drivers - if (!$this->authorize($user, $func)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); - } - $out = $this->u->userRightsGet($user); - // if the user does not exist in the internal database, add it - if (!Arsse::$db->userExists($user)) { - $this->autoProvision($user, "", null, $out); - } - return $out; - case User\Driver::FUNC_INTERNAL: - // internal functions handle their own authorization - return $this->u->userRightsGet($user); - case User\Driver::FUNCT_NOT_IMPLEMENTED: - // assume all users are unprivileged - return User\Driver::RIGHTS_NONE; - } - } - - public function rightsSet(string $user, int $level): bool { - $func = "userRightsSet"; - switch ($this->u->driverFunctions($func)) { - case User\Driver::FUNC_EXTERNAL: - // we handle authorization checks for external drivers - if (!$this->authorize($user, $func)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); - } - $out = $this->u->userRightsSet($user, $level); - // if the user does not exist in the internal database, add it - if ($out && Arsse::$db->userExists($user)) { - $authz = $this->authorizationEnabled(); - $this->authorizationEnabled(false); - Arsse::$db->userRightsSet($user, $level); - $this->authorizationEnabled($authz); - } elseif ($out) { - $this->autoProvision($user, "", null, $level); - } - return $out; - case User\Driver::FUNC_INTERNAL: - // internal functions handle their own authorization - return $this->u->userRightsSet($user, $level); - case User\Driver::FUNCT_NOT_IMPLEMENTED: - throw new User\ExceptionNotImplemented("notImplemented", ["action" => $func, "user" => $user]); - } - } - - protected function autoProvision(string $user, string $password = null, array $properties = null, int $rights = 0): string { - // temporarily disable authorization checks, to avoid potential problems - $this->authorizationEnabled(false); - // create the user + protected function autoProvision(string $user, string $password = null): string { $out = Arsse::$db->userAdd($user, $password); - // set the user rights - Arsse::$db->userRightsSet($user, $rights); - // set the user properties... - if ($properties===null) { - // if nothing is provided but the driver uses an external function, try to get the current values from the external source - try { - if ($this->u->driverFunctions("userPropertiesGet")==User\Driver::FUNC_EXTERNAL) { - Arsse::$db->userPropertiesSet($user, $this->u->userPropertiesGet($user)); - } - } catch (\Throwable $e) { - } - } else { - // otherwise if values are provided, use those - Arsse::$db->userPropertiesSet($user, $properties); - } - // re-enable authorization and return - $this->authorizationEnabled(true); return $out; } } diff --git a/lib/User/Driver.php b/lib/User/Driver.php index 86718ee1..7b07dc4e 100644 --- a/lib/User/Driver.php +++ b/lib/User/Driver.php @@ -11,12 +11,6 @@ interface Driver { const FUNC_INTERNAL = 1; const FUNC_EXTERNAL = 2; - const RIGHTS_NONE = 0; // normal user - const RIGHTS_DOMAIN_MANAGER = 25; // able to act for any normal users on same domain; cannot elevate other users - const RIGHTS_DOMAIN_ADMIN = 50; // able to act for any users on same domain not above themselves; may elevate users on same domain to domain manager or domain admin - const RIGHTS_GLOBAL_MANAGER = 75; // able to act for any normal users on any domain; cannot elevate other users - const RIGHTS_GLOBAL_ADMIN = 100; // is completely unrestricted - // returns an instance of a class implementing this interface. public function __construct(); // returns a human-friendly name for the driver (for display in installer, for example) @@ -32,15 +26,7 @@ interface Driver { // removes a user public function userRemove(string $user): bool; // lists all users - public function userList(string $domain = null): array; + 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; - // gets user metadata (currently not useful) - public function userPropertiesGet(string $user): array; - // sets user metadata (currently not useful) - public function userPropertiesSet(string $user, array $properties): array; - // returns a user's access level according to RIGHTS_* constants (or some custom semantics, if using custom implementation of authorize()) - public function userRightsGet(string $user): int; - // sets a user's access level - public function userRightsSet(string $user, int $level): bool; } diff --git a/lib/User/Internal/Driver.php b/lib/User/Internal/Driver.php index 3f7a555a..ad3e50d9 100644 --- a/lib/User/Internal/Driver.php +++ b/lib/User/Internal/Driver.php @@ -17,10 +17,6 @@ final class Driver implements \JKingWeb\Arsse\User\Driver { "userAdd" => self::FUNC_INTERNAL, "userRemove" => self::FUNC_INTERNAL, "userPasswordSet" => self::FUNC_INTERNAL, - "userPropertiesGet" => self::FUNC_INTERNAL, - "userPropertiesSet" => self::FUNC_INTERNAL, - "userRightsGet" => self::FUNC_INTERNAL, - "userRightsSet" => self::FUNC_INTERNAL, ]; public static function driverName(): string { diff --git a/lib/User/Internal/InternalFunctions.php b/lib/User/Internal/InternalFunctions.php index b88d15ad..e834b3d9 100644 --- a/lib/User/Internal/InternalFunctions.php +++ b/lib/User/Internal/InternalFunctions.php @@ -39,27 +39,11 @@ trait InternalFunctions { return Arsse::$db->userRemove($user); } - public function userList(string $domain = null): array { - return Arsse::$db->userList($domain); + public function userList(): array { + return Arsse::$db->userList(); } public function userPasswordSet(string $user, string $newPassword = null, string $oldPassword = null): string { return Arsse::$db->userPasswordSet($user, $newPassword); } - - public function userPropertiesGet(string $user): array { - return Arsse::$db->userPropertiesGet($user); - } - - public function userPropertiesSet(string $user, array $properties): array { - return Arsse::$db->userPropertiesSet($user, $properties); - } - - public function userRightsGet(string $user): int { - return Arsse::$db->userRightsGet($user); - } - - public function userRightsSet(string $user, int $level): bool { - return Arsse::$db->userRightsSet($user, $level); - } } diff --git a/locale/en.php b/locale/en.php index 477f04a3..66ae80df 100644 --- a/locale/en.php +++ b/locale/en.php @@ -163,10 +163,7 @@ return [ 'Exception.JKingWeb/Arsse/User/Exception.authFailed' => 'Authentication failed', 'Exception.JKingWeb/Arsse/User/ExceptionAuthz.notAuthorized' => '{action, select, - userList {{user, select, - global {Authenticated user is not authorized to view the global user list} - other {Authenticated user is not authorized to view the user list for domain {user}} - }} + userList {Authenticated user is not authorized to view the user list} other {Authenticated user is not authorized to perform the action "{action}" on behalf of {user}} }', 'Exception.JKingWeb/Arsse/User/ExceptionSession.invalid' => 'Session with ID {0} does not exist', diff --git a/tests/cases/REST/NextCloudNews/TestV1_2.php b/tests/cases/REST/NextCloudNews/TestV1_2.php index 2ee0b1b1..4ee6e513 100644 --- a/tests/cases/REST/NextCloudNews/TestV1_2.php +++ b/tests/cases/REST/NextCloudNews/TestV1_2.php @@ -314,7 +314,7 @@ class TestV1_2 extends \JKingWeb\Arsse\Test\AbstractTest { $server['HTTP_CONTENT_TYPE'] = "application/json"; } $req = new ServerRequest($server, [], $url, $method, "php://memory"); - if (Arsse::$user->auth()) { + if (Arsse::$user->auth("john.doe@example.com", "secret")) { $req = $req->withAttribute("authenticated", true)->withAttribute("authenticatedUser", "john.doe@example.com"); } foreach ($headers as $key => $value) { @@ -344,7 +344,6 @@ class TestV1_2 extends \JKingWeb\Arsse\Test\AbstractTest { // create a mock user manager Arsse::$user = Phake::mock(User::class); Phake::when(Arsse::$user)->auth->thenReturn(true); - Phake::when(Arsse::$user)->rightsGet->thenReturn(100); Arsse::$user->id = "john.doe@example.com"; // create a mock database interface Arsse::$db = Phake::mock(Database::class); @@ -696,10 +695,6 @@ class TestV1_2 extends \JKingWeb\Arsse\Test\AbstractTest { Phake::when(Arsse::$db)->feedListStale->thenReturn($this->v(array_column($out, "id"))); $exp = new Response(['feeds' => $out]); $this->assertMessage($exp, $this->req("GET", "/feeds/all")); - // retrieving the list when not an admin fails - Phake::when(Arsse::$user)->rightsGet->thenReturn(0); - $exp = new EmptyResponse(403); - $this->assertMessage($exp, $this->req("GET", "/feeds/all")); } public function testUpdateAFeed() { @@ -721,10 +716,6 @@ class TestV1_2 extends \JKingWeb\Arsse\Test\AbstractTest { $this->assertMessage($exp, $this->req("GET", "/feeds/update", json_encode($in[2]))); $this->assertMessage($exp, $this->req("GET", "/feeds/update", json_encode($in[3]))); $this->assertMessage($exp, $this->req("GET", "/feeds/update", json_encode($in[4]))); - // updating a feed when not an admin fails - Phake::when(Arsse::$user)->rightsGet->thenReturn(0); - $exp = new EmptyResponse(403); - $this->assertMessage($exp, $this->req("GET", "/feeds/update", json_encode($in[0]))); } public function testListArticles() { @@ -929,10 +920,6 @@ class TestV1_2 extends \JKingWeb\Arsse\Test\AbstractTest { $exp = new EmptyResponse(204); $this->assertMessage($exp, $this->req("GET", "/cleanup/before-update")); Phake::verify(Arsse::$db)->feedCleanup(); - // performing a cleanup when not an admin fails - Phake::when(Arsse::$user)->rightsGet->thenReturn(0); - $exp = new EmptyResponse(403); - $this->assertMessage($exp, $this->req("GET", "/cleanup/before-update")); } public function testCleanUpAfterUpdate() { @@ -940,9 +927,5 @@ class TestV1_2 extends \JKingWeb\Arsse\Test\AbstractTest { $exp = new EmptyResponse(204); $this->assertMessage($exp, $this->req("GET", "/cleanup/after-update")); Phake::verify(Arsse::$db)->articleCleanup(); - // performing a cleanup when not an admin fails - Phake::when(Arsse::$user)->rightsGet->thenReturn(0); - $exp = new EmptyResponse(403); - $this->assertMessage($exp, $this->req("GET", "/cleanup/after-update")); } } diff --git a/tests/cases/REST/TinyTinyRSS/TestAPI.php b/tests/cases/REST/TinyTinyRSS/TestAPI.php index 9aea12e0..e6861c1e 100644 --- a/tests/cases/REST/TinyTinyRSS/TestAPI.php +++ b/tests/cases/REST/TinyTinyRSS/TestAPI.php @@ -181,7 +181,6 @@ LONG_STRING; // create a mock user manager Arsse::$user = Phake::mock(User::class); Phake::when(Arsse::$user)->auth->thenReturn(true); - Phake::when(Arsse::$user)->rightsGet->thenReturn(100); Arsse::$user->id = "john.doe@example.com"; // create a mock database interface Arsse::$db = Phake::mock(Database::class); diff --git a/tests/cases/User/TestAuthorization.php b/tests/cases/User/TestAuthorization.php deleted file mode 100644 index 6bbb0fb1..00000000 --- a/tests/cases/User/TestAuthorization.php +++ /dev/null @@ -1,338 +0,0 @@ - Driver::RIGHTS_NONE, - 'user@example.org' => Driver::RIGHTS_NONE, - 'dman@example.com' => Driver::RIGHTS_DOMAIN_MANAGER, - 'dman@example.org' => Driver::RIGHTS_DOMAIN_MANAGER, - 'dadm@example.com' => Driver::RIGHTS_DOMAIN_ADMIN, - 'dadm@example.org' => Driver::RIGHTS_DOMAIN_ADMIN, - 'gman@example.com' => Driver::RIGHTS_GLOBAL_MANAGER, - 'gman@example.org' => Driver::RIGHTS_GLOBAL_MANAGER, - 'gadm@example.com' => Driver::RIGHTS_GLOBAL_ADMIN, - 'gadm@example.org' => Driver::RIGHTS_GLOBAL_ADMIN, - // invalid rights levels - 'bad1@example.com' => Driver::RIGHTS_NONE+1, - 'bad1@example.org' => Driver::RIGHTS_NONE+1, - 'bad2@example.com' => Driver::RIGHTS_DOMAIN_MANAGER+1, - 'bad2@example.org' => Driver::RIGHTS_DOMAIN_MANAGER+1, - 'bad3@example.com' => Driver::RIGHTS_DOMAIN_ADMIN+1, - 'bad3@example.org' => Driver::RIGHTS_DOMAIN_ADMIN+1, - 'bad4@example.com' => Driver::RIGHTS_GLOBAL_MANAGER+1, - 'bad4@example.org' => Driver::RIGHTS_GLOBAL_MANAGER+1, - 'bad5@example.com' => Driver::RIGHTS_GLOBAL_ADMIN+1, - 'bad5@example.org' => Driver::RIGHTS_GLOBAL_ADMIN+1, - - ]; - const LEVELS = [ - Driver::RIGHTS_NONE, - Driver::RIGHTS_DOMAIN_MANAGER, - Driver::RIGHTS_DOMAIN_ADMIN, - Driver::RIGHTS_GLOBAL_MANAGER, - Driver::RIGHTS_GLOBAL_ADMIN, - ]; - const DOMAINS = [ - '@example.com', - '@example.org', - "", - ]; - - protected $data; - - public function setUp(string $drv = \JkingWeb\Arsse\Test\User\DriverInternalMock::class, string $db = null) { - $this->clearData(); - $conf = new Conf(); - $conf->userDriver = $drv; - $conf->userPreAuth = false; - Arsse::$conf = $conf; - if ($db !== null) { - Arsse::$db = new $db(); - } - Arsse::$user = Phake::partialMock(User::class); - Phake::when(Arsse::$user)->authorize->thenReturn(true); - foreach (self::USERS as $user => $level) { - Arsse::$user->add($user, ""); - Arsse::$user->rightsSet($user, $level); - } - Phake::reset(Arsse::$user); - } - - public function tearDown() { - $this->clearData(); - } - - public function testToggleLogic() { - $this->assertTrue(Arsse::$user->authorizationEnabled()); - $this->assertTrue(Arsse::$user->authorizationEnabled(true)); - $this->assertFalse(Arsse::$user->authorizationEnabled(false)); - $this->assertFalse(Arsse::$user->authorizationEnabled(false)); - $this->assertFalse(Arsse::$user->authorizationEnabled(true)); - $this->assertTrue(Arsse::$user->authorizationEnabled(true)); - } - - public function testSelfActionLogic() { - foreach (array_keys(self::USERS) as $user) { - Arsse::$user->auth($user, ""); - // users should be able to do basic actions for themselves - $this->assertTrue(Arsse::$user->authorize($user, "userExists"), "User $user could not act for themselves."); - $this->assertTrue(Arsse::$user->authorize($user, "userRemove"), "User $user could not act for themselves."); - } - } - - public function testRegularUserLogic() { - foreach (self::USERS as $actor => $rights) { - if ($rights != Driver::RIGHTS_NONE) { - continue; - } - Arsse::$user->auth($actor, ""); - foreach (array_keys(self::USERS) as $affected) { - // regular users should only be able to act for themselves - if ($actor==$affected) { - $this->assertTrue(Arsse::$user->authorize($affected, "userExists"), "User $actor acted properly for $affected, but the action was denied."); - $this->assertTrue(Arsse::$user->authorize($affected, "userRemove"), "User $actor acted properly for $affected, but the action was denied."); - } else { - $this->assertFalse(Arsse::$user->authorize($affected, "userExists"), "User $actor acted improperly for $affected, but the action was allowed."); - $this->assertFalse(Arsse::$user->authorize($affected, "userRemove"), "User $actor acted improperly for $affected, but the action was allowed."); - } - // they should never be able to set rights - foreach (self::LEVELS as $level) { - $this->assertFalse(Arsse::$user->authorize($affected, "userRightsSet", $level), "User $actor acted improperly for $affected settings rights level $level, but the action was allowed."); - } - } - // they should not be able to list users - foreach (self::DOMAINS as $domain) { - $this->assertFalse(Arsse::$user->authorize($domain, "userList"), "User $actor improperly checked user list for domain '$domain', but the action was allowed."); - } - } - } - - public function testDomainManagerLogic() { - foreach (self::USERS as $actor => $actorRights) { - if ($actorRights != Driver::RIGHTS_DOMAIN_MANAGER) { - continue; - } - $actorDomain = substr($actor, strrpos($actor, "@")+1); - Arsse::$user->auth($actor, ""); - foreach (self::USERS as $affected => $affectedRights) { - $affectedDomain = substr($affected, strrpos($affected, "@")+1); - // domain managers should be able to check any user on the same domain - if ($actorDomain==$affectedDomain) { - $this->assertTrue(Arsse::$user->authorize($affected, "userExists"), "User $actor acted properly for $affected, but the action was denied."); - } else { - $this->assertFalse(Arsse::$user->authorize($affected, "userExists"), "User $actor acted improperly for $affected, but the action was allowed."); - } - // they should only be able to act for regular users on the same domain - if ($actor==$affected || ($actorDomain==$affectedDomain && $affectedRights==User\Driver::RIGHTS_NONE)) { - $this->assertTrue(Arsse::$user->authorize($affected, "userRemove"), "User $actor acted properly for $affected, but the action was denied."); - } else { - $this->assertFalse(Arsse::$user->authorize($affected, "userRemove"), "User $actor acted improperly for $affected, but the action was allowed."); - } - // and they should only be able to set their own rights to regular user - foreach (self::LEVELS as $level) { - if ($actor==$affected && in_array($level, [User\Driver::RIGHTS_NONE, Driver::RIGHTS_DOMAIN_MANAGER])) { - $this->assertTrue(Arsse::$user->authorize($affected, "userRightsSet", $level), "User $actor acted properly for $affected settings rights level $level, but the action was denied."); - } else { - $this->assertFalse(Arsse::$user->authorize($affected, "userRightsSet", $level), "User $actor acted improperly for $affected settings rights level $level, but the action was allowed."); - } - } - } - // they should also be able to list all users on their own domain - foreach (self::DOMAINS as $domain) { - if ($domain=="@".$actorDomain) { - $this->assertTrue(Arsse::$user->authorize($domain, "userList"), "User $actor properly checked user list for domain '$domain', but the action was denied."); - } else { - $this->assertFalse(Arsse::$user->authorize($domain, "userList"), "User $actor improperly checked user list for domain '$domain', but the action was allowed."); - } - } - } - } - - public function testDomainAdministratorLogic() { - foreach (self::USERS as $actor => $actorRights) { - if ($actorRights != Driver::RIGHTS_DOMAIN_ADMIN) { - continue; - } - $actorDomain = substr($actor, strrpos($actor, "@")+1); - Arsse::$user->auth($actor, ""); - $allowed = [User\Driver::RIGHTS_NONE,User\Driver::RIGHTS_DOMAIN_MANAGER,User\Driver::RIGHTS_DOMAIN_ADMIN]; - foreach (self::USERS as $affected => $affectedRights) { - $affectedDomain = substr($affected, strrpos($affected, "@")+1); - // domain admins should be able to check any user on the same domain - if ($actorDomain==$affectedDomain) { - $this->assertTrue(Arsse::$user->authorize($affected, "userExists"), "User $actor acted properly for $affected, but the action was denied."); - } else { - $this->assertFalse(Arsse::$user->authorize($affected, "userExists"), "User $actor acted improperly for $affected, but the action was allowed."); - } - // they should be able to act for any user on the same domain who is not a global manager or admin - if ($actorDomain==$affectedDomain && in_array($affectedRights, $allowed)) { - $this->assertTrue(Arsse::$user->authorize($affected, "userRemove"), "User $actor acted properly for $affected, but the action was denied."); - } else { - $this->assertFalse(Arsse::$user->authorize($affected, "userRemove"), "User $actor acted improperly for $affected, but the action was allowed."); - } - // they should be able to set rights for any user on their domain who is not a global manager or admin, up to domain admin level - foreach (self::LEVELS as $level) { - if ($actorDomain==$affectedDomain && in_array($affectedRights, $allowed) && in_array($level, $allowed)) { - $this->assertTrue(Arsse::$user->authorize($affected, "userRightsSet", $level), "User $actor acted properly for $affected settings rights level $level, but the action was denied."); - } else { - $this->assertFalse(Arsse::$user->authorize($affected, "userRightsSet", $level), "User $actor acted improperly for $affected settings rights level $level, but the action was allowed."); - } - } - } - // they should also be able to list all users on their own domain - foreach (self::DOMAINS as $domain) { - if ($domain=="@".$actorDomain) { - $this->assertTrue(Arsse::$user->authorize($domain, "userList"), "User $actor properly checked user list for domain '$domain', but the action was denied."); - } else { - $this->assertFalse(Arsse::$user->authorize($domain, "userList"), "User $actor improperly checked user list for domain '$domain', but the action was allowed."); - } - } - } - } - - public function testGlobalManagerLogic() { - foreach (self::USERS as $actor => $actorRights) { - if ($actorRights != Driver::RIGHTS_GLOBAL_MANAGER) { - continue; - } - $actorDomain = substr($actor, strrpos($actor, "@")+1); - Arsse::$user->auth($actor, ""); - foreach (self::USERS as $affected => $affectedRights) { - $affectedDomain = substr($affected, strrpos($affected, "@")+1); - // global managers should be able to check any user - $this->assertTrue(Arsse::$user->authorize($affected, "userExists"), "User $actor acted properly for $affected, but the action was denied."); - // they should only be able to act for regular users - if ($actor==$affected || $affectedRights==User\Driver::RIGHTS_NONE) { - $this->assertTrue(Arsse::$user->authorize($affected, "userRemove"), "User $actor acted properly for $affected, but the action was denied."); - } else { - $this->assertFalse(Arsse::$user->authorize($affected, "userRemove"), "User $actor acted improperly for $affected, but the action was allowed."); - } - // and they should only be able to set their own rights to regular user - foreach (self::LEVELS as $level) { - if ($actor==$affected && in_array($level, [User\Driver::RIGHTS_NONE, Driver::RIGHTS_GLOBAL_MANAGER])) { - $this->assertTrue(Arsse::$user->authorize($affected, "userRightsSet", $level), "User $actor acted properly for $affected settings rights level $level, but the action was denied."); - } else { - $this->assertFalse(Arsse::$user->authorize($affected, "userRightsSet", $level), "User $actor acted improperly for $affected settings rights level $level, but the action was allowed."); - } - } - } - // they should also be able to list all users - foreach (self::DOMAINS as $domain) { - $this->assertTrue(Arsse::$user->authorize($domain, "userList"), "User $actor properly checked user list for domain '$domain', but the action was denied."); - } - } - } - - public function testGlobalAdministratorLogic() { - foreach (self::USERS as $actor => $actorRights) { - if ($actorRights != Driver::RIGHTS_GLOBAL_ADMIN) { - continue; - } - Arsse::$user->auth($actor, ""); - // global admins can do anything - foreach (self::USERS as $affected => $affectedRights) { - $this->assertTrue(Arsse::$user->authorize($affected, "userExists"), "User $actor acted properly for $affected, but the action was denied."); - $this->assertTrue(Arsse::$user->authorize($affected, "userRemove"), "User $actor acted properly for $affected, but the action was denied."); - foreach (self::LEVELS as $level) { - $this->assertTrue(Arsse::$user->authorize($affected, "userRightsSet", $level), "User $actor acted properly for $affected settings rights level $level, but the action was denied."); - } - } - foreach (self::DOMAINS as $domain) { - $this->assertTrue(Arsse::$user->authorize($domain, "userList"), "User $actor properly checked user list for domain '$domain', but the action was denied."); - } - } - } - - public function testInvalidLevelLogic() { - foreach (self::USERS as $actor => $rights) { - if (in_array($rights, self::LEVELS)) { - continue; - } - Arsse::$user->auth($actor, ""); - foreach (array_keys(self::USERS) as $affected) { - // users with unknown/invalid rights should be treated just like regular users and only be able to act for themselves - if ($actor==$affected) { - $this->assertTrue(Arsse::$user->authorize($affected, "userExists"), "User $actor acted properly for $affected, but the action was denied."); - $this->assertTrue(Arsse::$user->authorize($affected, "userRemove"), "User $actor acted properly for $affected, but the action was denied."); - } else { - $this->assertFalse(Arsse::$user->authorize($affected, "userExists"), "User $actor acted improperly for $affected, but the action was allowed."); - $this->assertFalse(Arsse::$user->authorize($affected, "userRemove"), "User $actor acted improperly for $affected, but the action was allowed."); - } - // they should never be able to set rights - foreach (self::LEVELS as $level) { - $this->assertFalse(Arsse::$user->authorize($affected, "userRightsSet", $level), "User $actor acted improperly for $affected settings rights level $level, but the action was allowed."); - } - } - // they should not be able to list users - foreach (self::DOMAINS as $domain) { - $this->assertFalse(Arsse::$user->authorize($domain, "userList"), "User $actor improperly checked user list for domain '$domain', but the action was allowed."); - } - } - } - - public function testInternalExceptionLogic() { - $tests = [ - // methods of User class to test, with parameters besides affected user - 'exists' => [], - 'remove' => [], - 'add' => [''], - 'passwordSet' => [''], - 'propertiesGet' => [], - 'propertiesSet' => [[]], - 'rightsGet' => [], - 'rightsSet' => [User\Driver::RIGHTS_GLOBAL_ADMIN], - 'list' => [], - ]; - // try first with a global admin (there should be no exception) - Arsse::$user->auth("gadm@example.com", ""); - $this->assertCount(0, $this->checkExceptions("user@example.org", $tests)); - // next try with a regular user acting on another user (everything should fail) - Arsse::$user->auth("user@example.com", ""); - $this->assertCount(sizeof($tests), $this->checkExceptions("user@example.org", $tests)); - } - - public function testExternalExceptionLogic() { - // set up the test for an external driver - $this->setUp(\JKingWeb\Arsse\Test\User\DriverExternalMock::class, \JKingWeb\Arsse\Test\User\Database::class); - // run the previous test with the external driver set up - $this->testInternalExceptionLogic(); - } - - // meat of testInternalExceptionLogic and testExternalExceptionLogic - // calls each requested function with supplied arguments, catches authorization exceptions, and returns an array of caught failed calls - protected function checkExceptions(string $user, $tests): array { - $err = []; - foreach ($tests as $func => $args) { - // list method does not take an affected user, so do not unshift for that one - if ($func != "list") { - array_unshift($args, $user); - } - try { - call_user_func_array(array(Arsse::$user, $func), $args); - } catch (\JKingWeb\Arsse\User\ExceptionAuthz $e) { - $err[] = $func; - } - } - return $err; - } - - public function testMissingUserLogic() { - Arsse::$user->auth("gadm@example.com", ""); - $this->assertTrue(Arsse::$user->authorize("user@example.com", "someFunction")); - $this->assertException("doesNotExist", "User"); - Arsse::$user->authorize("this_user_does_not_exist@example.org", "someFunction"); - } -} diff --git a/tests/cases/User/TestMockExternal.php b/tests/cases/User/TestMockExternal.php deleted file mode 100644 index 928edc7c..00000000 --- a/tests/cases/User/TestMockExternal.php +++ /dev/null @@ -1,17 +0,0 @@ - 'int', ], 'rows' => [ - ["admin@example.net", '$2y$10$PbcG2ZR3Z8TuPzM7aHTF8.v61dtCjzjK78gdZJcp4UePE8T9jEgBW', "Hard Lip Herbert", UserDriver::RIGHTS_GLOBAL_ADMIN], // password is hash of "secret" - ["jane.doe@example.com", "", "Jane Doe", UserDriver::RIGHTS_NONE], - ["john.doe@example.com", "", "John Doe", UserDriver::RIGHTS_NONE], + ["admin@example.net", '$2y$10$PbcG2ZR3Z8TuPzM7aHTF8.v61dtCjzjK78gdZJcp4UePE8T9jEgBW', "Hard Lip Herbert", 100], // password is hash of "secret" + ["jane.doe@example.com", "", "Jane Doe", 0], + ["john.doe@example.com", "", "John Doe", 0], ], ], ]; @@ -63,7 +63,7 @@ trait SeriesUser { $this->assertSame("", 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, UserDriver::RIGHTS_NONE]; + $state['arsse_users']['rows'][] = ["john.doe@example.org", null, 0]; $this->compareExpectations($state); } @@ -125,24 +125,12 @@ trait SeriesUser { Phake::verify(Arsse::$user)->authorize("", "userList"); } - public function testListUsersOnADomain() { - $users = ["jane.doe@example.com", "john.doe@example.com"]; - $this->assertSame($users, Arsse::$db->userList("example.com")); - Phake::verify(Arsse::$user)->authorize("@example.com", "userList"); - } - public function testListAllUsersWithoutAuthority() { Phake::when(Arsse::$user)->authorize->thenReturn(false); $this->assertException("notAuthorized", "User", "ExceptionAuthz"); Arsse::$db->userList(); } - public function testListUsersOnADomainWithoutAuthority() { - Phake::when(Arsse::$user)->authorize->thenReturn(false); - $this->assertException("notAuthorized", "User", "ExceptionAuthz"); - Arsse::$db->userList("example.com"); - } - /** * @depends testGetAPassword */ @@ -172,105 +160,4 @@ trait SeriesUser { $this->assertException("notAuthorized", "User", "ExceptionAuthz"); Arsse::$db->userPasswordSet("john.doe@example.com", "secret"); } - - public function testGetUserProperties() { - $exp = [ - 'name' => 'Hard Lip Herbert', - 'rights' => UserDriver::RIGHTS_GLOBAL_ADMIN, - ]; - $props = Arsse::$db->userPropertiesGet("admin@example.net"); - Phake::verify(Arsse::$user)->authorize("admin@example.net", "userPropertiesGet"); - $this->assertArraySubset($exp, $props); - $this->assertArrayNotHasKey("password", $props); - } - - public function testGetThePropertiesOfAMissingUser() { - $this->assertException("doesNotExist", "User"); - Arsse::$db->userPropertiesGet("john.doe@example.org"); - } - - public function testGetUserPropertiesWithoutAuthority() { - Phake::when(Arsse::$user)->authorize->thenReturn(false); - $this->assertException("notAuthorized", "User", "ExceptionAuthz"); - Arsse::$db->userPropertiesGet("john.doe@example.com"); - } - - public function testSetUserProperties() { - $try = [ - 'name' => 'James Kirk', // only this should actually change - 'password' => '000destruct0', - 'rights' => UserDriver::RIGHTS_NONE, - 'lifeform' => 'tribble', - ]; - $exp = [ - 'name' => 'James Kirk', - 'rights' => UserDriver::RIGHTS_GLOBAL_ADMIN, - ]; - $props = Arsse::$db->userPropertiesSet("admin@example.net", $try); - Phake::verify(Arsse::$user)->authorize("admin@example.net", "userPropertiesSet"); - $this->assertArraySubset($exp, $props); - $this->assertArrayNotHasKey("password", $props); - $state = $this->primeExpectations($this->data, ['arsse_users' => ['id','password','name','rights']]); - $state['arsse_users']['rows'][0][2] = "James Kirk"; - $this->compareExpectations($state); - // making now changes should make no changes :) - Arsse::$db->userPropertiesSet("admin@example.net", ['lifeform' => "tribble"]); - $this->compareExpectations($state); - } - - public function testSetThePropertiesOfAMissingUser() { - $try = ['name' => 'John Doe']; - $this->assertException("doesNotExist", "User"); - Arsse::$db->userPropertiesSet("john.doe@example.org", $try); - } - - public function testSetUserPropertiesWithoutAuthority() { - $try = ['name' => 'John Doe']; - Phake::when(Arsse::$user)->authorize->thenReturn(false); - $this->assertException("notAuthorized", "User", "ExceptionAuthz"); - Arsse::$db->userPropertiesSet("john.doe@example.com", $try); - } - - public function testGetUserRights() { - $user1 = "john.doe@example.com"; - $user2 = "admin@example.net"; - $this->assertSame(UserDriver::RIGHTS_NONE, Arsse::$db->userRightsGet($user1)); - $this->assertSame(UserDriver::RIGHTS_GLOBAL_ADMIN, Arsse::$db->userRightsGet($user2)); - Phake::verify(Arsse::$user)->authorize($user1, "userRightsGet"); - Phake::verify(Arsse::$user)->authorize($user2, "userRightsGet"); - } - - public function testGetTheRightsOfAMissingUser() { - $this->assertSame(UserDriver::RIGHTS_NONE, Arsse::$db->userRightsGet("john.doe@example.org")); - Phake::verify(Arsse::$user)->authorize("john.doe@example.org", "userRightsGet"); - } - - public function testGetUserRightsWithoutAuthority() { - Phake::when(Arsse::$user)->authorize->thenReturn(false); - $this->assertException("notAuthorized", "User", "ExceptionAuthz"); - Arsse::$db->userRightsGet("john.doe@example.com"); - } - - public function testSetUserRights() { - $user = "john.doe@example.com"; - $rights = UserDriver::RIGHTS_GLOBAL_ADMIN; - $this->assertTrue(Arsse::$db->userRightsSet($user, $rights)); - Phake::verify(Arsse::$user)->authorize($user, "userRightsSet", $rights); - $state = $this->primeExpectations($this->data, ['arsse_users' => ['id','rights']]); - $state['arsse_users']['rows'][2][1] = $rights; - $this->compareExpectations($state); - } - - public function testSetTheRightsOfAMissingUser() { - $rights = UserDriver::RIGHTS_GLOBAL_ADMIN; - $this->assertException("doesNotExist", "User"); - Arsse::$db->userRightsSet("john.doe@example.org", $rights); - } - - public function testSetUserRightsWithoutAuthority() { - $rights = UserDriver::RIGHTS_GLOBAL_ADMIN; - Phake::when(Arsse::$user)->authorize->thenReturn(false); - $this->assertException("notAuthorized", "User", "ExceptionAuthz"); - Arsse::$db->userRightsSet("john.doe@example.com", $rights); - } } diff --git a/tests/lib/User/CommonTests.php b/tests/lib/User/CommonTests.php deleted file mode 100644 index 81d5d48b..00000000 --- a/tests/lib/User/CommonTests.php +++ /dev/null @@ -1,154 +0,0 @@ -clearData(); - $conf = new Conf(); - $conf->userDriver = $this->drv; - $conf->userPreAuth = false; - Arsse::$conf = $conf; - Arsse::$db = new Database(); - Arsse::$user = Phake::partialMock(User::class); - Phake::when(Arsse::$user)->authorize->thenReturn(true); - $_SERVER['PHP_AUTH_USER'] = self::USER1; - $_SERVER['PHP_AUTH_PW'] = "secret"; - // call the additional setup method if it exists - if (method_exists($this, "setUpSeries")) { - $this->setUpSeries(); - } - } - - public function tearDown() { - $this->clearData(); - // call the additional teardiwn method if it exists - if (method_exists($this, "tearDownSeries")) { - $this->tearDownSeries(); - } - } - - public function testListUsers() { - $this->assertCount(0, Arsse::$user->list()); - } - - public function testCheckIfAUserDoesNotExist() { - $this->assertFalse(Arsse::$user->exists(self::USER1)); - } - - public function testAddAUser() { - Arsse::$user->add(self::USER1, ""); - $this->assertCount(1, Arsse::$user->list()); - } - - public function testCheckIfAUserDoesExist() { - Arsse::$user->add(self::USER1, ""); - $this->assertTrue(Arsse::$user->exists(self::USER1)); - } - - public function testAddADuplicateUser() { - Arsse::$user->add(self::USER1, ""); - $this->assertException("alreadyExists", "User"); - Arsse::$user->add(self::USER1, ""); - } - - public function testAddMultipleUsers() { - Arsse::$user->add(self::USER1, ""); - Arsse::$user->add(self::USER2, ""); - $this->assertCount(2, Arsse::$user->list()); - } - - public function testRemoveAUser() { - Arsse::$user->add(self::USER1, ""); - $this->assertCount(1, Arsse::$user->list()); - Arsse::$user->remove(self::USER1); - $this->assertCount(0, Arsse::$user->list()); - } - - public function testRemoveAMissingUser() { - $this->assertException("doesNotExist", "User"); - Arsse::$user->remove(self::USER1); - } - - /** @group slow */ - public function testAuthenticateAUser() { - $_SERVER['PHP_AUTH_USER'] = self::USER1; - $_SERVER['PHP_AUTH_PW'] = "secret"; - Arsse::$user->add(self::USER1, "secret"); - Arsse::$user->add(self::USER2, ""); - $this->assertTrue(Arsse::$user->auth()); - $this->assertTrue(Arsse::$user->auth(self::USER1, "secret")); - $this->assertFalse(Arsse::$user->auth(self::USER1, "superman")); - $this->assertTrue(Arsse::$user->auth(self::USER2, "")); - } - - /** @group slow */ - public function testChangeAPassword() { - Arsse::$user->add(self::USER1, "secret"); - $this->assertEquals("superman", Arsse::$user->passwordSet(self::USER1, "superman")); - $this->assertTrue(Arsse::$user->auth(self::USER1, "superman")); - $this->assertFalse(Arsse::$user->auth(self::USER1, "secret")); - $this->assertEquals("", Arsse::$user->passwordSet(self::USER1, "")); - $this->assertTrue(Arsse::$user->auth(self::USER1, "")); - $this->assertEquals(Arsse::$conf->userTempPasswordLength, strlen(Arsse::$user->passwordSet(self::USER1))); - } - - public function testChangeAPasswordForAMissingUser() { - $this->assertException("doesNotExist", "User"); - Arsse::$user->passwordSet(self::USER1, "superman"); - } - - public function testGetThePropertiesOfAUser() { - Arsse::$user->add(self::USER1, "secret"); - $p = Arsse::$user->propertiesGet(self::USER1); - $this->assertArrayHasKey('id', $p); - $this->assertArrayHasKey('name', $p); - $this->assertArrayHasKey('domain', $p); - $this->assertArrayHasKey('rights', $p); - $this->assertArrayNotHasKey('password', $p); - $this->assertEquals(self::USER1, $p['name']); - } - - public function testSetThePropertiesOfAUser() { - $pSet = [ - 'name' => 'John Doe', - 'id' => 'invalid', - 'domain' => 'localhost', - 'rights' => Driver::RIGHTS_GLOBAL_ADMIN, - 'password' => 'superman', - ]; - $pGet = [ - 'name' => 'John Doe', - 'id' => self::USER1, - 'domain' => 'example.com', - 'rights' => Driver::RIGHTS_NONE, - ]; - Arsse::$user->add(self::USER1, "secret"); - Arsse::$user->propertiesSet(self::USER1, $pSet); - $p = Arsse::$user->propertiesGet(self::USER1); - $this->assertArraySubset($pGet, $p); - $this->assertArrayNotHasKey('password', $p); - $this->assertFalse(Arsse::$user->auth(self::USER1, "superman")); - } - - public function testGetTheRightsOfAUser() { - Arsse::$user->add(self::USER1, ""); - $this->assertEquals(Driver::RIGHTS_NONE, Arsse::$user->rightsGet(self::USER1)); - } - - public function testSetTheRightsOfAUser() { - Arsse::$user->add(self::USER1, ""); - Arsse::$user->rightsSet(self::USER1, Driver::RIGHTS_GLOBAL_ADMIN); - $this->assertEquals(Driver::RIGHTS_GLOBAL_ADMIN, Arsse::$user->rightsGet(self::USER1)); - } -} diff --git a/tests/lib/User/Database.php b/tests/lib/User/Database.php deleted file mode 100644 index 9255d89e..00000000 --- a/tests/lib/User/Database.php +++ /dev/null @@ -1,133 +0,0 @@ -authorize($user, __FUNCTION__)) { - throw new ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); - } - return parent::userExists($user); - } - - public function userAdd(string $user, string $password = null): string { - if (!Arsse::$user->authorize($user, __FUNCTION__)) { - throw new ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); - } - if ($this->userExists($user)) { - throw new Exception("alreadyExists", ["action" => __FUNCTION__, "user" => $user]); - } - if ($password===null) { - $password = (new PassGen)->length(Arsse::$conf->userTempPasswordLength)->get(); - } - return parent::userAdd($user, $password); - } - - public function userRemove(string $user): bool { - if (!Arsse::$user->authorize($user, __FUNCTION__)) { - throw new ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); - } - if (!$this->userExists($user)) { - throw new Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); - } - return parent::userRemove($user); - } - - public function userList(string $domain = null): array { - if ($domain===null) { - if (!Arsse::$user->authorize("", __FUNCTION__)) { - throw new ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => "global"]); - } - return parent::userList(); - } else { - $suffix = '@'.$domain; - if (!Arsse::$user->authorize($suffix, __FUNCTION__)) { - throw new ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $domain]); - } - return parent::userList($domain); - } - } - - public function userPasswordSet(string $user, string $newPassword = null, string $oldPassword = null): string { - if (!Arsse::$user->authorize($user, __FUNCTION__)) { - throw new ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); - } - if (!$this->userExists($user)) { - throw new Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); - } - if ($newPassword===null) { - $newPassword = (new PassGen)->length(Arsse::$conf->userTempPasswordLength)->get(); - } - return parent::userPasswordSet($user, $newPassword); - } - - public function userPropertiesGet(string $user): array { - if (!Arsse::$user->authorize($user, __FUNCTION__)) { - throw new ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); - } - if (!$this->userExists($user)) { - throw new Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); - } - $out = parent::userPropertiesGet($user); - unset($out['password']); - return $out; - } - - public function userPropertiesSet(string $user, array $properties): array { - if (!Arsse::$user->authorize($user, __FUNCTION__)) { - throw new ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); - } - if (!$this->userExists($user)) { - throw new Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); - } - parent::userPropertiesSet($user, $properties); - return $this->userPropertiesGet($user); - } - - public function userRightsGet(string $user): int { - if (!Arsse::$user->authorize($user, __FUNCTION__)) { - throw new ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); - } - if (!$this->userExists($user)) { - throw new Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); - } - return parent::userRightsGet($user); - } - - public function userRightsSet(string $user, int $level): bool { - if (!Arsse::$user->authorize($user, __FUNCTION__)) { - throw new ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); - } - if (!$this->userExists($user)) { - throw new Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); - } - return parent::userRightsSet($user, $level); - } - - // specific to mock database - - public function userPasswordGet(string $user): string { - if (!Arsse::$user->authorize($user, __FUNCTION__)) { - throw new ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); - } - if (!$this->userExists($user)) { - throw new Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); - } - return $this->db[$user]['password']; - } -} diff --git a/tests/lib/User/DriverExternalMock.php b/tests/lib/User/DriverExternalMock.php deleted file mode 100644 index f91d20fd..00000000 --- a/tests/lib/User/DriverExternalMock.php +++ /dev/null @@ -1,127 +0,0 @@ - Driver::FUNC_EXTERNAL, - "userList" => Driver::FUNC_EXTERNAL, - "userExists" => Driver::FUNC_EXTERNAL, - "userAdd" => Driver::FUNC_EXTERNAL, - "userRemove" => Driver::FUNC_EXTERNAL, - "userPasswordSet" => Driver::FUNC_EXTERNAL, - "userPropertiesGet" => Driver::FUNC_EXTERNAL, - "userPropertiesSet" => Driver::FUNC_EXTERNAL, - "userRightsGet" => Driver::FUNC_EXTERNAL, - "userRightsSet" => Driver::FUNC_EXTERNAL, - ]; - - public static function driverName(): string { - return "Mock External Driver"; - } - - public function driverFunctions(string $function = null) { - if ($function===null) { - return $this->functions; - } - if (array_key_exists($function, $this->functions)) { - return $this->functions[$function]; - } else { - return Driver::FUNC_NOT_IMPLEMENTED; - } - } - - public function __construct() { - } - - public function auth(string $user, string $password): bool { - if (!$this->userExists($user)) { - return false; - } - if ($password==="" && $this->db[$user]['password']==="") { - return true; - } - if (password_verify($password, $this->db[$user]['password'])) { - return true; - } - return false; - } - - public function userExists(string $user): bool { - return parent::userExists($user); - } - - public function userAdd(string $user, string $password = null): string { - if ($this->userExists($user)) { - throw new Exception("alreadyExists", ["action" => __FUNCTION__, "user" => $user]); - } - if ($password===null) { - $password = (new PassGen)->length(Arsse::$conf->userTempPasswordLength)->get(); - } - return parent::userAdd($user, $password); - } - - public function userRemove(string $user): bool { - if (!$this->userExists($user)) { - throw new Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); - } - return parent::userRemove($user); - } - - public function userList(string $domain = null): array { - if ($domain===null) { - return parent::userList(); - } else { - return parent::userList($domain); - } - } - - public function userPasswordSet(string $user, string $newPassword = null, string $oldPassword = null): string { - if (!$this->userExists($user)) { - throw new Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); - } - if ($newPassword===null) { - $newPassword = (new PassGen)->length(Arsse::$conf->userTempPasswordLength)->get(); - } - return parent::userPasswordSet($user, $newPassword); - } - - public function userPropertiesGet(string $user): array { - if (!$this->userExists($user)) { - throw new Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); - } - return parent::userPropertiesGet($user); - } - - public function userPropertiesSet(string $user, array $properties): array { - if (!$this->userExists($user)) { - throw new Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); - } - parent::userPropertiesSet($user, $properties); - return $this->userPropertiesGet($user); - } - - public function userRightsGet(string $user): int { - if (!$this->userExists($user)) { - throw new Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); - } - return parent::userRightsGet($user); - } - - public function userRightsSet(string $user, int $level): bool { - if (!$this->userExists($user)) { - throw new Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]); - } - return parent::userRightsSet($user, $level); - } -} diff --git a/tests/lib/User/DriverInternalMock.php b/tests/lib/User/DriverInternalMock.php deleted file mode 100644 index c8e64ddb..00000000 --- a/tests/lib/User/DriverInternalMock.php +++ /dev/null @@ -1,56 +0,0 @@ - Driver::FUNC_INTERNAL, - "userList" => Driver::FUNC_INTERNAL, - "userExists" => Driver::FUNC_INTERNAL, - "userAdd" => Driver::FUNC_INTERNAL, - "userRemove" => Driver::FUNC_INTERNAL, - "userPasswordSet" => Driver::FUNC_INTERNAL, - "userPropertiesGet" => Driver::FUNC_INTERNAL, - "userPropertiesSet" => Driver::FUNC_INTERNAL, - "userRightsGet" => Driver::FUNC_INTERNAL, - "userRightsSet" => Driver::FUNC_INTERNAL, - ]; - - public static function driverName(): string { - return "Mock Internal Driver"; - } - - public function driverFunctions(string $function = null) { - if ($function===null) { - return $this->functions; - } - if (array_key_exists($function, $this->functions)) { - return $this->functions[$function]; - } else { - return Driver::FUNC_NOT_IMPLEMENTED; - } - } - - public function __construct() { - } - - public function auth(string $user, string $password): bool { - if (!$this->userExists($user)) { - return false; - } - if ($password==="" && $this->db[$user]['password']==="") { - return true; - } - if (password_verify($password, $this->db[$user]['password'])) { - return true; - } - return false; - } -} diff --git a/tests/lib/User/DriverSkeleton.php b/tests/lib/User/DriverSkeleton.php deleted file mode 100644 index e9c234bd..00000000 --- a/tests/lib/User/DriverSkeleton.php +++ /dev/null @@ -1,72 +0,0 @@ -db); - } - - public function userAdd(string $user, string $password = null): string { - $u = [ - 'password' => $password ? password_hash($password, \PASSWORD_DEFAULT) : "", - 'rights' => Driver::RIGHTS_NONE, - ]; - $this->db[$user] = $u; - return $password; - } - - public function userRemove(string $user): bool { - unset($this->db[$user]); - return true; - } - - public function userList(string $domain = null): array { - $list = array_keys($this->db); - if ($domain===null) { - return $list; - } else { - $suffix = '@'.$domain; - $len = -1 * strlen($suffix); - return array_filter($list, function ($user) use ($suffix, $len) { - return substr_compare($user, $suffix, $len); - }); - } - } - - public function userPasswordSet(string $user, string $newPassword = null, string $oldPassword = null): string { - $this->db[$user]['password'] = password_hash($newPassword, \PASSWORD_DEFAULT); - return $newPassword; - } - - public function userPropertiesGet(string $user): array { - $out = $this->db[$user]; - return $out; - } - - public function userPropertiesSet(string $user, array $properties): array { - $this->db[$user] = array_merge($this->db[$user], $properties); - return $this->userPropertiesGet($user); - } - - public function userRightsGet(string $user): int { - return $this->db[$user]['rights']; - } - - public function userRightsSet(string $user, int $level): bool { - $this->db[$user]['rights'] = $level; - return true; - } -} diff --git a/tests/phpunit.xml b/tests/phpunit.xml index b58b0cd0..2a653ec2 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -34,10 +34,8 @@ cases/Misc/TestContext.php - cases/User/TestMockInternal.php - cases/User/TestMockExternal.php + cases/User/TestUser.php cases/User/TestInternal.php - cases/User/TestAuthorization.php cases/Feed/TestFetching.php @@ -100,4 +98,4 @@ cases/Service/TestService.php - \ No newline at end of file + From 057d72c8165ea5bcf80c63db36d3ab52668c6681 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Sun, 28 Oct 2018 13:50:57 -0400 Subject: [PATCH 02/16] Remove the distinction between internal and external user functionality --- lib/User.php | 164 ++++++++---------------- lib/User/Driver.php | 2 - lib/User/Internal/Driver.php | 48 ++++--- lib/User/Internal/InternalFunctions.php | 49 ------- 4 files changed, 80 insertions(+), 183 deletions(-) delete mode 100644 lib/User/Internal/InternalFunctions.php diff --git a/lib/User.php b/lib/User.php index 2e130345..ad645f88 100644 --- a/lib/User.php +++ b/lib/User.php @@ -6,6 +6,8 @@ declare(strict_types=1); namespace JKingWeb\Arsse; +use JKingWeb\Arsse\User\Internal\Driver as InternalDriver; + class User { public $id = null; @@ -44,30 +46,13 @@ class User { public function auth(string $user, string $password): bool { $prevUser = $this->id ?? null; $this->id = $user; - switch ($this->u->driverFunctions("auth")) { - case User\Driver::FUNC_EXTERNAL: - if (Arsse::$conf->userPreAuth) { - $out = true; - } else { - $out = $this->u->auth($user, $password); - } - if ($out && !Arsse::$db->userExists($user)) { - $this->autoProvision($user, $password); - } - break; - case User\Driver::FUNC_INTERNAL: - if (Arsse::$conf->userPreAuth) { - if (!Arsse::$db->userExists($user)) { - $this->autoProvision($user, $password); - } - $out = true; - } else { - $out = $this->u->auth($user, $password); - } - break; - case User\Driver::FUNCT_NOT_IMPLEMENTED: - $out = false; - break; + if (Arsse::$conf->userPreAuth) { + $out = true; + } else { + $out = $this->u->auth($user, $password); + } + if ($out && !Arsse::$db->userExists($user)) { + $this->autoProvision($user, $password); } if (!$out) { $this->id = $prevUser; @@ -75,118 +60,71 @@ class User { return $out; } - public function driverFunctions(string $function = null) { - return $this->u->driverFunctions($function); - } - public function list(): array { $func = "userList"; - switch ($this->u->driverFunctions($func)) { - case User\Driver::FUNC_EXTERNAL: - // we handle authorization checks for external drivers - if (!$this->authorize("", $func)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => ""]); - } - // no break - case User\Driver::FUNC_INTERNAL: - // internal functions handle their own authorization - return $this->u->userList($domain); - case User\Driver::FUNCT_NOT_IMPLEMENTED: - throw new User\ExceptionNotImplemented("notImplemented", ["action" => $func, "user" => $domain]); + if (!$this->authorize("", $func)) { + throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => ""]); } + return $this->u->userList($domain); } public function exists(string $user): bool { $func = "userExists"; - switch ($this->u->driverFunctions($func)) { - case User\Driver::FUNC_EXTERNAL: - // we handle authorization checks for external drivers - if (!$this->authorize($user, $func)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); - } - $out = $this->u->userExists($user); - if ($out && !Arsse::$db->userExists($user)) { - $this->autoProvision($user, ""); - } - return $out; - case User\Driver::FUNC_INTERNAL: - // internal functions handle their own authorization - return $this->u->userExists($user); - case User\Driver::FUNCT_NOT_IMPLEMENTED: - // throwing an exception here would break all kinds of stuff; we just report that the user exists - return true; + if (!$this->authorize($user, $func)) { + throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); } + $out = $this->u->userExists($user); + if (!$this->u instanceof InternalDriver) { + // if an alternative driver doesn't match the internal database, add or remove the user as appropriate + if (!$out && Arsse::$db->userExists($user)) { + Arsse::$db->userRemove($user); + } elseif ($out && !Arsse::$db->userExists($user)) { + $this->autoProvision($user, ""); + } + } + return $out; } public function add($user, $password = null): string { $func = "userAdd"; - switch ($this->u->driverFunctions($func)) { - case User\Driver::FUNC_EXTERNAL: - // we handle authorization checks for external drivers - if (!$this->authorize($user, $func)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); - } - $newPassword = $this->u->userAdd($user, $password); - // if there was no exception and we don't have the user in the internal database, add it - if (!Arsse::$db->userExists($user)) { - $this->autoProvision($user, $newPassword); - } - return $newPassword; - case User\Driver::FUNC_INTERNAL: - // internal functions handle their own authorization - return $this->u->userAdd($user, $password); - case User\Driver::FUNCT_NOT_IMPLEMENTED: - throw new User\ExceptionNotImplemented("notImplemented", ["action" => $func, "user" => $user]); + if (!$this->authorize($user, $func)) { + throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); } + $newPassword = $this->u->userAdd($user, $password); + // if there was no exception and we don't have the user in the internal database, add it + if (!$this->u instanceof InternalDriver && !Arsse::$db->userExists($user)) { + $this->autoProvision($user, $newPassword); + } + return $newPassword; } public function remove(string $user): bool { $func = "userRemove"; - switch ($this->u->driverFunctions($func)) { - case User\Driver::FUNC_EXTERNAL: - // we handle authorization checks for external drivers - if (!$this->authorize($user, $func)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); - } - $out = $this->u->userRemove($user); - if ($out && Arsse::$db->userExists($user)) { - // if the user was removed and we have it in our data, remove it there - if (!Arsse::$db->userExists($user)) { - Arsse::$db->userRemove($user); - } - } - return $out; - case User\Driver::FUNC_INTERNAL: - // internal functions handle their own authorization - return $this->u->userRemove($user); - case User\Driver::FUNCT_NOT_IMPLEMENTED: - throw new User\ExceptionNotImplemented("notImplemented", ["action" => $func, "user" => $user]); + if (!$this->authorize($user, $func)) { + throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); } + $out = $this->u->userRemove($user); + if ($out && !$this->u instanceof InternalDriver && Arsse::$db->userExists($user)) { + // if the user was removed and we have it in our data, remove it there + Arsse::$db->userRemove($user); + } + return $out; } public function passwordSet(string $user, string $newPassword = null, $oldPassword = null): string { $func = "userPasswordSet"; - switch ($this->u->driverFunctions($func)) { - case User\Driver::FUNC_EXTERNAL: - // we handle authorization checks for external drivers - if (!$this->authorize($user, $func)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); - } - $out = $this->u->userPasswordSet($user, $newPassword, $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); - } else { - // if the user does not exists in the internal database, create it - $this->autoProvision($user, $out); - } - return $out; - case User\Driver::FUNC_INTERNAL: - // internal functions handle their own authorization - return $this->u->userPasswordSet($user, $newPassword); - case User\Driver::FUNCT_NOT_IMPLEMENTED: - throw new User\ExceptionNotImplemented("notImplemented", ["action" => $func, "user" => $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)) { + // if the password change was successful and the user exists, set the internal password to the same value + Arsse::$db->userPasswordSet($user, $out); + } elseif (!$this->u instanceof InternalDriver){ + // if the user does not exists in the internal database, create it + $this->autoProvision($user, $out); + } + return $out; } protected function autoProvision(string $user, string $password = null): string { diff --git a/lib/User/Driver.php b/lib/User/Driver.php index 7b07dc4e..fa23ef9b 100644 --- a/lib/User/Driver.php +++ b/lib/User/Driver.php @@ -15,8 +15,6 @@ interface Driver { public function __construct(); // returns a human-friendly name for the driver (for display in installer, for example) public static function driverName(): string; - // returns an array (or single queried member of same) of methods defined by this interface and whether the class implements the internal function or a custom version - public function driverFunctions(string $function = null); // authenticates a user against their name and password public function auth(string $user, string $password): bool; // checks whether a user exists diff --git a/lib/User/Internal/Driver.php b/lib/User/Internal/Driver.php index ad3e50d9..25bcc74c 100644 --- a/lib/User/Internal/Driver.php +++ b/lib/User/Internal/Driver.php @@ -7,32 +7,42 @@ declare(strict_types=1); namespace JKingWeb\Arsse\User\Internal; final class Driver implements \JKingWeb\Arsse\User\Driver { - use InternalFunctions; - - protected $db; - protected $functions = [ - "auth" => self::FUNC_INTERNAL, - "userList" => self::FUNC_INTERNAL, - "userExists" => self::FUNC_INTERNAL, - "userAdd" => self::FUNC_INTERNAL, - "userRemove" => self::FUNC_INTERNAL, - "userPasswordSet" => self::FUNC_INTERNAL, - ]; + public function __construct() { + } public static function driverName(): string { return Arsse::$lang->msg("Driver.User.Internal.Name"); } - public function driverFunctions(string $function = null) { - if ($function===null) { - return $this->functions; + public function auth(string $user, string $password): bool { + try { + $hash = Arsse::$db->userPasswordGet($user); + } catch (Exception $e) { + return false; } - if (array_key_exists($function, $this->functions)) { - return $this->functions[$function]; - } else { - return self::FUNC_NOT_IMPLEMENTED; + if ($password==="" && $hash==="") { + return true; } + return password_verify($password, $hash); } - // see InternalFunctions.php for bulk of methods + public function userExists(string $user): bool { + return Arsse::$db->userExists($user); + } + + public function userAdd(string $user, string $password = null): string { + return Arsse::$db->userAdd($user, $password); + } + + public function userRemove(string $user): bool { + return Arsse::$db->userRemove($user); + } + + public function userList(): array { + return Arsse::$db->userList(); + } + + public function userPasswordSet(string $user, string $newPassword = null, string $oldPassword = null): string { + return Arsse::$db->userPasswordSet($user, $newPassword); + } } diff --git a/lib/User/Internal/InternalFunctions.php b/lib/User/Internal/InternalFunctions.php deleted file mode 100644 index e834b3d9..00000000 --- a/lib/User/Internal/InternalFunctions.php +++ /dev/null @@ -1,49 +0,0 @@ -userPasswordGet($user); - } catch (Exception $e) { - return false; - } - if ($password==="" && $hash==="") { - return true; - } - return password_verify($password, $hash); - } - - public function userExists(string $user): bool { - return Arsse::$db->userExists($user); - } - - public function userAdd(string $user, string $password = null): string { - return Arsse::$db->userAdd($user, $password); - } - - public function userRemove(string $user): bool { - return Arsse::$db->userRemove($user); - } - - public function userList(): array { - return Arsse::$db->userList(); - } - - public function userPasswordSet(string $user, string $newPassword = null, string $oldPassword = null): string { - return Arsse::$db->userPasswordSet($user, $newPassword); - } -} From 27edcddc9b36fbe5edb53a6b8291739d09394b07 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Sun, 28 Oct 2018 13:59:09 -0400 Subject: [PATCH 03/16] Simplify NCNv1 userStatus call --- lib/REST/NextCloudNews/V1_2.php | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/lib/REST/NextCloudNews/V1_2.php b/lib/REST/NextCloudNews/V1_2.php index 0ae19792..dacb86b2 100644 --- a/lib/REST/NextCloudNews/V1_2.php +++ b/lib/REST/NextCloudNews/V1_2.php @@ -651,24 +651,12 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler { } protected function userStatus(array $url, array $data): ResponseInterface { - $data = Arsse::$user->propertiesGet(Arsse::$user->id, true); - // construct the avatar structure, if an image is available - if (isset($data['avatar'])) { - $avatar = [ - 'data' => base64_encode($data['avatar']['data']), - 'mime' => (string) $data['avatar']['type'], - ]; - } else { - $avatar = null; - } - // construct the rest of the structure - $out = [ + return new Response([ 'userId' => (string) Arsse::$user->id, - 'displayName' => (string) ($data['name'] ?? Arsse::$user->id), + 'displayName' => (string) Arsse::$user->id, 'lastLoginTimestamp' => time(), - 'avatar' => $avatar, - ]; - return new Response($out); + 'avatar' => null, + ]); } protected function cleanupBefore(array $url, array $data): ResponseInterface { From 40d679844bd8f09f0ed90cca8d5adee9fad5df47 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Wed, 31 Oct 2018 14:32:11 -0400 Subject: [PATCH 04/16] Ensure the Lang class always exists when throwing exceptions --- lib/AbstractException.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/AbstractException.php b/lib/AbstractException.php index a4310603..38ff02bb 100644 --- a/lib/AbstractException.php +++ b/lib/AbstractException.php @@ -93,7 +93,7 @@ abstract class AbstractException extends \Exception { $code = self::CODES[$codeID]; $msg = "Exception.".str_replace("\\", "/", $class).".$msgID"; } - $msg = Arsse::$lang->msg($msg, $vars); + $msg = (Arsse::$lang ?? new Lang)->msg($msg, $vars); } parent::__construct($msg, $code, $e); } From 898533bde5ac2074ccf6e4dce6042377708b6ee7 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Fri, 2 Nov 2018 10:01:49 -0400 Subject: [PATCH 05/16] More simplification Authentication is now used as the primary point of synchronization between the internal database and any external database --- lib/User.php | 59 ++++++++++++------------------------ lib/User/Driver.php | 2 ++ lib/User/Internal/Driver.php | 6 +++- 3 files changed, 26 insertions(+), 41 deletions(-) diff --git a/lib/User.php b/lib/User.php index ad645f88..6e474645 100644 --- a/lib/User.php +++ b/lib/User.php @@ -29,34 +29,33 @@ class User { return $classes; } - public function __construct() { - $driver = Arsse::$conf->userDriver; - $this->u = new $driver(); + public function __construct(\JKingWeb\Arsse\User\Driver $driver = null) { + $this->u = $driver ?? new Arsse::$conf->userDriver; } public function __toString() { return (string) $this->id; } - // at one time there was a complicated authorization system; it exists vestigially to support a later revival if desired public function authorize(string $affectedUser, string $action): bool { - return true; + // at one time there was a complicated authorization system; it exists vestigially to support a later revival if desired + return $this->u->authorize($affectedUser, $action); } public function auth(string $user, string $password): bool { - $prevUser = $this->id ?? null; + $prevUser = $this->id; $this->id = $user; if (Arsse::$conf->userPreAuth) { $out = true; } else { $out = $this->u->auth($user, $password); } + // if authentication was successful and we don't have the user in the internal database, add it + // users must be in the internal database to preserve referential integrity if ($out && !Arsse::$db->userExists($user)) { - $this->autoProvision($user, $password); - } - if (!$out) { - $this->id = $prevUser; + Arsse::$db->userAdd($user, $password); } + $this->id = $prevUser; return $out; } @@ -65,7 +64,7 @@ class User { if (!$this->authorize("", $func)) { throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => ""]); } - return $this->u->userList($domain); + return $this->u->userList(); } public function exists(string $user): bool { @@ -73,16 +72,7 @@ class User { if (!$this->authorize($user, $func)) { throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); } - $out = $this->u->userExists($user); - if (!$this->u instanceof InternalDriver) { - // if an alternative driver doesn't match the internal database, add or remove the user as appropriate - if (!$out && Arsse::$db->userExists($user)) { - Arsse::$db->userRemove($user); - } elseif ($out && !Arsse::$db->userExists($user)) { - $this->autoProvision($user, ""); - } - } - return $out; + return $this->u->userExists($user); } public function add($user, $password = null): string { @@ -90,12 +80,7 @@ class User { if (!$this->authorize($user, $func)) { throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); } - $newPassword = $this->u->userAdd($user, $password); - // if there was no exception and we don't have the user in the internal database, add it - if (!$this->u instanceof InternalDriver && !Arsse::$db->userExists($user)) { - $this->autoProvision($user, $newPassword); - } - return $newPassword; + return $this->u->userAdd($user, $password); } public function remove(string $user): bool { @@ -103,12 +88,14 @@ class User { if (!$this->authorize($user, $func)) { throw new User\ExceptionAuthz("notAuthorized", ["action" => $func, "user" => $user]); } - $out = $this->u->userRemove($user); - if ($out && !$this->u instanceof InternalDriver && Arsse::$db->userExists($user)) { - // if the user was removed and we have it in our data, remove it there - Arsse::$db->userRemove($user); + try { + return $this->u->userRemove($user); + } finally { + 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 = null, $oldPassword = null): string { @@ -120,15 +107,7 @@ class User { if (!$this->u instanceof InternalDriver && 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); - } elseif (!$this->u instanceof InternalDriver){ - // if the user does not exists in the internal database, create it - $this->autoProvision($user, $out); } return $out; } - - protected function autoProvision(string $user, string $password = null): string { - $out = Arsse::$db->userAdd($user, $password); - return $out; - } } diff --git a/lib/User/Driver.php b/lib/User/Driver.php index fa23ef9b..91aeaa0e 100644 --- a/lib/User/Driver.php +++ b/lib/User/Driver.php @@ -17,6 +17,8 @@ interface Driver { public static function driverName(): string; // authenticates a user against their name and password public function auth(string $user, string $password): bool; + // check whether a user is authorized to perform a certain action; not currently used and subject to change + public function authorize(string $affectedUser, string $action): bool; // checks whether a user exists public function userExists(string $user): bool; // adds a user diff --git a/lib/User/Internal/Driver.php b/lib/User/Internal/Driver.php index 25bcc74c..5d65b764 100644 --- a/lib/User/Internal/Driver.php +++ b/lib/User/Internal/Driver.php @@ -6,7 +6,7 @@ declare(strict_types=1); namespace JKingWeb\Arsse\User\Internal; -final class Driver implements \JKingWeb\Arsse\User\Driver { +class Driver implements \JKingWeb\Arsse\User\Driver { public function __construct() { } @@ -26,6 +26,10 @@ final class Driver implements \JKingWeb\Arsse\User\Driver { return password_verify($password, $hash); } + public function authorize(string $affectedUser, string $action): bool { + return true; + } + public function userExists(string $user): bool { return Arsse::$db->userExists($user); } From 5959c0672d18be9c076f25964afd8fc5efbeca77 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Fri, 2 Nov 2018 10:02:37 -0400 Subject: [PATCH 06/16] Tests for most of the User class --- tests/cases/User/TestUser.php | 225 ++++++++++++++++++++++++++++++++++ 1 file changed, 225 insertions(+) create mode 100644 tests/cases/User/TestUser.php diff --git a/tests/cases/User/TestUser.php b/tests/cases/User/TestUser.php new file mode 100644 index 00000000..f5a746a4 --- /dev/null +++ b/tests/cases/User/TestUser.php @@ -0,0 +1,225 @@ +clearData(); + Arsse::$conf = new Conf; + // create a mock database interface + Arsse::$db = Phake::mock(Database::class); + Phake::when(Arsse::$db)->begin->thenReturn(Phake::mock(\JKingWeb\Arsse\Db\Transaction::class)); + } + + public function provideDriver() { + yield "Internal driver" => [Phake::mock(InternalDriver::class)]; + yield "Non-Internal Driver" => [Phake::mock(Driver::class)]; + } + + /** @dataProvider provideDriver */ + public function testConstruct(Driver $driver) { + $this->assertInstanceOf(User::class, new User($driver)); + } + + public function testConstructConfiguredDriver() { + $this->assertInstanceOf(User::class, new User); + } + + public function testConversionToString() { + $u = new User; + $u->id = "john.doe@example.com"; + $this->assertSame("john.doe@example.com", (string) $u); + $u->id = null; + $this->assertSame("", (string) $u); + } + + /** @dataProvider provideAuthentication */ + public function testAuthenticateAUser(Driver $driver, bool $preAuth, string $user, string $password, bool $exp) { + Arsse::$conf->userPreAuth = $preAuth; + Phake::when($driver)->auth->thenReturn(false); + Phake::when($driver)->auth("john.doe@example.com", "secret")->thenReturn(true); + Phake::when($driver)->auth("jane.doe@example.com", "superman")->thenReturn(true); + Phake::when(Arsse::$db)->userExists("john.doe@example.com")->thenReturn(true); + Phake::when(Arsse::$db)->userExists("jane.doe@example.com")->thenReturn(false); + Phake::when(Arsse::$db)->userAdd->thenReturn(""); + $u = new User($driver); + $this->assertSame($exp, $u->auth($user, $password)); + $this->assertNull($u->id); + Phake::verify(Arsse::$db, Phake::times($exp ? 1 : 0))->userExists($user); + Phake::verify(Arsse::$db, Phake::times($exp && $user == "jane.doe@example.com" ? 1 : 0))->userAdd($user, $password); + } + + public function provideAuthentication() { + $john = "john.doe@example.com"; + $jane = "jane.doe@example.com"; + $tests = [ + [false, $john, "secret", true], + [false, $john, "superman", false], + [false, $jane, "secret", false], + [false, $jane, "superman", true], + [true, $john, "secret", true], + [true, $john, "superman", true], + [true, $jane, "secret", true], + [true, $jane, "superman", true], + ]; + for ($a = 0; $a < sizeof($tests); $a++) { + list($preAuth, $user, $password, $outcome) = $tests[$a]; + foreach ($this->provideDriver() as $name => list($driver)) { + yield "$name #$a" => [$driver, $preAuth, $user, $password, $outcome]; + } + } + } + + /** @dataProvider provideUserList */ + public function testListUsers(Driver $driver, bool $authorized, $exp) { + $u = new User($driver); + Phake::when($driver)->authorize->thenReturn($authorized); + Phake::when($driver)->userList->thenReturn(["john.doe@example.com", "jane.doe@example.com"]); + if ($exp instanceof Exception) { + $this->assertException("notAuthorized", "User", "ExceptionAuthz"); + } + $this->assertSame($exp, $u->list()); + } + + public function provideUserList() { + $john = "john.doe@example.com"; + $jane = "jane.doe@example.com"; + $tests = [ + [false, new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], + [true, [$john, $jane]], + ]; + for ($a = 0; $a < sizeof($tests); $a++) { + list($authorized, $outcome) = $tests[$a]; + foreach ($this->provideDriver() as $name => list($driver)) { + yield "$name #$a" => [$driver, $authorized, $outcome]; + } + } + } + + /** @dataProvider provideExistence */ + public function testCheckThatAUserExists(Driver $driver, bool $authorized, string $user, $exp) { + $u = new User($driver); + Phake::when($driver)->authorize->thenReturn($authorized); + Phake::when($driver)->userExists("john.doe@example.com")->thenReturn(true); + Phake::when($driver)->userExists("jane.doe@example.com")->thenReturn(false); + if ($exp instanceof Exception) { + $this->assertException("notAuthorized", "User", "ExceptionAuthz"); + } + $this->assertSame($exp, $u->exists($user)); + } + + public function provideExistence() { + $john = "john.doe@example.com"; + $jane = "jane.doe@example.com"; + $tests = [ + [false, $john, new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], + [false, $jane, new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], + [true, $john, true], + [true, $jane, false], + ]; + for ($a = 0; $a < sizeof($tests); $a++) { + list($authorized, $user, $outcome) = $tests[$a]; + foreach ($this->provideDriver() as $name => list($driver)) { + yield "$name #$a" => [$driver, $authorized, $user, $outcome]; + } + } + } + + /** @dataProvider provideAdditions */ + public function testAddAUser(Driver $driver, bool $authorized, string $user, $password, $exp) { + $u = new User($driver); + Phake::when($driver)->authorize->thenReturn($authorized); + Phake::when($driver)->userAdd("john.doe@example.com", $this->anything())->thenThrow(new \JKingWeb\Arsse\User\Exception("alreadyExists")); + Phake::when($driver)->userAdd("jane.doe@example.com", $this->anything())->thenReturnCallback(function($user, $pass) { + return $pass ?? "random password"; + }); + if ($exp instanceof Exception) { + if ($exp instanceof \JKingWeb\Arsse\User\ExceptionAuthz) { + $this->assertException("notAuthorized", "User", "ExceptionAuthz"); + } else { + $this->assertException("alreadyExists", "User"); + } + } + $this->assertSame($exp, $u->add($user, $password)); + } + + public function provideAdditions() { + $john = "john.doe@example.com"; + $jane = "jane.doe@example.com"; + $tests = [ + [false, $john, "secret", new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], + [false, $jane, "superman", new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], + [true, $john, "secret", new \JKingWeb\Arsse\User\Exception("alreadyExists")], + [true, $jane, "superman", "superman"], + [true, $jane, null, "random password"], + ]; + for ($a = 0; $a < sizeof($tests); $a++) { + list($authorized, $user, $password, $outcome) = $tests[$a]; + foreach ($this->provideDriver() as $name => list($driver)) { + yield "$name #$a" => [$driver, $authorized, $user, $password, $outcome]; + } + } + } + + /** @dataProvider provideRemovals */ + public function testRemoveAUser(Driver $driver, bool $authorized, string $user, bool $exists, $exp) { + $u = new User($driver); + Phake::when($driver)->authorize->thenReturn($authorized); + Phake::when($driver)->userRemove("john.doe@example.com")->thenReturn(true); + Phake::when($driver)->userRemove("jane.doe@example.com")->thenThrow(new \JKingWeb\Arsse\User\Exception("doesNotExist")); + Phake::when(Arsse::$db)->userExists->thenReturn($exists); + Phake::when(Arsse::$db)->userRemove->thenReturn(true); + if ($exp instanceof Exception) { + if ($exp instanceof \JKingWeb\Arsse\User\ExceptionAuthz) { + $this->assertException("notAuthorized", "User", "ExceptionAuthz"); + } else { + $this->assertException("doesNotExist", "User"); + } + } + try { + $this->assertSame($exp, $u->remove($user)); + } finally { + Phake::verify(Arsse::$db, Phake::times((int) $authorized))->userExists($user); + Phake::verify(Arsse::$db, Phake::times((int) ($authorized && $exists)))->userRemove($user); + } + } + + public function provideRemovals() { + $john = "john.doe@example.com"; + $jane = "jane.doe@example.com"; + $tests = [ + [false, $john, true, new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], + [false, $john, false, new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], + [false, $jane, true, new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], + [false, $jane, false, new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], + [true, $john, true, true], + [true, $john, false, true], + [true, $jane, true, new \JKingWeb\Arsse\User\Exception("doesNotExist")], + [true, $jane, false, new \JKingWeb\Arsse\User\Exception("doesNotExist")], + ]; + for ($a = 0; $a < sizeof($tests); $a++) { + list($authorized, $user, $exists, $outcome) = $tests[$a]; + foreach ($this->provideDriver() as $name => list($driver)) { + yield "$name #$a" => [$driver, $authorized, $user, $exists, $outcome]; + } + } + } +} From 1b8e1e499b48d3e524c7d991725a60a76954428f Mon Sep 17 00:00:00 2001 From: "J. King" Date: Fri, 2 Nov 2018 10:02:49 -0400 Subject: [PATCH 07/16] Dev dependency update --- vendor-bin/csfixer/composer.lock | 32 ++++++++++++++++---------------- vendor-bin/robo/composer.lock | 22 +++++++++++----------- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/vendor-bin/csfixer/composer.lock b/vendor-bin/csfixer/composer.lock index 9b25d497..8df10e6d 100644 --- a/vendor-bin/csfixer/composer.lock +++ b/vendor-bin/csfixer/composer.lock @@ -811,7 +811,7 @@ }, { "name": "symfony/polyfill-ctype", - "version": "v1.9.0", + "version": "v1.10.0", "source": { "type": "git", "url": "https://github.com/symfony/polyfill-ctype.git", @@ -869,16 +869,16 @@ }, { "name": "symfony/polyfill-mbstring", - "version": "v1.9.0", + "version": "v1.10.0", "source": { "type": "git", "url": "https://github.com/symfony/polyfill-mbstring.git", - "reference": "d0cd638f4634c16d8df4508e847f14e9e43168b8" + "reference": "c79c051f5b3a46be09205c73b80b346e4153e494" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/symfony/polyfill-mbstring/zipball/d0cd638f4634c16d8df4508e847f14e9e43168b8", - "reference": "d0cd638f4634c16d8df4508e847f14e9e43168b8", + "url": "https://api.github.com/repos/symfony/polyfill-mbstring/zipball/c79c051f5b3a46be09205c73b80b346e4153e494", + "reference": "c79c051f5b3a46be09205c73b80b346e4153e494", "shasum": "" }, "require": { @@ -924,20 +924,20 @@ "portable", "shim" ], - "time": "2018-08-06T14:22:27+00:00" + "time": "2018-09-21T13:07:52+00:00" }, { "name": "symfony/polyfill-php70", - "version": "v1.9.0", + "version": "v1.10.0", "source": { "type": "git", "url": "https://github.com/symfony/polyfill-php70.git", - "reference": "1e24b0c4a56d55aaf368763a06c6d1c7d3194934" + "reference": "6b88000cdd431cd2e940caa2cb569201f3f84224" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/symfony/polyfill-php70/zipball/1e24b0c4a56d55aaf368763a06c6d1c7d3194934", - "reference": "1e24b0c4a56d55aaf368763a06c6d1c7d3194934", + "url": "https://api.github.com/repos/symfony/polyfill-php70/zipball/6b88000cdd431cd2e940caa2cb569201f3f84224", + "reference": "6b88000cdd431cd2e940caa2cb569201f3f84224", "shasum": "" }, "require": { @@ -983,20 +983,20 @@ "portable", "shim" ], - "time": "2018-08-06T14:22:27+00:00" + "time": "2018-09-21T06:26:08+00:00" }, { "name": "symfony/polyfill-php72", - "version": "v1.9.0", + "version": "v1.10.0", "source": { "type": "git", "url": "https://github.com/symfony/polyfill-php72.git", - "reference": "95c50420b0baed23852452a7f0c7b527303ed5ae" + "reference": "9050816e2ca34a8e916c3a0ae8b9c2fccf68b631" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/symfony/polyfill-php72/zipball/95c50420b0baed23852452a7f0c7b527303ed5ae", - "reference": "95c50420b0baed23852452a7f0c7b527303ed5ae", + "url": "https://api.github.com/repos/symfony/polyfill-php72/zipball/9050816e2ca34a8e916c3a0ae8b9c2fccf68b631", + "reference": "9050816e2ca34a8e916c3a0ae8b9c2fccf68b631", "shasum": "" }, "require": { @@ -1038,7 +1038,7 @@ "portable", "shim" ], - "time": "2018-08-06T14:22:27+00:00" + "time": "2018-09-21T13:07:52+00:00" }, { "name": "symfony/process", diff --git a/vendor-bin/robo/composer.lock b/vendor-bin/robo/composer.lock index 7418050b..04c63162 100644 --- a/vendor-bin/robo/composer.lock +++ b/vendor-bin/robo/composer.lock @@ -300,16 +300,16 @@ }, { "name": "consolidation/self-update", - "version": "1.1.4", + "version": "1.1.5", "source": { "type": "git", "url": "https://github.com/consolidation/self-update.git", - "reference": "4422e52d3fabeca9129ecb1780f198f202debdce" + "reference": "a1c273b14ce334789825a09d06d4c87c0a02ad54" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/consolidation/self-update/zipball/4422e52d3fabeca9129ecb1780f198f202debdce", - "reference": "4422e52d3fabeca9129ecb1780f198f202debdce", + "url": "https://api.github.com/repos/consolidation/self-update/zipball/a1c273b14ce334789825a09d06d4c87c0a02ad54", + "reference": "a1c273b14ce334789825a09d06d4c87c0a02ad54", "shasum": "" }, "require": { @@ -346,7 +346,7 @@ } ], "description": "Provides a self:update command for Symfony Console applications.", - "time": "2018-10-21T20:17:55+00:00" + "time": "2018-10-28T01:52:03+00:00" }, { "name": "container-interop/container-interop", @@ -1228,7 +1228,7 @@ }, { "name": "symfony/polyfill-ctype", - "version": "v1.9.0", + "version": "v1.10.0", "source": { "type": "git", "url": "https://github.com/symfony/polyfill-ctype.git", @@ -1286,16 +1286,16 @@ }, { "name": "symfony/polyfill-mbstring", - "version": "v1.9.0", + "version": "v1.10.0", "source": { "type": "git", "url": "https://github.com/symfony/polyfill-mbstring.git", - "reference": "d0cd638f4634c16d8df4508e847f14e9e43168b8" + "reference": "c79c051f5b3a46be09205c73b80b346e4153e494" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/symfony/polyfill-mbstring/zipball/d0cd638f4634c16d8df4508e847f14e9e43168b8", - "reference": "d0cd638f4634c16d8df4508e847f14e9e43168b8", + "url": "https://api.github.com/repos/symfony/polyfill-mbstring/zipball/c79c051f5b3a46be09205c73b80b346e4153e494", + "reference": "c79c051f5b3a46be09205c73b80b346e4153e494", "shasum": "" }, "require": { @@ -1341,7 +1341,7 @@ "portable", "shim" ], - "time": "2018-08-06T14:22:27+00:00" + "time": "2018-09-21T13:07:52+00:00" }, { "name": "symfony/process", From 31cdf313a4d499b9efe101724d882ccf28666b32 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Fri, 2 Nov 2018 11:47:10 -0400 Subject: [PATCH 08/16] Add missing return type hints where possible --- lib/Db/PDOError.php | 2 +- lib/Db/PDOResult.php | 4 ++-- lib/Db/Result.php | 4 ++-- lib/Db/ResultAggregate.php | 4 ++-- lib/Db/ResultEmpty.php | 4 ++-- lib/Db/SQLite3/ExceptionBuilder.php | 2 +- lib/Db/SQLite3/Result.php | 4 ++-- 7 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/Db/PDOError.php b/lib/Db/PDOError.php index 37341e4f..03e1f89c 100644 --- a/lib/Db/PDOError.php +++ b/lib/Db/PDOError.php @@ -7,7 +7,7 @@ declare(strict_types=1); namespace JKingWeb\Arsse\Db; trait PDOError { - public function exceptionBuild() { + public function exceptionBuild(): array { if ($this instanceof Statement) { $err = $this->st->errorInfo(); } else { diff --git a/lib/Db/PDOResult.php b/lib/Db/PDOResult.php index 32400e94..8889511c 100644 --- a/lib/Db/PDOResult.php +++ b/lib/Db/PDOResult.php @@ -16,11 +16,11 @@ class PDOResult extends AbstractResult { // actual public methods - public function changes() { + public function changes(): int { return $this->rows; } - public function lastId() { + public function lastId(): int { return $this->id; } diff --git a/lib/Db/Result.php b/lib/Db/Result.php index 91d0b42d..88c908b5 100644 --- a/lib/Db/Result.php +++ b/lib/Db/Result.php @@ -17,6 +17,6 @@ interface Result extends \Iterator { public function getAll(): array; public function getValue(); - public function changes(); - public function lastId(); + public function changes(): int; + public function lastId(): int; } diff --git a/lib/Db/ResultAggregate.php b/lib/Db/ResultAggregate.php index 84fb2822..4f53b9d2 100644 --- a/lib/Db/ResultAggregate.php +++ b/lib/Db/ResultAggregate.php @@ -15,13 +15,13 @@ class ResultAggregate extends AbstractResult { // actual public methods - public function changes() { + public function changes(): int { return array_reduce($this->data, function ($sum, $value) { return $sum + $value->changes(); }, 0); } - public function lastId() { + public function lastId(): int { return $this->data[sizeof($this->data) - 1]->lastId(); } diff --git a/lib/Db/ResultEmpty.php b/lib/Db/ResultEmpty.php index f11d75ea..60478b23 100644 --- a/lib/Db/ResultEmpty.php +++ b/lib/Db/ResultEmpty.php @@ -9,11 +9,11 @@ namespace JKingWeb\Arsse\Db; use JKingWeb\Arsse\Db\Exception; class ResultEmpty extends AbstractResult { - public function changes() { + public function changes(): int { return 0; } - public function lastId() { + public function lastId(): int { return 0; } diff --git a/lib/Db/SQLite3/ExceptionBuilder.php b/lib/Db/SQLite3/ExceptionBuilder.php index 6d5bee0a..4e631632 100644 --- a/lib/Db/SQLite3/ExceptionBuilder.php +++ b/lib/Db/SQLite3/ExceptionBuilder.php @@ -11,7 +11,7 @@ use JKingWeb\Arsse\Db\ExceptionInput; use JKingWeb\Arsse\Db\ExceptionTimeout; trait ExceptionBuilder { - public function exceptionBuild() { + public function exceptionBuild(): array { switch ($this->db->lastErrorCode()) { case self::SQLITE_BUSY: return [ExceptionTimeout::class, 'general', $this->db->lastErrorMsg()]; diff --git a/lib/Db/SQLite3/Result.php b/lib/Db/SQLite3/Result.php index 853a4bfd..d5e652fe 100644 --- a/lib/Db/SQLite3/Result.php +++ b/lib/Db/SQLite3/Result.php @@ -17,11 +17,11 @@ class Result extends \JKingWeb\Arsse\Db\AbstractResult { // actual public methods - public function changes() { + public function changes(): int { return $this->rows; } - public function lastId() { + public function lastId(): int { return $this->id; } From 931fe3b585e3d0b4b9f15698cc32e7f189c7f216 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Fri, 2 Nov 2018 11:52:55 -0400 Subject: [PATCH 09/16] 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"); From ffa7bd5a5d883f2b35f2364d7beb93eb76a476da Mon Sep 17 00:00:00 2001 From: "J. King" Date: Fri, 2 Nov 2018 12:01:03 -0400 Subject: [PATCH 10/16] Fix error in previous commit --- lib/Db/AbstractResult.php | 4 ++-- tests/lib/Result.php | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/Db/AbstractResult.php b/lib/Db/AbstractResult.php index fe4df26d..6086cf68 100644 --- a/lib/Db/AbstractResult.php +++ b/lib/Db/AbstractResult.php @@ -36,9 +36,9 @@ abstract class AbstractResult implements Result { return iterator_to_array($this, false); } - abstract public function changes(); + abstract public function changes(): int; - abstract public function lastId(); + abstract public function lastId(): int; // PHP iterator methods diff --git a/tests/lib/Result.php b/tests/lib/Result.php index 7381a344..554c50a8 100644 --- a/tests/lib/Result.php +++ b/tests/lib/Result.php @@ -37,11 +37,11 @@ class Result implements \JKingWeb\Arsse\Db\Result { return iterator_to_array($this, false); } - public function changes() { + public function changes(): int { return $this->rows; } - public function lastId() { + public function lastId(): int { return $this->id; } From a52b985826af31a4a1dec3023d76487049419e0a Mon Sep 17 00:00:00 2001 From: "J. King" Date: Fri, 2 Nov 2018 12:14:46 -0400 Subject: [PATCH 11/16] Cover the Query class with database tests for now --- tests/cases/Db/SQLite3/Database/TestArticle.php | 5 ++++- tests/cases/Db/SQLite3/Database/TestCleanup.php | 5 ++++- tests/cases/Db/SQLite3/Database/TestFeed.php | 5 ++++- tests/cases/Db/SQLite3/Database/TestFolder.php | 5 ++++- tests/cases/Db/SQLite3/Database/TestLabel.php | 5 ++++- tests/cases/Db/SQLite3/Database/TestMeta.php | 5 ++++- tests/cases/Db/SQLite3/Database/TestMiscellany.php | 5 ++++- tests/cases/Db/SQLite3/Database/TestSession.php | 5 ++++- tests/cases/Db/SQLite3/Database/TestSubscription.php | 5 ++++- tests/cases/Db/SQLite3/Database/TestUser.php | 5 ++++- tests/cases/Db/SQLite3PDO/Database/TestArticle.php | 5 ++++- tests/cases/Db/SQLite3PDO/Database/TestCleanup.php | 5 ++++- tests/cases/Db/SQLite3PDO/Database/TestFeed.php | 5 ++++- tests/cases/Db/SQLite3PDO/Database/TestFolder.php | 5 ++++- tests/cases/Db/SQLite3PDO/Database/TestLabel.php | 5 ++++- tests/cases/Db/SQLite3PDO/Database/TestMeta.php | 5 ++++- tests/cases/Db/SQLite3PDO/Database/TestMiscellany.php | 5 ++++- tests/cases/Db/SQLite3PDO/Database/TestSession.php | 5 ++++- tests/cases/Db/SQLite3PDO/Database/TestSubscription.php | 5 ++++- tests/cases/Db/SQLite3PDO/Database/TestUser.php | 5 ++++- 20 files changed, 80 insertions(+), 20 deletions(-) diff --git a/tests/cases/Db/SQLite3/Database/TestArticle.php b/tests/cases/Db/SQLite3/Database/TestArticle.php index 67480adf..9531d4d1 100644 --- a/tests/cases/Db/SQLite3/Database/TestArticle.php +++ b/tests/cases/Db/SQLite3/Database/TestArticle.php @@ -6,7 +6,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestArticle extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3; diff --git a/tests/cases/Db/SQLite3/Database/TestCleanup.php b/tests/cases/Db/SQLite3/Database/TestCleanup.php index 3ecba415..5374e1b4 100644 --- a/tests/cases/Db/SQLite3/Database/TestCleanup.php +++ b/tests/cases/Db/SQLite3/Database/TestCleanup.php @@ -6,7 +6,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestCleanup extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3; diff --git a/tests/cases/Db/SQLite3/Database/TestFeed.php b/tests/cases/Db/SQLite3/Database/TestFeed.php index 0d5bc9d9..e46a17fe 100644 --- a/tests/cases/Db/SQLite3/Database/TestFeed.php +++ b/tests/cases/Db/SQLite3/Database/TestFeed.php @@ -6,7 +6,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestFeed extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3; diff --git a/tests/cases/Db/SQLite3/Database/TestFolder.php b/tests/cases/Db/SQLite3/Database/TestFolder.php index 0f6c2a88..bc88e9af 100644 --- a/tests/cases/Db/SQLite3/Database/TestFolder.php +++ b/tests/cases/Db/SQLite3/Database/TestFolder.php @@ -6,7 +6,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestFolder extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3; diff --git a/tests/cases/Db/SQLite3/Database/TestLabel.php b/tests/cases/Db/SQLite3/Database/TestLabel.php index 2f5af8d1..70923207 100644 --- a/tests/cases/Db/SQLite3/Database/TestLabel.php +++ b/tests/cases/Db/SQLite3/Database/TestLabel.php @@ -2,7 +2,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestLabel extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3; diff --git a/tests/cases/Db/SQLite3/Database/TestMeta.php b/tests/cases/Db/SQLite3/Database/TestMeta.php index 17f8900d..0693d302 100644 --- a/tests/cases/Db/SQLite3/Database/TestMeta.php +++ b/tests/cases/Db/SQLite3/Database/TestMeta.php @@ -6,7 +6,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestMeta extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3; diff --git a/tests/cases/Db/SQLite3/Database/TestMiscellany.php b/tests/cases/Db/SQLite3/Database/TestMiscellany.php index d9b408cf..77014288 100644 --- a/tests/cases/Db/SQLite3/Database/TestMiscellany.php +++ b/tests/cases/Db/SQLite3/Database/TestMiscellany.php @@ -6,7 +6,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestMiscellany extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3; diff --git a/tests/cases/Db/SQLite3/Database/TestSession.php b/tests/cases/Db/SQLite3/Database/TestSession.php index 22600abd..f8344b52 100644 --- a/tests/cases/Db/SQLite3/Database/TestSession.php +++ b/tests/cases/Db/SQLite3/Database/TestSession.php @@ -2,7 +2,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestSession extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3; diff --git a/tests/cases/Db/SQLite3/Database/TestSubscription.php b/tests/cases/Db/SQLite3/Database/TestSubscription.php index 9f91cafb..c7c6c57e 100644 --- a/tests/cases/Db/SQLite3/Database/TestSubscription.php +++ b/tests/cases/Db/SQLite3/Database/TestSubscription.php @@ -6,7 +6,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestSubscription extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3; diff --git a/tests/cases/Db/SQLite3/Database/TestUser.php b/tests/cases/Db/SQLite3/Database/TestUser.php index 02042128..3659bf9f 100644 --- a/tests/cases/Db/SQLite3/Database/TestUser.php +++ b/tests/cases/Db/SQLite3/Database/TestUser.php @@ -6,7 +6,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3; diff --git a/tests/cases/Db/SQLite3PDO/Database/TestArticle.php b/tests/cases/Db/SQLite3PDO/Database/TestArticle.php index ac63498c..30521b42 100644 --- a/tests/cases/Db/SQLite3PDO/Database/TestArticle.php +++ b/tests/cases/Db/SQLite3PDO/Database/TestArticle.php @@ -6,7 +6,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3PDO\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestArticle extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3PDO; diff --git a/tests/cases/Db/SQLite3PDO/Database/TestCleanup.php b/tests/cases/Db/SQLite3PDO/Database/TestCleanup.php index 76e45308..708001d4 100644 --- a/tests/cases/Db/SQLite3PDO/Database/TestCleanup.php +++ b/tests/cases/Db/SQLite3PDO/Database/TestCleanup.php @@ -6,7 +6,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3PDO\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestCleanup extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3PDO; diff --git a/tests/cases/Db/SQLite3PDO/Database/TestFeed.php b/tests/cases/Db/SQLite3PDO/Database/TestFeed.php index 9ffc733e..e662d8e6 100644 --- a/tests/cases/Db/SQLite3PDO/Database/TestFeed.php +++ b/tests/cases/Db/SQLite3PDO/Database/TestFeed.php @@ -6,7 +6,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3PDO\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestFeed extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3PDO; diff --git a/tests/cases/Db/SQLite3PDO/Database/TestFolder.php b/tests/cases/Db/SQLite3PDO/Database/TestFolder.php index 30cff605..777a0110 100644 --- a/tests/cases/Db/SQLite3PDO/Database/TestFolder.php +++ b/tests/cases/Db/SQLite3PDO/Database/TestFolder.php @@ -6,7 +6,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3PDO\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestFolder extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3PDO; diff --git a/tests/cases/Db/SQLite3PDO/Database/TestLabel.php b/tests/cases/Db/SQLite3PDO/Database/TestLabel.php index 38bd1238..b2fe1580 100644 --- a/tests/cases/Db/SQLite3PDO/Database/TestLabel.php +++ b/tests/cases/Db/SQLite3PDO/Database/TestLabel.php @@ -2,7 +2,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3PDO\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestLabel extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3PDO; diff --git a/tests/cases/Db/SQLite3PDO/Database/TestMeta.php b/tests/cases/Db/SQLite3PDO/Database/TestMeta.php index 7d3e7ed7..96981311 100644 --- a/tests/cases/Db/SQLite3PDO/Database/TestMeta.php +++ b/tests/cases/Db/SQLite3PDO/Database/TestMeta.php @@ -6,7 +6,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3PDO\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestMeta extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3PDO; diff --git a/tests/cases/Db/SQLite3PDO/Database/TestMiscellany.php b/tests/cases/Db/SQLite3PDO/Database/TestMiscellany.php index cc8ca608..868e7fc5 100644 --- a/tests/cases/Db/SQLite3PDO/Database/TestMiscellany.php +++ b/tests/cases/Db/SQLite3PDO/Database/TestMiscellany.php @@ -6,7 +6,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3PDO\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestMiscellany extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3PDO; diff --git a/tests/cases/Db/SQLite3PDO/Database/TestSession.php b/tests/cases/Db/SQLite3PDO/Database/TestSession.php index dbc71fb1..88535b2b 100644 --- a/tests/cases/Db/SQLite3PDO/Database/TestSession.php +++ b/tests/cases/Db/SQLite3PDO/Database/TestSession.php @@ -2,7 +2,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3PDO\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestSession extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3PDO; diff --git a/tests/cases/Db/SQLite3PDO/Database/TestSubscription.php b/tests/cases/Db/SQLite3PDO/Database/TestSubscription.php index 0205b48e..83e7daf8 100644 --- a/tests/cases/Db/SQLite3PDO/Database/TestSubscription.php +++ b/tests/cases/Db/SQLite3PDO/Database/TestSubscription.php @@ -6,7 +6,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3PDO\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestSubscription extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3PDO; diff --git a/tests/cases/Db/SQLite3PDO/Database/TestUser.php b/tests/cases/Db/SQLite3PDO/Database/TestUser.php index b77822dd..18b0c05a 100644 --- a/tests/cases/Db/SQLite3PDO/Database/TestUser.php +++ b/tests/cases/Db/SQLite3PDO/Database/TestUser.php @@ -6,7 +6,10 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\SQLite3PDO\Database; -/** @covers \JKingWeb\Arsse\Database */ +/** + * @covers \JKingWeb\Arsse\Database + * @covers \JKingWeb\Arsse\Misc\Query + */ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { use \JKingWeb\Arsse\Test\Database\Setup; use \JKingWeb\Arsse\Test\Database\DriverSQLite3PDO; From b8f8a617fec7ec4ebad75360763ad279cea0d819 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Fri, 2 Nov 2018 17:28:12 -0400 Subject: [PATCH 12/16] Simply user test data providers The user manager no longer differentiates between the internal driver and other drivers, making the duplication unnecessary --- tests/cases/User/TestUser.php | 103 +++++++++++----------------------- 1 file changed, 33 insertions(+), 70 deletions(-) diff --git a/tests/cases/User/TestUser.php b/tests/cases/User/TestUser.php index f5a746a4..572d9896 100644 --- a/tests/cases/User/TestUser.php +++ b/tests/cases/User/TestUser.php @@ -27,19 +27,12 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { // create a mock database interface Arsse::$db = Phake::mock(Database::class); Phake::when(Arsse::$db)->begin->thenReturn(Phake::mock(\JKingWeb\Arsse\Db\Transaction::class)); + // create a mock user driver + $this->drv = Phake::mock(Driver::class); } - public function provideDriver() { - yield "Internal driver" => [Phake::mock(InternalDriver::class)]; - yield "Non-Internal Driver" => [Phake::mock(Driver::class)]; - } - - /** @dataProvider provideDriver */ - public function testConstruct(Driver $driver) { - $this->assertInstanceOf(User::class, new User($driver)); - } - - public function testConstructConfiguredDriver() { + public function testConstruct() { + $this->assertInstanceOf(User::class, new User($this->drv)); $this->assertInstanceOf(User::class, new User); } @@ -52,15 +45,15 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { } /** @dataProvider provideAuthentication */ - public function testAuthenticateAUser(Driver $driver, bool $preAuth, string $user, string $password, bool $exp) { + public function testAuthenticateAUser(bool $preAuth, string $user, string $password, bool $exp) { Arsse::$conf->userPreAuth = $preAuth; - Phake::when($driver)->auth->thenReturn(false); - Phake::when($driver)->auth("john.doe@example.com", "secret")->thenReturn(true); - Phake::when($driver)->auth("jane.doe@example.com", "superman")->thenReturn(true); + Phake::when($this->drv)->auth->thenReturn(false); + Phake::when($this->drv)->auth("john.doe@example.com", "secret")->thenReturn(true); + Phake::when($this->drv)->auth("jane.doe@example.com", "superman")->thenReturn(true); Phake::when(Arsse::$db)->userExists("john.doe@example.com")->thenReturn(true); Phake::when(Arsse::$db)->userExists("jane.doe@example.com")->thenReturn(false); Phake::when(Arsse::$db)->userAdd->thenReturn(""); - $u = new User($driver); + $u = new User($this->drv); $this->assertSame($exp, $u->auth($user, $password)); $this->assertNull($u->id); Phake::verify(Arsse::$db, Phake::times($exp ? 1 : 0))->userExists($user); @@ -70,7 +63,7 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { public function provideAuthentication() { $john = "john.doe@example.com"; $jane = "jane.doe@example.com"; - $tests = [ + return [ [false, $john, "secret", true], [false, $john, "superman", false], [false, $jane, "secret", false], @@ -80,19 +73,13 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { [true, $jane, "secret", true], [true, $jane, "superman", true], ]; - for ($a = 0; $a < sizeof($tests); $a++) { - list($preAuth, $user, $password, $outcome) = $tests[$a]; - foreach ($this->provideDriver() as $name => list($driver)) { - yield "$name #$a" => [$driver, $preAuth, $user, $password, $outcome]; - } - } } /** @dataProvider provideUserList */ - public function testListUsers(Driver $driver, bool $authorized, $exp) { - $u = new User($driver); - Phake::when($driver)->authorize->thenReturn($authorized); - Phake::when($driver)->userList->thenReturn(["john.doe@example.com", "jane.doe@example.com"]); + public function testListUsers(bool $authorized, $exp) { + $u = new User($this->drv); + Phake::when($this->drv)->authorize->thenReturn($authorized); + Phake::when($this->drv)->userList->thenReturn(["john.doe@example.com", "jane.doe@example.com"]); if ($exp instanceof Exception) { $this->assertException("notAuthorized", "User", "ExceptionAuthz"); } @@ -102,24 +89,18 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { public function provideUserList() { $john = "john.doe@example.com"; $jane = "jane.doe@example.com"; - $tests = [ + return [ [false, new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], [true, [$john, $jane]], ]; - for ($a = 0; $a < sizeof($tests); $a++) { - list($authorized, $outcome) = $tests[$a]; - foreach ($this->provideDriver() as $name => list($driver)) { - yield "$name #$a" => [$driver, $authorized, $outcome]; - } - } } /** @dataProvider provideExistence */ - public function testCheckThatAUserExists(Driver $driver, bool $authorized, string $user, $exp) { - $u = new User($driver); - Phake::when($driver)->authorize->thenReturn($authorized); - Phake::when($driver)->userExists("john.doe@example.com")->thenReturn(true); - Phake::when($driver)->userExists("jane.doe@example.com")->thenReturn(false); + public function testCheckThatAUserExists(bool $authorized, string $user, $exp) { + $u = new User($this->drv); + Phake::when($this->drv)->authorize->thenReturn($authorized); + Phake::when($this->drv)->userExists("john.doe@example.com")->thenReturn(true); + Phake::when($this->drv)->userExists("jane.doe@example.com")->thenReturn(false); if ($exp instanceof Exception) { $this->assertException("notAuthorized", "User", "ExceptionAuthz"); } @@ -129,26 +110,20 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { public function provideExistence() { $john = "john.doe@example.com"; $jane = "jane.doe@example.com"; - $tests = [ + return [ [false, $john, new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], [false, $jane, new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], [true, $john, true], [true, $jane, false], ]; - for ($a = 0; $a < sizeof($tests); $a++) { - list($authorized, $user, $outcome) = $tests[$a]; - foreach ($this->provideDriver() as $name => list($driver)) { - yield "$name #$a" => [$driver, $authorized, $user, $outcome]; - } - } } /** @dataProvider provideAdditions */ - public function testAddAUser(Driver $driver, bool $authorized, string $user, $password, $exp) { - $u = new User($driver); - Phake::when($driver)->authorize->thenReturn($authorized); - Phake::when($driver)->userAdd("john.doe@example.com", $this->anything())->thenThrow(new \JKingWeb\Arsse\User\Exception("alreadyExists")); - Phake::when($driver)->userAdd("jane.doe@example.com", $this->anything())->thenReturnCallback(function($user, $pass) { + public function testAddAUser(bool $authorized, string $user, $password, $exp) { + $u = new User($this->drv); + Phake::when($this->drv)->authorize->thenReturn($authorized); + Phake::when($this->drv)->userAdd("john.doe@example.com", $this->anything())->thenThrow(new \JKingWeb\Arsse\User\Exception("alreadyExists")); + Phake::when($this->drv)->userAdd("jane.doe@example.com", $this->anything())->thenReturnCallback(function($user, $pass) { return $pass ?? "random password"; }); if ($exp instanceof Exception) { @@ -164,27 +139,21 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { public function provideAdditions() { $john = "john.doe@example.com"; $jane = "jane.doe@example.com"; - $tests = [ + return [ [false, $john, "secret", new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], [false, $jane, "superman", new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], [true, $john, "secret", new \JKingWeb\Arsse\User\Exception("alreadyExists")], [true, $jane, "superman", "superman"], [true, $jane, null, "random password"], ]; - for ($a = 0; $a < sizeof($tests); $a++) { - list($authorized, $user, $password, $outcome) = $tests[$a]; - foreach ($this->provideDriver() as $name => list($driver)) { - yield "$name #$a" => [$driver, $authorized, $user, $password, $outcome]; - } - } } /** @dataProvider provideRemovals */ - public function testRemoveAUser(Driver $driver, bool $authorized, string $user, bool $exists, $exp) { - $u = new User($driver); - Phake::when($driver)->authorize->thenReturn($authorized); - Phake::when($driver)->userRemove("john.doe@example.com")->thenReturn(true); - Phake::when($driver)->userRemove("jane.doe@example.com")->thenThrow(new \JKingWeb\Arsse\User\Exception("doesNotExist")); + public function testRemoveAUser(bool $authorized, string $user, bool $exists, $exp) { + $u = new User($this->drv); + Phake::when($this->drv)->authorize->thenReturn($authorized); + Phake::when($this->drv)->userRemove("john.doe@example.com")->thenReturn(true); + Phake::when($this->drv)->userRemove("jane.doe@example.com")->thenThrow(new \JKingWeb\Arsse\User\Exception("doesNotExist")); Phake::when(Arsse::$db)->userExists->thenReturn($exists); Phake::when(Arsse::$db)->userRemove->thenReturn(true); if ($exp instanceof Exception) { @@ -205,7 +174,7 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { public function provideRemovals() { $john = "john.doe@example.com"; $jane = "jane.doe@example.com"; - $tests = [ + return [ [false, $john, true, new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], [false, $john, false, new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], [false, $jane, true, new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], @@ -215,11 +184,5 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { [true, $jane, true, new \JKingWeb\Arsse\User\Exception("doesNotExist")], [true, $jane, false, new \JKingWeb\Arsse\User\Exception("doesNotExist")], ]; - for ($a = 0; $a < sizeof($tests); $a++) { - list($authorized, $user, $exists, $outcome) = $tests[$a]; - foreach ($this->provideDriver() as $name => list($driver)) { - yield "$name #$a" => [$driver, $authorized, $user, $exists, $outcome]; - } - } } } From 1ac85df46bd595366b5b77a8de6134ac86973cf8 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Sat, 3 Nov 2018 13:26:22 -0400 Subject: [PATCH 13/16] Last set of tests for User class --- lib/User/Internal/Driver.php | 2 + tests/cases/User/TestUser.php | 118 +++++++++++++++++++++++++++++++++- 2 files changed, 117 insertions(+), 3 deletions(-) diff --git a/lib/User/Internal/Driver.php b/lib/User/Internal/Driver.php index b6dfe96c..e5928a28 100644 --- a/lib/User/Internal/Driver.php +++ b/lib/User/Internal/Driver.php @@ -6,6 +6,8 @@ declare(strict_types=1); namespace JKingWeb\Arsse\User\Internal; +use JKingWeb\Arsse\Arsse; + class Driver implements \JKingWeb\Arsse\User\Driver { public function __construct() { } diff --git a/tests/cases/User/TestUser.php b/tests/cases/User/TestUser.php index 572d9896..bdb5d032 100644 --- a/tests/cases/User/TestUser.php +++ b/tests/cases/User/TestUser.php @@ -17,9 +17,6 @@ use Phake; /** @covers \JKingWeb\Arsse\User */ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { - public static function setUpBeforeClass() { - Arsse::$lang = new \JKingWeb\Arsse\Lang(); - } public function setUp() { $this->clearData(); @@ -30,6 +27,12 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { // create a mock user driver $this->drv = Phake::mock(Driver::class); } + public function testListDrivers() { + $exp = [ + 'JKingWeb\\Arsse\\User\\Internal\\Driver' => Arsse::$lang->msg("Driver.User.Internal.Name"), + ]; + $this->assertArraySubset($exp, User::driverList()); + } public function testConstruct() { $this->assertInstanceOf(User::class, new User($this->drv)); @@ -136,6 +139,36 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { $this->assertSame($exp, $u->add($user, $password)); } + /** @dataProvider provideAdditions */ + public function testAddAUserWithARandomPassword(bool $authorized, string $user, $password, $exp) { + $u = Phake::partialMock(User::class, $this->drv); + Phake::when($this->drv)->authorize->thenReturn($authorized); + Phake::when($this->drv)->userAdd($this->anything(), $this->isNull())->thenReturn(null); + Phake::when($this->drv)->userAdd("john.doe@example.com", $this->logicalNot($this->isNull()))->thenThrow(new \JKingWeb\Arsse\User\Exception("alreadyExists")); + Phake::when($this->drv)->userAdd("jane.doe@example.com", $this->logicalNot($this->isNull()))->thenReturnCallback(function($user, $pass) { + return $pass; + }); + if ($exp instanceof Exception) { + if ($exp instanceof \JKingWeb\Arsse\User\ExceptionAuthz) { + $this->assertException("notAuthorized", "User", "ExceptionAuthz"); + $calls = 0; + } else { + $this->assertException("alreadyExists", "User"); + $calls = 2; + } + } else { + $calls = 4; + } + try { + $pass1 = $u->add($user, null); + $pass2 = $u->add($user, null); + $this->assertNotEquals($pass1, $pass2); + } finally { + Phake::verify($this->drv, Phake::times($calls))->userAdd; + Phake::verify($u, Phake::times($calls / 2))->generatePassword; + } + } + public function provideAdditions() { $john = "john.doe@example.com"; $jane = "jane.doe@example.com"; @@ -185,4 +218,83 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { [true, $jane, false, new \JKingWeb\Arsse\User\Exception("doesNotExist")], ]; } + + /** @dataProvider providePasswordChanges */ + public function testChangeAPassword(bool $authorized, string $user, $password, bool $exists, $exp) { + $u = new User($this->drv); + Phake::when($this->drv)->authorize->thenReturn($authorized); + Phake::when($this->drv)->userPasswordSet("john.doe@example.com", $this->anything(), $this->anything())->thenReturnCallback(function($user, $pass, $old) { + return $pass ?? "random password"; + }); + Phake::when($this->drv)->userPasswordSet("jane.doe@example.com", $this->anything(), $this->anything())->thenThrow(new \JKingWeb\Arsse\User\Exception("doesNotExist")); + Phake::when(Arsse::$db)->userExists->thenReturn($exists); + if ($exp instanceof Exception) { + if ($exp instanceof \JKingWeb\Arsse\User\ExceptionAuthz) { + $this->assertException("notAuthorized", "User", "ExceptionAuthz"); + } else { + $this->assertException("doesNotExist", "User"); + } + $calls = 0; + } else{ + $calls = 1; + } + try { + $this->assertSame($exp, $u->passwordSet($user, $password)); + } finally { + Phake::verify(Arsse::$db, Phake::times($calls))->userExists($user); + Phake::verify(Arsse::$db, Phake::times($exists ? $calls : 0))->userPasswordSet($user, $password ?? "random password", null); + } + } + + /** @dataProvider providePasswordChanges */ + public function testChangeAPasswordToARandomPassword(bool $authorized, string $user, $password, bool $exists, $exp) { + $u = Phake::partialMock(User::class, $this->drv); + Phake::when($this->drv)->authorize->thenReturn($authorized); + Phake::when($this->drv)->userPasswordSet($this->anything(), $this->isNull(), $this->anything())->thenReturn(null); + Phake::when($this->drv)->userPasswordSet("john.doe@example.com", $this->logicalNot($this->isNull()), $this->anything())->thenReturnCallback(function($user, $pass, $old) { + return $pass ?? "random password"; + }); + Phake::when($this->drv)->userPasswordSet("jane.doe@example.com", $this->logicalNot($this->isNull()), $this->anything())->thenThrow(new \JKingWeb\Arsse\User\Exception("doesNotExist")); + Phake::when(Arsse::$db)->userExists->thenReturn($exists); + if ($exp instanceof Exception) { + if ($exp instanceof \JKingWeb\Arsse\User\ExceptionAuthz) { + $this->assertException("notAuthorized", "User", "ExceptionAuthz"); + $calls = 0; + } else { + $this->assertException("doesNotExist", "User"); + $calls = 2; + } + } else { + $calls = 4; + } + try { + $pass1 = $u->passwordSet($user, null); + $pass2 = $u->passwordSet($user, null); + $this->assertNotEquals($pass1, $pass2); + } finally { + Phake::verify($this->drv, Phake::times($calls))->userPasswordSet; + Phake::verify($u, Phake::times($calls / 2))->generatePassword; + Phake::verify(Arsse::$db, Phake::times($calls==4 ? 2 : 0))->userExists($user); + if ($calls == 4) { + Phake::verify(Arsse::$db, Phake::times($exists ? 1 : 0))->userPasswordSet($user, $pass1, null); + Phake::verify(Arsse::$db, Phake::times($exists ? 1 : 0))->userPasswordSet($user, $pass2, null); + } else { + Phake::verify(Arsse::$db, Phake::times(0))->userPasswordSet; + } + } + } + + public function providePasswordChanges() { + $john = "john.doe@example.com"; + $jane = "jane.doe@example.com"; + return [ + [false, $john, "secret", true, new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], + [false, $jane, "superman", false, new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")], + [true, $john, "superman", true, "superman"], + [true, $john, null, true, "random password"], + [true, $john, "superman", false, "superman"], + [true, $john, null, false, "random password"], + [true, $jane, "secret", true, new \JKingWeb\Arsse\User\Exception("doesNotExist")], + ]; + } } From 5f775bef7a12278c9d3e5abac7b6077f4e85743f Mon Sep 17 00:00:00 2001 From: "J. King" Date: Sat, 3 Nov 2018 13:49:02 -0400 Subject: [PATCH 14/16] Appease phpdbg coverage bug --- lib/User.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/User.php b/lib/User.php index b0b05bcd..6ccdbcc2 100644 --- a/lib/User.php +++ b/lib/User.php @@ -90,7 +90,7 @@ class User { } try { return $this->u->userRemove($user); - } finally { + } finally { // @codeCoverageIgnore 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); From a8cc9a4780a8765d61fc06d061fadb334d2b415a Mon Sep 17 00:00:00 2001 From: "J. King" Date: Sun, 4 Nov 2018 12:06:30 -0500 Subject: [PATCH 15/16] Tests for internal user driver; closes #50 --- lib/User/Internal/Driver.php | 7 +- tests/cases/User/TestInternal.php | 137 ++++++++++++++++++++++++++++++ tests/cases/User/TestUser.php | 3 +- tests/phpunit.xml | 2 +- 4 files changed, 146 insertions(+), 3 deletions(-) create mode 100644 tests/cases/User/TestInternal.php diff --git a/lib/User/Internal/Driver.php b/lib/User/Internal/Driver.php index e5928a28..4c730257 100644 --- a/lib/User/Internal/Driver.php +++ b/lib/User/Internal/Driver.php @@ -7,6 +7,7 @@ declare(strict_types=1); namespace JKingWeb\Arsse\User\Internal; use JKingWeb\Arsse\Arsse; +use JKingWeb\Arsse\User\Exception; class Driver implements \JKingWeb\Arsse\User\Driver { public function __construct() { @@ -18,7 +19,7 @@ class Driver implements \JKingWeb\Arsse\User\Driver { public function auth(string $user, string $password): bool { try { - $hash = Arsse::$db->userPasswordGet($user); + $hash = $this->userPasswordGet($user); } catch (Exception $e) { return false; } @@ -56,4 +57,8 @@ class Driver implements \JKingWeb\Arsse\User\Driver { // do nothing: the internal database is updated regardless of what the driver does (assuming it does not throw an exception) return $newPassword; } + + protected function userPasswordGet(string $user): string { + return Arsse::$db->userPasswordGet($user); + } } diff --git a/tests/cases/User/TestInternal.php b/tests/cases/User/TestInternal.php new file mode 100644 index 00000000..68b597d4 --- /dev/null +++ b/tests/cases/User/TestInternal.php @@ -0,0 +1,137 @@ +clearData(); + $this->setConf(); + // create a mock database interface + Arsse::$db = Phake::mock(Database::class); + Phake::when(Arsse::$db)->begin->thenReturn(Phake::mock(\JKingWeb\Arsse\Db\Transaction::class)); + } + + public function testConstruct() { + $this->assertInstanceOf(DriverInterface::class, new Driver); + } + + public function testFetchDriverName() { + $this->assertTrue(strlen(Driver::driverName()) > 0); + } + + /** + * @dataProvider provideAuthentication + * @group slow + */ + public function testAuthenticateAUser(bool $authorized, string $user, string $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")); + } else { + Phake::when(Arsse::$db)->userPasswordGet->thenThrow(new \JKingWeb\Arsse\User\ExceptionAuthz("notAuthorized")); + } + $this->assertSame($exp, (new Driver)->auth($user, $password)); + } + + public function provideAuthentication() { + $john = "john.doe@example.com"; + $jane = "jane.doe@example.com"; + $owen = "owen.hardy@example.com"; + $kira = "kira.nerys@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], + ]; + } + + public function testAuthorizeAnAction() { + Phake::verifyNoFurtherInteraction(Arsse::$db); + $this->assertTrue((new Driver)->authorize("someone", "something")); + } + + public function testListUsers() { + $john = "john.doe@example.com"; + $jane = "jane.doe@example.com"; + Phake::when(Arsse::$db)->userList->thenReturn([$john, $jane])->thenReturn([$jane, $john]); + $driver = new Driver; + $this->assertSame([$john, $jane], $driver->userList()); + $this->assertSame([$jane, $john], $driver->userList()); + Phake::verify(Arsse::$db, Phake::times(2))->userList; + } + + public function testCheckThatAUserExists() { + $john = "john.doe@example.com"; + $jane = "jane.doe@example.com"; + Phake::when(Arsse::$db)->userExists($john)->thenReturn(true); + Phake::when(Arsse::$db)->userExists($jane)->thenReturn(false); + $driver = new Driver; + $this->assertTrue($driver->userExists($john)); + Phake::verify(Arsse::$db)->userExists($john); + $this->assertFalse($driver->userExists($jane)); + Phake::verify(Arsse::$db)->userExists($jane); + } + + public function testAddAUser() { + $john = "john.doe@example.com"; + Phake::when(Arsse::$db)->userAdd->thenReturnCallback(function($user, $pass) { + return $pass; + }); + $driver = new Driver; + $this->assertNull($driver->userAdd($john)); + $this->assertNull($driver->userAdd($john, null)); + $this->assertSame("secret", $driver->userAdd($john, "secret")); + Phake::verify(Arsse::$db)->userAdd($john, "secret"); + Phake::verify(Arsse::$db)->userAdd; + } + + public function testRemoveAUser() { + $john = "john.doe@example.com"; + Phake::when(Arsse::$db)->userRemove->thenReturn(true)->thenThrow(new \JKingWeb\Arsse\User\Exception("doesNotExist")); + $driver = new Driver; + $this->assertTrue($driver->userRemove($john)); + Phake::verify(Arsse::$db, Phake::times(1))->userRemove($john); + $this->assertException("doesNotExist", "User"); + try { + $this->assertFalse($driver->userRemove($john)); + } finally { + Phake::verify(Arsse::$db, Phake::times(2))->userRemove($john); + } + } + + public function testSetAPassword() { + $john = "john.doe@example.com"; + Phake::verifyNoFurtherInteraction(Arsse::$db); + $this->assertSame("superman", (new Driver)->userPasswordSet($john, "superman")); + $this->assertSame(null, (new Driver)->userPasswordSet($john, null)); + } +} diff --git a/tests/cases/User/TestUser.php b/tests/cases/User/TestUser.php index bdb5d032..806bfff2 100644 --- a/tests/cases/User/TestUser.php +++ b/tests/cases/User/TestUser.php @@ -20,13 +20,14 @@ class TestUser extends \JKingWeb\Arsse\Test\AbstractTest { public function setUp() { $this->clearData(); - Arsse::$conf = new Conf; + $this->setConf(); // create a mock database interface Arsse::$db = Phake::mock(Database::class); Phake::when(Arsse::$db)->begin->thenReturn(Phake::mock(\JKingWeb\Arsse\Db\Transaction::class)); // create a mock user driver $this->drv = Phake::mock(Driver::class); } + public function testListDrivers() { $exp = [ 'JKingWeb\\Arsse\\User\\Internal\\Driver' => Arsse::$lang->msg("Driver.User.Internal.Name"), diff --git a/tests/phpunit.xml b/tests/phpunit.xml index 2a653ec2..0cbb54bc 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -34,8 +34,8 @@ cases/Misc/TestContext.php - cases/User/TestUser.php cases/User/TestInternal.php + cases/User/TestUser.php cases/Feed/TestFetching.php From 4869559fb3645b69461c90b890ac23046ad41a55 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Tue, 6 Nov 2018 13:21:53 -0500 Subject: [PATCH 16/16] Test NCNv1 user query --- tests/cases/REST/NextCloudNews/TestV1_2.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/cases/REST/NextCloudNews/TestV1_2.php b/tests/cases/REST/NextCloudNews/TestV1_2.php index 4ee6e513..e07e90be 100644 --- a/tests/cases/REST/NextCloudNews/TestV1_2.php +++ b/tests/cases/REST/NextCloudNews/TestV1_2.php @@ -928,4 +928,15 @@ class TestV1_2 extends \JKingWeb\Arsse\Test\AbstractTest { $this->assertMessage($exp, $this->req("GET", "/cleanup/after-update")); Phake::verify(Arsse::$db)->articleCleanup(); } + + public function testQueryTheUserStatus() { + $act = $this->req("GET", "/user"); + $exp = new Response([ + 'userId' => Arsse::$user->id, + 'displayName' => Arsse::$user->id, + 'lastLoginTimestamp' => $this->approximateTime($act->getPayload()['lastLoginTimestamp'], new \DateTimeImmutable), + 'avatar' => null, + ]); + $this->assertMessage($exp, $act); + } }