check_visibility docs + used in more places, refs #1829

Closes #1847

datasette/app.py

@@ -736,7 +736,7 @@ async def check_visibility(
         self,
         actor: dict,
         action: Optional[str] = None,
-        resource: Optional[str] = None,
+        resource: Optional[Union[str, Tuple[str, str]]] = None,
         permissions: Optional[
             Sequence[Union[Tuple[str, Union[str, Tuple[str, str]]], str]]
         ] = None,

datasette/views/database.py

@@ -50,13 +50,6 @@ async def data(self, request, default_labels=False, _size=None):
         if not visible:
             raise Forbidden("You do not have permission to view this database")
 
-        await self.ds.ensure_permissions(
-            request.actor,
-            [
-                ("view-database", database),
-                "view-instance",
-            ],
-        )
         metadata = (self.ds.metadata("databases") or {}).get(database, {})
         self.ds.update_with_inherited_metadata(metadata)
 

datasette/views/index.py

@@ -23,25 +23,25 @@ async def get(self, request):
         await self.ds.ensure_permissions(request.actor, ["view-instance"])
         databases = []
         for name, db in self.ds.databases.items():
-            visible, database_private = await self.ds.check_visibility(
+            database_visible, database_private = await self.ds.check_visibility(
                 request.actor,
                 "view-database",
                 name,
             )
-            if not visible:
+            if not database_visible:
                 continue
             table_names = await db.table_names()
             hidden_table_names = set(await db.hidden_table_names())
 
             views = []
             for view_name in await db.view_names():
-                visible, private = await self.ds.check_visibility(
+                view_visible, view_private = await self.ds.check_visibility(
                     request.actor,
                     "view-table",
                     (name, view_name),
                 )
-                if visible:
-                    views.append({"name": view_name, "private": private})
+                if view_visible:
+                    views.append({"name": view_name, "private": view_private})
 
             # Perform counts only for immutable or DBS with <= COUNT_TABLE_LIMIT tables
             table_counts = {}

docs/internals.rst

@@ -349,7 +349,7 @@ await .ensure_permissions(actor, permissions)
 ``permissions`` - list
     A list of permissions to check. Each permission in that list can be a string ``action`` name or a 2-tuple of ``(action, resource)``.
 
-This method allows multiple permissions to be checked at onced. It raises a ``datasette.Forbidden`` exception if any of the checks are denied before one of them is explicitly granted.
+This method allows multiple permissions to be checked at once. It raises a ``datasette.Forbidden`` exception if any of the checks are denied before one of them is explicitly granted.
 
 This is useful when you need to check multiple permissions at once. For example, an actor should be able to view a table if either one of the following checks returns ``True`` or not a single one of them returns ``False``:
 
@@ -366,18 +366,21 @@ This is useful when you need to check multiple permissions at once. For example,
 
 .. _datasette_check_visibilty:
 
-await .check_visibility(actor, action, resource=None)
------------------------------------------------------
+await .check_visibility(actor, action=None, resource=None, permissions=None)
+----------------------------------------------------------------------------
 
 ``actor`` - dictionary
     The authenticated actor. This is usually ``request.actor``.
 
-``action`` - string
+``action`` - string, optional
     The name of the action that is being permission checked.
 
 ``resource`` - string or tuple, optional
     The resource, e.g. the name of the database, or a tuple of two strings containing the name of the database and the name of the table. Only some permissions apply to a resource.
 
+``permissions`` - list of ``action`` strings or ``(action, resource)`` tuples, optional
+    Provide this instead of ``action`` and ``resource`` to check multiple permissions at once.
+
 This convenience method can be used to answer the question "should this item be considered private, in that it is visible to me but it is not visible to anonymous users?"
 
 It returns a tuple of two booleans, ``(visible, private)``. ``visible`` indicates if the actor can see this resource. ``private`` will be ``True`` if an anonymous user would not be able to view the resource.
@@ -387,7 +390,20 @@ This example checks if the user can access a specific table, and sets ``private`
 .. code-block:: python
 
     visible, private = await self.ds.check_visibility(
-        request.actor, "view-table", (database, table)
+        request.actor, action="view-table", resource=(database, table)
+    )
+
+The following example runs three checks in a row, similar to :ref:`datasette_ensure_permissions`. If any of the checks are denied before one of them is explicitly granted then ``visible`` will be ``False``. ``private`` will be ``True`` if an anonymous user would not be able to view the resource.
+
+.. code-block:: python
+
+    visible, private = await self.ds.check_visibility(
+        request.actor,
+        permissions=[
+            ("view-table", (database, table)),
+            ("view-database", database),
+            "view-instance",
+        ],
     )
 
 .. _datasette_get_database:

tests/test_internals_datasette.py

@@ -84,28 +84,44 @@ async def test_num_sql_threads_zero():
 
 @pytest.mark.asyncio
 @pytest.mark.parametrize(
-    "actor,metadata,permissions,should_allow",
+    "actor,metadata,permissions,should_allow,expected_private",
     (
-        (None, ALLOW_ROOT, ["view-instance"], False),
-        (ROOT, ALLOW_ROOT, ["view-instance"], True),
+        (None, ALLOW_ROOT, ["view-instance"], False, False),
+        (ROOT, ALLOW_ROOT, ["view-instance"], True, True),
         (
             None,
             {"databases": {"_memory": ALLOW_ROOT}},
             [("view-database", "_memory")],
             False,
+            False,
         ),
         (
             ROOT,
             {"databases": {"_memory": ALLOW_ROOT}},
             [("view-database", "_memory")],
             True,
+            True,
+        ),
+        # Check private is false for non-protected instance check
+        (
+            ROOT,
+            {"allow": True},
+            ["view-instance"],
+            True,
+            False,
         ),
     ),
 )
-async def test_datasette_ensure_permissions(actor, metadata, permissions, should_allow):
+async def test_datasette_ensure_permissions_check_visibility(
+    actor, metadata, permissions, should_allow, expected_private
+):
     ds = Datasette([], memory=True, metadata=metadata)
     if not should_allow:
         with pytest.raises(Forbidden):
             await ds.ensure_permissions(actor, permissions)
     else:
         await ds.ensure_permissions(actor, permissions)
+    # And try check_visibility too:
+    visible, private = await ds.check_visibility(actor, permissions=permissions)
+    assert visible == should_allow
+    assert private == expected_private

tests/test_permissions.py

@@ -482,11 +482,15 @@ def test_permissions_cascade(cascade_app_client, path, permissions, expected_sta
         updated_metadata["databases"]["fixtures"]["queries"]["magic_parameters"][
             "allow"
         ] = (allow if "query" in permissions else deny)
-        cascade_app_client.ds._local_metadata = updated_metadata
+        cascade_app_client.ds._metadata_local = updated_metadata
         response = cascade_app_client.get(
             path,
             cookies={"ds_actor": cascade_app_client.actor_cookie(actor)},
         )
-        assert expected_status == response.status
+        assert (
+            response.status == expected_status
+        ), "path: {}, permissions: {}, expected_status: {}, status: {}".format(
+            path, permissions, expected_status, response.status
+        )
     finally:
         cascade_app_client.ds._local_metadata = previous_metadata

tests/test_plugins.py

@@ -823,8 +823,14 @@ def test_hook_forbidden(restore_working_directory):
         assert 403 == response.status
         response2 = client.get("/data2")
         assert 302 == response2.status
-        assert "/login?message=view-database" == response2.headers["Location"]
-        assert "view-database" == client.ds._last_forbidden_message
+        assert (
+            response2.headers["Location"]
+            == "/login?message=You do not have permission to view this database"
+        )
+        assert (
+            client.ds._last_forbidden_message
+            == "You do not have permission to view this database"
+        )
 
 
 def test_hook_handle_exception(app_client):