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

Ability to manage global permissions #10

Open
simonw opened this issue Aug 31, 2024 · 11 comments
Open

Ability to manage global permissions #10

simonw opened this issue Aug 31, 2024 · 11 comments
Labels
enhancement New feature or request

Comments

@simonw
Copy link
Contributor

simonw commented Aug 31, 2024

Permissions not associated with a resource, like the datasette-acl permission itself.

@simonw simonw added the enhancement New feature or request label Aug 31, 2024
@simonw simonw added this to the Feature complete milestone Aug 31, 2024
@simonw
Copy link
Contributor Author

simonw commented Sep 1, 2024

The UI for this is a bit of a challenge.

The grid I use for the table only really works there because there aren't many table permissions:

CleanShot 2024-08-31 at 21 28 39@2x

Once we get to global permissions there are a whole lot more - and plugins can register their own too.

I'm inclined to say each global permission should get its own dedicated page, but in that case a grid of checkboxes doesn't feel right.

@simonw
Copy link
Contributor Author

simonw commented Sep 1, 2024

Here's our permission_allowed plugin hook implementation at the moment - it only triggers for permission checks with a (database, resource) tuple:

@hookimpl
def permission_allowed(datasette, actor, action, resource):
if not resource or len(resource) != 2:
return None
async def inner():
if not actor:
return None
await update_dynamic_groups(
datasette, actor, skip_cache=hasattr(sys, "_pytest_running")
)
db = datasette.get_internal_database()
result = await db.execute(
ACL_RESOURCE_PAIR_SQL,
{
"actor_id": actor["id"],
"database": resource[0],
"resource": resource[1],
"action": action,
},
)
return result.single_value() or None
return inner

@simonw
Copy link
Contributor Author

simonw commented Sep 19, 2024

The UI challenge is solved now that I'm using <select multiple> instead:

@simonw
Copy link
Contributor Author

simonw commented Sep 19, 2024

Which actions should apply globally? A couple of options:

  • ALL actions can be applied globally - if you want to give insert-row to EVERY staff user across all attached databases, go right ahead
  • Only global actions can be applied globally - permissions like insert-row can be applied at the database level (Ability to manage database-level permissions #11) or table level only

I'm going to go for the second option for the moment, I think having actions at the global level may be too confusing.

But... I may actually implement support for global permissions like insert-row at the database level anyway, to see how they feel.

UPDATE: I decided to go with option one instead, since that's what datasette-auth-tokens does.

@simonw
Copy link
Contributor Author

simonw commented Sep 19, 2024

An interesting thing to consider here is what happens with permissions that are added by other plugins using the https://docs.datasette.io/en/latest/plugin_hooks.html#register-permissions-datasette hook - reminder, that looks like this:

@hookimpl
def register_permissions(datasette):
    return [
        Permission(
            name="upload-csvs",
            abbr=None,
            description="Upload CSV files",
            takes_database=True,
            takes_resource=False,
            default=False,
        )
    ]

@simonw
Copy link
Contributor Author

simonw commented Sep 19, 2024

Here's what the UI for datasette-auth-tokens looks like on an instance with a bunch of plugins installed that added more permissions:

CleanShot 2024-09-19 at 15 45 28@2x

@simonw
Copy link
Contributor Author

simonw commented Sep 19, 2024

Since that plugin already decided that all actions can be applied globally, I should have this plugin make the same decision for consistency.

@simonw
Copy link
Contributor Author

simonw commented Sep 19, 2024

... but I might NOT show the ones that default to True (like view-table) since that's confusing, since users can do those things even if you don't issue them those permissions.

@simonw
Copy link
Contributor Author

simonw commented Sep 19, 2024

Here's the code in datasette-auth-tokens that figures out what those are: https://github.com/simonw/datasette-auth-tokens/blob/8e2907e0321e8f29b9bdf402e54ef2c86944fb38/datasette_auth_tokens/views.py#L163C1-L183C11

        "all_permissions": [
            {"name": key, "description": value.description}
            for key, value in datasette.permissions.items()
            if key
            not in (
                "auth-tokens-create",
                "auth-tokens-revoke-all",
                "debug-menu",
                "permissions-debug",
            )
        ],
        "database_permissions": [
            {"name": key, "description": value.description}
            for key, value in datasette.permissions.items()
            if value.takes_database
        ],
        "resource_permissions": [
            {"name": key, "description": value.description}
            for key, value in datasette.permissions.items()
            if value.takes_resource
        ],

@simonw
Copy link
Contributor Author

simonw commented Sep 19, 2024

New SQL query:

with actor_groups as (
  select group_id
  from acl_actor_groups
  where actor_id = :actor_id
),
target_resources as (
  select id
  from acl_resources
  where (database = :database and resource = :resource)
    or (database = :database and resource = '')
    or (database = '' and resource = '')
),
target_action as (
  select id
  from acl_actions
  where name = :action
),
combined_permissions as (
    select resource_id, action_id
    from acl
    where actor_id = :actor_id
  union
    select resource_id, action_id
    from acl
    where group_id in (select group_id from actor_groups)
)
select count(*)
  from combined_permissions
  where resource_id in (select id from target_resources)
  and action_id = (select id from target_action)

That should work against all levels of resource - per-table, per-database and per-instance, thanks to this bit:

target_resources as (
  select id
  from acl_resources
  where (database = :database and resource = :resource)
    or (database = :database and resource = '')
    or (database = '' and resource = '')
),

@simonw
Copy link
Contributor Author

simonw commented Sep 19, 2024

... and I think I can call this exact SQL query with database of empty string and/or resource of empty string to run checks for any kind of arguments passed to the permission_allowed() hook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant