From 8eccf6e123ae5cc8fe1312166013b502e1975a73 Mon Sep 17 00:00:00 2001 From: Abdul Muqadim Date: Sun, 5 Jul 2026 01:38:09 +0500 Subject: [PATCH 1/2] feat: grant staff access to superusers automatically Django's is_superuser and is_staff are independent flags, so a superuser does not get the is_staff access that gates the Django admin and various staff-only Studio/LMS views. Since a superuser can grant itself is_staff at any time, that split offers no protection and is only surprising. Add a pre_save signal on the User model that marks any superuser as staff, and a data migration to backfill existing superusers. Discussion: https://discuss.openedx.org/t/shouldnt-superuser-automatically-inherit-staff-access-in-open-edx/18657 --- ...0050_grant_staff_to_existing_superusers.py | 38 +++++++++++++++++ .../djangoapps/student/signals/receivers.py | 23 ++++++++++ .../student/tests/test_receivers.py | 42 +++++++++++++++++++ 3 files changed, 103 insertions(+) create mode 100644 common/djangoapps/student/migrations/0050_grant_staff_to_existing_superusers.py diff --git a/common/djangoapps/student/migrations/0050_grant_staff_to_existing_superusers.py b/common/djangoapps/student/migrations/0050_grant_staff_to_existing_superusers.py new file mode 100644 index 000000000000..ef6fb11a2b80 --- /dev/null +++ b/common/djangoapps/student/migrations/0050_grant_staff_to_existing_superusers.py @@ -0,0 +1,38 @@ +""" +Backfill ``is_staff`` for existing superusers. + +A new ``pre_save`` signal keeps ``is_staff`` in sync with ``is_superuser`` for +any user saved from now on, but that does not touch superusers that already +exist in the database. This one-time data migration grants staff access to +those accounts so the behaviour is consistent for old and new superusers alike. +""" + +from django.conf import settings +from django.db import migrations + + +def grant_staff_to_superusers(apps, schema_editor): + """Mark every existing superuser as staff.""" + User = apps.get_model(*settings.AUTH_USER_MODEL.split('.', 1)) + User.objects.filter(is_superuser=True, is_staff=False).update(is_staff=True) + + +def noop(apps, schema_editor): + """ + Reverse is a no-op. + + We cannot know which superusers were intentionally non-staff before this + migration ran, so removing ``is_staff`` on reverse would be unsafe. + """ + + +class Migration(migrations.Migration): + + dependencies = [ + ('student', '0049_manualenrollmentaudit_statetransition_typo'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.RunPython(grant_staff_to_superusers, noop), + ] diff --git a/common/djangoapps/student/signals/receivers.py b/common/djangoapps/student/signals/receivers.py index d503a746bfd1..c7b7077c3b90 100644 --- a/common/djangoapps/student/signals/receivers.py +++ b/common/djangoapps/student/signals/receivers.py @@ -66,6 +66,29 @@ def on_user_updated(sender, instance, **kwargs): ) +@receiver(pre_save, sender=get_user_model()) +def grant_staff_access_to_superusers(sender, instance, **kwargs): + """ + Keep Django's ``is_staff`` flag in sync with ``is_superuser``. + + A superuser bypasses Django's permission checks and can grant itself the + ``is_staff`` flag at any time, so the state where a user is a superuser but + not staff is confusing and offers no real protection: it just hides the + Django admin and various staff-only Studio/LMS views from that account for + no good reason. To avoid that surprise, ensure every superuser is also + marked as staff. + + See: + https://discuss.openedx.org/t/shouldnt-superuser-automatically-inherit-staff-access-in-open-edx/18657 + """ + # Don't interfere with objects being loaded verbatim from a fixture. + if kwargs.get('raw'): + return + + if instance.is_superuser and not instance.is_staff: + instance.is_staff = True + + @receiver(post_save, sender=CourseEnrollment) def create_course_enrollment_celebration(sender, instance, created, **kwargs): """ diff --git a/common/djangoapps/student/tests/test_receivers.py b/common/djangoapps/student/tests/test_receivers.py index 4dacc2f1b9c5..c29d69532e72 100644 --- a/common/djangoapps/student/tests/test_receivers.py +++ b/common/djangoapps/student/tests/test_receivers.py @@ -3,6 +3,8 @@ from unittest import skipUnless from unittest.mock import patch +from django.contrib.auth import get_user_model +from django.test import TestCase from edx_toggles.toggles.testutils import override_waffle_flag from common.djangoapps.student.models import CourseEnrollmentCelebration, PendingNameChange, UserProfile @@ -92,3 +94,43 @@ def test_listen_for_user_email_changed(self, mock_get_email_client): assert mock_get_email_client.called assert request.session.get('email', None) == user.email + + +class GrantStaffToSuperusersTest(TestCase): + """ + Tests for the ``grant_staff_access_to_superusers`` pre_save receiver, which + keeps ``is_staff`` in sync with ``is_superuser``. + """ + + def test_new_superuser_is_marked_staff(self): + """ A user created as a superuser is automatically marked as staff. """ + user = get_user_model().objects.create( + username='super', email='super@test.com', is_superuser=True, + ) + user.refresh_from_db() + assert user.is_superuser is True + assert user.is_staff is True + + def test_promoting_to_superuser_grants_staff(self): + """ Promoting an existing non-staff user to superuser grants staff access. """ + user = UserFactory(is_superuser=False, is_staff=False) + assert user.is_staff is False + + user.is_superuser = True + user.save() + + user.refresh_from_db() + assert user.is_staff is True + + def test_non_superuser_is_not_forced_to_staff(self): + """ A regular, non-superuser user keeps its (non-)staff status untouched. """ + user = UserFactory(is_superuser=False, is_staff=False) + user.refresh_from_db() + assert user.is_staff is False + + def test_existing_staff_superuser_unchanged(self): + """ A superuser that is already staff stays staff. """ + user = UserFactory(is_superuser=True, is_staff=True) + user.save() + user.refresh_from_db() + assert user.is_staff is True From bede33fe5a44f8d87f0c862193ba6d2ba8c1c930 Mon Sep 17 00:00:00 2001 From: Abdul Muqadim Date: Mon, 6 Jul 2026 22:58:10 +0500 Subject: [PATCH 2/2] refactor: address review feedback on superuser staff grant - Reword the receiver, migration, and test docstrings to describe the behaviour as grant-only rather than keeping is_staff "in sync" with is_superuser, since demoting a superuser never revokes is_staff. - Exercise the pre_save receiver with an explicit save() in the non-superuser test instead of relying only on factory creation. - Add test_demotion_does_not_remove_staff to lock in the one-directional contract. --- ...0050_grant_staff_to_existing_superusers.py | 8 ++++---- .../djangoapps/student/signals/receivers.py | 6 +++++- .../student/tests/test_receivers.py | 20 +++++++++++++++++-- 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/common/djangoapps/student/migrations/0050_grant_staff_to_existing_superusers.py b/common/djangoapps/student/migrations/0050_grant_staff_to_existing_superusers.py index ef6fb11a2b80..b313783d4feb 100644 --- a/common/djangoapps/student/migrations/0050_grant_staff_to_existing_superusers.py +++ b/common/djangoapps/student/migrations/0050_grant_staff_to_existing_superusers.py @@ -1,10 +1,10 @@ """ Backfill ``is_staff`` for existing superusers. -A new ``pre_save`` signal keeps ``is_staff`` in sync with ``is_superuser`` for -any user saved from now on, but that does not touch superusers that already -exist in the database. This one-time data migration grants staff access to -those accounts so the behaviour is consistent for old and new superusers alike. +A new ``pre_save`` signal grants ``is_staff`` to any superuser saved from now +on, but that does not touch superusers that already exist in the database. This +one-time data migration grants staff access to those accounts so the behaviour +is consistent for old and new superusers alike. """ from django.conf import settings diff --git a/common/djangoapps/student/signals/receivers.py b/common/djangoapps/student/signals/receivers.py index c7b7077c3b90..bc5e8cad841a 100644 --- a/common/djangoapps/student/signals/receivers.py +++ b/common/djangoapps/student/signals/receivers.py @@ -69,7 +69,7 @@ def on_user_updated(sender, instance, **kwargs): @receiver(pre_save, sender=get_user_model()) def grant_staff_access_to_superusers(sender, instance, **kwargs): """ - Keep Django's ``is_staff`` flag in sync with ``is_superuser``. + Grant Django's ``is_staff`` flag to any superuser being saved. A superuser bypasses Django's permission checks and can grant itself the ``is_staff`` flag at any time, so the state where a user is a superuser but @@ -78,6 +78,10 @@ def grant_staff_access_to_superusers(sender, instance, **kwargs): no good reason. To avoid that surprise, ensure every superuser is also marked as staff. + This is intentionally one-directional (grant-only): it only ever adds + ``is_staff`` to superusers and never removes it, so demoting a superuser + leaves ``is_staff`` untouched. + See: https://discuss.openedx.org/t/shouldnt-superuser-automatically-inherit-staff-access-in-open-edx/18657 """ diff --git a/common/djangoapps/student/tests/test_receivers.py b/common/djangoapps/student/tests/test_receivers.py index c29d69532e72..87ab56df21d4 100644 --- a/common/djangoapps/student/tests/test_receivers.py +++ b/common/djangoapps/student/tests/test_receivers.py @@ -99,7 +99,8 @@ def test_listen_for_user_email_changed(self, mock_get_email_client): class GrantStaffToSuperusersTest(TestCase): """ Tests for the ``grant_staff_access_to_superusers`` pre_save receiver, which - keeps ``is_staff`` in sync with ``is_superuser``. + grants ``is_staff`` to any superuser. The behaviour is grant-only: it never + removes ``is_staff`` when a user stops being a superuser. """ def test_new_superuser_is_marked_staff(self): @@ -123,8 +124,12 @@ def test_promoting_to_superuser_grants_staff(self): assert user.is_staff is True def test_non_superuser_is_not_forced_to_staff(self): - """ A regular, non-superuser user keeps its (non-)staff status untouched. """ + """ Saving a regular, non-superuser user leaves its non-staff status untouched. """ user = UserFactory(is_superuser=False, is_staff=False) + + # Explicitly re-save to exercise the pre_save receiver on an update. + user.save() + user.refresh_from_db() assert user.is_staff is False @@ -134,3 +139,14 @@ def test_existing_staff_superuser_unchanged(self): user.save() user.refresh_from_db() assert user.is_staff is True + + def test_demotion_does_not_remove_staff(self): + """ Dropping ``is_superuser`` is grant-only, so ``is_staff`` is left in place. """ + user = UserFactory(is_superuser=True, is_staff=True) + + user.is_superuser = False + user.save() + + user.refresh_from_db() + assert user.is_superuser is False + assert user.is_staff is True