Remove permission_allowed hook entirely, refs #2528

The permission_allowed hook has been fully replaced by permission_resources_sql.
This commit removes:
- hookspec definition from hookspecs.py
- 4 implementations from default_permissions.py
- implementations from test plugins (my_plugin.py, my_plugin_2.py)
- hook monitoring infrastructure from conftest.py
- references from fixtures.py
- Also fixes test_get_permission to use ds.get_action() instead of ds.get_permission()
- Removes 5th column (source_plugin) from PermissionSQL queries

This completes the migration to the SQL-based permission system.

datasette/default_permissions.py

@@ -5,80 +5,6 @@ import itsdangerous
 import time
 
 
-@hookimpl(tryfirst=True, specname="permission_allowed")
-async def permission_allowed_sql_bridge(datasette, actor, action, resource):
-    """
-    Bridge config-based permission rules to the old permission_allowed API.
-
-    This allows views using the old string/tuple resource API to benefit from
-    config blocks defined in datasette.yaml without using the new resource-based system.
-
-    Note: This does NOT apply default allow rules - those should come from the
-    Permission object's default value to maintain backward compatibility.
-    """
-    # Only check config-based rules - don't apply defaults
-    config_rules = await _config_permission_rules(datasette, actor, action)
-    if not config_rules:
-        return None
-
-    # Evaluate config rules for this specific resource
-    for rule in config_rules:
-        if rule.params:  # Has config-based rules
-            from datasette.utils.permissions import resolve_permissions_with_candidates
-
-            # Build candidate based on resource
-            if resource is None:
-                candidates = [(None, None)]
-            elif isinstance(resource, str):
-                candidates = [(resource, None)]
-            elif isinstance(resource, tuple):
-                candidates = [(resource[0], resource[1])]
-            else:
-                return None
-
-            db = datasette.get_internal_database()
-            results = await resolve_permissions_with_candidates(
-                db, actor, [rule], candidates, action, implicit_deny=False
-            )
-            if results:
-                # Use the first result's allow value
-                for result in results:
-                    if result.get("allow") is not None:
-                        return bool(result["allow"])
-    return None
-
-
-@hookimpl(tryfirst=True, specname="permission_allowed")
-def permission_allowed_default_allow_sql(datasette, actor, action, resource):
-    """
-    Enforce the default_allow_sql setting for execute-sql permission.
-
-    When default_allow_sql is set to False, deny all execute-sql permissions.
-    This runs before other permission checks to ensure the setting is respected.
-    """
-    if action == "execute-sql":
-        if not datasette.setting("default_allow_sql"):
-            return False
-    return None
-
-
-@hookimpl(tryfirst=True, specname="permission_allowed")
-def permission_allowed_root(datasette, actor, action, resource):
-    """
-    Grant all permissions to root user when Datasette started with --root flag.
-
-    The --root flag is a localhost development tool. When used, it sets
-    datasette.root_enabled = True and creates an actor with id="root".
-    This hook grants that actor all permissions.
-
-    Other plugins can use the same pattern: check datasette.root_enabled
-    to decide whether to honor root users.
-    """
-    if datasette.root_enabled and actor and actor.get("id") == "root":
-        return True
-    return None
-
-
 @hookimpl
 async def permission_resources_sql(datasette, actor, action):
     rules: list[PermissionSQL] = []
@@ -89,7 +15,7 @@ async def permission_resources_sql(datasette, actor, action):
         # Add a single global-level allow rule (NULL, NULL) for root
         # This allows root to access everything by default, but database-level
         # and table-level deny rules in config can still block specific resources
-        sql = "SELECT NULL AS parent, NULL AS child, 1 AS allow, 'root user' AS reason, 'root_permissions' AS source_plugin"
+        sql = "SELECT NULL AS parent, NULL AS child, 1 AS allow, 'root user' AS reason"
         rules.append(
             PermissionSQL(
                 source="root_permissions",
@@ -104,7 +30,7 @@ async def permission_resources_sql(datasette, actor, action):
     # Check default_allow_sql setting for execute-sql action
     if action == "execute-sql" and not datasette.setting("default_allow_sql"):
         # Return a deny rule for all databases
-        sql = "SELECT NULL AS parent, NULL AS child, 0 AS allow, 'default_allow_sql is false' AS reason, 'default_allow_sql_setting' AS source_plugin"
+        sql = "SELECT NULL AS parent, NULL AS child, 0 AS allow, 'default_allow_sql is false' AS reason"
         rules.append(
             PermissionSQL(
                 source="default_allow_sql_setting",
@@ -129,8 +55,7 @@ async def permission_resources_sql(datasette, actor, action):
     if action in default_allow_actions:
         reason = f"default allow for {action}".replace("'", "''")
         sql = (
-            "SELECT NULL AS parent, NULL AS child, 1 AS allow, "
+            "SELECT NULL AS parent, NULL AS child, 1 AS allow, " f"'{reason}' AS reason"
-            f"'{reason}' AS reason, 'default_permissions' AS source_plugin"
         )
         rules.append(
             PermissionSQL(
@@ -287,7 +212,7 @@ async def _config_permission_rules(datasette, actor, action) -> list[PermissionS
     for idx, (parent, child, allow, reason) in enumerate(rows):
         key = f"cfg_{idx}"
         parts.append(
-            f"SELECT :{key}_parent AS parent, :{key}_child AS child, :{key}_allow AS allow, :{key}_reason AS reason, 'config_permissions' AS source_plugin"
+            f"SELECT :{key}_parent AS parent, :{key}_child AS child, :{key}_allow AS allow, :{key}_reason AS reason"
         )
         params[f"{key}_parent"] = parent
         params[f"{key}_child"] = child
@@ -355,22 +280,6 @@ def restrictions_allow_action(
     return False
 
 
-@hookimpl(specname="permission_allowed")
-def permission_allowed_actor_restrictions(datasette, actor, action, resource):
-    if actor is None:
-        return None
-    if "_r" not in actor:
-        # No restrictions, so we have no opinion
-        return None
-    _r = actor.get("_r")
-    if restrictions_allow_action(datasette, _r, action, resource):
-        # Return None because we do not have an opinion here
-        return None
-    else:
-        # Block this permission check
-        return False
-
-
 @hookimpl
 def actor_from_request(datasette, request):
     prefix = "dstok_"

datasette/hookspecs.py

@@ -110,11 +110,6 @@ def filters_from_request(request, database, table, datasette):
     ) based on the request"""
 
 
-@hookspec
-def permission_allowed(datasette, actor, action, resource):
-    """Check if actor is allowed to perform this action - return True, False or None"""
-
-
 @hookspec
 def permission_resources_sql(datasette, actor, action):
     """Return SQL query fragments for permission checks on resources.

tests/conftest.py

@@ -145,7 +145,7 @@ def check_permission_actions_are_documented():
     )
 
     def before(hook_name, hook_impls, kwargs):
-        if hook_name == "permission_allowed":
+        if hook_name == "permission_resources_sql":
             datasette = kwargs["datasette"]
             assert kwargs["action"] in datasette.actions, (
                 "'{}' has not been registered with register_actions()".format(
@@ -156,9 +156,7 @@ def check_permission_actions_are_documented():
             action = kwargs.get("action").replace("-", "_")
             assert (
                 action in documented_permission_actions
-            ), "Undocumented permission action: {}, resource: {}".format(
+            ), "Undocumented permission action: {}".format(action)
-                action, kwargs["resource"]
-            )
 
     pm.add_hookcall_monitoring(
         before=before, after=lambda outcome, hook_name, hook_impls, kwargs: None

tests/fixtures.py

@@ -44,7 +44,6 @@ EXPECTED_PLUGINS = [
             "forbidden",
             "homepage_actions",
             "menu_links",
-            "permission_allowed",
             "permission_resources_sql",
             "prepare_connection",
             "prepare_jinja2_environment",
@@ -74,7 +73,6 @@ EXPECTED_PLUGINS = [
             "extra_template_vars",
             "handle_exception",
             "menu_links",
-            "permission_allowed",
             "prepare_jinja2_environment",
             "register_routes",
             "render_cell",

tests/plugins/my_plugin.py

@@ -214,29 +214,6 @@ def asgi_wrapper():
     return wrap
 
 
-@hookimpl
-def permission_allowed(actor, action):
-    if action == "this_is_allowed":
-        return True
-    elif action == "this_is_denied":
-        return False
-    elif action == "view-database-download":
-        return actor.get("can_download") if actor else None
-    # Special permissions for latest.datasette.io demos
-    # See https://github.com/simonw/todomvc-datasette/issues/2
-    actor_id = None
-    if actor:
-        actor_id = actor.get("id")
-    if actor_id == "todomvc" and action in (
-        "insert-row",
-        "create-table",
-        "drop-table",
-        "delete-row",
-        "update-row",
-    ):
-        return True
-
-
 @hookimpl
 def register_routes():
     async def one(datasette):
@@ -549,24 +526,30 @@ def permission_resources_sql(datasette, actor, action):
 
     # Handle test actions used in test_hook_permission_allowed
     if action == "this_is_allowed":
-        sql = "SELECT NULL AS parent, NULL AS child, 1 AS allow, 'test plugin allows this_is_allowed' AS reason, 'my_plugin' AS source_plugin"
+        sql = "SELECT NULL AS parent, NULL AS child, 1 AS allow, 'test plugin allows this_is_allowed' AS reason"
         return PermissionSQL(source="my_plugin", sql=sql, params={})
     elif action == "this_is_denied":
-        sql = "SELECT NULL AS parent, NULL AS child, 0 AS allow, 'test plugin denies this_is_denied' AS reason, 'my_plugin' AS source_plugin"
+        sql = "SELECT NULL AS parent, NULL AS child, 0 AS allow, 'test plugin denies this_is_denied' AS reason"
         return PermissionSQL(source="my_plugin", sql=sql, params={})
     elif action == "this_is_allowed_async":
-        sql = "SELECT NULL AS parent, NULL AS child, 1 AS allow, 'test plugin allows this_is_allowed_async' AS reason, 'my_plugin' AS source_plugin"
+        sql = "SELECT NULL AS parent, NULL AS child, 1 AS allow, 'test plugin allows this_is_allowed_async' AS reason"
         return PermissionSQL(source="my_plugin", sql=sql, params={})
     elif action == "this_is_denied_async":
-        sql = "SELECT NULL AS parent, NULL AS child, 0 AS allow, 'test plugin denies this_is_denied_async' AS reason, 'my_plugin' AS source_plugin"
+        sql = "SELECT NULL AS parent, NULL AS child, 0 AS allow, 'test plugin denies this_is_denied_async' AS reason"
         return PermissionSQL(source="my_plugin", sql=sql, params={})
     elif action == "view-database-download":
         # Return rule based on actor's can_download permission
         if actor and actor.get("can_download"):
-            sql = "SELECT NULL AS parent, NULL AS child, 1 AS allow, 'actor has can_download' AS reason, 'my_plugin' AS source_plugin"
+            sql = "SELECT NULL AS parent, NULL AS child, 1 AS allow, 'actor has can_download' AS reason"
         else:
             return None  # No opinion
         return PermissionSQL(source="my_plugin", sql=sql, params={})
+    elif action == "view-database":
+        # Also grant view-database if actor has can_download (needed for download to work)
+        if actor and actor.get("can_download"):
+            sql = "SELECT NULL AS parent, NULL AS child, 1 AS allow, 'actor has can_download, grants view-database' AS reason"
+            return PermissionSQL(source="my_plugin", sql=sql, params={})
+        return None
     elif action in (
         "insert-row",
         "create-table",
@@ -577,7 +560,7 @@ def permission_resources_sql(datasette, actor, action):
         # Special permissions for latest.datasette.io demos
         actor_id = actor.get("id") if actor else None
         if actor_id == "todomvc":
-            sql = f"SELECT NULL AS parent, NULL AS child, 1 AS allow, 'todomvc actor allowed for {action}' AS reason, 'my_plugin' AS source_plugin"
+            sql = f"SELECT NULL AS parent, NULL AS child, 1 AS allow, 'todomvc actor allowed for {action}' AS reason"
             return PermissionSQL(source="my_plugin", sql=sql, params={})
 
     return None

tests/plugins/my_plugin_2.py

@@ -113,24 +113,6 @@ def actor_from_request(datasette, request):
     return inner
 
 
-@hookimpl
-def permission_allowed(datasette, actor, action):
-    # Testing asyncio version of permission_allowed
-    async def inner():
-        assert (
-            2
-            == (
-                await datasette.get_internal_database().execute("select 1 + 1")
-            ).first()[0]
-        )
-        if action == "this_is_allowed_async":
-            return True
-        elif action == "this_is_denied_async":
-            return False
-
-    return inner
-
-
 @hookimpl
 def prepare_jinja2_environment(env, datasette):
     env.filters["format_numeric"] = lambda s: f"{float(s):,.0f}"

tests/test_internals_datasette.py

@@ -164,14 +164,13 @@ def test_datasette_error_if_string_not_list(tmpdir):
 async def test_get_permission(ds_client):
     ds = ds_client.ds
     for name_or_abbr in ("vi", "view-instance", "vt", "view-table"):
-        permission = ds.get_permission(name_or_abbr)
+        action = ds.get_action(name_or_abbr)
         if "-" in name_or_abbr:
-            assert permission.name == name_or_abbr
+            assert action.name == name_or_abbr
         else:
-            assert permission.abbr == name_or_abbr
+            assert action.abbr == name_or_abbr
-    # And test KeyError
+    # And test None return for missing action
-    with pytest.raises(KeyError):
+    assert ds.get_action("missing-permission") is None
-        ds.get_permission("missing-permission")
 
 
 @pytest.mark.asyncio