From 5feb5fcf5d92e3ae72708eea2feb781977b95558 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Sat, 25 Oct 2025 08:52:48 -0700 Subject: [PATCH] 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 | 99 ++----------------------------- datasette/hookspecs.py | 5 -- tests/conftest.py | 6 +- tests/fixtures.py | 2 - tests/plugins/my_plugin.py | 41 ++++--------- tests/plugins/my_plugin_2.py | 18 ------ tests/test_internals_datasette.py | 11 ++-- 7 files changed, 23 insertions(+), 159 deletions(-) diff --git a/datasette/default_permissions.py b/datasette/default_permissions.py index 7eda09c8..fe8db31e 100644 --- a/datasette/default_permissions.py +++ b/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, " - f"'{reason}' AS reason, 'default_permissions' AS source_plugin" + "SELECT NULL AS parent, NULL AS child, 1 AS allow, " f"'{reason}' AS reason" ) 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_" diff --git a/datasette/hookspecs.py b/datasette/hookspecs.py index c2ef9495..3f6a1425 100644 --- a/datasette/hookspecs.py +++ b/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. diff --git a/tests/conftest.py b/tests/conftest.py index 376a61b4..6e00c5d8 100644 --- a/tests/conftest.py +++ b/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( - action, kwargs["resource"] - ) + ), "Undocumented permission action: {}".format(action) pm.add_hookcall_monitoring( before=before, after=lambda outcome, hook_name, hook_impls, kwargs: None diff --git a/tests/fixtures.py b/tests/fixtures.py index 48b3653a..5c51a7c5 100644 --- a/tests/fixtures.py +++ b/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", diff --git a/tests/plugins/my_plugin.py b/tests/plugins/my_plugin.py index 59565dcb..cf3d6125 100644 --- a/tests/plugins/my_plugin.py +++ b/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 diff --git a/tests/plugins/my_plugin_2.py b/tests/plugins/my_plugin_2.py index bb82b8c1..35775ef9 100644 --- a/tests/plugins/my_plugin_2.py +++ b/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}" diff --git a/tests/test_internals_datasette.py b/tests/test_internals_datasette.py index c7c54b4b..9990db04 100644 --- a/tests/test_internals_datasette.py +++ b/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 - # And test KeyError - with pytest.raises(KeyError): - ds.get_permission("missing-permission") + assert action.abbr == name_or_abbr + # And test None return for missing action + assert ds.get_action("missing-permission") is None @pytest.mark.asyncio