Additional actor restriction should not grant access to additional actions (#2569)

Closes #2568

datasette/default_permissions.py

@@ -88,17 +88,33 @@ async def _config_permission_rules(datasette, actor, action) -> list[PermissionS
     has_restrictions = actor_dict and "_r" in actor_dict if actor_dict else False
     restrictions = actor_dict.get("_r", {}) if actor_dict else {}
 
+    action_obj = datasette.actions.get(action)
+    action_checks = {action}
+    if action_obj and action_obj.abbr:
+        action_checks.add(action_obj.abbr)
+
+    restricted_databases: set[str] = set()
+    restricted_tables: set[tuple[str, str]] = set()
+    if has_restrictions:
+        restricted_databases = {
+            db_name
+            for db_name, db_actions in (restrictions.get("d") or {}).items()
+            if action_checks.intersection(db_actions)
+        }
+        restricted_tables = {
+            (db_name, table_name)
+            for db_name, tables in (restrictions.get("r") or {}).items()
+            for table_name, table_actions in tables.items()
+            if action_checks.intersection(table_actions)
+        }
+        # Tables implicitly reference their parent databases
+        restricted_databases.update(db for db, _ in restricted_tables)
+
     def is_in_restriction_allowlist(parent, child, action):
         """Check if a resource is in the actor's restriction allowlist for this action"""
         if not has_restrictions:
             return True  # No restrictions, all resources allowed
 
-        # Check action with abbreviations
-        action_obj = datasette.actions.get(action)
-        action_checks = {action}
-        if action_obj and action_obj.abbr:
-            action_checks.add(action_obj.abbr)
-
         # Check global allowlist
         if action_checks.intersection(restrictions.get("a", [])):
             return True
@@ -110,9 +126,25 @@ async def _config_permission_rules(datasette, actor, action) -> list[PermissionS
             return True
 
         # Check table-level allowlist
-        if parent and child:
-            table_actions = restrictions.get("r", {}).get(parent, {}).get(child, [])
-            if action_checks.intersection(table_actions):
+        if parent:
+            table_restrictions = (restrictions.get("r", {}) or {}).get(parent, {})
+            if child:
+                table_actions = table_restrictions.get(child, [])
+                if action_checks.intersection(table_actions):
+                    return True
+            else:
+                # Parent query should proceed if any child in this database is allowlisted
+                for table_actions in table_restrictions.values():
+                    if action_checks.intersection(table_actions):
+                        return True
+
+        # Parent/child both None: include if any restrictions exist for this action
+        if parent is None and child is None:
+            if action_checks.intersection(restrictions.get("a", [])):
+                return True
+            if restricted_databases:
+                return True
+            if restricted_tables:
                 return True
 
         return False
@@ -142,15 +174,32 @@ async def _config_permission_rules(datasette, actor, action) -> list[PermissionS
             return
 
         result = evaluate(allow_block)
+        bool_result = bool(result)
         # If result is None (no match) or False, treat as deny
         rows.append(
             (
                 parent,
                 child,
-                bool(result),  # None becomes False, False stays False, True stays True
+                bool_result,  # None becomes False, False stays False, True stays True
                 f"config {'allow' if result else 'deny'} {scope}",
             )
         )
+        if has_restrictions and not bool_result and child is None:
+            reason = f"config deny {scope} (restriction gate)"
+            if parent is None:
+                # Root-level deny: add more specific denies for restricted resources
+                if action_obj and action_obj.takes_parent:
+                    for db_name in restricted_databases:
+                        rows.append((db_name, None, 0, reason))
+                if action_obj and action_obj.takes_child:
+                    for db_name, table_name in restricted_tables:
+                        rows.append((db_name, table_name, 0, reason))
+            else:
+                # Database-level deny: add child-level denies for restricted tables
+                if action_obj and action_obj.takes_child:
+                    for db_name, table_name in restricted_tables:
+                        if db_name == parent:
+                            rows.append((db_name, table_name, 0, reason))
 
     root_perm = (config.get("permissions") or {}).get(action)
     add_row(None, None, evaluate(root_perm), f"permissions for {action}")

docs/authentication.rst

@@ -1033,6 +1033,12 @@ This example outputs the following::
       }
     }
 
+Restrictions act as an allowlist layered on top of the actor's existing
+permissions. They can only remove access the actor would otherwise have—they
+cannot grant new access. If the underlying actor is denied by ``allow`` rules in
+``datasette.yaml`` or by a plugin, a token that lists that resource in its
+``"_r"`` section will still be denied.
+
 
 .. _permissions_plugins:
 

tests/test_permissions.py

@@ -1117,16 +1117,29 @@ async def test_api_explorer_visibility(
 
 
 @pytest.mark.asyncio
-async def test_view_table_token_can_access_table(perms_ds):
-    actor = {
-        "id": "restricted-token",
-        "token": "dstok",
-        # Restricted to just view-table on perms_ds_two/t1
-        "_r": {"r": {"perms_ds_two": {"t1": ["vt"]}}},
+async def test_view_table_token_cannot_gain_access_without_base_permission(perms_ds):
+    # Only allow a different actor to view this table
+    previous_config = perms_ds.config
+    perms_ds.config = {
+        "databases": {
+            "perms_ds_two": {
+                # Only someone-else can see anything in this database
+                "allow": {"id": "someone-else"},
+            }
+        }
     }
-    cookies = {"ds_actor": perms_ds.client.actor_cookie(actor)}
-    response = await perms_ds.client.get("/perms_ds_two/t1.json", cookies=cookies)
-    assert response.status_code == 200
+    try:
+        actor = {
+            "id": "restricted-token",
+            "token": "dstok",
+            # Restricted token claims access to perms_ds_two/t1 only
+            "_r": {"r": {"perms_ds_two": {"t1": ["vt"]}}},
+        }
+        cookies = {"ds_actor": perms_ds.client.actor_cookie(actor)}
+        response = await perms_ds.client.get("/perms_ds_two/t1.json", cookies=cookies)
+        assert response.status_code == 403
+    finally:
+        perms_ds.config = previous_config
 
 
 @pytest.mark.asyncio
@@ -1337,6 +1350,35 @@ async def test_actor_restrictions_filters_allowed_resources(perms_ds):
     assert len(db_page.resources) == 0
 
 
+@pytest.mark.asyncio
+async def test_actor_restrictions_do_not_expand_allowed_resources(perms_ds):
+    """Restrictions cannot grant access not already allowed to the actor."""
+
+    previous_config = perms_ds.config
+    perms_ds.config = {
+        "databases": {
+            "perms_ds_one": {
+                "allow": {"id": "someone-else"},
+            }
+        }
+    }
+    try:
+        actor = {"id": "user", "_r": {"r": {"perms_ds_one": {"t1": ["vt"]}}}}
+
+        # Base actor is not allowed to see t1, so restrictions should not change that
+        page = await perms_ds.allowed_resources("view-table", actor)
+        assert len(page.resources) == 0
+
+        # And explicit permission checks should still deny
+        response = await perms_ds.client.get(
+            "/perms_ds_one/t1.json",
+            cookies={"ds_actor": perms_ds.client.actor_cookie(actor)},
+        )
+        assert response.status_code == 403
+    finally:
+        perms_ds.config = previous_config
+
+
 @pytest.mark.asyncio
 async def test_actor_restrictions_database_level(perms_ds):
     """Test database-level restrictions allow all tables in database - issue #2534"""