From 86dec9e8fffd6c4efec928ae9b5713748dec7e74 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Sat, 6 Jun 2020 22:30:36 -0700 Subject: [PATCH] Added permission check to every view, closes #808 --- datasette/app.py | 5 ++ datasette/templates/permissions_debug.html | 2 +- datasette/utils/asgi.py | 4 + datasette/views/base.py | 14 ++++ datasette/views/database.py | 8 ++ datasette/views/index.py | 1 + datasette/views/table.py | 5 ++ docs/authentication.rst | 88 ++++++++++++++++++++++ tests/conftest.py | 38 ++++++++++ tests/fixtures.py | 16 ++++ tests/test_api.py | 2 +- tests/test_auth.py | 1 + tests/test_html.py | 38 ++++++++++ 13 files changed, 220 insertions(+), 2 deletions(-) diff --git a/datasette/app.py b/datasette/app.py index 1624f6ea..f433a10a 100644 --- a/datasette/app.py +++ b/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 diff --git a/datasette/templates/permissions_debug.html b/datasette/templates/permissions_debug.html index fb098c5c..dda57dfa 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 ba131dc8..fa78c8df 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 315c96fe..9c2cbbcc 100644 --- a/datasette/views/base.py +++ b/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") diff --git a/datasette/views/database.py b/datasette/views/database.py index 4e9a6da7..eb7c29ca 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 @@ 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") ): diff --git a/datasette/views/index.py b/datasette/views/index.py index fe88a38c..40c41002 100644 --- a/datasette/views/index.py +++ b/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() diff --git a/datasette/views/table.py b/datasette/views/table.py index ec1b6c7c..32c7f839 100644 --- a/datasette/views/table.py +++ b/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 diff --git a/docs/authentication.rst b/docs/authentication.rst index fd70000e..b0473ee8 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 a19ad18d..1921ae3a 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 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 + ) diff --git a/tests/fixtures.py b/tests/fixtures.py index 75bd6b94..d175dfd5 100644 --- a/tests/fixtures.py +++ b/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 + ) diff --git a/tests/test_api.py b/tests/test_api.py index b35c0a2d..555e394a 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 ac8d7abe..40dc2587 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 2d2a141a..3569b92c 100644 --- a/tests/test_html.py +++ b/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