From c9f1ec616e5a8c83f554baaedd38663569fb9b91 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Mon, 8 Jun 2020 11:51:03 -0700 Subject: [PATCH] Removed resource_type from permissions system, closes #817 Refs #811, #699 --- datasette/app.py | 4 +--- datasette/default_permissions.py | 5 +--- datasette/hookspecs.py | 2 +- datasette/templates/permissions_debug.html | 4 ++-- datasette/utils/__init__.py | 16 +++---------- datasette/views/base.py | 5 +--- datasette/views/database.py | 28 ++++++++-------------- datasette/views/index.py | 6 ++--- datasette/views/table.py | 10 ++++---- docs/authentication.rst | 19 ++------------- docs/internals.rst | 7 ++---- docs/plugins.rst | 9 +++---- tests/conftest.py | 4 ++-- tests/fixtures.py | 9 +++---- 14 files changed, 39 insertions(+), 89 deletions(-) diff --git a/datasette/app.py b/datasette/app.py index 87e542c18a..c12e0af0cd 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -465,7 +465,7 @@ def _show_messages(self, request): return [] async def permission_allowed( - self, actor, action, resource_type=None, resource_identifier=None, default=False + self, actor, action, resource_identifier=None, default=False ): "Check permissions using the permissions_allowed plugin hook" result = None @@ -473,7 +473,6 @@ async def permission_allowed( datasette=self, actor=actor, action=action, - resource_type=resource_type, resource_identifier=resource_identifier, ): if callable(check): @@ -491,7 +490,6 @@ async def permission_allowed( "when": datetime.datetime.utcnow().isoformat(), "actor": actor, "action": action, - "resource_type": resource_type, "resource_identifier": resource_identifier, "used_default": used_default, "result": result, diff --git a/datasette/default_permissions.py b/datasette/default_permissions.py index dd1770a347..d27704aa35 100644 --- a/datasette/default_permissions.py +++ b/datasette/default_permissions.py @@ -3,7 +3,7 @@ @hookimpl -def permission_allowed(datasette, actor, action, resource_type, resource_identifier): +def permission_allowed(datasette, actor, action, resource_identifier): if action == "permissions-debug": if actor and actor.get("id") == "root": return True @@ -12,13 +12,11 @@ def permission_allowed(datasette, actor, action, resource_type, resource_identif if allow is not None: return actor_matches_allow(actor, allow) elif action == "view-database": - assert resource_type == "database" database_allow = datasette.metadata("allow", database=resource_identifier) if database_allow is None: return True return actor_matches_allow(actor, database_allow) elif action == "view-table": - assert resource_type == "table" database, table = resource_identifier tables = datasette.metadata("tables", database=database) or {} table_allow = (tables.get(table) or {}).get("allow") @@ -27,7 +25,6 @@ def permission_allowed(datasette, actor, action, resource_type, resource_identif return actor_matches_allow(actor, table_allow) elif action == "view-query": # Check if this query has a "allow" block in metadata - assert resource_type == "query" database, query_name = resource_identifier queries_metadata = datasette.metadata("queries", database=database) assert query_name in queries_metadata diff --git a/datasette/hookspecs.py b/datasette/hookspecs.py index 71d06661fc..3c202553fd 100644 --- a/datasette/hookspecs.py +++ b/datasette/hookspecs.py @@ -66,5 +66,5 @@ def actor_from_request(datasette, request): @hookspec -def permission_allowed(datasette, actor, action, resource_type, resource_identifier): +def permission_allowed(datasette, actor, action, resource_identifier): "Check if actor is allowed to perfom this action - return True, False or None" diff --git a/datasette/templates/permissions_debug.html b/datasette/templates/permissions_debug.html index dda57dfa6f..7d3ee71206 100644 --- a/datasette/templates/permissions_debug.html +++ b/datasette/templates/permissions_debug.html @@ -46,8 +46,8 @@

{% endif %}

Actor: {{ check.actor|tojson }}

- {% if check.resource_type %} -

Resource: {{ check.resource_type }} = {{ check.resource_identifier }}

+ {% if check.resource_identifier %} +

Resource: {{ check.resource_identifier }}

{% endif %} {% endfor %} diff --git a/datasette/utils/__init__.py b/datasette/utils/__init__.py index 3d964049ad..257d1285c1 100644 --- a/datasette/utils/__init__.py +++ b/datasette/utils/__init__.py @@ -876,24 +876,14 @@ def actor_matches_allow(actor, allow): return False -async def check_visibility( - datasette, actor, action, resource_type, resource_identifier, default=True -): +async def check_visibility(datasette, actor, action, resource_identifier, default=True): "Returns (visible, private) - visible = can you see it, private = can others see it too" visible = await datasette.permission_allowed( - actor, - action, - resource_type=resource_type, - resource_identifier=resource_identifier, - default=default, + actor, action, resource_identifier=resource_identifier, default=default, ) if not visible: return (False, False) private = not await datasette.permission_allowed( - None, - action, - resource_type=resource_type, - resource_identifier=resource_identifier, - default=default, + None, action, resource_identifier=resource_identifier, default=default, ) return visible, private diff --git a/datasette/views/base.py b/datasette/views/base.py index 000d354b2e..2ca5e86ac5 100644 --- a/datasette/views/base.py +++ b/datasette/views/base.py @@ -64,13 +64,10 @@ async def head(self, *args, **kwargs): response.body = b"" return response - async def check_permission( - self, request, action, resource_type=None, resource_identifier=None - ): + async def check_permission(self, request, action, resource_identifier=None): ok = await self.ds.permission_allowed( request.actor, action, - resource_type=resource_type, resource_identifier=resource_identifier, default=True, ) diff --git a/datasette/views/database.py b/datasette/views/database.py index 824cb6321d..d562ecb1ab 100644 --- a/datasette/views/database.py +++ b/datasette/views/database.py @@ -21,7 +21,7 @@ class DatabaseView(DataView): 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) metadata = (self.ds.metadata("databases") or {}).get(database, {}) self.ds.update_with_inherited_metadata(metadata) @@ -43,7 +43,7 @@ async def data(self, request, database, hash, default_labels=False, _size=None): views = [] for view_name in await db.view_names(): visible, private = await check_visibility( - self.ds, request.actor, "view-table", "table", (database, view_name), + self.ds, request.actor, "view-table", (database, view_name), ) if visible: views.append( @@ -53,7 +53,7 @@ async def data(self, request, database, hash, default_labels=False, _size=None): tables = [] for table in table_counts: visible, private = await check_visibility( - self.ds, request.actor, "view-table", "table", (database, table), + self.ds, request.actor, "view-table", (database, table), ) if not visible: continue @@ -75,11 +75,7 @@ async def data(self, request, database, hash, default_labels=False, _size=None): canned_queries = [] for query in self.ds.get_canned_queries(database): visible, private = await check_visibility( - self.ds, - request.actor, - "view-query", - "query", - (database, query["name"]), + self.ds, request.actor, "view-query", (database, query["name"]), ) if visible: canned_queries.append(dict(query, private=private)) @@ -112,10 +108,8 @@ class DatabaseDownload(DataView): 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 - ) + await self.check_permission(request, "view-database", database) + await self.check_permission(request, "view-database-download", database) if database not in self.ds.databases: raise DatasetteError("Invalid database", status=404) db = self.ds.databases[database] @@ -155,17 +149,15 @@ async def data( # Respect canned query permissions await self.check_permission(request, "view-instance") - await self.check_permission(request, "view-database", "database", database) + await self.check_permission(request, "view-database", database) private = False if canned_query: - await self.check_permission( - request, "view-query", "query", (database, canned_query) - ) + await self.check_permission(request, "view-query", (database, canned_query)) private = not await self.ds.permission_allowed( - None, "view-query", "query", (database, canned_query), default=True + None, "view-query", (database, canned_query), default=True ) else: - await self.check_permission(request, "execute-sql", "database", database) + 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/index.py b/datasette/views/index.py index a3e8388cf0..b2706251a3 100644 --- a/datasette/views/index.py +++ b/datasette/views/index.py @@ -26,7 +26,7 @@ async def get(self, request, as_format): databases = [] for name, db in self.ds.databases.items(): visible, database_private = await check_visibility( - self.ds, request.actor, "view-database", "database", name, + self.ds, request.actor, "view-database", name, ) if not visible: continue @@ -36,7 +36,7 @@ async def get(self, request, as_format): views = [] for view_name in await db.view_names(): visible, private = await check_visibility( - self.ds, request.actor, "view-table", "table", (name, view_name), + self.ds, request.actor, "view-table", (name, view_name), ) if visible: views.append({"name": view_name, "private": private}) @@ -52,7 +52,7 @@ async def get(self, request, as_format): tables = {} for table in table_names: visible, private = await check_visibility( - self.ds, request.actor, "view-table", "table", (name, table), + self.ds, request.actor, "view-table", (name, table), ) if not visible: continue diff --git a/datasette/views/table.py b/datasette/views/table.py index cd95256861..4cec0cda8c 100644 --- a/datasette/views/table.py +++ b/datasette/views/table.py @@ -268,11 +268,11 @@ async def data( 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-database", database) + await self.check_permission(request, "view-table", (database, table)) private = not await self.ds.permission_allowed( - None, "view-table", "table", (database, table), default=True + None, "view-table", (database, table), default=True ) pks = await db.primary_keys(table) @@ -854,8 +854,8 @@ 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-database", database) + await self.check_permission(request, "view-table", (database, table)) 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 bda6a0b7ee..67112969c2 100644 --- a/docs/authentication.rst +++ b/docs/authentication.rst @@ -52,7 +52,7 @@ The URL on the first line includes a one-use token which can be used to sign in Permissions =========== -Datasette plugins can check if an actor has permission to perform an action using the :ref:`datasette.permission_allowed(...)` method. This method is also used by Datasette core code itself, which allows plugins to help make decisions on which actions are allowed by implementing the :ref:`permission_allowed(...) ` plugin hook. +Datasette plugins can check if an actor has permission to perform an action using the :ref:`datasette.permission_allowed(...)` method. This method is also used by Datasette core code itself, which allows plugins to help make decisions on which actions are allowed by implementing the :ref:`plugin_permission_allowed` plugin hook. .. _authentication_permissions_canned_queries: @@ -159,7 +159,7 @@ This is designed to help administrators and plugin authors understand exactly ho 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 the ``resource_identifier`` if it was passed. .. _permissions_view_instance: @@ -176,9 +176,6 @@ 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 @@ -189,9 +186,6 @@ 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 @@ -202,9 +196,6 @@ 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 @@ -215,9 +206,6 @@ 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 @@ -228,9 +216,6 @@ execute-sql 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 diff --git a/docs/internals.rst b/docs/internals.rst index 7498f017d7..1d61b6cb5a 100644 --- a/docs/internals.rst +++ b/docs/internals.rst @@ -121,8 +121,8 @@ Renders a `Jinja template `__ usin .. _datasette_permission_allowed: -await .permission_allowed(actor, action, resource_type=None, resource_identifier=None, default=False) ------------------------------------------------------------------------------------------------------ +await .permission_allowed(actor, action, resource_identifier=None, default=False) +--------------------------------------------------------------------------------- ``actor`` - dictionary The authenticated actor. This is usually ``request.actor``. @@ -130,9 +130,6 @@ await .permission_allowed(actor, action, resource_type=None, resource_identifier ``action`` - string The name of the action that is being permission checked. -``resource_type`` - string, optional - The type of resource being checked, e.g. ``"table"``. - ``resource_identifier`` - string, optional The resource identifier, e.g. the name of the table. diff --git a/docs/plugins.rst b/docs/plugins.rst index ecc7cbf172..118fab8427 100644 --- a/docs/plugins.rst +++ b/docs/plugins.rst @@ -1005,8 +1005,8 @@ Instead of returning a dictionary, this function can return an awaitable functio .. _plugin_permission_allowed: -permission_allowed(datasette, actor, action, resource_type, resource_identifier) -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +permission_allowed(datasette, actor, action, resource_identifier) +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ``datasette`` - :ref:`internals_datasette` You can use this to access plugin configuration options via ``datasette.plugin_config(your_plugin_name)``, or to execute SQL queries. @@ -1017,10 +1017,7 @@ permission_allowed(datasette, actor, action, resource_type, resource_identifier) ``action`` - string The action to be performed, e.g. ``"edit-table"``. -``resource_type`` - string - The type of resource being acted on, e.g. ``"table"``. - -``resource`` - string +``resource_identifier`` - string An identifier for the individual resource, e.g. the name of the table. Called to check that an actor has permission to perform an action on a resource. Can return ``True`` if the action is allowed, ``False`` if the action is not allowed or ``None`` if the plugin does not have an opinion one way or the other. diff --git a/tests/conftest.py b/tests/conftest.py index 1921ae3a56..7f1e9387c5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -70,8 +70,8 @@ def before(hook_name, hook_impls, kwargs): 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"] + ), "Undocumented permission action: {}, resource_identifier: {}".format( + action, kwargs["resource_identifier"] ) pm.add_hookcall_monitoring( diff --git a/tests/fixtures.py b/tests/fixtures.py index 2ac73fb19e..8210d34f0e 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -857,24 +857,21 @@ def generate_sortable_rows(num): def assert_permissions_checked(datasette, actions): - # actions is a list of "action" or (action, resource_type, resource_identifier) tuples + # actions is a list of "action" or (action, resource_identifier) tuples for action in actions: if isinstance(action, str): - resource_type = None resource_identifier = None else: - action, resource_type, resource_identifier = action + action, 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={} + ], """Missing expected permission check: action={}, resource_identifier={} Permission checks seen: {} """.format( action, - resource_type, resource_identifier, json.dumps(list(datasette._permission_checks), indent=4), )