Table/database that is private due to inherited permissions does not show padlock

[simonw]

I noticed that a table page that is private because the database or instance is private, e.g. this one:

Is not displaying the padlock icon that indicates the table is not visible to the public.

Same issue for the database page too, which in this case is private due to view-instance.

[simonw]

Here's the relevant code from the table page:

datasette/datasette/views/table.py

Lines 215 to 227 in 4218c9c

	# Ensure user has permission to view this table
	await self.ds.ensure_permissions(
	request.actor,
	[
	("view-table", (database_name, table_name)),
	("view-database", database_name),
	"view-instance",
	],
	)
	private = not await self.ds.permission_allowed(
	None, "view-table", (database_name, table_name), default=True
	)

Note how ensure_permissions() there takes the table, database and instance into account... but the private assignment (used to decide if the padlock should display or not) only considers the view-table check.

Here's the same code for the database page:

datasette/datasette/views/database.py

Lines 139 to 141 in 4218c9c

	"private": not await self.ds.permission_allowed(
	None, "view-database", database, default=True
	),

And for canned query pages:

datasette/datasette/views/database.py

Lines 228 to 240 in 4218c9c

	if canned_query:
	# Respect canned query permissions
	await self.ds.ensure_permissions(
	request.actor,
	[
	("view-query", (database, canned_query)),
	("view-database", database),
	"view-instance",
	],
	)
	private = not await self.ds.permission_allowed(
	None, "view-query", (database, canned_query), default=True
	)

[simonw]

There's also a check_visibility() helper which I'm not using in these particular cases but which may be relevant. It's called like this:

datasette/datasette/views/database.py

Lines 65 to 77 in 4218c9c

	for view_name in await db.view_names():
	visible, private = await self.ds.check_visibility(
	request.actor,
	"view-table",
	(database, view_name),
	)
	if visible:
	views.append(
	{
	"name": view_name,
	"private": private,
	}
	)

And is defined here:

datasette/datasette/app.py

Lines 694 to 710 in 4218c9c

	async def check_visibility(self, actor, action, resource):
	"""Returns (visible, private) - visible = can you see it, private = can others see it too"""
	visible = await self.permission_allowed(
	actor,
	action,
	resource=resource,
	default=True,
	)
	if not visible:
	return False, False
	private = not await self.permission_allowed(
	None,
	action,
	resource=resource,
	default=True,
	)
	return visible, private

It's actually documented as a public method here: https://docs.datasette.io/en/stable/internals.html#await-check-visibility-actor-action-resource-none

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.

Note that this documented method cannot actually do the right thing - because it's not being given the multiple permissions that need to be checked in order to completely answer the question.

So I probably need to redesign that method a bit.

[simonw]

I think check_visibility should be changed to optionally accept permissions= which is the same list of tuples that can be passed to ensure_permissions.

[simonw]

Here's what I've got so far:

diff --git a/datasette/app.py b/datasette/app.py
index 5fa4955c..df9eae49 100644
--- a/datasette/app.py
+++ b/datasette/app.py
@@ -1,5 +1,5 @@
 import asyncio
-from typing import Sequence, Union, Tuple
+from typing import Sequence, Union, Tuple, Optional
 import asgi_csrf
 import collections
 import datetime
@@ -707,7 +707,7 @@ class Datasette:
 
         Raises datasette.Forbidden() if any of the checks fail
         """
-        assert actor is None or isinstance(actor, dict)
+        assert actor is None or isinstance(actor, dict), "actor must be None or a dict"
         for permission in permissions:
             if isinstance(permission, str):
                 action = permission
@@ -732,23 +732,34 @@ class Datasette:
                 else:
                     raise Forbidden(action)
 
-    async def check_visibility(self, actor, action, resource):
+    async def check_visibility(
+        self,
+        actor: dict,
+        action: Optional[str] = None,
+        resource: Optional[str] = None,
+        permissions: Optional[
+            Sequence[Union[Tuple[str, Union[str, Tuple[str, str]]], str]]
+        ] = None,
+    ):
         """Returns (visible, private) - visible = can you see it, private = can others see it too"""
-        visible = await self.permission_allowed(
-            actor,
-            action,
-            resource=resource,
-            default=True,
-        )
-        if not visible:
+        if permissions:
+            assert (
+                not action and not resource
+            ), "Can't use action= or resource= with permissions="
+        else:
+            permissions = [(action, resource)]
+        try:
+            await self.ensure_permissions(actor, permissions)
+        except Forbidden:
             return False, False
-        private = not await self.permission_allowed(
-            None,
-            action,
-            resource=resource,
-            default=True,
-        )
-        return visible, private
+        # User can see it, but can the anonymous user see it?
+        try:
+            await self.ensure_permissions(None, permissions)
+        except Forbidden:
+            # It's visible but private
+            return True, True
+        # It's visible to everyone
+        return True, False
 
     async def execute(
         self,
diff --git a/datasette/views/table.py b/datasette/views/table.py
index 60c092f9..f73b0957 100644
--- a/datasette/views/table.py
+++ b/datasette/views/table.py
@@ -28,7 +28,7 @@ from datasette.utils import (
     urlsafe_components,
     value_as_boolean,
 )
-from datasette.utils.asgi import BadRequest, NotFound
+from datasette.utils.asgi import BadRequest, Forbidden, NotFound
 from datasette.filters import Filters
 from .base import DataView, DatasetteError, ureg
 from .database import QueryView
@@ -213,18 +213,16 @@ class TableView(DataView):
             raise NotFound(f"Table not found: {table_name}")
 
         # Ensure user has permission to view this table
-        await self.ds.ensure_permissions(
+        visible, private = await self.ds.check_visibility(
             request.actor,
-            [
+            permissions=[
                 ("view-table", (database_name, table_name)),
                 ("view-database", database_name),
                 "view-instance",
             ],
         )
-
-        private = not await self.ds.permission_allowed(
-            None, "view-table", (database_name, table_name), default=True
-        )
+        if not visible:
+            raise Forbidden("You do not have permission to view this table")
 
         # Handle ?_filter_column and redirect, if present
         redirect_params = filters_should_redirect(request.args)

Still needs tests and a documentation update.

Also this fix is currently only applied on the table page - needs to be applied on database, row and query pages too.

[simonw]

I'll finish this in a PR:

• check_visibility can now take multiple permissions into account #1842

[simonw]

Useful test: if you sign in as root to https://latest.datasette.io/_internal/columns/_internal,columns,database_name you can see there's no padlock icon on that page or on https://latest.datasette.io/_internal/columns - fixing this bug should fix that.

[simonw]

Updated docs for check_visibility(): https://docs.datasette.io/en/latest/internals.html#await-check-visibility-actor-action-none-resource-none-permissions-none

[simonw]

Visit https://latest.datasette.io/login-as-root and then:

https://latest.datasette.io/

https://latest.datasette.io/_internal/columns

https://latest.datasette.io/_internal/columns/_internal,columns,cid

https://latest.datasette.io/_internal/from_hook

That's all as it should be.