From fcb7936db09beff8ea096271791c9b905e14d7a1 Mon Sep 17 00:00:00 2001 From: Tanbir Hossain Ramim <96797470+TanbirRamim@users.noreply.github.com> Date: Thu, 1 Oct 2026 03:05:31 +0200 Subject: [PATCH] Attribute permission rules to the plugin that returned them gather_permission_sql_from_hooks() paired hook results with get_hookimpls() by index. pluggy calls implementations in reverse registration order and leaves out the ones that return None, so the default source of a PermissionSQL could be credited to another plugin. Call each implementation directly instead, so every result stays paired with the implementation that produced it. Refs #2978 --- datasette/utils/permissions.py | 27 +++++++++++--- tests/test_actions_sql.py | 64 ++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 4 deletions(-) diff --git a/datasette/utils/permissions.py b/datasette/utils/permissions.py index 5a8ee8e2..ea974c00 100644 --- a/datasette/utils/permissions.py +++ b/datasette/utils/permissions.py @@ -32,15 +32,20 @@ async def gather_permission_sql_from_hooks( return SKIP_PERMISSION_CHECKS hook_caller = pm.hook.permission_resources_sql - hookimpls = hook_caller.get_hookimpls() - hook_results = list(hook_caller(datasette=datasette, actor=actor, action=action)) + hook_kwargs = {"datasette": datasette, "actor": actor, "action": action} collected: list[PermissionSQL] = [] actor_json = json.dumps(actor) if actor is not None else None actor_id = actor.get("id") if isinstance(actor, dict) else None - for index, result in enumerate(hook_results): - hookimpl = hookimpls[index] + # Call each implementation directly so every result is paired with the + # plugin that produced it. Calling the hook as a whole would not allow + # that: pluggy runs implementations in reverse registration order and + # leaves out the ones that return None, so the results cannot be matched + # back to get_hookimpls() by position. + for hookimpl in _hookimpls_in_call_order(hook_caller): + args = [hook_kwargs[argname] for argname in hookimpl.argnames] + result = hookimpl.function(*args) resolved = await await_me_maybe(result) default_source = _plugin_name_from_hookimpl(hookimpl) for permission_sql in _iter_permission_sql_from_result(resolved, action=action): @@ -55,6 +60,20 @@ async def gather_permission_sql_from_hooks( return collected +def _hookimpls_in_call_order(hook_caller) -> list: + """Non-wrapper implementations of a hook, in the order pluggy calls them. + + pluggy keeps registered implementations in registration order (with + ``trylast`` ones first and ``tryfirst`` ones last) and calls them in + reverse. Hook wrappers never produce a result of their own. + """ + return [ + hookimpl + for hookimpl in reversed(hook_caller.get_hookimpls()) + if not (hookimpl.hookwrapper or getattr(hookimpl, "wrapper", False)) + ] + + def _plugin_name_from_hookimpl(hookimpl) -> str: if getattr(hookimpl, "plugin_name", None): return hookimpl.plugin_name diff --git a/tests/test_actions_sql.py b/tests/test_actions_sql.py index 76320bd1..062be549 100644 --- a/tests/test_actions_sql.py +++ b/tests/test_actions_sql.py @@ -15,6 +15,7 @@ from datasette import hookimpl from datasette.app import Datasette from datasette.permissions import PermissionSQL from datasette.resources import DatabaseResource, QueryResource, TableResource +from datasette.utils.permissions import gather_permission_sql_from_hooks def test_resource_string_representations(): @@ -327,3 +328,66 @@ async def test_sql_does_filtering_not_python(test_ds): finally: test_ds.pm.unregister(plugin, name="test_plugin") + + +@pytest.mark.asyncio +async def test_gather_permission_sql_attributes_source_to_its_plugin(test_ds): + """Each rule's default source must be the plugin that returned it. + + pluggy calls implementations in reverse registration order and drops the + ones that return None, so pairing results with get_hookimpls() by index + attributed rules to the wrong plugin. + """ + + class AlphaPlugin: + @hookimpl + def permission_resources_sql(self, datasette, actor, action): + if action == "view-table": + return PermissionSQL( + sql="select 'a' as parent, 'x' as child, 1 as allow, 'from-alpha' as reason" + ) + return None + + class BetaPlugin: + @hookimpl + def permission_resources_sql(self, datasette, actor, action): + if action in ("view-table", "insert-row"): + return PermissionSQL( + sql="select 'b' as parent, 'y' as child, 1 as allow, 'from-beta' as reason" + ) + return None + + test_ds.pm.register(AlphaPlugin(), name="alpha") + test_ds.pm.register(BetaPlugin(), name="beta") + try: + rules = await gather_permission_sql_from_hooks( + datasette=test_ds, actor=None, action="view-table" + ) + sources = {rule.sql: rule.source for rule in rules} + assert ( + sources[ + "select 'a' as parent, 'x' as child, 1 as allow, 'from-alpha' as reason" + ] + == "alpha" + ) + assert ( + sources[ + "select 'b' as parent, 'y' as child, 1 as allow, 'from-beta' as reason" + ] + == "beta" + ) + + # Alpha returns None here, so Beta's rule is the only result and must + # not be attributed to Alpha + rules = await gather_permission_sql_from_hooks( + datasette=test_ds, actor=None, action="insert-row" + ) + assert [(rule.source, rule.sql) for rule in rules] == [ + ( + "beta", + "select 'b' as parent, 'y' as child, 1 as allow, 'from-beta' as reason", + ) + ] + finally: + test_ds.pm.unregister(name="alpha") + test_ds.pm.unregister(name="beta")