mirror of
https://github.com/simonw/datasette.git
synced 2025-12-10 16:51:24 +01:00
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
This commit is contained in:
Simon Willison
parent
b8d26754df
commit
2ed2849a14
1 changed files with 111 additions and 151 deletions
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue