From be9ebf9ca1a2b70831b330577ee797fc766f7140 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Wed, 22 Feb 2017 23:22:45 -0500 Subject: [PATCH] Remove authz from User driver; moved to main class --- lib/User.php | 50 ++++++++++++++++++++------- lib/User/Driver.php | 4 +-- lib/User/DriverInternal.php | 1 - lib/User/InternalFunctions.php | 29 ---------------- locale/en.php | 5 +-- tests/lib/User/DriverInternalMock.php | 5 --- 6 files changed, 42 insertions(+), 52 deletions(-) diff --git a/lib/User.php b/lib/User.php index 36e332e5..d0d13514 100644 --- a/lib/User.php +++ b/lib/User.php @@ -36,6 +36,44 @@ class User { return (string) $this->id; } + // checks whether the logged in user is authorized to act for the affected user (used especially when granting rights) + 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->authz) 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==$this->data->user->id && $action != "userRightsSet") return true; + // get properties of actor if not already available + if(!sizeof($this->actor)) $this->actor = $this->propertiesGet($this->data->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->data->conf->userComposeNames && $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") { + // managers can only set their own rights, and only lower + if(($rights==User\Driver::RIGHTS_DOMAIN_MANAGER || $rights==User\Driver::RIGHTS_GLOBAL_MANAGER)) { + if($affectedUser != $this->data->user->id || $newRightsLevel != User\Driver::RIGHTS_NONE) return false; + } + // setting rights above your own is not allowed + if($newRightsLevel > $rights) return false; + } + $affectedRights = $this->rightsGet($affectedUser); + // acting for users with rights greater than your own (or equal, for managers) is not allowed + if($affectedRights > $rights || ($rights != User\Driver::RIGHTS_DOMAIN_ADMIN && $affectedRights==$rights)) return false; + return true; + } + public function credentials(): array { if($this->data->conf->userAuthPreferHTTP) { return $this->credentialsHTTP(); @@ -119,18 +157,6 @@ class User { } } - public function authorize(string $affectedUser, string $action, int $promoteLevel = 0): bool { - // if authorization checks are disabled (either because we're running the installer or the background updater) just return true - if(!$this->authz) return true; - // if we don't have a logged-in user, fetch credentials - if($this->id===null) $this->credentials(); - // if the driver implements authorization, return the result - if($this->authzSupported) return $this->u->authorize($affectedUser, $action, $promoteLevel); - // if the driver does not implement authorization, only allow operation for the logged-in user (this means no new users can be added) - if($affectedUser==$this->id && $action != "userRightsSet") return true; - return false; - } - public function authorizationEnabled(bool $setting = null): bool { if($setting===null) return $this->authz; $this->authz = $setting; diff --git a/lib/User/Driver.php b/lib/User/Driver.php index 3bfc93e4..36b02352 100644 --- a/lib/User/Driver.php +++ b/lib/User/Driver.php @@ -10,7 +10,7 @@ Interface Driver { 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 user below themselves; can elevate users 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. Implemented as a static method for consistency with database classes @@ -21,8 +21,6 @@ Interface Driver { function driverFunctions(string $function = null); // authenticates a user against their name and password function auth(string $user, string $password): bool; - // checks whether the logged in user is authorized to act for the affected user (used especially when granting rights) - function authorize(string $affectedUser, string $action): bool; // checks whether a user exists function userExists(string $user): bool; // adds a user diff --git a/lib/User/DriverInternal.php b/lib/User/DriverInternal.php index da81947f..f5e50f61 100644 --- a/lib/User/DriverInternal.php +++ b/lib/User/DriverInternal.php @@ -10,7 +10,6 @@ final class DriverInternal implements Driver { protected $db; protected $functions = [ "auth" => Driver::FUNC_INTERNAL, - "authorize" => Driver::FUNC_INTERNAL, "userList" => Driver::FUNC_INTERNAL, "userExists" => Driver::FUNC_INTERNAL, "userAdd" => Driver::FUNC_INTERNAL, diff --git a/lib/User/InternalFunctions.php b/lib/User/InternalFunctions.php index 194227c9..5db1e6bf 100644 --- a/lib/User/InternalFunctions.php +++ b/lib/User/InternalFunctions.php @@ -17,35 +17,6 @@ trait InternalFunctions { return password_verify($password, $hash); } - function authorize(string $affectedUser, string $action, int $newRightsLevel = 0): bool { - // if the affected user is the actor and the actor is not trying to grant themselves rights, accept the request - if($affectedUser==$this->data->user->id && $action != "userRightsSet") return true; - // get properties of actor if not already available - if(!sizeof($this->actor)) $this->actor = $this->data->user->propertiesGet($this->data->user->id); - $rights =& $this->actor["rights"]; - // if actor is a global admin, accept the request - if($rights==self::RIGHTS_GLOBAL_ADMIN) return true; - // if actor is a common user, deny the request - if($rights==self::RIGHTS_NONE) return false; - // if actor is not some other sort of admin, deny the request - if(!in_array($rights,[self::RIGHTS_GLOBAL_MANAGER,self::RIGHTS_DOMAIN_MANAGER,self::RIGHTS_DOMAIN_ADMIN],true)) return false; - // if actor is a domain admin/manager and domains don't match, deny the request - if($this->data->conf->userComposeNames && $this->actor["domain"] && $rights != self::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 (or equal to your own, for managers) is not allowed - if($newRightsLevel > $rights || ($rights != self::RIGHTS_DOMAIN_ADMIN && $newRightsLevel==$rights)) return false; - } - $affectedRights = $this->data->user->rightsGet($affectedUser); - // acting for users with rights greater than your own (or equal, for managers) is not allowed - if($affectedRights > $rights || ($rights != self::RIGHTS_DOMAIN_ADMIN && $affectedRights==$rights)) return false; - return true; - } - function userExists(string $user): bool { return $this->db->userExists($user); } diff --git a/locale/en.php b/locale/en.php index c6010de7..80d22259 100644 --- a/locale/en.php +++ b/locale/en.php @@ -1,9 +1,10 @@ 'Internal', - + + // this should only be encountered in testing (because tests should cover all exceptions!) 'Exception.JKingWeb/NewsSync/Exception.uncoded' => 'The specified exception symbol {0} has no code specified in Exception.php', - //this should not usually be encountered + // this should not usually be encountered 'Exception.JKingWeb/NewsSync/Exception.unknown' => 'An unknown error has occurred', 'Exception.JKingWeb/NewsSync/Lang/Exception.defaultFileMissing' => 'Default language file "{0}" missing', diff --git a/tests/lib/User/DriverInternalMock.php b/tests/lib/User/DriverInternalMock.php index d8d182ba..f1789774 100644 --- a/tests/lib/User/DriverInternalMock.php +++ b/tests/lib/User/DriverInternalMock.php @@ -13,7 +13,6 @@ class DriverInternalMock implements Driver { protected $data; protected $functions = [ "auth" => Driver::FUNC_INTERNAL, - "authorize" => Driver::FUNC_INTERNAL, "userList" => Driver::FUNC_INTERNAL, "userExists" => Driver::FUNC_INTERNAL, "userAdd" => Driver::FUNC_INTERNAL, @@ -52,10 +51,6 @@ class DriverInternalMock implements Driver { return false; } - function authorize(string $affectedUser, string $action, int $newRightsLevel = 0): bool { - return false; - } - function userExists(string $user): bool { if(!$this->data->user->authorize($user, __FUNCTION__)) throw new ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); return array_key_exists($user, $this->db);