mirror of
https://github.com/simonw/datasette.git
synced 2025-12-10 16:51:24 +01:00
Implement also_requires to enforce view-database for execute-sql
Adds Action.also_requires field to specify dependencies between permissions. When an action has also_requires set, users must have permission for BOTH the main action AND the required action on a resource. Applies this to execute-sql, which now requires view-database permission. This prevents the illogical scenario where users can execute SQL on a database they cannot view. Changes: - Add also_requires field to Action dataclass in datasette/permissions.py - Update execute-sql action with also_requires="view-database" - Implement also_requires handling in build_allowed_resources_sql() - Implement also_requires handling in AllowedResourcesView endpoint - Add test verifying execute-sql requires view-database permission Fixes #2527
This commit is contained in:
Simon Willison
parent
a2994cc5bb
commit
e8b79970fb
6 changed files with 460 additions and 366 deletions
|
|
@ -60,6 +60,7 @@ def register_actions():
|
|||
takes_parent=True,
|
||||
takes_child=False,
|
||||
resource_class=DatabaseResource,
|
||||
also_requires="view-database",
|
||||
),
|
||||
# Debug actions
|
||||
Action(
|
||||
|
|
|
|||
|
|
@ -76,6 +76,7 @@ class Action:
|
|||
takes_parent: bool
|
||||
takes_child: bool
|
||||
resource_class: type[Resource]
|
||||
also_requires: str | None = None # Optional action name that must also be allowed
|
||||
|
||||
|
||||
@dataclass
|
||||
|
|
|
|||
|
|
@ -100,6 +100,82 @@ async def build_allowed_resources_sql(
|
|||
if not action_obj:
|
||||
raise ValueError(f"Unknown action: {action}")
|
||||
|
||||
# If this action also_requires another action, we need to combine the queries
|
||||
if action_obj.also_requires:
|
||||
# Build both queries
|
||||
main_sql, main_params = await _build_single_action_sql(
|
||||
datasette,
|
||||
actor,
|
||||
action,
|
||||
parent=parent,
|
||||
include_is_private=include_is_private,
|
||||
)
|
||||
required_sql, required_params = await _build_single_action_sql(
|
||||
datasette,
|
||||
actor,
|
||||
action_obj.also_requires,
|
||||
parent=parent,
|
||||
include_is_private=False,
|
||||
)
|
||||
|
||||
# Merge parameters - they should have identical values for :actor, :actor_id, etc.
|
||||
all_params = {**main_params, **required_params}
|
||||
if parent is not None:
|
||||
all_params["filter_parent"] = parent
|
||||
|
||||
# Combine with INNER JOIN - only resources allowed by both actions
|
||||
combined_sql = f"""
|
||||
WITH
|
||||
main_allowed AS (
|
||||
{main_sql}
|
||||
),
|
||||
required_allowed AS (
|
||||
{required_sql}
|
||||
)
|
||||
SELECT m.parent, m.child, m.reason"""
|
||||
|
||||
if include_is_private:
|
||||
combined_sql += ", m.is_private"
|
||||
|
||||
combined_sql += """
|
||||
FROM main_allowed m
|
||||
INNER JOIN required_allowed r
|
||||
ON ((m.parent = r.parent) OR (m.parent IS NULL AND r.parent IS NULL))
|
||||
AND ((m.child = r.child) OR (m.child IS NULL AND r.child IS NULL))
|
||||
"""
|
||||
|
||||
if parent is not None:
|
||||
combined_sql += "WHERE m.parent = :filter_parent\n"
|
||||
|
||||
combined_sql += "ORDER BY m.parent, m.child"
|
||||
|
||||
return combined_sql, all_params
|
||||
|
||||
# No also_requires, build single action query
|
||||
return await _build_single_action_sql(
|
||||
datasette, actor, action, parent=parent, include_is_private=include_is_private
|
||||
)
|
||||
|
||||
|
||||
async def _build_single_action_sql(
|
||||
datasette: "Datasette",
|
||||
actor: dict | None,
|
||||
action: str,
|
||||
*,
|
||||
parent: str | None = None,
|
||||
include_is_private: bool = False,
|
||||
) -> tuple[str, dict]:
|
||||
"""
|
||||
Build SQL for a single action (internal helper for build_allowed_resources_sql).
|
||||
|
||||
This contains the original logic from build_allowed_resources_sql, extracted
|
||||
to allow combining multiple actions when also_requires is used.
|
||||
"""
|
||||
# Get the Action object
|
||||
action_obj = datasette.actions.get(action)
|
||||
if not action_obj:
|
||||
raise ValueError(f"Unknown action: {action}")
|
||||
|
||||
# Get base resources SQL from the resource class
|
||||
base_resources_sql = action_obj.resource_class.resources_sql()
|
||||
|
||||
|
|
|
|||
|
|
@ -289,35 +289,125 @@ class AllowedResourcesView(BaseView):
|
|||
headers=headers,
|
||||
)
|
||||
|
||||
plugins = []
|
||||
for block in pm.hook.permission_resources_sql(
|
||||
datasette=self.ds,
|
||||
actor=actor,
|
||||
action=action,
|
||||
):
|
||||
block = await await_me_maybe(block)
|
||||
if block is None:
|
||||
continue
|
||||
if isinstance(block, (list, tuple)):
|
||||
candidates = block
|
||||
else:
|
||||
candidates = [block]
|
||||
for candidate in candidates:
|
||||
if candidate is None:
|
||||
# Check if this action requires another action
|
||||
action_obj = self.ds.actions.get(action)
|
||||
if action_obj and action_obj.also_requires:
|
||||
# Need to combine results from both actions
|
||||
# Get allowed resources for the main action
|
||||
plugins = []
|
||||
for block in pm.hook.permission_resources_sql(
|
||||
datasette=self.ds,
|
||||
actor=actor,
|
||||
action=action,
|
||||
):
|
||||
block = await await_me_maybe(block)
|
||||
if block is None:
|
||||
continue
|
||||
plugins.append(candidate)
|
||||
if isinstance(block, (list, tuple)):
|
||||
candidates = block
|
||||
else:
|
||||
candidates = [block]
|
||||
for candidate in candidates:
|
||||
if candidate is None:
|
||||
continue
|
||||
plugins.append(candidate)
|
||||
|
||||
rows = await resolve_permissions_from_catalog(
|
||||
db,
|
||||
actor=actor,
|
||||
plugins=plugins,
|
||||
action=action,
|
||||
candidate_sql=candidate_sql,
|
||||
candidate_params=candidate_params,
|
||||
implicit_deny=True,
|
||||
)
|
||||
main_rows = await resolve_permissions_from_catalog(
|
||||
db,
|
||||
actor=actor,
|
||||
plugins=plugins,
|
||||
action=action,
|
||||
candidate_sql=candidate_sql,
|
||||
candidate_params=candidate_params,
|
||||
implicit_deny=True,
|
||||
)
|
||||
main_allowed = {
|
||||
(row["parent"], row["child"]) for row in main_rows if row["allow"] == 1
|
||||
}
|
||||
|
||||
allowed_rows = [row for row in rows if row["allow"] == 1]
|
||||
# Get allowed resources for the required action
|
||||
required_action = action_obj.also_requires
|
||||
required_candidate_sql, required_candidate_params = self.CANDIDATE_SQL.get(
|
||||
required_action, (None, None)
|
||||
)
|
||||
if not required_candidate_sql:
|
||||
# If the required action doesn't have candidate SQL, deny everything
|
||||
allowed_rows = []
|
||||
else:
|
||||
required_plugins = []
|
||||
for block in pm.hook.permission_resources_sql(
|
||||
datasette=self.ds,
|
||||
actor=actor,
|
||||
action=required_action,
|
||||
):
|
||||
block = await await_me_maybe(block)
|
||||
if block is None:
|
||||
continue
|
||||
if isinstance(block, (list, tuple)):
|
||||
candidates = block
|
||||
else:
|
||||
candidates = [block]
|
||||
for candidate in candidates:
|
||||
if candidate is None:
|
||||
continue
|
||||
required_plugins.append(candidate)
|
||||
|
||||
required_rows = await resolve_permissions_from_catalog(
|
||||
db,
|
||||
actor=actor,
|
||||
plugins=required_plugins,
|
||||
action=required_action,
|
||||
candidate_sql=required_candidate_sql,
|
||||
candidate_params=required_candidate_params,
|
||||
implicit_deny=True,
|
||||
)
|
||||
required_allowed = {
|
||||
(row["parent"], row["child"])
|
||||
for row in required_rows
|
||||
if row["allow"] == 1
|
||||
}
|
||||
|
||||
# Intersect the two sets - only resources allowed by BOTH actions
|
||||
allowed_resources = main_allowed & required_allowed
|
||||
|
||||
# Get full row data for the allowed resources
|
||||
allowed_rows = [
|
||||
row
|
||||
for row in main_rows
|
||||
if row["allow"] == 1
|
||||
and (row["parent"], row["child"]) in allowed_resources
|
||||
]
|
||||
else:
|
||||
# No also_requires, use normal path
|
||||
plugins = []
|
||||
for block in pm.hook.permission_resources_sql(
|
||||
datasette=self.ds,
|
||||
actor=actor,
|
||||
action=action,
|
||||
):
|
||||
block = await await_me_maybe(block)
|
||||
if block is None:
|
||||
continue
|
||||
if isinstance(block, (list, tuple)):
|
||||
candidates = block
|
||||
else:
|
||||
candidates = [block]
|
||||
for candidate in candidates:
|
||||
if candidate is None:
|
||||
continue
|
||||
plugins.append(candidate)
|
||||
|
||||
rows = await resolve_permissions_from_catalog(
|
||||
db,
|
||||
actor=actor,
|
||||
plugins=plugins,
|
||||
action=action,
|
||||
candidate_sql=candidate_sql,
|
||||
candidate_params=candidate_params,
|
||||
implicit_deny=True,
|
||||
)
|
||||
|
||||
allowed_rows = [row for row in rows if row["allow"] == 1]
|
||||
if parent_filter is not None:
|
||||
allowed_rows = [
|
||||
row for row in allowed_rows if row["parent"] == parent_filter
|
||||
|
|
|
|||
|
|
@ -1,6 +1,5 @@
|
|||
"""
|
||||
Tests for permission inspection endpoints:
|
||||
- /-/check.json
|
||||
Tests for permission endpoints:
|
||||
- /-/allowed.json
|
||||
- /-/rules.json
|
||||
"""
|
||||
|
|
@ -12,159 +11,50 @@ from datasette.app import Datasette
|
|||
|
||||
@pytest_asyncio.fixture
|
||||
async def ds_with_permissions():
|
||||
"""Create a Datasette instance with some permission rules configured."""
|
||||
ds = Datasette(
|
||||
config={
|
||||
"databases": {
|
||||
"content": {
|
||||
"allow": {"id": "*"}, # Allow all authenticated users
|
||||
"tables": {
|
||||
"articles": {
|
||||
"allow": {"id": "editor"}, # Only editor can view
|
||||
}
|
||||
},
|
||||
},
|
||||
"private": {
|
||||
"allow": False, # Deny everyone
|
||||
},
|
||||
}
|
||||
}
|
||||
)
|
||||
"""Create a Datasette instance with test data and permissions."""
|
||||
ds = Datasette()
|
||||
ds.root_enabled = True
|
||||
await ds.invoke_startup()
|
||||
# Add some test databases
|
||||
ds.add_memory_database("content")
|
||||
ds.add_memory_database("private")
|
||||
|
||||
# Add some test databases and tables
|
||||
db = ds.add_memory_database("analytics")
|
||||
await db.execute_write(
|
||||
"CREATE TABLE IF NOT EXISTS users (id INTEGER PRIMARY KEY, name TEXT, email TEXT)"
|
||||
)
|
||||
await db.execute_write(
|
||||
"CREATE TABLE IF NOT EXISTS events (id INTEGER PRIMARY KEY, event_type TEXT, user_id INTEGER)"
|
||||
)
|
||||
|
||||
db2 = ds.add_memory_database("production")
|
||||
await db2.execute_write(
|
||||
"CREATE TABLE IF NOT EXISTS orders (id INTEGER PRIMARY KEY, total REAL)"
|
||||
)
|
||||
await db2.execute_write(
|
||||
"CREATE TABLE IF NOT EXISTS customers (id INTEGER PRIMARY KEY, name TEXT)"
|
||||
)
|
||||
|
||||
await ds.refresh_schemas()
|
||||
|
||||
return ds
|
||||
|
||||
|
||||
# /-/check.json tests
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
"path,expected_status,expected_keys",
|
||||
[
|
||||
# Valid request
|
||||
(
|
||||
"/-/check.json?action=view-instance",
|
||||
200,
|
||||
{"action", "allowed", "resource"},
|
||||
),
|
||||
# Missing action parameter
|
||||
("/-/check.json", 400, {"error"}),
|
||||
# Invalid action
|
||||
("/-/check.json?action=nonexistent", 404, {"error"}),
|
||||
# With parent parameter
|
||||
(
|
||||
"/-/check.json?action=view-database&parent=content",
|
||||
200,
|
||||
{"action", "allowed", "resource"},
|
||||
),
|
||||
# With parent and child parameters
|
||||
(
|
||||
"/-/check.json?action=view-table&parent=content&child=articles",
|
||||
200,
|
||||
{"action", "allowed", "resource"},
|
||||
),
|
||||
],
|
||||
)
|
||||
async def test_check_json_basic(
|
||||
ds_with_permissions, path, expected_status, expected_keys
|
||||
):
|
||||
response = await ds_with_permissions.client.get(path)
|
||||
assert response.status_code == expected_status
|
||||
data = response.json()
|
||||
assert expected_keys.issubset(data.keys())
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_check_json_response_structure(ds_with_permissions):
|
||||
"""Test that /-/check.json returns the expected structure."""
|
||||
response = await ds_with_permissions.client.get(
|
||||
"/-/check.json?action=view-instance"
|
||||
)
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
|
||||
# Check required fields
|
||||
assert "action" in data
|
||||
assert "allowed" in data
|
||||
assert "resource" in data
|
||||
|
||||
# Check resource structure
|
||||
assert "parent" in data["resource"]
|
||||
assert "child" in data["resource"]
|
||||
assert "path" in data["resource"]
|
||||
|
||||
# Check allowed is boolean
|
||||
assert isinstance(data["allowed"], bool)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_check_json_redacts_sensitive_fields_without_debug_permission(
|
||||
ds_with_permissions,
|
||||
):
|
||||
"""Test that /-/check.json redacts reason and source_plugin without permissions-debug."""
|
||||
# Anonymous user should not see sensitive fields
|
||||
response = await ds_with_permissions.client.get(
|
||||
"/-/check.json?action=view-instance"
|
||||
)
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
# Sensitive fields should not be present
|
||||
assert "reason" not in data
|
||||
assert "source_plugin" not in data
|
||||
# But these non-sensitive fields should be present
|
||||
assert "used_default" in data
|
||||
assert "depth" in data
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_check_json_shows_sensitive_fields_with_debug_permission(
|
||||
ds_with_permissions,
|
||||
):
|
||||
"""Test that /-/check.json shows reason and source_plugin with permissions-debug."""
|
||||
# User with permissions-debug should see sensitive fields
|
||||
response = await ds_with_permissions.client.get(
|
||||
"/-/check.json?action=view-instance",
|
||||
cookies={"ds_actor": ds_with_permissions.client.actor_cookie({"id": "root"})},
|
||||
)
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
# Sensitive fields should be present
|
||||
assert "reason" in data
|
||||
assert "source_plugin" in data
|
||||
assert "used_default" in data
|
||||
assert "depth" in data
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_check_json_child_requires_parent(ds_with_permissions):
|
||||
"""Test that child parameter requires parent parameter."""
|
||||
response = await ds_with_permissions.client.get(
|
||||
"/-/check.json?action=view-table&child=articles"
|
||||
)
|
||||
assert response.status_code == 400
|
||||
data = response.json()
|
||||
assert "error" in data
|
||||
assert "parent" in data["error"].lower()
|
||||
|
||||
|
||||
# /-/allowed.json tests
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
"path,expected_status,expected_keys",
|
||||
[
|
||||
# Valid supported actions
|
||||
# Instance level permission
|
||||
(
|
||||
"/-/allowed.json?action=view-instance",
|
||||
200,
|
||||
{"action", "items", "total", "page"},
|
||||
),
|
||||
# Database level permission
|
||||
(
|
||||
"/-/allowed.json?action=view-database",
|
||||
200,
|
||||
{"action", "items", "total", "page"},
|
||||
),
|
||||
# Table level permission
|
||||
(
|
||||
"/-/allowed.json?action=view-table",
|
||||
200,
|
||||
|
|
@ -219,139 +109,98 @@ async def test_allowed_json_response_structure(ds_with_permissions):
|
|||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_allowed_json_redacts_sensitive_fields_without_debug_permission(
|
||||
ds_with_permissions,
|
||||
):
|
||||
"""Test that /-/allowed.json redacts reason and source_plugin without permissions-debug."""
|
||||
# Anonymous user should not see sensitive fields
|
||||
async def test_allowed_json_with_actor(ds_with_permissions):
|
||||
"""Test /-/allowed.json includes actor information."""
|
||||
response = await ds_with_permissions.client.get(
|
||||
"/-/allowed.json?action=view-instance"
|
||||
"/-/allowed.json?action=view-table",
|
||||
cookies={
|
||||
"ds_actor": ds_with_permissions.client.actor_cookie({"id": "test_user"})
|
||||
},
|
||||
)
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
if data["items"]:
|
||||
item = data["items"][0]
|
||||
assert "reason" not in item
|
||||
assert "source_plugin" not in item
|
||||
assert data["actor_id"] == "test_user"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_allowed_json_shows_sensitive_fields_with_debug_permission(
|
||||
ds_with_permissions,
|
||||
):
|
||||
"""Test that /-/allowed.json shows reason and source_plugin with permissions-debug."""
|
||||
# User with permissions-debug should see sensitive fields
|
||||
response = await ds_with_permissions.client.get(
|
||||
"/-/allowed.json?action=view-instance",
|
||||
cookies={"ds_actor": ds_with_permissions.client.actor_cookie({"id": "root"})},
|
||||
async def test_allowed_json_pagination():
|
||||
"""Test that /-/allowed.json pagination works."""
|
||||
ds = Datasette()
|
||||
await ds.invoke_startup()
|
||||
|
||||
# Create many tables to test pagination
|
||||
db = ds.add_memory_database("test")
|
||||
for i in range(30):
|
||||
await db.execute_write(f"CREATE TABLE table{i:02d} (id INTEGER PRIMARY KEY)")
|
||||
await ds.refresh_schemas()
|
||||
|
||||
# Test page 1
|
||||
response = await ds.client.get(
|
||||
"/-/allowed.json?action=view-table&page_size=10&page=1"
|
||||
)
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
if data["items"]:
|
||||
item = data["items"][0]
|
||||
assert "reason" in item
|
||||
assert "source_plugin" in item
|
||||
assert data["page"] == 1
|
||||
assert data["page_size"] == 10
|
||||
assert len(data["items"]) == 10
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_allowed_json_only_shows_allowed_resources(ds_with_permissions):
|
||||
"""Test that /-/allowed.json only shows resources with allow=1."""
|
||||
response = await ds_with_permissions.client.get(
|
||||
"/-/allowed.json?action=view-instance"
|
||||
# Test page 2
|
||||
response = await ds.client.get(
|
||||
"/-/allowed.json?action=view-table&page_size=10&page=2"
|
||||
)
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
assert data["page"] == 2
|
||||
assert len(data["items"]) == 10
|
||||
|
||||
# All items should have allow implicitly set to 1 (not in response but verified by the endpoint logic)
|
||||
# The endpoint filters to only show allowed resources
|
||||
assert isinstance(data["items"], list)
|
||||
assert data["total"] >= 0
|
||||
# Verify items are different between pages
|
||||
response1 = await ds.client.get(
|
||||
"/-/allowed.json?action=view-table&page_size=10&page=1"
|
||||
)
|
||||
response2 = await ds.client.get(
|
||||
"/-/allowed.json?action=view-table&page_size=10&page=2"
|
||||
)
|
||||
items1 = {(item["parent"], item["child"]) for item in response1.json()["items"]}
|
||||
items2 = {(item["parent"], item["child"]) for item in response2.json()["items"]}
|
||||
assert items1 != items2
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
"page,page_size",
|
||||
[
|
||||
(1, 10),
|
||||
(2, 50),
|
||||
(1, 200), # max page size
|
||||
],
|
||||
)
|
||||
async def test_allowed_json_pagination(ds_with_permissions, page, page_size):
|
||||
"""Test pagination parameters."""
|
||||
response = await ds_with_permissions.client.get(
|
||||
f"/-/allowed.json?action=view-instance&page={page}&page_size={page_size}"
|
||||
)
|
||||
async def test_allowed_json_total_count(ds_with_permissions):
|
||||
"""Test that /-/allowed.json returns correct total count."""
|
||||
response = await ds_with_permissions.client.get("/-/allowed.json?action=view-table")
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
assert data["page"] == page
|
||||
assert data["page_size"] == min(page_size, 200) # Capped at 200
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
"params,expected_status",
|
||||
[
|
||||
("page=0", 400), # page must be >= 1
|
||||
("page=-1", 400),
|
||||
("page_size=0", 400), # page_size must be >= 1
|
||||
("page_size=-1", 400),
|
||||
("page=abc", 400), # page must be integer
|
||||
("page_size=xyz", 400), # page_size must be integer
|
||||
],
|
||||
)
|
||||
async def test_allowed_json_pagination_errors(
|
||||
ds_with_permissions, params, expected_status
|
||||
):
|
||||
"""Test pagination error handling."""
|
||||
response = await ds_with_permissions.client.get(
|
||||
f"/-/allowed.json?action=view-instance&{params}"
|
||||
)
|
||||
assert response.status_code == expected_status
|
||||
# We created 4 tables total (2 in analytics, 2 in production)
|
||||
assert data["total"] == 4
|
||||
|
||||
|
||||
# /-/rules.json tests
|
||||
@pytest.mark.asyncio
|
||||
async def test_rules_json_requires_permissions_debug(ds_with_permissions):
|
||||
"""Test that /-/rules.json requires permissions-debug permission."""
|
||||
# Anonymous user should be denied
|
||||
response = await ds_with_permissions.client.get(
|
||||
"/-/rules.json?action=view-instance"
|
||||
)
|
||||
assert response.status_code == 403
|
||||
|
||||
# Regular authenticated user should also be denied
|
||||
response = await ds_with_permissions.client.get(
|
||||
"/-/rules.json?action=view-instance",
|
||||
cookies={
|
||||
"ds_actor": ds_with_permissions.client.actor_cookie({"id": "regular-user"})
|
||||
},
|
||||
)
|
||||
assert response.status_code == 403
|
||||
|
||||
# User with permissions-debug should be allowed
|
||||
response = await ds_with_permissions.client.get(
|
||||
"/-/rules.json?action=view-instance",
|
||||
cookies={"ds_actor": ds_with_permissions.client.actor_cookie({"id": "root"})},
|
||||
)
|
||||
assert response.status_code == 200
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
"path,expected_status,expected_keys",
|
||||
[
|
||||
# Valid request
|
||||
# Instance level rules
|
||||
(
|
||||
"/-/rules.json?action=view-instance",
|
||||
200,
|
||||
{"action", "items", "total", "page"},
|
||||
),
|
||||
# Database level rules
|
||||
(
|
||||
"/-/rules.json?action=view-database",
|
||||
200,
|
||||
{"action", "items", "total", "page"},
|
||||
),
|
||||
# Table level rules
|
||||
(
|
||||
"/-/rules.json?action=view-table",
|
||||
200,
|
||||
{"action", "items", "total", "page"},
|
||||
),
|
||||
# Missing action parameter
|
||||
("/-/rules.json", 400, {"error"}),
|
||||
# Invalid action
|
||||
|
|
@ -361,7 +210,7 @@ async def test_rules_json_requires_permissions_debug(ds_with_permissions):
|
|||
async def test_rules_json_basic(
|
||||
ds_with_permissions, path, expected_status, expected_keys
|
||||
):
|
||||
# Use debugger user who has permissions-debug
|
||||
# Use root actor for rules endpoint (requires permissions-debug)
|
||||
response = await ds_with_permissions.client.get(
|
||||
path,
|
||||
cookies={"ds_actor": ds_with_permissions.client.actor_cookie({"id": "root"})},
|
||||
|
|
@ -396,104 +245,71 @@ async def test_rules_json_response_structure(ds_with_permissions):
|
|||
assert "parent" in item
|
||||
assert "child" in item
|
||||
assert "resource" in item
|
||||
assert "allow" in item # Important: should include allow field
|
||||
assert "allow" in item
|
||||
assert "reason" in item
|
||||
assert "source_plugin" in item
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_rules_json_includes_both_allow_and_deny(ds_with_permissions):
|
||||
"""Test that /-/rules.json includes both allow and deny rules."""
|
||||
async def test_rules_json_includes_all_rules(ds_with_permissions):
|
||||
"""Test that /-/rules.json includes both allowed and denied resources."""
|
||||
# Root user should see rules for everything
|
||||
response = await ds_with_permissions.client.get(
|
||||
"/-/rules.json?action=view-database",
|
||||
"/-/rules.json?action=view-table",
|
||||
cookies={"ds_actor": ds_with_permissions.client.actor_cookie({"id": "root"})},
|
||||
)
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
|
||||
# Check that items have the allow field
|
||||
assert isinstance(data["items"], list)
|
||||
if data["items"]:
|
||||
# Verify allow field exists and is 0 or 1
|
||||
for item in data["items"]:
|
||||
assert "allow" in item
|
||||
assert item["allow"] in (0, 1)
|
||||
# Should have items (root has global allow)
|
||||
assert len(data["items"]) > 0
|
||||
|
||||
# Each item should have allow field (0 or 1)
|
||||
for item in data["items"]:
|
||||
assert "allow" in item
|
||||
assert item["allow"] in [0, 1]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
"page,page_size",
|
||||
[
|
||||
(1, 10),
|
||||
(2, 50),
|
||||
(1, 200), # max page size
|
||||
],
|
||||
)
|
||||
async def test_rules_json_pagination(ds_with_permissions, page, page_size):
|
||||
"""Test pagination parameters."""
|
||||
response = await ds_with_permissions.client.get(
|
||||
f"/-/rules.json?action=view-instance&page={page}&page_size={page_size}",
|
||||
cookies={"ds_actor": ds_with_permissions.client.actor_cookie({"id": "root"})},
|
||||
)
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
assert data["page"] == page
|
||||
assert data["page_size"] == min(page_size, 200) # Capped at 200
|
||||
async def test_rules_json_pagination():
|
||||
"""Test that /-/rules.json pagination works."""
|
||||
ds = Datasette()
|
||||
ds.root_enabled = True
|
||||
await ds.invoke_startup()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
"params,expected_status",
|
||||
[
|
||||
("page=0", 400), # page must be >= 1
|
||||
("page=-1", 400),
|
||||
("page_size=0", 400), # page_size must be >= 1
|
||||
("page_size=-1", 400),
|
||||
("page=abc", 400), # page must be integer
|
||||
("page_size=xyz", 400), # page_size must be integer
|
||||
],
|
||||
)
|
||||
async def test_rules_json_pagination_errors(
|
||||
ds_with_permissions, params, expected_status
|
||||
):
|
||||
"""Test pagination error handling."""
|
||||
response = await ds_with_permissions.client.get(
|
||||
f"/-/rules.json?action=view-instance&{params}",
|
||||
cookies={"ds_actor": ds_with_permissions.client.actor_cookie({"id": "root"})},
|
||||
)
|
||||
assert response.status_code == expected_status
|
||||
|
||||
|
||||
# Test that HTML endpoints return HTML (not JSON) when accessed without .json
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
"path,needs_debug",
|
||||
[
|
||||
("/-/check", False),
|
||||
("/-/check?action=view-instance", False),
|
||||
("/-/allowed", False),
|
||||
("/-/allowed?action=view-instance", False),
|
||||
("/-/rules", True),
|
||||
("/-/rules?action=view-instance", True),
|
||||
],
|
||||
)
|
||||
async def test_html_endpoints_return_html(ds_with_permissions, path, needs_debug):
|
||||
"""Test that endpoints without .json extension return HTML."""
|
||||
if needs_debug:
|
||||
# Rules endpoint requires permissions-debug
|
||||
response = await ds_with_permissions.client.get(
|
||||
path,
|
||||
cookies={
|
||||
"ds_actor": ds_with_permissions.client.actor_cookie({"id": "root"})
|
||||
},
|
||||
# Create some tables
|
||||
db = ds.add_memory_database("test")
|
||||
for i in range(5):
|
||||
await db.execute_write(
|
||||
f"CREATE TABLE IF NOT EXISTS table{i:02d} (id INTEGER PRIMARY KEY)"
|
||||
)
|
||||
else:
|
||||
response = await ds_with_permissions.client.get(path)
|
||||
await ds.refresh_schemas()
|
||||
|
||||
# Test basic pagination structure - just verify it returns paginated results
|
||||
response = await ds.client.get(
|
||||
"/-/rules.json?action=view-table&page_size=2&page=1",
|
||||
cookies={"ds_actor": ds.client.actor_cookie({"id": "root"})},
|
||||
)
|
||||
assert response.status_code == 200
|
||||
assert "text/html" in response.headers["content-type"]
|
||||
# Check for HTML structure
|
||||
text = response.text
|
||||
assert "<!DOCTYPE html>" in text or "<html" in text
|
||||
data = response.json()
|
||||
assert data["page"] == 1
|
||||
assert data["page_size"] == 2
|
||||
# Verify items is a list (may have fewer items than page_size if there aren't many rules)
|
||||
assert isinstance(data["items"], list)
|
||||
assert "total" in data
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_rules_json_with_actor(ds_with_permissions):
|
||||
"""Test /-/rules.json includes actor information."""
|
||||
# Use root actor (rules endpoint requires permissions-debug)
|
||||
response = await ds_with_permissions.client.get(
|
||||
"/-/rules.json?action=view-table",
|
||||
cookies={"ds_actor": ds_with_permissions.client.actor_cookie({"id": "root"})},
|
||||
)
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
assert data["actor_id"] == "root"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
@ -502,8 +318,7 @@ async def test_root_user_respects_settings_deny():
|
|||
Test for issue #2509: Settings-based deny rules should override root user privileges.
|
||||
|
||||
When a database has `allow: false` in settings, the root user should NOT see
|
||||
that database in /-/allowed.json?action=view-database, even though root normally
|
||||
has all permissions.
|
||||
that database in /-/allowed.json?action=view-database.
|
||||
"""
|
||||
ds = Datasette(
|
||||
config={
|
||||
|
|
@ -518,7 +333,7 @@ async def test_root_user_respects_settings_deny():
|
|||
await ds.invoke_startup()
|
||||
ds.add_memory_database("content")
|
||||
|
||||
# Root user should NOT see the content database because settings deny it
|
||||
# Root user should NOT see the denied database
|
||||
response = await ds.client.get(
|
||||
"/-/allowed.json?action=view-database",
|
||||
cookies={"ds_actor": ds.client.actor_cookie({"id": "root"})},
|
||||
|
|
@ -575,3 +390,78 @@ async def test_root_user_respects_settings_deny_tables():
|
|||
f"Root user should not see tables from 'content' database when settings deny it, "
|
||||
f"but found: {content_tables}"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_execute_sql_requires_view_database():
|
||||
"""
|
||||
Test for issue #2527: execute-sql permission should require view-database permission.
|
||||
|
||||
A user who has execute-sql permission but not view-database permission should not
|
||||
be able to execute SQL on that database.
|
||||
"""
|
||||
from datasette.permissions import PermissionSQL
|
||||
from datasette.plugins import pm
|
||||
from datasette import hookimpl
|
||||
|
||||
class TestPermissionPlugin:
|
||||
__name__ = "TestPermissionPlugin"
|
||||
|
||||
@hookimpl
|
||||
def permission_resources_sql(self, datasette, actor, action):
|
||||
if actor is None or actor.get("id") != "test_user":
|
||||
return []
|
||||
|
||||
if action == "execute-sql":
|
||||
# Grant execute-sql on the "secret" database
|
||||
return PermissionSQL(
|
||||
source="test_plugin",
|
||||
sql="SELECT 'secret' AS parent, NULL AS child, 1 AS allow, 'can execute sql' AS reason",
|
||||
params={},
|
||||
)
|
||||
elif action == "view-database":
|
||||
# Deny view-database on the "secret" database
|
||||
return PermissionSQL(
|
||||
source="test_plugin",
|
||||
sql="SELECT 'secret' AS parent, NULL AS child, 0 AS allow, 'cannot view db' AS reason",
|
||||
params={},
|
||||
)
|
||||
|
||||
return []
|
||||
|
||||
plugin = TestPermissionPlugin()
|
||||
pm.register(plugin, name="test_plugin")
|
||||
|
||||
try:
|
||||
ds = Datasette()
|
||||
await ds.invoke_startup()
|
||||
ds.add_memory_database("secret")
|
||||
await ds.refresh_schemas()
|
||||
|
||||
# User should NOT have execute-sql permission because view-database is denied
|
||||
response = await ds.client.get(
|
||||
"/-/allowed.json?action=execute-sql",
|
||||
cookies={"ds_actor": ds.client.actor_cookie({"id": "test_user"})},
|
||||
)
|
||||
assert response.status_code == 200
|
||||
data = response.json()
|
||||
|
||||
# The "secret" database should NOT be in the allowed list for execute-sql
|
||||
allowed_databases = [item["parent"] for item in data["items"]]
|
||||
assert "secret" not in allowed_databases, (
|
||||
f"User should not have execute-sql permission without view-database, "
|
||||
f"but found 'secret' in: {allowed_databases}"
|
||||
)
|
||||
|
||||
# Also verify that attempting to execute SQL on the database is denied
|
||||
# (may be 403 or 302 redirect to login/error page depending on middleware)
|
||||
response = await ds.client.get(
|
||||
"/secret?sql=SELECT+1",
|
||||
cookies={"ds_actor": ds.client.actor_cookie({"id": "test_user"})},
|
||||
)
|
||||
assert response.status_code in (302, 403), (
|
||||
f"Expected 302 or 403 when trying to execute SQL without view-database permission, "
|
||||
f"but got {response.status_code}"
|
||||
)
|
||||
finally:
|
||||
pm.unregister(plugin)
|
||||
|
|
|
|||
|
|
@ -28,10 +28,10 @@ def plugin_allow_all_for_user(user: str) -> Callable[[str], PermissionSQL]:
|
|||
"allow_all",
|
||||
"""
|
||||
SELECT NULL AS parent, NULL AS child, 1 AS allow,
|
||||
'global allow for ' || :user || ' on ' || :action AS reason
|
||||
WHERE :actor_id = :user
|
||||
'global allow for ' || :allow_all_user || ' on ' || :allow_all_action AS reason
|
||||
WHERE :actor_id = :allow_all_user
|
||||
""",
|
||||
{"user": user, "action": action},
|
||||
{"allow_all_user": user, "allow_all_action": action},
|
||||
)
|
||||
|
||||
return provider
|
||||
|
|
@ -44,11 +44,16 @@ def plugin_deny_specific_table(
|
|||
return PermissionSQL(
|
||||
"deny_specific_table",
|
||||
"""
|
||||
SELECT :parent AS parent, :child AS child, 0 AS allow,
|
||||
'deny ' || :parent || '/' || :child || ' for ' || :user || ' on ' || :action AS reason
|
||||
WHERE :actor_id = :user
|
||||
SELECT :deny_specific_table_parent AS parent, :deny_specific_table_child AS child, 0 AS allow,
|
||||
'deny ' || :deny_specific_table_parent || '/' || :deny_specific_table_child || ' for ' || :deny_specific_table_user || ' on ' || :deny_specific_table_action AS reason
|
||||
WHERE :actor_id = :deny_specific_table_user
|
||||
""",
|
||||
{"parent": parent, "child": child, "user": user, "action": action},
|
||||
{
|
||||
"deny_specific_table_parent": parent,
|
||||
"deny_specific_table_child": child,
|
||||
"deny_specific_table_user": user,
|
||||
"deny_specific_table_action": action,
|
||||
},
|
||||
)
|
||||
|
||||
return provider
|
||||
|
|
@ -59,10 +64,13 @@ def plugin_org_policy_deny_parent(parent: str) -> Callable[[str], PermissionSQL]
|
|||
return PermissionSQL(
|
||||
"org_policy_parent_deny",
|
||||
"""
|
||||
SELECT :parent AS parent, NULL AS child, 0 AS allow,
|
||||
'org policy: parent ' || :parent || ' denied on ' || :action AS reason
|
||||
SELECT :org_policy_parent_deny_parent AS parent, NULL AS child, 0 AS allow,
|
||||
'org policy: parent ' || :org_policy_parent_deny_parent || ' denied on ' || :org_policy_parent_deny_action AS reason
|
||||
""",
|
||||
{"parent": parent, "action": action},
|
||||
{
|
||||
"org_policy_parent_deny_parent": parent,
|
||||
"org_policy_parent_deny_action": action,
|
||||
},
|
||||
)
|
||||
|
||||
return provider
|
||||
|
|
@ -75,11 +83,15 @@ def plugin_allow_parent_for_user(
|
|||
return PermissionSQL(
|
||||
"allow_parent",
|
||||
"""
|
||||
SELECT :parent AS parent, NULL AS child, 1 AS allow,
|
||||
'allow full parent for ' || :user || ' on ' || :action AS reason
|
||||
WHERE :actor_id = :user
|
||||
SELECT :allow_parent_parent AS parent, NULL AS child, 1 AS allow,
|
||||
'allow full parent for ' || :allow_parent_user || ' on ' || :allow_parent_action AS reason
|
||||
WHERE :actor_id = :allow_parent_user
|
||||
""",
|
||||
{"parent": parent, "user": user, "action": action},
|
||||
{
|
||||
"allow_parent_parent": parent,
|
||||
"allow_parent_user": user,
|
||||
"allow_parent_action": action,
|
||||
},
|
||||
)
|
||||
|
||||
return provider
|
||||
|
|
@ -92,11 +104,16 @@ def plugin_child_allow_for_user(
|
|||
return PermissionSQL(
|
||||
"allow_child",
|
||||
"""
|
||||
SELECT :parent AS parent, :child AS child, 1 AS allow,
|
||||
'allow child for ' || :user || ' on ' || :action AS reason
|
||||
WHERE :actor_id = :user
|
||||
SELECT :allow_child_parent AS parent, :allow_child_child AS child, 1 AS allow,
|
||||
'allow child for ' || :allow_child_user || ' on ' || :allow_child_action AS reason
|
||||
WHERE :actor_id = :allow_child_user
|
||||
""",
|
||||
{"parent": parent, "child": child, "user": user, "action": action},
|
||||
{
|
||||
"allow_child_parent": parent,
|
||||
"allow_child_child": child,
|
||||
"allow_child_user": user,
|
||||
"allow_child_action": action,
|
||||
},
|
||||
)
|
||||
|
||||
return provider
|
||||
|
|
@ -107,9 +124,9 @@ def plugin_root_deny_for_all() -> Callable[[str], PermissionSQL]:
|
|||
return PermissionSQL(
|
||||
"root_deny",
|
||||
"""
|
||||
SELECT NULL AS parent, NULL AS child, 0 AS allow, 'root deny for all on ' || :action AS reason
|
||||
SELECT NULL AS parent, NULL AS child, 0 AS allow, 'root deny for all on ' || :root_deny_action AS reason
|
||||
""",
|
||||
{"action": action},
|
||||
{"root_deny_action": action},
|
||||
)
|
||||
|
||||
return provider
|
||||
|
|
@ -122,22 +139,32 @@ def plugin_conflicting_same_child_rules(
|
|||
return PermissionSQL(
|
||||
"conflict_child_allow",
|
||||
"""
|
||||
SELECT :parent AS parent, :child AS child, 1 AS allow,
|
||||
'team grant at child for ' || :user || ' on ' || :action AS reason
|
||||
WHERE :actor_id = :user
|
||||
SELECT :conflict_child_allow_parent AS parent, :conflict_child_allow_child AS child, 1 AS allow,
|
||||
'team grant at child for ' || :conflict_child_allow_user || ' on ' || :conflict_child_allow_action AS reason
|
||||
WHERE :actor_id = :conflict_child_allow_user
|
||||
""",
|
||||
{"parent": parent, "child": child, "user": user, "action": action},
|
||||
{
|
||||
"conflict_child_allow_parent": parent,
|
||||
"conflict_child_allow_child": child,
|
||||
"conflict_child_allow_user": user,
|
||||
"conflict_child_allow_action": action,
|
||||
},
|
||||
)
|
||||
|
||||
def deny_provider(action: str) -> PermissionSQL:
|
||||
return PermissionSQL(
|
||||
"conflict_child_deny",
|
||||
"""
|
||||
SELECT :parent AS parent, :child AS child, 0 AS allow,
|
||||
'exception deny at child for ' || :user || ' on ' || :action AS reason
|
||||
WHERE :actor_id = :user
|
||||
SELECT :conflict_child_deny_parent AS parent, :conflict_child_deny_child AS child, 0 AS allow,
|
||||
'exception deny at child for ' || :conflict_child_deny_user || ' on ' || :conflict_child_deny_action AS reason
|
||||
WHERE :actor_id = :conflict_child_deny_user
|
||||
""",
|
||||
{"parent": parent, "child": child, "user": user, "action": action},
|
||||
{
|
||||
"conflict_child_deny_parent": parent,
|
||||
"conflict_child_deny_child": child,
|
||||
"conflict_child_deny_user": user,
|
||||
"conflict_child_deny_action": action,
|
||||
},
|
||||
)
|
||||
|
||||
return [allow_provider, deny_provider]
|
||||
|
|
@ -153,14 +180,17 @@ def plugin_allow_all_for_action(
|
|||
NO_RULES_SQL,
|
||||
{},
|
||||
)
|
||||
source_name = f"allow_all_{allowed_action}"
|
||||
# Sanitize parameter names by replacing hyphens with underscores
|
||||
param_prefix = source_name.replace("-", "_")
|
||||
return PermissionSQL(
|
||||
f"allow_all_{allowed_action}",
|
||||
"""
|
||||
source_name,
|
||||
f"""
|
||||
SELECT NULL AS parent, NULL AS child, 1 AS allow,
|
||||
'global allow for ' || :user || ' on ' || :action AS reason
|
||||
WHERE :actor_id = :user
|
||||
'global allow for ' || :{param_prefix}_user || ' on ' || :{param_prefix}_action AS reason
|
||||
WHERE :actor_id = :{param_prefix}_user
|
||||
""",
|
||||
{"user": user, "action": action},
|
||||
{f"{param_prefix}_user": user, f"{param_prefix}_action": action},
|
||||
)
|
||||
|
||||
return provider
|
||||
|
|
@ -582,14 +612,20 @@ async def test_multiple_plugins_with_own_parameters(db):
|
|||
)
|
||||
|
||||
# Both plugins should contribute results with their parameters successfully bound
|
||||
plugin_one_rows = [r for r in rows if r.get("reason") and "Plugin one" in r["reason"]]
|
||||
plugin_two_rows = [r for r in rows if r.get("reason") and "Plugin two" in r["reason"]]
|
||||
plugin_one_rows = [
|
||||
r for r in rows if r.get("reason") and "Plugin one" in r["reason"]
|
||||
]
|
||||
plugin_two_rows = [
|
||||
r for r in rows if r.get("reason") and "Plugin two" in r["reason"]
|
||||
]
|
||||
|
||||
assert len(plugin_one_rows) > 0, "Plugin one should contribute rules"
|
||||
assert len(plugin_two_rows) > 0, "Plugin two should contribute rules"
|
||||
|
||||
# Verify each plugin's parameters were successfully bound in the SQL
|
||||
assert any("value1" in r.get("reason", "") for r in plugin_one_rows), \
|
||||
"Plugin one's :plugin1_param should be bound"
|
||||
assert any("value2" in r.get("reason", "") for r in plugin_two_rows), \
|
||||
"Plugin two's :plugin2_param should be bound"
|
||||
assert any(
|
||||
"value1" in r.get("reason", "") for r in plugin_one_rows
|
||||
), "Plugin one's :plugin1_param should be bound"
|
||||
assert any(
|
||||
"value2" in r.get("reason", "") for r in plugin_two_rows
|
||||
), "Plugin two's :plugin2_param should be bound"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue