diff --git a/apps/core/management/commands/seed_walkthrough.py b/apps/core/management/commands/seed_walkthrough.py index 6c34af4..80b3466 100644 --- a/apps/core/management/commands/seed_walkthrough.py +++ b/apps/core/management/commands/seed_walkthrough.py @@ -61,6 +61,7 @@ from apps.esg.models import ESGFieldReport from apps.qc.models import QCResult from apps.shipments.models import Shipment from apps.subscriptions.models import Subscription +from apps.users.auth import is_staff_for_role from apps.users.models import User BUYER_EMAIL = "buyer@greenfieldfoods.example" @@ -622,7 +623,13 @@ class Command(BaseCommand): ) buyer.role = "buyer" buyer.full_name = BUYER_NAME - buyer.save(update_fields=["role", "full_name"]) + # Staff status is derived from the role, and the seed must not leave the + # flag disagreeing with it: a staff buyer is inside the admin area until + # the next sign-in re-derives the flag. Setting it on the update path too + # closes the window the seed used to reopen on every run. + buyer.is_staff = is_staff_for_role(buyer.role) + buyer.is_active = True + buyer.save(update_fields=["role", "full_name", "is_staff", "is_active"]) Subscription.objects.update_or_create( user=buyer, plan=Subscription.Plan.PROFESSIONAL, defaults={ diff --git a/apps/core/middleware.py b/apps/core/middleware.py index 5d05f19..74d88c6 100644 --- a/apps/core/middleware.py +++ b/apps/core/middleware.py @@ -3,6 +3,9 @@ import re from django.conf import settings from django.shortcuts import redirect +# The administration area is decided by role, not by the staff flag alone. +ADMIN_PATH = re.compile(r"^/admin/") + class RBACMiddleware: ROLE_ROUTES = { @@ -16,22 +19,33 @@ class RBACMiddleware: def __call__(self, request): user = request.user - if user.is_authenticated and not user.is_staff and not user.is_superuser: + if user.is_authenticated and not user.is_superuser: path = request.path_info user_role = getattr(request.user, "role", None) - allowed = self.ROLE_ROUTES.get(user_role, []) - if any(re.match(pattern, path) for pattern in allowed): - return self.get_response(request) - # The path belongs to a portal this role may not enter. Send them to - # the role-aware landing view rather than straight into a portal they - # cannot use. It used to redirect if *any* role's pattern matched the - # path without belonging to this role, which bounced a buyer off - # /portal/ itself (the admin patterns include /portal/), and looped - # with the landing redirect. - if any( - re.match(pattern, path) - for patterns in self.ROLE_ROUTES.values() - for pattern in patterns - ): + + # /admin/ is for the admin role. is_staff alone is not enough: an ops + # or buyer account carrying the flag must not reach the + # administration area, whose index embeds the model registry, and + # where a later group or permission grant would be inherited. A + # superuser (above) is the deliberate exception. + if ADMIN_PATH.match(path) and user_role != "admin": return redirect(settings.LOGIN_REDIRECT_URL) + + if not user.is_staff: + allowed = self.ROLE_ROUTES.get(user_role, []) + if any(re.match(pattern, path) for pattern in allowed): + return self.get_response(request) + # The path belongs to a portal this role may not enter. Send them + # to the role-aware landing view rather than straight into a + # portal they cannot use. It used to redirect if *any* role's + # pattern matched the path without belonging to this role, which + # bounced a buyer off /portal/ itself (the admin patterns include + # /portal/), and looped with the landing redirect. + if any( + re.match(pattern, path) + for patterns in self.ROLE_ROUTES.values() + for pattern in patterns + ): + return redirect(settings.LOGIN_REDIRECT_URL) return self.get_response(request) + diff --git a/apps/core/tests.py b/apps/core/tests.py index aa653d2..c1b1a3f 100644 --- a/apps/core/tests.py +++ b/apps/core/tests.py @@ -197,6 +197,59 @@ class RoleRoutingTests(TestCase): self.assertIn("/oidc/authenticate/", response["Location"]) +class AdminAreaRoleTests(TestCase): + """The administration area is for the admin role, not for the staff flag. + + A staff flag on an ops or buyer account used to be enough to reach /admin/, + whose index embeds the model registry, and where a later group or permission + grant would be inherited. Staff status is derived from the role, but the gate + must not rest on the flag alone. + """ + + def make_user(self, email, role, is_staff): + return User.objects.create_user( + email=email, password="pass1234", role=role, is_staff=is_staff + ) + + def test_a_staff_ops_account_is_turned_away_from_admin(self): + client = Client(SERVER_NAME="app.ixgltd.com") + client.force_login(self.make_user("gate-ops@ixg.local", "ops", True)) + response = client.get("/admin/") + self.assertEqual(response.status_code, 302) + self.assertEqual(response["Location"], "/") + + def test_a_staff_buyer_account_is_turned_away_from_admin(self): + client = Client(SERVER_NAME="app.ixgltd.com") + client.force_login(self.make_user("gate-buyer@ixg.local", "buyer", True)) + response = client.get("/admin/") + self.assertEqual(response.status_code, 302) + self.assertEqual(response["Location"], "/") + + def test_an_admin_role_reaches_admin(self): + client = Client(SERVER_NAME="app.ixgltd.com") + client.force_login(self.make_user("gate-admin@ixg.local", "admin", True)) + self.assertEqual(client.get("/admin/").status_code, 200) + + def test_a_superuser_reaches_admin(self): + client = Client(SERVER_NAME="app.ixgltd.com") + client.force_login( + User.objects.create_superuser( + email="gate-super@ixg.local", password="pass1234" + ) + ) + self.assertEqual(client.get("/admin/").status_code, 200) + + def test_the_ops_sidebar_hides_admin_from_a_non_admin(self): + client = Client(SERVER_NAME="ops.ixgltd.com") + client.force_login(self.make_user("gate-ops2@ixg.local", "ops", True)) + self.assertNotContains(client.get("/ops/"), 'href="/admin/"') + + def test_the_ops_sidebar_shows_admin_to_an_admin(self): + client = Client(SERVER_NAME="ops.ixgltd.com") + client.force_login(self.make_user("gate-admin2@ixg.local", "admin", True)) + self.assertContains(client.get("/ops/"), 'href="/admin/"') + + class ApiPermissionBoundaryTests(TestCase): """The API permission boundary, pinned for every viewset. @@ -471,6 +524,20 @@ class SeedWalkthroughTests(TestCase): self.assertEqual(CAPARecord.objects.count(), 1) self.assertEqual(ProcessingBatch.objects.filter(batch_reference="IXG-TEST-001").count(), 1) + def test_the_seeded_buyer_is_not_staff(self): + """Staff status is derived from the role; the seed must not disagree.""" + buyer = User.objects.get(email="buyer@greenfieldfoods.example") + self.assertFalse(buyer.is_staff) + + def test_re_running_the_seed_corrects_a_staff_buyer(self): + """The seed used to set is_staff only on creation, so a buyer whose flag + had drifted to True stayed inside the admin area until its next sign-in. + Re-running the seed must correct it, not reopen the window.""" + User.objects.filter(email="buyer@greenfieldfoods.example").update(is_staff=True) + call_command("seed_walkthrough", verbosity=0) + buyer = User.objects.get(email="buyer@greenfieldfoods.example") + self.assertFalse(buyer.is_staff) + def test_every_intake_keeps_within_what_it_received(self): """The rule the ops lineage screen enforces, applied to the training data. diff --git a/apps/users/migrations/0003_align_is_staff_with_role.py b/apps/users/migrations/0003_align_is_staff_with_role.py new file mode 100644 index 0000000..1e69df0 --- /dev/null +++ b/apps/users/migrations/0003_align_is_staff_with_role.py @@ -0,0 +1,32 @@ +from django.db import migrations + +# Staff status is derived from the role on the write path +# (apps/users/auth.py::is_staff_for_role). Existing rows could still disagree — +# a seeded buyer carried is_staff=True, and an ops account is_staff=False — +# which left a staff buyer inside the admin area until their next sign-in +# re-derived the flag. Align the stored flag with the role once, so the flag +# never disagrees with the role. + +STAFF_ROLES = ("ops", "admin") + + +def align_is_staff(apps, schema_editor): + User = apps.get_model("users", "User") + User.objects.filter(is_superuser=True).update(is_staff=True) + User.objects.filter(is_superuser=False, role__in=STAFF_ROLES).update(is_staff=True) + User.objects.filter(is_superuser=False).exclude(role__in=STAFF_ROLES).update( + is_staff=False + ) + + +class Migration(migrations.Migration): + + dependencies = [ + ("users", "0002_alter_user_managers"), + ] + + operations = [ + # Not reversed: the previous flag values are not recoverable, and the + # invariant is what we want to keep. + migrations.RunPython(align_is_staff, migrations.RunPython.noop), + ] diff --git a/templates/ops/base_ops.html b/templates/ops/base_ops.html index c12ad74..6c7a5b4 100644 --- a/templates/ops/base_ops.html +++ b/templates/ops/base_ops.html @@ -59,7 +59,9 @@ body { font-size: .875rem; background: #f5f6fa; }
+ {% if request.user.role == 'admin' or request.user.is_superuser %} Admin + {% endif %} {{ request.user.full_name|default:request.user.email }}
{% csrf_token %}