From 65c427e4eeac82462b0e78395a581fbcfdea683d Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Thu, 23 Oct 2025 09:48:55 -0700 Subject: [PATCH] Ensure :actor, :actor_id and :action are all available to permissions SQL, closes #2520 - Updated build_rules_union() to accept actor as dict and provide :actor (JSON) and :actor_id - Updated resolve_permissions_from_catalog() and resolve_permissions_with_candidates() to accept actor dict - :actor is now the full actor dict as JSON (use json_extract() to access fields) - :actor_id is the actor's id field for simple comparisons - :action continues to be available as before - Updated all call sites and tests to use new parameter format - Added test demonstrating all three parameters working together --- datasette/utils/permissions.py | 16 +++-- datasette/views/special.py | 3 +- tests/test_utils_permissions.py | 112 +++++++++++++++++++++++++++----- 3 files changed, 105 insertions(+), 26 deletions(-) diff --git a/datasette/utils/permissions.py b/datasette/utils/permissions.py index 43752b68..1803181b 100644 --- a/datasette/utils/permissions.py +++ b/datasette/utils/permissions.py @@ -1,6 +1,7 @@ # perm_utils.py from __future__ import annotations +import json from typing import Any, Callable, Dict, Iterable, List, Optional, Sequence, Tuple, Union import sqlite3 @@ -36,17 +37,19 @@ PluginOrFactory = Union[PermissionSQL, PluginProvider] def build_rules_union( - actor: str, plugins: Sequence[PermissionSQL] + actor: Optional[dict], plugins: Sequence[PermissionSQL] ) -> Tuple[str, Dict[str, Any]]: """ Compose plugin SQL into a UNION ALL with namespaced parameters. Returns: union_sql: a SELECT with columns (parent, child, allow, reason, source_plugin) - params: dict of bound parameters including :actor and namespaced plugin params + params: dict of bound parameters including :actor (JSON), :actor_id, and namespaced plugin params """ parts: List[str] = [] - params: Dict[str, Any] = {"actor": actor} + actor_json = json.dumps(actor) if actor else None + actor_id = actor.get("id") if actor else None + params: Dict[str, Any] = {"actor": actor_json, "actor_id": actor_id} for i, p in enumerate(plugins): rewrite, ns_params = _namespace_params(i, p.params) @@ -77,7 +80,7 @@ def build_rules_union( async def resolve_permissions_from_catalog( db, - actor: str, + actor: Optional[dict], plugins: Sequence[PluginOrFactory], action: str, candidate_sql: str, @@ -95,6 +98,7 @@ async def resolve_permissions_from_catalog( where rows is an iterable of sqlite3.Row - plugins are either PermissionSQL objects or callables accepting (action: str) and returning PermissionSQL instances selecting (parent, child, allow, reason) + - actor is the actor dict (or None), made available as :actor (JSON), :actor_id, and :action Decision policy: 1) Specificity first: child (depth=2) > parent (depth=1) > root (depth=0) @@ -121,7 +125,6 @@ async def resolve_permissions_from_catalog( all_params = { **(candidate_params or {}), **rule_params, - "actor": actor, "action": action, } @@ -191,7 +194,7 @@ async def resolve_permissions_from_catalog( async def resolve_permissions_with_candidates( db, - actor: str, + actor: Optional[dict], plugins: Sequence[PluginOrFactory], candidates: List[Tuple[str, Optional[str]]], action: str, @@ -203,6 +206,7 @@ async def resolve_permissions_with_candidates( the candidates as a UNION of parameterized SELECTs in a CTE. candidates: list of (parent, child) where child can be None for parent-scoped actions. + actor: actor dict (or None), made available as :actor (JSON), :actor_id, and :action """ # Build a small CTE for candidates. cand_rows_sql: List[str] = [] diff --git a/datasette/views/special.py b/datasette/views/special.py index a4388163..fd80bb5b 100644 --- a/datasette/views/special.py +++ b/datasette/views/special.py @@ -312,10 +312,9 @@ class AllowedResourcesView(BaseView): continue plugins.append(candidate) - actor_id = actor.get("id") if actor else None rows = await resolve_permissions_from_catalog( db, - actor=str(actor_id) if actor_id is not None else "", + actor=actor, plugins=plugins, action=action, candidate_sql=candidate_sql, diff --git a/tests/test_utils_permissions.py b/tests/test_utils_permissions.py index 0693cbc1..6c0bd336 100644 --- a/tests/test_utils_permissions.py +++ b/tests/test_utils_permissions.py @@ -32,7 +32,7 @@ def plugin_allow_all_for_user(user: str) -> PluginProvider: """ SELECT NULL AS parent, NULL AS child, 1 AS allow, 'global allow for ' || :user || ' on ' || :action AS reason - WHERE :actor = :user + WHERE :actor_id = :user """, {"user": user, "action": action}, ) @@ -47,7 +47,7 @@ def plugin_deny_specific_table(user: str, parent: str, child: str) -> PluginProv """ SELECT :parent AS parent, :child AS child, 0 AS allow, 'deny ' || :parent || '/' || :child || ' for ' || :user || ' on ' || :action AS reason - WHERE :actor = :user + WHERE :actor_id = :user """, {"parent": parent, "child": child, "user": user, "action": action}, ) @@ -76,7 +76,7 @@ def plugin_allow_parent_for_user(user: str, parent: str) -> PluginProvider: """ SELECT :parent AS parent, NULL AS child, 1 AS allow, 'allow full parent for ' || :user || ' on ' || :action AS reason - WHERE :actor = :user + WHERE :actor_id = :user """, {"parent": parent, "user": user, "action": action}, ) @@ -91,7 +91,7 @@ def plugin_child_allow_for_user(user: str, parent: str, child: str) -> PluginPro """ SELECT :parent AS parent, :child AS child, 1 AS allow, 'allow child for ' || :user || ' on ' || :action AS reason - WHERE :actor = :user + WHERE :actor_id = :user """, {"parent": parent, "child": child, "user": user, "action": action}, ) @@ -121,7 +121,7 @@ def plugin_conflicting_same_child_rules( """ SELECT :parent AS parent, :child AS child, 1 AS allow, 'team grant at child for ' || :user || ' on ' || :action AS reason - WHERE :actor = :user + WHERE :actor_id = :user """, {"parent": parent, "child": child, "user": user, "action": action}, ) @@ -132,7 +132,7 @@ def plugin_conflicting_same_child_rules( """ SELECT :parent AS parent, :child AS child, 0 AS allow, 'exception deny at child for ' || :user || ' on ' || :action AS reason - WHERE :actor = :user + WHERE :actor_id = :user """, {"parent": parent, "child": child, "user": user, "action": action}, ) @@ -153,7 +153,7 @@ def plugin_allow_all_for_action(user: str, allowed_action: str) -> PluginProvide """ SELECT NULL AS parent, NULL AS child, 1 AS allow, 'global allow for ' || :user || ' on ' || :action AS reason - WHERE :actor = :user + WHERE :actor_id = :user """, {"user": user, "action": action}, ) @@ -247,7 +247,12 @@ async def test_alice_global_allow_with_specific_denies_catalog(db): plugin_org_policy_deny_parent("hr"), ] rows = await resolve_permissions_from_catalog( - db, "alice", plugins, VIEW_TABLE, TABLE_CANDIDATES_SQL, implicit_deny=True + db, + {"id": "alice"}, + plugins, + VIEW_TABLE, + TABLE_CANDIDATES_SQL, + implicit_deny=True, ) # Alice can see everything except accounting/sales and hr/* assert "/accounting/sales" in res_denied(rows) @@ -269,7 +274,12 @@ async def test_carol_parent_allow_but_child_conflict_deny_wins_catalog(db): *plugin_conflicting_same_child_rules("carol", "analytics", "secret"), ] rows = await resolve_permissions_from_catalog( - db, "carol", plugins, VIEW_TABLE, TABLE_CANDIDATES_SQL, implicit_deny=True + db, + {"id": "carol"}, + plugins, + VIEW_TABLE, + TABLE_CANDIDATES_SQL, + implicit_deny=True, ) allowed_analytics = res_allowed(rows, parent="analytics") denied_analytics = res_denied(rows, parent="analytics") @@ -290,7 +300,12 @@ async def test_specificity_child_allow_overrides_parent_deny_catalog(db): ), # child allow beats parent deny ] rows = await resolve_permissions_from_catalog( - db, "alice", plugins, VIEW_TABLE, TABLE_CANDIDATES_SQL, implicit_deny=True + db, + {"id": "alice"}, + plugins, + VIEW_TABLE, + TABLE_CANDIDATES_SQL, + implicit_deny=True, ) # table02 allowed, other analytics tables denied @@ -311,7 +326,7 @@ async def test_root_deny_all_but_parent_allow_rescues_specific_parent_catalog(db ), # parent allow (more specific) ] rows = await resolve_permissions_from_catalog( - db, "bob", plugins, VIEW_TABLE, TABLE_CANDIDATES_SQL, implicit_deny=True + db, {"id": "bob"}, plugins, VIEW_TABLE, TABLE_CANDIDATES_SQL, implicit_deny=True ) for r in rows: if r["parent"] == "accounting": @@ -328,7 +343,12 @@ async def test_parent_scoped_candidates(db): plugin_allow_parent_for_user("carol", "analytics"), ] rows = await resolve_permissions_from_catalog( - db, "carol", plugins, VIEW_TABLE, PARENT_CANDIDATES_SQL, implicit_deny=True + db, + {"id": "carol"}, + plugins, + VIEW_TABLE, + PARENT_CANDIDATES_SQL, + implicit_deny=True, ) d = {r["resource"]: r["allow"] for r in rows} assert d["/analytics"] == 1 @@ -342,13 +362,23 @@ async def test_implicit_deny_behavior(db): # implicit_deny=True -> everything denied with reason 'implicit deny' rows = await resolve_permissions_from_catalog( - db, "erin", plugins, VIEW_TABLE, TABLE_CANDIDATES_SQL, implicit_deny=True + db, + {"id": "erin"}, + plugins, + VIEW_TABLE, + TABLE_CANDIDATES_SQL, + implicit_deny=True, ) assert all(r["allow"] == 0 and r["reason"] == "implicit deny" for r in rows) # implicit_deny=False -> no winner => allow is None, reason is None rows2 = await resolve_permissions_from_catalog( - db, "erin", plugins, VIEW_TABLE, TABLE_CANDIDATES_SQL, implicit_deny=False + db, + {"id": "erin"}, + plugins, + VIEW_TABLE, + TABLE_CANDIDATES_SQL, + implicit_deny=False, ) assert all(r["allow"] is None and r["reason"] is None for r in rows2) @@ -384,7 +414,7 @@ async def test_candidate_filters_via_params(db): # Case 1: exclude memory dbs, require schema_version >= 2 -> only analytics appear, and thus are allowed rows = await resolve_permissions_from_catalog( db, - "dev", + {"id": "dev"}, plugins, VIEW_TABLE, candidate_sql, @@ -398,7 +428,7 @@ async def test_candidate_filters_via_params(db): # but root deny wins except where specifically allowed (none except analytics parent allow doesn’t apply to table depth if candidate includes children; still fine—policy is explicit). rows2 = await resolve_permissions_from_catalog( db, - "dev", + {"id": "dev"}, plugins, VIEW_TABLE, candidate_sql, @@ -418,7 +448,7 @@ async def test_action_specific_rules(db): view_rows = await resolve_permissions_from_catalog( db, - "dana", + {"id": "dana"}, plugins, VIEW_TABLE, TABLE_CANDIDATES_SQL, @@ -429,7 +459,7 @@ async def test_action_specific_rules(db): insert_rows = await resolve_permissions_from_catalog( db, - "dana", + {"id": "dana"}, plugins, "insert-row", TABLE_CANDIDATES_SQL, @@ -438,3 +468,49 @@ async def test_action_specific_rules(db): assert insert_rows and all(r["allow"] == 0 for r in insert_rows) assert all(r["reason"] == "implicit deny" for r in insert_rows) assert all(r["action"] == "insert-row" for r in insert_rows) + + +@pytest.mark.asyncio +async def test_actor_actor_id_action_parameters_available(db): + """Test that :actor (JSON), :actor_id, and :action are all available in SQL""" + await seed_catalog(db) + + def plugin_using_all_parameters() -> PluginProvider: + def provider(action: str) -> PermissionSQL: + return PermissionSQL( + "test_all_params", + """ + SELECT NULL AS parent, NULL AS child, 1 AS allow, + 'Actor ID: ' || COALESCE(:actor_id, 'null') || + ', Actor JSON: ' || COALESCE(:actor, 'null') || + ', Action: ' || :action AS reason + WHERE :actor_id = 'test_user' AND :action = 'view-table' + AND json_extract(:actor, '$.role') = 'admin' + """, + {}, + ) + + return provider + + plugins = [plugin_using_all_parameters()] + + # Test with full actor dict + rows = await resolve_permissions_from_catalog( + db, + {"id": "test_user", "role": "admin"}, + plugins, + "view-table", + TABLE_CANDIDATES_SQL, + implicit_deny=True, + ) + + # Should have allowed rows with reason containing all the info + allowed = [r for r in rows if r["allow"] == 1] + assert len(allowed) > 0 + + # Check that the reason string contains evidence of all parameters + reason = allowed[0]["reason"] + assert "test_user" in reason + assert "view-table" in reason + # The :actor parameter should be the JSON string + assert "Actor JSON:" in reason