mirror of
https://github.com/simonw/datasette.git
synced 2026-09-11 02:54:17 +02:00
Limit derived-table permissions to one source hop
Simplify the solution to 5de0c1724e - avoid contextvar.
This commit is contained in:
parent
f70edbfa60
commit
92c7d4b608
9 changed files with 235 additions and 64 deletions
|
|
@ -308,12 +308,6 @@ DEFAULT_NOT_SET = object()
|
|||
|
||||
ResourcesSQL = collections.namedtuple("ResourcesSQL", ("sql", "params"))
|
||||
|
||||
# Tracks recursive view-table inheritance checks and fails closed if a
|
||||
# malicious or malformed schema declares a cycle of FTS content tables.
|
||||
_derived_permission_stack = contextvars.ContextVar(
|
||||
"derived_permission_stack", default=()
|
||||
)
|
||||
|
||||
|
||||
def _permission_cache_key(actor, action, parent, child):
|
||||
# Key on the full serialized actor so actors differing in any field
|
||||
|
|
@ -1762,6 +1756,26 @@ class Datasette:
|
|||
)
|
||||
return ResourcesSQL(sql, params)
|
||||
|
||||
async def _allowed_derived_table_source(
|
||||
self, database, source, *, actor, dependencies
|
||||
):
|
||||
"""Check an immediate source, denying sources that are themselves derived."""
|
||||
if any(
|
||||
TableResource.normalize_child(table)
|
||||
== TableResource.normalize_child(source)
|
||||
for table in dependencies
|
||||
):
|
||||
return False
|
||||
# The source has no dependency in this map. Evaluate its own permission
|
||||
# and prerequisites without starting another dependency check.
|
||||
verdicts = await self._allowed_many(
|
||||
actions=["view-table"],
|
||||
resource=TableResource(database, source),
|
||||
actor=actor,
|
||||
check_derived=False,
|
||||
)
|
||||
return verdicts["view-table"]
|
||||
|
||||
async def _apply_derived_table_permissions_to_sql(
|
||||
self,
|
||||
sql,
|
||||
|
|
@ -1776,12 +1790,17 @@ class Datasette:
|
|||
if parent in self.databases
|
||||
else ([] if parent is not None else list(self.databases.items()))
|
||||
)
|
||||
dependency_maps = await asyncio.gather(
|
||||
*(db.derived_table_dependencies() for _, db in databases)
|
||||
dependency_maps = dict(
|
||||
zip(
|
||||
(name for name, _ in databases),
|
||||
await asyncio.gather(
|
||||
*(db.derived_table_dependencies() for _, db in databases)
|
||||
),
|
||||
)
|
||||
)
|
||||
dependencies = [
|
||||
(database_name, child, source)
|
||||
for (database_name, _), dependency_map in zip(databases, dependency_maps)
|
||||
for database_name, dependency_map in dependency_maps.items()
|
||||
for child, source in dependency_map.items()
|
||||
]
|
||||
if not dependencies:
|
||||
|
|
@ -1792,10 +1811,11 @@ class Datasette:
|
|||
)
|
||||
actor_verdicts = await asyncio.gather(
|
||||
*(
|
||||
self.allowed(
|
||||
action="view-table",
|
||||
resource=TableResource(database_name, source),
|
||||
self._allowed_derived_table_source(
|
||||
database_name,
|
||||
source,
|
||||
actor=actor,
|
||||
dependencies=dependency_maps[database_name],
|
||||
)
|
||||
for database_name, source in sources
|
||||
)
|
||||
|
|
@ -1806,10 +1826,11 @@ class Datasette:
|
|||
if include_is_private:
|
||||
anonymous_verdicts = await asyncio.gather(
|
||||
*(
|
||||
self.allowed(
|
||||
action="view-table",
|
||||
resource=TableResource(database_name, source),
|
||||
self._allowed_derived_table_source(
|
||||
database_name,
|
||||
source,
|
||||
actor=None,
|
||||
dependencies=dependency_maps[database_name],
|
||||
)
|
||||
for database_name, source in sources
|
||||
)
|
||||
|
|
@ -2066,6 +2087,12 @@ ORDER BY allowed.parent, allowed.child
|
|||
)
|
||||
# {"edit-schema": True, "drop-table": True, "insert-row": False}
|
||||
"""
|
||||
return await self._allowed_many(
|
||||
actions=actions, resource=resource, actor=actor, check_derived=True
|
||||
)
|
||||
|
||||
async def _allowed_many(self, *, actions, resource, actor, check_derived):
|
||||
"""Evaluate permissions, optionally applying the one-hop source policy."""
|
||||
from datasette.permissions import (
|
||||
_permission_check_cache,
|
||||
_skip_permission_checks,
|
||||
|
|
@ -2119,40 +2146,27 @@ ORDER BY allowed.parent, allowed.child
|
|||
child=child,
|
||||
)
|
||||
|
||||
# Automatically derived implementation tables cannot be more visible
|
||||
# than the logical/content table they expose. Keep the requested
|
||||
# table's own permission too: either side can make access private.
|
||||
if (
|
||||
"view-table" in to_check
|
||||
check_derived
|
||||
and "view-table" in to_check
|
||||
and raw.get("view-table")
|
||||
and isinstance(resource, TableResource)
|
||||
and parent in self.databases
|
||||
):
|
||||
dependency = await self.databases[parent].derived_table_dependencies()
|
||||
dependency = next(
|
||||
dependencies = await self.databases[parent].derived_table_dependencies()
|
||||
source = next(
|
||||
(
|
||||
source
|
||||
for table, source in dependency.items()
|
||||
for table, source in dependencies.items()
|
||||
if TableResource.normalize_child(table)
|
||||
== TableResource.normalize_child(child)
|
||||
),
|
||||
None,
|
||||
)
|
||||
if dependency is not None:
|
||||
stack = _derived_permission_stack.get()
|
||||
dependency_key = (parent, dependency)
|
||||
if dependency_key in stack or dependency == child:
|
||||
raw["view-table"] = False
|
||||
else:
|
||||
token = _derived_permission_stack.set(stack + ((parent, child),))
|
||||
try:
|
||||
raw["view-table"] = await self.allowed(
|
||||
action="view-table",
|
||||
resource=TableResource(parent, dependency),
|
||||
actor=actor,
|
||||
)
|
||||
finally:
|
||||
_derived_permission_stack.reset(token)
|
||||
if source is not None:
|
||||
raw["view-table"] = await self._allowed_derived_table_source(
|
||||
parent, source, actor=actor, dependencies=dependencies
|
||||
)
|
||||
|
||||
def resolve(name):
|
||||
# final verdict = own rules AND verdict of also_requires chain
|
||||
|
|
|
|||
|
|
@ -170,14 +170,14 @@ def sqlite_derived_table_dependencies(
|
|||
does not report which virtual table owns a shadow table or which table is
|
||||
named by an FTS ``content=`` option. Derive those relationships from
|
||||
``sqlite_master`` DDL and the documented shadow-table suffixes.
|
||||
|
||||
Database errors propagate: failed discovery must not be mistaken for an
|
||||
empty dependency map and cached as permission to skip inheritance.
|
||||
"""
|
||||
schema_table = _sqlite_schema_table(schema)
|
||||
try:
|
||||
rows = conn.execute(
|
||||
f"select name, sql from {schema_table} where type = 'table'"
|
||||
).fetchall()
|
||||
except sqlite3.DatabaseError:
|
||||
return {}
|
||||
rows = conn.execute(
|
||||
f"select name, sql from {schema_table} where type = 'table'"
|
||||
).fetchall()
|
||||
|
||||
table_names = {row[0] for row in rows}
|
||||
# SQLite identifiers fold ASCII letters only.
|
||||
|
|
@ -212,7 +212,7 @@ def sqlite_derived_table_dependencies(
|
|||
if source
|
||||
else None
|
||||
)
|
||||
# An unresolved source uses the existing cycle guard to deny access.
|
||||
# An unresolved source is itself derived, so the one-hop policy denies it.
|
||||
dependencies[virtual_table] = source or virtual_table
|
||||
|
||||
return dependencies
|
||||
|
|
|
|||
|
|
@ -1382,10 +1382,11 @@ view-table
|
|||
|
||||
Actor is allowed to view a table (or view) page, e.g. https://latest.datasette.io/fixtures/complex_foreign_keys
|
||||
|
||||
FTS vocabulary tables (``fts5vocab`` and ``fts4aux``) also require access to
|
||||
their source FTS table and, for an external-content index, its content table.
|
||||
The vocabulary table's own permission rules still apply. Vocabulary tables
|
||||
whose source cannot be resolved within the same SQLite schema are denied.
|
||||
Derived implementation tables require access to their immediate source: FTS and RTree shadow tables require access to their virtual table, external-content FTS tables require access to their content table, and FTS vocabulary tables (``fts5vocab`` and ``fts4aux``) require access to their FTS table. The derived table's own permission rules also apply.
|
||||
|
||||
Access is always denied if the source table is itself derived, or if a vocabulary table's source cannot be identified.
|
||||
|
||||
The same rules apply to individual permission checks and table listings, including whether they are private. If a database error prevents dependency discovery, the check or listing fails with an error instead of ignoring the dependencies. Failed discovery results are not cached, so later checks can retry.
|
||||
|
||||
``resource`` - ``datasette.resources.TableResource(database, table)``
|
||||
``database`` is the name of the database (string)
|
||||
|
|
|
|||
|
|
@ -261,6 +261,15 @@ If you run ``datasette plugins --all`` it will include default plugins that ship
|
|||
"permission_resources_sql"
|
||||
]
|
||||
},
|
||||
{
|
||||
"name": "datasette.default_permissions.sqlite_statistics",
|
||||
"static": false,
|
||||
"templates": false,
|
||||
"version": null,
|
||||
"hooks": [
|
||||
"permission_resources_sql"
|
||||
]
|
||||
},
|
||||
{
|
||||
"name": "datasette.default_permissions.tokens",
|
||||
"static": false,
|
||||
|
|
|
|||
|
|
@ -102,14 +102,11 @@ async def test_database_page(ds_client):
|
|||
"tags",
|
||||
}
|
||||
|
||||
# Expected hidden tables
|
||||
# The external-content index is visible, but its shadow tables need a
|
||||
# second dependency hop and are excluded by the one-hop permission policy.
|
||||
expected_hidden_tables = {
|
||||
"no_primary_key",
|
||||
"searchable_fts",
|
||||
"searchable_fts_config",
|
||||
"searchable_fts_data",
|
||||
"searchable_fts_docsize",
|
||||
"searchable_fts_idx",
|
||||
}
|
||||
|
||||
# Verify all expected tables exist
|
||||
|
|
|
|||
|
|
@ -1,7 +1,121 @@
|
|||
import pytest
|
||||
|
||||
from datasette import hookimpl
|
||||
from datasette.app import Datasette
|
||||
from datasette.permissions import Action, PermissionSQL, _permission_check_cache
|
||||
from datasette.resources import DatabaseResource, TableResource
|
||||
from datasette.utils.sqlite import sqlite3, sqlite_derived_table_dependencies
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("fts_module", ["fts4", "fts5"])
|
||||
@pytest.mark.parametrize("actor", [None, {"id": "root"}], ids=["anonymous", "root"])
|
||||
async def test_derived_permissions_allow_one_hop_but_deny_nested_sources(
|
||||
fts_module, actor
|
||||
):
|
||||
class InspectPlugin:
|
||||
@hookimpl
|
||||
def register_actions(self):
|
||||
return [
|
||||
Action(
|
||||
name="inspect-derived",
|
||||
description="Inspect a table",
|
||||
resource_class=TableResource,
|
||||
also_requires="view-table",
|
||||
)
|
||||
]
|
||||
|
||||
@hookimpl
|
||||
def permission_resources_sql(self, action):
|
||||
if action == "inspect-derived":
|
||||
return PermissionSQL(
|
||||
sql="SELECT NULL AS parent, NULL AS child, 1 AS allow, 'inspect allowed' AS reason"
|
||||
)
|
||||
|
||||
ds = Datasette(memory=True)
|
||||
ds.pm.register(InspectPlugin(), name="inspect-derived-test")
|
||||
db = ds.add_memory_database(
|
||||
f"derived_one_hop_{fts_module}_{actor is not None}", name="data"
|
||||
)
|
||||
await db.execute_write("create table Documents (body text)")
|
||||
await db.execute_write(
|
||||
f"create virtual table Search using {fts_module}(body, content='Documents')"
|
||||
)
|
||||
await db.execute_write(
|
||||
f"create virtual table Nested using {fts_module}(body, content='sEaRcH')"
|
||||
)
|
||||
await ds.invoke_startup()
|
||||
token = _permission_check_cache.set({})
|
||||
try:
|
||||
# Both direct permissions are allowed, but a derived source makes its
|
||||
# dependent unavailable even to an actor who can view the whole chain.
|
||||
# Check and cache Search first so its cached grant cannot grant Nested.
|
||||
for table, expected in (
|
||||
("Documents", True),
|
||||
("Search", True),
|
||||
("Nested", False),
|
||||
("Search_docsize", False),
|
||||
):
|
||||
for spelling in (table, table.upper(), table.lower()):
|
||||
assert await ds.allowed_many(
|
||||
actions=["view-table", "inspect-derived"],
|
||||
resource=TableResource("data", spelling),
|
||||
actor=actor,
|
||||
) == {"view-table": expected, "inspect-derived": expected}
|
||||
|
||||
page = await ds.allowed_resources(
|
||||
"view-table", actor, parent="data", include_is_private=True, limit=1000
|
||||
)
|
||||
allowed = {resource.child for resource in page.resources}
|
||||
assert {"Documents", "Search"}.issubset(allowed)
|
||||
assert "Nested" not in allowed
|
||||
assert "Search_docsize" not in allowed
|
||||
finally:
|
||||
_permission_check_cache.reset(token)
|
||||
ds.pm.unregister(name="inspect-derived-test")
|
||||
ds.close()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("listing", [False, True], ids=["individual", "listing"])
|
||||
async def test_derived_permission_discovery_error_is_retried(monkeypatch, listing):
|
||||
ds = Datasette(memory=True)
|
||||
db = ds.add_memory_database(f"derived_discovery_error_{listing}", name="data")
|
||||
await db.execute_write("create table documents (id integer primary key)")
|
||||
await ds.invoke_startup()
|
||||
|
||||
class UnavailableSchema:
|
||||
def execute(self, sql):
|
||||
raise sqlite3.DatabaseError("schema temporarily unavailable")
|
||||
|
||||
async def check():
|
||||
if listing:
|
||||
return await ds.allowed_resources("view-table", parent="data")
|
||||
return await ds.allowed(
|
||||
action="view-table", resource=TableResource("data", "documents")
|
||||
)
|
||||
|
||||
token = _permission_check_cache.set({})
|
||||
try:
|
||||
with monkeypatch.context() as patch:
|
||||
patch.setattr(
|
||||
"datasette.database.sqlite_derived_table_dependencies",
|
||||
lambda conn: sqlite_derived_table_dependencies(UnavailableSchema()),
|
||||
)
|
||||
with pytest.raises(sqlite3.DatabaseError, match="schema temporarily"):
|
||||
await check()
|
||||
|
||||
# Failed discovery must not cache an empty map or a permission grant.
|
||||
assert db._cached_derived_table_dependencies is None
|
||||
assert not _permission_check_cache.get()
|
||||
result = await check()
|
||||
if listing:
|
||||
assert [resource.child for resource in result.resources] == ["documents"]
|
||||
else:
|
||||
assert result is True
|
||||
assert db._cached_derived_table_dependencies is not None
|
||||
finally:
|
||||
_permission_check_cache.reset(token)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
@ -243,8 +357,11 @@ async def test_derived_tables_propagate_private_flag_and_route_permissions():
|
|||
resource.child: resource for resource in actor_page.resources
|
||||
}
|
||||
derived_names = set(await db.derived_table_dependencies())
|
||||
assert derived_names.issubset(actor_resources)
|
||||
assert all(actor_resources[name].private for name in derived_names)
|
||||
assert "secret_fts" in actor_resources
|
||||
assert actor_resources["secret_fts"].private
|
||||
# Shadow tables depend on the already-derived external-content FTS
|
||||
# table, so they remain unavailable even to the permitted reader.
|
||||
assert not (derived_names - {"secret_fts"}).intersection(actor_resources)
|
||||
|
||||
anonymous_page = await ds.allowed_resources(
|
||||
"view-table", parent="data", limit=1000
|
||||
|
|
|
|||
|
|
@ -36,8 +36,10 @@ def test_homepage(app_client_two_attached_databases):
|
|||
h2 = soup.select("h2")[0]
|
||||
assert "extra database" == h2.text.strip()
|
||||
counts_p, links_p = h2.find_all_next("p")[:2]
|
||||
# Shadow tables of the external-content index are denied, so they do not
|
||||
# contribute to the table or row totals.
|
||||
assert (
|
||||
"2 rows in 1 table, 5 rows in 4 hidden tables, 1 view" == counts_p.text.strip()
|
||||
"2 rows in 1 table, 2 rows in 1 hidden table, 1 view" == counts_p.text.strip()
|
||||
)
|
||||
# We should only show visible, not hidden tables here:
|
||||
table_links = [
|
||||
|
|
|
|||
|
|
@ -20,7 +20,11 @@ from datasette.database import (
|
|||
_deliver_write_result,
|
||||
)
|
||||
from datasette.utils import Column
|
||||
from datasette.utils.sqlite import sqlite3, supports_returning
|
||||
from datasette.utils.sqlite import (
|
||||
sqlite3,
|
||||
sqlite_derived_table_dependencies,
|
||||
supports_returning,
|
||||
)
|
||||
|
||||
requires_sqlite_returning = pytest.mark.skipif(
|
||||
not supports_returning(), reason="SQLite does not support RETURNING"
|
||||
|
|
@ -39,6 +43,31 @@ async def test_execute(db):
|
|||
assert 15 == len(results)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_derived_dependency_cache_survives_failed_refresh(monkeypatch):
|
||||
ds = Datasette(memory=True)
|
||||
db = ds.add_memory_database(uuid.uuid4().hex, name="data")
|
||||
await db.derived_table_dependencies()
|
||||
previous_cache = db._cached_derived_table_dependencies
|
||||
await db.execute_write("create table dependency_cache_refresh (id integer)")
|
||||
|
||||
class UnavailableSchema:
|
||||
def execute(self, sql):
|
||||
raise sqlite3.DatabaseError("schema temporarily unavailable")
|
||||
|
||||
with monkeypatch.context() as patch:
|
||||
patch.setattr(
|
||||
"datasette.database.sqlite_derived_table_dependencies",
|
||||
lambda conn: sqlite_derived_table_dependencies(UnavailableSchema()),
|
||||
)
|
||||
with pytest.raises(sqlite3.DatabaseError, match="schema temporarily"):
|
||||
await db.derived_table_dependencies()
|
||||
assert db._cached_derived_table_dependencies == previous_cache
|
||||
|
||||
await db.derived_table_dependencies()
|
||||
assert db._cached_derived_table_dependencies[0] != previous_cache[0]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_results_first(db):
|
||||
assert None is (await db.execute("select * from facetable where pk > 100")).first()
|
||||
|
|
|
|||
|
|
@ -35,17 +35,20 @@ def test_vocabulary_dependency_identity(module, arguments, vocab_name):
|
|||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("module", ["fts5", "fts4"])
|
||||
@pytest.mark.parametrize("external_content", [False, True], ids=["one-hop", "two-hop"])
|
||||
@pytest.mark.parametrize(
|
||||
"source_allowed,vocab_allowed", [(False, True), (True, False), (True, True)]
|
||||
)
|
||||
async def test_vocabulary_transitive_permissions(module, source_allowed, vocab_allowed):
|
||||
async def test_vocabulary_immediate_source_permissions(
|
||||
module, external_content, source_allowed, vocab_allowed
|
||||
):
|
||||
ds = Datasette(
|
||||
memory=True,
|
||||
config={
|
||||
"databases": {
|
||||
"data": {
|
||||
"tables": {
|
||||
"documents": {
|
||||
"search": {
|
||||
"permissions": {
|
||||
"view-table": (
|
||||
{"id": "reader"} if source_allowed else False
|
||||
|
|
@ -60,9 +63,8 @@ async def test_vocabulary_transitive_permissions(module, source_allowed, vocab_a
|
|||
)
|
||||
db = ds.add_memory_database(uuid.uuid4().hex, name="data")
|
||||
await db.execute_write("create table documents(body text)")
|
||||
await db.execute_write(
|
||||
f"create virtual table search using {module}(body, content='documents')"
|
||||
)
|
||||
options = "body, content='documents'" if external_content else "body"
|
||||
await db.execute_write(f"create virtual table search using {module}({options})")
|
||||
definition = (
|
||||
"fts5vocab('SEARCH', 'row')" if module == "fts5" else "fts4aux('SEARCH')"
|
||||
)
|
||||
|
|
@ -70,7 +72,7 @@ async def test_vocabulary_transitive_permissions(module, source_allowed, vocab_a
|
|||
await ds.invoke_startup()
|
||||
try:
|
||||
actor = {"id": "reader"}
|
||||
expected = source_allowed and vocab_allowed
|
||||
expected = source_allowed and vocab_allowed and not external_content
|
||||
for name in ("words", "WORDS"):
|
||||
assert (
|
||||
await ds.allowed(
|
||||
|
|
@ -112,7 +114,7 @@ def test_cross_schema_vocabulary_is_unresolved(module, definition):
|
|||
conn.execute(f"create virtual table search using {module}(body)")
|
||||
conn.execute(f"create virtual table temp.words using {definition}")
|
||||
# Cross-schema ownership is not representable by the current map.
|
||||
# The self-dependency invokes the permission layer's cycle denial.
|
||||
# The source is itself derived, so the immediate-source policy denies it.
|
||||
assert (
|
||||
sqlite_derived_table_dependencies(conn, schema="temp")["words"] == "words"
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue