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..b313783d4feb --- /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 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 +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..bc5e8cad841a 100644 --- a/common/djangoapps/student/signals/receivers.py +++ b/common/djangoapps/student/signals/receivers.py @@ -66,6 +66,33 @@ def on_user_updated(sender, instance, **kwargs): ) +@receiver(pre_save, sender=get_user_model()) +def grant_staff_access_to_superusers(sender, instance, **kwargs): + """ + 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 + 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. + + 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 + """ + # 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..87ab56df21d4 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,59 @@ 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 + 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): + """ 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): + """ 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 + + 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 + + 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