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"