Nested permission checks for all views, refs #811

This commit is contained in:
Simon Willison 2020-06-07 13:03:08 -07:00
commit 4340845754
6 changed files with 97 additions and 48 deletions

View file

@ -19,6 +19,7 @@ class DatabaseView(DataView):
name = "database" name = "database"
async def data(self, request, database, hash, default_labels=False, _size=None): 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) await self.check_permission(request, "view-database", "database", database)
metadata = (self.ds.metadata("databases") or {}).get(database, {}) metadata = (self.ds.metadata("databases") or {}).get(database, {})
self.ds.update_with_inherited_metadata(metadata) self.ds.update_with_inherited_metadata(metadata)
@ -90,6 +91,8 @@ class DatabaseDownload(DataView):
name = "database_download" name = "database_download"
async def view_get(self, request, database, hash, correct_hash_present, **kwargs): 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( await self.check_permission(
request, "view-database-download", "database", database request, "view-database-download", "database", database
) )
@ -132,6 +135,8 @@ class QueryView(DataView):
# Respect canned query permissions # Respect canned query permissions
if canned_query: if canned_query:
await self.check_permission(request, "view-instance")
await self.check_permission(request, "view-database", "database", database)
await self.check_permission( await self.check_permission(
request, "view-query", "query", (database, canned_query) request, "view-query", "query", (database, canned_query)
) )
@ -140,7 +145,10 @@ class QueryView(DataView):
request.scope.get("actor", None), metadata.get("allow") request.scope.get("actor", None), metadata.get("allow")
): ):
return Response("Permission denied", status=403) 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 # Extract any :named parameters
named_parameters = named_parameters or self.re_named_parameter.findall(sql) named_parameters = named_parameters or self.re_named_parameter.findall(sql)
named_parameter_values = { named_parameter_values = {

View file

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

View file

@ -267,6 +267,8 @@ class TableView(RowTableShared):
if not is_view and not table_exists: if not is_view and not table_exists:
raise NotFound("Table not found: {}".format(table)) 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)) await self.check_permission(request, "view-table", "table", (database, table))
pks = await db.primary_keys(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): async def data(self, request, database, hash, table, pk_path, default_labels=False):
pk_values = urlsafe_components(pk_path) 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( await self.check_permission(
request, "view-row", "row", tuple([database, table] + list(pk_values)) request, "view-row", "row", tuple([database, table] + list(pk_values))
) )

View file

@ -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. 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: .. _permissions_view_database:
@ -232,6 +232,19 @@ Actor is allowed to view a :ref:`canned query <canned_queries>` page, e.g. https
``resource_identifier`` - string ``resource_identifier`` - string
The name of the canned query 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_permissions_debug:
permissions-debug permissions-debug

View file

@ -842,17 +842,25 @@ if __name__ == "__main__":
) )
def assert_permission_checked( def assert_permissions_checked(datasette, actions):
datasette, action, resource_type=None, resource_identifier=None # actions is a list of "action" or (action, resource_type, resource_identifier) tuples
): for action in actions:
assert [ if isinstance(action, str):
pc resource_type = None
for pc in datasette._permission_checks resource_identifier = None
if pc["action"] == action else:
and pc["resource_type"] == resource_type action, resource_type, resource_identifier = action
and pc["resource_identifier"] == resource_identifier assert [
], """Missing expected permission check: action={}, resource_type={}, resource_identifier={} pc
Permission checks seen: {} for pc in datasette._permission_checks
""".format( if pc["action"] == action
action, resource_type, resource_identifier, datasette._permission_checks 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),
)

View file

@ -4,7 +4,7 @@ from .fixtures import ( # noqa
app_client_shorter_time_limit, app_client_shorter_time_limit,
app_client_two_attached_databases, app_client_two_attached_databases,
app_client_with_hash, app_client_with_hash,
assert_permission_checked, assert_permissions_checked,
make_app_client, make_app_client,
METADATA, METADATA,
) )
@ -18,7 +18,7 @@ import urllib.parse
def test_homepage(app_client_two_attached_databases): def test_homepage(app_client_two_attached_databases):
response = app_client_two_attached_databases.get("/") 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 response.status == 200
assert "text/html; charset=utf-8" == response.headers["content-type"] assert "text/html; charset=utf-8" == response.headers["content-type"]
soup = Soup(response.body, "html.parser") soup = Soup(response.body, "html.parser")
@ -77,11 +77,8 @@ def test_static_mounts():
def test_memory_database_page(): def test_memory_database_page():
for client in make_app_client(memory=True): for client in make_app_client(memory=True):
response = client.get("/:memory:") response = client.get("/:memory:")
assert_permission_checked( assert_permissions_checked(
client.ds, client.ds, ["view-instance", ("view-database", "database", ":memory:")]
"view-database",
resource_type="database",
resource_identifier=":memory:",
) )
assert response.status == 200 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): def test_database_page(app_client):
response = app_client.get("/fixtures") response = app_client.get("/fixtures")
assert_permission_checked( assert_permissions_checked(
app_client.ds, app_client.ds, ["view-instance", ("view-database", "database", "fixtures")]
"view-database",
resource_type="database",
resource_identifier="fixtures",
) )
soup = Soup(response.body, "html.parser") soup = Soup(response.body, "html.parser")
queries_ul = soup.find("h2", text="Queries").find_next_sibling("ul") 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}): for client in make_app_client(config={"truncate_cells_html": 5}):
response = client.get("/fixtures/facetable/1") response = client.get("/fixtures/facetable/1")
assert response.status == 200 assert response.status == 200
assert_permission_checked( assert_permissions_checked(
client.ds, client.ds,
"view-row", [
resource_type="row", "view-instance",
resource_identifier=("fixtures", "facetable", "1"), ("view-table", "table", ("fixtures", "facetable")),
("view-row", "row", ("fixtures", "facetable", "1")),
],
) )
table = Soup(response.body, "html.parser").find("table") table = Soup(response.body, "html.parser").find("table")
assert table["class"] == ["rows-and-columns"] 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): def test_table_html_simple_primary_key(app_client):
response = app_client.get("/fixtures/simple_primary_key?_size=3") response = app_client.get("/fixtures/simple_primary_key?_size=3")
assert_permission_checked( assert_permissions_checked(
app_client.ds, app_client.ds,
"view-table", [
resource_type="table", "view-instance",
resource_identifier=("fixtures", "simple_primary_key"), ("view-database", "database", "fixtures"),
("view-table", "table", ("fixtures", "simple_primary_key")),
],
) )
assert response.status == 200 assert response.status == 200
table = Soup(response.body, "html.parser").find("table") table = Soup(response.body, "html.parser").find("table")
@ -887,6 +885,19 @@ def test_database_metadata(app_client):
assert_footer_links(soup) 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): def test_database_metadata_with_custom_sql(app_client):
response = app_client.get("/fixtures?sql=select+*+from+simple_primary_key") response = app_client.get("/fixtures?sql=select+*+from+simple_primary_key")
assert response.status == 200 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$")})) assert len(soup.findAll("a", {"href": re.compile(r"\.db$")}))
# Check we can actually download it # Check we can actually download it
assert 200 == client.get("/fixtures.db").status assert 200 == client.get("/fixtures.db").status
assert_permission_checked( assert_permissions_checked(
client.ds, client.ds,
"view-database-download", [
resource_type="database", "view-instance",
resource_identifier="fixtures", ("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): def test_canned_query_with_custom_metadata(app_client):
response = app_client.get("/fixtures/neighborhood_search?text=town") response = app_client.get("/fixtures/neighborhood_search?text=town")
assert_permission_checked( assert_permissions_checked(
app_client.ds, app_client.ds,
"view-query", [
resource_type="query", "view-instance",
resource_identifier=("fixtures", "neighborhood_search"), ("view-database", "database", "fixtures"),
("view-query", "query", ("fixtures", "neighborhood_search")),
],
) )
assert response.status == 200 assert response.status == 200
soup = Soup(response.body, "html.parser") soup = Soup(response.body, "html.parser")