Gate the administration area by role, not by is_staff
RBACMiddleware only guarded non-staff users, so any staff account stepped past the role routing for /admin/ — an ops or buyer account with the flag set could reach the admin index, which embeds the model registry, and a later permission grant would have been inherited. Restrict /admin/ to the admin role (superuser excepted) and hide the ops Admin link from non-admins. Also close the second finding: the seed set is_staff only on creation, so a buyer whose flag had drifted stayed staff on every re-run. Derive it from the role on the update path too, and align existing rows with a one-off migration so the flag never disagrees with the role.
This commit is contained in:
parent
df2a90bc71
commit
ef6f217e45
5 changed files with 138 additions and 16 deletions
|
|
@ -61,6 +61,7 @@ from apps.esg.models import ESGFieldReport
|
||||||
from apps.qc.models import QCResult
|
from apps.qc.models import QCResult
|
||||||
from apps.shipments.models import Shipment
|
from apps.shipments.models import Shipment
|
||||||
from apps.subscriptions.models import Subscription
|
from apps.subscriptions.models import Subscription
|
||||||
|
from apps.users.auth import is_staff_for_role
|
||||||
from apps.users.models import User
|
from apps.users.models import User
|
||||||
|
|
||||||
BUYER_EMAIL = "buyer@greenfieldfoods.example"
|
BUYER_EMAIL = "buyer@greenfieldfoods.example"
|
||||||
|
|
@ -622,7 +623,13 @@ class Command(BaseCommand):
|
||||||
)
|
)
|
||||||
buyer.role = "buyer"
|
buyer.role = "buyer"
|
||||||
buyer.full_name = BUYER_NAME
|
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(
|
Subscription.objects.update_or_create(
|
||||||
user=buyer, plan=Subscription.Plan.PROFESSIONAL,
|
user=buyer, plan=Subscription.Plan.PROFESSIONAL,
|
||||||
defaults={
|
defaults={
|
||||||
|
|
|
||||||
|
|
@ -3,6 +3,9 @@ import re
|
||||||
from django.conf import settings
|
from django.conf import settings
|
||||||
from django.shortcuts import redirect
|
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:
|
class RBACMiddleware:
|
||||||
ROLE_ROUTES = {
|
ROLE_ROUTES = {
|
||||||
|
|
@ -16,22 +19,33 @@ class RBACMiddleware:
|
||||||
|
|
||||||
def __call__(self, request):
|
def __call__(self, request):
|
||||||
user = request.user
|
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
|
path = request.path_info
|
||||||
user_role = getattr(request.user, "role", None)
|
user_role = getattr(request.user, "role", None)
|
||||||
allowed = self.ROLE_ROUTES.get(user_role, [])
|
|
||||||
if any(re.match(pattern, path) for pattern in allowed):
|
# /admin/ is for the admin role. is_staff alone is not enough: an ops
|
||||||
return self.get_response(request)
|
# or buyer account carrying the flag must not reach the
|
||||||
# The path belongs to a portal this role may not enter. Send them to
|
# administration area, whose index embeds the model registry, and
|
||||||
# the role-aware landing view rather than straight into a portal they
|
# where a later group or permission grant would be inherited. A
|
||||||
# cannot use. It used to redirect if *any* role's pattern matched the
|
# superuser (above) is the deliberate exception.
|
||||||
# path without belonging to this role, which bounced a buyer off
|
if ADMIN_PATH.match(path) and user_role != "admin":
|
||||||
# /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 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)
|
return self.get_response(request)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -197,6 +197,59 @@ class RoleRoutingTests(TestCase):
|
||||||
self.assertIn("/oidc/authenticate/", response["Location"])
|
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):
|
class ApiPermissionBoundaryTests(TestCase):
|
||||||
"""The API permission boundary, pinned for every viewset.
|
"""The API permission boundary, pinned for every viewset.
|
||||||
|
|
||||||
|
|
@ -471,6 +524,20 @@ class SeedWalkthroughTests(TestCase):
|
||||||
self.assertEqual(CAPARecord.objects.count(), 1)
|
self.assertEqual(CAPARecord.objects.count(), 1)
|
||||||
self.assertEqual(ProcessingBatch.objects.filter(batch_reference="IXG-TEST-001").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):
|
def test_every_intake_keeps_within_what_it_received(self):
|
||||||
"""The rule the ops lineage screen enforces, applied to the training data.
|
"""The rule the ops lineage screen enforces, applied to the training data.
|
||||||
|
|
||||||
|
|
|
||||||
32
apps/users/migrations/0003_align_is_staff_with_role.py
Normal file
32
apps/users/migrations/0003_align_is_staff_with_role.py
Normal file
|
|
@ -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),
|
||||||
|
]
|
||||||
|
|
@ -59,7 +59,9 @@ body { font-size: .875rem; background: #f5f6fa; }
|
||||||
<div class="topbar">
|
<div class="topbar">
|
||||||
<span></span>
|
<span></span>
|
||||||
<div class="d-flex align-items-center gap-3">
|
<div class="d-flex align-items-center gap-3">
|
||||||
|
{% if request.user.role == 'admin' or request.user.is_superuser %}
|
||||||
<a href="/admin/" class="text-decoration-none text-secondary"><i class="bi bi-shield-lock"></i> Admin</a>
|
<a href="/admin/" class="text-decoration-none text-secondary"><i class="bi bi-shield-lock"></i> Admin</a>
|
||||||
|
{% endif %}
|
||||||
<span class="text-secondary">{{ request.user.full_name|default:request.user.email }}</span>
|
<span class="text-secondary">{{ request.user.full_name|default:request.user.email }}</span>
|
||||||
<form method="post" action="{% url 'oidc_logout' %}" class="m-0">
|
<form method="post" action="{% url 'oidc_logout' %}" class="m-0">
|
||||||
{% csrf_token %}
|
{% csrf_token %}
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue