Cascade for restricted token view-table/view-database/view-instance o…

…perations (#2154)

Closes #2102

* Permission is now a dataclass, not a namedtuple - refs #2154
* datasette.get_permission() method

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

datasette/default_permissions.py

@@ -2,39 +2,120 @@
 from datasette.utils import actor_matches_allow
 import itsdangerous
 import time
+from typing import Union, Tuple
 
 
 @hookimpl
 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,50 +257,96 @@ 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}
     permission = datasette.permissions.get(action)
     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_"

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

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()
+                ],
             },
         )
 

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 <plugin_register_permissions>`.
+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 <plugin_register_permissions>`.
 
 .. _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 <plugin_register_permissions>` representing the permission, or raises a ``KeyError`` if one is not found.
+
 .. _datasette_get_database:
 
 .get_database(name)

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 <canned_queries>`.
 
-``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.

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