Cascade for restricted token view-table/view-database/view-instance operations

[simonw]

Refs:

• API tokens with view-table but not view-database/view-instance cannot access the table #2102

Also includes a prototype implementation of --actor option which I'm using for testing this, from:

• Datasette --get --actor option #2153

---

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

[simonw]

Can be tested with:

pip install https://github.com/simonw/datasette/archive/6d57a8c23043e99b27f7a2afbe58f4d58815fd51.zip

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03% 🎉

Comparison is base (2e28258) 92.82% compared to head (3e49fd3) 92.85%.
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2154      +/-   ##
==========================================
+ Coverage   92.82%   92.85%   +0.03%     
==========================================
  Files          40       40              
  Lines        5948     6008      +60     
==========================================
+ Hits         5521     5579      +58     
- Misses        427      429       +2     

Files Changed	Coverage Δ
datasette/views/special.py	94.06% <ø> (-0.85%)	⬇️
datasette/app.py	94.37% <100.00%> (+0.07%)	⬆️
datasette/default_permissions.py	97.48% <100.00%> (+0.57%)	⬆️
datasette/permissions.py	100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[simonw]

Notes on manual testing so far - it looks like this might be working!

• API tokens with view-table but not view-database/view-instance cannot access the table #2102 (comment)

[simonw]

This code is pretty complicated. Maybe it would be simple and better if the fact that a permission cascades like this was modeled on the permission classes here instead:

datasette/datasette/default_permissions.py

Lines 7 to 38 in 2e28258

	@hookimpl
	def register_permissions():
	return (
	# name, abbr, description, takes_database, takes_resource, default
	Permission(
	"view-instance", "vi", "View Datasette instance", False, False, True
	),
	Permission("view-database", "vd", "View database", True, False, True),
	Permission(
	"view-database-download", "vdd", "Download database file", True, False, 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
	),
	Permission(
	"permissions-debug",
	"pd",
	"Access permission debug tool",
	False,
	False,
	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),
	)

[simonw]

Permissions is currently a namedtuple:

datasette/datasette/permissions.py

Lines 3 to 6 in 2e28258

	Permission = collections.namedtuple(
	"Permission",
	("name", "abbr", "description", "takes_database", "takes_resource", "default"),
	)

It's used by a few plugins too (post-1.0 alpha) via this plugin hook: https://docs.datasette.io/en/1.0a4/plugin_hooks.html#register-permissions-datasette

[simonw]

I could add a implies_can_view=False property to that - if it's true, then it means that having this permission implies that you can view the database (if the permission takes_database) and the instance.

[simonw]

Or... I could have a special subclass of Permission called PermissionImpliesView which is reserved for use by core, and isn't something that's documented for plugins to register their own.

[simonw]

I'm going to upgrade Permission from a named tuple to a dataclass.

[simonw]

Should this have implies_can_view=True too? Probably:

datasette/datasette/default_permissions.py

Lines 20 to 22 in d64a989

	Permission(
	"execute-sql", "es", "Execute read-only SQL queries", True, False, True
	),

[simonw]

Reminder that I also need to confirm that insert-row works if you have it at the instance level, the database level or the resource level in _r.

[simonw]

Code for this might be cleaner with a Restrictions() class that takes a "_r" dictionary to the constructor and can then answer questions like .any_resource_has_permission("view-table") - where it can resolve aliases etc as well.

[simonw]

The code to refactor is this:

datasette/datasette/default_permissions.py

Lines 181 to 280 in d64a989

	@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")
	# Special case for view-instance: it's allowed if there are any view-database
	# or view-table permissions defined
	if action == "view-instance":
	all_rules = _r.get("a") or []
	if (
	"view-database" in all_rules
	or "vd" in all_rules
	or "view-table" in all_rules
	or "vt" in all_rules
	):
	return None
	database_rules = _r.get("d") or {}
	for rules in database_rules.values():
	if (
	"vd" in rules
	or "view-database" in rules
	or "vt" in rules
	or "view-table" in rules
	):
	return None
	# Now check resources
	resource_rules = _r.get("r") or {}
	for _database, resources in resource_rules.items():
	for rules in resources.values():
	if "vt" in rules or "view-table" in rules:
	return None
	# Special case for view-database: it's allowed if there are any view-table permissions
	# defined within that database
	if action == "view-database":
	database_name = resource
	all_rules = _r.get("a") or []
	if (
	"view-database" in all_rules
	or "vd" in all_rules
	or "view-table" in all_rules
	or "vt" in all_rules
	):
	return None
	database_rules = (_r.get("d") or {}).get(database_name) or {}
	if "vt" in database_rules or "view-table" in database_rules:
	return None
	resource_rules = _r.get("r") or {}
	resources_in_database = resource_rules.get(database_name) or {}
	for rules in resources_in_database.values():
	if "vt" in rules or "view-table" in rules:
	return None
	if action == "view-table":
	# Can view table if they have view-table in database or instance too
	database_name = resource[0]
	all_rules = _r.get("a") or []
	if "view-table" in all_rules or "vt" in all_rules:
	return None
	database_rules = (_r.get("d") or {}).get(database_name) or {}
	if "vt" in database_rules or "view-table" in database_rules:
	return None
	# 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
	# 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")
	if all_allowed is not None:
	assert isinstance(all_allowed, list)
	if to_check.intersection(all_allowed):
	return None
	# How about for the current database?
	if isinstance(resource, str):
	database_allowed = _r.get("d", {}).get(resource)
	if database_allowed is not None:
	assert isinstance(database_allowed, list)
	if to_check.intersection(database_allowed):
	return None
	# 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)
	# 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
	# This action is not specifically allowed, so reject it
	return False

I'm going to turn that into a more general restrictions_allow_action function.

[simonw]

In that last commit I also upgraded Permission from a named tuple to a dataclass, and added a implies_can_view=True private (not documented) option to it.

Last step is to refactor the code to use that new property.