Fable review of JSON API consistency and subsequent improvements

Merge PR #2824

Author: simonw

datasette/__init__.py

@@ -1,7 +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.tokens import TokenHandler, TokenInvalid, TokenRestrictions  # noqa
 from datasette.utils.asgi import (  # noqa
     Forbidden,
     NotFound,

datasette/app.py

@@ -111,6 +111,7 @@
     baseconv,
     call_with_supported_arguments,
     detect_json1,
+    add_cors_headers,
     display_actor,
     escape_css_string,
     escape_sqlite,
@@ -130,6 +131,7 @@
     redact_keys,
     row_sql_params_pks,
 )
+from .tokens import TokenInvalid
 from .utils.asgi import (
     AsgiLifespan,
     Forbidden,
@@ -910,7 +912,9 @@ 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.
+        token, or None if no handler accepts it. A handler may raise
+        TokenInvalid for a token it recognizes but rejects (bad signature,
+        expired) - Datasette turns that into a 401 response.
         """
         for token_handler in self._token_handlers():
             result = await token_handler.verify_token(self, token)
@@ -2173,6 +2177,18 @@ def _connected_databases(self):
             for name, d in self.databases.items()
         ]
 
+    async def _connected_databases_for_actor(self, actor):
+        page = await self.allowed_resources("view-database", actor)
+        allowed_names = {resource.parent async for resource in page.all()}
+        return [
+            database
+            for database in self._connected_databases()
+            if database["name"] in allowed_names
+        ]
+
+    async def _databases_data(self, request):
+        return {"databases": await self._connected_databases_for_actor(request.actor)}
+
     def _versions(self):
         conn = sqlite3.connect(":memory:")
         self._prepare_connection(conn, "_memory")
@@ -2519,8 +2535,8 @@ def _routes(self):
         def add_route(view, regex):
             routes.append((regex, view))
 
-        add_route(IndexView.as_view(self), r"/(\.(?P<format>jsono?))?$")
-        add_route(IndexView.as_view(self), r"/-/(\.(?P<format>jsono?))?$")
+        add_route(IndexView.as_view(self), r"/(\.(?P<format>json))?$")
+        add_route(IndexView.as_view(self), r"/-/(\.(?P<format>json))?$")
         add_route(permanent_redirect("/-/"), r"/-$")
         add_route(favicon, "/favicon.ico")
 
@@ -2556,7 +2572,10 @@ def add_route(view, regex):
         )
         add_route(
             JsonDataView.as_view(
-                self, "plugins.json", self._plugins, needs_request=True
+                self,
+                "plugins.json",
+                lambda request: {"plugins": self._plugins(request)},
+                needs_request=True,
             ),
             r"/-/plugins(\.(?P<format>json))?$",
         )
@@ -2569,11 +2588,18 @@ def add_route(view, regex):
             r"/-/config(\.(?P<format>json))?$",
         )
         add_route(
-            JsonDataView.as_view(self, "threads.json", self._threads),
+            JsonDataView.as_view(
+                self, "threads.json", self._threads, permission="permissions-debug"
+            ),
             r"/-/threads(\.(?P<format>json))?$",
         )
         add_route(
-            JsonDataView.as_view(self, "databases.json", self._connected_databases),
+            JsonDataView.as_view(
+                self,
+                "databases.json",
+                self._databases_data,
+                needs_request=True,
+            ),
             r"/-/databases(\.(?P<format>json))?$",
         )
         add_route(
@@ -2586,7 +2612,7 @@ def add_route(view, regex):
             JsonDataView.as_view(
                 self,
                 "actions.json",
-                self._actions,
+                lambda: {"actions": self._actions()},
                 template="debug_actions.html",
                 permission="permissions-debug",
             ),
@@ -2876,13 +2902,24 @@ async def route_path(self, scope, receive, send, path):
         # Handle authentication
         default_actor = scope.get("actor") or None
         actor = None
+        token_error = None
         results = pm.hook.actor_from_request(datasette=self.ds, request=request)
         for result in results:
-            result = await await_me_maybe(result)
+            try:
+                result = await await_me_maybe(result)
+            except TokenInvalid as ex:
+                # A presented token was recognized but rejected - fail the
+                # request with a 401 even if another credential is valid,
+                # but keep awaiting the remaining coroutines first
+                if token_error is None:
+                    token_error = ex
+                continue
             if result and actor is None:
                 actor = result
                 # Don't break — we must await all coroutines to avoid
                 # "coroutine was never awaited" warnings
+        if token_error is not None:
+            return await self.handle_401(request, send, token_error)
         scope_modifications["actor"] = actor or default_actor
         scope = dict(scope, **scope_modifications)
 
@@ -2914,6 +2951,15 @@ async def route_path(self, scope, receive, send, path):
         except Exception as exception:
             return await self.handle_exception(request, send, exception)
 
+    async def handle_401(self, request, send, exception):
+        # A presented bearer token was recognized by a handler but rejected.
+        # Bearer tokens are API credentials, so this is always JSON.
+        headers = {"www-authenticate": 'Bearer error="invalid_token"'}
+        if self.ds.cors:
+            add_cors_headers(headers)
+        response = Response.error([str(exception)], 401, headers=headers)
+        await response.asgi_send(send)
+
     async def handle_404(self, request, send, exception=None):
         # If path contains % encoding, redirect to tilde encoding
         if "%" in request.path:

datasette/extras.py

@@ -5,6 +5,8 @@
 
 from asyncinject import Registry
 
+from datasette.utils.asgi import BadRequest
+
 
 def extra_names_from_request(request):
     extra_bits = request.args.getlist("_extra")
@@ -113,6 +115,17 @@ def _allowed_names_for_scope(self, scope, include_internal):
             self._allowed_names[key] = names
         return names
 
+    def validate_requested(self, requested, scope):
+        """
+        Raise BadRequest if any requested extra name is not a public extra
+        for this scope. Used by data formats such as .json - HTML pages
+        silently ignore unknown names instead.
+        """
+        allowed = self._allowed_names_for_scope(scope, include_internal=False)
+        unknown = sorted(name for name in requested if name not in allowed)
+        if unknown:
+            raise BadRequest("Unknown _extra: {}".format(", ".join(unknown)))
+
     async def resolve(self, requested, context, scope, include_internal=False):
         allowed_names = self._allowed_names_for_scope(scope, include_internal)
         requested_names = [name for name in requested if name in allowed_names]

datasette/forbidden.py

@@ -1,9 +1,19 @@
 from datasette import hookimpl, Response
+from .utils import add_cors_headers
 
 
 @hookimpl(trylast=True)
 def forbidden(datasette, request, message):
     async def inner():
+        if (
+            request.path.split("?")[0].endswith(".json")
+            or "application/json" in (request.headers.get("accept") or "")
+            or request.headers.get("content-type") == "application/json"
+        ):
+            headers = {}
+            if datasette.cors:
+                add_cors_headers(headers)
+            return Response.error(message, 403, headers=headers)
         return Response.html(
             await datasette.render_template(
                 "error.html",

datasette/handle_exception.py

@@ -1,5 +1,5 @@
 from datasette import hookimpl, Response
-from .utils import add_cors_headers
+from .utils import add_cors_headers, error_body
 from .utils.asgi import (
     Base400,
 )
@@ -28,6 +28,7 @@ async def inner():
             rich.get_console().print_exception(show_locals=True)
 
         title = None
+        plain_message = None
         if isinstance(exception, Base400):
             status = exception.status
             info = {}
@@ -36,6 +37,7 @@ async def inner():
             status = exception.status
             info = exception.error_dict
             message = exception.message
+            plain_message = exception.plain_message
             if exception.message_is_html:
                 message = Markup(message)
             title = exception.title
@@ -45,6 +47,13 @@ async def inner():
             message = str(exception)
             traceback.print_exc()
         templates = [f"{status}.html", "error.html"]
+        headers = {}
+        if datasette.cors:
+            add_cors_headers(headers)
+        if request.path.split("?")[0].endswith(".json"):
+            body = dict(info)
+            body.update(error_body(plain_message or message, status))
+            return Response.json(body, status=status, headers=headers)
         info.update(
             {
                 "ok": False,
@@ -53,24 +62,18 @@ async def inner():
                 "title": title,
             }
         )
-        headers = {}
-        if datasette.cors:
-            add_cors_headers(headers)
-        if request.path.split("?")[0].endswith(".json"):
-            return Response.json(info, status=status, headers=headers)
-        else:
-            environment = datasette.get_jinja_environment(request)
-            template = environment.select_template(templates)
-            return Response.html(
-                await template.render_async(
-                    dict(
-                        info,
-                        urls=datasette.urls,
-                        menu_links=lambda: [],
-                    )
-                ),
-                status=status,
-                headers=headers,
-            )
+        environment = datasette.get_jinja_environment(request)
+        template = environment.select_template(templates)
+        return Response.html(
+            await template.render_async(
+                dict(
+                    info,
+                    urls=datasette.urls,
+                    menu_links=lambda: [],
+                )
+            ),
+            status=status,
+            headers=headers,
+        )
 
     return inner

datasette/renderer.py

@@ -1,6 +1,7 @@
 import json
 from datasette.extras import extra_names_from_request
 from datasette.utils import (
+    error_body,
     value_as_boolean,
     remove_infinites,
     CustomJSONEncoder,
@@ -52,8 +53,7 @@ def json_renderer(request, args, data, error, truncated=None):
     if error:
         shape = "objects"
         status_code = 400
-        data["error"] = error
-        data["ok"] = False
+        data.update(error_body(error, status_code))
 
     if truncated is not None:
         data["truncated"] = truncated
@@ -87,7 +87,8 @@ def json_renderer(request, args, data, error, truncated=None):
                         object_rows[pk_string] = row
                     data = object_rows
             if shape_error:
-                data = {"ok": False, "error": shape_error}
+                status_code = 400
+                data = error_body(shape_error, status_code)
         elif shape == "array":
             data = data["rows"]
 
@@ -100,12 +101,7 @@ def json_renderer(request, args, data, error, truncated=None):
             data["rows"] = [list(row.values()) for row in data["rows"]]
     else:
         status_code = 400
-        data = {
-            "ok": False,
-            "error": f"Invalid _shape: {shape}",
-            "status": 400,
-            "title": None,
-        }
+        data = error_body(f"Invalid _shape: {shape}", status_code)
 
     # Don't include "columns" in output
     # https://github.com/simonw/datasette/issues/2136

datasette/stored_queries.py

@@ -62,7 +62,6 @@ def stored_query_to_dict(query: StoredQuery) -> dict[str, Any]:
         "description_html": query.description_html,
         "hide_sql": query.hide_sql,
         "fragment": query.fragment,
-        "params": list(query.parameters),
         "parameters": list(query.parameters),
         "is_write": query.is_write,
         "is_private": query.is_private,
@@ -84,7 +83,6 @@ def stored_query_page_to_dict(page: StoredQueryPage) -> dict[str, Any]:
     return {
         "queries": [stored_query_to_dict(query) for query in page.queries],
         "next": page.next,
-        "has_more": page.has_more,
         "limit": page.limit,
     }
 

datasette/templates/debug_actions.html

@@ -9,7 +9,7 @@ <h1>Registered actions</h1>
 {% include "_permissions_debug_tabs.html" %}
 
 <p style="margin-bottom: 2em;">
-  This Datasette instance has registered {{ data|length }} action{{ data|length != 1 and "s" or "" }}.
+  This Datasette instance has registered {{ data.actions|length }} action{{ data.actions|length != 1 and "s" or "" }}.
   Actions are used by the permission system to control access to different features.
 </p>
 
@@ -26,7 +26,7 @@ <h1>Registered actions</h1>
     </tr>
   </thead>
   <tbody>
-    {% for action in data %}
+    {% for action in data.actions %}
     <tr>
       <td><strong>{{ action.name }}</strong></td>
       <td>{% if action.abbr %}<code>{{ action.abbr }}</code>{% endif %}</td>

datasette/templates/debug_allowed.html

@@ -49,7 +49,7 @@ <h1>Allowed resources</h1>
 
         <div class="form-section">
             <label for="page_size">Page size:</label>
-            <input type="number" id="page_size" name="page_size" value="50" min="1" max="200" style="max-width: 100px;">
+            <input type="number" id="page_size" name="_size" value="50" min="1" max="200" style="max-width: 100px;">
             <small>Number of results per page (max 200)</small>
         </div>
 
@@ -88,7 +88,7 @@ <h2>Results</h2>
 (function() {
     const params = populateFormFromURL();
     const action = params.get('action');
-    const page = params.get('page');
+    const page = params.get('_page');
     if (action) {
         fetchResults(page ? parseInt(page) : 1);
     }
@@ -102,14 +102,14 @@ <h2>Results</h2>
     const params = new URLSearchParams();
 
     for (const [key, value] of formData.entries()) {
-        if (value && key !== 'page_size') {
+        if (value && key !== '_size' && key !== '_page') {
             params.append(key, value);
         }
     }
 
     const pageSize = document.getElementById('page_size').value || '50';
-    params.append('page', page.toString());
-    params.append('page_size', pageSize);
+    params.append('_page', page.toString());
+    params.append('_size', pageSize);
 
     try {
         const response = await fetch('{{ urls.path("-/allowed.json") }}?' + params.toString(), {