Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify permissions terminology around "resource" #2457

Open
Tracked by #2456
simonw opened this issue Jan 16, 2025 · 9 comments
Open
Tracked by #2456

Clarify permissions terminology around "resource" #2457

simonw opened this issue Jan 16, 2025 · 9 comments

Comments

@simonw
Copy link
Owner

simonw commented Jan 16, 2025

The term "resource" is not consistently applied within Datasette's permissions code at the moment.

https://github.com/simonw/datasette/blob/d9a450b19797e89d70aec70406ed595d9f14dcc1/docs/authentication.rst defines our permissions model clearly:

Is this actor allowed to perform this action, optionally against this particular resource?

But elsewhere a "resource" is a thing that's either a table or a canned query that lives within a database, e.g. here:

@dataclass
class Permission:
name: str
abbr: Optional[str]
description: Optional[str]
takes_database: bool
takes_resource: bool
default: bool

The unclear thing is whether a database itself is a resource or not?

@simonw
Copy link
Owner Author

simonw commented Jan 16, 2025

There are a few other things to consider here. Currently every Datasette permission is either attached to the whole instance, a database or a resource (in the table-or-canned-query definition) within a database.

But... it's increasingly likely there may be other things that do NOT belong to a database that still need permissions applied to them, for example:

  • Individual enrichments - maybe only some users have permission to run the QuickJS one for example
  • Groups in the datasette-acl world - who is allowed to add and remove members from a specific group?
  • Named dashboards (if we build those in a plugin)
  • LLMs - perhaps the expensive o1 model is available to a limited number of people?

On that basis, I think "resource" SHOULD be a term that can mean database or table or canned query or group or dashboard or ...

As such, the design of that Permission class looks incorrect. The question isn't "does this permission apply to a resource or a database", it's "which type of resource does this permission apply to"?

@simonw
Copy link
Owner Author

simonw commented Jan 16, 2025

It's a bit more complicated than that. The problem is that some resources are children of databases, such that granting e.g. insert-row permission at the database level allows that user to apply that action to every child-resource of that specified database.

@simonw
Copy link
Owner Author

simonw commented Jan 16, 2025

If I do change the definition of Permission I think I do it as a hard break in one of the 1.0 alphas, such that I can be sure that all plugins are upgraded to the new shape before the 1.0 release.

@simonw
Copy link
Owner Author

simonw commented Jan 16, 2025

The most important thing about resources is that they need to fit the permissions system at the database level. datasette-acl for example needs to record the resources that permissions have been assigned to - it currently does that with a _internal/acl_resources table that looks like this:

id database resource
1 data people
2 data incidents

If I introduce the concept of a "resource type" maybe that table looks like this instead:

id type resource
1 table data/people
2 table data/incidents
3 database data

Storing tables as databasename/table might be a good way to handle the concept of "child" resources - as a pure naming convention. I can use tilde-encoding just in case the database name or the table name include the / character.

@simonw
Copy link
Owner Author

simonw commented Jan 16, 2025

The biggest challenge here then is how to cleanly implement permission checks that say "if the user has insert-row on the data/incidents table OR has insert-row on the data database" - how many places would need to implement checks like that?

It also relates to this:

I'll need to be sure there's a clean, efficient way to say "list all resources that this user has view-table on or for which they have view-database on the parent database and there's no special thing that says they shouldn't be able to view a certain table".

@simonw
Copy link
Owner Author

simonw commented Jan 16, 2025

It's pretty clear to me that "resource" should mean a database or a table or a query or another type of thing provided by a plugin - so that special treatment for database needs to be cleaned up.

But at the same time, the fact that some permissions when applied to databases are inherited by their "child" resources does need to be clarified, both in documentation and at the implementation level.

I think the best way to clarify that would be with a development spike that covers both Datasette and datasette-acl, with the goal of implementing efficient view permissions for the list of tables to be shown on the homepage:

@simonw
Copy link
Owner Author

simonw commented Jan 16, 2025

How about if Permission looked like this instead?

 class Permission: 
     name: str 
     abbr: Optional[str] 
     description: Optional[str] 
     resources: Tuple[str, ...]

Then a permission might look like this:

Permission(
    name="view-table",
    abbr="vt",
    description="View table",
    resources=("database", "table"),
    default=True,
    implies_can_view=True,
)

@simonw
Copy link
Owner Author

simonw commented Jan 16, 2025

Maybe a register_resource_types() plugin hook is called for here?

@simonw
Copy link
Owner Author

simonw commented Jan 16, 2025

I like the idea of registering resource classes - maybe with a naming convention like DatabaseResource etc to differentiate from existing Database class.

Then could do this:

Permission(
    name="view-table",
    abbr="vt",
    description="View table",
    resources=(InstanceResource, DatabaseResource, TableResource),
    default=True,
    implies_can_view=True,
)

But when checking permissions could pass instances of them, like DatabaseResource("data") or TableResource("data", "species").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant