Repository navigation
Require an attendance record's user to share its session's dataset - #15667
namanjain24-sudo wants to merge 5 commits into
Conversation
Membership and Role both reject a user and collection that belong to different datasets, enforcing it in infer_dataset. AttendanceRecord links the same two things through its session but only looked up the session's dataset, and it has no pre_save, so nothing checked its user at all. On a device hosting more than one facility that let a coach record attendance for a learner in another facility: the row was stored in the coach's dataset while its user foreign key pointed into the other one. The record serializer accepts any user on the device, so the API returned 201 for it. Check both sides in infer_dataset, matching the sibling models. Fixes learningequality#15666
github-actions
Bot
added
DEV: backend
|
👋 Hi @namanjain24-sudo, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
📢✨ Before we assign a reviewer:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. |
|
Confirming both checklist items:
|
Build Artifacts
Smoke test screenshot |
|
One thing I wanted to check before leaving this sitting: changing I traced it and then measured it rather than reasoning about it:
Both reject it; the change only moves the rejection slightly earlier and makes the exception match what Worth noting one measurement trap in case anyone re-runs this: under |
The guard added to infer_dataset does not cover bulk_update: it creates records with bulk_create, which skips pre_save, so ensure_dataset never runs there. A coach could still post a learner from another facility to that endpoint and get a 200. Validate it in BulkUpdateSerializer, where the existing user-id check already lives, so the endpoint answers 400 rather than writing a record whose dataset disagrees with its user's.
|
Pushed a follow-up commit, because the original fix was incomplete and I'd rather flag that myself than have it found in review.
The follow-up validates it in Worth saying plainly: the model-level guard on its own is not sufficient for this module, because any future code path using |
|
One coverage caveat worth knowing when judging this, since it is easy to read the green tick as broader than it is.
So CI green on this PR does not represent sync coverage for the change. That is why I verified the deserialization behaviour locally instead and reported it in the comment above. Flagging it in case you'd like the path filter widened, or would rather I add coverage somewhere that does run. |
|
📢✨ Before we assign a reviewer, we'll turn on |
🟡 Waiting for changesOutcome: reviewed Last updated: 2026-10-28 21:23 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15667: the session endpoint now returns 500 for a cross-dataset user. It returned 201 before; the issue's API repro ran there.
The model-level check mirrors Membership/Role. The bulk endpoint correctly returns 400. CI passing.
- suggestion: reject a cross-dataset user in nested
attendance_recordswith 400, and test it (models.py:113). - nitpick: delete the two test comments that repeat the code and test names.
Next step: Needs a full code review by a human engineer.
- The author is outside the team: confirm this should be built at all.
- Decide whether the session endpoint should reject a cross-dataset nested record with 400, or whether the 500 is acceptable now that the model invariant holds.
- Confirm that adding this check to
infer_datasetcannot break syncing in existing attendance records whose user and session already have different datasets.
Written by rtibblesbot, an LLM-based coding agent.
The nested attendance_records write path saved records straight through the model, so a user from another facility reached infer_dataset and came back as an IntegrityError the exception handler does not catch -- a 500 where the endpoint returned 201 before this branch. Validate the records against the collection's dataset first, sharing the check and its message with the bulk endpoint.
|
Good catch on the 500 — that is a regression this branch introduced and I had missed it. Fixed and pushed. suggestion — 400 for a cross-dataset user in nested The check is shared with nitpick — test comments. Both deleted. On your third question for the human reviewer — whether this check can break syncing for existing records whose user and session already differ — it can, and it is worth a decision rather than an assumption:
So the options look like: accept it, add a data migration that clears or re-points such rows, or let
|
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15667: both prior findings are resolved. One new nitpick is inline. CI is pending.
Prior-finding status
RESOLVED — kolibri/core/attendance/models.py:113 — Cross-dataset user in nested attendance_records returns 500, not 400
RESOLVED — kolibri/core/attendance/test/test_api.py — Test comment repeats the code/test name
Next step: Needs a full code review by a human engineer.
- The author is outside the team: confirm this should be built at all.
Written by rtibblesbot, an LLM-based coding agent.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15667: all 3 prior findings resolved. No new findings. CI still running.
Prior-finding status
RESOLVED — kolibri/core/attendance/models.py:113 — cross-dataset user in nested attendance_records gets a 500, not a 400
RESOLVED — kolibri/core/attendance/test/test_api.py — comment repeats viewsets/attendance_record.py:77
RESOLVED — kolibri/core/attendance/utils.py:6 — docstring restates the signature
Next step: Needs a full code review by a human engineer.
- The author is outside the team: confirm this should be built at all.
Written by rtibblesbot, an LLM-based coding agent.
|
Following up on my own sentence rather than leaving it as an assumption. I wrote above that "the coach UI would not" send a cross-facility user id, which narrows how much existing bad data there could be — but I had not actually checked it. I have now, and it is a stronger guarantee than I claimed. The records the UI sends are built from the class summary's learners: // kolibri/plugins/coach/frontend/composables/useAttendanceForm.js:158-162
return sortedLearners.value.map(learner => ({
user: learner.id,
present: !!attendanceMap.value[learner.id],
}));and if user_dataset_id != collection_dataset_id:
raise KolibriValidationError(
"Collection and user for a Membership object must be in same dataset."
)So a learner cannot be a member of a classroom in another facility in the first place, which means the coach UI has no path to producing a cross-dataset record — not merely that it happens not to. One could still only arise from a hand-written API call against That does not make the question go away, since such a row would still fail to deserialize forever if one exists. But it does suggest the likelihood is low enough that accepting it, rather than writing a data migration, may be the proportionate call. Still your decision — I just did not want the "the UI would not" part resting on my assumption. |
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
The infer_dataset check and the tests are right; the 400 handling is overbuilt.
- blocking: drop
utils.pyand do each endpoint's check inline (inline comments). - Keep the model change and all three tests.
CI passing.
| from rest_framework.serializers import ValidationError | ||
|
|
||
|
|
||
| def validate_users_share_dataset(user_dataset_ids, dataset_id, field): |
There was a problem hiding this comment.
blocking: Please delete this module. Its two callers have to reshape their data to fit it, and each endpoint can do the check in a line or two (see the other comments).
| session = data["attendance_session"] | ||
| user_ids = {r["user"] for r in records} | ||
| existing_ids = set( | ||
| FacilityUser.objects.filter(id__in=user_ids).values_list("id", flat=True) | ||
| users = list( | ||
| FacilityUser.objects.filter(id__in=user_ids).values_list("id", "dataset_id") | ||
| ) | ||
| missing = user_ids - existing_ids | ||
| missing = user_ids - {user_id for user_id, _ in users} | ||
| if missing: | ||
| raise ValidationError( | ||
| {"records": f"Invalid user IDs: {', '.join(sorted(missing))}"} | ||
| ) | ||
| validate_users_share_dataset(users, session.dataset_id, "records") |
There was a problem hiding this comment.
blocking: Scope the existing lookup and revert the rest:
existing_ids = set(
FacilityUser.objects.filter(
id__in=user_ids, dataset_id=data["attendance_session"].dataset_id
).values_list("id", flat=True)
)A cross-dataset user then gets the existing "Invalid user IDs" 400.
| records = attrs.get("attendance_records") | ||
| if records: | ||
| collection = attrs.get("collection") or self.instance.collection | ||
| validate_users_share_dataset( | ||
| [(record["user"].id, record["user"].dataset_id) for record in records], | ||
| collection.dataset_id, | ||
| "attendance_records", | ||
| ) |
There was a problem hiding this comment.
blocking: The records are already FacilityUser objects, so this can be inline:
collection = attrs.get("collection") or self.instance.collection
if any(
r["user"].dataset_id != collection.dataset_id
for r in attrs.get("attendance_records", [])
):
raise ValidationError({"attendance_records": "Invalid user IDs"})The helper made both callers reshape their data to fit it. The bulk endpoint scopes its existing user lookup by the session's dataset instead, so a cross-dataset user falls into the "Invalid user IDs" response it already returns, and the session endpoint compares the records it has already resolved.
|
Done — and the scoped-lookup version is better than what I had, since it reuses the response the endpoint already gives. blocking — blocking — bulk endpoint. The existing user lookup is now scoped by blocking — session endpoint. Inline Model change and all three tests kept. I checked both inlined checks are red when reverted, so the two API tests still cover what they claim.
|
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15667: all 3 prior findings resolved. No new findings. CI pending.
Prior-finding status
RESOLVED — kolibri/core/attendance/models.py:113 — cross-dataset user in nested attendance_records gets a 500, not a 400
RESOLVED — kolibri/core/attendance/test/test_api.py — comment repeats the one at viewsets/attendance_record.py:77
RESOLVED — kolibri/core/attendance/utils.py — docstring restates the signature (file deleted)
Next step: Needs a full code review by a human engineer.
- The author is outside the team: confirm this should be built at all.
Written by rtibblesbot, an LLM-based coding agent.
Summary
AttendanceRecordlinks aFacilityUserto anAttendanceSession, which belongs to aCollection— the same user-to-collection pairing thatMembershipandRolemodel. Both of those reject the two sides belonging to different datasets, and both enforce it ininfer_dataset:AttendanceRecord.infer_datasetonly looked up the session's dataset, and the model has nopre_save, so nothing checked itsuserat all — the basepre_saveonly callsensure_dataset(), and the cross-dataset guardenforce_authoring_user_fieldis opt-in per field and, within the attendance app, is wired only toAttendanceSession.created_by. (It is used more widely elsewhere — lessons, exams and courses all call it — so this is a gap in this module rather than an unused mechanism; corrected after @rtibblesbot flagged the original wording as too broad on #15666.)That gap is reachable from the API: the nested record serializer accepts any user on the device (
user = PrimaryKeyRelatedField(queryset=FacilityUser.objects.all())), permissions are evaluated against the session's collection, andAttendanceSessionSerializer.createthen creates each record without relating itsuserback to that collection. On a device hosting more than one facility, a coach in facility A could record attendance for a learner in facility B and get a201— storing a row in facility A's dataset whoseuserforeign key points into facility B's.This checks both sides in
infer_dataset, matching the sibling models.References
Fixes #15666
Reviewer guidance
pytest kolibri/core/attendance/test/test_models.py -k cross_dataset_useris the regression test. Againstdevelopit fails withAssertionError: IntegrityError not raised; it passes here.pytest kolibri/core/attendance/— 60 passed.pytest kolibri/core/auth/test/— 822 passed, 16 skipped (the base model is shared, so I ran it too). No regressions.prek run --files …clean.201and the row was committed. After: the legitimate same-facility POST still returns201, the cross-facility one is refused, no record is written, and the surroundingtransaction.atomic()increaterolls the session back too, so no orphan session is left behind.IntegrityError, which means a500rather than a400. That is exactly howMembershipbehaves today for the equivalent pairing, so this is consistent rather than new, and I preferred consistency over inventing a different contract for one model. If you'd rather it be a validation error, the natural place is the record serializer, and I'm happy to add that here or in a follow-up.userand the session's collection, but that is a membership-validation question rather than a dataset-integrity one, and the answer ("must the learner be enrolled?") is a product decision. I noted it in AttendanceRecord does not require its user and session to share a dataset #15666 rather than deciding it here.AI usage
Claude Code (on my instruction to scan the newer code deeply for a real bug and then fix it) found this by comparing
AttendanceRecordagainst the two existing models that link a user to a collection and noticing it was the only one not enforcing the shared-dataset invariant. It confirmed the behaviour by running code rather than by reading: one probe creating the same cross-facility pairing through both models, showingMembershipraising whileAttendanceRecordcommitted a mismatched row, and another driving the real API as a logged-in coach, which returned201. It then worked test-first — the regression test went in before the change and was confirmed red — and re-ran the API probe afterwards to check the legitimate path still returns201and that no orphan session survives the rollback. It also caught and corrected its own mistake mid-change, having first importedKolibriValidationErrorfrom the wrong module. I reviewed the comparison againstMembership/Role, the decision to match theirIntegrityErrorbehaviour rather than invent a400, the choice to leave the classroom-membership question out of scope, and every test run above before opening this PR.