diff --git a/datasette/app.py b/datasette/app.py index 1624f6eacf..f433a10ac5 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -49,6 +49,7 @@ ) from .utils.asgi import ( AsgiLifespan, + Forbidden, NotFound, Request, Response, @@ -1003,6 +1004,10 @@ async def handle_500(self, scope, receive, send, exception): 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 diff --git a/datasette/templates/permissions_debug.html b/datasette/templates/permissions_debug.html index fb098c5c7f..dda57dfa6f 100644 --- a/datasette/templates/permissions_debug.html +++ b/datasette/templates/permissions_debug.html @@ -47,7 +47,7 @@

Actor: {{ check.actor|tojson }}

{% if check.resource_type %} -

Resource: {{ check.resource_type }}: {{ check.resource_identifier }}

+

Resource: {{ check.resource_type }} = {{ check.resource_identifier }}

{% endif %} {% endfor %} diff --git a/datasette/utils/asgi.py b/datasette/utils/asgi.py index ba131dc8be..fa78c8df87 100644 --- a/datasette/utils/asgi.py +++ b/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 diff --git a/datasette/views/base.py b/datasette/views/base.py index 315c96fe72..9c2cbbcc9d 100644 --- a/datasette/views/base.py +++ b/datasette/views/base.py @@ -29,6 +29,7 @@ AsgiWriter, AsgiRouter, AsgiView, + Forbidden, NotFound, Response, ) @@ -63,6 +64,19 @@ async def head(self, *args, **kwargs): 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") diff --git a/datasette/views/database.py b/datasette/views/database.py index 4e9a6da7e6..eb7c29ca9f 100644 --- a/datasette/views/database.py +++ b/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 @@ async def data( # 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") ): diff --git a/datasette/views/index.py b/datasette/views/index.py index fe88a38c27..40c41002e6 100644 --- a/datasette/views/index.py +++ b/datasette/views/index.py @@ -22,6 +22,7 @@ def __init__(self, datasette): 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() diff --git a/datasette/views/table.py b/datasette/views/table.py index ec1b6c7c24..32c7f8394e 100644 --- a/datasette/views/table.py +++ b/datasette/views/table.py @@ -267,6 +267,8 @@ async def data( 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 diff --git a/docs/authentication.rst b/docs/authentication.rst index fd70000ed8..b0473ee80c 100644 --- a/docs/authentication.rst +++ b/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 ` 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. diff --git a/tests/conftest.py b/tests/conftest.py index a19ad18db6..1921ae3a56 100644 --- a/tests/conftest.py +++ b/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 return_to_previous(): 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 + ) diff --git a/tests/fixtures.py b/tests/fixtures.py index 75bd6b94bd..d175dfd544 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -840,3 +840,19 @@ def generate_sortable_rows(num): 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 + ) diff --git a/tests/test_api.py b/tests/test_api.py index b35c0a2da3..555e394ae0 100644 --- a/tests/test_api.py +++ b/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( diff --git a/tests/test_auth.py b/tests/test_auth.py index ac8d7abec2..40dc2587c8 100644 --- a/tests/test_auth.py +++ b/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") diff --git a/tests/test_html.py b/tests/test_html.py index 2d2a141a52..3569b92c70 100644 --- a/tests/test_html.py +++ b/tests/test_html.py @@ -4,6 +4,7 @@ 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 @@ 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