Cascade for restricted token view-table/view-database/view-instance operations by simonw · Pull Request #2154 · simonw/datasette

simonw · 2023-08-24T14:24:23Z

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/

Also includes a prototype implementation of --actor option from #2153 which I'm using for testing this.

simonw · 2023-08-24T14:25:56Z

Can be tested with:

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

codecov · 2023-08-24T14:31:42Z

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 · 2023-08-24T14:57:39Z

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)

Refs #2154

simonw · 2023-08-29T00:01:50Z

datasette/default_permissions.py

+    # 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
+


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

)

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

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.

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.

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

Refs #2154

simonw · 2023-08-29T00:58:34Z

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 · 2023-08-29T01:35:37Z

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 · 2023-08-29T01:37:47Z

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 · 2023-08-29T14:57:54Z

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 · 2023-08-29T15:54:12Z

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.

Experimental fix for token restriction bug in #2102

6d57a8c

Also includes a prototype implementation of --actor option from #2153 which I'm using for testing this.

simonw added a commit that referenced this pull request Aug 28, 2023

datasette --get --actor 'JSON' option, closes #2153

d8351b0

Refs #2154

simonw added 2 commits August 28, 2023 16:54

Tests and fixes for #2102

6fca0e8

Fixed test I broke, refs #2102

51965c5

simonw commented Aug 29, 2023

View reviewed changes

simonw added 2 commits August 28, 2023 17:05

Merge branch 'main' into issue-2102

fb21e1e

Permission is now a dataclass, not a namedtuple

d64a989

Refs #2154

Refactored into restrictions_allow_action()

49c1d74

simonw linked an issue Aug 29, 2023 that may be closed by this pull request

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

Closed

simonw added 2 commits August 29, 2023 09:04

datasette.get_permission() method

3e49fd3

Refactored logic to use implies_can_view

1b38cba

simonw marked this pull request as ready for review August 29, 2023 16:32

simonw merged commit 50da908 into main Aug 29, 2023

simonw deleted the issue-2102 branch August 29, 2023 16:32

	@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),
	)

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

Uh oh!

Conversation

simonw commented Aug 24, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonw commented Aug 24, 2023

Uh oh!

codecov bot commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

simonw commented Aug 24, 2023

Uh oh!

simonw Aug 29, 2023

Choose a reason for hiding this comment

Uh oh!

simonw Aug 29, 2023

Choose a reason for hiding this comment

Uh oh!

simonw Aug 29, 2023

Choose a reason for hiding this comment

Uh oh!

simonw Aug 29, 2023

Choose a reason for hiding this comment

Uh oh!

simonw Aug 29, 2023

Choose a reason for hiding this comment

Uh oh!

simonw commented Aug 29, 2023

Uh oh!

simonw commented Aug 29, 2023

Uh oh!

simonw commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonw commented Aug 29, 2023

Uh oh!

simonw commented Aug 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

simonw commented Aug 24, 2023 •

edited by github-actions bot

Loading

codecov bot commented Aug 24, 2023 •

edited

Loading

simonw commented Aug 29, 2023 •

edited

Loading