From a639cca9edc13f9566f4ade26a1f88b9f14d630a Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Tue, 23 Jun 2026 05:25:49 -0700 Subject: [PATCH] Add admin user/group structure: Editors + Contributors (#1125) Stop using shared, role-named user accounts (gradmin/ugradmin/collabmin) as de-facto groups. Introduce two declarative Django groups so individuals get personal accounts with per-person attribution and clean offboarding: - Editors (PhD students + long-term staff): full add/change/delete/view on the public content models. Excludes Grant and Award (admin-only) and all account-administration models (no user/group mgmt outside the superuser, which would be a privilege-escalation path). - Contributors (ugrads/interns): Person add/change/view + add/view on publication/talk/poster/projectrole (submit their own work and review it, via the changelist). No deletes anywhere. setup_admin_groups is idempotent (.set() makes each group match the spec exactly), so it is the source of truth for these two groups' permissions; group membership and all other auth objects are left to /admin. Wired into docker-entrypoint.sh (step 4.10) because the test/prod servers have no shell access -- the entrypoint one-shot is the only push-deploy-compatible path. Tests (16) cover two layers: (1) exact permission codenames + the security boundaries (no auth.* perms; no grant/award; no contributor deletes) -- this caught a since-removed ProjectHeader model seeded from a stale DB dump; and (2) anti-lockout invariants -- the command never creates/deletes users, never changes is_staff/is_active/is_superuser or group membership, a superuser keeps full access regardless of group config, effective has_perm matches the spec through the stack, an unknown model in the spec is skipped (not raised), and the entrypoint has no `set -e` so a command failure can't block the site boot. Docs in docs/ADMIN_USERS_AND_GROUPS.md plus a pointer in CLAUDE.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- CLAUDE.md | 2 + docker-entrypoint.sh | 5 + docs/ADMIN_USERS_AND_GROUPS.md | 95 +++++++ .../management/commands/setup_admin_groups.py | 150 +++++++++++ website/tests/test_setup_admin_groups.py | 233 ++++++++++++++++++ 5 files changed, 485 insertions(+) create mode 100644 docs/ADMIN_USERS_AND_GROUPS.md create mode 100644 website/management/commands/setup_admin_groups.py create mode 100644 website/tests/test_setup_admin_groups.py diff --git a/CLAUDE.md b/CLAUDE.md index aa39db4a..cfa79e7b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -85,6 +85,8 @@ Each domain concept gets a dedicated file across three parallel directories. Whe Custom admin organization lives in `website/admin/admin_site.py` (`MakeabilityLabAdminSite`). It overrides Django's default app-based grouping with workflow-based groups: Artifacts (Publications/Talks/Posters/Videos), People & News, Projects & Media, Grants & Funding, Configuration, Administration. Section order and which models go in which group are defined in `CUSTOM_GROUPS`. Update this when adding a new top-level model that should appear on the admin index. +**Admin users & permissions (#1125):** editing access is structured as personal accounts assigned to one of two declarative groups — `Editors` (PhD/staff, full content) and `Contributors` (ugrads/interns, submit-and-review, no deletes) — plus superuser (maintainer + a break-glass backup). Grant, Award, and all account-administration models are superuser-only. The groups' permission sets are the source of truth in `setup_admin_groups` (run on every container start via `docker-entrypoint.sh`, pinned by `test_setup_admin_groups`); group *membership* is managed in `/admin`. When adding a new model that editors should manage, add it to `EDITORS_MODELS`/`CONTRIBUTORS_SPEC` and update the test. Full reference + onboarding/offboarding runbook: `docs/ADMIN_USERS_AND_GROUPS.md`. + ### Key model relationships - A `Publication` is the central artifact. `Talk`, `Poster`, `Video` are related artifacts; the admin tip is to start from the Publication's edit page so shared fields (title, authors, date, venue) auto-fill on the children. diff --git a/docker-entrypoint.sh b/docker-entrypoint.sh index a5f3d7ce..536b3cc9 100755 --- a/docker-entrypoint.sh +++ b/docker-entrypoint.sh @@ -148,6 +148,11 @@ echo "4.9 Running 'python manage.py propagate_publication_projects' to link talk echo "******************************************" python manage.py propagate_publication_projects +echo "****************** STEP 4.10/5: docker-entrypoint.sh ************************" +echo "4.10 Running 'python manage.py setup_admin_groups' to create/refresh the Editors and Contributors admin groups (#1125)" +echo "******************************************" +python manage.py setup_admin_groups + # echo "****************** STEP 4.3/5: docker-entrypoint.sh ************************" # echo "4.3 Running 'python manage.py rename_person_images' to rename person images" # echo "******************************************" diff --git a/docs/ADMIN_USERS_AND_GROUPS.md b/docs/ADMIN_USERS_AND_GROUPS.md new file mode 100644 index 00000000..d16b1b25 --- /dev/null +++ b/docs/ADMIN_USERS_AND_GROUPS.md @@ -0,0 +1,95 @@ +# Admin users, groups & permissions (#1125) + +How editing access to the Django admin (`/admin`) is structured for the +Makeability Lab site, and the runbook for onboarding/offboarding people. + +## Principle: Users = people, Groups = roles + +Historically the site used a few shared, role-named *user* accounts +(`gradmin`, `ugradmin`, `collabmin`) as if they were groups — everyone in a +role shared one login. That cost us per-person attribution in the admin history, +clean offboarding (revoking one person meant rotating a shared password), and +least-privilege scoping. + +Going forward: **each person gets their own account**, and **Groups carry the +permissions**. A person's access is determined by which group(s) they're in. + +## The tiers + +| Tier | Who | Account | How access is granted | +|---|---|---|---| +| **Superuser** | The maintainer (Jon) + one locked **break-glass** backup | personal | `is_superuser` flag (bypasses all permission checks) | +| **Editors** | PhD students & long-term staff who maintain the site | personal, one each | member of the `Editors` group | +| **Contributors** | Undergrads / interns | shared `contributor`, or personal if they become a regular maintainer | member of the `Contributors` group | + +Why two superusers: never rely on exactly one. The break-glass account has a +strong, unique password and is used only to recover if the primary account is +locked out or broken. + +Why account creation stays superuser-only: in Django's default admin, anyone who +can add/change `User` or `Group` objects can grant themselves permissions — an +escalation path. So neither group gets any `auth.*` permission; **only a +superuser creates accounts and assigns groups.** + +## Permission sets + +These are defined declaratively in +`website/management/commands/setup_admin_groups.py` and pinned by +`website/tests/test_setup_admin_groups.py`. + +**`Editors`** — full `add`/`change`/`delete`/`view` on the public content models: +`banner, person, position, project, keyword, talk, publication, poster, news, +video, photo, projectumbrella, sponsor, projectrole`. + +**`Contributors`** — submit-and-review, never destroy: +- `person`: `add`, `change`, `view` (edit bios) +- `publication`, `talk`, `poster`, `projectrole`: `add` + `view` (create their + own work and review it; `view` is required so the admin changelist is + reachable after an add) +- **no `delete` anywhere** + +### Deliberately admin-only (neither group) + +- **`Grant`** (Grants & Funding — funding data) and **`Award`** (curated external + recognitions). Note: *paper* awards live on `Publication.award`, which Editors + *can* edit via the publication; only the standalone `Award` model is withheld. +- `User`, `Group`, `Permission`, `LogEntry`, sessions — account/audit administration. + +## How it's enforced + +`setup_admin_groups` is **idempotent and the source of truth** for these two +groups' *permissions*: each run calls `Group.permissions.set(...)`, so a +permission added or removed by hand in the admin is reverted on the next run. + +It runs automatically on every container start via `docker-entrypoint.sh` +(step 4.10) — this is the only push-deploy-compatible path because the test/prod +servers have no shell access. Edit the spec → redeploy to change a group's +permissions; don't hand-edit them in `/admin` (the change won't survive). + +**Group *membership* is NOT managed by the command** — who belongs to a group is +set in `/admin → Users` and persists across deploys. Likewise users, the +superuser flag, and any other groups are untouched. + +To preview without writing: `python manage.py setup_admin_groups --dry-run`. + +## Runbook + +**Onboard a PhD student / long-term editor** +1. `/admin → Users → Add`: create their account (they set their own password). +2. Set `is_staff = True`. Do **not** set `is_superuser`. +3. Add them to the `Editors` group. Save. + +**Onboard an undergrad / intern** +- Either add them to the shared `contributor` account's owners, or (preferred for + anyone staying a while) create a personal account in the `Contributors` group. +- Promote to `Editors` if they become a regular site maintainer. + +**Offboard anyone** +- `/admin → Users`: set `is_active = False`. **Do not delete** the account — + deleting it orphans their admin-history (`LogEntry`) attribution. Deactivating + blocks login while preserving the record of what they changed. + +**Rotate** +- The legacy shared accounts (`gradmin`/`ugradmin`/`collabmin`/old `makeadmin`) + should be deactivated, and any retained superuser password reset, since their + hashes exist in old DB dumps. diff --git a/website/management/commands/setup_admin_groups.py b/website/management/commands/setup_admin_groups.py new file mode 100644 index 00000000..42ba137d --- /dev/null +++ b/website/management/commands/setup_admin_groups.py @@ -0,0 +1,150 @@ +import logging +from django.core.management.base import BaseCommand +from django.contrib.auth.models import Group, Permission +from django.contrib.contenttypes.models import ContentType + +# This retrieves a Python logging instance (or creates it) +_logger = logging.getLogger(__name__) + +# The four auto-generated Django model permissions (add/change/delete/view). +CRUD = ("add", "change", "delete", "view") + +# --------------------------------------------------------------------------- +# Permission specs (issue #1125 — "Users = people, Groups = roles"). +# +# We deliberately stopped using shared, role-named *user* accounts (gradmin, +# ugradmin, collabmin) as if they were groups. Instead, individuals get personal +# accounts assigned to one of these two groups; the superuser flag (you, plus a +# break-glass backup) covers everything a group cannot. +# +# Each spec maps (app_label, model_name) -> tuple of actions to grant. +# --------------------------------------------------------------------------- + +# Editors — PhD students and long-term staff who maintain the site. Full content +# management on the public-facing models. DELIBERATELY EXCLUDES: +# - grant, award -> admin-only by decision (funding data / curated +# external recognitions stay with the superuser) +# - user/group/permission/logentry/session -> no account administration +# outside the superuser +# - contenttype, easy_thumbnails.* -> infra/cache tables nobody hand-edits +# (these were collateral on the old gradmin account) +EDITORS_MODELS = [ + "banner", "person", "position", "project", "keyword", "talk", + "publication", "poster", "news", "video", + "photo", "projectumbrella", "sponsor", "projectrole", +] +EDITORS_SPEC = {("website", model): CRUD for model in EDITORS_MODELS} + +# Contributors — undergrads / interns (shared `contributor` account, or a personal +# account promoted to Editors if they become a regular maintainer). Narrowest +# useful tier: edit bios, and add (with view) on the main artifacts so they can +# submit their own work AND review what they submitted, but never change or delete +# anyone else's. NO deletes anywhere. This merges the two real shapes the legacy +# accounts had: ugradmin (Person add/change/view) and collabmin (add across +# artifacts), plus `view` so the changelist is reachable after an add (without it +# Django bounces them to the admin index with no way to see their own entry). +CONTRIBUTORS_SPEC = { + ("website", "person"): ("add", "change", "view"), + ("website", "publication"): ("add", "view"), + ("website", "talk"): ("add", "view"), + ("website", "poster"): ("add", "view"), + ("website", "projectrole"): ("add", "view"), +} + +GROUPS = { + "Editors": EDITORS_SPEC, + "Contributors": CONTRIBUTORS_SPEC, +} + + +class Command(BaseCommand): + help = ( + "Create/refresh the Editors and Contributors admin groups with their " + "intended permission sets (issue #1125). Idempotent: each run sets each " + "group's permissions to exactly the spec below — extra permissions added " + "by hand are REMOVED and missing ones are added, so this command is the " + "source of truth for these two groups. It does NOT create user accounts, " + "assign users to groups, or touch the superuser flag; do that in /admin. " + "Run with --dry-run first to preview changes." + ) + + def add_arguments(self, parser): + parser.add_argument( + "--dry-run", + action="store_true", + help="Report what would change without writing to the database.", + ) + + def _resolve_perms(self, spec): + """Turn a {(app_label, model): (actions,)} spec into a set of Permission + objects, warning (not failing) on any codename/content type that can't be + found so a single typo or missing model never aborts the whole run.""" + perms = set() + for (app_label, model_name), actions in spec.items(): + try: + content_type = ContentType.objects.get( + app_label=app_label, model=model_name + ) + except ContentType.DoesNotExist: + _logger.warning( + f"setup_admin_groups: no content type for " + f"{app_label}.{model_name} — skipping (is the model migrated?)" + ) + continue + for action in actions: + codename = f"{action}_{model_name}" + try: + perms.add( + Permission.objects.get( + content_type=content_type, codename=codename + ) + ) + except Permission.DoesNotExist: + _logger.warning( + f"setup_admin_groups: no permission '{codename}' for " + f"{app_label}.{model_name} — skipping." + ) + return perms + + def handle(self, *args, **options): + dry_run = options["dry_run"] + _logger.debug(f"Running setup_admin_groups.py (dry_run={dry_run})") + + for group_name, spec in GROUPS.items(): + desired = self._resolve_perms(spec) + + if dry_run: + group = Group.objects.filter(name=group_name).first() + current = set(group.permissions.all()) if group else set() + else: + group, created = Group.objects.get_or_create(name=group_name) + if created: + _logger.info(f"Created group '{group_name}'") + current = set(group.permissions.all()) + + to_add = desired - current + to_remove = current - desired + + for perm in sorted(to_add, key=lambda p: p.codename): + _logger.info( + f"[{'dry-run' if dry_run else 'apply'}] {group_name}: " + f"+ {perm.codename}" + ) + for perm in sorted(to_remove, key=lambda p: p.codename): + _logger.info( + f"[{'dry-run' if dry_run else 'apply'}] {group_name}: " + f"- {perm.codename}" + ) + + if not dry_run: + # set() makes the group match the spec exactly (idempotent). + group.permissions.set(desired) + + verb = "Would set" if dry_run else "Set" + _logger.info( + f"setup_admin_groups: {verb} '{group_name}' to " + f"{len(desired)} permission(s) " + f"(+{len(to_add)} / -{len(to_remove)})." + ) + + _logger.debug("Completed setup_admin_groups.py") diff --git a/website/tests/test_setup_admin_groups.py b/website/tests/test_setup_admin_groups.py new file mode 100644 index 00000000..78ed83d9 --- /dev/null +++ b/website/tests/test_setup_admin_groups.py @@ -0,0 +1,233 @@ +""" +Regression tests for the setup_admin_groups management command (#1125). + +Two layers: + +1. SetupAdminGroupsTests pins the *exact* permission sets of the Editors and + Contributors groups so a future model rename/removal, or an accidental edit + to the spec, can't silently widen or drop a group's access. They also assert + the design's security boundaries: neither group can manage users/groups + (account admin stays superuser-only), nor touch Grant or Award (admin-only). + +2. SetupAdminGroupsSafetyTests + DockerEntrypointWiringTests cover the + anti-lockout invariants. The command runs on EVERY container start, so it + must never touch user accounts, the superuser flag, login flags + (is_staff/is_active), or group membership — only the two groups' permission + lists. And a superuser must keep full access no matter how the groups are + configured, so the maintainer can always log in and repair things. + +The command builds permissions from ContentType + Permission rows, which Django +auto-creates during test-DB setup, so these run on the standard DatabaseTestCase. +""" + +from pathlib import Path + +from django.contrib.auth.models import Group, Permission, User +from django.core.management import call_command + +from website.tests.base import DatabaseTestCase + +REPO_ROOT = Path(__file__).resolve().parents[2] + + +def _codenames(group_name): + """Return the set of 'app_label.codename' strings granted to a group.""" + group = Group.objects.get(name=group_name) + return { + f"{p.content_type.app_label}.{p.codename}" + for p in group.permissions.select_related("content_type") + } + + +# The full content set Editors manage, each with all four CRUD actions. +EDITORS_MODELS = [ + "banner", "person", "position", "project", "keyword", "talk", + "publication", "poster", "news", "video", + "photo", "projectumbrella", "sponsor", "projectrole", +] +EXPECTED_EDITORS = { + f"website.{action}_{model}" + for model in EDITORS_MODELS + for action in ("add", "change", "delete", "view") +} + +EXPECTED_CONTRIBUTORS = { + "website.add_person", "website.change_person", "website.view_person", + "website.add_publication", "website.view_publication", + "website.add_talk", "website.view_talk", + "website.add_poster", "website.view_poster", + "website.add_projectrole", "website.view_projectrole", +} + + +class SetupAdminGroupsTests(DatabaseTestCase): + def test_creates_both_groups(self): + self.assertFalse(Group.objects.filter(name="Editors").exists()) + call_command("setup_admin_groups") + self.assertTrue(Group.objects.filter(name="Editors").exists()) + self.assertTrue(Group.objects.filter(name="Contributors").exists()) + + def test_editors_permission_set_is_exact(self): + call_command("setup_admin_groups") + self.assertEqual(_codenames("Editors"), EXPECTED_EDITORS) + + def test_contributors_permission_set_is_exact(self): + call_command("setup_admin_groups") + self.assertEqual(_codenames("Contributors"), EXPECTED_CONTRIBUTORS) + + def test_neither_group_can_administer_accounts(self): + """The escalation boundary: no user/group/permission perms anywhere.""" + call_command("setup_admin_groups") + for group in ("Editors", "Contributors"): + for codename in _codenames(group): + self.assertNotIn("auth.", codename, f"{group} has {codename}") + self.assertNotIn("logentry", codename, f"{group} has {codename}") + + def test_grant_and_award_are_admin_only(self): + """Grant (funding) and Award (external recognitions) stay superuser-only.""" + call_command("setup_admin_groups") + for group in ("Editors", "Contributors"): + for codename in _codenames(group): + self.assertNotIn("_grant", codename, f"{group} has {codename}") + self.assertNotIn("_award", codename, f"{group} has {codename}") + + def test_contributors_cannot_delete_anything(self): + call_command("setup_admin_groups") + for codename in _codenames("Contributors"): + self.assertFalse( + codename.split(".")[1].startswith("delete_"), + f"Contributors unexpectedly has delete perm {codename}", + ) + + def test_idempotent_and_self_healing(self): + """Re-running converges to the same set; a hand-added perm is removed + and a hand-removed perm is restored (the command is source of truth).""" + call_command("setup_admin_groups") + editors = Group.objects.get(name="Editors") + + # Pollute: grant a perm that isn't in the spec, drop one that is. + stray = Permission.objects.get(codename="delete_grant") + editors.permissions.add(stray) + keep = Permission.objects.get(codename="add_publication") + editors.permissions.remove(keep) + + call_command("setup_admin_groups") + + self.assertEqual(_codenames("Editors"), EXPECTED_EDITORS) + + +class SetupAdminGroupsSafetyTests(DatabaseTestCase): + """Anti-lockout invariants: the command must only ever touch the two groups' + permission lists, never user accounts, login ability, or membership — and a + superuser must always retain full access.""" + + def test_superuser_keeps_full_access_regardless_of_groups(self): + """A superuser bypasses all permission checks — the ultimate anti-lockout + guarantee. Even right after the groups are (re)built, the superuser can + still do the admin-only things no group is granted.""" + boss = User.objects.create_superuser("boss", "boss@example.com", "x") + call_command("setup_admin_groups") + boss.refresh_from_db() + self.assertTrue(boss.is_superuser) + self.assertTrue(boss.is_staff) + self.assertTrue(boss.is_active) + # Implicitly holds every permission, including the withheld ones. + self.assertTrue(boss.has_perm("auth.add_user")) + self.assertTrue(boss.has_perm("website.delete_grant")) + self.assertTrue(boss.has_perm("website.add_award")) + + def test_does_not_create_or_delete_users(self): + User.objects.create_user("alice") + before = set(User.objects.values_list("pk", flat=True)) + call_command("setup_admin_groups") + call_command("setup_admin_groups") # entrypoint reruns it every boot + after = set(User.objects.values_list("pk", flat=True)) + self.assertEqual(before, after) + + def test_does_not_change_login_flags(self): + """Logging into /admin requires is_staff; the command must never flip it + (or is_active / is_superuser) on anyone.""" + editor = User.objects.create_user("ed", is_staff=True, is_active=True) + call_command("setup_admin_groups") + editor.refresh_from_db() + self.assertTrue(editor.is_staff) + self.assertTrue(editor.is_active) + self.assertFalse(editor.is_superuser) + + def test_preserves_group_membership_across_reruns(self): + """Group membership is assigned in /admin and must survive every deploy — + the command manages permissions, not who belongs to a group.""" + call_command("setup_admin_groups") + editor = User.objects.create_user("ed") + editor.groups.add(Group.objects.get(name="Editors")) + + call_command("setup_admin_groups") # simulate a redeploy + + editor.refresh_from_db() + self.assertEqual( + set(editor.groups.values_list("name", flat=True)), {"Editors"} + ) + + def test_editor_effective_permissions_match_spec(self): + """Through-the-stack: a non-superuser staff user in Editors actually gets + exactly the spec's access via has_perm. Re-fetch the user to clear + Django's per-instance permission cache after the membership change.""" + call_command("setup_admin_groups") + ed = User.objects.create_user("ed", is_staff=True) + ed.groups.add(Group.objects.get(name="Editors")) + ed = User.objects.get(pk=ed.pk) + + self.assertTrue(ed.has_perm("website.change_publication")) + self.assertTrue(ed.has_perm("website.delete_talk")) + # ...but not the admin-only or account-admin perms. + self.assertFalse(ed.has_perm("website.delete_grant")) + self.assertFalse(ed.has_perm("website.add_award")) + self.assertFalse(ed.has_perm("auth.add_user")) + self.assertFalse(ed.has_perm("auth.change_group")) + + def test_contributor_effective_permissions_match_spec(self): + call_command("setup_admin_groups") + intern = User.objects.create_user("intern", is_staff=True) + intern.groups.add(Group.objects.get(name="Contributors")) + intern = User.objects.get(pk=intern.pk) + + self.assertTrue(intern.has_perm("website.add_publication")) + self.assertTrue(intern.has_perm("website.view_publication")) + self.assertTrue(intern.has_perm("website.change_person")) + # No deletes, no editing others' artifacts, no admin-only, no escalation. + self.assertFalse(intern.has_perm("website.delete_publication")) + self.assertFalse(intern.has_perm("website.change_publication")) + self.assertFalse(intern.has_perm("website.add_grant")) + self.assertFalse(intern.has_perm("auth.add_user")) + + def test_resilient_to_unknown_model_in_spec(self): + """A spec entry for a model that doesn't exist (e.g. a future removal, + like ProjectHeader was) is skipped, not raised — so a stale spec can never + crash the entrypoint and block the site from booting.""" + from website.management.commands.setup_admin_groups import Command + + perms = Command()._resolve_perms({ + ("website", "ghost_model_that_does_not_exist"): ("add", "change"), + ("website", "person"): ("add",), + }) + self.assertEqual({p.codename for p in perms}, {"add_person"}) + + +class DockerEntrypointWiringTests(DatabaseTestCase): + """Guard the deploy wiring: the command must run on container start, and the + entrypoint must not 'set -e' before it — otherwise a single failing step would + abort the boot and lock everyone out of a site that never starts.""" + + def _entrypoint(self): + return (REPO_ROOT / "docker-entrypoint.sh").read_text() + + def test_entrypoint_invokes_setup_admin_groups(self): + self.assertIn("manage.py setup_admin_groups", self._entrypoint()) + + def test_entrypoint_does_not_abort_on_error(self): + for line in self._entrypoint().splitlines(): + stripped = line.strip() + self.assertNotEqual(stripped, "set -e") + self.assertFalse(stripped.startswith("set -e ")) + self.assertFalse(stripped.startswith("set -o errexit")) + self.assertFalse(stripped.startswith("set -eu"))