Repository: JosephSilber/bouncer (language: php)

Issue:
Unexpected scoping behavior
My [reproduction repo](https://github.com/grantholle/bouncer-scoping-reproduction).

The issue is essentially if I add a permission with a scope, then remove the scope entirely and check the same permission, it comes back as a successful check because of missing scoping query clauses when the scope is `null`.

As you can see in my [test](https://github.com/grantholle/bouncer-scoping-reproduction/blob/main/tests/Feature/PermissionAssignmentTest.php), I am removing the scope entirely and checking to see if a user has permission on the current scope (none/`null`). The query without a scope does not explicitly check if `scope = null`, and therefore if a scope is currently not set in the application, the permission will pass regardless of what the original permission's scope was.

In my mind how it should work is that, if a permission is added when there is no scope set, then it should pass regardless of what the set scope is, which is the current behavior (`"permissions"."scope" = # or "permissions"."scope" is null`).

However, if I set a permission with a scope, it *should only work if that is the current scope*. I'm proposing we explicitly set the scope where clauses when there isn't a scope set for the application (`"permissions"."scope" is null`).


Discussion / hints:
I recreated the repo based on what I was trying to achieve in my own project, because I didn't know if it was something i was doing. I've added a simpler test case that shows the unexpected behavior.

Fix patch:
diff --git a/src/Database/Scope/Scope.php b/src/Database/Scope/Scope.php
index 63fe0e1d..7e7edd23 100644
--- a/src/Database/Scope/Scope.php
+++ b/src/Database/Scope/Scope.php
@@ -109,7 +109,7 @@ public function applyToModel(Model $model)
      */
     public function applyToModelQuery($query, $table = null)
     {
-        if (is_null($this->scope) || $this->onlyScopeRelations) {
+        if ($this->onlyScopeRelations) {
             return $query;
         }
 
@@ -129,10 +129,6 @@ public function applyToModelQuery($query, $table = null)
      */
     public function applyToRelationQuery($query, $table)
     {
-        if (is_null($this->scope)) {
-            return $query;
-        }
-
         return $this->applyToQuery($query, $table);
     }
 
@@ -166,8 +162,11 @@ public function applyToRelation(BelongsToMany $relation)
     protected function applyToQuery($query, $table)
     {
         return $query->where(function ($query) use ($table) {
-            $query->where("{$table}.scope", $this->scope)
-                  ->orWhereNull("{$table}.scope");
+            $query->whereNull("{$table}.scope");
+
+            if (! is_null($this->scope)) {
+                $query->orWhere("{$table}.scope", $this->scope);
+            }
         });
     }
 


Test patch:
diff --git a/tests/MultiTenancyTest.php b/tests/MultiTenancyTest.php
index b7d74e1f..18de1e7b 100644
--- a/tests/MultiTenancyTest.php
+++ b/tests/MultiTenancyTest.php
@@ -136,6 +136,26 @@ function syncing_abilities_is_properly_scoped($provider)
         $this->assertEquals(1, $user->abilities()->count());
     }
 
+    /**
+     * @test
+     * @dataProvider bouncerProvider
+     */
+    function scoped_abilities_do_not_work_when_unscoped($provider)
+    {
+        list($bouncer, $user) = $provider();
+
+        $bouncer->scope()->to(1);
+        $bouncer->allow($user)->to(['write', 'read']);
+
+        $this->assertTrue($bouncer->can('write'));
+        $this->assertTrue($bouncer->can('read'));
+        $this->assertEquals(2, $user->abilities()->count());
+
+        $bouncer->scope()->to(null);
+        $this->assertFalse($bouncer->can('write'));
+        $this->assertFalse($bouncer->can('read'));
+    }
+
     /**
      * @test
      * @dataProvider bouncerProvider
@@ -326,6 +346,9 @@ function assigning_and_retracting_roles_scopes_them_properly($provider)
 
         $bouncer->scope()->to(2);
         $this->assertFalse($bouncer->is($user)->an('admin'));
+
+        $bouncer->scope()->to(null);
+        $this->assertFalse($bouncer->is($user)->an('admin'));
     }
 
     /**