From 2ed2849a1424418b3c6f5c64d77b776d4c6354d4 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Thu, 23 Oct 2025 13:53:01 -0700 Subject: [PATCH] Eliminate duplicate config checking by removing old permission_allowed hooks - Removed permission_allowed_default() hook (checked config twice) - Removed _resolve_config_view_permissions() and _resolve_config_permissions_blocks() helpers - Added permission_allowed_sql_bridge() to bridge old permission_allowed() API to new SQL system - Moved default_allow_sql setting check into permission_resources_sql() - Made root-level allow blocks apply to all view-* actions (view-database, view-table, view-query) - Added add_row_allow_block() helper for allow blocks that should deny when no match This resolves the duplicate checking issue where config blocks were evaluated twice: once in permission_allowed hooks and once in permission_resources_sql hooks. Note: One test still failing (test_permissions_checked for database download) - needs investigation --- datasette/default_permissions.py | 262 +++++++++++++------------------ 1 file changed, 111 insertions(+), 151 deletions(-) diff --git a/datasette/default_permissions.py b/datasette/default_permissions.py index 710c73c9..5cdc8052 100644 --- a/datasette/default_permissions.py +++ b/datasette/default_permissions.py @@ -127,6 +127,67 @@ def register_permissions(): ) +@hookimpl(tryfirst=True, specname="permission_allowed") +async def permission_allowed_sql_bridge(datasette, actor, action, resource): + """ + Bridge the old permission_allowed API to the new SQL-based system. + + This allows views using the old string/tuple resource API to benefit from + the SQL-based permission rules including config blocks. + """ + # First try to use the new resource-based system if possible + if action in datasette.actions: + action_obj = datasette.actions[action] + if action_obj and action_obj.resource_class: + # Try to convert string/tuple resource to Resource object + resource_obj = None + try: + if resource is None: + # Can't create a resource object without a resource identifier + pass # Fall through to config checking below + elif isinstance(resource, str): + resource_obj = action_obj.resource_class(database=resource) + elif isinstance(resource, tuple) and len(resource) == 2: + resource_obj = action_obj.resource_class(database=resource[0], table=resource[1]) + except Exception: + pass # Fall through to config checking below + + if resource_obj is not None: + # Successfully created resource object, use new system + return await datasette.allowed(action=action, resource=resource_obj, actor=actor) + + # Fall back to checking config permission blocks manually via SQL + 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_root(datasette, actor, action, resource): """ @@ -144,37 +205,6 @@ def permission_allowed_root(datasette, actor, action, resource): return None -@hookimpl(tryfirst=True, specname="permission_allowed") -def permission_allowed_default(datasette, actor, action, resource): - async def inner(): - # Resolve view permissions in allow blocks in configuration - if action in ( - "view-instance", - "view-database", - "view-table", - "view-query", - "execute-sql", - ): - result = await _resolve_config_view_permissions( - datasette, actor, action, resource - ) - if result is not None: - return result - - # Resolve custom permissions: blocks in configuration - result = await _resolve_config_permissions_blocks( - datasette, actor, action, resource - ) - if result is not None: - return result - - # --setting default_allow_sql - if action == "execute-sql" and not datasette.setting("default_allow_sql"): - return False - - return inner - - @hookimpl async def permission_resources_sql(datasette, actor, action): # Root user with root_enabled gets all permissions @@ -198,6 +228,18 @@ async def permission_resources_sql(datasette, actor, action): config_rules = await _config_permission_rules(datasette, actor, action) rules.extend(config_rules) + # 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" + rules.append( + PermissionSQL( + source="default_allow_sql_setting", + sql=sql, + params={}, + ) + ) + default_allow_actions = { "view-instance", "view-database", @@ -254,6 +296,21 @@ async def _config_permission_rules(datasette, actor, action) -> list[PermissionS ) ) + def add_row_allow_block(parent, child, allow_block, scope): + """For 'allow' blocks, always add a row if the block exists - deny if no match""" + if allow_block is None: + return + result = evaluate(allow_block) + # If result is None (no match) or False, treat as deny + rows.append( + ( + parent, + child, + bool(result), # None becomes False, False stays False, True stays True + f"config {'allow' if result else 'deny'} {scope}", + ) + ) + root_perm = (config.get("permissions") or {}).get(action) add_row(None, None, evaluate(root_perm), f"permissions for {action}") @@ -274,10 +331,10 @@ async def _config_permission_rules(datasette, actor, action) -> list[PermissionS if action == "view-table": table_allow = (table_config or {}).get("allow") - add_row( + add_row_allow_block( db_name, table_name, - evaluate(table_allow), + table_allow, f"allow for {action} on {db_name}/{table_name}", ) @@ -293,49 +350,53 @@ async def _config_permission_rules(datasette, actor, action) -> list[PermissionS ) if action == "view-query": query_allow = query_config.get("allow") - add_row( + add_row_allow_block( db_name, query_name, - evaluate(query_allow), + query_allow, f"allow for {action} on {db_name}/{query_name}", ) if action == "view-database": db_allow = db_config.get("allow") - add_row( - db_name, None, evaluate(db_allow), f"allow for {action} on {db_name}" + add_row_allow_block( + db_name, None, db_allow, f"allow for {action} on {db_name}" ) if action == "execute-sql": db_allow_sql = db_config.get("allow_sql") - add_row(db_name, None, evaluate(db_allow_sql), f"allow_sql for {db_name}") + add_row_allow_block(db_name, None, db_allow_sql, f"allow_sql for {db_name}") if action == "view-table": # Database-level allow block affects all tables in that database db_allow = db_config.get("allow") - add_row( - db_name, None, evaluate(db_allow), f"allow for {action} on {db_name}" + add_row_allow_block( + db_name, None, db_allow, f"allow for {action} on {db_name}" ) + # Root-level allow block applies to all view-* actions if action == "view-instance": allow_block = config.get("allow") - add_row(None, None, evaluate(allow_block), "allow for view-instance") + add_row_allow_block(None, None, allow_block, "allow for view-instance") + + if action == "view-database": + # Root-level allow block also applies to view-database + allow_block = config.get("allow") + add_row_allow_block(None, None, allow_block, "allow for view-database") if action == "view-table": - # Tables handled in loop - pass + # Root-level allow block also applies to view-table + allow_block = config.get("allow") + add_row_allow_block(None, None, allow_block, "allow for view-table") if action == "view-query": - # Queries handled in loop - pass + # Root-level allow block also applies to view-query + allow_block = config.get("allow") + add_row_allow_block(None, None, allow_block, "allow for view-query") if action == "execute-sql": allow_sql = config.get("allow_sql") - add_row(None, None, evaluate(allow_sql), "allow_sql") - - if action == "view-database": - # already handled per-database - pass + add_row_allow_block(None, None, allow_sql, "allow_sql") if not rows: return [] @@ -356,107 +417,6 @@ async def _config_permission_rules(datasette, actor, action) -> list[PermissionS return [PermissionSQL(source="config_permissions", sql=sql, params=params)] -async def _resolve_config_permissions_blocks(datasette, actor, action, resource): - # Check custom permissions: blocks - config = datasette.config or {} - root_block = (config.get("permissions", None) or {}).get(action) - if root_block: - root_result = actor_matches_allow(actor, root_block) - if root_result is not None: - return root_result - # Now try database-specific blocks - if not resource: - return None - if isinstance(resource, str): - database = resource - else: - database = resource[0] - database_block = ( - (config.get("databases", {}).get(database, {}).get("permissions", None)) or {} - ).get(action) - if database_block: - database_result = actor_matches_allow(actor, database_block) - if database_result is not None: - return database_result - # Finally try table/query specific blocks - if not isinstance(resource, tuple): - return None - database, table_or_query = resource - table_block = ( - ( - config.get("databases", {}) - .get(database, {}) - .get("tables", {}) - .get(table_or_query, {}) - .get("permissions", None) - ) - or {} - ).get(action) - if table_block: - table_result = actor_matches_allow(actor, table_block) - if table_result is not None: - return table_result - # Finally the canned queries - query_block = ( - ( - config.get("databases", {}) - .get(database, {}) - .get("queries", {}) - .get(table_or_query, {}) - .get("permissions", None) - ) - or {} - ).get(action) - if query_block: - query_result = actor_matches_allow(actor, query_block) - if query_result is not None: - return query_result - return None - - -async def _resolve_config_view_permissions(datasette, actor, action, resource): - config = datasette.config or {} - if action == "view-instance": - allow = config.get("allow") - if allow is not None: - return actor_matches_allow(actor, allow) - elif action == "view-database": - database_allow = ((config.get("databases") or {}).get(resource) or {}).get( - "allow" - ) - if database_allow is None: - return None - return actor_matches_allow(actor, database_allow) - elif action == "view-table": - database, table = resource - tables = ((config.get("databases") or {}).get(database) or {}).get( - "tables" - ) or {} - table_allow = (tables.get(table) or {}).get("allow") - if table_allow is None: - return None - return actor_matches_allow(actor, table_allow) - elif action == "view-query": - # Check if this query has a "allow" block in config - database, query_name = resource - query = await datasette.get_canned_query(database, query_name, actor) - assert query is not None - allow = query.get("allow") - if allow is None: - return None - return actor_matches_allow(actor, allow) - elif action == "execute-sql": - # Use allow_sql block from database block, or from top-level - database_allow_sql = ((config.get("databases") or {}).get(resource) or {}).get( - "allow_sql" - ) - if database_allow_sql is None: - database_allow_sql = config.get("allow_sql") - if database_allow_sql is None: - return None - return actor_matches_allow(actor, database_allow_sql) - - def restrictions_allow_action( datasette: "Datasette", restrictions: dict,