From ce2f5ea223f281ffb01a87c8ab3d4be9ec4f6473 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 6 Feb 2026 01:31:22 +0000 Subject: [PATCH 001/481] Deep code review of SQL permissions system Comprehensive analysis of the permission system introduced in 1.0a20 through 1.0a24, covering architecture, security, performance, and design concerns across 12 identified issues with prioritized recommendations. https://claude.ai/code/session_013EkyroQKPhcjdMbpHc9g4X --- permissions-notes.md | 337 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 337 insertions(+) create mode 100644 permissions-notes.md diff --git a/permissions-notes.md b/permissions-notes.md new file mode 100644 index 00000000..3b61a06b --- /dev/null +++ b/permissions-notes.md @@ -0,0 +1,337 @@ +# SQL Permissions System - Deep Code Review Notes + +## Overview + +The SQL permissions system was introduced in Datasette 1.0a20 and subsequently refined through 1.0a24. It replaces the older plugin hook-based `permission_allowed` system with a SQL-driven approach where all permission decisions are resolved by executing SQL queries against the internal SQLite database. + +Key commits: +- `95a1fef` (1.0a20): Initial introduction of `permissions.py`, `utils/permissions.py`, `default_permissions.py` +- `23a640d`: `--default-deny` option +- `d814e81`: `skip_permission_checks` context variable, `actions_sql.py` +- `0a92452`: Split `default_permissions.py` into a package with 7 modules +- `66d2a03`: Ruff lint fixes + +## Architecture Summary + +### Permission Check Flow + +``` +Request → Authentication → Action Check + ↓ + permission_resources_sql hook + ↓ + Multiple PermissionSQL objects collected + ↓ + UNION ALL into rules CTE + ↓ + Cascading evaluation: + child(2) → parent(1) → global(0) + DENY beats ALLOW at same level + ↓ + restriction_sql INTERSECT filtering + ↓ + Boolean result (or resource list) +``` + +### Two Code Paths + +1. **Single resource check** (`check_permission_for_resource` in `actions_sql.py:494-587`): Uses `ROW_NUMBER() OVER (PARTITION BY ...)` with ORDER BY depth to pick a winner. Used by `datasette.allowed()`. + +2. **All resources check** (`_build_single_action_sql` in `actions_sql.py:130-425`): Uses separate `child_lvl`, `parent_lvl`, `global_lvl` CTEs with `MAX(CASE ...)` aggregates, then a cascading CASE statement. Used by `datasette.allowed_resources()`. + +These two code paths implement the **same cascading logic** but with completely different SQL structures. + +### Key Files + +| File | Lines | Purpose | +|------|-------|---------| +| `datasette/permissions.py` | 210 | Core abstractions: `Resource`, `Action`, `PermissionSQL`, `SkipPermissions` | +| `datasette/resources.py` | 91 | `DatabaseResource`, `TableResource`, `QueryResource` | +| `datasette/utils/actions_sql.py` | 587 | SQL builders for `allowed_resources()` and `allowed()` | +| `datasette/utils/permissions.py` | 439 | Hook gathering, `resolve_permissions_from_catalog()` (3rd implementation) | +| `datasette/default_permissions/__init__.py` | 59 | Package init, re-exports, CSRF skip, canned_queries | +| `datasette/default_permissions/config.py` | 442 | `ConfigPermissionProcessor` - datasette.yaml rules | +| `datasette/default_permissions/defaults.py` | 70 | `DEFAULT_ALLOW_ACTIONS`, `default_allow_sql` | +| `datasette/default_permissions/restrictions.py` | 195 | Actor `_r` allowlist handling | +| `datasette/default_permissions/helpers.py` | 85 | `PermissionRowCollector`, action name variants | +| `datasette/default_permissions/root.py` | 29 | Root user global allow | +| `datasette/default_permissions/tokens.py` | 95 | Signed API token auth | + +--- + +## Findings + +### Tests: All Pass + +263 tests pass, 3 xpassed. Test files: +- `test_permissions.py` (largest, 1713 lines) +- `test_config_permission_rules.py` (163 lines) +- `test_utils_permissions.py` (612 lines) +- `test_permission_endpoints.py` (501 lines) +- `test_default_deny.py` (129 lines) +- `test_restriction_sql.py` +- `test_allowed_resources.py` +- `test_actions_sql.py` + +--- + +## Issues Found + +### ISSUE 1 (Design Concern): Root user blocked by `allow:` blocks that don't include "root" + +**Severity: Medium (by design per #2509, but potentially surprising UX)** + +When a table has an `allow:` block in config like: +```yaml +databases: + mydb: + tables: + secrets: + allow: + id: admin +``` + +The root user (--root) is **denied access** to that table. This happens because: + +1. `root_user_permissions_sql()` returns a global (NULL, NULL) ALLOW +2. `config_permissions_sql()` generates a child-level (mydb, secrets) DENY for actors not matching `{id: admin}` (root's id is "root", not "admin") +3. The cascading logic says child-level beats global-level + +**Observed behavior:** +``` +curl -b [root-cookies] /test_perms/secrets.json → 403 Forbidden +``` + +**Rules visible in /-/rules.json:** +```json +[ + {"parent": null, "child": null, "allow": 1, "reason": "root user"}, + {"parent": "test_perms", "child": "secrets", "allow": 0, "reason": "config deny allow..."} +] +``` + +**This is intentional per issue #2509**: `test_root_user_respects_settings_deny` in `test_permission_endpoints.py:355` explicitly asserts that config deny rules override root. The same logic applies to `allow: {id: admin}` - since root's id doesn't match, it becomes a deny. + +**However, this is a UX concern**: An admin starting Datasette with `--root` may reasonably expect full access. With `allow: {id: admin}`, the workaround is `allow: {id: [admin, root]}`, but with `allow: false` there is no config-based workaround. + +**Recommendation**: Document this clearly in `--root` documentation. Consider whether a future `--root-bypass-config` flag or equivalent would be useful for debugging scenarios. + +--- + +### ISSUE 2 (Design): Three separate implementations of cascading logic + +The cascading permission resolution (child > parent > global, deny beats allow) is implemented in three different places: + +1. **`actions_sql.py:_build_single_action_sql()`** (lines 246-384): Uses separate CTEs (`child_lvl`, `parent_lvl`, `global_lvl`) each doing `LEFT JOIN` + `GROUP BY` with `MAX()` aggregates, then a CASE cascade in `decisions`. + +2. **`actions_sql.py:check_permission_for_resource()`** (lines 555-587): Uses `ROW_NUMBER() OVER (PARTITION BY parent, child ORDER BY depth DESC, ...)` to pick a single winner. + +3. **`permissions.py:resolve_permissions_from_catalog()`** (lines 141-397): Yet another implementation using `ROW_NUMBER()` like #2 but with different structure, including massive SQL duplication when restriction_sql is present (the entire query is repeated in the restriction case). + +**Important note**: `resolve_permissions_from_catalog()` is **only used in tests** (`test_utils_permissions.py`), not in any production code path. This means it's a test-only implementation of the same logic, which could drift out of sync with the actual production implementations (#1 and #2). If the production SQL is changed, these tests might still pass on the old test-only implementation while production behavior changes. + +The two production paths (#1 and #2) implement the same cascading logic but with different SQL patterns. This is fragile - a logic change must be applied in both places. + +--- + +### ISSUE 3 (Code Quality): Massive SQL duplication in `resolve_permissions_from_catalog()` + +In `utils/permissions.py:256-391`, when `restriction_sqls` is present, the **entire CTE chain** (cands, rules, matched, ranked, winner) is duplicated - once for the main query and once for the restriction filtering. This results in ~135 lines of nearly identical SQL being emitted twice. + +The restriction-with-restrictions path generates SQL that embeds the full resolution query inside a `permitted_resources` CTE, then creates a `filtered` CTE, and then re-creates cands/rules/matched/ranked/winner *again* to get the full output columns. This could be simplified significantly. + +--- + +### ISSUE 4 (Code Quality): Global `_reason_id` counter in `PermissionSQL` + +`permissions.py:157` has a module-level `_reason_id` counter that increments forever: + +```python +_reason_id = 1 + +class PermissionSQL: + @classmethod + def allow(cls, reason, _allow=True): + global _reason_id + i = _reason_id + _reason_id += 1 + ... +``` + +This means: +- Every `PermissionSQL.allow()` or `.deny()` call increments a process-global counter +- In a long-running server, param keys grow: `:reason_1`, `:reason_2`, ..., `:reason_100000` +- Not thread-safe (though Python's GIL provides some protection) +- Makes SQL non-deterministic between requests (harder to cache or compare) +- The counter never resets + +This isn't a memory leak per se (the SQL is transient), but it's an unusual pattern. A better approach would be to use a per-call counter or deterministic naming. + +--- + +### ISSUE 5 (Security): `source_plugin` name injected into SQL without parameterization + +In three places, the plugin name is interpolated directly into SQL: + +```python +# actions_sql.py:185 +f"SELECT parent, child, allow, reason, '{permission_sql.source}' AS source_plugin FROM ..." + +# actions_sql.py:484 +f"SELECT parent, child, allow, reason, '{permission_sql.source}' AS source_plugin FROM ..." + +# permissions.py:121 +f"SELECT parent, child, allow, reason, '{p.source}' AS source_plugin FROM ..." +``` + +The `source` field comes from `_plugin_name_from_hookimpl()` which extracts the Python module name. While unlikely to contain SQL injection payloads in practice, a malicious plugin with a single-quote in its name could inject SQL. This should use parameterized values. + +--- + +### ISSUE 6 (Security): `QueryResource.resources_sql()` uses manual quote escaping + +In `resources.py:82-88`: +```python +db_escaped = db_name.replace("'", "''") +query_escaped = query_name.replace("'", "''") +selects.append(f"SELECT '{db_escaped}' AS parent, '{query_escaped}' AS child") +``` + +This manually escapes single quotes by doubling them instead of using parameterized queries. While the double-quote escape is the correct SQLite approach, parameterized queries would be safer and more robust. + +The limitation here is that `resources_sql()` returns a SQL string, not (SQL, params) - so the API would need to change to support parameterization. + +--- + +### ISSUE 7 (Performance): `include_is_private` doubles the permission SQL + +When `include_is_private=True` is used (which is the default for database and index page views), the entire permission resolution is run twice: +1. Once for the actual actor +2. Once for `actor=None` (anonymous) + +This generates separate `anon_rules`, `anon_child_lvl`, `anon_parent_lvl`, `anon_global_lvl`, and `anon_decisions` CTEs - effectively doubling the size and cost of the query. + +Looking at the trace output for an anonymous user viewing the database page, the view-table permission query with `include_is_private=True` was the slowest query at ~4.2ms. For authenticated users with many rules, this would be worse. + +**Optimization opportunity**: When the actor IS anonymous (`actor=None`), the `is_private` computation is trivially 0 for all allowed resources since the actor and anonymous actor are the same. This case could be short-circuited. + +--- + +### ISSUE 8 (Performance): Homepage counts ALL tables, not just visible ones + +In the trace for the homepage, `table_counts` queries are issued for ALL tables: +``` +select count(*) from [posts] limit 10001 -- visible to anon +select count(*) from [secrets] limit 10001 -- NOT visible to anon +select count(*) from [users] limit 10001 -- NOT visible to anon +``` + +The count results for `secrets` and `users` are computed but then discarded because those tables aren't in the allowed set. This is wasteful, especially with large tables. The count queries should only be issued for tables the user can actually see. + +--- + +### ISSUE 9 (Design): `allow:` blocks generate DENYs, not restrictions + +The current design converts `allow: {id: admin}` blocks into **deny** rules for non-matching actors and **allow** rules for matching actors. This means: + +```yaml +tables: + secrets: + allow: + id: admin +``` + +Generates two separate rules depending on the actor: +- For admin: `(test_perms, secrets, allow=1, "config allow...")` +- For everyone else: `(test_perms, secrets, allow=0, "config deny...")` + +The deny rule is emitted at the child level, which means it **cannot be overridden by any global or parent-level allow**. This is the root cause of Issue 1. + +A more nuanced approach might: +- Only emit allow rules from `allow:` blocks +- Use a separate "last-resort" deny mechanism that doesn't interfere with higher-priority allows +- Or use a "priority" system where root > config > defaults + +--- + +### ISSUE 10 (Design): No explicit deny mechanism for specific actors + +The system has `allow:` blocks to restrict access to specific actors, but there's no explicit `deny:` block in config to deny specific actors while allowing everyone else. The only way to deny a specific actor is through the permission resolution system's cascading logic, which is indirect. + +A `deny:` block could be useful: +```yaml +databases: + mydb: + deny: + id: malicious_bot +``` + +--- + +### ISSUE 11 (Design Gap): `also_requires` only supports one level + +The `Action` dataclass has `also_requires: str | None` which links one action to another (e.g., `execute-sql` requires `view-database`). This only supports one level of dependency. If action A requires B which requires C, the system doesn't automatically chain these. + +Currently, `also_requires` is handled explicitly in both `allowed()` (recursive call) and `build_allowed_resources_sql()` (INNER JOIN of two queries). The recursive call in `allowed()` would handle chains, but `build_allowed_resources_sql()` only handles one level. + +--- + +### ISSUE 12 (Observability): Permission reason tracking loses deny information + +When a permission check results in a deny, the `allowed()` method logs the result as `result=False` but doesn't capture the reason. The `check_permission_for_resource()` function only returns a boolean, discarding the reason and source plugin information. + +For debugging, it would be valuable to know *why* access was denied - especially for the root user scenario in Issue 1. + +--- + +## Positive Observations + +1. **Clean separation of concerns**: The `default_permissions/` package split is well-organized with each module having a clear, focused responsibility. + +2. **Parameterized SQL throughout**: All user-controlled values (actor_id, action names, database names, table names in PermissionRowCollector) use parameterized queries. The exceptions noted above (source_plugin, QueryResource) are edge cases. + +3. **Comprehensive test coverage**: 263 tests covering a wide range of scenarios including cascading logic, restrictions, config rules, default deny, and endpoints. + +4. **Debuggability**: The `/-/rules.json` and `/-/allowed.json` endpoints make it straightforward to understand why a permission decision was made. The trace system exposes the actual SQL executed. + +5. **Extension points**: The `permission_resources_sql` hook is well-designed for plugins to contribute rules. The `restriction_sql` mechanism for actor allowlists is elegant. + +6. **Pagination**: `allowed_resources()` supports keyset pagination, which is important for instances with many tables/databases. + +--- + +## Recommendations (Priority Order) + +### P1: Document root user config interaction (Issue 1) + +The `--root` flag documentation should explicitly note that `allow:` blocks in config can override root access. For users who want root to bypass all restrictions, they should include "root" in their allow blocks: `allow: {id: [admin, root]}`. + +Consider in the future: a `--root-bypass-config` flag or similar for debugging scenarios where root truly needs unrestricted access. + +### P1: Consolidate cascading logic (Issue 2) + +Extract the cascading logic into a single shared SQL builder. Both `check_permission_for_resource()` and `_build_single_action_sql()` should call the same underlying function. The `resolve_permissions_from_catalog()` in `utils/permissions.py` should either be deprecated or aligned. + +### P1: Fix `source_plugin` SQL injection (Issue 5) + +Pass `source_plugin` as a parameter instead of interpolating it. This is a straightforward fix. + +### P2: Optimize `include_is_private` for anonymous users (Issue 7) + +Short-circuit when `actor=None` - the anonymous check is redundant. + +### P2: Only count visible tables (Issue 8) + +Pass the allowed table set to the counting logic to avoid wasted queries. + +### P3: Replace global `_reason_id` counter (Issue 4) + +Use a per-invocation counter or UUID-based naming for reason parameters. + +### P3: Simplify `resolve_permissions_from_catalog()` restriction handling (Issue 3) + +Refactor to avoid duplicating the entire CTE chain when restrictions are present. + +### P4: Add deny reason to permission check logging (Issue 12) + +Return `(allowed, reason)` tuples from `check_permission_for_resource()`. From 1fa23f4a42e95c5212672306fdeffd7b2d9662f2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 6 Feb 2026 01:52:08 +0000 Subject: [PATCH 002/481] Add consolidation proposal for cascading logic duplication Detailed design for extracting build_cascading_ctes(), collect_permission_rules(), and build_restriction_filter() to replace three separate implementations with one shared SQL builder. Includes migration plan and handles the include_is_private complication. https://claude.ai/code/session_013EkyroQKPhcjdMbpHc9g4X --- permissions-notes.md | 390 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 390 insertions(+) diff --git a/permissions-notes.md b/permissions-notes.md index 3b61a06b..00695f08 100644 --- a/permissions-notes.md +++ b/permissions-notes.md @@ -335,3 +335,393 @@ Refactor to avoid duplicating the entire CTE chain when restrictions are present ### P4: Add deny reason to permission check logging (Issue 12) Return `(allowed, reason)` tuples from `check_permission_for_resource()`. + +--- + +## Proposal: Consolidating the Cascading Logic (Issues 2 + 3) + +### The problem + +The cascading logic ("child > parent > global; deny beats allow at each level") is implemented three times in two files: + +| # | Function | File | Used by | SQL pattern | +|---|----------|------|---------|-------------| +| 1 | `_build_single_action_sql()` | `actions_sql.py:246-384` | `allowed_resources()` (production) | 3 separate CTEs (`child_lvl`, `parent_lvl`, `global_lvl`) with `LEFT JOIN` + `GROUP BY` + `MAX()`, then CASE cascade | +| 2 | `check_permission_for_resource()` | `actions_sql.py:555-587` | `allowed()` (production) | `ROW_NUMBER() OVER (PARTITION BY ... ORDER BY depth DESC, ...)` + `LIMIT 1` | +| 3 | `resolve_permissions_from_catalog()` | `permissions.py:197-391` | Tests only | Same `ROW_NUMBER()` as #2, but with the entire CTE chain **tripled** when restrictions are present | + +These three implementations must all agree on the resolution semantics. A logic change (e.g., adding a priority tier) would need to be replicated in all three places. + +### Why three exist + +Each serves a different purpose with different requirements: + +- **#1 (bulk resources)**: Needs to evaluate every `(parent, child)` in the `base` CTE. Can't use `ROW_NUMBER()` as easily because it needs the per-resource aggregates available for the `include_is_private` anonymous pass too. Outputs `reason` as JSON array and `is_private`. +- **#2 (single resource)**: Only checks one `(parent, child)`. Much simpler — just filter matching rules, rank, pick winner. Returns boolean. +- **#3 (test utility)**: Returns full resolution details (allow, reason, source_plugin, depth) for every candidate. Used in tests to verify the cascading logic itself. + +### Proposed design: One SQL builder, three callers + +Introduce a single function `build_cascading_ctes()` that generates the shared CTE fragment, then each caller wraps it with its own `SELECT` and extras. + +#### Step 1: Extract `build_rules_union_from_permission_sqls()` + +Both production paths (#1 and #2) already have nearly identical code to iterate over `PermissionSQL` objects, collect params, collect `restriction_sqls`, and build the UNION ALL. Factor this into a single shared function: + +```python +# In actions_sql.py (or a new shared module) + +@dataclass +class CollectedRules: + """Result of collecting PermissionSQL objects into SQL fragments.""" + rules_union: str # UNION ALL of all rule SELECTs + params: dict[str, Any] # All collected params + restriction_sqls: list[str] # restriction_sql fragments + +def collect_permission_rules( + permission_sqls: list[PermissionSQL], +) -> CollectedRules | None: + """ + Iterate PermissionSQL objects, build the UNION ALL, collect params + and restriction_sqls. Returns None if no rule SQL was found. + """ + rule_parts = [] + all_params = {} + restriction_sqls = [] + + for i, psql in enumerate(permission_sqls): + all_params.update(psql.params or {}) + if psql.restriction_sql: + restriction_sqls.append(psql.restriction_sql) + if psql.sql is None: + continue + # Parameterize source_plugin instead of interpolating (fixes Issue 5) + source_key = f"_src_{i}" + all_params[source_key] = psql.source + rule_parts.append( + f"SELECT parent, child, allow, reason, :{source_key} AS source_plugin" + f" FROM ({psql.sql})" + ) + + if not rule_parts: + return None + + return CollectedRules( + rules_union=" UNION ALL ".join(rule_parts), + params=all_params, + restriction_sqls=restriction_sqls, + ) +``` + +This already fixes **Issue 5** (`source_plugin` injection) as a side effect. + +#### Step 2: Extract `build_cascading_ctes()` + +The core cascading logic — given a `base` CTE and an `all_rules` CTE, produce a `decisions` CTE — can be expressed as a single function that returns CTE SQL fragments: + +```python +def build_cascading_ctes( + *, + rules_alias: str = "all_rules", + base_alias: str = "base", + include_reasons: bool = False, +) -> str: + """ + Return CTE SQL for child_lvl, parent_lvl, global_lvl, decisions. + + Expects the caller to already have defined CTEs named `base_alias` + (with columns: parent, child) and `rules_alias` (with columns: + parent, child, allow, reason, source_plugin). + + The output `decisions` CTE has columns: + parent, child, is_allowed, reason + Where `reason` is either a json_group_array (include_reasons=True) + or the single winning reason text. + """ + # The three level CTEs + level_ctes = [] + for level_name, join_condition in [ + ("child_lvl", f"ar.parent = b.parent AND ar.child = b.child"), + ("parent_lvl", f"ar.parent = b.parent AND ar.child IS NULL"), + ("global_lvl", f"ar.parent IS NULL AND ar.child IS NULL"), + ]: + reason_cols = "" + if include_reasons: + reason_cols = ( + ",\n json_group_array(CASE WHEN ar.allow = 0 " + "THEN ar.source_plugin || ': ' || ar.reason END) AS deny_reasons" + ",\n json_group_array(CASE WHEN ar.allow = 1 " + "THEN ar.source_plugin || ': ' || ar.reason END) AS allow_reasons" + ) + level_ctes.append(f"""{level_name} AS ( + SELECT b.parent, b.child, + MAX(CASE WHEN ar.allow = 0 THEN 1 ELSE 0 END) AS any_deny, + MAX(CASE WHEN ar.allow = 1 THEN 1 ELSE 0 END) AS any_allow{reason_cols} + FROM {base_alias} b + LEFT JOIN {rules_alias} ar ON {join_condition} + GROUP BY b.parent, b.child +)""") + + # The decisions CTE + if include_reasons: + reason_case = """ + CASE + WHEN cl.any_deny = 1 THEN cl.deny_reasons + WHEN cl.any_allow = 1 THEN cl.allow_reasons + WHEN pl.any_deny = 1 THEN pl.deny_reasons + WHEN pl.any_allow = 1 THEN pl.allow_reasons + WHEN gl.any_deny = 1 THEN gl.deny_reasons + WHEN gl.any_allow = 1 THEN gl.allow_reasons + ELSE '[]' + END AS reason""" + else: + reason_case = "'implicit deny' AS reason" # simple placeholder + + null_safe_join = ( + "b.parent = {a}.parent AND " + "(b.child = {a}.child OR (b.child IS NULL AND {a}.child IS NULL))" + ) + + decisions_cte = f"""decisions AS ( + SELECT + b.parent, b.child, + CASE + WHEN cl.any_deny = 1 THEN 0 + WHEN cl.any_allow = 1 THEN 1 + WHEN pl.any_deny = 1 THEN 0 + WHEN pl.any_allow = 1 THEN 1 + WHEN gl.any_deny = 1 THEN 0 + WHEN gl.any_allow = 1 THEN 1 + ELSE 0 + END AS is_allowed, + {reason_case} + FROM {base_alias} b + JOIN child_lvl cl ON {null_safe_join.format(a='cl')} + JOIN parent_lvl pl ON {null_safe_join.format(a='pl')} + JOIN global_lvl gl ON {null_safe_join.format(a='gl')} +)""" + + return ",\n".join(level_ctes) + ",\n" + decisions_cte +``` + +#### Step 3: Extract `build_restriction_filter()` + +Restriction handling is also duplicated. A single function can generate the restriction CTE and WHERE clause: + +```python +def build_restriction_filter(restriction_sqls: list[str]) -> tuple[str, str]: + """ + Returns (cte_sql, where_clause) for restriction filtering. + + cte_sql: ", restriction_list AS (...)" to append to WITH block + where_clause: "AND EXISTS (...)" to append to WHERE + """ + restriction_intersect = "\nINTERSECT\n".join( + f"SELECT * FROM ({sql})" for sql in restriction_sqls + ) + cte_sql = f",\nrestriction_list AS (\n {restriction_intersect}\n)" + where_clause = """ + AND EXISTS ( + SELECT 1 FROM restriction_list r + WHERE (r.parent = decisions.parent OR r.parent IS NULL) + AND (r.child = decisions.child OR r.child IS NULL) + )""" + return cte_sql, where_clause +``` + +#### Step 4: Rewrite the three callers + +**`_build_single_action_sql()` (bulk resources)** + +```python +async def _build_single_action_sql(datasette, actor, action, *, parent=None, + include_is_private=False): + action_obj = datasette.actions.get(action) + base_resources_sql = await action_obj.resource_class.resources_sql(datasette) + + permission_sqls = await gather_permission_sql_from_hooks(...) + if permission_sqls is SKIP_PERMISSION_CHECKS: + return ... # early return unchanged + + collected = collect_permission_rules(permission_sqls) + if collected is None: + return ... # empty result unchanged + + all_params = collected.params + + # Build WITH clause + cte_parts = [ + f"WITH\nbase AS (\n {base_resources_sql}\n)", + f"all_rules AS (\n {collected.rules_union}\n)", + ] + + # Anonymous rules for is_private (if needed) + if include_is_private: + anon_collected = ... # same anon logic as before, but using collect_permission_rules + cte_parts.append(f"anon_rules AS (\n {anon_collected.rules_union}\n)") + + # Core cascading logic — ONE call + cte_parts.append(build_cascading_ctes(include_reasons=True)) + + if include_is_private: + cte_parts.append(build_cascading_ctes( + rules_alias="anon_rules", + base_alias="base", + # Use different CTE names to avoid collision: + # This variant would need a prefix parameter, e.g. prefix="anon_" + )) + # ... or simpler: call a second time with aliased names + + # Restriction filter + restriction_cte = "" + restriction_where = "" + if collected.restriction_sqls: + restriction_cte, restriction_where = build_restriction_filter( + collected.restriction_sqls + ) + + # Final SELECT + select_cols = "parent, child, reason" + if include_is_private: + select_cols += ", is_private" + + query = ( + ",\n".join(cte_parts) + restriction_cte + + f"\nSELECT {select_cols}\nFROM decisions\nWHERE is_allowed = 1" + + restriction_where + + (f"\n AND parent = :filter_parent" if parent else "") + + "\nORDER BY parent, child" + ) + return query, all_params +``` + +**`check_permission_for_resource()` (single resource)** + +This can now be rewritten to use the same `build_cascading_ctes()` with a single-row `base`: + +```python +async def check_permission_for_resource(*, datasette, actor, action, parent, child): + rules_union, all_params, restriction_sqls = await build_permission_rules_sql( + datasette, actor, action + ) + if not rules_union: + return False + + all_params["_check_parent"] = parent + all_params["_check_child"] = child + + # Check restrictions first (unchanged fast-path) + if restriction_sqls: + ... # existing restriction check, unchanged + + # Use the shared cascading logic with a single-row base + base_sql = "SELECT :_check_parent AS parent, :_check_child AS child" + cascade = build_cascading_ctes() + + query = f""" +WITH +base AS ({base_sql}), +all_rules AS ({rules_union}), +{cascade} +SELECT COALESCE((SELECT is_allowed FROM decisions), 0) AS is_allowed +""" + result = await datasette.get_internal_database().execute(query, all_params) + return bool(result.rows[0][0]) if result.rows else False +``` + +This replaces the current depth/ROW_NUMBER approach with the same `child_lvl`/`parent_lvl`/`global_lvl` pattern, ensuring identical semantics. + +**`resolve_permissions_from_catalog()` (test utility)** + +This becomes a thin wrapper too. Since it's test-only, the main benefit is eliminating 250 lines of duplicated SQL: + +```python +async def resolve_permissions_from_catalog(db, actor, plugins, action, + candidate_sql, candidate_params=None, + *, implicit_deny=True): + # Resolve plugins (existing code, unchanged) + resolved_plugins, restriction_sqls = ... + + union_sql, rule_params = build_rules_union(actor, resolved_plugins) + all_params = {**(candidate_params or {}), **rule_params, "action": action} + + cascade = build_cascading_ctes( + include_reasons=True, + base_alias="cands", + rules_alias="rules", + ) + + # One query, no duplication + restriction_cte = "" + restriction_where = "" + if restriction_sqls: + restriction_cte, restriction_where = build_restriction_filter(restriction_sqls) + + sql = f""" + WITH + cands AS ({candidate_sql}), + rules AS ({union_sql}), + {cascade} + {restriction_cte} + SELECT + c.parent, c.child, + COALESCE(d.is_allowed, CASE WHEN :implicit_deny THEN 0 ELSE NULL END) AS allow, + d.reason, :action AS action, + ... + FROM cands c + LEFT JOIN decisions d ON c.parent = d.parent AND c.child = d.child + {restriction_where} + ORDER BY c.parent, c.child + """ + + rows = await db.execute(sql, {**all_params, "implicit_deny": ...}) + return [dict(r) for r in rows] +``` + +This eliminates the 135-line SQL triplication entirely. + +### The `include_is_private` complication + +The `include_is_private` path is the one wrinkle. It needs to run the cascading logic twice: once for the real actor and once for anonymous. The current code duplicates all three level CTEs with `anon_` prefixes. + +With the shared builder, we'd need `build_cascading_ctes()` to accept a `prefix` parameter so it can generate `anon_child_lvl`, `anon_parent_lvl`, etc.: + +```python +def build_cascading_ctes(*, rules_alias="all_rules", base_alias="base", + include_reasons=False, prefix=""): + # Use prefix for all CTE names: + # f"{prefix}child_lvl", f"{prefix}parent_lvl", etc. +``` + +Then the caller does: + +```python +ctes = build_cascading_ctes(include_reasons=True) # -> child_lvl, decisions +ctes += build_cascading_ctes(rules_alias="anon_rules", # -> anon_child_lvl, anon_decisions + prefix="anon_", + include_reasons=False) +``` + +And the final SELECT joins both `decisions` and `anon_decisions`. + +### Impact summary + +| Before | After | +|--------|-------| +| `_build_single_action_sql`: ~180 lines of CTE construction | ~40 lines + shared builder | +| `check_permission_for_resource`: ~35 lines of cascading SQL | ~10 lines + shared builder | +| `resolve_permissions_from_catalog`: ~250 lines, SQL tripled when restrictions present | ~30 lines + shared builder | +| `source_plugin` interpolated unsafely in 3 places | Parameterized in `collect_permission_rules()` | +| `build_rules_union` in `permissions.py` (test-only duplicate) | Replaced by shared `collect_permission_rules()` | + +Total: ~465 lines of SQL-building code reduced to ~80 lines of callers + ~80 lines of shared builders. Three implementations of cascading logic become one. + +### Migration plan + +1. Add `collect_permission_rules()` and `build_cascading_ctes()` and `build_restriction_filter()` to `actions_sql.py` (or a new `datasette/utils/permission_sql_builder.py`) +2. Rewrite `check_permission_for_resource()` to use the shared builder +3. Rewrite `_build_single_action_sql()` to use the shared builder (including `include_is_private` prefix support) +4. Rewrite `resolve_permissions_from_catalog()` to use the shared builder +5. Delete `build_rules_union()` from `permissions.py` +6. Run the full test suite — all 263 tests must still pass since behavior is unchanged +7. Verify via `?_trace=1` that the generated SQL is correct and equivalently performant From c9429466cd7588a90b41fc4eb1152f1be01fdac0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 6 Feb 2026 02:10:36 +0000 Subject: [PATCH 003/481] Remove test-only cascading logic implementation, fix params bug Three changes: 1. Rewrite test_utils_permissions.py to exercise production code paths (allowed_resources / allowed) instead of the test-only resolve_permissions_from_catalog function. Tests now register plugins via ds.pm.register and call the real Datasette methods. 2. Remove resolve_permissions_from_catalog, resolve_permissions_with_candidates, and build_rules_union from datasette/utils/permissions.py. These were only used by tests and implemented the cascading logic a third time, independently of the two production implementations. 3. Fix bug in gather_permission_sql_from_hooks where empty params dicts ({}) would cause framework-injected params (:actor_id, :actor, :action) to be silently lost. The expression `params = permission_sql.params or {}` creates a new dict when params is {} (falsy), so setdefault writes to a throwaway dict. Fixed by explicitly checking `is None`. https://claude.ai/code/session_013EkyroQKPhcjdMbpHc9g4X --- datasette/utils/permissions.py | 367 +--------- tests/test_utils_permissions.py | 1120 ++++++++++++++++--------------- 2 files changed, 580 insertions(+), 907 deletions(-) diff --git a/datasette/utils/permissions.py b/datasette/utils/permissions.py index 6c30a12a..4f4858d0 100644 --- a/datasette/utils/permissions.py +++ b/datasette/utils/permissions.py @@ -2,8 +2,7 @@ from __future__ import annotations import json -from typing import Any, Dict, Iterable, List, Sequence, Tuple -import sqlite3 +from typing import Any, Iterable, List from datasette.permissions import PermissionSQL from datasette.plugins import pm @@ -46,10 +45,11 @@ async def gather_permission_sql_from_hooks( for permission_sql in _iter_permission_sql_from_result(resolved, action=action): if not permission_sql.source: permission_sql.source = default_source - params = permission_sql.params or {} - params.setdefault("action", action) - params.setdefault("actor", actor_json) - params.setdefault("actor_id", actor_id) + if permission_sql.params is None: + permission_sql.params = {} + permission_sql.params.setdefault("action", action) + permission_sql.params.setdefault("actor", actor_json) + permission_sql.params.setdefault("actor_id", actor_id) collected.append(permission_sql) return collected @@ -82,358 +82,3 @@ def _iter_permission_sql_from_result( raise TypeError( "Plugin providers must return PermissionSQL instances, sequences, or callables" ) - - -# ----------------------------- -# Plugin interface & utilities -# ----------------------------- - - -def build_rules_union( - actor: dict | None, plugins: Sequence[PermissionSQL] -) -> Tuple[str, Dict[str, Any]]: - """ - Compose plugin SQL into a UNION ALL. - - Returns: - union_sql: a SELECT with columns (parent, child, allow, reason, source_plugin) - params: dict of bound parameters including :actor (JSON), :actor_id, and plugin params - - Note: Plugins are responsible for ensuring their parameter names don't conflict. - The system reserves these parameter names: :actor, :actor_id, :action, :filter_parent - Plugin parameters should be prefixed with a unique identifier (e.g., source name). - """ - parts: List[str] = [] - actor_json = json.dumps(actor) if actor else None - actor_id = actor.get("id") if actor else None - params: Dict[str, Any] = {"actor": actor_json, "actor_id": actor_id} - - for p in plugins: - # No namespacing - just use plugin params as-is - params.update(p.params or {}) - - # Skip plugins that only provide restriction_sql (no permission rules) - if p.sql is None: - continue - - parts.append( - f""" - SELECT parent, child, allow, reason, '{p.source}' AS source_plugin FROM ( - {p.sql} - ) - """.strip() - ) - - if not parts: - # Empty UNION that returns no rows - union_sql = "SELECT NULL parent, NULL child, NULL allow, NULL reason, 'none' source_plugin WHERE 0" - else: - union_sql = "\nUNION ALL\n".join(parts) - - return union_sql, params - - -# ----------------------------------------------- -# Core resolvers (no temp tables, no custom UDFs) -# ----------------------------------------------- - - -async def resolve_permissions_from_catalog( - db, - actor: dict | None, - plugins: Sequence[Any], - action: str, - candidate_sql: str, - candidate_params: Dict[str, Any] | None = None, - *, - implicit_deny: bool = True, -) -> List[Dict[str, Any]]: - """ - Resolve permissions by embedding the provided *candidate_sql* in a CTE. - - Expectations: - - candidate_sql SELECTs: parent TEXT, child TEXT - (Use child=NULL for parent-scoped actions like "execute-sql".) - - *db* exposes: rows = await db.execute(sql, params) - where rows is an iterable of sqlite3.Row - - plugins: hook results handled by await_me_maybe - can be sync/async, - single PermissionSQL, list, or callable returning PermissionSQL - - actor is the actor dict (or None), made available as :actor (JSON), :actor_id, and :action - - Decision policy: - 1) Specificity first: child (depth=2) > parent (depth=1) > root (depth=0) - 2) Within the same depth: deny (0) beats allow (1) - 3) If no matching rule: - - implicit_deny=True -> treat as allow=0, reason='implicit deny' - - implicit_deny=False -> allow=None, reason=None - - Returns: list of dict rows - - parent, child, allow, reason, source_plugin, depth - - resource (rendered "/parent/child" or "/parent" or "/") - """ - resolved_plugins: List[PermissionSQL] = [] - restriction_sqls: List[str] = [] - - for plugin in plugins: - if callable(plugin) and not isinstance(plugin, PermissionSQL): - resolved = plugin(action) # type: ignore[arg-type] - else: - resolved = plugin # type: ignore[assignment] - if not isinstance(resolved, PermissionSQL): - raise TypeError("Plugin providers must return PermissionSQL instances") - resolved_plugins.append(resolved) - - # Collect restriction SQL filters - if resolved.restriction_sql: - restriction_sqls.append(resolved.restriction_sql) - - union_sql, rule_params = build_rules_union(actor, resolved_plugins) - all_params = { - **(candidate_params or {}), - **rule_params, - "action": action, - } - - sql = f""" - WITH - cands AS ( - {candidate_sql} - ), - rules AS ( - {union_sql} - ), - matched AS ( - SELECT - c.parent, c.child, - r.allow, r.reason, r.source_plugin, - CASE - WHEN r.child IS NOT NULL THEN 2 -- child-level (most specific) - WHEN r.parent IS NOT NULL THEN 1 -- parent-level - ELSE 0 -- root/global - END AS depth - FROM cands c - JOIN rules r - ON (r.parent IS NULL OR r.parent = c.parent) - AND (r.child IS NULL OR r.child = c.child) - ), - ranked AS ( - SELECT *, - ROW_NUMBER() OVER ( - PARTITION BY parent, child - ORDER BY - depth DESC, -- specificity first - CASE WHEN allow=0 THEN 0 ELSE 1 END, -- then deny over allow at same depth - source_plugin -- stable tie-break - ) AS rn - FROM matched - ), - winner AS ( - SELECT parent, child, - allow, reason, source_plugin, depth - FROM ranked WHERE rn = 1 - ) - SELECT - c.parent, c.child, - COALESCE(w.allow, CASE WHEN :implicit_deny THEN 0 ELSE NULL END) AS allow, - COALESCE(w.reason, CASE WHEN :implicit_deny THEN 'implicit deny' ELSE NULL END) AS reason, - w.source_plugin, - COALESCE(w.depth, -1) AS depth, - :action AS action, - CASE - WHEN c.parent IS NULL THEN '/' - WHEN c.child IS NULL THEN '/' || c.parent - ELSE '/' || c.parent || '/' || c.child - END AS resource - FROM cands c - LEFT JOIN winner w - ON ((w.parent = c.parent) OR (w.parent IS NULL AND c.parent IS NULL)) - AND ((w.child = c.child ) OR (w.child IS NULL AND c.child IS NULL)) - ORDER BY c.parent, c.child - """ - - # If there are restriction filters, wrap the query with INTERSECT - # This ensures only resources in the restriction allowlist are returned - if restriction_sqls: - # Start with the main query, but select only parent/child for the INTERSECT - main_query_for_intersect = f""" - WITH - cands AS ( - {candidate_sql} - ), - rules AS ( - {union_sql} - ), - matched AS ( - SELECT - c.parent, c.child, - r.allow, r.reason, r.source_plugin, - CASE - WHEN r.child IS NOT NULL THEN 2 -- child-level (most specific) - WHEN r.parent IS NOT NULL THEN 1 -- parent-level - ELSE 0 -- root/global - END AS depth - FROM cands c - JOIN rules r - ON (r.parent IS NULL OR r.parent = c.parent) - AND (r.child IS NULL OR r.child = c.child) - ), - ranked AS ( - SELECT *, - ROW_NUMBER() OVER ( - PARTITION BY parent, child - ORDER BY - depth DESC, -- specificity first - CASE WHEN allow=0 THEN 0 ELSE 1 END, -- then deny over allow at same depth - source_plugin -- stable tie-break - ) AS rn - FROM matched - ), - winner AS ( - SELECT parent, child, - allow, reason, source_plugin, depth - FROM ranked WHERE rn = 1 - ), - permitted_resources AS ( - SELECT c.parent, c.child - FROM cands c - LEFT JOIN winner w - ON ((w.parent = c.parent) OR (w.parent IS NULL AND c.parent IS NULL)) - AND ((w.child = c.child ) OR (w.child IS NULL AND c.child IS NULL)) - WHERE COALESCE(w.allow, CASE WHEN :implicit_deny THEN 0 ELSE NULL END) = 1 - ) - SELECT parent, child FROM permitted_resources - """ - - # Build restriction list with INTERSECT (all must match) - # Then filter to resources that match hierarchically - # Wrap each restriction_sql in a subquery to avoid operator precedence issues - # with UNION ALL inside the restriction SQL statements - restriction_intersect = "\nINTERSECT\n".join( - f"SELECT * FROM ({sql})" for sql in restriction_sqls - ) - - # Combine: resources allowed by permissions AND in restriction allowlist - # Database-level restrictions (parent, NULL) should match all children (parent, *) - filtered_resources = f""" - WITH restriction_list AS ( - {restriction_intersect} - ), - permitted AS ( - {main_query_for_intersect} - ), - filtered AS ( - SELECT p.parent, p.child - FROM permitted p - WHERE EXISTS ( - SELECT 1 FROM restriction_list r - WHERE (r.parent = p.parent OR r.parent IS NULL) - AND (r.child = p.child OR r.child IS NULL) - ) - ) - """ - - # Now join back to get full results for only the filtered resources - sql = f""" - {filtered_resources} - , cands AS ( - {candidate_sql} - ), - rules AS ( - {union_sql} - ), - matched AS ( - SELECT - c.parent, c.child, - r.allow, r.reason, r.source_plugin, - CASE - WHEN r.child IS NOT NULL THEN 2 -- child-level (most specific) - WHEN r.parent IS NOT NULL THEN 1 -- parent-level - ELSE 0 -- root/global - END AS depth - FROM cands c - JOIN rules r - ON (r.parent IS NULL OR r.parent = c.parent) - AND (r.child IS NULL OR r.child = c.child) - ), - ranked AS ( - SELECT *, - ROW_NUMBER() OVER ( - PARTITION BY parent, child - ORDER BY - depth DESC, -- specificity first - CASE WHEN allow=0 THEN 0 ELSE 1 END, -- then deny over allow at same depth - source_plugin -- stable tie-break - ) AS rn - FROM matched - ), - winner AS ( - SELECT parent, child, - allow, reason, source_plugin, depth - FROM ranked WHERE rn = 1 - ) - SELECT - c.parent, c.child, - COALESCE(w.allow, CASE WHEN :implicit_deny THEN 0 ELSE NULL END) AS allow, - COALESCE(w.reason, CASE WHEN :implicit_deny THEN 'implicit deny' ELSE NULL END) AS reason, - w.source_plugin, - COALESCE(w.depth, -1) AS depth, - :action AS action, - CASE - WHEN c.parent IS NULL THEN '/' - WHEN c.child IS NULL THEN '/' || c.parent - ELSE '/' || c.parent || '/' || c.child - END AS resource - FROM filtered c - LEFT JOIN winner w - ON ((w.parent = c.parent) OR (w.parent IS NULL AND c.parent IS NULL)) - AND ((w.child = c.child ) OR (w.child IS NULL AND c.child IS NULL)) - ORDER BY c.parent, c.child - """ - - rows_iter: Iterable[sqlite3.Row] = await db.execute( - sql, - {**all_params, "implicit_deny": 1 if implicit_deny else 0}, - ) - return [dict(r) for r in rows_iter] - - -async def resolve_permissions_with_candidates( - db, - actor: dict | None, - plugins: Sequence[Any], - candidates: List[Tuple[str, str | None]], - action: str, - *, - implicit_deny: bool = True, -) -> List[Dict[str, Any]]: - """ - Resolve permissions without any external candidate table by embedding - the candidates as a UNION of parameterized SELECTs in a CTE. - - candidates: list of (parent, child) where child can be None for parent-scoped actions. - actor: actor dict (or None), made available as :actor (JSON), :actor_id, and :action - """ - # Build a small CTE for candidates. - cand_rows_sql: List[str] = [] - cand_params: Dict[str, Any] = {} - for i, (parent, child) in enumerate(candidates): - pkey = f"cand_p_{i}" - ckey = f"cand_c_{i}" - cand_params[pkey] = parent - cand_params[ckey] = child - cand_rows_sql.append(f"SELECT :{pkey} AS parent, :{ckey} AS child") - candidate_sql = ( - "\nUNION ALL\n".join(cand_rows_sql) - if cand_rows_sql - else "SELECT NULL AS parent, NULL AS child WHERE 0" - ) - - return await resolve_permissions_from_catalog( - db, - actor, - plugins, - action, - candidate_sql=candidate_sql, - candidate_params=cand_params, - implicit_deny=implicit_deny, - ) diff --git a/tests/test_utils_permissions.py b/tests/test_utils_permissions.py index b412de0f..8105e8f4 100644 --- a/tests/test_utils_permissions.py +++ b/tests/test_utils_permissions.py @@ -1,612 +1,640 @@ +""" +Tests for cascading permission resolution logic. + +These tests verify the core cascading semantics through the production +code paths (allowed_resources / allowed) rather than through a separate +test-only SQL builder. Every test registers a lightweight plugin via +``ds.pm.register`` and calls the real ``Datasette.allowed_resources()`` +and/or ``Datasette.allowed()`` methods. + +Cascading semantics tested: + 1. child (depth 2) > parent (depth 1) > global (depth 0) + 2. DENY beats ALLOW at the same depth + 3. No matching rule → implicit deny + 4. Multiple plugins can contribute rules with independent parameters + 5. :actor, :actor_id, :action are available in SQL +""" + import pytest +import pytest_asyncio from datasette.app import Datasette from datasette.permissions import PermissionSQL -from datasette.utils.permissions import resolve_permissions_from_catalog -from typing import Callable, List +from datasette.resources import TableResource, DatabaseResource +from datasette import hookimpl -@pytest.fixture -def db(): - ds = Datasette() - import tempfile - from datasette.database import Database +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- - path = tempfile.mktemp(suffix="demo.db") - db = ds.add_database(Database(ds, path=path)) - return db +class PermissionRulesPlugin: + """Thin shim that delegates to a callback for permission_resources_sql.""" + + def __init__(self, rules_callback): + self.rules_callback = rules_callback + + @hookimpl + def permission_resources_sql(self, datasette, actor, action): + return self.rules_callback(datasette, actor, action) -NO_RULES_SQL = ( - "SELECT NULL AS parent, NULL AS child, NULL AS allow, NULL AS reason WHERE 0" -) +@pytest_asyncio.fixture +async def ds(): + """ + Create a Datasette instance with catalog tables that mirror the + original test_utils_permissions layout: + + databases: perm_accounting, perm_hr, perm_analytics + tables per db: table01..table10 + special tables: perm_accounting/sales, perm_analytics/secret + + Uses default_deny=True so that only the test-registered plugins + determine permission outcomes (no built-in default-allow rules). + + Database names are prefixed with ``perm_`` to avoid collisions with + other test fixtures that create memory databases in the same process. + """ + instance = Datasette(default_deny=True) + await instance.invoke_startup() + + per_parent = 10 + parents = ["perm_accounting", "perm_hr", "perm_analytics"] + specials = {"perm_accounting": ["sales"], "perm_analytics": ["secret"], "perm_hr": []} + + for parent in parents: + db = instance.add_memory_database(parent) + base_tables = [f"table{i:02d}" for i in range(1, per_parent + 1)] + for s in specials.get(parent, []): + if s not in base_tables: + base_tables[0] = s + for tbl in base_tables: + await db.execute_write( + f"CREATE TABLE IF NOT EXISTS [{tbl}] (id INTEGER PRIMARY KEY)" + ) + + await instance._refresh_schemas() + yield instance + # Cleanup: remove databases to avoid polluting other tests + for parent in parents: + instance.remove_database(parent) -def plugin_allow_all_for_user(user: str) -> Callable[[str], PermissionSQL]: - def provider(action: str) -> PermissionSQL: +# --------------------------------------------------------------------------- +# Plugin factories — return callables suitable for PermissionRulesPlugin +# --------------------------------------------------------------------------- + +def _cb_allow_all_for_user(user): + """Global allow for a specific user.""" + def cb(datasette, actor, action): + if not actor or actor.get("id") != user: + return None return PermissionSQL( - """ - SELECT NULL AS parent, NULL AS child, 1 AS allow, - 'global allow for ' || :allow_all_user || ' on ' || :allow_all_action AS reason - WHERE :actor_id = :allow_all_user - """, - {"allow_all_user": user, "allow_all_action": action}, + sql=( + "SELECT NULL AS parent, NULL AS child, 1 AS allow, " + "'global allow for ' || :_aau_user || ' on ' || :action AS reason" + ), + params={"_aau_user": user}, + ) + return cb + + +def _cb_deny_specific_table(user, parent, child): + """Child-level deny for a specific user + table.""" + def cb(datasette, actor, action): + if not actor or actor.get("id") != user: + return None + return PermissionSQL( + sql=( + "SELECT :_dst_parent AS parent, :_dst_child AS child, 0 AS allow, " + "'deny ' || :_dst_parent || '/' || :_dst_child || ' for ' || :_dst_user AS reason" + ), + params={"_dst_parent": parent, "_dst_child": child, "_dst_user": user}, + ) + return cb + + +def _cb_org_policy_deny_parent(parent): + """Unconditional parent-level deny (applies to all actors).""" + def cb(datasette, actor, action): + return PermissionSQL( + sql=( + "SELECT :_opd_parent AS parent, NULL AS child, 0 AS allow, " + "'org policy: deny ' || :_opd_parent AS reason" + ), + params={"_opd_parent": parent}, + ) + return cb + + +def _cb_allow_parent_for_user(user, parent): + """Parent-level allow for a specific user.""" + def cb(datasette, actor, action): + if not actor or actor.get("id") != user: + return None + return PermissionSQL( + sql=( + "SELECT :_apu_parent AS parent, NULL AS child, 1 AS allow, " + "'allow parent ' || :_apu_parent || ' for ' || :_apu_user AS reason" + ), + params={"_apu_parent": parent, "_apu_user": user}, + ) + return cb + + +def _cb_child_allow_for_user(user, parent, child): + """Child-level allow for a specific user.""" + def cb(datasette, actor, action): + if not actor or actor.get("id") != user: + return None + return PermissionSQL( + sql=( + "SELECT :_cau_parent AS parent, :_cau_child AS child, 1 AS allow, " + "'allow child ' || :_cau_parent || '/' || :_cau_child || ' for ' || :_cau_user AS reason" + ), + params={"_cau_parent": parent, "_cau_child": child, "_cau_user": user}, + ) + return cb + + +def _cb_root_deny_for_all(): + """Unconditional global deny.""" + def cb(datasette, actor, action): + return PermissionSQL( + sql=( + "SELECT NULL AS parent, NULL AS child, 0 AS allow, " + "'root deny for all' AS reason" + ), + ) + return cb + + +def _cb_conflicting_same_child_rules(user, parent, child): + """Two plugins: one allow + one deny at the same child level.""" + def cb_allow(datasette, actor, action): + if not actor or actor.get("id") != user: + return None + return PermissionSQL( + sql=( + "SELECT :_csca_parent AS parent, :_csca_child AS child, 1 AS allow, " + "'team grant at child' AS reason" + ), + params={"_csca_parent": parent, "_csca_child": child}, ) - return provider - - -def plugin_deny_specific_table( - user: str, parent: str, child: str -) -> Callable[[str], PermissionSQL]: - def provider(action: str) -> PermissionSQL: + def cb_deny(datasette, actor, action): + if not actor or actor.get("id") != user: + return None return PermissionSQL( - """ - 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 - """, - { - "deny_specific_table_parent": parent, - "deny_specific_table_child": child, - "deny_specific_table_user": user, - "deny_specific_table_action": action, - }, + sql=( + "SELECT :_cscd_parent AS parent, :_cscd_child AS child, 0 AS allow, " + "'exception deny at child' AS reason" + ), + params={"_cscd_parent": parent, "_cscd_child": child}, ) - return provider + return cb_allow, cb_deny -def plugin_org_policy_deny_parent(parent: str) -> Callable[[str], PermissionSQL]: - def provider(action: str) -> PermissionSQL: - return PermissionSQL( - """ - 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 - """, - { - "org_policy_parent_deny_parent": parent, - "org_policy_parent_deny_action": action, - }, - ) - - return provider - - -def plugin_allow_parent_for_user( - user: str, parent: str -) -> Callable[[str], PermissionSQL]: - def provider(action: str) -> PermissionSQL: - return PermissionSQL( - """ - 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 - """, - { - "allow_parent_parent": parent, - "allow_parent_user": user, - "allow_parent_action": action, - }, - ) - - return provider - - -def plugin_child_allow_for_user( - user: str, parent: str, child: str -) -> Callable[[str], PermissionSQL]: - def provider(action: str) -> PermissionSQL: - return PermissionSQL( - """ - 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 - """, - { - "allow_child_parent": parent, - "allow_child_child": child, - "allow_child_user": user, - "allow_child_action": action, - }, - ) - - return provider - - -def plugin_root_deny_for_all() -> Callable[[str], PermissionSQL]: - def provider(action: str) -> PermissionSQL: - return PermissionSQL( - """ - SELECT NULL AS parent, NULL AS child, 0 AS allow, 'root deny for all on ' || :root_deny_action AS reason - """, - {"root_deny_action": action}, - ) - - return provider - - -def plugin_conflicting_same_child_rules( - user: str, parent: str, child: str -) -> List[Callable[[str], PermissionSQL]]: - def allow_provider(action: str) -> PermissionSQL: - return PermissionSQL( - """ - 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 - """, - { - "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( - """ - 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 - """, - { - "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] - - -def plugin_allow_all_for_action( - user: str, allowed_action: str -) -> Callable[[str], PermissionSQL]: - def provider(action: str) -> PermissionSQL: +def _cb_allow_all_for_action(user, allowed_action): + """Global allow for a specific user on a specific action only.""" + def cb(datasette, actor, action): if action != allowed_action: - return PermissionSQL(NO_RULES_SQL) - # Sanitize parameter names by replacing hyphens with underscores - param_prefix = action.replace("-", "_") + return None + if not actor or actor.get("id") != user: + return None return PermissionSQL( - f""" - SELECT NULL AS parent, NULL AS child, 1 AS allow, - 'global allow for ' || :{param_prefix}_user || ' on ' || :{param_prefix}_action AS reason - WHERE :actor_id = :{param_prefix}_user - """, - {f"{param_prefix}_user": user, f"{param_prefix}_action": action}, + sql=( + "SELECT NULL AS parent, NULL AS child, 1 AS allow, " + "'global allow for ' || :_aafa_user || ' on ' || :action AS reason" + ), + params={"_aafa_user": user}, ) + return cb - return provider +# --------------------------------------------------------------------------- +# Helpers for asserting results +# --------------------------------------------------------------------------- + +def _allowed_set(resources): + """Convert PaginatedResources.resources to {(parent, child), ...}.""" + return {(r.parent, r.child) for r in resources} + + +def _allowed_set_for_parent(resources, parent): + return {(r.parent, r.child) for r in resources if r.parent == parent} + + +# --------------------------------------------------------------------------- +# Tests +# --------------------------------------------------------------------------- VIEW_TABLE = "view-table" -# ---------- Catalog DDL (from your schema) ---------- -CATALOG_DDL = """ -CREATE TABLE IF NOT EXISTS catalog_databases ( - database_name TEXT PRIMARY KEY, - path TEXT, - is_memory INTEGER, - schema_version INTEGER -); -CREATE TABLE IF NOT EXISTS catalog_tables ( - database_name TEXT, - table_name TEXT, - rootpage INTEGER, - sql TEXT, - PRIMARY KEY (database_name, table_name), - FOREIGN KEY (database_name) REFERENCES catalog_databases(database_name) -); -""" - -PARENTS = ["accounting", "hr", "analytics"] -SPECIALS = {"accounting": ["sales"], "analytics": ["secret"], "hr": []} - -TABLE_CANDIDATES_SQL = ( - "SELECT database_name AS parent, table_name AS child FROM catalog_tables" -) -PARENT_CANDIDATES_SQL = ( - "SELECT database_name AS parent, NULL AS child FROM catalog_databases" -) - - -# ---------- Helpers ---------- -async def seed_catalog(db, per_parent: int = 10) -> None: - await db.execute_write_script(CATALOG_DDL) - # databases - db_rows = [(p, f"/{p}.db", 0, 1) for p in PARENTS] - await db.execute_write_many( - "INSERT OR REPLACE INTO catalog_databases(database_name, path, is_memory, schema_version) VALUES (?,?,?,?)", - db_rows, - ) - - # tables - def tables_for(parent: str, n: int): - base = [f"table{i:02d}" for i in range(1, n + 1)] - for s in SPECIALS.get(parent, []): - if s not in base: - base[0] = s - return base - - table_rows = [] - for p in PARENTS: - for t in tables_for(p, per_parent): - table_rows.append((p, t, 0, f"CREATE TABLE {t} (id INTEGER PRIMARY KEY)")) - await db.execute_write_many( - "INSERT OR REPLACE INTO catalog_tables(database_name, table_name, rootpage, sql) VALUES (?,?,?,?)", - table_rows, - ) - - -def res_allowed(rows, parent=None): - return sorted( - r["resource"] - for r in rows - if r["allow"] == 1 and (parent is None or r["parent"] == parent) - ) - - -def res_denied(rows, parent=None): - return sorted( - r["resource"] - for r in rows - if r["allow"] == 0 and (parent is None or r["parent"] == parent) - ) - - -# ---------- Tests ---------- @pytest.mark.asyncio -async def test_alice_global_allow_with_specific_denies_catalog(db): - await seed_catalog(db) - plugins = [ - plugin_allow_all_for_user("alice"), - plugin_deny_specific_table("alice", "accounting", "sales"), - plugin_org_policy_deny_parent("hr"), - ] - rows = await resolve_permissions_from_catalog( - db, - {"id": "alice"}, - plugins, - VIEW_TABLE, - TABLE_CANDIDATES_SQL, - implicit_deny=True, - ) - # Alice can see everything except accounting/sales and hr/* - assert "/accounting/sales" in res_denied(rows) - for r in rows: - if r["parent"] == "hr": - assert r["allow"] == 0 - elif r["resource"] == "/accounting/sales": - assert r["allow"] == 0 - else: - assert r["allow"] == 1 - - -@pytest.mark.asyncio -async def test_carol_parent_allow_but_child_conflict_deny_wins_catalog(db): - await seed_catalog(db) - plugins = [ - plugin_org_policy_deny_parent("hr"), - plugin_allow_parent_for_user("carol", "analytics"), - *plugin_conflicting_same_child_rules("carol", "analytics", "secret"), - ] - rows = await resolve_permissions_from_catalog( - db, - {"id": "carol"}, - plugins, - VIEW_TABLE, - TABLE_CANDIDATES_SQL, - implicit_deny=True, - ) - allowed_analytics = res_allowed(rows, parent="analytics") - denied_analytics = res_denied(rows, parent="analytics") - - assert "/analytics/secret" in denied_analytics - # 10 analytics children total, 1 denied - assert len(allowed_analytics) == 9 - - -@pytest.mark.asyncio -async def test_specificity_child_allow_overrides_parent_deny_catalog(db): - await seed_catalog(db) - plugins = [ - plugin_allow_all_for_user("alice"), - plugin_org_policy_deny_parent("analytics"), # parent-level deny - plugin_child_allow_for_user( - "alice", "analytics", "table02" - ), # child allow beats parent deny - ] - rows = await resolve_permissions_from_catalog( - db, - {"id": "alice"}, - plugins, - VIEW_TABLE, - TABLE_CANDIDATES_SQL, - implicit_deny=True, - ) - - # table02 allowed, other analytics tables denied - assert any(r["resource"] == "/analytics/table02" and r["allow"] == 1 for r in rows) - assert all( - (r["parent"] != "analytics" or r["child"] == "table02" or r["allow"] == 0) - for r in rows - ) - - -@pytest.mark.asyncio -async def test_root_deny_all_but_parent_allow_rescues_specific_parent_catalog(db): - await seed_catalog(db) - plugins = [ - plugin_root_deny_for_all(), # root deny - plugin_allow_parent_for_user( - "bob", "accounting" - ), # parent allow (more specific) - ] - rows = await resolve_permissions_from_catalog( - db, {"id": "bob"}, plugins, VIEW_TABLE, TABLE_CANDIDATES_SQL, implicit_deny=True - ) - for r in rows: - if r["parent"] == "accounting": - assert r["allow"] == 1 - else: - assert r["allow"] == 0 - - -@pytest.mark.asyncio -async def test_parent_scoped_candidates(db): - await seed_catalog(db) - plugins = [ - plugin_org_policy_deny_parent("hr"), - plugin_allow_parent_for_user("carol", "analytics"), - ] - rows = await resolve_permissions_from_catalog( - db, - {"id": "carol"}, - plugins, - VIEW_TABLE, - PARENT_CANDIDATES_SQL, - implicit_deny=True, - ) - d = {r["resource"]: r["allow"] for r in rows} - assert d["/analytics"] == 1 - assert d["/hr"] == 0 - - -@pytest.mark.asyncio -async def test_implicit_deny_behavior(db): - await seed_catalog(db) - plugins = [] # no rules at all - - # implicit_deny=True -> everything denied with reason 'implicit deny' - rows = await resolve_permissions_from_catalog( - db, - {"id": "erin"}, - plugins, - VIEW_TABLE, - TABLE_CANDIDATES_SQL, - implicit_deny=True, - ) - assert all(r["allow"] == 0 and r["reason"] == "implicit deny" for r in rows) - - # implicit_deny=False -> no winner => allow is None, reason is None - rows2 = await resolve_permissions_from_catalog( - db, - {"id": "erin"}, - plugins, - VIEW_TABLE, - TABLE_CANDIDATES_SQL, - implicit_deny=False, - ) - assert all(r["allow"] is None and r["reason"] is None for r in rows2) - - -@pytest.mark.asyncio -async def test_candidate_filters_via_params(db): - await seed_catalog(db) - # Add some metadata to test filtering - # Mark 'hr' as is_memory=1 and increment analytics schema_version - await db.execute_write( - "UPDATE catalog_databases SET is_memory=1 WHERE database_name='hr'" - ) - await db.execute_write( - "UPDATE catalog_databases SET schema_version=2 WHERE database_name='analytics'" - ) - - # Candidate SQL that filters by db metadata via params - candidate_sql = """ - SELECT t.database_name AS parent, t.table_name AS child - FROM catalog_tables t - JOIN catalog_databases d ON d.database_name = t.database_name - WHERE (:exclude_memory = 1 AND d.is_memory = 1) IS NOT 1 - AND (:min_schema_version IS NULL OR d.schema_version >= :min_schema_version) +async def test_alice_global_allow_with_specific_denies(ds): """ + Alice has global allow, but: + - accounting/sales is denied at child level + - hr/* is denied at parent level + She should see everything except those. + """ + # Combine three plugin callbacks into one that returns a list + deny_table_cb = _cb_deny_specific_table("alice", "perm_accounting", "sales") + deny_parent_cb = _cb_org_policy_deny_parent("perm_hr") + allow_cb = _cb_allow_all_for_user("alice") - plugins = [ - plugin_root_deny_for_all(), - plugin_allow_parent_for_user( - "dev", "analytics" - ), # analytics rescued if included by candidates - ] + def combined(datasette, actor, action): + results = [] + for cb in (allow_cb, deny_table_cb, deny_parent_cb): + r = cb(datasette, actor, action) + if r is not None: + results.append(r) + return results - # Case 1: exclude memory dbs, require schema_version >= 2 -> only analytics appear, and thus are allowed - rows = await resolve_permissions_from_catalog( - db, - {"id": "dev"}, - plugins, - VIEW_TABLE, - candidate_sql, - candidate_params={"exclude_memory": 1, "min_schema_version": 2}, - implicit_deny=True, - ) - assert rows and all(r["parent"] == "analytics" for r in rows) - assert all(r["allow"] == 1 for r in rows) + plugin = PermissionRulesPlugin(combined) + ds.pm.register(plugin, name="test_plugin") - # Case 2: include memory dbs, min_schema_version = None -> accounting/hr/analytics appear, - # but root deny wins except where specifically allowed (none except analytics parent allow doesn’t apply to table depth if candidate includes children; still fine—policy is explicit). - rows2 = await resolve_permissions_from_catalog( - db, - {"id": "dev"}, - plugins, - VIEW_TABLE, - candidate_sql, - candidate_params={"exclude_memory": 0, "min_schema_version": None}, - implicit_deny=True, - ) - assert any(r["parent"] == "accounting" for r in rows2) - assert any(r["parent"] == "hr" for r in rows2) - # For table-scoped candidates, the parent-level allow does not override root deny unless you have child-level rules - assert all(r["allow"] in (0, 1) for r in rows2) + try: + actor = {"id": "alice"} + result = await ds.allowed_resources(VIEW_TABLE, actor) + allowed = _allowed_set(result.resources) + + # accounting/sales should be denied (child deny beats global allow) + assert ("perm_accounting", "sales") not in allowed + + # All hr tables should be denied (parent deny beats global allow) + hr_tables = {(p, c) for p, c in allowed if p == "perm_hr"} + assert len(hr_tables) == 0 + + # analytics tables should all be allowed + analytics_tables = {(p, c) for p, c in allowed if p == "perm_analytics"} + assert len(analytics_tables) == 10 + + # accounting tables (except sales) should be allowed + accounting_tables = {(p, c) for p, c in allowed if p == "perm_accounting"} + assert len(accounting_tables) == 9 # 10 - 1 denied + + # Verify with allowed() single-resource checks + assert not await ds.allowed( + action=VIEW_TABLE, + resource=TableResource("perm_accounting", "sales"), + actor=actor, + ) + assert not await ds.allowed( + action=VIEW_TABLE, + resource=TableResource("perm_hr", "table01"), + actor=actor, + ) + assert await ds.allowed( + action=VIEW_TABLE, + resource=TableResource("perm_analytics", "table01"), + actor=actor, + ) + finally: + ds.pm.unregister(plugin, name="test_plugin") @pytest.mark.asyncio -async def test_action_specific_rules(db): - await seed_catalog(db) - plugins = [plugin_allow_all_for_action("dana", VIEW_TABLE)] - - view_rows = await resolve_permissions_from_catalog( - db, - {"id": "dana"}, - plugins, - VIEW_TABLE, - TABLE_CANDIDATES_SQL, - implicit_deny=True, +async def test_parent_allow_but_child_conflict_deny_wins(ds): + """ + Carol has parent-level allow on analytics, but there are conflicting + child-level allow + deny on analytics/secret. DENY should win. + hr is parent-level denied. + """ + cb_allow, cb_deny = _cb_conflicting_same_child_rules( + "carol", "perm_analytics", "secret" ) - assert view_rows and all(r["allow"] == 1 for r in view_rows) - assert all(r["action"] == VIEW_TABLE for r in view_rows) + allow_parent_cb = _cb_allow_parent_for_user("carol", "perm_analytics") + deny_parent_cb = _cb_org_policy_deny_parent("perm_hr") - insert_rows = await resolve_permissions_from_catalog( - db, - {"id": "dana"}, - plugins, - "insert-row", - TABLE_CANDIDATES_SQL, - implicit_deny=True, - ) - assert insert_rows and all(r["allow"] == 0 for r in insert_rows) - assert all(r["reason"] == "implicit deny" for r in insert_rows) - assert all(r["action"] == "insert-row" for r in insert_rows) + def combined(datasette, actor, action): + results = [] + for cb in (deny_parent_cb, allow_parent_cb, cb_allow, cb_deny): + r = cb(datasette, actor, action) + if r is not None: + results.append(r) + return results + + plugin = PermissionRulesPlugin(combined) + ds.pm.register(plugin, name="test_plugin") + + try: + actor = {"id": "carol"} + result = await ds.allowed_resources(VIEW_TABLE, actor) + analytics_allowed = _allowed_set_for_parent(result.resources, "perm_analytics") + + # analytics/secret should be denied (child deny beats child allow) + assert ("perm_analytics", "secret") not in analytics_allowed + # 10 analytics tables, 1 denied + assert len(analytics_allowed) == 9 + + # Verify via allowed() + assert not await ds.allowed( + action=VIEW_TABLE, + resource=TableResource("perm_analytics", "secret"), + actor=actor, + ) + assert await ds.allowed( + action=VIEW_TABLE, + resource=TableResource("perm_analytics", "table02"), + actor=actor, + ) + finally: + ds.pm.unregister(plugin, name="test_plugin") @pytest.mark.asyncio -async def test_actor_actor_id_action_parameters_available(db): - """Test that :actor (JSON), :actor_id, and :action are all available in SQL""" - await seed_catalog(db) +async def test_specificity_child_allow_overrides_parent_deny(ds): + """ + analytics is parent-level denied, but alice has a child-level allow + on analytics/table02. Child beats parent. + """ + allow_cb = _cb_allow_all_for_user("alice") + deny_parent_cb = _cb_org_policy_deny_parent("perm_analytics") + child_allow_cb = _cb_child_allow_for_user("alice", "perm_analytics", "table02") - def plugin_using_all_parameters() -> Callable[[str], PermissionSQL]: - def provider(action: str) -> PermissionSQL: - return PermissionSQL( - """ + def combined(datasette, actor, action): + results = [] + for cb in (allow_cb, deny_parent_cb, child_allow_cb): + r = cb(datasette, actor, action) + if r is not None: + results.append(r) + return results + + plugin = PermissionRulesPlugin(combined) + ds.pm.register(plugin, name="test_plugin") + + try: + actor = {"id": "alice"} + result = await ds.allowed_resources(VIEW_TABLE, actor) + analytics_allowed = _allowed_set_for_parent(result.resources, "perm_analytics") + + # table02 should be allowed (child allow beats parent deny) + assert ("perm_analytics", "table02") in analytics_allowed + # All other analytics tables should be denied (parent deny, no child rule) + assert len(analytics_allowed) == 1 + + # Verify via allowed() + assert await ds.allowed( + action=VIEW_TABLE, + resource=TableResource("perm_analytics", "table02"), + actor=actor, + ) + assert not await ds.allowed( + action=VIEW_TABLE, + resource=TableResource("perm_analytics", "table01"), + actor=actor, + ) + finally: + ds.pm.unregister(plugin, name="test_plugin") + + +@pytest.mark.asyncio +async def test_root_deny_all_but_parent_allow_rescues_specific_parent(ds): + """ + Global deny for all, but bob has parent-level allow on accounting. + Parent beats global. + """ + deny_cb = _cb_root_deny_for_all() + allow_cb = _cb_allow_parent_for_user("bob", "perm_accounting") + + def combined(datasette, actor, action): + results = [] + for cb in (deny_cb, allow_cb): + r = cb(datasette, actor, action) + if r is not None: + results.append(r) + return results + + plugin = PermissionRulesPlugin(combined) + ds.pm.register(plugin, name="test_plugin") + + try: + actor = {"id": "bob"} + result = await ds.allowed_resources(VIEW_TABLE, actor) + allowed = _allowed_set(result.resources) + + # Only accounting tables should be allowed + assert all(p == "perm_accounting" for p, c in allowed) + assert len(allowed) == 10 + + # Verify via allowed() + assert await ds.allowed( + action=VIEW_TABLE, + resource=TableResource("perm_accounting", "table01"), + actor=actor, + ) + assert not await ds.allowed( + action=VIEW_TABLE, + resource=TableResource("perm_hr", "table01"), + actor=actor, + ) + finally: + ds.pm.unregister(plugin, name="test_plugin") + + +@pytest.mark.asyncio +async def test_parent_scoped_action(ds): + """ + For parent-scoped resources (databases), verify cascading. + analytics allowed, hr denied, accounting implicitly denied. + """ + deny_cb = _cb_org_policy_deny_parent("perm_hr") + allow_cb = _cb_allow_parent_for_user("carol", "perm_analytics") + + def combined(datasette, actor, action): + results = [] + for cb in (deny_cb, allow_cb): + r = cb(datasette, actor, action) + if r is not None: + results.append(r) + return results + + plugin = PermissionRulesPlugin(combined) + ds.pm.register(plugin, name="test_plugin") + + try: + actor = {"id": "carol"} + result = await ds.allowed_resources("view-database", actor) + allowed = {r.parent for r in result.resources} + + assert "perm_analytics" in allowed + # hr is explicitly denied, accounting has no matching rule → implicit deny + assert "perm_hr" not in allowed + + # Verify via allowed() + assert await ds.allowed( + action="view-database", + resource=DatabaseResource("perm_analytics"), + actor=actor, + ) + assert not await ds.allowed( + action="view-database", + resource=DatabaseResource("perm_hr"), + actor=actor, + ) + finally: + ds.pm.unregister(plugin, name="test_plugin") + + +@pytest.mark.asyncio +async def test_implicit_deny_when_no_rules(ds): + """ + When no plugins return any rules, everything is denied (implicit deny). + """ + def no_rules(datasette, actor, action): + return None + + plugin = PermissionRulesPlugin(no_rules) + ds.pm.register(plugin, name="test_plugin") + + try: + actor = {"id": "erin"} + result = await ds.allowed_resources(VIEW_TABLE, actor) + assert len(result.resources) == 0 + + # Single resource check too + assert not await ds.allowed( + action=VIEW_TABLE, + resource=TableResource("perm_accounting", "table01"), + actor=actor, + ) + finally: + ds.pm.unregister(plugin, name="test_plugin") + + +@pytest.mark.asyncio +async def test_action_specific_rules(ds): + """ + Rules that only apply to view-table should not grant insert-row. + """ + cb = _cb_allow_all_for_action("dana", VIEW_TABLE) + plugin = PermissionRulesPlugin(cb) + ds.pm.register(plugin, name="test_plugin") + + try: + actor = {"id": "dana"} + + # view-table should be allowed + result = await ds.allowed_resources(VIEW_TABLE, actor) + assert len(result.resources) == 30 # 3 dbs x 10 tables + + assert await ds.allowed( + action=VIEW_TABLE, + resource=TableResource("perm_accounting", "table01"), + actor=actor, + ) + + # insert-row should be denied (no rules for it) + assert not await ds.allowed( + action="insert-row", + resource=TableResource("perm_accounting", "table01"), + actor=actor, + ) + finally: + ds.pm.unregister(plugin, name="test_plugin") + + +@pytest.mark.asyncio +async def test_actor_parameters_available_in_sql(ds): + """ + Test that :actor (JSON), :actor_id, and :action are all available in plugin SQL. + """ + def cb(datasette, actor, action): + return PermissionSQL( + sql=""" SELECT NULL AS parent, NULL AS child, 1 AS allow, 'Actor ID: ' || COALESCE(:actor_id, 'null') || - ', Actor JSON: ' || COALESCE(:actor, 'null') || ', Action: ' || :action AS reason WHERE :actor_id = 'test_user' AND :action = 'view-table' AND json_extract(:actor, '$.role') = 'admin' - """ - ) + """, + params={}, # :actor_id, :actor, :action are added by the framework + ) - return provider + plugin = PermissionRulesPlugin(cb) + ds.pm.register(plugin, name="test_plugin") - plugins = [plugin_using_all_parameters()] + try: + actor = {"id": "test_user", "role": "admin"} - # Test with full actor dict - rows = await resolve_permissions_from_catalog( - db, - {"id": "test_user", "role": "admin"}, - plugins, - "view-table", - TABLE_CANDIDATES_SQL, - implicit_deny=True, - ) + # Should be allowed because the SQL conditions are met + assert await ds.allowed( + action=VIEW_TABLE, + resource=TableResource("perm_accounting", "table01"), + actor=actor, + ) - # Should have allowed rows with reason containing all the info - allowed = [r for r in rows if r["allow"] == 1] - assert len(allowed) > 0 + # Different actor should be denied + assert not await ds.allowed( + action=VIEW_TABLE, + resource=TableResource("perm_accounting", "table01"), + actor={"id": "other_user", "role": "admin"}, + ) - # Check that the reason string contains evidence of all parameters - reason = allowed[0]["reason"] - assert "test_user" in reason - assert "view-table" in reason - # The :actor parameter should be the JSON string - assert "Actor JSON:" in reason + # Verify allowed_resources also works + result = await ds.allowed_resources(VIEW_TABLE, actor) + assert len(result.resources) == 30 # all tables allowed + finally: + ds.pm.unregister(plugin, name="test_plugin") @pytest.mark.asyncio -async def test_multiple_plugins_with_own_parameters(db): +async def test_multiple_plugins_with_own_parameters(ds): """ - Test that multiple plugins can use their own parameter names without conflict. - - This verifies that the parameter naming convention works: plugins prefix their - parameters (e.g., :plugin1_pattern, :plugin2_message) and both sets of parameters - are successfully bound in the SQL queries. + Multiple plugins can use their own parameter names without conflict. """ - await seed_catalog(db) - - def plugin_one() -> Callable[[str], PermissionSQL]: - def provider(action: str) -> PermissionSQL: - if action != "view-table": - return PermissionSQL("plugin_one", "SELECT NULL WHERE 0", {}) - return PermissionSQL( - """ + def cb_one(datasette, actor, action): + if action != VIEW_TABLE: + return None + return PermissionSQL( + sql=""" SELECT database_name AS parent, table_name AS child, - 1 AS allow, 'Plugin one used param: ' || :plugin1_param AS reason + 1 AS allow, 'Plugin one: ' || :p1_param AS reason FROM catalog_tables - WHERE database_name = 'accounting' - """, - { - "plugin1_param": "value1", - }, - ) + WHERE database_name = 'perm_accounting' + """, + params={"p1_param": "value1"}, + ) - return provider - - def plugin_two() -> Callable[[str], PermissionSQL]: - def provider(action: str) -> PermissionSQL: - if action != "view-table": - return PermissionSQL("plugin_two", "SELECT NULL WHERE 0", {}) - return PermissionSQL( - """ + def cb_two(datasette, actor, action): + if action != VIEW_TABLE: + return None + return PermissionSQL( + sql=""" SELECT database_name AS parent, table_name AS child, - 1 AS allow, 'Plugin two used param: ' || :plugin2_param AS reason + 1 AS allow, 'Plugin two: ' || :p2_param AS reason FROM catalog_tables - WHERE database_name = 'hr' - """, - { - "plugin2_param": "value2", - }, - ) + WHERE database_name = 'perm_hr' + """, + params={"p2_param": "value2"}, + ) - return provider + plugin_one = PermissionRulesPlugin(cb_one) + plugin_two = PermissionRulesPlugin(cb_two) + ds.pm.register(plugin_one, name="test_plugin_one") + ds.pm.register(plugin_two, name="test_plugin_two") - plugins = [plugin_one(), plugin_two()] + try: + actor = {"id": "test_user"} + result = await ds.allowed_resources(VIEW_TABLE, actor, include_reasons=True) + allowed = _allowed_set(result.resources) - rows = await resolve_permissions_from_catalog( - db, - {"id": "test_user"}, - plugins, - "view-table", - TABLE_CANDIDATES_SQL, - implicit_deny=False, - ) + # Both plugins should contribute — accounting from plugin one, hr from plugin two + accounting_allowed = {(p, c) for p, c in allowed if p == "perm_accounting"} + hr_allowed = {(p, c) for p, c in allowed if p == "perm_hr"} - # 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"] - ] + assert len(accounting_allowed) == 10 + assert len(hr_allowed) == 10 - 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" + # Check reasons contain the parameterized values + for r in result.resources: + if r.parent == "perm_accounting": + assert any("value1" in reason for reason in r.reasons) + elif r.parent == "perm_hr": + assert any("value2" in reason for reason in r.reasons) + finally: + ds.pm.unregister(plugin_one, name="test_plugin_one") + ds.pm.unregister(plugin_two, name="test_plugin_two") From 6a5086b23cc4f80595250884398c0736f2513edd Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 6 Feb 2026 02:25:51 +0000 Subject: [PATCH 004/481] Apply Black formatting to test_utils_permissions.py https://claude.ai/code/session_013EkyroQKPhcjdMbpHc9g4X --- tests/test_utils_permissions.py | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/tests/test_utils_permissions.py b/tests/test_utils_permissions.py index 8105e8f4..b1ba3091 100644 --- a/tests/test_utils_permissions.py +++ b/tests/test_utils_permissions.py @@ -27,6 +27,7 @@ from datasette import hookimpl # Helpers # --------------------------------------------------------------------------- + class PermissionRulesPlugin: """Thin shim that delegates to a callback for permission_resources_sql.""" @@ -59,7 +60,11 @@ async def ds(): per_parent = 10 parents = ["perm_accounting", "perm_hr", "perm_analytics"] - specials = {"perm_accounting": ["sales"], "perm_analytics": ["secret"], "perm_hr": []} + specials = { + "perm_accounting": ["sales"], + "perm_analytics": ["secret"], + "perm_hr": [], + } for parent in parents: db = instance.add_memory_database(parent) @@ -83,8 +88,10 @@ async def ds(): # Plugin factories — return callables suitable for PermissionRulesPlugin # --------------------------------------------------------------------------- + def _cb_allow_all_for_user(user): """Global allow for a specific user.""" + def cb(datasette, actor, action): if not actor or actor.get("id") != user: return None @@ -95,11 +102,13 @@ def _cb_allow_all_for_user(user): ), params={"_aau_user": user}, ) + return cb def _cb_deny_specific_table(user, parent, child): """Child-level deny for a specific user + table.""" + def cb(datasette, actor, action): if not actor or actor.get("id") != user: return None @@ -110,11 +119,13 @@ def _cb_deny_specific_table(user, parent, child): ), params={"_dst_parent": parent, "_dst_child": child, "_dst_user": user}, ) + return cb def _cb_org_policy_deny_parent(parent): """Unconditional parent-level deny (applies to all actors).""" + def cb(datasette, actor, action): return PermissionSQL( sql=( @@ -123,11 +134,13 @@ def _cb_org_policy_deny_parent(parent): ), params={"_opd_parent": parent}, ) + return cb def _cb_allow_parent_for_user(user, parent): """Parent-level allow for a specific user.""" + def cb(datasette, actor, action): if not actor or actor.get("id") != user: return None @@ -138,11 +151,13 @@ def _cb_allow_parent_for_user(user, parent): ), params={"_apu_parent": parent, "_apu_user": user}, ) + return cb def _cb_child_allow_for_user(user, parent, child): """Child-level allow for a specific user.""" + def cb(datasette, actor, action): if not actor or actor.get("id") != user: return None @@ -153,11 +168,13 @@ def _cb_child_allow_for_user(user, parent, child): ), params={"_cau_parent": parent, "_cau_child": child, "_cau_user": user}, ) + return cb def _cb_root_deny_for_all(): """Unconditional global deny.""" + def cb(datasette, actor, action): return PermissionSQL( sql=( @@ -165,11 +182,13 @@ def _cb_root_deny_for_all(): "'root deny for all' AS reason" ), ) + return cb def _cb_conflicting_same_child_rules(user, parent, child): """Two plugins: one allow + one deny at the same child level.""" + def cb_allow(datasette, actor, action): if not actor or actor.get("id") != user: return None @@ -197,6 +216,7 @@ def _cb_conflicting_same_child_rules(user, parent, child): def _cb_allow_all_for_action(user, allowed_action): """Global allow for a specific user on a specific action only.""" + def cb(datasette, actor, action): if action != allowed_action: return None @@ -209,6 +229,7 @@ def _cb_allow_all_for_action(user, allowed_action): ), params={"_aafa_user": user}, ) + return cb @@ -216,6 +237,7 @@ def _cb_allow_all_for_action(user, allowed_action): # Helpers for asserting results # --------------------------------------------------------------------------- + def _allowed_set(resources): """Convert PaginatedResources.resources to {(parent, child), ...}.""" return {(r.parent, r.child) for r in resources} @@ -484,6 +506,7 @@ async def test_implicit_deny_when_no_rules(ds): """ When no plugins return any rules, everything is denied (implicit deny). """ + def no_rules(datasette, actor, action): return None @@ -542,6 +565,7 @@ async def test_actor_parameters_available_in_sql(ds): """ Test that :actor (JSON), :actor_id, and :action are all available in plugin SQL. """ + def cb(datasette, actor, action): return PermissionSQL( sql=""" @@ -586,6 +610,7 @@ async def test_multiple_plugins_with_own_parameters(ds): """ Multiple plugins can use their own parameter names without conflict. """ + def cb_one(datasette, actor, action): if action != VIEW_TABLE: return None From 80b7f987cad59113896f28a29828ffe856218216 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Mon, 9 Feb 2026 13:20:33 -0800 Subject: [PATCH 005/481] write_wrapper plugin hook for intercepting write operations (#2636) * Implement write_wrapper plugin hook for intercepting database writes Add a new `write_wrapper` plugin hook that lets plugins wrap write operations with before/after logic using a generator-based context manager pattern. The hook receives (datasette, database, request, transaction) and returns a generator function that takes a conn, yields once to let the write execute, and can run cleanup after. The write result is sent back via `generator.send()` and exceptions are thrown via `generator.throw()`, giving plugins full visibility. Also adds `request=None` parameter to execute_write, execute_write_fn, execute_write_script, and execute_write_many, and threads request through all view-layer call sites (insert, upsert, update, delete, drop, create table, canned queries). * Add documentation for wrap_write hook, fix lint issues Document the wrap_write plugin hook in plugin_hooks.rst with parameter descriptions and two examples: a simple logging wrapper and an advanced SQLite authorizer-based table protection pattern. Also fix black formatting and remove unused variable flagged by ruff. * Rename wrap_write hook to write_wrapper for consistency with asgi_wrapper * Move write_wrapper docs to just below prepare_connection * Refactor write_wrapper tests to use pytest.parametrize Consolidate duplicate test cases: merge before/after tests for execute_write_fn and execute_write into one parametrized test, and merge three parameter-passing tests into one parametrized test. Claude Code transcript: https://gisthost.github.io/?c4c12079434e69677e4aa8ac664b21b8/index.html --- datasette/database.py | 77 ++++++- datasette/hookspecs.py | 22 ++ datasette/views/database.py | 6 +- datasette/views/row.py | 4 +- datasette/views/table.py | 4 +- docs/plugin_hooks.rst | 87 ++++++++ tests/test_plugins.py | 30 +++ tests/test_write_wrapper.py | 387 ++++++++++++++++++++++++++++++++++++ 8 files changed, 604 insertions(+), 13 deletions(-) create mode 100644 tests/test_write_wrapper.py diff --git a/datasette/database.py b/datasette/database.py index 8e4ee2b6..1e6f9032 100644 --- a/datasette/database.py +++ b/datasette/database.py @@ -130,25 +130,25 @@ class Database: for connection in self._all_file_connections: connection.close() - async def execute_write(self, sql, params=None, block=True): + async def execute_write(self, sql, params=None, block=True, request=None): def _inner(conn): return conn.execute(sql, params or []) with trace("sql", database=self.name, sql=sql.strip(), params=params): - results = await self.execute_write_fn(_inner, block=block) + results = await self.execute_write_fn(_inner, block=block, request=request) return results - async def execute_write_script(self, sql, block=True): + async def execute_write_script(self, sql, block=True, request=None): def _inner(conn): return conn.executescript(sql) with trace("sql", database=self.name, sql=sql.strip(), executescript=True): results = await self.execute_write_fn( - _inner, block=block, transaction=False + _inner, block=block, transaction=False, request=request ) return results - async def execute_write_many(self, sql, params_seq, block=True): + async def execute_write_many(self, sql, params_seq, block=True, request=None): def _inner(conn): count = 0 @@ -163,7 +163,9 @@ class Database: with trace( "sql", database=self.name, sql=sql.strip(), executemany=True ) as kwargs: - results, count = await self.execute_write_fn(_inner, block=block) + results, count = await self.execute_write_fn( + _inner, block=block, request=request + ) kwargs["count"] = count return results @@ -187,7 +189,8 @@ class Database: # Threaded mode - send to write thread return await self._send_to_write_thread(fn, isolated_connection=True) - async def execute_write_fn(self, fn, block=True, transaction=True): + async def execute_write_fn(self, fn, block=True, transaction=True, request=None): + fn = self._wrap_fn_with_hooks(fn, request, transaction) if self.ds.executor is None: # non-threaded mode if self._write_connection is None: @@ -203,6 +206,25 @@ class Database: fn, block=block, transaction=transaction ) + def _wrap_fn_with_hooks(self, fn, request, transaction): + from .plugins import pm + + wrappers = pm.hook.write_wrapper( + datasette=self.ds, + database=self.name, + request=request, + transaction=transaction, + ) + wrappers = [w for w in wrappers if w is not None] + if not wrappers: + return fn + # Build the wrapped fn by nesting context manager generators. + # The first wrapper returned by pluggy is outermost. + original_fn = fn + for wrapper_factory in reversed(wrappers): + original_fn = _apply_write_wrapper(original_fn, wrapper_factory) + return original_fn + async def _send_to_write_thread( self, fn, block=True, isolated_connection=False, transaction=True ): @@ -680,6 +702,47 @@ class Database: return f"" +def _apply_write_wrapper(fn, wrapper_factory): + """Apply a single write_wrapper context manager around fn. + + ``wrapper_factory`` is a callable that takes ``(conn)`` and returns a + generator that yields exactly once. Code before the yield runs before + ``fn(conn)``, code after the yield runs after. The result of + ``fn(conn)`` is sent into the generator via ``.send()``, and any + exception raised by ``fn(conn)`` is thrown via ``.throw()``. + """ + + def wrapped(conn): + gen = wrapper_factory(conn) + # Advance to the yield point (run "before" code) + try: + next(gen) + except StopIteration: + # Generator didn't yield — just run fn unchanged + return fn(conn) + + # Execute the actual write + try: + result = fn(conn) + except Exception: + # Throw exception into generator so it can handle it + try: + gen.throw(*sys.exc_info()) + except StopIteration: + pass + # Re-raise the original exception + raise + else: + # Send the result back through the yield + try: + gen.send(result) + except StopIteration: + pass + return result + + return wrapped + + class WriteTask: __slots__ = ("fn", "task_id", "reply_queue", "isolated_connection", "transaction") diff --git a/datasette/hookspecs.py b/datasette/hookspecs.py index 3f6a1425..b993fb61 100644 --- a/datasette/hookspecs.py +++ b/datasette/hookspecs.py @@ -220,3 +220,25 @@ def top_query(datasette, request, database, sql): @hookspec def top_canned_query(datasette, request, database, query_name): """HTML to include at the top of the canned query page""" + + +@hookspec +def write_wrapper(datasette, database, request, transaction): + """Called when a write function is about to execute. + + Return a generator function that accepts a ``conn`` argument. + The generator should ``yield`` exactly once: code before the + ``yield`` runs before the write, code after the ``yield`` runs + after the write completes. The result of the write is sent + back through the ``yield``, so you can capture it with + ``result = yield``. + + If the write raises an exception, it is thrown into the generator + so you can handle it with a try/except around the ``yield``. + + ``request`` may be ``None`` for writes not originating from an + HTTP request. ``transaction`` is ``True`` if the write will + be wrapped in a transaction. + + Return ``None`` to skip wrapping. + """ diff --git a/datasette/views/database.py b/datasette/views/database.py index 51c752a0..e5f2cf16 100644 --- a/datasette/views/database.py +++ b/datasette/views/database.py @@ -466,7 +466,9 @@ class QueryView(View): ok = None redirect_url = None try: - cursor = await db.execute_write(canned_query["sql"], params_for_query) + cursor = await db.execute_write( + canned_query["sql"], params_for_query, request=request + ) # success message can come from on_success_message or on_success_message_sql message = None message_type = datasette.INFO @@ -1119,7 +1121,7 @@ class TableCreateView(BaseView): return table.schema try: - schema = await db.execute_write_fn(create_table) + schema = await db.execute_write_fn(create_table, request=request) except Exception as e: return _error([str(e)]) diff --git a/datasette/views/row.py b/datasette/views/row.py index 718ee00c..ff0a3594 100644 --- a/datasette/views/row.py +++ b/datasette/views/row.py @@ -245,7 +245,7 @@ class RowDeleteView(BaseView): sqlite_utils.Database(conn)[resolved.table].delete(resolved.pk_values) try: - await resolved.db.execute_write_fn(delete_row) + await resolved.db.execute_write_fn(delete_row, request=request) except Exception as e: return _error([str(e)], 500) @@ -305,7 +305,7 @@ class RowUpdateView(BaseView): ) try: - await resolved.db.execute_write_fn(update_row) + await resolved.db.execute_write_fn(update_row, request=request) except Exception as e: return _error([str(e)], 400) diff --git a/datasette/views/table.py b/datasette/views/table.py index b07b62ae..d4dbc194 100644 --- a/datasette/views/table.py +++ b/datasette/views/table.py @@ -550,7 +550,7 @@ class TableInsertView(BaseView): method_all(rows, **kwargs) try: - rows = await db.execute_write_fn(insert_or_upsert_rows) + rows = await db.execute_write_fn(insert_or_upsert_rows, request=request) except Exception as e: return _error([str(e)]) result = {"ok": True} @@ -670,7 +670,7 @@ class TableDropView(BaseView): def drop_table(conn): sqlite_utils.Database(conn)[table_name].drop() - await db.execute_write_fn(drop_table) + await db.execute_write_fn(drop_table, request=request) await self.ds.track_event( DropTableEvent( actor=request.actor, database=database_name, table=table_name diff --git a/docs/plugin_hooks.rst b/docs/plugin_hooks.rst index ad4a70f8..468b0ade 100644 --- a/docs/plugin_hooks.rst +++ b/docs/plugin_hooks.rst @@ -61,6 +61,92 @@ arguments and can be called like this:: Examples: `datasette-jellyfish `__, `datasette-jq `__, `datasette-haversine `__, `datasette-rure `__ +.. _plugin_hook_write_wrapper: + +write_wrapper(datasette, database, request, transaction) +-------------------------------------------------------- + +``datasette`` - :ref:`internals_datasette` + You can use this to access plugin configuration options via ``datasette.plugin_config(your_plugin_name)``. + +``database`` - string + The name of the database being written to. + +``request`` - :ref:`internals_request` or ``None`` + The HTTP request that triggered this write, if available. This will be ``None`` for writes that do not originate from an HTTP request (e.g. writes triggered by plugins during startup). + +``transaction`` - bool + ``True`` if the write will be wrapped in a database transaction. + +Return a generator function that accepts a ``conn`` argument (a SQLite connection object). The generator should ``yield`` exactly once. Code before the ``yield`` runs before the write function executes; code after the ``yield`` runs after it completes. + +The result of the write function is sent back through the ``yield``, so you can capture it with ``result = yield``. + +If the write function raises an exception, it is thrown into the generator so you can handle it with a ``try`` / ``except`` around the ``yield``. + +Return ``None`` to skip wrapping for this particular write. + +This example logs every write operation: + +.. code-block:: python + + from datasette import hookimpl + + + @hookimpl + def write_wrapper(datasette, database, request): + def wrapper(conn): + print(f"Before write to {database}") + result = yield + print(f"After write to {database}") + + return wrapper + +This more advanced example uses the SQLite authorizer callback to block writes to a specific table for non-admin users: + +.. code-block:: python + + import sqlite3 + from datasette import hookimpl + + WRITE_ACTIONS = ( + sqlite3.SQLITE_INSERT, + sqlite3.SQLITE_UPDATE, + sqlite3.SQLITE_DELETE, + ) + + + @hookimpl + def write_wrapper(datasette, database, request): + actor = None + if request: + actor = request.actor + if actor and actor.get("id") == "admin": + return None + + def wrapper(conn): + def authorizer( + action, arg1, arg2, db_name, trigger + ): + if ( + action in WRITE_ACTIONS + and arg1 == "protected_table" + ): + return sqlite3.SQLITE_DENY + return sqlite3.SQLITE_OK + + conn.set_authorizer(authorizer) + try: + yield + finally: + conn.set_authorizer(None) + + return wrapper + +The ``conn`` object passed to the generator is the same connection that the write function will use. Because the generator and the write function execute together in a single call on the write thread, any state you set on the connection (authorizers, pragmas, temporary tables) is visible to the write and can be cleaned up afterwards. + +When multiple plugins implement ``write_wrapper``, they are nested following pluggy's default calling convention. + .. _plugin_hook_prepare_jinja2_environment: prepare_jinja2_environment(env, datasette) @@ -2249,3 +2335,4 @@ The plugin can then call ``datasette.track_event(...)`` to send a ``ban-user`` e await datasette.track_event( BanUserEvent(user={"id": 1, "username": "cleverbot"}) ) + diff --git a/tests/test_plugins.py b/tests/test_plugins.py index 6c23b3ef..7c2180e8 100644 --- a/tests/test_plugins.py +++ b/tests/test_plugins.py @@ -1524,6 +1524,36 @@ async def test_hook_register_events(): assert any(k.__name__ == "OneEvent" for k in datasette.event_classes) +@pytest.mark.asyncio +async def test_hook_write_wrapper(): + datasette = Datasette(memory=True) + log = [] + + class WrapWritePlugin: + __name__ = "WrapWritePlugin" + + @staticmethod + @hookimpl + def write_wrapper(datasette, database, request, transaction): + if database != "_memory": + return None + + def wrapper(conn): + log.append("before") + yield + log.append("after") + + return wrapper + + pm.register(WrapWritePlugin(), name="WrapWritePluginTest") + try: + db = datasette.get_database("_memory") + await db.execute_write("create table t (id integer primary key)") + assert log == ["before", "after"] + finally: + pm.unregister(name="WrapWritePluginTest") + + @pytest.mark.asyncio async def test_hook_register_actions_view_collection(): datasette = Datasette(memory=True, plugins_dir=PLUGINS_DIR) diff --git a/tests/test_write_wrapper.py b/tests/test_write_wrapper.py new file mode 100644 index 00000000..e05a2a9f --- /dev/null +++ b/tests/test_write_wrapper.py @@ -0,0 +1,387 @@ +""" +Tests for the write_wrapper plugin hook. +""" + +from datasette.app import Datasette +from datasette.hookspecs import hookimpl +from datasette.plugins import pm +import pytest +import time + + +@pytest.fixture +def datasette(tmp_path): + db_path = str(tmp_path / "test.db") + ds = Datasette([db_path]) + return ds + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "use_execute_write", + (False, True), + ids=["execute_write_fn", "execute_write"], +) +async def test_write_wrapper_before_and_after(datasette, use_execute_write): + """Test that code before and after yield both execute.""" + log = [] + + class Plugin: + __name__ = "Plugin" + + @staticmethod + @hookimpl + def write_wrapper(datasette, database, request, transaction): + def wrapper(conn): + log.append("before") + yield + log.append("after") + + return wrapper + + pm.register(Plugin(), name="test_before_after") + try: + db = datasette.get_database("test") + if use_execute_write: + await db.execute_write( + "create table if not exists t (id integer primary key)" + ) + else: + await db.execute_write_fn( + lambda conn: conn.execute( + "create table if not exists t (id integer primary key)" + ) + ) + assert log == ["before", "after"] + finally: + pm.unregister(name="test_before_after") + + +@pytest.mark.asyncio +async def test_write_wrapper_receives_result_via_yield(datasette): + """Test that the result of fn(conn) is sent back through yield.""" + captured = {} + + class Plugin: + __name__ = "Plugin" + + @staticmethod + @hookimpl + def write_wrapper(datasette, database, request, transaction): + def wrapper(conn): + result = yield + captured["result"] = result + + return wrapper + + pm.register(Plugin(), name="test_result") + try: + db = datasette.get_database("test") + await db.execute_write_fn( + lambda conn: conn.execute( + "create table if not exists t2 (id integer primary key)" + ) + ) + assert "result" in captured + # Should be a sqlite3 Cursor + assert captured["result"] is not None + finally: + pm.unregister(name="test_result") + + +@pytest.mark.asyncio +async def test_write_wrapper_exception_thrown_into_generator(datasette): + """Test that exceptions from fn(conn) are thrown into the generator.""" + caught = {} + + class Plugin: + __name__ = "Plugin" + + @staticmethod + @hookimpl + def write_wrapper(datasette, database, request, transaction): + def wrapper(conn): + try: + yield + except Exception as e: + caught["error"] = e + + return wrapper + + pm.register(Plugin(), name="test_exception") + try: + db = datasette.get_database("test") + with pytest.raises(Exception, match="deliberate"): + await db.execute_write_fn( + lambda conn: (_ for _ in ()).throw(Exception("deliberate")) + ) + assert "error" in caught + assert str(caught["error"]) == "deliberate" + finally: + pm.unregister(name="test_exception") + + +@pytest.mark.asyncio +async def test_write_wrapper_conn_is_usable(datasette): + """Test that the conn passed to the wrapper can execute SQL.""" + + class Plugin: + __name__ = "Plugin" + + @staticmethod + @hookimpl + def write_wrapper(datasette, database, request, transaction): + def wrapper(conn): + conn.execute("create table if not exists hook_log (msg text)") + conn.execute("insert into hook_log values ('before')") + yield + conn.execute("insert into hook_log values ('after')") + + return wrapper + + pm.register(Plugin(), name="test_conn") + try: + db = datasette.get_database("test") + await db.execute_write_fn( + lambda conn: conn.execute( + "create table if not exists t3 (id integer primary key)" + ) + ) + result = await db.execute("select msg from hook_log order by rowid") + messages = [row[0] for row in result.rows] + assert messages == ["before", "after"] + finally: + pm.unregister(name="test_conn") + + +@pytest.mark.asyncio +async def test_write_wrapper_multiple_plugins_nest(datasette): + """Test that multiple write_wrapper plugins nest correctly.""" + log = [] + + class PluginA: + __name__ = "PluginA" + + @staticmethod + @hookimpl + def write_wrapper(datasette, database, request, transaction): + def wrapper(conn): + log.append("A-before") + yield + log.append("A-after") + + return wrapper + + class PluginB: + __name__ = "PluginB" + + @staticmethod + @hookimpl + def write_wrapper(datasette, database, request, transaction): + def wrapper(conn): + log.append("B-before") + yield + log.append("B-after") + + return wrapper + + pm.register(PluginA(), name="PluginA") + pm.register(PluginB(), name="PluginB") + try: + db = datasette.get_database("test") + await db.execute_write_fn( + lambda conn: conn.execute( + "create table if not exists t4 (id integer primary key)" + ) + ) + assert set(log) == {"A-before", "A-after", "B-before", "B-after"} + # Verify proper nesting: each plugin's before/after should be + # symmetric around the write + a_before = log.index("A-before") + a_after = log.index("A-after") + b_before = log.index("B-before") + b_after = log.index("B-after") + if a_before < b_before: + assert a_after > b_after, "A is outer so A-after should come after B-after" + else: + assert b_after > a_after, "B is outer so B-after should come after A-after" + finally: + pm.unregister(name="PluginA") + pm.unregister(name="PluginB") + + +@pytest.mark.asyncio +async def test_write_wrapper_return_none_skips(datasette): + """Test that returning None from write_wrapper means no wrapping.""" + log = [] + + class Plugin: + __name__ = "Plugin" + + @staticmethod + @hookimpl + def write_wrapper(datasette, database, request, transaction): + log.append("hook-called") + return None + + pm.register(Plugin(), name="test_skip") + try: + db = datasette.get_database("test") + await db.execute_write_fn( + lambda conn: conn.execute( + "create table if not exists t5 (id integer primary key)" + ) + ) + assert log == ["hook-called"] + finally: + pm.unregister(name="test_skip") + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "request_value,transaction_value,expected_request,expected_transaction", + ( + ("fake-request", True, "fake-request", True), + (None, True, None, True), + (None, False, None, False), + ), + ids=["with-request", "request-none-by-default", "transaction-false"], +) +async def test_write_wrapper_hook_parameters( + datasette, + request_value, + transaction_value, + expected_request, + expected_transaction, +): + """Test that request and transaction parameters are passed through.""" + captured = {} + + class Plugin: + __name__ = "Plugin" + + @staticmethod + @hookimpl + def write_wrapper(datasette, database, request, transaction): + captured["request"] = request + captured["database"] = database + captured["transaction"] = transaction + + pm.register(Plugin(), name="test_params") + try: + db = datasette.get_database("test") + kwargs = {"transaction": transaction_value} + if request_value is not None: + kwargs["request"] = request_value + await db.execute_write_fn( + lambda conn: conn.execute( + "create table if not exists t6 (id integer primary key)" + ), + **kwargs, + ) + assert captured["request"] == expected_request + assert captured["database"] == "test" + assert captured["transaction"] == expected_transaction + finally: + pm.unregister(name="test_params") + + +@pytest.mark.asyncio +async def test_write_wrapper_via_api(tmp_path): + """Test that write_wrapper fires for API write operations.""" + log = [] + + db_path = str(tmp_path / "test.db") + ds = Datasette([db_path], pdb=False) + ds.root_enabled = True + + class Plugin: + __name__ = "Plugin" + + @staticmethod + @hookimpl + def write_wrapper(datasette, database, request, transaction): + if database != "test": + return None + + def wrapper(conn): + log.append("before") + yield + log.append("after") + + return wrapper + + pm.register(Plugin(), name="test_api") + try: + db = ds.get_database("test") + await db.execute_write( + "create table if not exists api_test (id integer primary key, name text)" + ) + log.clear() + + token = "dstok_{}".format( + ds.sign( + {"a": "root", "token": "dstok", "t": int(time.time())}, + namespace="token", + ) + ) + response = await ds.client.post( + "/test/api_test/-/insert", + json={"row": {"name": "test"}, "return": True}, + headers={ + "Authorization": "Bearer {}".format(token), + "Content-Type": "application/json", + }, + ) + assert response.status_code == 201, response.json() + assert log == ["before", "after"] + finally: + pm.unregister(name="test_api") + + +@pytest.mark.asyncio +async def test_write_wrapper_change_group_pattern(datasette): + """Test the motivating use case: activating a change group around a write.""" + db = datasette.get_database("test") + + await db.execute_write( + "create table if not exists groups (id integer primary key, current integer)" + ) + await db.execute_write( + "create table if not exists data (id integer primary key, value text)" + ) + await db.execute_write("insert into groups (id, current) values (1, null)") + + class Plugin: + __name__ = "Plugin" + + @staticmethod + @hookimpl + def write_wrapper(datasette, database, request, transaction): + if request and getattr(request, "group_id", None): + group_id = request.group_id + + def wrapper(conn): + conn.execute( + "update groups set current = 1 where id = ?", [group_id] + ) + yield + conn.execute("update groups set current = null where current = 1") + + return wrapper + + pm.register(Plugin(), name="test_change_group") + try: + + class FakeRequest: + group_id = 1 + + await db.execute_write_fn( + lambda conn: conn.execute("insert into data (value) values ('test')"), + request=FakeRequest(), + ) + + result = await db.execute("select current from groups where id = 1") + assert result.rows[0][0] is None + finally: + pm.unregister(name="test_change_group") From 8a315f3d7df8c668fdca216bbb55fe7ef44626dd Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Mon, 9 Feb 2026 13:27:23 -0800 Subject: [PATCH 006/481] Added a test to exercise the write_wrapper example This example in the docs is now dulicated in a test: https://github.com/simonw/datasette/blob/80b7f987cad59113896f28a29828ffe856218216/docs/plugin_hooks.rst#write-wrapper-datasette-database-request-transaction Refs #2637 --- tests/test_write_wrapper.py | 90 +++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/tests/test_write_wrapper.py b/tests/test_write_wrapper.py index e05a2a9f..38e5c94e 100644 --- a/tests/test_write_wrapper.py +++ b/tests/test_write_wrapper.py @@ -6,6 +6,7 @@ from datasette.app import Datasette from datasette.hookspecs import hookimpl from datasette.plugins import pm import pytest +import sqlite3 import time @@ -385,3 +386,92 @@ async def test_write_wrapper_change_group_pattern(datasette): assert result.rows[0][0] is None finally: pm.unregister(name="test_change_group") + + +WRITE_ACTIONS = ( + sqlite3.SQLITE_INSERT, + sqlite3.SQLITE_UPDATE, + sqlite3.SQLITE_DELETE, +) + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "actor,table,should_deny", + ( + (None, "protected_table", True), + ({"id": "regular"}, "protected_table", True), + ({"id": "admin"}, "protected_table", False), + (None, "other_table", False), + ({"id": "regular"}, "other_table", False), + ), + ids=[ + "no-actor-protected", + "regular-user-protected", + "admin-protected", + "no-actor-other", + "regular-user-other", + ], +) +async def test_write_wrapper_set_authorizer(datasette, actor, table, should_deny): + """Test the docs example that uses set_authorizer to block writes to a protected table.""" + db = datasette.get_database("test") + await db.execute_write( + "create table if not exists protected_table (id integer primary key, value text)" + ) + await db.execute_write( + "create table if not exists other_table (id integer primary key, value text)" + ) + + class Plugin: + __name__ = "Plugin" + + @staticmethod + @hookimpl + def write_wrapper(datasette, database, request, transaction): + actor = None + if request: + actor = request.actor + if actor and actor.get("id") == "admin": + return None + + def wrapper(conn): + def authorizer(action, arg1, arg2, db_name, trigger): + if action in WRITE_ACTIONS and arg1 == "protected_table": + return sqlite3.SQLITE_DENY + return sqlite3.SQLITE_OK + + conn.set_authorizer(authorizer) + try: + yield + finally: + conn.set_authorizer(None) + + return wrapper + + class FakeRequest: + def __init__(self, actor): + self.actor = actor + + pm.register(Plugin(), name="test_set_authorizer") + try: + request = FakeRequest(actor) + if should_deny: + with pytest.raises(Exception): + await db.execute_write_fn( + lambda conn: conn.execute( + f"insert into {table} (value) values ('test')" + ), + request=request, + ) + else: + await db.execute_write_fn( + lambda conn: conn.execute( + f"insert into {table} (value) values ('test')" + ), + request=request, + ) + result = await db.execute(f"select value from {table} order by rowid desc limit 1") + assert result.rows[0][0] == "test" + finally: + pm.unregister(name="test_set_authorizer") From 170f9de774fd3d7487a40c9f67dc12a2c626e96e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 17 Feb 2026 18:21:25 +0000 Subject: [PATCH 007/481] Add pks parameter to render_cell() plugin hook The render_cell() hook now receives a pks parameter containing the list of primary key column names for the table being rendered. This avoids plugins needing to make redundant async calls to look up primary keys. For tables without an explicit primary key, pks is ["rowid"]. For custom SQL queries and views, pks is an empty list []. https://claude.ai/code/session_01HFYfevAziq4fSYTNRD9ZCh --- datasette/hookspecs.py | 2 +- datasette/views/database.py | 1 + datasette/views/row.py | 1 + datasette/views/table.py | 3 +++ docs/plugin_hooks.rst | 9 +++++--- tests/fixtures.py | 2 ++ tests/plugins/my_plugin.py | 3 ++- tests/test_plugins.py | 46 +++++++++++++++++++++++++++++++++++++ 8 files changed, 62 insertions(+), 5 deletions(-) diff --git a/datasette/hookspecs.py b/datasette/hookspecs.py index b993fb61..89be6a65 100644 --- a/datasette/hookspecs.py +++ b/datasette/hookspecs.py @@ -55,7 +55,7 @@ def publish_subcommand(publish): @hookspec -def render_cell(row, value, column, table, database, datasette, request): +def render_cell(row, value, column, table, pks, database, datasette, request): """Customize rendering of HTML table cell values""" diff --git a/datasette/views/database.py b/datasette/views/database.py index e5f2cf16..a42ac758 100644 --- a/datasette/views/database.py +++ b/datasette/views/database.py @@ -1205,6 +1205,7 @@ async def display_rows(datasette, database, request, rows, columns): value=value, column=column, table=None, + pks=[], database=database, datasette=datasette, request=request, diff --git a/datasette/views/row.py b/datasette/views/row.py index ff0a3594..9c59cd3b 100644 --- a/datasette/views/row.py +++ b/datasette/views/row.py @@ -130,6 +130,7 @@ class RowView(DataView): value=value, column=column, table=table, + pks=resolved.pks, database=database, datasette=self.ds, request=request, diff --git a/datasette/views/table.py b/datasette/views/table.py index d4dbc194..594e925e 100644 --- a/datasette/views/table.py +++ b/datasette/views/table.py @@ -235,6 +235,7 @@ async def display_columns_and_rows( value=value, column=column, table=table_name, + pks=pks_for_display, database=database_name, datasette=datasette, request=request, @@ -1494,6 +1495,7 @@ async def table_view_data( async def extra_render_cell(): "Rendered HTML for each cell using the render_cell plugin hook" + pks_for_display = pks if pks else (["rowid"] if not is_view else []) columns = [col[0] for col in results.description] rendered_rows = [] for row in rows: @@ -1506,6 +1508,7 @@ async def table_view_data( value=value, column=column, table=table_name, + pks=pks_for_display, database=database_name, datasette=datasette, request=request, diff --git a/docs/plugin_hooks.rst b/docs/plugin_hooks.rst index 468b0ade..068469a8 100644 --- a/docs/plugin_hooks.rst +++ b/docs/plugin_hooks.rst @@ -9,7 +9,7 @@ Each plugin can implement one or more hooks using the ``@hookimpl`` decorator ag When you implement a plugin hook you can accept any or all of the parameters that are documented as being passed to that hook. -For example, you can implement the ``render_cell`` plugin hook like this even though the full documented hook signature is ``render_cell(row, value, column, table, database, datasette)``: +For example, you can implement the ``render_cell`` plugin hook like this even though the full documented hook signature is ``render_cell(row, value, column, table, pks, database, datasette, request)``: .. code-block:: python @@ -474,8 +474,8 @@ Examples: `datasette-publish-fly Date: Tue, 17 Feb 2026 20:09:04 +0000 Subject: [PATCH 008/481] Fix test assertions broken by new fixture rows in 170f9de The render_cell pks parameter commit added rows to compound_primary_key (2->3 rows) and no_primary_key (201->202 rows) tables but did not update existing tests that had hardcoded row count expectations. https://claude.ai/code/session_01XfPSZfK57bzRRiEa7Kz5n1 --- tests/test_api.py | 4 ++-- tests/test_table_api.py | 17 +++++++++-------- tests/test_table_html.py | 6 ++++++ 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/tests/test_api.py b/tests/test_api.py index e3951df9..95958a72 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -182,7 +182,7 @@ async def test_database_page(ds_client): # -- compound primary keys compound_pk = tables_by_name["compound_primary_key"] assert compound_pk["primary_keys"] == ["pk1", "pk2"] - assert compound_pk["count"] == 2 + assert compound_pk["count"] == 3 compound_three = tables_by_name["compound_three_primary_keys"] assert compound_three["primary_keys"] == ["pk1", "pk2", "pk3"] @@ -196,7 +196,7 @@ async def test_database_page(ds_client): # -- no_primary_key: hidden table with generated data no_pk = tables_by_name["no_primary_key"] assert no_pk["hidden"] is True - assert no_pk["count"] == 201 + assert no_pk["count"] == 202 assert no_pk["primary_keys"] == [] # -- roadside attractions relationship chain diff --git a/tests/test_table_api.py b/tests/test_table_api.py index 49df3ad5..943a1549 100644 --- a/tests/test_table_api.py +++ b/tests/test_table_api.py @@ -136,6 +136,7 @@ async def test_table_shape_object_compound_primary_key(ds_client): assert response.json() == { "a,b": {"pk1": "a", "pk2": "b", "content": "c"}, "a~2Fb,~2Ec-d": {"pk1": "a/b", "pk2": ".c-d", "content": "c"}, + "d,e": {"pk1": "d", "pk2": "e", "content": "RENDER_CELL_DEMO"}, } @@ -169,11 +170,11 @@ async def test_table_with_reserved_word_name(ds_client): @pytest.mark.parametrize( "path,expected_rows,expected_pages", [ - ("/fixtures/no_primary_key.json", 201, 5), - ("/fixtures/paginated_view.json", 201, 9), - ("/fixtures/no_primary_key.json?_size=25", 201, 9), - ("/fixtures/paginated_view.json?_size=50", 201, 5), - ("/fixtures/paginated_view.json?_size=max", 201, 3), + ("/fixtures/no_primary_key.json", 202, 5), + ("/fixtures/paginated_view.json", 202, 9), + ("/fixtures/no_primary_key.json?_size=25", 202, 9), + ("/fixtures/paginated_view.json?_size=50", 202, 5), + ("/fixtures/paginated_view.json?_size=max", 202, 3), ("/fixtures/123_starts_with_digits.json", 0, 1), # Ensure faceting doesn't break pagination: ("/fixtures/compound_three_primary_keys.json?_facet=pk1", 1001, 21), @@ -232,7 +233,7 @@ async def test_page_size_zero(ds_client): ) assert response.status_code == 200 assert [] == response.json()["rows"] - assert 201 == response.json()["count"] + assert 202 == response.json()["count"] assert None is response.json()["next"] assert None is response.json()["next_url"] @@ -722,11 +723,11 @@ def test_page_size_matching_max_returned_rows( while path: response = app_client_returned_rows_matches_page_size.get(path) fetched.extend(response.json["rows"]) - assert len(response.json["rows"]) in (1, 50) + assert len(response.json["rows"]) in (2, 50) path = response.json["next_url"] if path: path = path.replace("http://localhost", "") - assert len(fetched) == 201 + assert len(fetched) == 202 @pytest.mark.asyncio diff --git a/tests/test_table_html.py b/tests/test_table_html.py index 90be591a..00cf9e19 100644 --- a/tests/test_table_html.py +++ b/tests/test_table_html.py @@ -597,6 +597,12 @@ async def test_table_html_compound_primary_key(ds_client): '.c-d', 'c', ], + [ + 'd,e', + 'd', + 'e', + '{"row": {"pk1": "d", "pk2": "e", "content": "RENDER_CELL_DEMO"}, "column": "content", "table": "compound_primary_key", "database": "fixtures", "pks": ["pk1", "pk2"], "config": {"depth": "database"}}', + ], ] assert [ [str(td) for td in tr.select("td")] for tr in table.select("tbody tr") From 5c3137d14858c0750c93bb61ef593d807cadba43 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Tue, 17 Feb 2026 13:30:24 -0800 Subject: [PATCH 009/481] Black formatting --- datasette/app.py | 10 ++----- datasette/cli.py | 8 ++--- datasette/database.py | 36 +++++------------------ datasette/default_permissions/defaults.py | 1 - datasette/facets.py | 12 ++------ datasette/inspect.py | 17 +++-------- datasette/permissions.py | 1 - datasette/utils/__init__.py | 4 +-- datasette/utils/actions_sql.py | 18 ++++-------- datasette/utils/internal_db.py | 14 +++------ datasette/utils/permissions.py | 7 ++--- datasette/views/base.py | 8 ++--- datasette/views/database.py | 8 ++--- datasette/views/index.py | 1 - datasette/views/special.py | 1 - tests/conftest.py | 1 - tests/fixtures.py | 20 +++---------- tests/plugins/my_plugin.py | 8 ++--- tests/test_cli.py | 8 ++--- tests/test_cli_serve_get.py | 4 +-- tests/test_config_dir.py | 6 ++-- tests/test_csv.py | 22 ++++---------- tests/test_html.py | 7 ++--- tests/test_internals_database.py | 12 +++----- tests/test_plugins.py | 10 ++----- tests/test_publish_cloudrun.py | 14 +++------ tests/test_routes.py | 6 ++-- tests/test_table_api.py | 8 ++--- tests/test_utils.py | 22 ++++---------- tests/test_utils_permissions.py | 6 ++-- tests/test_write_wrapper.py | 4 ++- 31 files changed, 82 insertions(+), 222 deletions(-) diff --git a/datasette/app.py b/datasette/app.py index 75f6071e..6efaa430 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -633,9 +633,7 @@ class Datasette: """ INSERT OR REPLACE INTO catalog_databases (database_name, path, is_memory, schema_version) VALUES {} - """.format( - placeholders - ), + """.format(placeholders), values, ) await populate_schema_tables(internal_db, db) @@ -813,14 +811,12 @@ class Datasette: return orig async def get_instance_metadata(self): - rows = await self.get_internal_database().execute( - """ + rows = await self.get_internal_database().execute(""" SELECT key, value FROM metadata_instance - """ - ) + """) return dict(rows) async def get_database_metadata(self, database_name: str): diff --git a/datasette/cli.py b/datasette/cli.py index 1d0cb022..121911ab 100644 --- a/datasette/cli.py +++ b/datasette/cli.py @@ -109,15 +109,11 @@ def sqlite_extensions(fn): return fn(*args, **kwargs) except AttributeError as e: if "enable_load_extension" in str(e): - raise click.ClickException( - textwrap.dedent( - """ + raise click.ClickException(textwrap.dedent(""" Your Python installation does not have the ability to load SQLite extensions. More information: https://datasette.io/help/extensions - """ - ).strip() - ) + """).strip()) raise return wrapped diff --git a/datasette/database.py b/datasette/database.py index 1e6f9032..fcf69c7f 100644 --- a/datasette/database.py +++ b/datasette/database.py @@ -532,10 +532,7 @@ class Database: ] if sqlite_version()[1] >= 37: - hidden_tables += [ - x[0] - for x in await self.execute( - """ + hidden_tables += [x[0] for x in await self.execute(""" with shadow_tables as ( select name from pragma_table_list @@ -554,14 +551,9 @@ class Database: select name from core_tables ) select name from combined order by 1 - """ - ) - ] + """)] else: - hidden_tables += [ - x[0] - for x in await self.execute( - """ + hidden_tables += [x[0] for x in await self.execute(""" WITH base AS ( SELECT name FROM sqlite_master @@ -607,22 +599,15 @@ class Database: SELECT name FROM fts3_shadow_tables ) SELECT name FROM final ORDER BY 1 - """ - ) - ] + """)] # Also hide any FTS tables that have a content= argument - hidden_tables += [ - x[0] - for x in await self.execute( - """ + hidden_tables += [x[0] for x in await self.execute(""" SELECT name FROM sqlite_master WHERE sql LIKE '%VIRTUAL TABLE%' AND sql LIKE '%USING FTS%' AND sql LIKE '%content=%' - """ - ) - ] + """)] has_spatialite = await self.execute_fn(detect_spatialite) if has_spatialite: @@ -641,16 +626,11 @@ class Database: "KNN", "KNN2", ] + [ - r[0] - for r in ( - await self.execute( - """ + r[0] for r in (await self.execute(""" select name from sqlite_master where name like "idx_%" and type = "table" - """ - ) - ).rows + """)).rows ] return hidden_tables diff --git a/datasette/default_permissions/defaults.py b/datasette/default_permissions/defaults.py index f5a6a270..4c74219d 100644 --- a/datasette/default_permissions/defaults.py +++ b/datasette/default_permissions/defaults.py @@ -14,7 +14,6 @@ if TYPE_CHECKING: from datasette import hookimpl from datasette.permissions import PermissionSQL - # Actions that are allowed by default (unless --default-deny is used) DEFAULT_ALLOW_ACTIONS = frozenset( { diff --git a/datasette/facets.py b/datasette/facets.py index dd149424..bc4b6904 100644 --- a/datasette/facets.py +++ b/datasette/facets.py @@ -233,9 +233,7 @@ class ColumnFacet(Facet): ) where {col} is not null group by {col} order by count desc, value limit {limit} - """.format( - col=escape_sqlite(column), sql=self.sql, limit=facet_size + 1 - ) + """.format(col=escape_sqlite(column), sql=self.sql, limit=facet_size + 1) try: facet_rows_results = await self.ds.execute( self.database, @@ -482,9 +480,7 @@ class DateFacet(Facet): select date({column}) from ( select * from ({sql}) limit 100 ) where {column} glob "????-??-*" - """.format( - column=escape_sqlite(column), sql=self.sql - ) + """.format(column=escape_sqlite(column), sql=self.sql) try: results = await self.ds.execute( self.database, @@ -530,9 +526,7 @@ class DateFacet(Facet): ) where date({col}) is not null group by date({col}) order by count desc, value limit {limit} - """.format( - col=escape_sqlite(column), sql=self.sql, limit=facet_size + 1 - ) + """.format(col=escape_sqlite(column), sql=self.sql, limit=facet_size + 1) try: facet_rows_results = await self.ds.execute( self.database, diff --git a/datasette/inspect.py b/datasette/inspect.py index ede142d0..5e681e03 100644 --- a/datasette/inspect.py +++ b/datasette/inspect.py @@ -10,7 +10,6 @@ from .utils import ( sqlite3, ) - HASH_BLOCK_SIZE = 1024 * 1024 @@ -70,16 +69,11 @@ def inspect_tables(conn, database_metadata): tables[table]["foreign_keys"] = info # Mark tables 'hidden' if they relate to FTS virtual tables - hidden_tables = [ - r["name"] - for r in conn.execute( - """ + hidden_tables = [r["name"] for r in conn.execute(""" select name from sqlite_master where rootpage = 0 and sql like '%VIRTUAL TABLE%USING FTS%' - """ - ) - ] + """)] if detect_spatialite(conn): # Also hide Spatialite internal tables @@ -94,14 +88,11 @@ def inspect_tables(conn, database_metadata): "views_geometry_columns", "virts_geometry_columns", ] + [ - r["name"] - for r in conn.execute( - """ + r["name"] for r in conn.execute(""" select name from sqlite_master where name like "idx_%" and type = "table" - """ - ) + """) ] for t in tables.keys(): diff --git a/datasette/permissions.py b/datasette/permissions.py index c48293ac..b5e72b8e 100644 --- a/datasette/permissions.py +++ b/datasette/permissions.py @@ -3,7 +3,6 @@ from dataclasses import dataclass from typing import Any, NamedTuple import contextvars - # Context variable to track when permission checks should be skipped _skip_permission_checks = contextvars.ContextVar( "skip_permission_checks", default=False diff --git a/datasette/utils/__init__.py b/datasette/utils/__init__.py index d0d216eb..c6973d06 100644 --- a/datasette/utils/__init__.py +++ b/datasette/utils/__init__.py @@ -677,9 +677,7 @@ def detect_fts_sql(table): and sql like '%VIRTUAL TABLE%USING FTS%' ) ) - """.format( - table=table.replace("'", "''") - ) + """.format(table=table.replace("'", "''")) def detect_json1(conn=None): diff --git a/datasette/utils/actions_sql.py b/datasette/utils/actions_sql.py index 9c2add0e..14383253 100644 --- a/datasette/utils/actions_sql.py +++ b/datasette/utils/actions_sql.py @@ -180,13 +180,11 @@ async def _build_single_action_sql( # Skip plugins that only provide restriction_sql (no permission rules) if permission_sql.sql is None: continue - rule_sqls.append( - f""" + rule_sqls.append(f""" SELECT parent, child, allow, reason, '{permission_sql.source}' AS source_plugin FROM ( {permission_sql.sql} ) - """.strip() - ) + """.strip()) # If no rules, return empty result (deny all) if not rule_sqls: @@ -405,14 +403,12 @@ async def _build_single_action_sql( # Add restriction filter if there are restrictions if restriction_sqls: - query_parts.append( - """ + query_parts.append(""" AND EXISTS ( SELECT 1 FROM restriction_list r WHERE (r.parent = decisions.parent OR r.parent IS NULL) AND (r.child = decisions.child OR r.child IS NULL) - )""" - ) + )""") # Add parent filter if specified if parent is not None: @@ -479,13 +475,11 @@ async def build_permission_rules_sql( if permission_sql.sql is None: continue - union_parts.append( - f""" + union_parts.append(f""" SELECT parent, child, allow, reason, '{permission_sql.source}' AS source_plugin FROM ( {permission_sql.sql} ) - """.strip() - ) + """.strip()) rules_union = " UNION ALL ".join(union_parts) return rules_union, all_params, restriction_sqls diff --git a/datasette/utils/internal_db.py b/datasette/utils/internal_db.py index a3afbab2..e4ebddde 100644 --- a/datasette/utils/internal_db.py +++ b/datasette/utils/internal_db.py @@ -3,8 +3,7 @@ from datasette.utils import table_column_details async def init_internal_db(db): - create_tables_sql = textwrap.dedent( - """ + create_tables_sql = textwrap.dedent(""" CREATE TABLE IF NOT EXISTS catalog_databases ( database_name TEXT PRIMARY KEY, path TEXT, @@ -68,16 +67,13 @@ async def init_internal_db(db): FOREIGN KEY (database_name) REFERENCES catalog_databases(database_name), FOREIGN KEY (database_name, table_name) REFERENCES catalog_tables(database_name, table_name) ); - """ - ).strip() + """).strip() await db.execute_write_script(create_tables_sql) await initialize_metadata_tables(db) async def initialize_metadata_tables(db): - await db.execute_write_script( - textwrap.dedent( - """ + await db.execute_write_script(textwrap.dedent(""" CREATE TABLE IF NOT EXISTS metadata_instance ( key text, value text, @@ -107,9 +103,7 @@ async def initialize_metadata_tables(db): value text, unique(database_name, resource_name, column_name, key) ); - """ - ) - ) + """)) async def populate_schema_tables(internal_db, db): diff --git a/datasette/utils/permissions.py b/datasette/utils/permissions.py index 6c30a12a..fd1e41a1 100644 --- a/datasette/utils/permissions.py +++ b/datasette/utils/permissions.py @@ -9,7 +9,6 @@ from datasette.permissions import PermissionSQL from datasette.plugins import pm from datasette.utils import await_me_maybe - # Sentinel object to indicate permission checks should be skipped SKIP_PERMISSION_CHECKS = object() @@ -116,13 +115,11 @@ def build_rules_union( if p.sql is None: continue - parts.append( - f""" + parts.append(f""" SELECT parent, child, allow, reason, '{p.source}' AS source_plugin FROM ( {p.sql} ) - """.strip() - ) + """.strip()) if not parts: # Empty UNION that returns no rows diff --git a/datasette/views/base.py b/datasette/views/base.py index bdc9f742..e4c1c738 100644 --- a/datasette/views/base.py +++ b/datasette/views/base.py @@ -241,8 +241,7 @@ class DataView(BaseView): data, extra_template_data, templates = response_or_template_contexts except QueryInterrupted as ex: raise DatasetteError( - textwrap.dedent( - """ + textwrap.dedent("""

SQL query took too long. The time limit is controlled by the sql_time_limit_ms configuration option.

@@ -251,10 +250,7 @@ class DataView(BaseView): let ta = document.querySelector("textarea"); ta.style.height = ta.scrollHeight + "px"; - """.format( - escape(ex.sql) - ) - ).strip(), + """.format(escape(ex.sql))).strip(), title="SQL Interrupted", status=400, message_is_html=True, diff --git a/datasette/views/database.py b/datasette/views/database.py index a42ac758..93ad8eda 100644 --- a/datasette/views/database.py +++ b/datasette/views/database.py @@ -615,8 +615,7 @@ class QueryView(View): rows = results.rows except QueryInterrupted as ex: raise DatasetteError( - textwrap.dedent( - """ + textwrap.dedent("""

SQL query took too long. The time limit is controlled by the sql_time_limit_ms configuration option.

@@ -625,10 +624,7 @@ class QueryView(View): let ta = document.querySelector("textarea"); ta.style.height = ta.scrollHeight + "px"; - """.format( - markupsafe.escape(ex.sql) - ) - ).strip(), + """.format(markupsafe.escape(ex.sql))).strip(), title="SQL Interrupted", status=400, message_is_html=True, diff --git a/datasette/views/index.py b/datasette/views/index.py index a59c687c..6a9462ac 100644 --- a/datasette/views/index.py +++ b/datasette/views/index.py @@ -12,7 +12,6 @@ from datasette.version import __version__ from .base import BaseView - # Truncate table list on homepage at: TRUNCATE_AT = 5 diff --git a/datasette/views/special.py b/datasette/views/special.py index 57a3024d..640c82eb 100644 --- a/datasette/views/special.py +++ b/datasette/views/special.py @@ -13,7 +13,6 @@ from .base import BaseView, View import secrets import urllib - logger = logging.getLogger(__name__) diff --git a/tests/conftest.py b/tests/conftest.py index ad7243c1..efa02c0a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -11,7 +11,6 @@ import time from dataclasses import dataclass from datasette import Event, hookimpl - try: import pysqlite3 as sqlite3 except ImportError: diff --git a/tests/fixtures.py b/tests/fixtures.py index 0c110a94..9f99519a 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -13,7 +13,6 @@ import string import tempfile import textwrap - # This temp file is used by one of the plugin config tests TEMP_PLUGIN_SECRET_FILE = os.path.join(tempfile.gettempdir(), "plugin-secret") @@ -331,16 +330,14 @@ CONFIG = { "sql": "select :_header_user_agent as user_agent, :_now_datetime_utc as datetime", }, "neighborhood_search": { - "sql": textwrap.dedent( - """ + "sql": textwrap.dedent(""" select _neighborhood, facet_cities.name, state from facetable join facet_cities on facetable._city_id = facet_cities.id where _neighborhood like '%' || :text || '%' order by _neighborhood; - """ - ), + """), "title": "Search neighborhoods", "description_html": "Demonstrating simple like search", "fragment": "fragment-goes-here", @@ -710,19 +707,10 @@ CREATE VIEW searchable_view_configured_by_metadata AS for a, b, c, content in generate_compound_rows(1001) ] ) - + "\n".join( - [ - """INSERT INTO sortable VALUES ( + + "\n".join(["""INSERT INTO sortable VALUES ( "{pk1}", "{pk2}", "{content}", {sortable}, {sortable_with_nulls}, {sortable_with_nulls_2}, "{text}"); - """.format( - **row - ).replace( - "None", "null" - ) - for row in generate_sortable_rows(201) - ] - ) + """.format(**row).replace("None", "null") for row in generate_sortable_rows(201)]) ) TABLE_PARAMETERIZED_SQL = [ ("insert into binary_data (data) values (?);", [b"\x15\x1c\x02\xc7\xad\x05\xfe"]), diff --git a/tests/plugins/my_plugin.py b/tests/plugins/my_plugin.py index c8794fad..20e7d111 100644 --- a/tests/plugins/my_plugin.py +++ b/tests/plugins/my_plugin.py @@ -261,8 +261,7 @@ def register_routes(): response = Response.redirect("/") datasette.set_actor_cookie(response, {"id": "root"}) return response - return Response.html( - """ + return Response.html("""

@@ -271,10 +270,7 @@ def register_routes(): style="font-size: 2em; padding: 0.1em 0.5em;">

- """.format( - request.path, request.scope["csrftoken"]() - ) - ) + """.format(request.path, request.scope["csrftoken"]())) def asgi_scope(scope): return Response.json(scope, default=repr) diff --git a/tests/test_cli.py b/tests/test_cli.py index 6cdfd924..7673c3f3 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -115,13 +115,9 @@ def test_plugins_cli(app_client): def test_metadata_yaml(): - yaml_file = io.StringIO( - textwrap.dedent( - """ + yaml_file = io.StringIO(textwrap.dedent(""" title: Hello from YAML - """ - ) - ) + """)) # Annoyingly we have to provide all default arguments here: ds = serve.callback( [], diff --git a/tests/test_cli_serve_get.py b/tests/test_cli_serve_get.py index 5ad01bfa..dc852201 100644 --- a/tests/test_cli_serve_get.py +++ b/tests/test_cli_serve_get.py @@ -16,9 +16,7 @@ def test_serve_with_get(tmp_path_factory): def startup(datasette): with open("{}", "w") as fp: fp.write("hello") - """.format( - str(plugins_dir / "hello.txt") - ), + """.format(str(plugins_dir / "hello.txt")), ), "utf-8", ) diff --git a/tests/test_config_dir.py b/tests/test_config_dir.py index f9a90fbe..ae7fe500 100644 --- a/tests/test_config_dir.py +++ b/tests/test_config_dir.py @@ -51,8 +51,7 @@ def config_dir(tmp_path_factory): for dbname in ("demo.db", "immutable.db", "j.sqlite3", "k.sqlite"): db = sqlite3.connect(str(config_dir / dbname)) - db.executescript( - """ + db.executescript(""" CREATE TABLE cities ( id integer primary key, name text @@ -60,8 +59,7 @@ def config_dir(tmp_path_factory): INSERT INTO cities (id, name) VALUES (1, 'San Francisco') ; - """ - ) + """) # Mark "immutable.db" as immutable (config_dir / "inspect-data.json").write_text( diff --git a/tests/test_csv.py b/tests/test_csv.py index 5589bd97..a2f03776 100644 --- a/tests/test_csv.py +++ b/tests/test_csv.py @@ -9,16 +9,12 @@ EXPECTED_TABLE_CSV = """id,content 3, 4,RENDER_CELL_DEMO 5,RENDER_CELL_ASYNC -""".replace( - "\n", "\r\n" -) +""".replace("\n", "\r\n") EXPECTED_CUSTOM_CSV = """content hello world -""".replace( - "\n", "\r\n" -) +""".replace("\n", "\r\n") EXPECTED_TABLE_WITH_LABELS_CSV = """ pk,created,planet_int,on_earth,state,_city_id,_city_id_label,_neighborhood,tags,complex_array,distinct_some_null,n @@ -37,17 +33,13 @@ pk,created,planet_int,on_earth,state,_city_id,_city_id_label,_neighborhood,tags, 13,2019-01-17 08:00:00,1,1,MI,3,Detroit,Corktown,[],[],, 14,2019-01-17 08:00:00,1,1,MI,3,Detroit,Mexicantown,[],[],, 15,2019-01-17 08:00:00,2,0,MC,4,Memnonia,Arcadia Planitia,[],[],, -""".lstrip().replace( - "\n", "\r\n" -) +""".lstrip().replace("\n", "\r\n") EXPECTED_TABLE_WITH_NULLABLE_LABELS_CSV = """ pk,foreign_key_with_label,foreign_key_with_label_label,foreign_key_with_blank_label,foreign_key_with_blank_label_label,foreign_key_with_no_label,foreign_key_with_no_label_label,foreign_key_compound_pk1,foreign_key_compound_pk2 1,1,hello,3,,1,1,a,b 2,,,,,,,, -""".lstrip().replace( - "\n", "\r\n" -) +""".lstrip().replace("\n", "\r\n") @pytest.mark.asyncio @@ -108,8 +100,7 @@ async def test_table_csv_with_invalid_labels(): ) await ds.invoke_startup() db = ds.add_memory_database("db_2214") - await db.execute_write_script( - """ + await db.execute_write_script(""" create table t1 (id integer primary key, name text); insert into t1 (id, name) values (1, 'one'); insert into t1 (id, name) values (2, 'two'); @@ -124,8 +115,7 @@ async def test_table_csv_with_invalid_labels(): insert into maintable (id, fk_integer, fk_text) values (1, 1, 'a'); insert into maintable (id, fk_integer, fk_text) values (2, 3, 'b'); -- invalid fk_integer insert into maintable (id, fk_integer, fk_text) values (3, 2, 'c'); -- invalid fk_text - """ - ) + """) response = await ds.client.get("/db_2214/maintable.csv?_labels=1") assert response.status_code == 200 assert response.text == ( diff --git a/tests/test_html.py b/tests/test_html.py index 8fad5764..757f3e6e 100644 --- a/tests/test_html.py +++ b/tests/test_html.py @@ -620,14 +620,11 @@ async def test_urlify_custom_queries(ds_client): response = await ds_client.get(path) assert response.status_code == 200 soup = Soup(response.content, "html.parser") - assert ( - """ + assert """ https://twitter.com/simonw -""" - == soup.find("td", {"class": "col-user_url"}).prettify().strip() - ) +""" == soup.find("td", {"class": "col-user_url"}).prettify().strip() @pytest.mark.asyncio diff --git a/tests/test_internals_database.py b/tests/test_internals_database.py index 02c67bfc..5e3459cd 100644 --- a/tests/test_internals_database.py +++ b/tests/test_internals_database.py @@ -747,19 +747,15 @@ async def test_replace_database(tmpdir): path1 = str(tmpdir / "data1.db") (tmpdir / "two").mkdir() path2 = str(tmpdir / "two" / "data1.db") - sqlite3.connect(path1).executescript( - """ + sqlite3.connect(path1).executescript(""" create table t (id integer primary key); insert into t (id) values (1); insert into t (id) values (2); - """ - ) - sqlite3.connect(path2).executescript( - """ + """) + sqlite3.connect(path2).executescript(""" create table t (id integer primary key); insert into t (id) values (1); - """ - ) + """) datasette = Datasette([path1]) db = datasette.get_database("data1") count = (await db.execute("select count(*) from t")).first()[0] diff --git a/tests/test_plugins.py b/tests/test_plugins.py index 190ef659..754b199c 100644 --- a/tests/test_plugins.py +++ b/tests/test_plugins.py @@ -233,9 +233,7 @@ async def test_hook_render_cell_pks_compound_pk(ds_client): @pytest.mark.asyncio async def test_hook_render_cell_pks_rowid_table(ds_client): """pks should be ["rowid"] for a table with no explicit primary key""" - response = await ds_client.get( - "/fixtures/no_primary_key?content=RENDER_CELL_DEMO" - ) + response = await ds_client.get("/fixtures/no_primary_key?content=RENDER_CELL_DEMO") soup = Soup(response.text, "html.parser") td = soup.find("td", {"class": "col-content"}) data = json.loads(td.string) @@ -457,14 +455,12 @@ def view_names_client(tmp_path_factory): ): (templates / template).write_text("view_name:{{ view_name }}", "utf-8") (plugins / "extra_vars.py").write_text( - textwrap.dedent( - """ + textwrap.dedent(""" from datasette import hookimpl @hookimpl def extra_template_vars(view_name): return {"view_name": view_name} - """ - ), + """), "utf-8", ) db_path = str(tmpdir / "fixtures.db") diff --git a/tests/test_publish_cloudrun.py b/tests/test_publish_cloudrun.py index f53e5059..6617bc77 100644 --- a/tests/test_publish_cloudrun.py +++ b/tests/test_publish_cloudrun.py @@ -231,16 +231,12 @@ def test_publish_cloudrun_plugin_secrets( with open("test.db", "w") as fp: fp.write("data") with open("metadata.yml", "w") as fp: - fp.write( - textwrap.dedent( - """ + fp.write(textwrap.dedent(""" title: Hello from metadata YAML plugins: datasette-auth-github: foo: bar - """ - ).strip() - ) + """).strip()) result = runner.invoke( cli.cli, [ @@ -333,8 +329,7 @@ def test_publish_cloudrun_apt_get_install( .split("\n====================\n")[0] .strip() ) - expected = textwrap.dedent( - r""" + expected = textwrap.dedent(r""" FROM python:3.11.0-slim-bullseye COPY . /app WORKDIR /app @@ -350,8 +345,7 @@ def test_publish_cloudrun_apt_get_install( ENV PORT 8001 EXPOSE 8001 CMD datasette serve --host 0.0.0.0 -i test.db --cors --inspect-file inspect-data.json --setting force_https_urls on --port $PORT - """ - ).strip() + """).strip() assert expected == dockerfile diff --git a/tests/test_routes.py b/tests/test_routes.py index 9866cc76..24c702fc 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -63,12 +63,10 @@ async def ds_with_route(): ds.remove_database("_memory") db = Database(ds, is_memory=True, memory_name="route-name-db") ds.add_database(db, name="original-name", route="custom-route-name") - await db.execute_write_script( - """ + await db.execute_write_script(""" create table if not exists t (id integer primary key); insert or replace into t (id) values (1); - """ - ) + """) return ds diff --git a/tests/test_table_api.py b/tests/test_table_api.py index 943a1549..51e40ad1 100644 --- a/tests/test_table_api.py +++ b/tests/test_table_api.py @@ -1243,9 +1243,7 @@ async def test_paginate_using_link_header(ds_client, qs): reason="generated columns were added in SQLite 3.31.0", ) def test_generated_columns_are_visible_in_datasette(): - with make_app_client( - extra_databases={ - "generated.db": """ + with make_app_client(extra_databases={"generated.db": """ CREATE TABLE generated_columns ( body TEXT, id INT GENERATED ALWAYS AS (json_extract(body, '$.number')) STORED, @@ -1253,9 +1251,7 @@ def test_generated_columns_are_visible_in_datasette(): ); INSERT INTO generated_columns (body) VALUES ( '{"number": 1, "string": "This is a string"}' - );""" - } - ) as client: + );"""}) as client: response = client.get("/generated/generated_columns.json?_shape=array") assert response.json == [ { diff --git a/tests/test_utils.py b/tests/test_utils.py index b8d047e9..85ab9e6b 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -201,9 +201,7 @@ def test_detect_fts(open_quote, close_quote): CREATE VIEW Test_View AS SELECT * FROM Dumb_Table; CREATE VIRTUAL TABLE {open}Street_Tree_List_fts{close} USING FTS4 ("qAddress", "qCaretaker", "qSpecies", content={open}Street_Tree_List{close}); CREATE VIRTUAL TABLE r USING rtree(a, b, c); - """.format( - open=open_quote, close=close_quote - ) + """.format(open=open_quote, close=close_quote) conn = utils.sqlite3.connect(":memory:") conn.executescript(sql) assert None is utils.detect_fts(conn, "Dumb_Table") @@ -220,9 +218,7 @@ def test_detect_fts_different_table_names(table): "qSpecies" TEXT ); CREATE VIRTUAL TABLE [{table}_fts] USING FTS4 ("qSpecies", content="{table}"); - """.format( - table=table - ) + """.format(table=table) conn = utils.sqlite3.connect(":memory:") conn.executescript(sql) assert "{table}_fts".format(table=table) == utils.detect_fts(conn, table) @@ -347,27 +343,21 @@ def test_compound_keys_after_sql(): ((a > :p0) or (a = :p0 and b > :p1)) - """.strip() == utils.compound_keys_after_sql( - ["a", "b"] - ) + """.strip() == utils.compound_keys_after_sql(["a", "b"]) assert """ ((a > :p0) or (a = :p0 and b > :p1) or (a = :p0 and b = :p1 and c > :p2)) - """.strip() == utils.compound_keys_after_sql( - ["a", "b", "c"] - ) + """.strip() == utils.compound_keys_after_sql(["a", "b", "c"]) def test_table_columns(): conn = sqlite3.connect(":memory:") - conn.executescript( - """ + conn.executescript(""" create table places (id integer primary key, name text, bob integer) - """ - ) + """) assert ["id", "name", "bob"] == utils.table_columns(conn, "places") diff --git a/tests/test_utils_permissions.py b/tests/test_utils_permissions.py index b412de0f..bc3599c2 100644 --- a/tests/test_utils_permissions.py +++ b/tests/test_utils_permissions.py @@ -497,16 +497,14 @@ async def test_actor_actor_id_action_parameters_available(db): def plugin_using_all_parameters() -> Callable[[str], PermissionSQL]: def provider(action: str) -> PermissionSQL: - return PermissionSQL( - """ + return PermissionSQL(""" SELECT NULL AS parent, NULL AS child, 1 AS allow, 'Actor ID: ' || COALESCE(:actor_id, 'null') || ', Actor JSON: ' || COALESCE(:actor, 'null') || ', Action: ' || :action AS reason WHERE :actor_id = 'test_user' AND :action = 'view-table' AND json_extract(:actor, '$.role') = 'admin' - """ - ) + """) return provider diff --git a/tests/test_write_wrapper.py b/tests/test_write_wrapper.py index 38e5c94e..cb320c06 100644 --- a/tests/test_write_wrapper.py +++ b/tests/test_write_wrapper.py @@ -471,7 +471,9 @@ async def test_write_wrapper_set_authorizer(datasette, actor, table, should_deny ), request=request, ) - result = await db.execute(f"select value from {table} order by rowid desc limit 1") + result = await db.execute( + f"select value from {table} order by rowid desc limit 1" + ) assert result.rows[0][0] == "test" finally: pm.unregister(name="test_set_authorizer") From 1c6c6d2e6897c1173ed6e209c8b7133688e75c58 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Tue, 17 Feb 2026 13:30:46 -0800 Subject: [PATCH 010/481] Fix test_write_wrapper_set_authorizer: use permissive callback instead of None conn.set_authorizer(None) does not clear the authorizer - SQLite treats None as an invalid callback. The denied state persists on the shared write connection, causing subsequent non-deny test cases to fail. Fixes test added in 8a315f3d. --- tests/test_write_wrapper.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_write_wrapper.py b/tests/test_write_wrapper.py index cb320c06..55e0461e 100644 --- a/tests/test_write_wrapper.py +++ b/tests/test_write_wrapper.py @@ -445,7 +445,7 @@ async def test_write_wrapper_set_authorizer(datasette, actor, table, should_deny try: yield finally: - conn.set_authorizer(None) + conn.set_authorizer(lambda *args: sqlite3.SQLITE_OK) return wrapper From 7a66456615cad38d9e70267a14ca30dcc4bca701 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Fri, 20 Feb 2026 11:19:19 -0800 Subject: [PATCH 011/481] black --version --- .github/workflows/test.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b1ba3232..a0f5477b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -34,7 +34,9 @@ jobs: # And the test that exceeds a localhost HTTPS server tests/test_datasette_https_server.sh - name: Black - run: black --check . + run: | + black --version + black --check . - name: Ruff run: ruff check datasette tests - name: Check if cog needs to be run From 2f0e64df681c7bf65e8ce3065380be36a4ccd266 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Fri, 20 Feb 2026 11:24:52 -0800 Subject: [PATCH 012/481] black==26.1.0 I'm getting CI failures for Black, maybe this will help --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index d9ef2a73..2ab2ce10 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -61,7 +61,7 @@ dev = [ "pytest-xdist>=2.2.1", "pytest-asyncio>=1.2.0", "beautifulsoup4>=4.8.1", - "black==25.11.0", + "black==26.1.0", "blacken-docs==1.20.0", "pytest-timeout>=1.4.2", "trustme>=0.7", From 6a2c27b15b300ba1b924ce00a61532943482392e Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Fri, 20 Feb 2026 11:28:39 -0800 Subject: [PATCH 013/481] blacken-docs --- docs/plugin_hooks.rst | 13 ++++--------- docs/spatialite.rst | 6 ++---- docs/testing_plugins.rst | 8 ++------ 3 files changed, 8 insertions(+), 19 deletions(-) diff --git a/docs/plugin_hooks.rst b/docs/plugin_hooks.rst index 068469a8..fa335368 100644 --- a/docs/plugin_hooks.rst +++ b/docs/plugin_hooks.rst @@ -1074,11 +1074,9 @@ You can also return an async function, which will be awaited on startup. Use thi async def inner(): db = datasette.get_database() if "my_table" not in await db.table_names(): - await db.execute_write( - """ + await db.execute_write(""" create table my_table (mycol text) - """ - ) + """) return inner @@ -1561,7 +1559,6 @@ The resolver will automatically apply the most specific rule. from datasette import hookimpl from datasette.permissions import PermissionSQL - TRUSTED = {"alice", "bob"} @@ -2261,8 +2258,7 @@ This example logs events to a ``datasette_events`` table in a database called `` def startup(datasette): async def inner(): db = datasette.get_database("events") - await db.execute_write( - """ + await db.execute_write(""" create table if not exists datasette_events ( id integer primary key, event_type text, @@ -2270,8 +2266,7 @@ This example logs events to a ``datasette_events`` table in a database called `` actor text, properties text ) - """ - ) + """) return inner diff --git a/docs/spatialite.rst b/docs/spatialite.rst index fbe0d75f..c93c1e00 100644 --- a/docs/spatialite.rst +++ b/docs/spatialite.rst @@ -90,12 +90,10 @@ Here's a recipe for taking a table with existing latitude and longitude columns, "SELECT AddGeometryColumn('museums', 'point_geom', 4326, 'POINT', 2);" ) # Now update that geometry column with the lat/lon points - conn.execute( - """ + conn.execute(""" UPDATE museums SET point_geom = GeomFromText('POINT('||"longitude"||' '||"latitude"||')',4326); - """ - ) + """) # Now add a spatial index to that column conn.execute( 'select CreateSpatialIndex("museums", "point_geom");' diff --git a/docs/testing_plugins.rst b/docs/testing_plugins.rst index fc1aa6f6..b0713e7c 100644 --- a/docs/testing_plugins.rst +++ b/docs/testing_plugins.rst @@ -233,15 +233,11 @@ As an example, here's a very simple plugin which executes an HTTP response and r async def fetch_url(datasette, request): if request.method == "GET": - return Response.html( - """ + return Response.html("""
-
""".format( - request.scope["csrftoken"]() - ) - ) + """.format(request.scope["csrftoken"]())) vars = await request.post_vars() url = vars["url"] return Response.text(httpx.get(url).text) From c96dc5ce2656607b9e81743acf600f8fd5f6a795 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Wed, 25 Feb 2026 16:32:45 -0800 Subject: [PATCH 014/481] register_token_handler() plugin hook for custom API token backends (#2650) Closes #2649 * Add register_token_handler plugin hook for pluggable token backends Adds a new register_token_handler hook that allows plugins to provide custom token creation and verification backends. This enables plugins like datasette-oauth to issue tokens without depending on specific backend plugins like datasette-auth-tokens. Key changes: - New datasette/tokens.py with TokenHandler base class and SignedTokenHandler (the default signed-token implementation moved here) - New register_token_handler hookspec in hookspecs.py - Datasette.create_token() is now async and delegates to token handlers - New Datasette.verify_token() method tries all handlers in sequence - handler= parameter on create_token() to select a specific backend - TokenHandler exported from datasette package for plugin use - Fixed actor_from_request loop to await all coroutines (avoids warnings) * Add documentation and hook test for register_token_handler Fixes CI failures: the new hook needs a section in docs/plugin_hooks.rst (checked by test_plugin_hooks_are_documented) and a test_hook_* function in test_plugins.py (checked by test_plugin_hooks_have_tests). * Register tokens module as separate default plugin Instead of re-exporting hookimpls from default_permissions/__init__.py, register datasette.default_permissions.tokens as its own DEFAULT_PLUGINS entry. Cleaner and avoids confusing import-for-side-effect patterns. * Replace restrict_x params with TokenRestrictions dataclass Consolidates the three separate restrict_all, restrict_database, and restrict_resource parameters into a single TokenRestrictions dataclass. Cleaner API surface for both Datasette.create_token() and TokenHandler.create_token(). Also clarifies docs re: default handler selection via pluggy ordering. * Add builder methods to TokenRestrictions Adds allow_all(), allow_database(), and allow_resource() methods that return self for chaining. Callers no longer need to manipulate nested dicts directly: restrictions = (TokenRestrictions() .allow_all("view-instance") .allow_database("mydb", "create-table") .allow_resource("mydb", "mytable", "insert-row")) * docs: add 1.0a25 upgrade guide section for create_token() signature change Ref: https://github.com/simonw/datasette/issues/2649#issuecomment-3962639393 * docs: note that create_token() is now async in upgrade guide * docs: update internals, plugin_hooks, authentication for new token API - internals.rst: new async create_token() signature with restrictions and handler params, add TokenRestrictions reference docs - plugin_hooks.rst: show full create_token signature in TokenHandler example, note list returns and error cases - authentication.rst: cross-reference TokenRestrictions from the restrictions section * style: apply black formatting to token handler files * docs: fix RST heading underline length in internals.rst * tests: add restrictions round-trip and expiration tests for token handler Covers allow_database/allow_resource builders, _r payload encoding, and token_expires in verified actors. Coverage 76% -> 90%. * tests: add test for signed tokens disabled * fix: add TokenRestrictions TYPE_CHECKING import to fix ruff F821 * docs: regenerate plugins.rst with cog * docs: reformat code blocks in plugin_hooks.rst with blacken-docs * docs: add await .verify_token() to internals.rst * tests: rewrite register_token_handler test to use real plugin handler Adds a HardcodedTokenHandler to the test plugins dir that creates tokens like dstok_hardcoded_token_1. The test now exercises creating tokens via the default handler (which is the plugin's hardcoded one), by explicitly naming the hardcoded handler, and by explicitly naming the signed handler -- then verifies each token round-trips correctly. * tests: clarify test_token_handler_via_http tests the default signed handler * fix: use handler="signed" explicitly where signed tokens are expected The HardcodedTokenHandler in my_plugin.py gets globally registered, so create_token() without a handler name picks it up as the default. Fix the create-token view, CLI, and tests to explicitly request the signed handler where they depend on signed token behavior. * fix: use handler="signed" in test_create_table_permissions https://claude.ai/code/session_013cQFiDQjYRrRBH2biFfKuS --- datasette/__init__.py | 1 + datasette/app.py | 102 +++++--- datasette/cli.py | 30 ++- datasette/default_permissions/__init__.py | 1 - datasette/default_permissions/tokens.py | 85 ++---- datasette/hookspecs.py | 5 + datasette/plugins.py | 1 + datasette/tokens.py | 180 +++++++++++++ datasette/views/special.py | 34 +-- docs/authentication.rst | 1 + docs/internals.rst | 81 ++++-- docs/plugin_hooks.rst | 59 +++++ docs/plugins.rst | 11 +- docs/upgrade_guide.md | 40 +++ tests/fixtures.py | 1 + tests/plugins/my_plugin.py | 27 ++ tests/test_api_write.py | 9 +- tests/test_permissions.py | 2 +- tests/test_plugins.py | 32 +++ tests/test_token_handler.py | 301 ++++++++++++++++++++++ 20 files changed, 839 insertions(+), 164 deletions(-) create mode 100644 datasette/tokens.py create mode 100644 tests/test_token_handler.py diff --git a/datasette/__init__.py b/datasette/__init__.py index 47d2b4f6..eb18e59e 100644 --- a/datasette/__init__.py +++ b/datasette/__init__.py @@ -1,6 +1,7 @@ from datasette.permissions import Permission # noqa from datasette.version import __version_info__, __version__ # noqa from datasette.events import Event # noqa +from datasette.tokens import TokenHandler, TokenRestrictions # noqa from datasette.utils.asgi import Forbidden, NotFound, Request, Response # noqa from datasette.utils import actor_matches_allow # noqa from datasette.views import Context # noqa diff --git a/datasette/app.py b/datasette/app.py index 6efaa430..2df6e4e8 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -7,6 +7,7 @@ from typing import TYPE_CHECKING, Any, Dict, Iterable, List if TYPE_CHECKING: from datasette.permissions import Resource + from datasette.tokens import TokenRestrictions import asgi_csrf import collections import dataclasses @@ -713,44 +714,70 @@ class Datasette: """ return _in_datasette_client.get() - def create_token( + def _token_handlers(self): + """Collect all registered token handlers from plugins.""" + from datasette.tokens import TokenHandler + + handlers = [] + for result in pm.hook.register_token_handler(datasette=self): + if isinstance(result, TokenHandler): + handlers.append(result) + elif isinstance(result, list): + handlers.extend(h for h in result if isinstance(h, TokenHandler)) + return handlers + + async def create_token( self, actor_id: str, *, expires_after: int | None = None, - restrict_all: Iterable[str] | None = None, - restrict_database: Dict[str, Iterable[str]] | None = None, - restrict_resource: Dict[str, Dict[str, Iterable[str]]] | None = None, - ): - token = {"a": actor_id, "t": int(time.time())} - if expires_after: - token["d"] = expires_after + restrictions: "TokenRestrictions | None" = None, + handler: str | None = None, + ) -> str: + """ + Create an API token for the given actor. - def abbreviate_action(action): - # rename to abbr if possible - action_obj = self.actions.get(action) - if not action_obj: - return action - return action_obj.abbr or action + Uses the first registered token handler by default, or a specific + handler if ``handler`` is provided (matched by handler name). - if expires_after: - token["d"] = expires_after - if restrict_all or restrict_database or restrict_resource: - token["_r"] = {} - if restrict_all: - token["_r"]["a"] = [abbreviate_action(a) for a in restrict_all] - if restrict_database: - token["_r"]["d"] = {} - for database, actions in restrict_database.items(): - token["_r"]["d"][database] = [abbreviate_action(a) for a in actions] - if restrict_resource: - token["_r"]["r"] = {} - for database, resources in restrict_resource.items(): - for resource, actions in resources.items(): - token["_r"]["r"].setdefault(database, {})[resource] = [ - abbreviate_action(a) for a in actions - ] - return "dstok_{}".format(self.sign(token, namespace="token")) + Pass a :class:`TokenRestrictions` to limit which actions the token + can perform. + """ + handlers = self._token_handlers() + if not handlers: + raise RuntimeError("No token handlers are registered") + + if handler is not None: + matched = [h for h in handlers if h.name == handler] + if not matched: + available = [h.name for h in handlers] + raise ValueError( + f"Token handler {handler!r} not found. " + f"Available handlers: {available}" + ) + chosen = matched[0] + else: + chosen = handlers[0] + + return await chosen.create_token( + self, + actor_id, + expires_after=expires_after, + restrictions=restrictions, + ) + + async def verify_token(self, token: str) -> dict | None: + """ + Verify an API token by trying all registered token handlers. + + Returns an actor dict from the first handler that recognizes the + token, or None if no handler accepts it. + """ + for token_handler in self._token_handlers(): + result = await token_handler.verify_token(self, token) + if result is not None: + return result + return None def get_database(self, name=None, route=None): if route is not None: @@ -2159,10 +2186,13 @@ class DatasetteRouter: # Handle authentication default_actor = scope.get("actor") or None actor = None - for actor in pm.hook.actor_from_request(datasette=self.ds, request=request): - actor = await await_me_maybe(actor) - if actor: - break + results = pm.hook.actor_from_request(datasette=self.ds, request=request) + for result in results: + result = await await_me_maybe(result) + if result and actor is None: + actor = result + # Don't break — we must await all coroutines to avoid + # "coroutine was never awaited" warnings scope_modifications["actor"] = actor or default_actor scope = dict(scope, **scope_modifications) diff --git a/datasette/cli.py b/datasette/cli.py index 121911ab..b473fbb7 100644 --- a/datasette/cli.py +++ b/datasette/cli.py @@ -832,21 +832,23 @@ def create_token( err=True, ) - restrict_database = {} - for database, action in databases: - restrict_database.setdefault(database, []).append(action) - restrict_resource = {} - for database, resource, action in resources: - restrict_resource.setdefault(database, {}).setdefault(resource, []).append( - action - ) + from datasette.tokens import TokenRestrictions - token = ds.create_token( - id, - expires_after=expires_after, - restrict_all=alls, - restrict_database=restrict_database, - restrict_resource=restrict_resource, + restrictions = TokenRestrictions() + for action in alls: + restrictions.allow_all(action) + for database, action in databases: + restrictions.allow_database(database, action) + for database, resource, action in resources: + restrictions.allow_resource(database, resource, action) + + token = run_sync( + lambda: ds.create_token( + id, + expires_after=expires_after, + restrictions=restrictions, + handler="signed", + ) ) click.echo(token) if debug: diff --git a/datasette/default_permissions/__init__.py b/datasette/default_permissions/__init__.py index 40373fa7..4ebe6147 100644 --- a/datasette/default_permissions/__init__.py +++ b/datasette/default_permissions/__init__.py @@ -37,7 +37,6 @@ from .defaults import ( default_action_permissions_sql as default_action_permissions_sql, DEFAULT_ALLOW_ACTIONS as DEFAULT_ALLOW_ACTIONS, ) -from .tokens import actor_from_signed_api_token as actor_from_signed_api_token @hookimpl diff --git a/datasette/default_permissions/tokens.py b/datasette/default_permissions/tokens.py index 474b0c23..7a359dc6 100644 --- a/datasette/default_permissions/tokens.py +++ b/datasette/default_permissions/tokens.py @@ -1,44 +1,35 @@ """ Token authentication for Datasette. -Handles signed API tokens (dstok_ prefix). +Registers the default SignedTokenHandler and delegates token verification +to datasette.verify_token() so all registered handlers are tried. """ from __future__ import annotations -import time from typing import TYPE_CHECKING, Optional if TYPE_CHECKING: from datasette.app import Datasette -import itsdangerous - from datasette import hookimpl +from datasette.tokens import SignedTokenHandler + + +@hookimpl +def register_token_handler(datasette: "Datasette"): + """Register the default signed token handler.""" + return SignedTokenHandler() @hookimpl(specname="actor_from_request") -def actor_from_signed_api_token(datasette: "Datasette", request) -> Optional[dict]: +async def actor_from_signed_api_token( + datasette: "Datasette", request +) -> Optional[dict]: """ - Authenticate requests using signed API tokens (dstok_ prefix). - - Token structure (signed JSON): - { - "a": "actor_id", # Actor ID - "t": 1234567890, # Timestamp (Unix epoch) - "d": 3600, # Optional: Duration in seconds - "_r": {...} # Optional: Restrictions - } + Authenticate requests using API tokens by delegating to all registered + token handlers via datasette.verify_token(). """ - prefix = "dstok_" - - # Check if tokens are enabled - if not datasette.setting("allow_signed_tokens"): - return None - - max_signed_tokens_ttl = datasette.setting("max_signed_tokens_ttl") - - # Get authorization header authorization = request.headers.get("authorization") if not authorization: return None @@ -46,50 +37,4 @@ def actor_from_signed_api_token(datasette: "Datasette", request) -> Optional[dic return None token = authorization[len("Bearer ") :] - if not token.startswith(prefix): - return None - - # Remove prefix and verify signature - token = token[len(prefix) :] - try: - decoded = datasette.unsign(token, namespace="token") - except itsdangerous.BadSignature: - return None - - # Validate timestamp - if "t" not in decoded: - return None - created = decoded["t"] - if not isinstance(created, int): - return None - - # Handle duration/expiry - duration = decoded.get("d") - if duration is not None and not isinstance(duration, int): - return None - - # Apply max TTL if configured - if (duration is None and max_signed_tokens_ttl) or ( - duration is not None - and max_signed_tokens_ttl - and duration > max_signed_tokens_ttl - ): - duration = max_signed_tokens_ttl - - # Check expiry - if duration: - if time.time() - created > duration: - return None - - # Build actor dict - actor = {"id": decoded["a"], "token": "dstok"} - - # Copy restrictions if present - if "_r" in decoded: - actor["_r"] = decoded["_r"] - - # Add expiry timestamp if applicable - if duration: - actor["token_expires"] = created + duration - - return actor + return await datasette.verify_token(token) diff --git a/datasette/hookspecs.py b/datasette/hookspecs.py index 89be6a65..64901900 100644 --- a/datasette/hookspecs.py +++ b/datasette/hookspecs.py @@ -222,6 +222,11 @@ def top_canned_query(datasette, request, database, query_name): """HTML to include at the top of the canned query page""" +@hookspec +def register_token_handler(datasette): + """Return a TokenHandler instance for token creation and verification""" + + @hookspec def write_wrapper(datasette, database, request, transaction): """Called when a write function is about to execute. diff --git a/datasette/plugins.py b/datasette/plugins.py index e9818885..992137bd 100644 --- a/datasette/plugins.py +++ b/datasette/plugins.py @@ -23,6 +23,7 @@ DEFAULT_PLUGINS = ( "datasette.sql_functions", "datasette.actor_auth_cookie", "datasette.default_permissions", + "datasette.default_permissions.tokens", "datasette.default_actions", "datasette.default_magic_parameters", "datasette.blob_renderer", diff --git a/datasette/tokens.py b/datasette/tokens.py new file mode 100644 index 00000000..5a12d8e0 --- /dev/null +++ b/datasette/tokens.py @@ -0,0 +1,180 @@ +""" +Token handler system for Datasette. + +Provides a base class for token handlers and the default signed token handler. +Plugins can implement register_token_handler to provide custom token backends +(e.g. database-backed tokens that can be revoked and audited). +""" + +from __future__ import annotations + +import dataclasses +import time +from typing import TYPE_CHECKING, Optional + +import itsdangerous + +if TYPE_CHECKING: + from datasette.app import Datasette + + +@dataclasses.dataclass +class TokenRestrictions: + """ + Restrictions to apply to a token, limiting which actions it can perform. + + Use the builder methods to construct restrictions:: + + restrictions = (TokenRestrictions() + .allow_all("view-instance") + .allow_database("mydb", "create-table") + .allow_resource("mydb", "mytable", "insert-row")) + """ + + all: list[str] = dataclasses.field(default_factory=list) + database: dict[str, list[str]] = dataclasses.field(default_factory=dict) + resource: dict[str, dict[str, list[str]]] = dataclasses.field(default_factory=dict) + + def allow_all(self, action: str) -> "TokenRestrictions": + """Allow an action across all databases and resources.""" + self.all.append(action) + return self + + def allow_database(self, database: str, action: str) -> "TokenRestrictions": + """Allow an action on a specific database.""" + self.database.setdefault(database, []).append(action) + return self + + def allow_resource( + self, database: str, resource: str, action: str + ) -> "TokenRestrictions": + """Allow an action on a specific resource within a database.""" + self.resource.setdefault(database, {}).setdefault(resource, []).append(action) + return self + + +class TokenHandler: + """ + Base class for token handlers. + + Subclass this and implement create_token() and verify_token() to provide + a custom token backend. Return an instance from the register_token_handler hook. + """ + + name: str = "" + + async def create_token( + self, + datasette: "Datasette", + actor_id: str, + *, + expires_after: Optional[int] = None, + restrictions: Optional[TokenRestrictions] = None, + ) -> str: + """Create and return a token string for the given actor.""" + raise NotImplementedError + + async def verify_token(self, datasette: "Datasette", token: str) -> Optional[dict]: + """ + Verify a token and return an actor dict, or None if this handler + does not recognize the token. + """ + raise NotImplementedError + + +class SignedTokenHandler(TokenHandler): + """ + Default token handler using itsdangerous signed tokens (dstok_ prefix). + """ + + name = "signed" + + async def create_token( + self, + datasette: "Datasette", + actor_id: str, + *, + expires_after: Optional[int] = None, + restrictions: Optional[TokenRestrictions] = None, + ) -> str: + if not datasette.setting("allow_signed_tokens"): + raise ValueError( + "Signed tokens are not enabled for this Datasette instance" + ) + + token = {"a": actor_id, "t": int(time.time())} + + def abbreviate_action(action): + action_obj = datasette.actions.get(action) + if not action_obj: + return action + return action_obj.abbr or action + + if expires_after: + token["d"] = expires_after + if restrictions and ( + restrictions.all or restrictions.database or restrictions.resource + ): + token["_r"] = {} + if restrictions.all: + token["_r"]["a"] = [abbreviate_action(a) for a in restrictions.all] + if restrictions.database: + token["_r"]["d"] = {} + for database, actions in restrictions.database.items(): + token["_r"]["d"][database] = [abbreviate_action(a) for a in actions] + if restrictions.resource: + token["_r"]["r"] = {} + for database, resources in restrictions.resource.items(): + for resource, actions in resources.items(): + token["_r"]["r"].setdefault(database, {})[resource] = [ + abbreviate_action(a) for a in actions + ] + return "dstok_{}".format(datasette.sign(token, namespace="token")) + + async def verify_token(self, datasette: "Datasette", token: str) -> Optional[dict]: + prefix = "dstok_" + + if not datasette.setting("allow_signed_tokens"): + return None + + max_signed_tokens_ttl = datasette.setting("max_signed_tokens_ttl") + + if not token.startswith(prefix): + return None + + raw = token[len(prefix) :] + try: + decoded = datasette.unsign(raw, namespace="token") + except itsdangerous.BadSignature: + return None + + if "t" not in decoded: + return None + created = decoded["t"] + if not isinstance(created, int): + return None + + duration = decoded.get("d") + if duration is not None and not isinstance(duration, int): + return None + + if (duration is None and max_signed_tokens_ttl) or ( + duration is not None + and max_signed_tokens_ttl + and duration > max_signed_tokens_ttl + ): + duration = max_signed_tokens_ttl + + if duration: + if time.time() - created > duration: + return None + + actor = {"id": decoded["a"], "token": "dstok"} + + if "_r" in decoded: + actor["_r"] = decoded["_r"] + + if duration: + actor["token_expires"] = created + duration + + return actor diff --git a/datasette/views/special.py b/datasette/views/special.py index 640c82eb..dbe5eab1 100644 --- a/datasette/views/special.py +++ b/datasette/views/special.py @@ -710,42 +710,36 @@ class CreateTokenView(BaseView): errors.append("Invalid expire duration unit") # Are there any restrictions? - restrict_all = [] - restrict_database = {} - restrict_resource = {} + from datasette.tokens import TokenRestrictions + + restrictions = TokenRestrictions() for key in form: if key.startswith("all:") and key.count(":") == 1: - restrict_all.append(key.split(":")[1]) + restrictions.allow_all(key.split(":")[1]) elif key.startswith("database:") and key.count(":") == 2: bits = key.split(":") - database = tilde_decode(bits[1]) - action = bits[2] - restrict_database.setdefault(database, []).append(action) + restrictions.allow_database(tilde_decode(bits[1]), bits[2]) elif key.startswith("resource:") and key.count(":") == 3: bits = key.split(":") - database = tilde_decode(bits[1]) - resource = tilde_decode(bits[2]) - action = bits[3] - restrict_resource.setdefault(database, {}).setdefault( - resource, [] - ).append(action) + restrictions.allow_resource( + tilde_decode(bits[1]), tilde_decode(bits[2]), bits[3] + ) - token = self.ds.create_token( + token = await self.ds.create_token( request.actor["id"], expires_after=expires_after, - restrict_all=restrict_all, - restrict_database=restrict_database, - restrict_resource=restrict_resource, + restrictions=restrictions, + handler="signed", ) token_bits = self.ds.unsign(token[len("dstok_") :], namespace="token") await self.ds.track_event( CreateTokenEvent( actor=request.actor, expires_after=expires_after, - restrict_all=restrict_all, - restrict_database=restrict_database, - restrict_resource=restrict_resource, + restrict_all=restrictions.all, + restrict_database=restrictions.database, + restrict_resource=restrictions.resource, ) ) context = await self.shared(request) diff --git a/docs/authentication.rst b/docs/authentication.rst index 69a6f606..1b949f9a 100644 --- a/docs/authentication.rst +++ b/docs/authentication.rst @@ -1072,6 +1072,7 @@ cannot grant new access. If the underlying actor is denied by ``allow`` rules in ``datasette.yaml`` or by a plugin, a token that lists that resource in its ``"_r"`` section will still be denied. +To create tokens with restrictions in Python code, use the :ref:`TokenRestrictions ` builder and pass it to :ref:`datasette.create_token() `. .. _permissions_plugins: diff --git a/docs/internals.rst b/docs/internals.rst index 0491c1f7..7d607bfe 100644 --- a/docs/internals.rst +++ b/docs/internals.rst @@ -673,8 +673,8 @@ This example checks if the user can access a specific table, and sets ``private` .. _datasette_create_token: -.create_token(actor_id, expires_after=None, restrict_all=None, restrict_database=None, restrict_resource=None) --------------------------------------------------------------------------------------------------------------- +await .create_token(actor_id, expires_after=None, restrictions=None, handler=None) +---------------------------------------------------------------------------------- ``actor_id`` - string The ID of the actor to create a token for. @@ -682,16 +682,13 @@ This example checks if the user can access a specific table, and sets ``private` ``expires_after`` - int, optional The number of seconds after which the token should expire. -``restrict_all`` - iterable, optional - A list of actions that this token should be restricted to across all databases and resources. +``restrictions`` - :ref:`TokenRestrictions `, optional + A :ref:`TokenRestrictions ` object limiting which actions the token can perform. -``restrict_database`` - dict, optional - For restricting actions within specific databases, e.g. ``{"mydb": ["view-table", "view-query"]}``. +``handler`` - string, optional + The name of a specific token handler to use. If omitted, the first registered handler is used. See :ref:`plugin_hook_register_token_handler`. -``restrict_resource`` - dict, optional - For restricting actions to specific resources (tables, SQL views and :ref:`canned_queries`) within a database. For example: ``{"mydb": {"mytable": ["insert-row", "update-row"]}}``. - -This method returns a signed :ref:`API token ` of the format ``dstok_...`` which can be used to authenticate requests to the Datasette API. +This is an ``async`` method that returns an :ref:`API token ` string which can be used to authenticate requests to the Datasette API. The default ``SignedTokenHandler`` returns tokens of the format ``dstok_...``. All tokens must have an ``actor_id`` string indicating the ID of the actor which the token will act on behalf of. @@ -699,28 +696,72 @@ Tokens default to lasting forever, but can be set to expire after a given number .. code-block:: python - token = datasette.create_token( + token = await datasette.create_token( actor_id="user1", expires_after=3600, ) -The three ``restrict_*`` arguments can be used to create a token that has additional restrictions beyond what the associated actor is allowed to do. +.. _TokenRestrictions: + +TokenRestrictions +~~~~~~~~~~~~~~~~~ + +The ``TokenRestrictions`` class uses a builder pattern to specify which actions a token is allowed to perform. Import it from ``datasette.tokens``: + +.. code-block:: python + + from datasette.tokens import TokenRestrictions + + restrictions = ( + TokenRestrictions() + .allow_all("view-instance") + .allow_all("view-table") + .allow_database("docs", "view-query") + .allow_resource("docs", "attachments", "insert-row") + .allow_resource("docs", "attachments", "update-row") + ) + +The builder methods are: + +- ``allow_all(action)`` - allow an action across all databases and resources +- ``allow_database(database, action)`` - allow an action on a specific database +- ``allow_resource(database, resource, action)`` - allow an action on a specific resource (table, SQL view or :ref:`canned query `) within a database + +Each method returns the ``TokenRestrictions`` instance so calls can be chained. The following example creates a token that can access ``view-instance`` and ``view-table`` across everything, can additionally use ``view-query`` for anything in the ``docs`` database and is allowed to execute ``insert-row`` and ``update-row`` in the ``attachments`` table in that database: .. code-block:: python - token = datasette.create_token( + token = await datasette.create_token( actor_id="user1", - restrict_all=("view-instance", "view-table"), - restrict_database={"docs": ("view-query",)}, - restrict_resource={ - "docs": { - "attachments": ("insert-row", "update-row") - } - }, + restrictions=( + TokenRestrictions() + .allow_all("view-instance") + .allow_all("view-table") + .allow_database("docs", "view-query") + .allow_resource("docs", "attachments", "insert-row") + .allow_resource("docs", "attachments", "update-row") + ), ) +.. _datasette_verify_token: + +await .verify_token(token) +-------------------------- + +``token`` - string + The token string to verify. + +This is an ``async`` method that verifies an API token by trying each registered token handler in order. Returns an actor dictionary from the first handler that recognizes the token, or ``None`` if no handler accepts it. + +.. code-block:: python + + actor = await datasette.verify_token(token) + if actor: + # Token was valid + print(actor["id"]) + .. _datasette_get_database: .get_database(name) diff --git a/docs/plugin_hooks.rst b/docs/plugin_hooks.rst index fa335368..b9701f7c 100644 --- a/docs/plugin_hooks.rst +++ b/docs/plugin_hooks.rst @@ -2334,3 +2334,62 @@ The plugin can then call ``datasette.track_event(...)`` to send a ``ban-user`` e BanUserEvent(user={"id": 1, "username": "cleverbot"}) ) +.. _plugin_hook_register_token_handler: + +register_token_handler(datasette) +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +``datasette`` - :ref:`internals_datasette` + You can use this to access plugin configuration options via ``datasette.plugin_config(your_plugin_name)``. + +Return a ``TokenHandler`` instance to provide a custom token creation and verification backend. This hook can return a single ``TokenHandler`` or a list of them. + +The default ``SignedTokenHandler`` uses itsdangerous signed tokens (``dstok_`` prefix). Plugins can provide alternative backends such as database-backed tokens that support revocation and auditing. + +.. code-block:: python + + from datasette import hookimpl, TokenHandler + + + class DatabaseTokenHandler(TokenHandler): + name = "database" + + async def create_token( + self, + datasette, + actor_id, + *, + expires_after=None, + restrictions=None + ): + # Store token in database and return token string + ... + + async def verify_token(self, datasette, token): + # Look up token in database, return actor dict or None + ... + + + @hookimpl + def register_token_handler(datasette): + return DatabaseTokenHandler() + +The ``create_token`` method receives a ``restrictions`` argument which will be a :ref:`TokenRestrictions ` instance or ``None``. + +Tokens can then be created and verified using :ref:`datasette.create_token() ` and ``datasette.verify_token()``, which delegate to the registered handlers. If no ``handler`` is specified, the first handler is used according to `pluggy call-time ordering `_. Use the ``handler`` parameter to select a specific backend by name: + +.. code-block:: python + + # Uses first registered handler (default) + token = await datasette.create_token("user123") + + # Uses a specific handler by name + token = await datasette.create_token( + "user123", handler="database" + ) + + # Verification tries all handlers + actor = await datasette.verify_token(token) + +If no handlers are registered, ``create_token()`` raises ``RuntimeError``. If the requested ``handler`` name is not found, it raises ``ValueError``. + diff --git a/docs/plugins.rst b/docs/plugins.rst index d5a98923..60bdc111 100644 --- a/docs/plugins.rst +++ b/docs/plugins.rst @@ -231,12 +231,21 @@ If you run ``datasette plugins --all`` it will include default plugins that ship "templates": false, "version": null, "hooks": [ - "actor_from_request", "canned_queries", "permission_resources_sql", "skip_csrf" ] }, + { + "name": "datasette.default_permissions.tokens", + "static": false, + "templates": false, + "version": null, + "hooks": [ + "actor_from_request", + "register_token_handler" + ] + }, { "name": "datasette.events", "static": false, diff --git a/docs/upgrade_guide.md b/docs/upgrade_guide.md index a3c321a4..861a8795 100644 --- a/docs/upgrade_guide.md +++ b/docs/upgrade_guide.md @@ -114,3 +114,43 @@ Instead, one should use the following methods on a Datasette class: ```{include} upgrade-1.0a20.md :heading-offset: 1 ``` + +(upgrade_guide_v1_a25)= +### Datasette 1.0a25: `create_token()` signature change + +`datasette.create_token()` is now an `async` method (previously it was synchronous). The `restrict_all`, `restrict_database`, and `restrict_resource` keyword arguments have been replaced by a single `restrictions` parameter that accepts a {ref}`TokenRestrictions ` object. + +Old code: + +```python +token = datasette.create_token( + actor_id="user1", + restrict_all=["view-instance", "view-table"], + restrict_database={"docs": ["view-query"]}, + restrict_resource={ + "docs": { + "attachments": ["insert-row", "update-row"] + } + }, +) +``` + +New code: + +```python +from datasette.tokens import TokenRestrictions + +token = await datasette.create_token( + actor_id="user1", + restrictions=( + TokenRestrictions() + .allow_all("view-instance") + .allow_all("view-table") + .allow_database("docs", "view-query") + .allow_resource("docs", "attachments", "insert-row") + .allow_resource("docs", "attachments", "update-row") + ), +) +``` + +The `datasette create-token` CLI command is unchanged. diff --git a/tests/fixtures.py b/tests/fixtures.py index 9f99519a..1f6c491d 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -51,6 +51,7 @@ EXPECTED_PLUGINS = [ "register_facet_classes", "register_magic_parameters", "register_routes", + "register_token_handler", "render_cell", "row_actions", "skip_csrf", diff --git a/tests/plugins/my_plugin.py b/tests/plugins/my_plugin.py index 20e7d111..77079557 100644 --- a/tests/plugins/my_plugin.py +++ b/tests/plugins/my_plugin.py @@ -1,6 +1,7 @@ import asyncio from datasette import hookimpl from datasette.facets import Facet +from datasette.tokens import TokenHandler from datasette import tracer from datasette.permissions import Action from datasette.resources import DatabaseResource @@ -586,3 +587,29 @@ def permission_resources_sql(datasette, actor, action): return PermissionSQL.allow(reason=f"todomvc actor allowed for {action}") return None + + +class HardcodedTokenHandler(TokenHandler): + name = "hardcoded" + _counter = 0 + + async def create_token( + self, + datasette, + actor_id, + *, + expires_after=None, + restrictions=None, + ): + HardcodedTokenHandler._counter += 1 + return f"dstok_hardcoded_token_{HardcodedTokenHandler._counter}" + + async def verify_token(self, datasette, token): + if token.startswith("dstok_hardcoded_token_"): + return {"id": "hardcoded-actor", "token": "hardcoded"} + return None + + +@hookimpl +def register_token_handler(datasette): + return HardcodedTokenHandler() diff --git a/tests/test_api_write.py b/tests/test_api_write.py index 05835e51..e59c4295 100644 --- a/tests/test_api_write.py +++ b/tests/test_api_write.py @@ -1362,7 +1362,14 @@ async def test_create_table( async def test_create_table_permissions( ds_write, permissions, body, expected_status, expected_errors ): - token = ds_write.create_token("root", restrict_all=["view-instance"] + permissions) + from datasette.tokens import TokenRestrictions + + restrictions = TokenRestrictions() + for action in ["view-instance"] + permissions: + restrictions.allow_all(action) + token = await ds_write.create_token( + "root", handler="signed", restrictions=restrictions + ) response = await ds_write.client.post( "/data/-/create", json=body, diff --git a/tests/test_permissions.py b/tests/test_permissions.py index 96c0cf6f..42a19ca4 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -1657,7 +1657,7 @@ async def test_permission_check_view_requires_debug_permission(): # Root user should have access (root has all permissions) ds_with_root = Datasette() ds_with_root.root_enabled = True - root_token = ds_with_root.create_token("root") + root_token = await ds_with_root.create_token("root", handler="signed") response = await ds_with_root.client.get( "/-/check.json?action=view-instance", headers={"Authorization": f"Bearer {root_token}"}, diff --git a/tests/test_plugins.py b/tests/test_plugins.py index 754b199c..fa9d1a1f 100644 --- a/tests/test_plugins.py +++ b/tests/test_plugins.py @@ -1566,6 +1566,38 @@ async def test_hook_register_events(): assert any(k.__name__ == "OneEvent" for k in datasette.event_classes) +@pytest.mark.asyncio +async def test_hook_register_token_handler(ds_client): + handlers = ds_client.ds._token_handlers() + handler_names = [h.name for h in handlers] + # Both the default signed handler and the test hardcoded handler + assert "signed" in handler_names + assert "hardcoded" in handler_names + + # Create a token using the hardcoded handler (first registered from plugins dir) + token = await ds_client.ds.create_token("test-user") + assert token.startswith("dstok_hardcoded_token_") + + # Verify it + actor = await ds_client.ds.verify_token(token) + assert actor["id"] == "hardcoded-actor" + assert actor["token"] == "hardcoded" + + # Create a token by explicitly requesting the hardcoded handler by name + token2 = await ds_client.ds.create_token("test-user", handler="hardcoded") + assert token2.startswith("dstok_hardcoded_token_") + actor2 = await ds_client.ds.verify_token(token2) + assert actor2["id"] == "hardcoded-actor" + + # Create a token by explicitly requesting the signed handler by name + signed_token = await ds_client.ds.create_token("test-user", handler="signed") + assert signed_token.startswith("dstok_") + assert not signed_token.startswith("dstok_hardcoded_token_") + signed_actor = await ds_client.ds.verify_token(signed_token) + assert signed_actor["id"] == "test-user" + assert signed_actor["token"] == "dstok" + + @pytest.mark.asyncio async def test_hook_write_wrapper(): datasette = Datasette(memory=True) diff --git a/tests/test_token_handler.py b/tests/test_token_handler.py new file mode 100644 index 00000000..83f09046 --- /dev/null +++ b/tests/test_token_handler.py @@ -0,0 +1,301 @@ +""" +Tests for the register_token_handler plugin hook. +""" + +from datasette.app import Datasette +from datasette.hookspecs import hookimpl +from datasette.plugins import pm +from datasette.tokens import TokenHandler, TokenRestrictions, SignedTokenHandler +import pytest + + +@pytest.fixture +def datasette(): + return Datasette() + + +@pytest.mark.asyncio +async def test_default_signed_handler_registered(datasette): + """The default SignedTokenHandler should be registered automatically.""" + handlers = datasette._token_handlers() + assert len(handlers) >= 1 + assert any(isinstance(h, SignedTokenHandler) for h in handlers) + assert any(h.name == "signed" for h in handlers) + + +@pytest.mark.asyncio +async def test_create_token_default(datasette): + """create_token() with handler='signed' should create a signed token.""" + token = await datasette.create_token("test_actor", handler="signed") + assert token.startswith("dstok_") + + +@pytest.mark.asyncio +async def test_create_token_with_restrictions(datasette): + """create_token() should handle restriction parameters.""" + token = await datasette.create_token( + "test_actor", + handler="signed", + expires_after=3600, + restrictions=TokenRestrictions().allow_all("view-instance"), + ) + assert token.startswith("dstok_") + # Verify the token contains the expected data + decoded = datasette.unsign(token[len("dstok_") :], namespace="token") + assert decoded["a"] == "test_actor" + assert decoded["d"] == 3600 + assert "_r" in decoded + assert "a" in decoded["_r"] + + +@pytest.mark.asyncio +async def test_verify_token_default(datasette): + """verify_token() should verify signed tokens.""" + token = await datasette.create_token("test_actor", handler="signed") + actor = await datasette.verify_token(token) + assert actor is not None + assert actor["id"] == "test_actor" + assert actor["token"] == "dstok" + + +@pytest.mark.asyncio +async def test_verify_token_unknown_returns_none(datasette): + """verify_token() should return None for unrecognized tokens.""" + result = await datasette.verify_token("unknown_token_format_xyz") + assert result is None + + +@pytest.mark.asyncio +async def test_verify_token_bad_signature_returns_none(datasette): + """verify_token() should return None for tokens with bad signatures.""" + result = await datasette.verify_token("dstok_tampered_data_here") + assert result is None + + +@pytest.mark.asyncio +async def test_create_token_with_named_handler(datasette): + """create_token(handler='signed') should select the signed handler.""" + token = await datasette.create_token("test_actor", handler="signed") + assert token.startswith("dstok_") + + +@pytest.mark.asyncio +async def test_create_token_unknown_handler_raises(datasette): + """create_token(handler='nonexistent') should raise ValueError.""" + with pytest.raises(ValueError, match="Token handler 'nonexistent' not found"): + await datasette.create_token("test_actor", handler="nonexistent") + + +@pytest.mark.asyncio +async def test_custom_token_handler(datasette): + """A custom token handler should be usable for both create and verify.""" + + class CustomHandler(TokenHandler): + name = "custom" + + async def create_token(self, datasette, actor_id, **kwargs): + return f"custom_{actor_id}" + + async def verify_token(self, datasette, token): + if token.startswith("custom_"): + return {"id": token[len("custom_") :], "token": "custom"} + return None + + class Plugin: + __name__ = "CustomTokenPlugin" + + @staticmethod + @hookimpl + def register_token_handler(datasette): + return CustomHandler() + + pm.register(Plugin(), name="test_custom_handler") + try: + handlers = datasette._token_handlers() + assert any(h.name == "custom" for h in handlers) + + # Create with custom handler + token = await datasette.create_token("alice", handler="custom") + assert token == "custom_alice" + + # Verify custom token + actor = await datasette.verify_token("custom_alice") + assert actor is not None + assert actor["id"] == "alice" + assert actor["token"] == "custom" + + # Signed tokens should still work + signed_token = await datasette.create_token("bob", handler="signed") + assert signed_token.startswith("dstok_") + actor = await datasette.verify_token(signed_token) + assert actor["id"] == "bob" + finally: + pm.unregister(name="test_custom_handler") + + +@pytest.mark.asyncio +async def test_verify_token_tries_all_handlers(datasette): + """verify_token() should try each handler until one matches.""" + + class HandlerA(TokenHandler): + name = "handler_a" + + async def create_token(self, datasette, actor_id, **kwargs): + return f"a_{actor_id}" + + async def verify_token(self, datasette, token): + if token.startswith("a_"): + return {"id": token[2:], "token": "handler_a"} + return None + + class HandlerB(TokenHandler): + name = "handler_b" + + async def create_token(self, datasette, actor_id, **kwargs): + return f"b_{actor_id}" + + async def verify_token(self, datasette, token): + if token.startswith("b_"): + return {"id": token[2:], "token": "handler_b"} + return None + + class PluginA: + __name__ = "PluginA" + + @staticmethod + @hookimpl + def register_token_handler(datasette): + return HandlerA() + + class PluginB: + __name__ = "PluginB" + + @staticmethod + @hookimpl + def register_token_handler(datasette): + return HandlerB() + + pm.register(PluginA(), name="test_handler_a") + pm.register(PluginB(), name="test_handler_b") + try: + # Both handler tokens should verify + actor_a = await datasette.verify_token("a_alice") + assert actor_a is not None + assert actor_a["id"] == "alice" + assert actor_a["token"] == "handler_a" + + actor_b = await datasette.verify_token("b_bob") + assert actor_b is not None + assert actor_b["id"] == "bob" + assert actor_b["token"] == "handler_b" + + # Unknown token should return None + assert await datasette.verify_token("c_charlie") is None + finally: + pm.unregister(name="test_handler_a") + pm.unregister(name="test_handler_b") + + +@pytest.mark.asyncio +async def test_token_handler_via_http(datasette): + """Default signed tokens should work through HTTP auth.""" + token = await datasette.create_token("http_user", handler="signed") + response = await datasette.client.get( + "/-/actor.json", + headers={"Authorization": f"Bearer {token}"}, + ) + assert response.status_code == 200 + actor = response.json()["actor"] + assert actor["id"] == "http_user" + assert actor["token"] == "dstok" + + +@pytest.mark.asyncio +async def test_custom_handler_via_http(datasette): + """Custom handler tokens should work through HTTP auth.""" + + class CustomHandler(TokenHandler): + name = "custom_http" + + async def create_token(self, datasette, actor_id, **kwargs): + return f"chttp_{actor_id}" + + async def verify_token(self, datasette, token): + if token.startswith("chttp_"): + return {"id": token[len("chttp_") :], "token": "custom_http"} + return None + + class Plugin: + __name__ = "CustomHTTPPlugin" + + @staticmethod + @hookimpl + def register_token_handler(datasette): + return CustomHandler() + + pm.register(Plugin(), name="test_custom_http") + try: + token = await datasette.create_token("web_user", handler="custom_http") + response = await datasette.client.get( + "/-/actor.json", + headers={"Authorization": f"Bearer {token}"}, + ) + assert response.status_code == 200 + actor = response.json()["actor"] + assert actor["id"] == "web_user" + assert actor["token"] == "custom_http" + finally: + pm.unregister(name="test_custom_http") + + +@pytest.mark.asyncio +async def test_token_handler_base_class_raises(): + """TokenHandler base class methods should raise NotImplementedError.""" + handler = TokenHandler() + ds = Datasette() + with pytest.raises(NotImplementedError): + await handler.create_token(ds, "test") + with pytest.raises(NotImplementedError): + await handler.verify_token(ds, "test") + + +@pytest.mark.asyncio +async def test_restrictions_round_trip(datasette): + """Tokens with database/resource restrictions should round-trip correctly.""" + restrictions = ( + TokenRestrictions() + .allow_all("view-instance") + .allow_database("docs", "view-query") + .allow_resource("docs", "attachments", "insert-row") + ) + token = await datasette.create_token( + "test_actor", handler="signed", restrictions=restrictions + ) + actor = await datasette.verify_token(token) + assert actor is not None + assert actor["id"] == "test_actor" + assert actor["_r"]["a"] == ["view-instance"] + assert actor["_r"]["d"] == {"docs": ["view-query"]} + assert actor["_r"]["r"] == {"docs": {"attachments": ["insert-row"]}} + + +@pytest.mark.asyncio +async def test_expires_after_round_trip(datasette): + """Tokens with expires_after should include token_expires in the actor.""" + token = await datasette.create_token( + "test_actor", handler="signed", expires_after=3600 + ) + actor = await datasette.verify_token(token) + assert actor is not None + assert actor["id"] == "test_actor" + assert "token_expires" in actor + + +@pytest.mark.asyncio +async def test_signed_tokens_disabled(): + """create_token and verify_token should fail/skip when signed tokens are disabled.""" + ds = Datasette(settings={"allow_signed_tokens": False}) + with pytest.raises(ValueError, match="Signed tokens are not enabled"): + await ds.create_token("test_actor", handler="signed") + # verify_token should return None rather than raising + assert await ds.verify_token("dstok_anything") is None From 24d801b7f799912cb4eb897a97e4f4a9fe76b966 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Wed, 25 Feb 2026 16:33:27 -0800 Subject: [PATCH 015/481] Respect metadata-defined facet ordering in sorted_facet_results (#2648) * Preserve metadata-defined facet ordering on table pages When facets are explicitly defined in table metadata/config, they now appear in the order specified in the configuration rather than being sorted by result count. Request-added facets still appear after metadata-defined facets, sorted by count as before. * Document metadata-defined facet ordering behavior * Apply black formatting https://claude.ai/code/session_01PbSHtjsUpNk3Fx7xjvVqDb --- datasette/views/table.py | 34 ++++++++++++++++++++++++++----- docs/facets.rst | 2 ++ tests/test_facets.py | 44 ++++++++++++++++++++++++++++++++++++---- 3 files changed, 71 insertions(+), 9 deletions(-) diff --git a/datasette/views/table.py b/datasette/views/table.py index 594e925e..e1e5507f 100644 --- a/datasette/views/table.py +++ b/datasette/views/table.py @@ -1580,11 +1580,35 @@ async def table_view_data( ] async def extra_sorted_facet_results(extra_facet_results): - return sorted( - extra_facet_results["results"].values(), - key=lambda f: (len(f["results"]), f["name"]), - reverse=True, - ) + facet_configs = table_metadata.get("facets", []) + if facet_configs: + # Build ordered list of facet names from metadata config + metadata_facet_names = [] + for fc in facet_configs: + if isinstance(fc, str): + metadata_facet_names.append(fc) + elif isinstance(fc, dict): + metadata_facet_names.append(list(fc.values())[0]) + metadata_order = {name: i for i, name in enumerate(metadata_facet_names)} + metadata_facets = [] + request_facets = [] + for f in extra_facet_results["results"].values(): + if f["name"] in metadata_order: + metadata_facets.append(f) + else: + request_facets.append(f) + metadata_facets.sort(key=lambda f: metadata_order[f["name"]]) + request_facets.sort( + key=lambda f: (len(f["results"]), f["name"]), + reverse=True, + ) + return metadata_facets + request_facets + else: + return sorted( + extra_facet_results["results"].values(), + key=lambda f: (len(f["results"]), f["name"]), + reverse=True, + ) async def extra_table_definition(): return await db.get_table_definition(table_name) diff --git a/docs/facets.rst b/docs/facets.rst index 15fe7227..2a135b69 100644 --- a/docs/facets.rst +++ b/docs/facets.rst @@ -153,6 +153,8 @@ Here's an example that turns on faceting by default for the ``qLegalStatus`` col Facets defined in this way will always be shown in the interface and returned in the API, regardless of the ``_facet`` arguments passed to the view. +Facets defined in metadata will be displayed in the order they are listed in the configuration. Any additional facets added via query string parameters (e.g. ``?_facet=column_name``) will appear after the metadata-defined facets, sorted by the number of unique values. + You can specify :ref:`array ` or :ref:`date ` facets in metadata using JSON objects with a single key of ``array`` or ``date`` and a value specifying the column, like this: .. [[[cog diff --git a/tests/test_facets.py b/tests/test_facets.py index a2b505ec..8c22ffce 100644 --- a/tests/test_facets.py +++ b/tests/test_facets.py @@ -623,12 +623,48 @@ def test_other_types_of_facet_in_metadata(): } ) as client: response = client.get("/fixtures/facetable") - for fragment in ( - "created (date)\n", - "tags (array)\n", + fragments = ( "state\n", - ): + "tags (array)\n", + "created (date)\n", + ) + for fragment in fragments: assert fragment in response.text + # Verify they appear in the metadata-defined order + positions = [response.text.index(f) for f in fragments] + assert positions == sorted( + positions + ), "Facets should appear in metadata-defined order" + + +def test_metadata_facet_ordering(): + with make_app_client( + metadata={ + "databases": { + "fixtures": { + "tables": { + "facetable": { + "facets": ["state", {"array": "tags"}, {"date": "created"}] + } + } + } + } + } + ) as client: + # JSON response should have facets in the metadata-defined order + response = client.get("/fixtures/facetable.json?_extra=sorted_facet_results") + data = response.json + facet_names = [f["name"] for f in data["sorted_facet_results"]] + assert facet_names == ["state", "tags", "created"] + + # With an additional request-based facet, metadata facets come first + # in their defined order, followed by request-based facets + response2 = client.get( + "/fixtures/facetable.json?_extra=sorted_facet_results&_facet=_city_id" + ) + data2 = response2.json + facet_names2 = [f["name"] for f in data2["sorted_facet_results"]] + assert facet_names2 == ["state", "tags", "created", "_city_id"] @pytest.mark.asyncio From 2bc1dd2275978e75622c5764729a4273ebac957e Mon Sep 17 00:00:00 2001 From: Daniel Bates Date: Wed, 25 Feb 2026 16:46:29 -0800 Subject: [PATCH 016/481] Fix --reload interpreting 'serve' command as a file argument (#2646) When hupper spawns the worker process, it calls the function specified by worker_path directly. Using "datasette.cli.serve" causes Click to parse sys.argv without going through the CLI group, so the literal word "serve" from the original command gets treated as a positional file argument. Change the worker path to "datasette.cli.cli" so the worker process goes through the Click group dispatcher, which properly recognizes "serve" as a subcommand and strips it from the argument list. Closes #2123 Co-authored-by: Claude Opus 4.6 Co-authored-by: Simon Willison --- datasette/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datasette/cli.py b/datasette/cli.py index b473fbb7..db777fe8 100644 --- a/datasette/cli.py +++ b/datasette/cli.py @@ -547,7 +547,7 @@ def serve( if reload: import hupper - reloader = hupper.start_reloader("datasette.cli.serve") + reloader = hupper.start_reloader("datasette.cli.cli") if immutable: reloader.watch_files(immutable) if config: From 1246c6576bb2f1ba9dc5c7d9811427d00d440976 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Wed, 25 Feb 2026 16:49:14 -0800 Subject: [PATCH 017/481] Release 1.0a25 Refs #2636, #2641, #2646, #2647, #2650 --- docs/changelog.rst | 41 +++++++++++++++++++++++++++++++++++++++++ docs/contributing.rst | 1 + docs/upgrade-1.0a20.md | 1 - docs/upgrade_guide.md | 1 + 4 files changed, 43 insertions(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 67ceeece..c0467793 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -4,6 +4,47 @@ Changelog ========= +.. _v1_0_a25: + +1.0a25 (2026-02-25) +------------------- + +``write_wrapper`` plugin hook for intercepting write operations +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +A new :ref:`write_wrapper() ` plugin hook allows plugins to intercept and wrap database write operations. (`#2636 `__) + +Plugins implement the hook as a generator-based context manager: + +.. code-block:: python + + @hookimpl + def write_wrapper(datasette, database, request): + def wrapper(conn): + # Setup code runs before the write + yield + # Cleanup code runs after the write + + return wrapper + +``register_token_handler()`` plugin hook for custom API token backends +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +A new :ref:`register_token_handler() ` plugin hook allows plugins to provide custom token backends for API authentication. (`#2650 `__) + +This includes a **backwards incompatible change**: the ``datasette.create_token()`` internal method is now an ``async`` method. Consult the :ref:`upgrade guide ` for details on how to update your code. + +``render_cell()`` now receives a ``pks`` parameter +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The :ref:`render_cell() ` plugin hook now receives a ``pks`` parameter containing the list of primary key column names for the table being rendered. This avoids plugins needing to make redundant async calls to look up primary keys. (`#2641 `__) + +Other changes +~~~~~~~~~~~~~ + +- Facets defined in metadata now preserve their configured order, instead of being sorted by result count. Request-based facets added via the ``_facet`` parameter are still sorted by result count and appear after metadata-defined facets. (:issue:`2647`) +- Fixed ``--reload`` incorrectly interpreting the ``serve`` command as a file argument. Thanks, `Daniel Bates `__. (`#2646 `__) + .. _v1_0_a24: 1.0a24 (2026-01-29) diff --git a/docs/contributing.rst b/docs/contributing.rst index 3d41a125..635ca60e 100644 --- a/docs/contributing.rst +++ b/docs/contributing.rst @@ -90,6 +90,7 @@ If you want to change Datasette's Python code you can use the ``--reload`` optio You can also use the ``fixtures.py`` script to recreate the testing version of ``metadata.json`` used by the unit tests. To do that:: uv run python tests/fixtures.py fixtures.db fixtures-metadata.json + Or to output the plugins used by the tests, run this:: uv run python tests/fixtures.py fixtures.db fixtures-metadata.json fixtures-plugins diff --git a/docs/upgrade-1.0a20.md b/docs/upgrade-1.0a20.md index 749d383c..fbc3f4a8 100644 --- a/docs/upgrade-1.0a20.md +++ b/docs/upgrade-1.0a20.md @@ -2,7 +2,6 @@ orphan: true --- -(upgrade_guide_v1_a20)= # Datasette 1.0a20 plugin upgrade guide Datasette 1.0a20 makes some breaking changes to Datasette's permission system. Plugins need to be updated if they use **any of the following**: diff --git a/docs/upgrade_guide.md b/docs/upgrade_guide.md index 861a8795..b67eb054 100644 --- a/docs/upgrade_guide.md +++ b/docs/upgrade_guide.md @@ -111,6 +111,7 @@ Instead, one should use the following methods on a Datasette class: - {ref}`get_resource_metadata() ` - {ref}`get_column_metadata() ` +(upgrade_guide_v1_a20)= ```{include} upgrade-1.0a20.md :heading-offset: 1 ``` From e4ff5e27d356ca5b3c807e821acedf8c71c37e47 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Wed, 25 Feb 2026 16:54:51 -0800 Subject: [PATCH 018/481] Fix RST heading underlin --- docs/changelog.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index c0467793..1e6a8e90 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -35,7 +35,7 @@ A new :ref:`register_token_handler() ` plugi This includes a **backwards incompatible change**: the ``datasette.create_token()`` internal method is now an ``async`` method. Consult the :ref:`upgrade guide ` for details on how to update your code. ``render_cell()`` now receives a ``pks`` parameter -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The :ref:`render_cell() ` plugin hook now receives a ``pks`` parameter containing the list of primary key column names for the table being rendered. This avoids plugins needing to make redundant async calls to look up primary keys. (`#2641 `__) From 8f0d60236f844a6d12bd1439f57b1b3d65fcad36 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Wed, 25 Feb 2026 17:01:03 -0800 Subject: [PATCH 019/481] Bump version for 1.0a25 --- datasette/version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datasette/version.py b/datasette/version.py index de7585ca..2907e537 100644 --- a/datasette/version.py +++ b/datasette/version.py @@ -1,2 +1,2 @@ -__version__ = "1.0a24" +__version__ = "1.0a25" __version_info__ = tuple(__version__.split(".")) From 1263380ea6b138ac63683edfd525323c6fe8eef9 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Wed, 25 Feb 2026 20:50:46 -0800 Subject: [PATCH 020/481] Better heading for write_wrapper() --- docs/changelog.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 1e6a8e90..2c9b7170 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -9,8 +9,8 @@ Changelog 1.0a25 (2026-02-25) ------------------- -``write_wrapper`` plugin hook for intercepting write operations -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +``write_wrapper()`` plugin hook for intercepting write operations +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ A new :ref:`write_wrapper() ` plugin hook allows plugins to intercept and wrap database write operations. (`#2636 `__) From 97201f067c4f64b00ccf7e02f787d65c767f9bc9 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Fri, 6 Mar 2026 20:16:50 -0800 Subject: [PATCH 021/481] Row pages link to foreign keys from table display, closes #1592 https://gisthost.github.io/?40813f5b3e4d83c0efe1c09135f84290/index.html Also now shows primary key column first and in bold on that page. --- datasette/views/row.py | 64 ++++++++++++++++++++++++++++++++++++++++-- tests/test_html.py | 32 +++++++++++++++++---- 2 files changed, 88 insertions(+), 8 deletions(-) diff --git a/datasette/views/row.py b/datasette/views/row.py index 9c59cd3b..7cc46368 100644 --- a/datasette/views/row.py +++ b/datasette/views/row.py @@ -5,12 +5,14 @@ from datasette.resources import TableResource from .base import DataView, BaseView, _error from datasette.utils import ( await_me_maybe, + CustomRow, make_slot_function, to_css_class, escape_sqlite, ) from datasette.plugins import pm import json +import markupsafe import sqlite_utils from .table import display_columns_and_rows, _get_extras @@ -42,13 +44,62 @@ class RowView(DataView): if not rows: raise NotFound(f"Record not found: {pk_values}") + pks = resolved.pks + async def template_data(): + # Reorder columns so primary keys come first + pk_set = set(pks) + pk_cols = [d for d in results.description if d[0] in pk_set] + non_pk_cols = [d for d in results.description if d[0] not in pk_set] + reordered_description = pk_cols + non_pk_cols + reordered_columns = [d[0] for d in reordered_description] + + # Reorder row data to match + reordered_rows = [] + for row in rows: + new_row = CustomRow(reordered_columns) + for col in reordered_columns: + new_row[col] = row[col] + reordered_rows.append(new_row) + + # Expand foreign key columns into dicts so display_columns_and_rows + # renders them as hyperlinks, matching the table view behavior + expanded_rows = reordered_rows + for fk in await db.foreign_keys_for_table(table): + column = fk["column"] + if column not in reordered_columns: + continue + column_index = reordered_columns.index(column) + values = [row[column_index] for row in expanded_rows] + expanded_labels = await self.ds.expand_foreign_keys( + request.actor, database, table, column, values + ) + if expanded_labels: + new_rows = [] + for row in expanded_rows: + new_row = CustomRow(reordered_columns) + for col in reordered_columns: + value = row[col] + if ( + col == column + and (col, value) in expanded_labels + and value is not None + ): + new_row[col] = { + "value": value, + "label": expanded_labels[(col, value)], + } + else: + new_row[col] = value + new_rows.append(new_row) + expanded_rows = new_rows + display_columns, display_rows = await display_columns_and_rows( self.ds, database, table, - results.description, - rows, + reordered_description, + expanded_rows, link_column=False, truncate_cells=0, request=request, @@ -56,6 +107,14 @@ class RowView(DataView): for column in display_columns: column["sortable"] = False + # Bold primary key cell values + for row in display_rows: + for cell in row: + if cell["column"] in pk_set: + cell["value"] = markupsafe.Markup( + "{}".format(cell["value"]) + ) + row_actions = [] for hook in pm.hook.row_actions( datasette=self.ds, @@ -71,6 +130,7 @@ class RowView(DataView): return { "private": private, + "columns": reordered_columns, "foreign_key_tables": await self.foreign_key_tables( database, table, pk_values ), diff --git a/tests/test_html.py b/tests/test_html.py index 757f3e6e..64ae7b2d 100644 --- a/tests/test_html.py +++ b/tests/test_html.py @@ -347,7 +347,7 @@ async def test_row_html_simple_primary_key(ds_client): assert ["id", "content"] == [th.string.strip() for th in table.select("thead th")] assert [ [ - '1', + '1', 'hello', ] ] == [[str(td) for td in tr.select("td")] for tr in table.select("tbody tr")] @@ -363,7 +363,7 @@ async def test_row_html_no_primary_key(ds_client): ] expected = [ [ - '1', + '1', '1', 'a1', 'b1', @@ -406,6 +406,26 @@ async def test_row_links_from_other_tables( assert link == expected_link +@pytest.mark.asyncio +async def test_row_foreign_key_links(ds_client): + # Row detail page should render foreign key values as hyperlinks + response = await ds_client.get("/fixtures/foreign_key_references/1") + assert response.status_code == 200 + soup = Soup(response.text, "html.parser") + # foreign_key_with_label=1 references simple_primary_key(id=1, content="hello") + td = soup.find("td", {"class": "col-foreign_key_with_label"}) + a = td.find("a") + assert a is not None, "Expected foreign key value to be a hyperlink" + assert a["href"] == "/fixtures/simple_primary_key/1" + assert a.text == "hello" + # Primary key column should be first and bold + table = soup.find("table") + headers = [th.text.strip() for th in table.select("thead th")] + assert headers[0] == "pk" + first_td = table.select("tbody tr td")[0] + assert first_td.find("strong") is not None, "PK value should be bold" + + @pytest.mark.asyncio @pytest.mark.parametrize( "path,expected", @@ -414,8 +434,8 @@ async def test_row_links_from_other_tables( "/fixtures/compound_primary_key/a,b", [ [ - 'a', - 'b', + 'a', + 'b', 'c', ] ], @@ -424,8 +444,8 @@ async def test_row_links_from_other_tables( "/fixtures/compound_primary_key/a~2Fb,~2Ec~2Dd", [ [ - 'a/b', - '.c-d', + 'a/b', + '.c-d', 'c', ] ], From e2c1e81ec9505f02566de840c1dba5ea7b0b121d Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Mon, 9 Mar 2026 17:45:24 -0700 Subject: [PATCH 022/481] UI for selecting and re-ordering columns on the table page (#2662) New Web Component on table/view page with a dialog for selecting and re-ordering columns. Closes #2661 Refs #1298 --- datasette/static/app.css | 19 + datasette/static/column-chooser.js | 698 ++++++++++++++++++++++++++ datasette/static/navigation-search.js | 13 +- datasette/static/table.js | 58 +++ datasette/templates/table.html | 9 + datasette/views/table.py | 6 + tests/test_html.py | 23 +- tests/test_table_html.py | 63 +++ 8 files changed, 882 insertions(+), 7 deletions(-) create mode 100644 datasette/static/column-chooser.js diff --git a/datasette/static/app.css b/datasette/static/app.css index a7fc7fa3..4183b58e 100644 --- a/datasette/static/app.css +++ b/datasette/static/app.css @@ -63,6 +63,14 @@ em { } /* end reset */ +/* Modal CSS variables (shared by web components via Shadow DOM) */ +:root { + --modal-backdrop-bg: rgba(0, 0, 0, 0.5); + --modal-backdrop-blur: blur(4px); + --modal-border-radius: 0.75rem; + --modal-shadow: 0 20px 25px -5px rgba(0, 0, 0, 0.1), 0 10px 10px -5px rgba(0, 0, 0, 0.04); + --modal-animation-duration: 0.2s; +} body { margin: 0; @@ -795,6 +803,17 @@ p.zero-results { .filters input.filter-value { width: 140px; } + button.choose-columns-mobile { + display: inline-block; + padding: 0.5rem 1rem; + margin-bottom: 1em; + font-size: 0.9rem; + font-family: inherit; + background: white; + border: 1px solid #ccc; + border-radius: 5px; + cursor: pointer; + } } svg.dropdown-menu-icon { diff --git a/datasette/static/column-chooser.js b/datasette/static/column-chooser.js new file mode 100644 index 00000000..9680398c --- /dev/null +++ b/datasette/static/column-chooser.js @@ -0,0 +1,698 @@ +class ColumnChooser extends HTMLElement { + constructor() { + super(); + this.attachShadow({ mode: "open" }); + + // State + this._items = []; + this._checked = new Set(); + this._savedItems = null; + this._savedChecked = null; + this._onApply = null; + + // Drag state + this._ghost = null; + this._dragSrcIdx = null; + this._dropTargetIdx = null; + this._dropPosition = null; + this._ghostOffX = 0; + this._ghostOffY = 0; + this._autoScrollRAF = null; + this._lastPointerY = 0; + this._lastPointerX = 0; + this._SCROLL_ZONE = 72; + this._SCROLL_SPEED = 0.4; + + // Bound handlers + this._onMove = this._onMove.bind(this); + this._onUp = this._onUp.bind(this); + + this.shadowRoot.innerHTML = ` + + + + +
+ + +
+
+
+
+
    +
    + +
    + `; + + // DOM refs + this._dialog = this.shadowRoot.querySelector("dialog"); + this._listWrap = this.shadowRoot.getElementById("listWrap"); + this._dragList = this.shadowRoot.getElementById("dragList"); + this._pulseTop = this.shadowRoot.getElementById("pulseTop"); + this._pulseBot = this.shadowRoot.getElementById("pulseBot"); + this._selectAllBtn = this.shadowRoot.getElementById("selectAllBtn"); + this._deselectAllBtn = this.shadowRoot.getElementById("deselectAllBtn"); + this._cancelBtn = this.shadowRoot.getElementById("cancelBtn"); + this._applyBtn = this.shadowRoot.getElementById("applyBtn"); + this._countEl = this.shadowRoot.getElementById("selectedCount"); + this._footerEl = this.shadowRoot.getElementById("footerInfo"); + + // Event listeners + this._selectAllBtn.addEventListener("click", () => this._selectAll()); + this._deselectAllBtn.addEventListener("click", () => this._deselectAll()); + this._cancelBtn.addEventListener("click", () => this._close()); + this._applyBtn.addEventListener("click", () => this._apply()); + this._dialog.addEventListener("click", (e) => { + if (e.target === this._dialog) this._close(); + }); + this._dialog.addEventListener("cancel", (e) => { + e.preventDefault(); + this._close(); + }); + } + + /** + * Open the column chooser dialog. + * @param {Object} opts + * @param {string[]} opts.columns - All available column names, in display order. + * @param {string[]} opts.selected - Column names that should be pre-checked. + * @param {function(string[]): void} opts.onApply - Called with the selected columns in order when Apply is clicked. + */ + open({ columns, selected = [], onApply }) { + this._items = [...columns]; + this._checked = new Set(selected); + this._onApply = onApply || null; + + // Save state for cancel/restore + this._savedItems = [...this._items]; + this._savedChecked = new Set(this._checked); + + this._render(); + this._dialog.showModal(); + } + + // ── Internal methods ── + + _close() { + this._items = this._savedItems ? [...this._savedItems] : this._items; + this._checked = this._savedChecked + ? new Set(this._savedChecked) + : this._checked; + this._dialog.close(); + } + + _selectAll() { + this._items.forEach((col) => this._checked.add(col)); + this._dragList.querySelectorAll('input[type="checkbox"]').forEach((cb) => { + cb.checked = true; + }); + this._updateCounts(); + } + + _deselectAll() { + this._checked.clear(); + this._dragList.querySelectorAll('input[type="checkbox"]').forEach((cb) => { + cb.checked = false; + }); + this._updateCounts(); + } + + _apply() { + const selected = this._items.filter((col) => this._checked.has(col)); + this._dialog.close(); + if (this._onApply) { + this._onApply(selected); + } + } + + _render() { + this._dragList.innerHTML = ""; + this._items.forEach((col, i) => { + const li = document.createElement("li"); + li.className = "drag-item"; + li.dataset.idx = i; + li.innerHTML = ` + + + + + + + + + + + +
    + `; + + li.querySelector("input").addEventListener("change", (e) => { + e.target.checked ? this._checked.add(col) : this._checked.delete(col); + this._updateCounts(); + }); + + li.querySelector(".drag-handle").addEventListener("pointerdown", (e) => + this._startDrag(e, i), + ); + this._dragList.appendChild(li); + }); + + this._updateCounts(); + } + + _updateCounts() { + const n = this._checked.size; + this._countEl.textContent = `${n} of ${this._items.length} selected`; + this._footerEl.textContent = `${this._items.length} columns`; + } + + // ── Drag engine ── + + _startDrag(e, idx) { + e.preventDefault(); + this._dragSrcIdx = idx; + + const srcEl = this._dragList.children[idx]; + const rect = srcEl.getBoundingClientRect(); + + this._ghostOffX = e.clientX - rect.left; + this._ghostOffY = e.clientY - rect.top; + + // Build ghost inside shadow DOM + this._ghost = document.createElement("div"); + this._ghost.className = "drag-ghost"; + this._ghost.style.width = rect.width + "px"; + this._ghost.style.height = rect.height + "px"; + this._ghost.innerHTML = srcEl.innerHTML; + this._ghost.querySelector(".drop-indicator")?.remove(); + const h = this._ghost.querySelector(".drag-handle"); + if (h) h.style.color = "var(--accent)"; + this.shadowRoot.appendChild(this._ghost); + + srcEl.classList.add("is-dragging"); + this._positionGhost(e.clientX, e.clientY); + + document.addEventListener("pointermove", this._onMove); + document.addEventListener("pointerup", this._onUp); + document.addEventListener("pointercancel", this._onUp); + } + + _positionGhost(cx, cy) { + this._ghost.style.left = cx - this._ghostOffX + "px"; + this._ghost.style.top = cy - this._ghostOffY + "px"; + } + + _onMove(e) { + this._lastPointerX = e.clientX; + this._lastPointerY = e.clientY; + this._positionGhost(e.clientX, e.clientY); + this._updateDropTarget(e.clientY); + this._updateAutoScroll(e.clientY); + } + + _onUp() { + document.removeEventListener("pointermove", this._onMove); + document.removeEventListener("pointerup", this._onUp); + document.removeEventListener("pointercancel", this._onUp); + + this._stopAutoScroll(); + + const noMove = + this._dropTargetIdx === null || this._dropTargetIdx === this._dragSrcIdx; + this._clearDropIndicators(); + + let dest = null; + if (!noMove) { + const moved = this._items.splice(this._dragSrcIdx, 1)[0]; + dest = this._dropTargetIdx; + if (this._dropPosition === "after") dest++; + if (dest > this._dragSrcIdx) dest--; + this._items.splice(dest, 0, moved); + } + + this._dragSrcIdx = null; + this._dropTargetIdx = null; + this._dropPosition = null; + + const g = this._ghost; + this._ghost = null; + + if (noMove) { + if (g) g.remove(); + this._render(); + return; + } + + this._render(); + + if (g && dest !== null) { + const landedEl = this._dragList.children[dest]; + if (landedEl) { + landedEl.style.opacity = "0"; + const r = landedEl.getBoundingClientRect(); + g.getBoundingClientRect(); + g.style.transition = + "left 0.15s cubic-bezier(0.22, 1, 0.36, 1), top 0.15s cubic-bezier(0.22, 1, 0.36, 1), box-shadow 0.15s, opacity 0.1s 0.1s"; + g.style.left = r.left + "px"; + g.style.top = r.top + "px"; + g.style.boxShadow = "0 1px 4px rgba(0,0,0,0.08)"; + g.style.opacity = "0"; + setTimeout(() => { + g.remove(); + if (landedEl) landedEl.style.opacity = ""; + }, 160); + } else { + g.remove(); + } + } else if (g) { + g.remove(); + } + } + + _updateDropTarget(clientY) { + this._clearDropIndicators(); + const listItems = [ + ...this._dragList.querySelectorAll(".drag-item:not(.is-dragging)"), + ]; + if (!listItems.length) return; + + let best = null, + bestDist = Infinity; + listItems.forEach((li) => { + const r = li.getBoundingClientRect(); + const mid = r.top + r.height / 2; + const dist = Math.abs(clientY - mid); + if (dist < bestDist) { + bestDist = dist; + best = li; + } + }); + + if (!best) return; + const r = best.getBoundingClientRect(); + const mid = r.top + r.height / 2; + const above = clientY < mid; + const indic = best.querySelector(".drop-indicator"); + + this._dropTargetIdx = parseInt(best.dataset.idx); + this._dropPosition = above ? "before" : "after"; + + if (indic) { + indic.className = "drop-indicator " + (above ? "top" : "bottom"); + } + } + + _clearDropIndicators() { + this._dragList.querySelectorAll(".drop-indicator").forEach((el) => { + el.className = "drop-indicator"; + }); + } + + _updateAutoScroll(clientY) { + const rect = this._listWrap.getBoundingClientRect(); + const relY = clientY - rect.top; + const distTop = relY; + const distBot = rect.height - relY; + + const inTop = distTop < this._SCROLL_ZONE && distTop >= 0; + const inBot = distBot < this._SCROLL_ZONE && distBot >= 0; + + this._pulseTop.classList.toggle("active", inTop); + this._pulseBot.classList.toggle("active", inBot); + + if ((inTop || inBot) && !this._autoScrollRAF) { + let lastTime = null; + const loop = (ts) => { + if (!this._ghost) { + this._stopAutoScroll(); + return; + } + if (lastTime !== null) { + const dt = ts - lastTime; + const rect2 = this._listWrap.getBoundingClientRect(); + const relY2 = this._lastPointerY - rect2.top; + const dTop = relY2; + const dBot = rect2.height - relY2; + + if (dTop < this._SCROLL_ZONE && dTop >= 0) { + const factor = 1 - dTop / this._SCROLL_ZONE; + this._listWrap.scrollTop -= this._SCROLL_SPEED * dt * factor * 2.5; + } else if (dBot < this._SCROLL_ZONE && dBot >= 0) { + const factor = 1 - dBot / this._SCROLL_ZONE; + this._listWrap.scrollTop += this._SCROLL_SPEED * dt * factor * 2.5; + } else { + this._stopAutoScroll(); + return; + } + this._updateDropTarget(this._lastPointerY); + } + lastTime = ts; + this._autoScrollRAF = requestAnimationFrame(loop); + }; + this._autoScrollRAF = requestAnimationFrame(loop); + } + + if (!inTop && !inBot) this._stopAutoScroll(); + } + + _stopAutoScroll() { + if (this._autoScrollRAF) { + cancelAnimationFrame(this._autoScrollRAF); + this._autoScrollRAF = null; + } + this._pulseTop.classList.remove("active"); + this._pulseBot.classList.remove("active"); + } +} + +customElements.define("column-chooser", ColumnChooser); diff --git a/datasette/static/navigation-search.js b/datasette/static/navigation-search.js index 48de5c4f..95e7dfc5 100644 --- a/datasette/static/navigation-search.js +++ b/datasette/static/navigation-search.js @@ -19,19 +19,20 @@ class NavigationSearch extends HTMLElement { dialog { border: none; - border-radius: 0.75rem; + border-radius: var(--modal-border-radius, 0.75rem); padding: 0; max-width: 90vw; width: 600px; max-height: 80vh; - box-shadow: 0 20px 25px -5px rgba(0, 0, 0, 0.1), 0 10px 10px -5px rgba(0, 0, 0, 0.04); - animation: slideIn 0.2s ease-out; + box-shadow: var(--modal-shadow, 0 20px 25px -5px rgba(0, 0, 0, 0.1), 0 10px 10px -5px rgba(0, 0, 0, 0.04)); + animation: slideIn var(--modal-animation-duration, 0.2s) ease-out; } dialog::backdrop { - background: rgba(0, 0, 0, 0.5); - backdrop-filter: blur(4px); - animation: fadeIn 0.2s ease-out; + background: var(--modal-backdrop-bg, rgba(0, 0, 0, 0.5)); + backdrop-filter: var(--modal-backdrop-blur, blur(4px)); + -webkit-backdrop-filter: var(--modal-backdrop-blur, blur(4px)); + animation: fadeIn var(--modal-animation-duration, 0.2s) ease-out; } @keyframes slideIn { diff --git a/datasette/static/table.js b/datasette/static/table.js index 0caeeb91..c26dda5a 100644 --- a/datasette/static/table.js +++ b/datasette/static/table.js @@ -4,6 +4,7 @@ var DROPDOWN_HTML = `