datasette.permission_allowed() should consider all checks, not just the last returned

[simonw]

Spotted this in the code for datasette.permission_allowed():

datasette/datasette/app.py

Lines 906 to 914 in 232a304

	for check in pm.hook.permission_allowed(
	datasette=self,
	actor=actor,
	action=action,
	resource=resource,
	):
	check = await await_me_maybe(check)
	if check is not None:
	result = check

This is effectively saying that the permission hook which "wins" is the last one checked in the sequence.

This is actually new behaviour, introduced in dfdbdf3#diff-83d29b1fb42ebaa92e01122d8142807188ab7b6689697c9defdcdca60ce13bc5L435 - prior to that commit the logic looked like this instead:

datasette/datasette/app.py

Lines 426 to 436 in 57cf513

	action=action,
	resource_type=resource_type,
	resource_identifier=resource_identifier,
	):
	if callable(check):
	check = check()
	if asyncio.iscoroutine(check):
	check = await check
	if check is not None:
	return check
	return default

So the original logic was first-non-Null-wins, and then I changed it to last-non-Null-wins in dfdbdf3 - but the commit message and issue for that change don't mention it, which makes me think this was a mistake:

• /-/permissions debugging tool #788

I don't think first wins OR last wins are the right solution here, especially given that Pluggy plugin order is a little bit non-deterministic (aside from when plugins use tryfirst= and trylast=).

Instead, I think a better solution would be to run ALL of the hooks and then have these rules:

• If any of them returned False then the answer is False (a veto rule)
• Otherwise, if any returned True then the answer is True
• If all returned None (no opinion) then the answer is whatever the default is for that permission

[simonw]

While investigating this issue I created a new debug mechanism, see this issue:

• DATASETTE_TRACE_PLUGINS for tracing Pluggy calls #2274

[simonw]

I implemented the new pattern like so:

 diff --git a/datasette/app.py b/datasette/app.py
index d943b97b..5a54630c 100644
--- a/datasette/app.py
+++ b/datasette/app.py
@@ -903,6 +903,8 @@ class Datasette:
         # Use default from registered permission, if available
         if default is DEFAULT_NOT_SET and action in self.permissions:
             default = self.permissions[action].default
+        results = []
+        # Every plugin gets a go
         for check in pm.hook.permission_allowed(
             datasette=self,
             actor=actor,
@@ -911,7 +913,16 @@ class Datasette:
         ):
             check = await await_me_maybe(check)
             if check is not None:
-                result = check
+                results.append(check)
+
+        result = None
+        # If anyone said False it's false
+        if any(r is False for r in results):
+            result = False
+        elif any(r is True for r in results):
+            # Otherwise, if anyone said True it's true
+            result = True
+
         used_default = False
         if result is None:
             result = default

All of the tests pass with the exception of this one:

    def test_permissions_cascade(cascade_app_client, path, permissions, expected_status):
        # ...
>           assert (
                response.status == expected_status
            ), "path: {}, permissions: {}, expected_status: {}, status: {}".format(
                path, permissions, expected_status, response.status
            )
E           AssertionError: path: /fixtures.db, permissions: ['download'], expected_status: 200, status: 403
E           assert 403 == 200
E            +  where 403 = <datasette.utils.testing.TestResponse object at 0x107ec6e30>.status

/Users/simon/Dropbox/Development/datasette/tests/test_permissions.py:535: AssertionError
================================== short test summary info ==================================
FAILED tests/test_permissions.py::test_permissions_cascade[/fixtures.db-permissions26-200] - AssertionError: path: /fixtures.db, permissions: ['download'], expected_status: 200, sta...
============================= 1 failed, 711 deselected in 1.70s =============================

Here's the relevant source code for that test:

datasette/tests/test_permissions.py

Lines 506 to 541 in 232a304

	("/fixtures.db", ["download"], 200),
	],
	)
	def test_permissions_cascade(cascade_app_client, path, permissions, expected_status):
	"""Test that e.g. having view-table but NOT view-database lets you view table page, etc"""
	allow = {"id": "*"}
	deny = {}
	previous_config = cascade_app_client.ds.config
	updated_config = copy.deepcopy(previous_config)
	actor = {"id": "test"}
	if "download" in permissions:
	actor["can_download"] = 1
	try:
	# Set up the different allow blocks
	updated_config["allow"] = allow if "instance" in permissions else deny
	updated_config["databases"]["fixtures"]["allow"] = (
	allow if "database" in permissions else deny
	)
	updated_config["databases"]["fixtures"]["tables"]["binary_data"] = {
	"allow": (allow if "table" in permissions else deny)
	}
	updated_config["databases"]["fixtures"]["queries"]["magic_parameters"][
	"allow"
	] = (allow if "query" in permissions else deny)
	cascade_app_client.ds.config = updated_config
	response = cascade_app_client.get(
	path,
	cookies={"ds_actor": cascade_app_client.actor_cookie(actor)},
	)
	assert (
	response.status == expected_status
	), "path: {}, permissions: {}, expected_status: {}, status: {}".format(
	path, permissions, expected_status, response.status
	)
	finally:
	cascade_app_client.ds.config = previous_config

So this tests expects that if an actor has "can_download": 1 they should be able to download the database file - response.status should be 200, but the test fails because it comes back 403.

Here's the plugin that respects that can_download thing:

datasette/tests/plugins/my_plugin.py

Lines 211 to 218 in 232a304

	@hookimpl
	def permission_allowed(actor, action):
	if action == "this_is_allowed":
	return True
	elif action == "this_is_denied":
	return False
	elif action == "view-database-download":
	return actor.get("can_download") if actor else None

I don't fully understand this failure, in part because I don't understand how the configuration works that default-blocks the database download.

[simonw]

Using this new feature:

• DATASETTE_TRACE_PLUGINS for tracing Pluggy calls #2274

DATASETTE_TRACE_PLUGINS=1 just test --lf -x --pdb

Gives:

actor_from_request:
{   'datasette': <datasette.app.Datasette object at 0x10914a110>,
    'request': <asgi.Request method="GET" url="http://localhost/fixtures.db">}
Hook implementations:
[   <HookImpl plugin_name='datasette.actor_auth_cookie', plugin=<module 'datasette.actor_auth_cookie' from '/Users/simon/Dropbox/Development/datasette/datasette/actor_auth_cookie.py'>>,
    <HookImpl plugin_name='datasette.default_permissions', plugin=<module 'datasette.default_permissions' from '/Users/simon/Dropbox/Development/datasette/datasette/default_permissions.py'>>,
    <HookImpl plugin_name='my_plugin.py', plugin=<module 'my_plugin.py' from '/Users/simon/Dropbox/Development/datasette/tests/plugins/my_plugin.py'>>,
    <HookImpl plugin_name='my_plugin_2.py', plugin=<module 'my_plugin_2.py' from '/Users/simon/Dropbox/Development/datasette/tests/plugins/my_plugin_2.py'>>]
Results:
[   <function actor_from_request.<locals>.inner at 0x10941c280>,
    {'can_download': 1, 'id': 'test'}]

permission_allowed:
{   'action': 'view-database-download',
    'actor': {   'can_download': 1,
                 'id': 'test'},
    'datasette': <datasette.app.Datasette object at 0x10914a110>,
    'resource': 'fixtures'}
Hook implementations:
[   <HookImpl plugin_name='datasette.default_permissions', plugin=<module 'datasette.default_permissions' from '/Users/simon/Dropbox/Development/datasette/datasette/default_permissions.py'>>,
    <HookImpl plugin_name='my_plugin.py', plugin=<module 'my_plugin.py' from '/Users/simon/Dropbox/Development/datasette/tests/plugins/my_plugin.py'>>,
    <HookImpl plugin_name='my_plugin_2.py', plugin=<module 'my_plugin_2.py' from '/Users/simon/Dropbox/Development/datasette/tests/plugins/my_plugin_2.py'>>,
    <HookImpl plugin_name='datasette.default_permissions', plugin=<module 'datasette.default_permissions' from '/Users/simon/Dropbox/Development/datasette/datasette/default_permissions.py'>>]
Results:
[   <function permission_allowed_default.<locals>.inner at 0x10941c5e0>,
    <function permission_allowed.<locals>.inner at 0x10941c790>,
    1]

permission_allowed:
{   'action': 'view-database',
    'actor': {   'can_download': 1,
                 'id': 'test'},
    'datasette': <datasette.app.Datasette object at 0x10914a110>,
    'resource': 'fixtures'}
Hook implementations:
[   <HookImpl plugin_name='datasette.default_permissions', plugin=<module 'datasette.default_permissions' from '/Users/simon/Dropbox/Development/datasette/datasette/default_permissions.py'>>,
    <HookImpl plugin_name='my_plugin.py', plugin=<module 'my_plugin.py' from '/Users/simon/Dropbox/Development/datasette/tests/plugins/my_plugin.py'>>,
    <HookImpl plugin_name='my_plugin_2.py', plugin=<module 'my_plugin_2.py' from '/Users/simon/Dropbox/Development/datasette/tests/plugins/my_plugin_2.py'>>,
    <HookImpl plugin_name='datasette.default_permissions', plugin=<module 'datasette.default_permissions' from '/Users/simon/Dropbox/Development/datasette/datasette/default_permissions.py'>>]
Results:
[   <function permission_allowed_default.<locals>.inner at 0x10941c040>,
    <function permission_allowed.<locals>.inner at 0x10941c430>]

forbidden:
{   'datasette': <datasette.app.Datasette object at 0x10914a110>,
    'message': 'view-database',
    'request': <asgi.Request method="GET" url="http://localhost/fixtures.db">}
Hook implementations:
[   <HookImpl plugin_name='datasette.forbidden', plugin=<module 'datasette.forbidden' from '/Users/simon/Dropbox/Development/datasette/datasette/forbidden.py'>>,
    <HookImpl plugin_name='my_plugin.py', plugin=<module 'my_plugin.py' from '/Users/simon/Dropbox/Development/datasette/tests/plugins/my_plugin.py'>>]
Results:
[   <function forbidden.<locals>.inner at 0x10941c5e0>]

Which suggests to me that possibly permission_allowed is passing for view-database-download and then failing for view-database.

But I'd expect that to work because of the cascade here:

datasette/datasette/views/database.py

Lines 296 to 305 in 232a304

	async def database_download(request, datasette):
	database = tilde_decode(request.url_vars["database"])
	await datasette.ensure_permissions(
	request.actor,
	[
	("view-database-download", database),
	("view-database", database),
	"view-instance",
	],
	)

[simonw]

Weirdly in the debugger:

> /Users/simon/Dropbox/Development/datasette/datasette/app.py(972)ensure_permissions()
-> if ok is not None:
(Pdb) ok
(Pdb) actor
{'id': 'test', 'can_download': 1}
(Pdb) action
'view-database-download'
(Pdb) list
967  	                actor,
968  	                action,
969  	                resource=resource,
970  	                default=None,
971  	            )
972  ->	            if ok is not None:
973  	                if ok:
974  	                    return
975  	                else:
976  	                    raise Forbidden(action)
977  	
(Pdb) resource
'fixtures'
(Pdb) actor
{'id': 'test', 'can_download': 1}
(Pdb) action
'view-database-download'
(Pdb) type(ok)
<class 'NoneType'>

I would expect ok to be True there because the view-database-download check should have succeeded.

[simonw]

It failed because the plugin returned 1 but my code checked for any(r is True for r in results).

[simonw]

Updated documentation to explain this more clearly: https://docs.datasette.io/en/latest/authentication.html#how-permissions-are-resolved