Remove more class admin things (#493)

Fixes #488 

In this PR:
- Removes "Administrator" from dropdown in the UI unless the person
still has this role
- Works towards #488 by updating permissions model to move the
`can_manage_assistants` permission over to `supervisor` instead of
`admin`.
- Allow testing on functions that write / delete permissions with the
authz mock server
- Allow inspection of permissions operations that occurred during the
test on the authz mock server
- Add new tests for the group creation endpoint to verify correct
permissions setting on public vs. private groups.
- We will no longer grant `admin` automatically to the creator of a
group.
- Note that `admin` role is required to set the API key. Since the
creator of the class had to have had `admin` anyway this shouldn't be an
issue.
 - Script for migrating permissions.


Migration steps:
- [x] Upgrade OpenFGA to 1.6.0 (this is actually optional but a good
idea)
- [ ] Run `python -m pingpong auth update_model` to update the latest
permissions model
- [ ] Run script to clean up permissions: `python -m pingpong auth
update_group_admin_perms`
- [ ] Finally, the permissions model can be updated again after the
migration is complete to remove the `class#admin` userset on
`can_manage_assistants`. This is needed during the migration in order to
check this permission and convert it to supervisor.
 - [ ] Restart pingpong srv to pick up newest model

Author: jnu

conftest.py

@@ -69,6 +69,20 @@ async def user(request, config, db):
         yield u
 
 
+@pytest.fixture
+async def institution(request, config, db):
+    if not hasattr(request, "param"):
+        yield None
+    else:
+        from pingpong.models import Institution
+
+        async with db.async_session() as session:
+            i = Institution(**request.param)
+            session.add(i)
+            await session.commit()
+        yield i
+
+
 @pytest.fixture
 async def valid_user_token(user, now):
     from pingpong.auth import encode_session_token

docker-compose.yml

@@ -4,7 +4,7 @@ services:
 
   authz:
     platform: linux/amd64
-    image: openfga/openfga:v1.5.0
+    image: openfga/openfga:v1.6.0
     container_name: pingpong-authz
     command: [run]
     expose: [8080, 8081, 3003]
@@ -13,6 +13,7 @@ services:
       OPENFGA_DATASTORE_MAX_OPEN_CONNS: 40
       OPENFGA_DATASTORE_MAX_IDLE_CONNS: 25
       OPENFGA_MAX_CONCURRENT_READS_FOR_CHECK: 100
+      OPENFGA_EXPERIMENTALS: enable-list-users
     secrets:
     - source: openfgacfg
       mode: 0400

pingpong/__main__.py

@@ -14,6 +14,7 @@
 from .canvas import canvas_sync_all
 from .config import config
 from .models import Base, User
+from .authz.admin_migration import remove_class_admin_perms
 
 from sqlalchemy import inspect
 
@@ -81,6 +82,11 @@ async def _update_model() -> None:
     asyncio.run(_update_model())
 
 
+@auth.command("update_group_admin_perms")
+def update_group_admin_perms() -> None:
+    asyncio.run(remove_class_admin_perms())
+
+
 @auth.command("login")
 @click.argument("email")
 @click.argument("redirect", default="/")

pingpong/authz/admin_migration.py

@@ -0,0 +1,82 @@
+from ..config import config
+from ..models import Class
+from .base import AuthzClient
+
+import logging
+
+import tqdm
+from sqlalchemy import select
+
+logger = logging.getLogger(__name__)
+
+
+async def swap_class_admin_for_teacher(c: AuthzClient, all_classes: list[int]):
+    """Find users who are class admins and swap them to teachers."""
+    logger.info("Swapping group admin role for teacher role ...")
+    total_migrated = 0
+    for class_id in tqdm.tqdm(all_classes):
+        # Use `expand` instead of `list-users` since we want to *ignore* everyone
+        # who has *inherited* permissions. Expand at level 1 will only return
+        # the direct assignments within the class.
+        expand_result = await c.expand(f"class:{class_id}", "admin", max_depth=1)
+        users_with_admin = [rel.entity for rel in expand_result if not rel.is_group]
+        await c.write_safe(
+            grant=[(u, "teacher", f"class:{class_id}") for u in users_with_admin],
+            revoke=[(u, "admin", f"class:{class_id}") for u in users_with_admin],
+        )
+        total_migrated += len(users_with_admin)
+    logger.info(f"Swapped {total_migrated} user(s) from admin to teacher role.")
+
+
+async def swap_class_admin_group_for_supervisor_group(
+    c: AuthzClient, all_classes: list[int]
+):
+    """Find groups where admins can manage assistants and grant to supervisors."""
+    logger.info("Swapping group admin group for supervisor group ...")
+    total_migrated = 0
+    for class_id in tqdm.tqdm(all_classes):
+        # NOTE that since supervisor is a supserset of admin, this will always be true even
+        # after classes are migrated. That's ok, since the update is idempotent.
+        checks = await c.check(
+            [(f"class:{class_id}#admin", "can_manage_assistants", f"class:{class_id}")]
+        )
+        if checks[0]:
+            await c.write_safe(
+                grant=[
+                    (
+                        f"class:{class_id}#supervisor",
+                        "can_manage_assistants",
+                        f"class:{class_id}",
+                    )
+                ],
+                revoke=[
+                    (
+                        f"class:{class_id}#admin",
+                        "can_manage_assistants",
+                        f"class:{class_id}",
+                    )
+                ],
+            )
+            total_migrated += 1
+    logger.info(f"Swapped {total_migrated} group(s) from admin to supervisor group.")
+
+
+async def remove_class_admin_perms():
+    """Clean up any `admin` permissions assigned on a class level.
+
+    1) Swap class `admin` for `teacher`
+    2) Swap `class:#admin` group for `class:#supervisor` where it is set in `can_manage_assistants`
+    """
+    logger.info("Updating group admin permissions ...")
+    await config.authz.driver.init()
+    async with config.db.driver.async_session() as session:
+        all_classes = await session.execute(select(Class))
+        all_class_ids = [c.id for c in all_classes.scalars()]
+
+    logger.info(f"Found {len(all_class_ids)} groups to migrate.")
+
+    async with config.authz.driver.get_client() as c:
+        await swap_class_admin_for_teacher(c, all_class_ids)
+        await swap_class_admin_group_for_supervisor_group(c, all_class_ids)
+
+    logger.info("Group admin permissions updated.")

pingpong/authz/authz.fga

@@ -33,7 +33,7 @@ type class
     define can_manage_users: supervisor
     define can_create_thread: member
     define can_create_assistants: [class#student] or supervisor
-    define can_manage_assistants: [class#admin]
+    define can_manage_assistants: [class#admin, class#supervisor]
     define can_publish_assistants: [class#student] or supervisor
     define can_manage_threads: [class#supervisor]
     define can_publish_threads: [class#student] or can_manage_threads