diff --git a/kolibri/core/attendance/models.py b/kolibri/core/attendance/models.py index 6db0a6161dc..6ca38271a5c 100644 --- a/kolibri/core/attendance/models.py +++ b/kolibri/core/attendance/models.py @@ -1,5 +1,4 @@ from django.db import models -from django.db.utils import IntegrityError from kolibri.core.auth.constants import role_kinds from kolibri.core.auth.models import AbstractFacilityDataModel @@ -50,14 +49,7 @@ def __str__(self): def pre_save(self, **kwargs): super().pre_save(**kwargs) - if self._state.adding and self.created_by is None: - raise IntegrityError("AttendanceSession must be saved with a creator") - if self.created_by and self.created_by.dataset_id != self.dataset_id: - if not self.created_by.is_superuser: - raise IntegrityError( - "AttendanceSession must have creator in the same dataset" - ) - self.created_by = None + self.enforce_authoring_user_field("created_by", **kwargs) def infer_dataset(self, *args, **kwargs): return self.cached_related_dataset_lookup("collection") diff --git a/kolibri/core/auth/models.py b/kolibri/core/auth/models.py index 582b4f5f02c..a7add382cfe 100644 --- a/kolibri/core/auth/models.py +++ b/kolibri/core/auth/models.py @@ -397,6 +397,40 @@ def pre_save(self, **kwargs): except KolibriValidationError as e: raise IntegrityError(str(e)) + def enforce_authoring_user_field(self, field_name, **save_kwargs): + """ + Enforce and sanitize an "authoring" foreign key to a ``FacilityUser`` -- e.g. + ``creator``, ``created_by``, ``assigned_by`` or ``activated_by``. + + Null is rejected only on local creation (``_state.adding``). Updates preserve + the existing author and deserialization may carry a null (e.g. the author's + ``FacilityUser`` has not synced to this device); both leave a null untouched. + Morango signals deserialization by passing ``update_dirty_bit_to=False``. + + A cross-dataset superuser reference -- the device's own super admin authoring + content in a synced-in facility -- is dropped to keep the record syncable; any + other cross-dataset user is rejected. + + The field must be ``blank=True, null=True`` so ``clean_fields`` accepts the + nulled value during deserialization. + """ + user = getattr(self, field_name) + is_deserialization = save_kwargs.get("update_dirty_bit_to") is False + if self._state.adding and not is_deserialization and user is None: + raise IntegrityError( + "{model}.{field} may not be null".format( + model=type(self).__name__, field=field_name + ) + ) + if user and user.dataset_id != self.dataset_id: + if not user.is_superuser: + raise IntegrityError( + "{model}.{field} must belong to the same dataset".format( + model=type(self).__name__, field=field_name + ) + ) + setattr(self, field_name, None) + def save(self, *args, **kwargs): self.pre_save(**kwargs) super().save(*args, **kwargs) diff --git a/kolibri/core/auth/test/test_morango_integration.py b/kolibri/core/auth/test/test_morango_integration.py index 1bbcfb1545b..dd9906cc391 100644 --- a/kolibri/core/auth/test/test_morango_integration.py +++ b/kolibri/core/auth/test/test_morango_integration.py @@ -33,8 +33,11 @@ from ..models import LearnerGroup from ..models import Membership from ..models import Role +from .helpers import create_superuser from .helpers import DUMMY_PASSWORD +from .helpers import provision_device from .sync_utils import multiple_kolibri_servers +from kolibri.core.attendance.models import AttendanceSession from kolibri.core.auth.constants import role_kinds from kolibri.core.auth.management.utils import get_client_and_server_certs from kolibri.core.auth.utils.sync import find_soud_sync_session_for_resume @@ -92,6 +95,106 @@ def test_deserializing_field(self): self.fail(e.message) +class CrossDatasetSuperuserDeserializationTestCase(TransactionTestCase): + """ + Regression test for quizzes and lessons authored by a superuser that belongs to a + different dataset than the facility the content lives in (e.g. a device's own super + admin creating content in a facility that was synced onto the device). + + In that case the models' ``pre_save`` deliberately nulls the creator/assigned_by + foreign key. The record must still deserialize on a receiving device: previously the + fields were ``blank=False``, so ``clean_fields()`` (which Morango runs during + deserialization) rejected the null value and the record stayed dirty in the Store, + never appearing on the receiving device. + """ + + def setUp(self): + self.controller = MorangoProfileController(PROFILE_FACILITY_DATA) + InstanceIDModel.get_or_create_current_instance() + provision_device() + # the facility the content lives in + self.facility = Facility.objects.create(name="Synced") + self.classroom = Classroom.objects.create(name="Class", parent=self.facility) + # a superuser belonging to a different dataset + self.other_facility = Facility.objects.create(name="Device") + self.superuser = create_superuser(self.other_facility, username="deviceadmin") + + def _assert_deserializes_cleanly(self, *instance_ids): + self.controller.serialize_into_store() + Store.objects.update(dirty_bit=True) + self.controller.deserialize_from_store() + for instance_id in instance_ids: + store = Store.objects.get(id=instance_id) + self.assertIsNone( + store.deserialization_error, + msg="{} failed to deserialize: {}".format( + store.model_name, store.deserialization_error + ), + ) + self.assertFalse(store.dirty_bit) + + def test_exam_authored_by_cross_dataset_superuser_deserializes(self): + exam = Exam.objects.create( + title="Quiz", + question_count=1, + question_sources=[ + { + "exercise_id": uuid.uuid4().hex, + "question_id": uuid.uuid4().hex, + "title": "a", + } + ], + collection=self.classroom, + creator=self.superuser, + active=True, + ) + assignment = ExamAssignment.objects.create( + exam=exam, + collection=self.classroom, + assigned_by=self.superuser, + ) + # pre_save nulls the cross-dataset superuser foreign keys + self.assertIsNone(Exam.objects.get(id=exam.id).creator_id) + self.assertIsNone(ExamAssignment.objects.get(id=assignment.id).assigned_by_id) + + self._assert_deserializes_cleanly(exam.id, assignment.id) + + def test_lesson_authored_by_cross_dataset_superuser_deserializes(self): + lesson = Lesson.objects.create( + title="Lesson", + resources=[ + { + "contentnode_id": uuid.uuid4().hex, + "content_id": uuid.uuid4().hex, + "channel_id": uuid.uuid4().hex, + } + ], + collection=self.classroom, + created_by=self.superuser, + is_active=True, + ) + assignment = LessonAssignment.objects.create( + lesson=lesson, + collection=self.classroom, + assigned_by=self.superuser, + ) + # pre_save nulls the cross-dataset superuser foreign keys + self.assertIsNone(Lesson.objects.get(id=lesson.id).created_by_id) + self.assertIsNone(LessonAssignment.objects.get(id=assignment.id).assigned_by_id) + + self._assert_deserializes_cleanly(lesson.id, assignment.id) + + def test_attendance_session_authored_by_cross_dataset_superuser_deserializes(self): + session = AttendanceSession.objects.create( + collection=self.classroom, + created_by=self.superuser, + ) + # pre_save nulls the cross-dataset superuser foreign key + self.assertIsNone(AttendanceSession.objects.get(id=session.id).created_by_id) + + self._assert_deserializes_cleanly(session.id) + + class MultipleServerTestCase(TestCase): """ A test case to do special teardown handling to prevent errors from our additional databases. diff --git a/kolibri/core/courses/models.py b/kolibri/core/courses/models.py index ade176fcee2..6abbd18604c 100644 --- a/kolibri/core/courses/models.py +++ b/kolibri/core/courses/models.py @@ -321,20 +321,7 @@ def get_course_content_download_priority(self, contentnode_id): def pre_save(self, **kwargs): super().pre_save(**kwargs) - - is_deserialization = kwargs.get("update_dirty_bit_to") is False - - if not is_deserialization and self.created_by is None: - raise IntegrityError( - "CourseSession must be saved with a non None created_by field" - ) - - if self.created_by and self.created_by.dataset_id != self.dataset_id: - if not self.created_by.is_superuser: - raise IntegrityError( - "CourseSession must have creator in the same dataset" - ) - self.created_by = None + self.enforce_authoring_user_field("created_by", **kwargs) def infer_dataset(self, *args, **kwargs): return self.cached_related_dataset_lookup("collection") @@ -427,22 +414,7 @@ def pre_save(self, **kwargs): "CourseSession assignment foreign models must be in same dataset" ) - is_deserialization = kwargs.get("update_dirty_bit_to") is False - - if not is_deserialization and self.assigned_by is None: - raise IntegrityError( - "CourseSession assignment must be saved with a non None assigned_by field" - ) - - # validate that datasets match so this would be syncable - if self.assigned_by and self.assigned_by.dataset_id != self.dataset_id: - # the only time assigned_by can be null is if it's a superuser - # and if we set it to none HERE - if not self.assigned_by.is_superuser: - raise IntegrityError( - "CourseSession assignment must have assigner in the same dataset" - ) - self.assigned_by = None + self.enforce_authoring_user_field("assigned_by", **kwargs) def infer_dataset(self, *args, **kwargs): # infer from course_session so assignments align with course_sessions @@ -614,20 +586,7 @@ def pre_save(self, **kwargs): "UnitTestAssignment collection must be the same as or a child of the CourseSession's collection" ) - is_deserialization = kwargs.get("update_dirty_bit_to") is False - - if not is_deserialization and self.activated_by is None: - raise IntegrityError( - "UnitTestAssignment must be saved with an non None activated_by field" - ) - - if self.activated_by and self.activated_by.dataset_id != self.dataset_id: - # Only allow null for superusers - if not self.activated_by.is_superuser: - raise IntegrityError( - "UnitTestAssignment activated_by must be in the same dataset" - ) - self.activated_by = None + self.enforce_authoring_user_field("activated_by", **kwargs) @classmethod def deserialize(cls, dict_model, sync_filter=None): diff --git a/kolibri/core/courses/test/test_unit_test_assignment.py b/kolibri/core/courses/test/test_unit_test_assignment.py index d77f16be68f..78fbe5e8024 100644 --- a/kolibri/core/courses/test/test_unit_test_assignment.py +++ b/kolibri/core/courses/test/test_unit_test_assignment.py @@ -448,6 +448,17 @@ def test_normal_save_passes_with_user_field_set(self): instance.refresh_from_db() self.assertIsNotNone(getattr(instance, self.user_field_name)) + def test_update_of_existing_record_allows_null_user_field(self): + # A record can legitimately have a null authoring field if it synced in + # from another dataset (the cross-dataset superuser author was dropped). + # Updating such a record locally afterwards must not re-raise; the null + # check only applies when the record is first created. + instance = self.build_instance(with_user=False) + instance.save(update_dirty_bit_to=False) + instance.save() + instance.refresh_from_db() + self.assertIsNone(getattr(instance, self.user_field_name)) + class CourseSessionPreSaveTestCase(PreSaveKwargsTestMixin, TestCase): model_class = models.CourseSession diff --git a/kolibri/core/exams/migrations/0011_allow_blank_creator_and_assigned_by.py b/kolibri/core/exams/migrations/0011_allow_blank_creator_and_assigned_by.py new file mode 100644 index 00000000000..aef6bf90413 --- /dev/null +++ b/kolibri/core/exams/migrations/0011_allow_blank_creator_and_assigned_by.py @@ -0,0 +1,47 @@ +# Generated by Django 3.2.25 on 2026-05-27 03:04 +import django.db.models.deletion +from django.db import migrations +from django.db import models + + +class Migration(migrations.Migration): + + dependencies = [ + ("exams", "0010_add_exam_report_visibility_field"), + ] + + operations = [ + migrations.AlterField( + model_name="draftexam", + name="creator", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="draftexams", + to="kolibriauth.FacilityUser", + ), + ), + migrations.AlterField( + model_name="exam", + name="creator", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="exams", + to="kolibriauth.FacilityUser", + ), + ), + migrations.AlterField( + model_name="examassignment", + name="assigned_by", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="assigned_exams", + to="kolibriauth.FacilityUser", + ), + ), + ] diff --git a/kolibri/core/exams/models.py b/kolibri/core/exams/models.py index 1bfb3dc7bd9..f81ab36c692 100644 --- a/kolibri/core/exams/models.py +++ b/kolibri/core/exams/models.py @@ -141,7 +141,7 @@ class Meta: creator = models.ForeignKey( FacilityUser, related_name="%(class)ss", - blank=False, + blank=True, null=True, on_delete=models.CASCADE, ) @@ -267,18 +267,7 @@ def delete(self, using=None, keep_parents=False): def pre_save(self, **kwargs): super().pre_save(**kwargs) - - # maintain stricter enforcement on when creator is allowed to be null - if self._state.adding and self.creator is None: - raise IntegrityError("Exam must be saved with an creator") - - # validate that datasets match so this would be syncable - if self.creator and self.creator.dataset_id != self.dataset_id: - # the only time creator can be null is if it's a superuser - # and if we set it to none HERE - if not self.creator.is_superuser: - raise IntegrityError("Exam must have creator in the same dataset") - self.creator = None + self.enforce_authoring_user_field("creator", **kwargs) def save(self, *args, **kwargs): # If archive is True during the save op, but there is no date_archived then @@ -342,7 +331,7 @@ class ExamAssignment(AbstractFacilityDataModel): assigned_by = models.ForeignKey( FacilityUser, related_name="assigned_exams", - blank=False, + blank=True, null=True, on_delete=models.CASCADE, ) @@ -360,21 +349,7 @@ def pre_save(self, **kwargs): "Exam assignment foreign models must be in same dataset" ) - # maintain stricter enforcement on when assigned_by is allowed to be null - # assignments aren't usually updated, but ensure only during creation - if self._state.adding and self.assigned_by is None: - raise IntegrityError("Exam assignment must be saved with an assigner") - - # validate that datasets match so this would be syncable - if self.assigned_by and self.assigned_by.dataset_id != self.dataset_id: - # the only time assigned_by can be null is if it's a superuser - # and if we set it to none HERE - if not self.assigned_by.is_superuser: - # maintain stricter enforcement on when assigned_by is allowed to be null - raise IntegrityError( - "Exam assignment must have assigner in the same dataset" - ) - self.assigned_by = None + self.enforce_authoring_user_field("assigned_by", **kwargs) def infer_dataset(self, *args, **kwargs): # infer from exam so assignments align with exams diff --git a/kolibri/core/lessons/migrations/0005_allow_blank_creator_and_assigned_by.py b/kolibri/core/lessons/migrations/0005_allow_blank_creator_and_assigned_by.py new file mode 100644 index 00000000000..72be6c812f3 --- /dev/null +++ b/kolibri/core/lessons/migrations/0005_allow_blank_creator_and_assigned_by.py @@ -0,0 +1,36 @@ +# Generated by Django 3.2.25 on 2026-05-27 03:04 +import django.db.models.deletion +from django.db import migrations +from django.db import models + + +class Migration(migrations.Migration): + + dependencies = [ + ("lessons", "0004_nullable_created_by_assigned_by"), + ] + + operations = [ + migrations.AlterField( + model_name="lesson", + name="created_by", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="lessons_created", + to="kolibriauth.FacilityUser", + ), + ), + migrations.AlterField( + model_name="lessonassignment", + name="assigned_by", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="assigned_lessons", + to="kolibriauth.FacilityUser", + ), + ), + ] diff --git a/kolibri/core/lessons/models.py b/kolibri/core/lessons/models.py index 7b295b55142..960be4d6f97 100644 --- a/kolibri/core/lessons/models.py +++ b/kolibri/core/lessons/models.py @@ -65,7 +65,7 @@ class Lesson(AbstractFacilityDataModel): created_by = models.ForeignKey( FacilityUser, related_name="lessons_created", - blank=False, + blank=True, null=True, on_delete=models.CASCADE, ) @@ -96,17 +96,7 @@ def __str__(self): def pre_save(self, **kwargs): super().pre_save(**kwargs) - - # maintain stricter enforcement on when assigned_by is allowed to be null - if self._state.adding and self.created_by is None: - raise IntegrityError("Lesson must be saved with a creator") - - # if the created_by user is in a different dataset than the dataset we've determined, then - # we'll unset it to prevent deserialization errors when syncing - if self.created_by and self.created_by.dataset_id != self.dataset_id: - if not self.created_by.is_superuser: - raise IntegrityError("Lesson must have creator in the same dataset") - self.created_by = None + self.enforce_authoring_user_field("created_by", **kwargs) def delete(self, using=None, keep_parents=False): """ @@ -152,7 +142,7 @@ class LessonAssignment(AbstractFacilityDataModel): assigned_by = models.ForeignKey( FacilityUser, related_name="assigned_lessons", - blank=False, + blank=True, null=True, on_delete=models.CASCADE, ) @@ -178,20 +168,7 @@ def pre_save(self, **kwargs): "Lesson assignment foreign models must be in same dataset" ) - # maintain stricter enforcement on when assigned_by is allowed to be null - # assignments aren't usually updated, but ensure only during creation - if self._state.adding and self.assigned_by is None: - raise IntegrityError("Lesson assignment must be saved with an assigner") - - # validate that datasets match so this would be syncable - if self.assigned_by and self.assigned_by.dataset_id != self.dataset_id: - # the only time assigned_by can be null is if it's a superuser - # and if we set it to none HERE - if not self.assigned_by.is_superuser: - raise IntegrityError( - "Lesson assignment must have assigner in the same dataset" - ) - self.assigned_by = None + self.enforce_authoring_user_field("assigned_by", **kwargs) def infer_dataset(self, *args, **kwargs): # infer from lesson so assignments align with lessons diff --git a/kolibri/core/lessons/test/test_lesson_api.py b/kolibri/core/lessons/test/test_lesson_api.py index 32e413e1afe..8c5958ab796 100644 --- a/kolibri/core/lessons/test/test_lesson_api.py +++ b/kolibri/core/lessons/test/test_lesson_api.py @@ -401,6 +401,33 @@ def test_can_update_lesson_multiple_same_title(self): ) self.assertEqual(response.status_code, status.HTTP_200_OK) + def test_can_update_lesson_with_null_created_by(self): + # A lesson synced in from another dataset may have a null created_by (its + # original cross-dataset superuser author is dropped on sync). Editing + # such a lesson locally must not raise. Regression test for an + # IntegrityError raised on PATCH ("Lesson.created_by may not be null"). + lesson = models.Lesson( + title="synced lesson", + is_active=True, + collection=self.classroom, + created_by=None, + ) + lesson.save(update_dirty_bit_to=False) + + self.client.login(username=self.admin.username, password=DUMMY_PASSWORD) + + response = self.client.patch( + reverse("kolibri:core:lesson-detail", kwargs={"pk": lesson.id}), + { + "id": lesson.id, + "title": "synced lesson", + "active": False, + "collection": self.classroom.id, + }, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + def test_can_update_lesson_to_same_title_as_other_lesson(self): self.client.login(username=self.admin.username, password=DUMMY_PASSWORD)