register_permissions() plugin hook

[simonw]

Refs #1939

From this comment: #1939 (comment)

• Unit test for the registration plugin hook itself
• Use them in check_permission_actions_are_documented test in conftest.py
• Add description field to Permissions (and update tests and docs)
• Documentation for datasette.permissions dictionary
• If no default= provided in call to permission_allowed() then use default from datasette.permissions list
• Remove default= from a bunch of places
• Throw an error if two permissions are registered with the same name or abbreviation (but other attributes differ)
• Update authentication and permissions documentation to explain that permissions are now registered and have a registered default

---

📚 Documentation preview 📚: https://datasette--1940.org.readthedocs.build/en/1940/

[simonw]

Here's my first test failure:

tests/test_permissions.py .......F
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

allow = {}, expected_anon = 403, expected_auth = 403, path = '/fixtures/compound_three_primary_keys'
padlock_client = <datasette.utils.testing.TestClient object at 0x107c09d20>

    @pytest.mark.parametrize(
        "allow,expected_anon,expected_auth",
        [
            (None, 200, 200),
            ({}, 403, 403),
            ({"id": "root"}, 403, 200),
        ],
    )
    @pytest.mark.parametrize(
        "path",
        (
            "/",
            "/fixtures",
            "/fixtures/compound_three_primary_keys",
            "/fixtures/compound_three_primary_keys/a,a,a",
            "/fixtures/two",  # Query
        ),
    )
    def test_view_padlock(allow, expected_anon, expected_auth, path, padlock_client):
        padlock_client.ds._metadata_local["allow"] = allow
        fragment = "🔒</h1>"
        anon_response = padlock_client.get(path)
>       assert expected_anon == anon_response.status
E       assert 403 == 200
E        +  where 200 = <datasette.utils.testing.TestResponse object at 0x107aea680>.status

/Users/simon/Dropbox/Development/datasette/tests/test_permissions.py:61: AssertionError
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> entering PDB >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PDB post_mortem (IO-capturing turned off) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> /Users/simon/Dropbox/Development/datasette/tests/test_permissions.py(61)test_view_padlock()
-> assert expected_anon == anon_response.status
(Pdb) anon_response
<datasette.utils.testing.TestResponse object at 0x107aea680>
(Pdb) anon_response.status
200
(Pdb) path
'/fixtures/compound_three_primary_keys'
(Pdb) padlock_client.ds._metadata_
*** AttributeError: 'Datasette' object has no attribute '_metadata_'
(Pdb) padlock_client.ds._metadata_local
{'databases': {'fixtures': {'queries': {'two': {'sql': 'select 1 + 1', 'name': 'two'}, 'from_async_hook': {'sql': 'select 2', 'name': 'from_async_hook'}, 'from_hook': {'sql': "select 1, 'null' as actor_id", 'name': 'from_hook'}}, 'source': None, 'source_url': None, 'license': None, 'license_url': None, 'about': None, 'about_url': None}}, 'allow': {}}
(Pdb) allow
{}

It looks like I've broken the allow logic that notices that if there's an "allow": {} on the root then anonymous users should not be allowed to view any pages.

[simonw]

I'm going to revert that last commit, see if I can get the tests running again and then apply the changes a line at a time to figure out which ones broke things.

[simonw]

It's this change which triggers the failures:

diff --git a/datasette/app.py b/datasette/app.py
index 760063d5..defa9688 100644
--- a/datasette/app.py
+++ b/datasette/app.py
@@ -707,9 +707,12 @@ class Datasette:
                 )
         return crumbs
 
-    async def permission_allowed(self, actor, action, resource=None, default=False):
+    async def permission_allowed(self, actor, action, resource=None, default=None):
         """Check permissions using the permissions_allowed plugin hook"""
         result = None
+        # Use default from registered permission, if available
+        if default is None and action in self.permissions:
+            default = self.permissions[action].default
         for check in pm.hook.permission_allowed(
             datasette=self,
             actor=actor,

[codecov]

Codecov Report

Base: 92.00% // Head: 92.03% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (a1317ab) compared to base (e539c1c).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head a1317ab differs from pull request most recent head 94e5c75. Consider uploading reports for the commit 94e5c75 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1940      +/-   ##
==========================================
+ Coverage   92.00%   92.03%   +0.02%     
==========================================
  Files          38       38              
  Lines        5378     5396      +18     
==========================================
+ Hits         4948     4966      +18     
  Misses        430      430              

Impacted Files	Coverage Δ
datasette/permissions.py	100.00% <ø> (ø)
datasette/views/database.py	96.26% <ø> (ø)
datasette/views/index.py	96.49% <ø> (ø)
datasette/views/special.py	79.20% <ø> (-0.21%)	⬇️
datasette/views/table.py	92.57% <ø> (ø)
datasette/__init__.py	100.00% <100.00%> (ø)
datasette/app.py	94.47% <100.00%> (+0.04%)	⬆️
datasette/default_permissions.py	95.20% <100.00%> (+0.39%)	⬆️
datasette/hookspecs.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[simonw]

Actually one last thing: I said that the error would only occur if the permissions differed in some way.

[simonw]

This PR ended up bundling part of the implementation of:

• "permissions" propery in metadata for configuring arbitrary permissions #1636

I'm going to be bad an NOT untangle that from this before I merge it.