From a528555e8491321ced5471540a84ec5b28a9cf83 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Sat, 1 Nov 2025 18:38:29 -0700 Subject: [PATCH] Additional actor restriction should not grant access to additional actions (#2569) Closes #2568 --- datasette/default_permissions.py | 69 +++++++++++++++++++++++++++----- docs/authentication.rst | 6 +++ tests/test_permissions.py | 60 ++++++++++++++++++++++----- 3 files changed, 116 insertions(+), 19 deletions(-) diff --git a/datasette/default_permissions.py b/datasette/default_permissions.py index 0f64cbc5..1fc85ebf 100644 --- a/datasette/default_permissions.py +++ b/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}") diff --git a/docs/authentication.rst b/docs/authentication.rst index 28fb76bb..e69b0aa4 100644 --- a/docs/authentication.rst +++ b/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: diff --git a/tests/test_permissions.py b/tests/test_permissions.py index c5f547ea..6def3840 100644 --- a/tests/test_permissions.py +++ b/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"""