Removed resource_type from permissions system, closes #817

Refs #811, #699
This commit is contained in:
Simon Willison 2020-06-08 11:51:03 -07:00
commit c9f1ec616e
14 changed files with 39 additions and 89 deletions

View file

@ -465,7 +465,7 @@ class Datasette:
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 @@ class Datasette:
datasette=self,
actor=actor,
action=action,
resource_type=resource_type,
resource_identifier=resource_identifier,
):
if callable(check):
@ -491,7 +490,6 @@ class Datasette:
"when": datetime.datetime.utcnow().isoformat(),
"actor": actor,
"action": action,
"resource_type": resource_type,
"resource_identifier": resource_identifier,
"used_default": used_default,
"result": result,

View file

@ -3,7 +3,7 @@ from datasette.utils import actor_matches_allow
@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

View file

@ -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"

View file

@ -46,8 +46,8 @@
{% endif %}
</h2>
<p><strong>Actor:</strong> {{ check.actor|tojson }}</p>
{% if check.resource_type %}
<p><strong>Resource:</strong> {{ check.resource_type }} = {{ check.resource_identifier }}</p>
{% if check.resource_identifier %}
<p><strong>Resource:</strong> {{ check.resource_identifier }}</p>
{% endif %}
</div>
{% endfor %}

View file

@ -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

View file

@ -64,13 +64,10 @@ class BaseView(AsgiView):
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,
)

View file

@ -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 @@ class DatabaseView(DataView):
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 @@ class DatabaseView(DataView):
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 @@ class DatabaseView(DataView):
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 @@ class QueryView(DataView):
# 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 = {

View file

@ -26,7 +26,7 @@ class IndexView(BaseView):
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 @@ class IndexView(BaseView):
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 @@ class IndexView(BaseView):
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

View file

@ -268,11 +268,11 @@ class TableView(RowTableShared):
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

View file

@ -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(...)<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_permission_allowed>` plugin hook.
Datasette plugins can check if an actor has permission to perform an action using the :ref:`datasette.permission_allowed(...)<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 <canned_queries>` 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

View file

@ -121,8 +121,8 @@ Renders a `Jinja template <https://jinja.palletsprojects.com/en/2.11.x/>`__ 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.

View file

@ -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.

View file

@ -70,8 +70,8 @@ def check_permission_actions_are_documented():
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(

View file

@ -857,24 +857,21 @@ if __name__ == "__main__":
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),
)