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:
parent
faef51ad05
commit
f4245dce66
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")
|
@hookimpl(tryfirst=True, specname="permission_allowed")
|
||||||
def permission_allowed_root(datasette, actor, action, resource):
|
def permission_allowed_root(datasette, actor, action, resource):
|
||||||
"""
|
"""
|
||||||
|
|
@ -144,37 +205,6 @@ def permission_allowed_root(datasette, actor, action, resource):
|
||||||
return None
|
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
|
@hookimpl
|
||||||
async def permission_resources_sql(datasette, actor, action):
|
async def permission_resources_sql(datasette, actor, action):
|
||||||
# Root user with root_enabled gets all permissions
|
# 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)
|
config_rules = await _config_permission_rules(datasette, actor, action)
|
||||||
rules.extend(config_rules)
|
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 = {
|
default_allow_actions = {
|
||||||
"view-instance",
|
"view-instance",
|
||||||
"view-database",
|
"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)
|
root_perm = (config.get("permissions") or {}).get(action)
|
||||||
add_row(None, None, evaluate(root_perm), f"permissions for {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":
|
if action == "view-table":
|
||||||
table_allow = (table_config or {}).get("allow")
|
table_allow = (table_config or {}).get("allow")
|
||||||
add_row(
|
add_row_allow_block(
|
||||||
db_name,
|
db_name,
|
||||||
table_name,
|
table_name,
|
||||||
evaluate(table_allow),
|
table_allow,
|
||||||
f"allow for {action} on {db_name}/{table_name}",
|
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":
|
if action == "view-query":
|
||||||
query_allow = query_config.get("allow")
|
query_allow = query_config.get("allow")
|
||||||
add_row(
|
add_row_allow_block(
|
||||||
db_name,
|
db_name,
|
||||||
query_name,
|
query_name,
|
||||||
evaluate(query_allow),
|
query_allow,
|
||||||
f"allow for {action} on {db_name}/{query_name}",
|
f"allow for {action} on {db_name}/{query_name}",
|
||||||
)
|
)
|
||||||
|
|
||||||
if action == "view-database":
|
if action == "view-database":
|
||||||
db_allow = db_config.get("allow")
|
db_allow = db_config.get("allow")
|
||||||
add_row(
|
add_row_allow_block(
|
||||||
db_name, None, evaluate(db_allow), f"allow for {action} on {db_name}"
|
db_name, None, db_allow, f"allow for {action} on {db_name}"
|
||||||
)
|
)
|
||||||
|
|
||||||
if action == "execute-sql":
|
if action == "execute-sql":
|
||||||
db_allow_sql = db_config.get("allow_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":
|
if action == "view-table":
|
||||||
# Database-level allow block affects all tables in that database
|
# Database-level allow block affects all tables in that database
|
||||||
db_allow = db_config.get("allow")
|
db_allow = db_config.get("allow")
|
||||||
add_row(
|
add_row_allow_block(
|
||||||
db_name, None, evaluate(db_allow), f"allow for {action} on {db_name}"
|
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":
|
if action == "view-instance":
|
||||||
allow_block = config.get("allow")
|
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":
|
if action == "view-table":
|
||||||
# Tables handled in loop
|
# Root-level allow block also applies to view-table
|
||||||
pass
|
allow_block = config.get("allow")
|
||||||
|
add_row_allow_block(None, None, allow_block, "allow for view-table")
|
||||||
|
|
||||||
if action == "view-query":
|
if action == "view-query":
|
||||||
# Queries handled in loop
|
# Root-level allow block also applies to view-query
|
||||||
pass
|
allow_block = config.get("allow")
|
||||||
|
add_row_allow_block(None, None, allow_block, "allow for view-query")
|
||||||
|
|
||||||
if action == "execute-sql":
|
if action == "execute-sql":
|
||||||
allow_sql = config.get("allow_sql")
|
allow_sql = config.get("allow_sql")
|
||||||
add_row(None, None, evaluate(allow_sql), "allow_sql")
|
add_row_allow_block(None, None, allow_sql, "allow_sql")
|
||||||
|
|
||||||
if action == "view-database":
|
|
||||||
# already handled per-database
|
|
||||||
pass
|
|
||||||
|
|
||||||
if not rows:
|
if not rows:
|
||||||
return []
|
return []
|
||||||
|
|
@ -356,107 +417,6 @@ async def _config_permission_rules(datasette, actor, action) -> list[PermissionS
|
||||||
return [PermissionSQL(source="config_permissions", sql=sql, params=params)]
|
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(
|
def restrictions_allow_action(
|
||||||
datasette: "Datasette",
|
datasette: "Datasette",
|
||||||
restrictions: dict,
|
restrictions: dict,
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue