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

@@ -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

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")