Added permission check to every view, closes #808

datasette/app.py

@@ -49,6 +49,7 @@ from .utils import (
 )
 from .utils.asgi import (
     AsgiLifespan,
+    Forbidden,
     NotFound,
     Request,
     Response,
@@ -1003,6 +1004,10 @@ class DatasetteRouter(AsgiRouter):
             status = 404
             info = {}
             message = exception.args[0]
+        elif isinstance(exception, Forbidden):
+            status = 403
+            info = {}
+            message = exception.args[0]
         elif isinstance(exception, DatasetteError):
             status = exception.status
             info = exception.error_dict

datasette/templates/permissions_debug.html

@@ -47,7 +47,7 @@
         </h2>
         <p><strong>Actor:</strong> {{ check.actor|tojson }}</p>
         {% if check.resource_type %}
-            <p><strong>Resource:</strong> {{ check.resource_type }}: {{ check.resource_identifier }}</p>
+            <p><strong>Resource:</strong> {{ check.resource_type }} = {{ check.resource_identifier }}</p>
         {% endif %}
     </div>
 {% endfor %}

datasette/utils/asgi.py

@@ -13,6 +13,10 @@ class NotFound(Exception):
     pass
 
 
+class Forbidden(Exception):
+    pass
+
+
 class Request:
     def __init__(self, scope, receive):
         self.scope = scope

datasette/views/base.py

@@ -29,6 +29,7 @@ from datasette.utils.asgi import (
     AsgiWriter,
     AsgiRouter,
     AsgiView,
+    Forbidden,
     NotFound,
     Response,
 )
@@ -63,6 +64,19 @@ class BaseView(AsgiView):
         response.body = b""
         return response
 
+    async def check_permission(
+        self, request, action, resource_type=None, resource_identifier=None
+    ):
+        ok = await self.ds.permission_allowed(
+            request.scope.get("actor"),
+            action,
+            resource_type=resource_type,
+            resource_identifier=resource_identifier,
+            default=True,
+        )
+        if not ok:
+            raise Forbidden(action)
+
     def database_url(self, database):
         db = self.ds.databases[database]
         base_url = self.ds.config("base_url")

datasette/views/database.py

@@ -19,6 +19,7 @@ class DatabaseView(DataView):
     name = "database"
 
     async def data(self, request, database, hash, default_labels=False, _size=None):
+        await self.check_permission(request, "view-database", "database", database)
         metadata = (self.ds.metadata("databases") or {}).get(database, {})
         self.ds.update_with_inherited_metadata(metadata)
 
@@ -89,6 +90,9 @@ class DatabaseDownload(DataView):
     name = "database_download"
 
     async def view_get(self, request, database, hash, correct_hash_present, **kwargs):
+        await self.check_permission(
+            request, "view-database-download", "database", database
+        )
         if database not in self.ds.databases:
             raise DatasetteError("Invalid database", status=404)
         db = self.ds.databases[database]
@@ -128,6 +132,10 @@ class QueryView(DataView):
 
         # Respect canned query permissions
         if canned_query:
+            await self.check_permission(
+                request, "view-query", "query", (database, canned_query)
+            )
+            # TODO: fix this to use that permission check
             if not actor_matches_allow(
                 request.scope.get("actor", None), metadata.get("allow")
             ):

datasette/views/index.py

@@ -22,6 +22,7 @@ class IndexView(BaseView):
         self.ds = datasette
 
     async def get(self, request, as_format):
+        await self.check_permission(request, "view-index")
         databases = []
         for name, db in self.ds.databases.items():
             table_names = await db.table_names()

datasette/views/table.py

@@ -267,6 +267,8 @@ class TableView(RowTableShared):
         if not is_view and not table_exists:
             raise NotFound("Table not found: {}".format(table))
 
+        await self.check_permission(request, "view-table", "table", (database, table))
+
         pks = await db.primary_keys(table)
         table_columns = await db.table_columns(table)
 
@@ -844,6 +846,9 @@ class RowView(RowTableShared):
 
     async def data(self, request, database, hash, table, pk_path, default_labels=False):
         pk_values = urlsafe_components(pk_path)
+        await self.check_permission(
+            request, "view-row", "row", tuple([database, table] + list(pk_values))
+        )
         db = self.ds.databases[database]
         pks = await db.primary_keys(table)
         use_rowid = not pks

docs/authentication.rst

@@ -150,3 +150,91 @@ The debug tool at ``/-/permissions`` is only available to the :ref:`authenticate
 It shows the thirty most recent permission checks that have been carried out by the Datasette instance.
 
 This is designed to help administrators and plugin authors understand exactly how permission checks are being carried out, in order to effectively configure Datasette's permission system.
+
+
+.. _permissions:
+
+Permissions
+===========
+
+This section lists all of the permission checks that are carried out by Datasette core, along with their ``resource_type`` and ``resource_identifier`` if those are passed.
+
+.. _permissions_view_index:
+
+view-index
+----------
+
+Actor is allowed to view the index page, e.g. https://latest.datasette.io/
+
+
+.. _permissions_view_database:
+
+view-database
+-------------
+
+Actor is allowed to view a database page, e.g. https://latest.datasette.io/fixtures
+
+``resource_type`` - string
+    "database"
+
+``resource_identifier`` - string
+    The name of the database
+
+.. _permissions_view_database_download:
+
+view-database-download
+-----------------------
+
+Actor is allowed to download a database, e.g. https://latest.datasette.io/fixtures.db
+
+``resource_type`` - string
+    "database"
+
+``resource_identifier`` - string
+    The name of the database
+
+.. _permissions_view_table:
+
+view-table
+----------
+
+Actor is allowed to view a table (or view) page, e.g. https://latest.datasette.io/fixtures/complex_foreign_keys
+
+``resource_type`` - string
+    "table" - even if this is actually a SQL view
+
+``resource_identifier`` - tuple: (string, string)
+    The name of the database, then the name of the table
+
+.. _permissions_view_row:
+
+view-row
+--------
+
+Actor is allowed to view a row page, e.g. https://latest.datasette.io/fixtures/compound_primary_key/a,b
+
+``resource_type`` - string
+    "row"
+
+``resource_identifier`` - tuple: (string, string, strings...)
+    The name of the database, then the name of the table, then the primary key of the row. The primary key may be a single value or multiple values, so the ``resource_identifier`` tuple may be three or more items long.
+
+.. _permissions_view_query:
+
+view-query
+----------
+
+Actor is allowed to view a :ref:`canned query <canned_queries>` page, e.g. https://latest.datasette.io/fixtures/pragma_cache_size
+
+``resource_type`` - string
+    "query"
+
+``resource_identifier`` - string
+    The name of the canned query
+
+.. _permissions_permissions_debug:
+
+permissions-debug
+-----------------
+
+Actor is allowed to view the ``/-/permissions`` debug page.

tests/conftest.py

@@ -1,5 +1,15 @@
 import os
+import pathlib
 import pytest
+import re
+
+UNDOCUMENTED_PERMISSIONS = {
+    "this_is_allowed",
+    "this_is_denied",
+    "this_is_allowed_async",
+    "this_is_denied_async",
+    "no_match",
+}
 
 
 def pytest_configure(config):
@@ -39,3 +49,31 @@ def restore_working_directory(tmpdir, request):
         os.chdir(previous_cwd)
 
     request.addfinalizer(return_to_previous)
+
+
+@pytest.fixture(scope="session", autouse=True)
+def check_permission_actions_are_documented():
+    from datasette.plugins import pm
+
+    content = (
+        (pathlib.Path(__file__).parent.parent / "docs" / "authentication.rst")
+        .open()
+        .read()
+    )
+    permissions_re = re.compile(r"\.\. _permissions_([^\s:]+):")
+    documented_permission_actions = set(permissions_re.findall(content)).union(
+        UNDOCUMENTED_PERMISSIONS
+    )
+
+    def before(hook_name, hook_impls, kwargs):
+        if hook_name == "permission_allowed":
+            action = kwargs.get("action").replace("-", "_")
+            assert (
+                action in documented_permission_actions
+            ), "Undocumented permission action: {}, resource_type: {}, resource_identifier: {}".format(
+                action, kwargs["resource_type"], kwargs["resource_identifier"]
+            )
+
+    pm.add_hookcall_monitoring(
+        before=before, after=lambda outcome, hook_name, hook_impls, kwargs: None
+    )

tests/fixtures.py

@@ -840,3 +840,19 @@ if __name__ == "__main__":
                 sys.argv[0]
             )
         )
+
+
+def assert_permission_checked(
+    datasette, action, resource_type=None, resource_identifier=None
+):
+    assert [
+        pc
+        for pc in datasette._permission_checks
+        if pc["action"] == action
+        and pc["resource_type"] == resource_type
+        and pc["resource_identifier"] == resource_identifier
+    ], """Missing expected permission check: action={}, resource_type={}, resource_identifier={}
+    Permission checks seen: {}
+    """.format(
+        action, resource_type, resource_identifier, datasette._permission_checks
+    )

tests/test_api.py

@@ -1721,7 +1721,7 @@ def test_trace(app_client):
         assert isinstance(trace["traceback"], list)
         assert isinstance(trace["database"], str)
         assert isinstance(trace["sql"], str)
-        assert isinstance(trace["params"], (list, dict))
+        assert isinstance(trace["params"], (list, dict, None.__class__))
 
 
 @pytest.mark.parametrize(

tests/test_auth.py

@@ -23,6 +23,7 @@ def test_actor_cookie(app_client):
 
 
 def test_permissions_debug(app_client):
+    app_client.ds._permission_checks.clear()
     assert 403 == app_client.get("/-/permissions").status
     # With the cookie it should work
     cookie = app_client.ds.sign({"id": "root"}, "actor")

tests/test_html.py

@@ -4,6 +4,7 @@ from .fixtures import (  # noqa
     app_client_shorter_time_limit,
     app_client_two_attached_databases,
     app_client_with_hash,
+    assert_permission_checked,
     make_app_client,
     METADATA,
 )
@@ -17,6 +18,7 @@ import urllib.parse
 
 def test_homepage(app_client_two_attached_databases):
     response = app_client_two_attached_databases.get("/")
+    assert_permission_checked(app_client_two_attached_databases.ds, "view-index")
     assert response.status == 200
     assert "text/html; charset=utf-8" == response.headers["content-type"]
     soup = Soup(response.body, "html.parser")
@@ -75,6 +77,12 @@ def test_static_mounts():
 def test_memory_database_page():
     for client in make_app_client(memory=True):
         response = client.get("/:memory:")
+        assert_permission_checked(
+            client.ds,
+            "view-database",
+            resource_type="database",
+            resource_identifier=":memory:",
+        )
         assert response.status == 200
 
 
@@ -87,6 +95,12 @@ def test_database_page_redirects_with_url_hash(app_client_with_hash):
 
 def test_database_page(app_client):
     response = app_client.get("/fixtures")
+    assert_permission_checked(
+        app_client.ds,
+        "view-database",
+        resource_type="database",
+        resource_identifier="fixtures",
+    )
     soup = Soup(response.body, "html.parser")
     queries_ul = soup.find("h2", text="Queries").find_next_sibling("ul")
     assert queries_ul is not None
@@ -197,6 +211,12 @@ def test_row_page_does_not_truncate():
     for client in make_app_client(config={"truncate_cells_html": 5}):
         response = client.get("/fixtures/facetable/1")
         assert response.status == 200
+        assert_permission_checked(
+            client.ds,
+            "view-row",
+            resource_type="row",
+            resource_identifier=("fixtures", "facetable", "1"),
+        )
         table = Soup(response.body, "html.parser").find("table")
         assert table["class"] == ["rows-and-columns"]
         assert ["Mission"] == [
@@ -506,6 +526,12 @@ def test_templates_considered(app_client, path, expected_considered):
 
 def test_table_html_simple_primary_key(app_client):
     response = app_client.get("/fixtures/simple_primary_key?_size=3")
+    assert_permission_checked(
+        app_client.ds,
+        "view-table",
+        resource_type="table",
+        resource_identifier=("fixtures", "simple_primary_key"),
+    )
     assert response.status == 200
     table = Soup(response.body, "html.parser").find("table")
     assert table["class"] == ["rows-and-columns"]
@@ -896,6 +922,12 @@ def test_database_download_allowed_for_immutable():
         assert len(soup.findAll("a", {"href": re.compile(r"\.db$")}))
         # Check we can actually download it
         assert 200 == client.get("/fixtures.db").status
+        assert_permission_checked(
+            client.ds,
+            "view-database-download",
+            resource_type="database",
+            resource_identifier="fixtures",
+        )
 
 
 def test_database_download_disallowed_for_mutable(app_client):
@@ -991,6 +1023,12 @@ def test_404_content_type(app_client):
 
 def test_canned_query_with_custom_metadata(app_client):
     response = app_client.get("/fixtures/neighborhood_search?text=town")
+    assert_permission_checked(
+        app_client.ds,
+        "view-query",
+        resource_type="query",
+        resource_identifier=("fixtures", "neighborhood_search"),
+    )
     assert response.status == 200
     soup = Soup(response.body, "html.parser")
     assert "Search neighborhoods" == soup.find("h1").text