diff --git a/datasette/views/base.py b/datasette/views/base.py index 6346a3f576..399b1a1f15 100644 --- a/datasette/views/base.py +++ b/datasette/views/base.py @@ -69,6 +69,29 @@ async def check_permission(self, request, action, resource=None): if not ok: raise Forbidden(action) + async def check_permissions(self, request, permissions): + "permissions is a list of (action, resource) tuples or 'action' strings" + for permission in permissions: + if isinstance(permission, str): + action = permission + resource = None + elif isinstance(permission, (tuple, list)) and len(permission) == 2: + action, resource = permission + else: + assert ( + False + ), "permission should be string or tuple of two items: {}".format( + repr(permission) + ) + ok = await self.ds.permission_allowed( + request.actor, action, resource=resource, default=None, + ) + if ok is not None: + if ok: + return + else: + 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 44750f5ba5..257305fd00 100644 --- a/datasette/views/database.py +++ b/datasette/views/database.py @@ -20,8 +20,9 @@ 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) + await self.check_permissions( + request, [("view-database", database), "view-instance",], + ) metadata = (self.ds.metadata("databases") or {}).get(database, {}) self.ds.update_with_inherited_metadata(metadata) @@ -88,7 +89,7 @@ async def data(self, request, database, hash, default_labels=False, _size=None): "views": views, "queries": canned_queries, "private": not await self.ds.permission_allowed( - None, "view-database", database + None, "view-database", database, default=True ), "allow_execute_sql": await self.ds.permission_allowed( request.actor, "execute-sql", database, default=True @@ -150,17 +151,23 @@ async def data( if "_shape" in params: params.pop("_shape") - # Respect canned query permissions - await self.check_permission(request, "view-instance") - await self.check_permission(request, "view-database", database) private = False if canned_query: - await self.check_permission(request, "view-query", (database, canned_query)) + # Respect canned query permissions + await self.check_permissions( + request, + [ + ("view-query", (database, canned_query)), + ("view-database", database), + "view-instance", + ], + ) private = not await self.ds.permission_allowed( None, "view-query", (database, canned_query), default=True ) else: await self.check_permission(request, "execute-sql", database) + # Extract any :named parameters named_parameters = named_parameters or self.re_named_parameter.findall(sql) named_parameter_values = { diff --git a/datasette/views/table.py b/datasette/views/table.py index 1a55a4950b..e0a52e2038 100644 --- a/datasette/views/table.py +++ b/datasette/views/table.py @@ -269,9 +269,14 @@ 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) - await self.check_permission(request, "view-table", (database, table)) + await self.check_permissions( + request, + [ + ("view-table", (database, table)), + ("view-database", database), + "view-instance", + ], + ) private = not await self.ds.permission_allowed( None, "view-table", (database, table), default=True diff --git a/tests/fixtures.py b/tests/fixtures.py index 94a3cce52f..a9b9a3968b 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -462,7 +462,9 @@ def generate_sortable_rows(num): "queries": { "𝐜𝐢𝐭𝐢𝐞𝐬": "select id, name from facet_cities order by id limit 1;", "pragma_cache_size": "PRAGMA cache_size;", - "magic_parameters": "select :_header_user_agent as user_agent, :_now_datetime_utc as datetime", + "magic_parameters": { + "sql": "select :_header_user_agent as user_agent, :_now_datetime_utc as datetime", + }, "neighborhood_search": { "sql": textwrap.dedent( """ diff --git a/tests/test_permissions.py b/tests/test_permissions.py index 241dd2e562..2d57b5e35d 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -1,5 +1,6 @@ from .fixtures import app_client, assert_permissions_checked, make_app_client from bs4 import BeautifulSoup as Soup +import copy import pytest @@ -43,7 +44,7 @@ def test_view_database(allow, expected_anon, expected_auth): "/fixtures/compound_three_primary_keys/a,a,a", ): anon_response = client.get(path) - assert expected_anon == anon_response.status + assert expected_anon == anon_response.status, path if allow and path == "/fixtures" and anon_response.status == 200: # Should be no padlock assert ">fixtures 🔒" not in anon_response.text @@ -348,3 +349,61 @@ def test_view_instance(path, view_instance_client): assert 403 == view_instance_client.get(path).status if path not in ("/-/permissions", "/-/messages", "/-/patterns"): assert 403 == view_instance_client.get(path + ".json").status + + +@pytest.fixture(scope="session") +def cascade_app_client(): + with make_app_client() as client: + yield client + + +@pytest.mark.parametrize( + "path,expected_status,permissions", + [ + ("/", 403, []), + ("/", 200, ["instance"]), + # Can view table even if not allowed database or instance + ("/fixtures/facet_cities", 403, []), + ("/fixtures/facet_cities", 403, ["database"]), + ("/fixtures/facet_cities", 403, ["instance"]), + ("/fixtures/facet_cities", 200, ["table"]), + ("/fixtures/facet_cities", 200, ["table", "database"]), + ("/fixtures/facet_cities", 200, ["table", "database", "instance"]), + # Can view query even if not allowed database or instance + ("/fixtures/magic_parameters", 403, []), + ("/fixtures/magic_parameters", 403, ["database"]), + ("/fixtures/magic_parameters", 403, ["instance"]), + ("/fixtures/magic_parameters", 200, ["query"]), + ("/fixtures/magic_parameters", 200, ["query", "database"]), + ("/fixtures/magic_parameters", 200, ["query", "database", "instance"]), + # Can view database even if not allowed instance + ("/fixtures", 403, []), + ("/fixtures", 403, ["instance"]), + ("/fixtures", 200, ["database"]), + ], +) +def test_permissions_cascade(cascade_app_client, path, expected_status, permissions): + "Test that e.g. having view-table but NOT view-database lets you view table page, etc" + allow = {"id": "*"} + deny = {} + previous_metadata = cascade_app_client.ds._metadata + updated_metadata = copy.deepcopy(previous_metadata) + try: + # Set up the different allow blocks + updated_metadata["allow"] = allow if "instance" in permissions else deny + updated_metadata["databases"]["fixtures"]["allow"] = ( + allow if "database" in permissions else deny + ) + updated_metadata["databases"]["fixtures"]["tables"]["facet_cities"]["allow"] = ( + allow if "table" in permissions else deny + ) + updated_metadata["databases"]["fixtures"]["queries"]["magic_parameters"][ + "allow" + ] = (allow if "query" in permissions else deny) + cascade_app_client.ds._metadata = updated_metadata + response = cascade_app_client.get( + path, cookies={"ds_actor": cascade_app_client.actor_cookie({"id": "test"})}, + ) + assert expected_status == response.status + finally: + cascade_app_client.ds._metadata = previous_metadata