From 4340845754e90fe778a7da8668b4fd9bf6ccc2c6 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Sun, 7 Jun 2020 13:03:08 -0700 Subject: [PATCH] Nested permission checks for all views, refs #811 --- datasette/views/database.py | 10 +++++- datasette/views/index.py | 2 +- datasette/views/table.py | 5 +++ docs/authentication.rst | 21 ++++++++--- tests/fixtures.py | 36 +++++++++++-------- tests/test_html.py | 71 ++++++++++++++++++++++--------------- 6 files changed, 97 insertions(+), 48 deletions(-) diff --git a/datasette/views/database.py b/datasette/views/database.py index eb7c29ca9f..4eae9e33dc 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-instance") await self.check_permission(request, "view-database", "database", database) metadata = (self.ds.metadata("databases") or {}).get(database, {}) self.ds.update_with_inherited_metadata(metadata) @@ -90,6 +91,8 @@ class DatabaseDownload(DataView): name = "database_download" async def view_get(self, request, database, hash, correct_hash_present, **kwargs): + await self.check_permission(request, "view-instance") + await self.check_permission(request, "view-database", "database", database) await self.check_permission( request, "view-database-download", "database", database ) @@ -132,6 +135,8 @@ async def data( # Respect canned query permissions if canned_query: + await self.check_permission(request, "view-instance") + await self.check_permission(request, "view-database", "database", database) await self.check_permission( request, "view-query", "query", (database, canned_query) ) @@ -140,7 +145,10 @@ async def data( request.scope.get("actor", None), metadata.get("allow") ): return Response("Permission denied", status=403) - + else: + await self.check_permission(request, "view-instance") + await self.check_permission(request, "view-database", "database", database) + await self.check_permission(request, "execute-query", "database", database) # Extract any :named parameters named_parameters = named_parameters or self.re_named_parameter.findall(sql) named_parameter_values = { diff --git a/datasette/views/index.py b/datasette/views/index.py index 40c41002e6..5f903474f1 100644 --- a/datasette/views/index.py +++ b/datasette/views/index.py @@ -22,7 +22,7 @@ def __init__(self, datasette): self.ds = datasette async def get(self, request, as_format): - await self.check_permission(request, "view-index") + await self.check_permission(request, "view-instance") 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 32c7f8394e..10d6725a0f 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-instance") + await self.check_permission(request, "view-database", "database", database) await self.check_permission(request, "view-table", "table", (database, table)) pks = await db.primary_keys(table) @@ -846,6 +848,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-instance") + await self.check_permission(request, "view-database", "database", database) + await self.check_permission(request, "view-table", "table", (database, table)) await self.check_permission( request, "view-row", "row", tuple([database, table] + list(pk_values)) ) diff --git a/docs/authentication.rst b/docs/authentication.rst index b0473ee80c..7fa96b3579 100644 --- a/docs/authentication.rst +++ b/docs/authentication.rst @@ -159,12 +159,12 @@ 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: +.. _permissions_view_instance: -view-index ----------- +view-instance +------------- -Actor is allowed to view the index page, e.g. https://latest.datasette.io/ +Top level permission - Actor is allowed to view any pages within this instance, starting at https://latest.datasette.io/ .. _permissions_view_database: @@ -232,6 +232,19 @@ Actor is allowed to view a :ref:`canned query ` page, e.g. https ``resource_identifier`` - string The name of the canned query +.. _permissions_execute_query: + +execute-query +------------- + +Actor is allowed to run arbitrary SQL queries against a specific database, e.g. https://latest.datasette.io/fixtures?sql=select+100 + +``resource_type`` - string + "database" + +``resource_identifier`` - string + The name of the database + .. _permissions_permissions_debug: permissions-debug diff --git a/tests/fixtures.py b/tests/fixtures.py index d175dfd544..f767dc84cb 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -842,17 +842,25 @@ def generate_sortable_rows(num): ) -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 - ) +def assert_permissions_checked(datasette, actions): + # actions is a list of "action" or (action, resource_type, resource_identifier) tuples + for action in actions: + if isinstance(action, str): + resource_type = None + resource_identifier = None + else: + action, resource_type, resource_identifier = action + 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, + json.dumps(list(datasette._permission_checks), indent=4), + ) diff --git a/tests/test_html.py b/tests/test_html.py index 3569b92c70..b41c1943d8 100644 --- a/tests/test_html.py +++ b/tests/test_html.py @@ -4,7 +4,7 @@ app_client_shorter_time_limit, app_client_two_attached_databases, app_client_with_hash, - assert_permission_checked, + assert_permissions_checked, make_app_client, METADATA, ) @@ -18,7 +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_permissions_checked(app_client_two_attached_databases.ds, ["view-instance"]) assert response.status == 200 assert "text/html; charset=utf-8" == response.headers["content-type"] soup = Soup(response.body, "html.parser") @@ -77,11 +77,8 @@ 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_permissions_checked( + client.ds, ["view-instance", ("view-database", "database", ":memory:")] ) assert response.status == 200 @@ -95,11 +92,8 @@ 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", + assert_permissions_checked( + app_client.ds, ["view-instance", ("view-database", "database", "fixtures")] ) soup = Soup(response.body, "html.parser") queries_ul = soup.find("h2", text="Queries").find_next_sibling("ul") @@ -211,11 +205,13 @@ 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( + assert_permissions_checked( client.ds, - "view-row", - resource_type="row", - resource_identifier=("fixtures", "facetable", "1"), + [ + "view-instance", + ("view-table", "table", ("fixtures", "facetable")), + ("view-row", "row", ("fixtures", "facetable", "1")), + ], ) table = Soup(response.body, "html.parser").find("table") assert table["class"] == ["rows-and-columns"] @@ -526,11 +522,13 @@ 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( + assert_permissions_checked( app_client.ds, - "view-table", - resource_type="table", - resource_identifier=("fixtures", "simple_primary_key"), + [ + "view-instance", + ("view-database", "database", "fixtures"), + ("view-table", "table", ("fixtures", "simple_primary_key")), + ], ) assert response.status == 200 table = Soup(response.body, "html.parser").find("table") @@ -887,6 +885,19 @@ def test_database_metadata(app_client): assert_footer_links(soup) +def test_database_query_permission_checks(app_client): + response = app_client.get("/fixtures?sql=select+1") + assert response.status == 200 + assert_permissions_checked( + app_client.ds, + [ + "view-instance", + ("view-database", "database", "fixtures"), + ("execute-query", "database", "fixtures"), + ], + ) + + def test_database_metadata_with_custom_sql(app_client): response = app_client.get("/fixtures?sql=select+*+from+simple_primary_key") assert response.status == 200 @@ -922,11 +933,13 @@ 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( + assert_permissions_checked( client.ds, - "view-database-download", - resource_type="database", - resource_identifier="fixtures", + [ + "view-instance", + ("view-database", "database", "fixtures"), + ("view-database-download", "database", "fixtures"), + ], ) @@ -1023,11 +1036,13 @@ 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( + assert_permissions_checked( app_client.ds, - "view-query", - resource_type="query", - resource_identifier=("fixtures", "neighborhood_search"), + [ + "view-instance", + ("view-database", "database", "fixtures"), + ("view-query", "query", ("fixtures", "neighborhood_search")), + ], ) assert response.status == 200 soup = Soup(response.body, "html.parser")