Removed most calls to userExists()

- functions not related to user management now have the existence of the affected user checked in the authorizer, when the affected user differs from the actor - User::authorizationEnabled() now nests: disabling twice and then enabling once leaves the authorizer disabled - Disabling of the authorizer is now tested - User tests now use a partial mock instead of relying on User::authorizationEnabled() - Added authorizer tests against a missing user - Removed folder tests related to missing users - Also added more subscription tests
2024-12-22 13:12:41 +00:00 · 2017-05-11 18:00:35 -04:00 · 2017-05-11 18:00:35 -04:00 · 98c950de58
commit 98c950de58
parent a580579627
6 changed files with 56 additions and 54 deletions
--- a/lib/Database.php
+++ b/lib/Database.php
@ -278,10 +278,6 @@ class Database {
        if(!Data::$user->authorize($user, __FUNCTION__)) {
            throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]);
        }
-        // If the user doesn't exist throw an exception.
-        if(!$this->userExists($user)) {
-            throw new User\Exception("doesNotExist", ["user" => $user, "action" => __FUNCTION__]);
-        }
        // if the desired folder name is missing or invalid, throw an exception
        if(!array_key_exists("name", $data) || $data['name']=="") {
            throw new Db\ExceptionInput("missing", ["action" => __FUNCTION__, "field" => "name"]);
@ -312,10 +308,6 @@ class Database {
        if(!Data::$user->authorize($user, __FUNCTION__)) {
            throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]);
        }
-        // if the user doesn't exist throw an exception.
-        if(!$this->userExists($user)) {
-            throw new User\Exception("doesNotExist", ["user" => $user, "action" => __FUNCTION__]);
-        }
        // check to make sure the parent exists, if one is specified
        if(!is_null($parent)) {
            if(!$this->db->prepare("SELECT count(*) from arsse_folders where owner is ? and id is ?", "str", "int")->run($user, $parent)->getValue()) {
@ -335,7 +327,6 @@ class Database {

    public function folderRemove(string $user, int $id): bool {
        if(!Data::$user->authorize($user, __FUNCTION__)) throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]);
-        if(!$this->userExists($user)) throw new User\Exception("doesNotExist", ["user" => $user, "action" => __FUNCTION__]);
        $changes = $this->db->prepare("DELETE FROM arsse_folders where owner is ? and id is ?", "str", "int")->run($user, $id)->changes();
        if(!$changes) throw new Db\ExceptionInput("idMissing", ["action" => __FUNCTION__, "field" => "folder", 'id' => $id]);
        return true;
@ -343,7 +334,6 @@ class Database {

    public function folderPropertiesGet(string $user, int $id): array {
        if(!Data::$user->authorize($user, __FUNCTION__)) throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]);
-        if(!$this->userExists($user)) throw new User\Exception("doesNotExist", ["user" => $user, "action" => __FUNCTION__]);
        $props = $this->db->prepare("SELECT id,name,parent from arsse_folders where owner is ? and id is ?", "str", "int")->run($user, $id)->getRow();
        if(!$props) throw new Db\ExceptionInput("idMissing", ["action" => __FUNCTION__, "field" => "folder", 'id' => $id]);
        return $props;
@ -351,7 +341,6 @@ class Database {

    public function folderPropertiesSet(string $user, int $id, array $data): bool {
        if(!Data::$user->authorize($user, __FUNCTION__)) throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]);
-        if(!$this->userExists($user)) throw new User\Exception("doesNotExist", ["user" => $user, "action" => __FUNCTION__]);
        // layer the existing folder properties onto the new desired ones; this also has the effect of checking whether the folder is valid
        $data = array_merge($this->folderPropertiesGet($user, $id), $data);
        // if the desired folder name is missing or invalid, throw an exception
@ -394,7 +383,6 @@ class Database {

    public function subscriptionAdd(string $user, string $url, string $fetchUser = "", string $fetchPassword = ""): int {
        if(!Data::$user->authorize($user, __FUNCTION__)) throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]);
-        if(!$this->userExists($user)) throw new User\Exception("doesNotExist", ["user" => $user, "action" => __FUNCTION__]);
        // check to see if the feed exists
        $feedID = $this->db->prepare("SELECT id from arsse_feeds where url is ? and username is ? and password is ?", "str", "str", "str")->run($url, $fetchUser, $fetchPassword)->getValue();
        if(is_null($feedID)) {
@ -415,7 +403,6 @@ class Database {

    public function subscriptionList(string $user, int $folder = null, int $id = null): Db\Result {
        if(!Data::$user->authorize($user, __FUNCTION__)) throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]);
-        if(!$this->userExists($user)) throw new User\Exception("doesNotExist", ["user" => $user, "action" => __FUNCTION__]);
        // check to make sure the folder exists, if one is specified
        $query = 
            "SELECT 
@ -442,7 +429,9 @@ class Database {

    public function subscriptionRemove(string $user, int $id): bool {
        if(!Data::$user->authorize($user, __FUNCTION__)) throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]);
-        return (bool) $this->db->prepare("DELETE from arsse_subscriptions where owner is ? and id is ?", "str", "int")->run($user, $id)->changes();
+        $changes = $this->db->prepare("DELETE from arsse_subscriptions where owner is ? and id is ?", "str", "int")->run($user, $id)->changes();
+        if(!$changes) throw new Db\ExceptionInput("idMissing", ["action" => __FUNCTION__, "field" => "folder", 'id' => $id]);
+        return true;
    }

    public function subscriptionPropertiesGet(string $user, int $id): array {
@ -457,7 +446,6 @@ class Database {

    public function subscriptionPropertiesSet(string $user, int $id, array $data): bool {
        if(!Data::$user->authorize($user, __FUNCTION__)) throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]);
-        if(!$this->userExists($user)) throw new User\Exception("doesNotExist", ["user" => $user, "action" => __FUNCTION__]);
        $tr = $this->db->begin();
        if(!$this->db->prepare("SELECT count(*) from arsse_subscriptions where owner is ? and id is ?", "str", "int")->run($user, $id)->getValue()) {
            // if the ID doesn't exist or doesn't belong to the user, throw an exception
--- a/lib/User.php
+++ b/lib/User.php
@ -6,7 +6,7 @@ class User {
    public  $id = null;

    protected $u;
-    protected $authz = true;
+    protected $authz = 0;
    protected $authzSupported = 0;
    protected $actor = [];

@ -36,14 +36,18 @@ class User {
    // 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(!$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==Data::$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(Data::$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(Data::$user->id);
-        $rights =& $this->actor["rights"];
+        $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
@ -162,9 +166,10 @@ class User {
    }

    public function authorizationEnabled(bool $setting = null): bool {
-        if($setting===null) return $this->authz;
-        $this->authz = $setting;
-        return $setting;
+        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 {
--- a/tests/User/TestAuthorization.php
+++ b/tests/User/TestAuthorization.php
@ -1,6 +1,7 @@
 <?php
 declare(strict_types=1);
 namespace JKingWeb\Arsse;
+use Phake;


 class TestAuthorization extends \PHPUnit\Framework\TestCase {
@ -55,19 +56,28 @@ class TestAuthorization extends \PHPUnit\Framework\TestCase {
        if($db !== null) {
            Data::$db = new $db();
        }
-        Data::$user = new User();
-        Data::$user->authorizationEnabled(false);
+        Data::$user = Phake::PartialMock(User::class);
+        Phake::when(Data::$user)->authorize->thenReturn(true);
        foreach(self::USERS as $user => $level) {
            Data::$user->add($user, "");
            Data::$user->rightsSet($user, $level);
        }
-        Data::$user->authorizationEnabled(true);
+        Phake::reset(Data::$user);
    }

    function tearDown() {
        $this->clearData();
    }

+    function testToggleLogic() {
+        $this->assertTrue(Data::$user->authorizationEnabled());
+        $this->assertTrue(Data::$user->authorizationEnabled(true));
+        $this->assertFalse(Data::$user->authorizationEnabled(false));
+        $this->assertFalse(Data::$user->authorizationEnabled(false));
+        $this->assertFalse(Data::$user->authorizationEnabled(true));
+        $this->assertTrue(Data::$user->authorizationEnabled(true));
+    }
+    
    function testSelfActionLogic() {
        foreach(array_keys(self::USERS) as $user) {
            Data::$user->auth($user, "");
@ -298,4 +308,11 @@ class TestAuthorization extends \PHPUnit\Framework\TestCase {
        }
        return $err;
    }
+
+    function testMissingUserLogic() {
+        Data::$user->auth("gadm@example.com", "");
+        $this->assertTrue(Data::$user->authorize("user@example.com", "someFunction"));
+        $this->assertException("doesNotExist", "User");
+        Data::$user->authorize("this_user_does_not_exist@example.org", "someFunction");
+    }
 }
--- a/tests/lib/Database/SeriesFolder.php
+++ b/tests/lib/Database/SeriesFolder.php
@ -45,11 +45,6 @@ trait SeriesFolder {
        Data::$db->folderAdd("john.doe@example.com", ['name' => "Sociology", 'parent' => 4]); // folder ID 4 belongs to Jane
    }

-    function testAddAFolderForAMissingUser() {
-        $this->assertException("doesNotExist", "User");
-        Data::$db->folderAdd("john.doe@example.org", ['name' => "Sociology"]);
-    }
-
    function testAddAFolderWithAMissingName() {
        $this->assertException("missing", "Db", "ExceptionInput");
        Data::$db->folderAdd("john.doe@example.com", []);
@ -119,11 +114,6 @@ trait SeriesFolder {
        Data::$db->folderList("john.doe@example.com", 4); // folder ID 4 belongs to Jane
    }

-    function testListFoldersForAMissingUser() {
-        $this->assertException("doesNotExist", "User");
-        Data::$db->folderList("john.doe@example.org");
-    }
-
    function testListFoldersWithoutAuthority() {
        Phake::when(Data::$user)->authorize->thenReturn(false);
        $this->assertException("notAuthorized", "User", "ExceptionAuthz");
@ -158,11 +148,6 @@ trait SeriesFolder {
        Data::$db->folderRemove("john.doe@example.com", 4); // folder ID 4 belongs to Jane
    }

-    function testRemoveAFolderForAMissingUser() {
-        $this->assertException("doesNotExist", "User");
-        Data::$db->folderRemove("john.doe@example.org", 1);
-    }
-
    function testRemoveAFolderWithoutAuthority() {
        Phake::when(Data::$user)->authorize->thenReturn(false);
        $this->assertException("notAuthorized", "User", "ExceptionAuthz");
@ -189,11 +174,6 @@ trait SeriesFolder {
        Data::$db->folderPropertiesGet("john.doe@example.com", 4); // folder ID 4 belongs to Jane
    }

-    function testGetThePropertiesOfAFolderForAMissingUser() {
-        $this->assertException("doesNotExist", "User");
-        Data::$db->folderPropertiesGet("john.doe@example.org", 1);
-    }
-
    function testGetThePropertiesOfAFolderWithoutAuthority() {
        Phake::when(Data::$user)->authorize->thenReturn(false);
        $this->assertException("notAuthorized", "User", "ExceptionAuthz");
@ -246,11 +226,6 @@ trait SeriesFolder {
        Data::$db->folderPropertiesSet("john.doe@example.com", 4, ['parent' => null]); // folder ID 4 belongs to Jane
    }

-    function testSetThePropertiesOfAFolderForAMissingUser() {
-        $this->assertException("doesNotExist", "User");
-        Data::$db->folderPropertiesSet("john.doe@example.org", 1, ['parent' => null]);
-    }
-
    function testSetThePropertiesOfAFolderWithoutAuthority() {
        Phake::when(Data::$user)->authorize->thenReturn(false);
        $this->assertException("notAuthorized", "User", "ExceptionAuthz");
--- a/tests/lib/Database/SeriesSubscription.php
+++ b/tests/lib/Database/SeriesSubscription.php
@ -20,6 +20,7 @@ trait SeriesSubscription {
                ],
                'rows' => [
                    [1,"http://example.com/feed1", "Ook", "", ""],
+                    [2,"http://example.com/feed2", "Eek", "", ""],
                ]
            ],
            'arsse_subscriptions' => [
@ -27,13 +28,14 @@ trait SeriesSubscription {
                    'id'    => "int",
                    'owner' => "str",
                    'feed'  => "int",
+                    'title' => "str",
                ],
                'rows' => [
-
+                    [1,"john.doe@example.com",2,null],
                ]
            ]
        ];
-        // merge folder table with base user table
+        // merge tables
        $this->data = array_merge($this->data, $data);
        $this->primeDatabase($this->data);
        // initialize a partial mock of the Database object to later manipulate the feedUpdate method
@ -78,7 +80,6 @@ trait SeriesSubscription {
        $user = "john.doe@example.com";
        $url = "http://example.org/feed1";
        $feedID = $this->nextID("arsse_feeds");
-        $subID = $this->nextID("arsse_subscriptions");
        Phake::when(Data::$db)->feedUpdate->thenThrow(new FeedException($url, new \PicoFeed\Client\InvalidUrlException()));
        try {
            Data::$db->subscriptionAdd($user, $url);
@ -94,4 +95,19 @@ trait SeriesSubscription {
            throw $e;
        }
    }
+
+    function testAddADuplicateSubscription() {
+        $user = "john.doe@example.com";
+        $url = "http://example.com/feed2";
+        $this->assertException("constraintViolation", "Db", "ExceptionInput");
+        Data::$db->subscriptionAdd($user, $url);
+    }
+
+    function testAddAFeedWithoutAuthority() {
+        $user = "john.doe@example.com";
+        $url = "http://example.com/feed1";
+        Phake::when(Data::$user)->authorize->thenReturn(false);
+        $this->assertException("notAuthorized", "User", "ExceptionAuthz");
+        Data::$db->subscriptionAdd($user, $url);
+    }
 }
--- a/tests/lib/User/CommonTests.php
+++ b/tests/lib/User/CommonTests.php
@ -5,6 +5,7 @@ use JKingWeb\Arsse\Data;
 use JKingWeb\Arsse\Conf;
 use JKingWeb\Arsse\User;
 use JKingWeb\Arsse\User\Driver;
+use Phake;

 trait CommonTests {

@ -15,8 +16,8 @@ trait CommonTests {
        $conf->userAuthPreferHTTP = true;
        Data::$conf = $conf;
        Data::$db = new Database();
-        Data::$user = new User();
-        Data::$user->authorizationEnabled(false);
+        Data::$user = Phake::PartialMock(User::class);
+        Phake::when(Data::$user)->authorize->thenReturn(true);
        $_SERVER['PHP_AUTH_USER'] = self::USER1;
        $_SERVER['PHP_AUTH_PW'] = "secret";
        // call the additional setup method if it exists