mirror of
https://github.com/simonw/datasette.git
synced 2025-12-10 16:51:24 +01:00
Remove implies_can_view logic from actor restrictions
Simplified restrictions_allow_action() to work on exact-match basis only. Actor restrictions no longer use permission implication logic - if an actor has view-table permission, they can view tables but NOT automatically view-instance or view-database. Updated test_restrictions_allow_action test cases to reflect new behavior: - Removed test cases expecting view-table to imply view-instance - Removed test cases expecting view-database to imply view-instance - Removed test cases expecting execute-sql to imply view-instance/view-database - Added test cases verifying exact matches work correctly - Added test case verifying abbreviations work (es -> execute-sql) This aligns actor restrictions with the new permission model where each action is checked independently without hierarchical implications. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
parent
e1582c1424
commit
30e2f9064b
2 changed files with 34 additions and 46 deletions
|
|
@ -303,50 +303,33 @@ def restrictions_allow_action(
|
||||||
action: str,
|
action: str,
|
||||||
resource: str | tuple[str, str],
|
resource: str | tuple[str, str],
|
||||||
):
|
):
|
||||||
"Do these restrictions allow the requested action against the requested resource?"
|
"""
|
||||||
if action == "view-instance":
|
Check if actor restrictions allow the requested action against the requested resource.
|
||||||
# Special case for view-instance: it's allowed if the restrictions include any
|
|
||||||
# actions that have the implies_can_view=True flag set
|
|
||||||
all_rules = restrictions.get("a") or []
|
|
||||||
for database_rules in (restrictions.get("d") or {}).values():
|
|
||||||
all_rules += database_rules
|
|
||||||
for database_resource_rules in (restrictions.get("r") or {}).values():
|
|
||||||
for resource_rules in database_resource_rules.values():
|
|
||||||
all_rules += resource_rules
|
|
||||||
actions = [datasette.get_action(action) for action in all_rules]
|
|
||||||
if any(a for a in actions if a and a.implies_can_view):
|
|
||||||
return True
|
|
||||||
|
|
||||||
if action == "view-database":
|
|
||||||
# Special case for view-database: it's allowed if the restrictions include any
|
|
||||||
# actions that have the implies_can_view=True flag set AND takes_parent
|
|
||||||
all_rules = restrictions.get("a") or []
|
|
||||||
database_rules = list((restrictions.get("d") or {}).get(resource) or [])
|
|
||||||
all_rules += database_rules
|
|
||||||
resource_rules = ((restrictions.get("r") or {}).get(resource) or {}).values()
|
|
||||||
for resource_rules in (restrictions.get("r") or {}).values():
|
|
||||||
for table_rules in resource_rules.values():
|
|
||||||
all_rules += table_rules
|
|
||||||
actions = [datasette.get_action(action) for action in all_rules]
|
|
||||||
if any(a for a in actions if a and a.implies_can_view and a.takes_parent):
|
|
||||||
return True
|
|
||||||
|
|
||||||
|
Restrictions work on an exact-match basis: if an actor has view-table permission,
|
||||||
|
they can view tables, but NOT automatically view-instance or view-database.
|
||||||
|
Each permission is checked independently without implication logic.
|
||||||
|
"""
|
||||||
# Does this action have an abbreviation?
|
# Does this action have an abbreviation?
|
||||||
to_check = {action}
|
to_check = {action}
|
||||||
action_obj = datasette.actions.get(action)
|
action_obj = datasette.actions.get(action)
|
||||||
if action_obj and action_obj.abbr:
|
if action_obj and action_obj.abbr:
|
||||||
to_check.add(action_obj.abbr)
|
to_check.add(action_obj.abbr)
|
||||||
|
|
||||||
# If restrictions is defined then we use those to further restrict the actor
|
# Check if restrictions explicitly allow this action
|
||||||
# Crucially, we only use this to say NO (return False) - we never
|
# Restrictions can be at three levels:
|
||||||
# use it to return YES (True) because that might over-ride other
|
# - "a": global (any resource)
|
||||||
# restrictions placed on this actor
|
# - "d": per-database
|
||||||
|
# - "r": per-table/resource
|
||||||
|
|
||||||
|
# Check global level (any resource)
|
||||||
all_allowed = restrictions.get("a")
|
all_allowed = restrictions.get("a")
|
||||||
if all_allowed is not None:
|
if all_allowed is not None:
|
||||||
assert isinstance(all_allowed, list)
|
assert isinstance(all_allowed, list)
|
||||||
if to_check.intersection(all_allowed):
|
if to_check.intersection(all_allowed):
|
||||||
return True
|
return True
|
||||||
# How about for the current database?
|
|
||||||
|
# Check database level
|
||||||
if resource:
|
if resource:
|
||||||
if isinstance(resource, str):
|
if isinstance(resource, str):
|
||||||
database_name = resource
|
database_name = resource
|
||||||
|
|
@ -357,17 +340,17 @@ def restrictions_allow_action(
|
||||||
assert isinstance(database_allowed, list)
|
assert isinstance(database_allowed, list)
|
||||||
if to_check.intersection(database_allowed):
|
if to_check.intersection(database_allowed):
|
||||||
return True
|
return True
|
||||||
# Or the current table? That's any time the resource is (database, table)
|
|
||||||
|
# Check table/resource level
|
||||||
if resource is not None and not isinstance(resource, str) and len(resource) == 2:
|
if resource is not None and not isinstance(resource, str) and len(resource) == 2:
|
||||||
database, table = resource
|
database, table = resource
|
||||||
table_allowed = restrictions.get("r", {}).get(database, {}).get(table)
|
table_allowed = restrictions.get("r", {}).get(database, {}).get(table)
|
||||||
# TODO: What should this do for canned queries?
|
|
||||||
if table_allowed is not None:
|
if table_allowed is not None:
|
||||||
assert isinstance(table_allowed, list)
|
assert isinstance(table_allowed, list)
|
||||||
if to_check.intersection(table_allowed):
|
if to_check.intersection(table_allowed):
|
||||||
return True
|
return True
|
||||||
|
|
||||||
# This action is not specifically allowed, so reject it
|
# This action is not explicitly allowed, so reject it
|
||||||
return False
|
return False
|
||||||
|
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -1240,25 +1240,30 @@ async def test_actor_restrictions(
|
||||||
@pytest.mark.parametrize(
|
@pytest.mark.parametrize(
|
||||||
"restrictions,action,resource,expected",
|
"restrictions,action,resource,expected",
|
||||||
(
|
(
|
||||||
|
# Exact match: view-instance restriction allows view-instance action
|
||||||
({"a": ["view-instance"]}, "view-instance", None, True),
|
({"a": ["view-instance"]}, "view-instance", None, True),
|
||||||
# view-table and view-database implies view-instance
|
# No implication: view-table does NOT imply view-instance
|
||||||
({"a": ["view-table"]}, "view-instance", None, True),
|
({"a": ["view-table"]}, "view-instance", None, False),
|
||||||
({"a": ["view-database"]}, "view-instance", None, True),
|
({"a": ["view-database"]}, "view-instance", None, False),
|
||||||
# update-row does not imply view-instance
|
# update-row does not imply view-instance
|
||||||
({"a": ["update-row"]}, "view-instance", None, False),
|
({"a": ["update-row"]}, "view-instance", None, False),
|
||||||
# view-table on a resource implies view-instance
|
# view-table on a resource does NOT imply view-instance
|
||||||
({"r": {"db1": {"t1": ["view-table"]}}}, "view-instance", None, True),
|
({"r": {"db1": {"t1": ["view-table"]}}}, "view-instance", None, False),
|
||||||
# execute-sql on a database implies view-instance, view-database
|
# execute-sql on a database does NOT imply view-instance or view-database
|
||||||
({"d": {"db1": ["es"]}}, "view-instance", None, True),
|
({"d": {"db1": ["es"]}}, "view-instance", None, False),
|
||||||
({"d": {"db1": ["es"]}}, "view-database", "db1", True),
|
({"d": {"db1": ["es"]}}, "view-database", "db1", False),
|
||||||
({"d": {"db1": ["es"]}}, "view-database", "db2", False),
|
({"d": {"db1": ["es"]}}, "view-database", "db2", False),
|
||||||
|
# But execute-sql abbreviation DOES allow execute-sql action on that database
|
||||||
|
({"d": {"db1": ["es"]}}, "execute-sql", "db1", True),
|
||||||
# update-row on a resource does not imply view-instance
|
# update-row on a resource does not imply view-instance
|
||||||
({"r": {"db1": {"t1": ["update-row"]}}}, "view-instance", None, False),
|
({"r": {"db1": {"t1": ["update-row"]}}}, "view-instance", None, False),
|
||||||
# view-database on a resource implies view-instance
|
# view-database on a database does NOT imply view-instance
|
||||||
({"d": {"db1": ["view-database"]}}, "view-instance", None, True),
|
({"d": {"db1": ["view-database"]}}, "view-instance", None, False),
|
||||||
|
# But it DOES allow view-database on that specific database
|
||||||
|
({"d": {"db1": ["view-database"]}}, "view-database", "db1", True),
|
||||||
# Having view-table on "a" allows access to any specific table
|
# Having view-table on "a" allows access to any specific table
|
||||||
({"a": ["view-table"]}, "view-table", ("dbname", "tablename"), True),
|
({"a": ["view-table"]}, "view-table", ("dbname", "tablename"), True),
|
||||||
# Ditto for on the database
|
# Having view-table on a database allows access to tables in that database
|
||||||
(
|
(
|
||||||
{"d": {"dbname": ["view-table"]}},
|
{"d": {"dbname": ["view-table"]}},
|
||||||
"view-table",
|
"view-table",
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue