From a2994cc5bbe229766f9ad3f1ce838c219f0a6b84 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Fri, 24 Oct 2025 11:44:43 -0700 Subject: [PATCH] Remove automatic parameter namespacing from permission plugins Simplifies the permission system by removing automatic parameter namespacing. Plugins are now responsible for using unique parameter names. The recommended convention is to prefix parameters with the plugin source name (e.g., :myplugin_user_id). System reserves :actor, :actor_id, :action, :filter_parent. - Remove _namespace_params() function from datasette/utils/permissions.py - Update build_rules_union() to use plugin params directly - Document parameter naming convention in plugin_hooks.rst - Update example plugins to use prefixed parameters - Add test_multiple_plugins_with_own_parameters() to verify convention works --- datasette/utils/permissions.py | 36 +++++----------- docs/plugin_hooks.rst | 20 +++++---- tests/test_utils_permissions.py | 74 +++++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 34 deletions(-) diff --git a/datasette/utils/permissions.py b/datasette/utils/permissions.py index 169f786c..863b2e70 100644 --- a/datasette/utils/permissions.py +++ b/datasette/utils/permissions.py @@ -13,49 +13,33 @@ from datasette.permissions import PermissionSQL # ----------------------------- -def _namespace_params(i: int, params: Dict[str, Any]) -> Tuple[str, Dict[str, Any]]: - """ - Rewrite parameter placeholders to distinct names per plugin block. - Returns (rewritten_sql, namespaced_params). - """ - - replacements = {key: f"{key}_{i}" for key in params.keys()} - - def rewrite(s: str) -> str: - for key in sorted(replacements.keys(), key=len, reverse=True): - s = s.replace(f":{key}", f":{replacements[key]}") - return s - - namespaced: Dict[str, Any] = {} - for key, value in params.items(): - namespaced[replacements[key]] = value - return rewrite, namespaced - - def build_rules_union( actor: dict | None, plugins: Sequence[PermissionSQL] ) -> Tuple[str, Dict[str, Any]]: """ - Compose plugin SQL into a UNION ALL with namespaced parameters. + Compose plugin SQL into a UNION ALL. Returns: union_sql: a SELECT with columns (parent, child, allow, reason, source_plugin) - params: dict of bound parameters including :actor (JSON), :actor_id, and namespaced plugin params + params: dict of bound parameters including :actor (JSON), :actor_id, and plugin params + + Note: Plugins are responsible for ensuring their parameter names don't conflict. + The system reserves these parameter names: :actor, :actor_id, :action, :filter_parent + Plugin parameters should be prefixed with a unique identifier (e.g., source name). """ parts: List[str] = [] 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) - sql_block = rewrite(p.sql) - params.update(ns_params) + for p in plugins: + # No namespacing - just use plugin params as-is + params.update(p.params) parts.append( f""" SELECT parent, child, allow, reason, '{p.source}' AS source_plugin FROM ( - {sql_block} + {p.sql} ) """.strip() ) diff --git a/docs/plugin_hooks.rst b/docs/plugin_hooks.rst index 5c72c165..979baa84 100644 --- a/docs/plugin_hooks.rst +++ b/docs/plugin_hooks.rst @@ -1461,8 +1461,12 @@ Return value Datasette's action-based permission resolver calls this hook to gather SQL rows describing which resources an actor may access (``allow = 1``) or should be denied (``allow = 0``) for a specific action. -Each SQL snippet should return ``parent``, ``child``, ``allow`` and ``reason`` columns. Any bound parameters -supplied via ``PluginSQL.params`` are automatically namespaced per plugin. +Each SQL snippet should return ``parent``, ``child``, ``allow`` and ``reason`` columns. + +**Parameter naming convention:** Plugin parameters in ``PluginSQL.params`` should use unique names +to avoid conflicts with other plugins. The recommended convention is to prefix parameters with your +plugin's source name (e.g., ``myplugin_user_id``). The system reserves these parameter names: +``:actor``, ``:actor_id``, ``:action``, and ``:filter_parent``. Permission plugin examples @@ -1531,10 +1535,10 @@ will pass through to the SQL snippet. 1 AS allow, 'execute-sql allowed for analytics_*' AS reason FROM catalog_databases - WHERE database_name LIKE :prefix + WHERE database_name LIKE :analytics_prefix """, params={ - "prefix": "analytics_%", + "analytics_prefix": "analytics_%", }, ) @@ -1564,12 +1568,12 @@ with columns ``(actor_id, action, parent, child, allow, reason)``. allow, COALESCE(reason, 'permission_grants table') AS reason FROM permission_grants - WHERE actor_id = :actor_id - AND action = :action + WHERE actor_id = :grants_actor_id + AND action = :grants_action """, params={ - "actor_id": actor.get("id"), - "action": action, + "grants_actor_id": actor.get("id"), + "grants_action": action, }, ) diff --git a/tests/test_utils_permissions.py b/tests/test_utils_permissions.py index 937a7a57..1e809670 100644 --- a/tests/test_utils_permissions.py +++ b/tests/test_utils_permissions.py @@ -519,3 +519,77 @@ async def test_actor_actor_id_action_parameters_available(db): assert "view-table" in reason # The :actor parameter should be the JSON string assert "Actor JSON:" in reason + + +@pytest.mark.asyncio +async def test_multiple_plugins_with_own_parameters(db): + """ + Test that multiple plugins can use their own parameter names without conflict. + + This verifies that the parameter naming convention works: plugins prefix their + parameters (e.g., :plugin1_pattern, :plugin2_message) and both sets of parameters + are successfully bound in the SQL queries. + """ + await seed_catalog(db) + + def plugin_one() -> Callable[[str], PermissionSQL]: + def provider(action: str) -> PermissionSQL: + if action != "view-table": + return PermissionSQL("plugin_one", "SELECT NULL WHERE 0", {}) + return PermissionSQL( + "plugin_one", + """ + SELECT database_name AS parent, table_name AS child, + 1 AS allow, 'Plugin one used param: ' || :plugin1_param AS reason + FROM catalog_tables + WHERE database_name = 'accounting' + """, + { + "plugin1_param": "value1", + }, + ) + + return provider + + def plugin_two() -> Callable[[str], PermissionSQL]: + def provider(action: str) -> PermissionSQL: + if action != "view-table": + return PermissionSQL("plugin_two", "SELECT NULL WHERE 0", {}) + return PermissionSQL( + "plugin_two", + """ + SELECT database_name AS parent, table_name AS child, + 1 AS allow, 'Plugin two used param: ' || :plugin2_param AS reason + FROM catalog_tables + WHERE database_name = 'hr' + """, + { + "plugin2_param": "value2", + }, + ) + + return provider + + plugins = [plugin_one(), plugin_two()] + + rows = await resolve_permissions_from_catalog( + db, + {"id": "test_user"}, + plugins, + "view-table", + TABLE_CANDIDATES_SQL, + implicit_deny=False, + ) + + # Both plugins should contribute results with their parameters successfully bound + plugin_one_rows = [r for r in rows if r.get("reason") and "Plugin one" in r["reason"]] + plugin_two_rows = [r for r in rows if r.get("reason") and "Plugin two" in r["reason"]] + + assert len(plugin_one_rows) > 0, "Plugin one should contribute rules" + assert len(plugin_two_rows) > 0, "Plugin two should contribute rules" + + # Verify each plugin's parameters were successfully bound in the SQL + assert any("value1" in r.get("reason", "") for r in plugin_one_rows), \ + "Plugin one's :plugin1_param should be bound" + assert any("value2" in r.get("reason", "") for r in plugin_two_rows), \ + "Plugin two's :plugin2_param should be bound"