Ability to create and delete groups

[simonw]

Split from:

• https://github.com/datasette/datasette-acl/issues/5#issuecomment-2323074155_

Open question is how this should affect the audit log.

[simonw]

Given this audit log design: https://github.com/datasette/datasette-acl/blob/0f9133d6bf5731252b1037b5e94d1b73d5e08387/datasette_acl/__init__.py#L40-L48

I could add created and deleted as operation types, then record group creation like this:

id	timestamp	operation_by	operation	group_id	actor_id
1	2024-08-31 10:15:30	root	created	100	NULL
2	2024-08-31 14:45:22	root	deleted	100	NULL

One challenge: that group_id points to a record in acl_groups which is how we find out the name of the group:

https://github.com/datasette/datasette-acl/blob/0f9133d6bf5731252b1037b5e94d1b73d5e08387/datasette_acl/__init__.py#L26-L29

If we delete that record we'll lose the history of what the name was.

Two options:

1. Keep the acl_groups record around but add a deleted column to it, so we can skip it when displaying groups. If the group is ever "created" again we'll set the deleted back to null.
2. Add another column to the acl_groups_audit table that we can use to record that extra detail.

[simonw]

Or.... I could redesign the schema such that group_id works like actor_id and is always a text string, not an integer.

This would simplify the DB schema a bit, at the cost of a tiny extra overhead in the database.

[simonw]

I'm going to try that in a branch.

[simonw]

I spiked on that a bit and decided I don't like it:

diff --git a/datasette_acl/__init__.py b/datasette_acl/__init__.py
index aec52ee..9091a33 100644
--- a/datasette_acl/__init__.py
+++ b/datasette_acl/__init__.py
@@ -22,18 +22,10 @@ create table if not exists acl_actions (
     name text not null unique
 );

--- new table for groups
-create table if not exists acl_groups (
-    id integer primary key,
-    name text not null unique
-);
-
--- new table for actor-group relationships
 create table if not exists acl_actor_groups (
     actor_id text,
-    group_id integer,
-    primary key (actor_id, group_id),
-    foreign key (group_id) references acl_groups(id)
+    group_id text,
+    primary key (actor_id, group_id)
 );

 -- Group membership audit log
@@ -42,18 +34,16 @@ create table if not exists acl_groups_audit (
     timestamp text default (datetime('now')),
     operation_by text,
     operation text check (operation in ('added', 'removed')),
-    group_id integer,
-    actor_id text,
-    foreign key (group_id) references acl_groups(id)
+    group_id text,
+    actor_id text
 );

 create table if not exists acl (
     acl_id integer primary key,
     actor_id text,
-    group_id integer,
+    group_id text,
     resource_id integer,
     action_id integer,
-    foreign key (group_id) references acl_groups(id),
     foreign key (resource_id) references acl_resources(id),
     foreign key (action_id) references acl_actions(id),
     check ((actor_id is null) != (group_id is null)),
@@ -68,9 +58,8 @@ create table if not exists acl_audit (
     operation text check (operation in ('added', 'removed')),
     action_id integer,
     resource_id integer,
-    group_id integer,
+    group_id text,
     actor_id text,
-    foreign key (group_id) references acl_groups(id),
     foreign key (resource_id) references acl_resources(id),
     foreign key (action_id) references acl_actions(id)
 )
@@ -109,35 +98,34 @@ select count(*)

 EXPECTED_GROUPS_SQL = """
 with expected_groups as (
-  select value as group_name
+  select value as group_id
   from json_each(:expected_groups_json)
 ),
 dynamic_groups as (
-  select value as group_name
+  select value as group_id
   from json_each(:dynamic_groups)
 ),
 actual_groups as (
-  select g.name as group_name
-  from acl_groups g
-  join acl_actor_groups ug on g.id = ug.group_id
-  where ug.actor_id = :actor_id
+  select group_id
+  from acl_actor_groups
+  where actor_id = :actor_id
 )
 select
   'should-add' as status,
-  eg.group_name
+  eg.group_id
 from expected_groups eg
-where eg.group_name not in (select group_name from actual_groups)
+where eg.group_id not in (select group_id from actual_groups)
   union all
 select
   'should-remove' as status,
-  ag.group_name
+  ag.group_id
 from actual_groups ag
-where ag.group_name not in (select group_name from expected_groups)
-and ag.group_name in (select group_name from dynamic_groups)
+where ag.group_id not in (select group_id from expected_groups)
+and ag.group_id in (select group_id from dynamic_groups)
   union all
 select
   'current' as status,
-  group_name
+  group_id
 from actual_groups
 """

@@ -154,14 +142,6 @@ def startup(datasette):
         """,
             [{"name": n} for n in datasette.permissions.keys()],
         )
-        # And any dynamic groups
-        config = datasette.plugin_config("datasette-acl")
-        groups = config.get("dynamic-groups")
-        if groups:
-            await db.execute_write_many(
-                "insert or ignore into acl_groups (name) values (:name)",
-                [{"name": name} for name in groups.keys()],
-            )

     return inner

@@ -201,8 +181,8 @@ async def update_dynamic_groups(datasette, actor, skip_cache=False):
         return
     # Figure out the groups the user should be in
     should_have_groups = set(
-        group_name
-        for group_name, allow_block in groups.items()
+        group_id
+        for group_id, allow_block in groups.items()
         if actor_matches_allow(actor, allow_block)
     )
     db = datasette.get_internal_database()
@@ -218,25 +198,20 @@ async def update_dynamic_groups(datasette, actor, skip_cache=False):
     should_remove = []
     for row in result.rows:
         if row["status"] == "should-add":
-            should_add.append(row["group_name"])
+            should_add.append(row["group_ids"])
         elif row["status"] == "should-remove":
-            should_remove.append(row["group_name"])
+            should_remove.append(row["group_id"])
     # Add/remove groups as needed
-    for group_name in should_add:
-        # Make sure the group exists
-        await db.execute_write(
-            "insert or ignore into acl_groups (name) VALUES (:name);",
-            {"name": group_name},
-        )
+    for group_id in should_add:
         await db.execute_write(
             """
             insert into acl_actor_groups (
                 actor_id, group_id
             ) values (
                 :actor_id,
-                (select id from acl_groups where name = :group_name)
+                :group_id
             )""",
-            {"actor_id": actor["id"], "group_name": group_name},
+            {"actor_id": actor["id"], "group_id": group_id},
         )
         await db.execute_write(
             """
@@ -245,23 +220,23 @@ async def update_dynamic_groups(datasette, actor, skip_cache=False):
             ) values (
                 null,
                 'added',
-                (select id from acl_groups where name = :group_name),
+                :group_id,
                 :actor_id
             )
         """,
             {
-                "group_name": group_name,
+                "group_id": group_id,
                 "actor_id": actor["id"],
             },
         )
-    for group_name in should_remove:
+    for group_id in should_remove:
         await db.execute_write(
             """
             delete from acl_actor_groups
             where actor_id = :actor_id
-            and group_id = (select id from acl_groups where name = :group_name)
+            and group_id = :group_id
             """,
-            {"actor_id": actor["id"], "group_name": group_name},
+            {"actor_id": actor["id"], "group_id": group_id},
         )
         await db.execute_write(
             """
@@ -270,12 +245,12 @@ async def update_dynamic_groups(datasette, actor, skip_cache=False):
             ) values (
                 null,
                 'removed',
-                (select id from acl_groups where name = :group_name),
+                :group_id,
                 :actor_id
             )
         """,
             {
-                "group_name": group_name,
+                "group_id": group_id,
                 "actor_id": actor["id"],
             },
         )
diff --git a/datasette_acl/templates/manage_table_acls.html b/datasette_acl/templates/manage_table_acls.html
index dcd9df1..ba1e194 100644
--- a/datasette_acl/templates/manage_table_acls.html
+++ b/datasette_acl/templates/manage_table_acls.html
@@ -98,7 +98,7 @@
         <td>{{ entry.timestamp }}</td>
         <td>{{ entry.operation_by }}</td>
         <td>{{ entry.operation }}</td>
-        <td>{{ entry.group_name or '' }}</td>
+        <td>{{ entry.group_id or '' }}</td>
         <td>{{ entry.actor_id or '' }}</td>
         <td>{{ entry.action_name }}</td>
       </tr>
diff --git a/datasette_acl/views/groups.py b/datasette_acl/views/groups.py
index 992a001..59e5243 100644
--- a/datasette_acl/views/groups.py
+++ b/datasette_acl/views/groups.py
@@ -4,24 +4,16 @@ import json

 GROUPS_SQL = """
 select
-    acl_groups.id,
-    acl_groups.name,
-    count(acl_actor_groups.actor_id) as size,
-    json_group_array(
-        acl_actor_groups.actor_id
-    ) filter (
-        where
-        acl_actor_groups.actor_id is not null
-    ) as actor_ids
+    group_id,
+    count(*) as size,
+    json_group_array(actor_id) as actor_ids
 from
-    acl_groups
-left join
-    acl_actor_groups on acl_groups.id = acl_actor_groups.group_id
+    acl_actor_groups
 {extra_where}
 group by
-    acl_groups.id, acl_groups.name
+    group_id
 order by
-    acl_groups.name;
+    group_id
 """

@@ -54,20 +46,20 @@ async def manage_groups(request, datasette):
 async def manage_group(request, datasette):
     if not await can_edit_permissions(datasette, request.actor):
         raise Forbidden("You do not have permission to edit permissions")
-    name = request.url_vars["name"]
+    group_id = request.url_vars["name"]
     internal_db = datasette.get_internal_database()
     group = (
         await internal_db.execute(
-            GROUPS_SQL.format(extra_where=" where acl_groups.name = :name"),
+            GROUPS_SQL.format(extra_where=" where group_id = :group_id"),
             {
-                "name": name,
+                "group_id": group_id,
             },
         )
     ).first()
     if not group:
         raise NotFound("Group does not exist")
     dynamic_groups = get_dynamic_groups(datasette)
-    dynamic_config = dynamic_groups.get(name)
+    dynamic_config = dynamic_groups.get(group_id)
     actor_ids = json.loads(group["actor_ids"])
     if request.method == "POST" and not dynamic_config:
         post_vars = await request.post_vars()
@@ -89,7 +81,7 @@ async def manage_group(request, datasette):
                     where actor_id = :actor_id
                     and group_id = :group_id
                 """,
-                    {"actor_id": to_remove, "group_id": group["id"]},
+                    {"actor_id": to_remove, "group_id": group_id},
                 )
                 datasette.add_message(request, f"Removed {to_remove}")
                 audit_operation = "removed"
@@ -106,7 +98,7 @@ async def manage_group(request, datasette):
                     insert into acl_actor_groups (actor_id, group_id)
                     values (:actor_id, :group_id)
                 """,
-                    {"actor_id": to_add, "group_id": group["id"]},
+                    {"actor_id": to_add, "group_id": group_id},
                 )
                 datasette.add_message(request, f"Added {to_add}")
                 audit_operation = "added"
diff --git a/datasette_acl/views/table_acls.py b/datasette_acl/views/table_acls.py
index d1bcf59..51d19c3 100644
--- a/datasette_acl/views/table_acls.py
+++ b/datasette_acl/views/table_acls.py
@@ -32,23 +32,22 @@ async def manage_table_acls(request, datasette):
     acl_rows = await internal_db.execute(
         """
         select
-          acl_groups.name as group_name,
+          acl.group_id,
           acl.actor_id,
           acl_actions.name as action_name
         from acl
-        left join acl_groups on acl.group_id = acl_groups.id
         join acl_actions on acl.action_id = acl_actions.id
         where acl.resource_id = ?
         """,
         [resource_id],
     )
     for row in acl_rows.rows:
-        group_name = row["group_name"]
+        group_id = row["group_id"]
         actor_id = row["actor_id"]
         action_name = row["action_name"]
-        if group_name:
-            current_group_permissions.setdefault(group_name, {})[action_name] = True
-            current_group_permissions[group_name][action_name] = True
+        if group_id:
+            current_group_permissions.setdefault(group_id, {})[action_name] = True
+            current_group_permissions[group_id][action_name] = True
         else:
             assert actor_id
             current_user_permissions.setdefault(actor_id, {})[action_name] = True
@@ -56,7 +55,7 @@ async def manage_table_acls(request, datasette):
     if request.method == "POST":
         group_changes_made = {"added": [], "removed": []}
         post_vars = await request.post_vars()
-        for group_name in groups:
+        for group_id in groups:
             for action_name in [
                 "insert-row",
                 "delete-row",
@@ -65,10 +64,10 @@ async def manage_table_acls(request, datasette):
                 "drop-table",
             ]:
                 new_value = bool(
-                    post_vars.get(f"group_permissions_{group_name}_{action_name}")
+                    post_vars.get(f"group_permissions_{group_id}_{action_name}")
                 )
                 current_value = bool(
-                    current_group_permissions.get(group_name, {}).get(action_name)
+                    current_group_permissions.get(group_id, {}).get(action_name)
                 )
                 if new_value != current_value:
                     if new_value:
@@ -78,37 +77,37 @@ async def manage_table_acls(request, datasette):
                             INSERT INTO acl (actor_id, group_id, resource_id, action_id)
                             VALUES (
                                 null,
-                                (SELECT id FROM acl_groups WHERE name = :group_name),
+                                :group_id,
                                 :resource_id,
                                 (SELECT id FROM acl_actions WHERE name = :action_name)
                             )
                             """,
                             {
-                                "group_name": group_name,
+                                "group_id": group_id,
                                 "action_name": action_name,
                                 "resource_id": resource_id,
                             },
                         )
                         operation = "added"
-                        group_changes_made["added"].append((group_name, action_name))
+                        group_changes_made["added"].append((group_id, action_name))
                     else:
                         # They removed it
                         await internal_db.execute_write(
                             """
                             delete from acl where
-                                actor_id is null and 
-                                group_id = (SELECT id FROM acl_groups WHERE name = :group_name)
+                                actor_id is null
+                                and group_id = :group_id
                                 and resource_id = :resource_id
                                 and action_id = (SELECT id FROM acl_actions WHERE name = :action_name)
                             """,
                             {
-                                "group_name": group_name,
+                                "group_id": group_id,
                                 "action_name": action_name,
                                 "resource_id": resource_id,
                             },
                         )
                         operation = "removed"
-                        group_changes_made["removed"].append((group_name, action_name))
+                        group_changes_made["removed"].append((group_id, action_name))
                     await internal_db.execute_write(
                         """
                         insert into acl_audit (
@@ -121,7 +120,7 @@ async def manage_table_acls(request, datasette):
                         ) values (
                             :operation,
                             null,
-                            (SELECT id FROM acl_groups WHERE name = :group_name),
+                            :group_id,
                             :resource_id,
                             (SELECT id FROM acl_actions WHERE name = :action_name),
                             :operation_by
@@ -129,7 +128,7 @@ async def manage_table_acls(request, datasette):
                         """,
                         {
                             "operation": operation,
-                            "group_name": group_name,
+                            "group_id": group_id,
                             "resource_id": resource_id,
                             "action_name": action_name,
                             "operation_by": request.actor["id"],
@@ -240,10 +239,9 @@ async def manage_table_acls(request, datasette):
             acl_audit.operation_by,
             acl_audit.operation,
             acl_audit.actor_id,
-            acl_groups.name as group_name,
+            acl_audit.group_id,
             acl_actions.name as action_name
         from acl_audit
-        left join acl_groups on acl_audit.group_id = acl_groups.id
         join acl_actions on acl_audit.action_id = acl_actions.id
         where acl_audit.resource_id = ?
         order by acl_audit.timestamp desc
@@ -258,14 +256,12 @@ async def manage_table_acls(request, datasette):
         for row in await internal_db.execute(
             """
             select
-                acl_groups.name as name,
+                group_id,
                 count(acl_actor_groups.actor_id) as size
             from
                 acl_groups
-            left join
-                acl_actor_groups on acl_groups.id = acl_actor_groups.group_id
             group by
-                acl_groups.id, acl_groups.name
+                group_id
             """
         )
     }
diff --git a/tests/conftest.py b/tests/conftest.py
index af0f7fb..dd9cdcd 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -21,14 +21,12 @@ async def ds():
             },
             # Root user can edit permissions
             "permissions": {"datasette-acl": {"id": "root"}},
-        }
+        },
+        pdb=True,
     )
     db = datasette.add_memory_database("db")
     await db.execute_write("create table t (id primary key)")
     await datasette.invoke_startup()
-    await datasette.get_internal_database().execute_write(
-        "insert into acl_groups (name) values (:name)", {"name": "dev"}
-    )
     yield datasette
     # Need to manually drop because in-memory databases shared across tests
     await db.execute_write("drop table t")
diff --git a/tests/test_table_acls.py b/tests/test_table_acls.py
index b100b5d..ec06d28 100644
--- a/tests/test_table_acls.py
+++ b/tests/test_table_acls.py
@@ -27,7 +27,7 @@ ManageTableTest = namedtuple(
             post_data={"group_permissions_staff_insert-row": "on"},
             expected_acls=[
                 {
-                    "group_name": "staff",
+                    "group_id": "staff",
                     "actor_id": None,
                     "action_name": "insert-row",
                     "database_name": "db",
@@ -43,7 +43,7 @@ ManageTableTest = namedtuple(
             ],
             expected_audit_rows=[
                 {
-                    "group_name": "staff",
+                    "group_id": "staff",
                     "actor_id": None,
                     "action_name": "insert-row",
                     "database_name": "db",
@@ -62,14 +62,14 @@ ManageTableTest = namedtuple(
             },
             expected_acls=[
                 {
-                    "group_name": "staff",
+                    "group_id": "staff",
                     "actor_id": None,
                     "action_name": "delete-row",
                     "database_name": "db",
                     "resource_name": "t",
                 },
                 {
-                    "group_name": "staff",
+                    "group_id": "staff",
                     "actor_id": None,
                     "action_name": "update-row",
                     "database_name": "db",
@@ -90,7 +90,7 @@ ManageTableTest = namedtuple(
             ],
             expected_audit_rows=[
                 {
-                    "group_name": "staff",
+                    "group_id": "staff",
                     "actor_id": None,
                     "action_name": "insert-row",
                     "database_name": "db",
@@ -99,7 +99,7 @@ ManageTableTest = namedtuple(
                     "operation": "added",
                 },
                 {
-                    "group_name": "staff",
+                    "group_id": "staff",
                     "actor_id": None,
                     "action_name": "insert-row",
                     "database_name": "db",
@@ -108,7 +108,7 @@ ManageTableTest = namedtuple(
                     "operation": "removed",
                 },
                 {
-                    "group_name": "staff",
+                    "group_id": "staff",
                     "actor_id": None,
                     "action_name": "delete-row",
                     "database_name": "db",
@@ -117,7 +117,7 @@ ManageTableTest = namedtuple(
                     "operation": "added",
                 },
                 {
-                    "group_name": "staff",
+                    "group_id": "staff",
                     "actor_id": None,
                     "action_name": "update-row",
                     "database_name": "db",
@@ -140,14 +140,14 @@ ManageTableTest = namedtuple(
                     "action_name": "insert-row",
                     "actor_id": "newbie",
                     "database_name": "db",
-                    "group_name": None,
+                    "group_id": None,
                     "resource_name": "t",
                 },
                 {
                     "action_name": "update-row",
                     "actor_id": "newbie",
                     "database_name": "db",
-                    "group_name": None,
+                    "group_id": None,
                     "resource_name": "t",
                 },
             ],
@@ -165,7 +165,7 @@ ManageTableTest = namedtuple(
             ],
             expected_audit_rows=[
                 {
-                    "group_name": None,
+                    "group_id": None,
                     "actor_id": "newbie",
                     "action_name": "insert-row",
                     "database_name": "db",
@@ -174,7 +174,7 @@ ManageTableTest = namedtuple(
                     "operation": "added",
                 },
                 {
-                    "group_name": None,
+                    "group_id": None,
                     "actor_id": "newbie",
                     "action_name": "update-row",
                     "database_name": "db",
@@ -198,7 +198,7 @@ ManageTableTest = namedtuple(
                     "action_name": "update-row",
                     "actor_id": "newbie",
                     "database_name": "db",
-                    "group_name": None,
+                    "group_id": None,
                     "resource_name": "t",
                 }
             ],
@@ -211,7 +211,7 @@ ManageTableTest = namedtuple(
             ],
             expected_audit_rows=[
                 {
-                    "group_name": None,
+                    "group_id": None,
                     "actor_id": "newbie",
                     "action_name": "insert-row",
                     "database_name": "db",
@@ -220,7 +220,7 @@ ManageTableTest = namedtuple(
                     "operation": "added",
                 },
                 {
-                    "group_name": None,
+                    "group_id": None,
                     "actor_id": "newbie",
                     "action_name": "insert-row",
                     "database_name": "db",
@@ -229,7 +229,7 @@ ManageTableTest = namedtuple(
                     "operation": "removed",
                 },
                 {
-                    "group_name": None,
+                    "group_id": None,
                     "actor_id": "newbie",
                     "action_name": "update-row",
                     "database_name": "db",
@@ -300,7 +300,7 @@ async def test_manage_table_permissions(
             await internal_db.execute(
                 """
         select
-          acl_groups.name as group_name,
+          acl_groups.name as group_id,
           acl.actor_id,
           acl_actions.name as action_name,
           acl_resources.database as database_name,
@@ -324,7 +324,7 @@ async def test_manage_table_permissions(
     # Check audit logs
     AUDIT_SQL = """
         select
-          acl_groups.name as group_name,
+          acl_groups.name as group_id,
           acl_audit.actor_id,
           acl_actions.name as action_name,
           acl_resources.database as database_name,
@@ -406,11 +406,11 @@ async def test_update_dynamic_groups():
         dict(r)
         for r in (
             await db.execute(
-                "select actor_id, (select name from acl_groups where id = group_id) as group_name from acl_actor_groups"
+                "select actor_id, (select name from acl_groups where id = group_id) as group_id from acl_actor_groups"
             )
         ).rows
     ] == [
-        {"actor_id": "staff", "group_name": "staff"},
+        {"actor_id": "staff", "group_id": "staff"},
     ]
     # If that user changes they should drop from the group
     await update_dynamic_groups(
@@ -420,7 +420,7 @@ async def test_update_dynamic_groups():
         dict(r)
         for r in (
             await db.execute(
-                "select actor_id, (select name from acl_groups where id = group_id) as group_name from acl_actor_groups"
+                "select actor_id, (select name from acl_groups where id = group_id) as group_id from acl_actor_groups"
             )
         ).rows
     ] == []
@@ -447,9 +447,8 @@ async def test_update_dynamic_groups():
         ]
     )
     # Groups that are not dynamic should not be modified
-    await db.execute_write("insert into acl_groups (id, name) values (2, 'static')")
     await db.execute_write(
-        "insert into acl_actor_groups (actor_id, group_id) values ('staff', 2)"
+        "insert into acl_actor_groups (actor_id, group_id) values ('staff', 'static')"
     )
     await update_dynamic_groups(
         datasette, {"is_staff": False, "id": "staff"}, skip_cache=True

I don't like that a group is invisible if it has no members, and that there's no way to attach a useful description to a group.

[simonw]

I'm going with the deleted column instead. Will need tests in a few places for that.

[simonw]

I’m going to enforce an alphanumeric name for groups as part of this. I may do that for actor_id in Datasette core too, in which case it should be enforced here for actor IDs.

[simonw]

Got it so you can recreate a group with the same name to "undelete" it - and deleting it removes all members.