diff --git a/datasette/app.py b/datasette/app.py index 4deb86974c..d95ec2bf5a 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -431,6 +431,20 @@ def __init__( self._root_token = secrets.token_hex(32) self.client = DatasetteClient(self) + def get_permission(self, name_or_abbr: str) -> "Permission": + """ + Returns a Permission object for the given name or abbreviation. Raises KeyError if not found. + """ + if name_or_abbr in self.permissions: + return self.permissions[name_or_abbr] + # Try abbreviation + for permission in self.permissions.values(): + if permission.abbr == name_or_abbr: + return permission + raise KeyError( + "No permission found with name or abbreviation {}".format(name_or_abbr) + ) + async def refresh_schemas(self): if self._refresh_schemas_lock.locked(): return diff --git a/datasette/default_permissions.py b/datasette/default_permissions.py index f0b086e989..960429fca9 100644 --- a/datasette/default_permissions.py +++ b/datasette/default_permissions.py @@ -2,6 +2,7 @@ from datasette.utils import actor_matches_allow import itsdangerous import time +from typing import Union, Tuple @hookimpl @@ -9,32 +10,112 @@ def register_permissions(): return ( # name, abbr, description, takes_database, takes_resource, default Permission( - "view-instance", "vi", "View Datasette instance", False, False, True + name="view-instance", + abbr="vi", + description="View Datasette instance", + takes_database=False, + takes_resource=False, + default=True, ), - Permission("view-database", "vd", "View database", True, False, True), Permission( - "view-database-download", "vdd", "Download database file", True, False, True + name="view-database", + abbr="vd", + description="View database", + takes_database=True, + takes_resource=False, + default=True, + implies_can_view=True, ), - Permission("view-table", "vt", "View table", True, True, True), - Permission("view-query", "vq", "View named query results", True, True, True), Permission( - "execute-sql", "es", "Execute read-only SQL queries", True, False, True + name="view-database-download", + abbr="vdd", + description="Download database file", + takes_database=True, + takes_resource=False, + default=True, ), Permission( - "permissions-debug", - "pd", - "Access permission debug tool", - False, - False, - False, + name="view-table", + abbr="vt", + description="View table", + takes_database=True, + takes_resource=True, + default=True, + implies_can_view=True, + ), + Permission( + name="view-query", + abbr="vq", + description="View named query results", + takes_database=True, + takes_resource=True, + default=True, + implies_can_view=True, + ), + Permission( + name="execute-sql", + abbr="es", + description="Execute read-only SQL queries", + takes_database=True, + takes_resource=False, + default=True, + ), + Permission( + name="permissions-debug", + abbr="pd", + description="Access permission debug tool", + takes_database=False, + takes_resource=False, + default=False, + ), + Permission( + name="debug-menu", + abbr="dm", + description="View debug menu items", + takes_database=False, + takes_resource=False, + default=False, + ), + Permission( + name="insert-row", + abbr="ir", + description="Insert rows", + takes_database=True, + takes_resource=True, + default=False, + ), + Permission( + name="delete-row", + abbr="dr", + description="Delete rows", + takes_database=True, + takes_resource=True, + default=False, + ), + Permission( + name="update-row", + abbr="ur", + description="Update rows", + takes_database=True, + takes_resource=True, + default=False, + ), + Permission( + name="create-table", + abbr="ct", + description="Create tables", + takes_database=True, + takes_resource=False, + default=False, + ), + Permission( + name="drop-table", + abbr="dt", + description="Drop tables", + takes_database=True, + takes_resource=True, + default=False, ), - Permission("debug-menu", "dm", "View debug menu items", False, False, False), - # Write API permissions - Permission("insert-row", "ir", "Insert rows", True, True, False), - Permission("delete-row", "dr", "Delete rows", True, True, False), - Permission("update-row", "ur", "Update rows", True, True, False), - Permission("create-table", "ct", "Create tables", True, False, False), - Permission("drop-table", "dt", "Drop tables", True, True, False), ) @@ -176,14 +257,39 @@ async def _resolve_metadata_view_permissions(datasette, actor, action, resource) return actor_matches_allow(actor, database_allow_sql) -@hookimpl(specname="permission_allowed") -def permission_allowed_actor_restrictions(datasette, actor, action, resource): - if actor is None: - return None - if "_r" not in actor: - # No restrictions, so we have no opinion - return None - _r = actor.get("_r") +def restrictions_allow_action( + datasette: "Datasette", + restrictions: dict, + action: str, + resource: Union[str, Tuple[str, str]], +): + "Do these restrictions allow the requested action against the requested resource?" + if action == "view-instance": + # Special case for view-instance: it's allowed if the restrictions include any + # permissions that have the implies_can_view=True flag set + all_rules = restrictions.get("a") or [] + for database_rules in (restrictions.get("d") or {}).values(): + all_rules += database_rules + for database_resource_rules in (restrictions.get("r") or {}).values(): + for resource_rules in database_resource_rules.values(): + all_rules += resource_rules + permissions = [datasette.get_permission(action) for action in all_rules] + if any(p for p in permissions if p.implies_can_view): + return True + + if action == "view-database": + # Special case for view-database: it's allowed if the restrictions include any + # permissions that have the implies_can_view=True flag set AND takes_database + all_rules = restrictions.get("a") or [] + database_rules = list((restrictions.get("d") or {}).get(resource) or []) + all_rules += database_rules + resource_rules = ((restrictions.get("r") or {}).get(resource) or {}).values() + for resource_rules in (restrictions.get("r") or {}).values(): + for table_rules in resource_rules.values(): + all_rules += table_rules + permissions = [datasette.get_permission(action) for action in all_rules] + if any(p for p in permissions if p.implies_can_view and p.takes_database): + return True # Does this action have an abbreviation? to_check = {action} @@ -191,35 +297,56 @@ def permission_allowed_actor_restrictions(datasette, actor, action, resource): if permission and permission.abbr: to_check.add(permission.abbr) - # If _r is defined then we use those to further restrict the actor + # If restrictions is defined then we use those to further restrict the actor # Crucially, we only use this to say NO (return False) - we never # use it to return YES (True) because that might over-ride other # restrictions placed on this actor - all_allowed = _r.get("a") + all_allowed = restrictions.get("a") if all_allowed is not None: assert isinstance(all_allowed, list) if to_check.intersection(all_allowed): - return None + return True # How about for the current database? - if isinstance(resource, str): - database_allowed = _r.get("d", {}).get(resource) + if resource: + if isinstance(resource, str): + database_name = resource + else: + database_name = resource[0] + database_allowed = restrictions.get("d", {}).get(database_name) if database_allowed is not None: assert isinstance(database_allowed, list) if to_check.intersection(database_allowed): - return None + return True # Or the current table? That's any time the resource is (database, table) if resource is not None and not isinstance(resource, str) and len(resource) == 2: database, table = resource - table_allowed = _r.get("r", {}).get(database, {}).get(table) + table_allowed = restrictions.get("r", {}).get(database, {}).get(table) # TODO: What should this do for canned queries? if table_allowed is not None: assert isinstance(table_allowed, list) if to_check.intersection(table_allowed): - return None + return True + # This action is not specifically allowed, so reject it return False +@hookimpl(specname="permission_allowed") +def permission_allowed_actor_restrictions(datasette, actor, action, resource): + if actor is None: + return None + if "_r" not in actor: + # No restrictions, so we have no opinion + return None + _r = actor.get("_r") + if restrictions_allow_action(datasette, _r, action, resource): + # Return None because we do not have an opinion here + return None + else: + # Block this permission check + return False + + @hookimpl def actor_from_request(datasette, request): prefix = "dstok_" diff --git a/datasette/permissions.py b/datasette/permissions.py index 1cd3474d43..152f17213c 100644 --- a/datasette/permissions.py +++ b/datasette/permissions.py @@ -1,6 +1,16 @@ -import collections +from dataclasses import dataclass, fields +from typing import Optional -Permission = collections.namedtuple( - "Permission", - ("name", "abbr", "description", "takes_database", "takes_resource", "default"), -) + +@dataclass +class Permission: + name: str + abbr: Optional[str] + description: Optional[str] + takes_database: bool + takes_resource: bool + default: bool + # This is deliberately undocumented: it's considered an internal + # implementation detail for view-table/view-database and should + # not be used by plugins as it may change in the future. + implies_can_view: bool = False diff --git a/datasette/views/special.py b/datasette/views/special.py index c1b84f8f9a..849750bff5 100644 --- a/datasette/views/special.py +++ b/datasette/views/special.py @@ -122,7 +122,17 @@ async def get(self, request): # list() avoids error if check is performed during template render: { "permission_checks": list(reversed(self.ds._permission_checks)), - "permissions": list(self.ds.permissions.values()), + "permissions": [ + ( + p.name, + p.abbr, + p.description, + p.takes_database, + p.takes_resource, + p.default, + ) + for p in self.ds.permissions.values() + ], }, ) diff --git a/docs/internals.rst b/docs/internals.rst index fe9a2fa769..474e3328f5 100644 --- a/docs/internals.rst +++ b/docs/internals.rst @@ -280,7 +280,7 @@ All databases are listed, irrespective of user permissions. Property exposing a dictionary of permissions that have been registered using the :ref:`plugin_register_permissions` plugin hook. -The dictionary keys are the permission names - e.g. ``view-instance`` - and the values are ``Permission()`` named tuples describing the permission. Here is a :ref:`description of that tuple `. +The dictionary keys are the permission names - e.g. ``view-instance`` - and the values are ``Permission()`` objects describing the permission. Here is a :ref:`description of that object `. .. _datasette_plugin_config: @@ -469,6 +469,16 @@ The following example creates a token that can access ``view-instance`` and ``vi }, ) +.. _datasette_get_permission: + +.get_permission(name_or_abbr) +----------------------------- + +``name_or_abbr`` - string + The name or abbreviation of the permission to look up, e.g. ``view-table`` or ``vt``. + +Returns a :ref:`Permission object ` representing the permission, or raises a ``KeyError`` if one is not found. + .. _datasette_get_database: .get_database(name) diff --git a/docs/plugin_hooks.rst b/docs/plugin_hooks.rst index 497508aedb..f8bb203d07 100644 --- a/docs/plugin_hooks.rst +++ b/docs/plugin_hooks.rst @@ -794,24 +794,24 @@ If your plugin needs to register additional permissions unique to that plugin - ) ] -The fields of the ``Permission`` named tuple are as follows: +The fields of the ``Permission`` class are as follows: -``name`` +``name`` - string The name of the permission, e.g. ``upload-csvs``. This should be unique across all plugins that the user might have installed, so choose carefully. -``abbr`` +``abbr`` - string or None An abbreviation of the permission, e.g. ``uc``. This is optional - you can set it to ``None`` if you do not want to pick an abbreviation. Since this needs to be unique across all installed plugins it's best not to specify an abbreviation at all. If an abbreviation is provided it will be used when creating restricted signed API tokens. -``description`` +``description`` - string or None A human-readable description of what the permission lets you do. Should make sense as the second part of a sentence that starts "A user with this permission can ...". -``takes_database`` +``takes_database`` - boolean ``True`` if this permission can be granted on a per-database basis, ``False`` if it is only valid at the overall Datasette instance level. -``takes_resource`` +``takes_resource`` - boolean ``True`` if this permission can be granted on a per-resource basis. A resource is a database table, SQL view or :ref:`canned query `. -``default`` +``default`` - boolean The default value for this permission if it is not explicitly granted to a user. ``True`` means the permission is granted by default, ``False`` means it is not. This should only be ``True`` if you want anonymous users to be able to take this action. diff --git a/tests/test_internals_datasette.py b/tests/test_internals_datasette.py index d59ff72976..c11e840c25 100644 --- a/tests/test_internals_datasette.py +++ b/tests/test_internals_datasette.py @@ -159,3 +159,17 @@ def test_datasette_error_if_string_not_list(tmpdir): db_path = str(tmpdir / "data.db") with pytest.raises(ValueError): ds = Datasette(db_path) + + +@pytest.mark.asyncio +async def test_get_permission(ds_client): + ds = ds_client.ds + for name_or_abbr in ("vi", "view-instance", "vt", "view-table"): + permission = ds.get_permission(name_or_abbr) + if "-" in name_or_abbr: + assert permission.name == name_or_abbr + else: + assert permission.abbr == name_or_abbr + # And test KeyError + with pytest.raises(KeyError): + ds.get_permission("missing-permission") diff --git a/tests/test_permissions.py b/tests/test_permissions.py index f940d48660..cad0525f94 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -1,6 +1,7 @@ import collections from datasette.app import Datasette from datasette.cli import cli +from datasette.default_permissions import restrictions_allow_action from .fixtures import app_client, assert_permissions_checked, make_app_client from click.testing import CliRunner from bs4 import BeautifulSoup as Soup @@ -35,6 +36,8 @@ async def perms_ds(): one = ds.add_memory_database("perms_ds_one") two = ds.add_memory_database("perms_ds_two") await one.execute_write("create table if not exists t1 (id integer primary key)") + await one.execute_write("insert or ignore into t1 (id) values (1)") + await one.execute_write("create view if not exists v1 as select * from t1") await one.execute_write("create table if not exists t2 (id integer primary key)") await two.execute_write("create table if not exists t1 (id integer primary key)") return ds @@ -585,7 +588,6 @@ def test_padlocks_on_database_page(cascade_app_client): ({"id": "t", "_r": {"a": ["vd"]}}, "view-database", "one", None, DEF), ({"id": "t", "_r": {"a": ["vt"]}}, "view-table", "one", "t1", DEF), # But not if it's the wrong permission - ({"id": "t", "_r": {"a": ["vd"]}}, "view-instance", None, None, False), ({"id": "t", "_r": {"a": ["vi"]}}, "view-database", "one", None, False), ({"id": "t", "_r": {"a": ["vd"]}}, "view-table", "one", "t1", False), # Works at the "d" for database level: @@ -629,11 +631,14 @@ def test_padlocks_on_database_page(cascade_app_client): "t1", DEF, ), + # view-instance is granted if you have view-database + ({"id": "t", "_r": {"a": ["vd"]}}, "view-instance", None, None, DEF), ), ) async def test_actor_restricted_permissions( perms_ds, actor, permission, resource_1, resource_2, expected_result ): + perms_ds.pdb = True cookies = {"ds_actor": perms_ds.sign({"a": {"id": "root"}}, "actor")} csrftoken = (await perms_ds.client.get("/-/permissions", cookies=cookies)).cookies[ "ds_csrftoken" @@ -1018,3 +1023,190 @@ async def test_api_explorer_visibility( assert response.status_code == 403 finally: perms_ds._metadata_local = prev_metadata + + +@pytest.mark.asyncio +async def test_view_table_token_can_access_table(perms_ds): + actor = { + "id": "restricted-token", + "token": "dstok", + # Restricted to just view-table on perms_ds_two/t1 + "_r": {"r": {"perms_ds_two": {"t1": ["vt"]}}}, + } + cookies = {"ds_actor": perms_ds.client.actor_cookie(actor)} + response = await perms_ds.client.get("/perms_ds_two/t1.json", cookies=cookies) + assert response.status_code == 200 + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "restrictions,verb,path,body,expected_status", + ( + # No restrictions + (None, "get", "/.json", None, 200), + (None, "get", "/perms_ds_one.json", None, 200), + (None, "get", "/perms_ds_one/t1.json", None, 200), + (None, "get", "/perms_ds_one/t1/1.json", None, 200), + (None, "get", "/perms_ds_one/v1.json", None, 200), + # Restricted to just view-instance + ({"a": ["vi"]}, "get", "/.json", None, 200), + ({"a": ["vi"]}, "get", "/perms_ds_one.json", None, 403), + ({"a": ["vi"]}, "get", "/perms_ds_one/t1.json", None, 403), + ({"a": ["vi"]}, "get", "/perms_ds_one/t1/1.json", None, 403), + ({"a": ["vi"]}, "get", "/perms_ds_one/v1.json", None, 403), + # Restricted to just view-database + ({"a": ["vd"]}, "get", "/.json", None, 200), # Can see instance too + ({"a": ["vd"]}, "get", "/perms_ds_one.json", None, 200), + ({"a": ["vd"]}, "get", "/perms_ds_one/t1.json", None, 403), + ({"a": ["vd"]}, "get", "/perms_ds_one/t1/1.json", None, 403), + ({"a": ["vd"]}, "get", "/perms_ds_one/v1.json", None, 403), + # Restricted to just view-table for specific database + ( + {"d": {"perms_ds_one": ["vt"]}}, + "get", + "/.json", + None, + 200, + ), # Can see instance + ( + {"d": {"perms_ds_one": ["vt"]}}, + "get", + "/perms_ds_one.json", + None, + 200, + ), # and this database + ( + {"d": {"perms_ds_one": ["vt"]}}, + "get", + "/perms_ds_two.json", + None, + 403, + ), # But not this one + ( + # Can see the table + {"d": {"perms_ds_one": ["vt"]}}, + "get", + "/perms_ds_one/t1.json", + None, + 200, + ), + ( + # And the view + {"d": {"perms_ds_one": ["vt"]}}, + "get", + "/perms_ds_one/v1.json", + None, + 200, + ), + # view-table access to a specific table + ( + {"r": {"perms_ds_one": {"t1": ["vt"]}}}, + "get", + "/.json", + None, + 200, + ), + ( + {"r": {"perms_ds_one": {"t1": ["vt"]}}}, + "get", + "/perms_ds_one.json", + None, + 200, + ), + ( + {"r": {"perms_ds_one": {"t1": ["vt"]}}}, + "get", + "/perms_ds_one/t1.json", + None, + 200, + ), + # But cannot see the other table + ( + {"r": {"perms_ds_one": {"t1": ["vt"]}}}, + "get", + "/perms_ds_one/t2.json", + None, + 403, + ), + # Or the view + ( + {"r": {"perms_ds_one": {"t1": ["vt"]}}}, + "get", + "/perms_ds_one/v1.json", + None, + 403, + ), + ), +) +async def test_actor_restrictions( + perms_ds, restrictions, verb, path, body, expected_status +): + actor = {"id": "user"} + if restrictions: + actor["_r"] = restrictions + method = getattr(perms_ds.client, verb) + kwargs = {"cookies": {"ds_actor": perms_ds.client.actor_cookie(actor)}} + if body: + kwargs["json"] = body + perms_ds._permission_checks.clear() + response = await method(path, **kwargs) + assert response.status_code == expected_status, json.dumps( + { + "verb": verb, + "path": path, + "body": body, + "restrictions": restrictions, + "expected_status": expected_status, + "response_status": response.status_code, + "checks": [ + { + "action": check["action"], + "resource": check["resource"], + "result": check["result"], + } + for check in perms_ds._permission_checks + ], + }, + indent=2, + ) + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "restrictions,action,resource,expected", + ( + ({"a": ["view-instance"]}, "view-instance", None, True), + # view-table and view-database implies view-instance + ({"a": ["view-table"]}, "view-instance", None, True), + ({"a": ["view-database"]}, "view-instance", None, True), + # update-row does not imply view-instance + ({"a": ["update-row"]}, "view-instance", None, False), + # view-table on a resource implies view-instance + ({"r": {"db1": {"t1": ["view-table"]}}}, "view-instance", None, True), + # update-row on a resource does not imply view-instance + ({"r": {"db1": {"t1": ["update-row"]}}}, "view-instance", None, False), + # view-database on a resource implies view-instance + ({"d": {"db1": ["view-database"]}}, "view-instance", None, True), + # Having view-table on "a" allows access to any specific table + ({"a": ["view-table"]}, "view-table", ("dbname", "tablename"), True), + # Ditto for on the database + ( + {"d": {"dbname": ["view-table"]}}, + "view-table", + ("dbname", "tablename"), + True, + ), + # But not if it's allowed on a different database + ( + {"d": {"dbname": ["view-table"]}}, + "view-table", + ("dbname2", "tablename"), + False, + ), + ), +) +async def test_restrictions_allow_action(restrictions, action, resource, expected): + ds = Datasette() + await ds.invoke_startup() + actual = restrictions_allow_action(ds, restrictions, action, resource) + assert actual == expected