Staff cannot book a lesson for a child or an unapproved student, and group enrolment vets nothing #185

New Issue

2026-08-24T21:42:24Z

thatguygriff commented

2026-08-24 21:42:24 +00:00

Found while testing the new Book a lesson for a student panel: picking a student and submitting gave "Choose a student to book for." and wiped the form. Three separate defects behind it, all in the staff-registers-someone-else paths.

1. The picker and the guard disagree about who is a student

AdminBooking::formData() builds the student select from the us_student role:

get_users( [ 'role' => RoleManager::STUDENT, ... ] )

but AdminBooking::book() vets the submitted id with the capability:

! user_can( $studentId, RoleManager::CAP_BOOK_LESSON )

Two user_has_cap filters strip book_lesson from accounts that keep the role:

Guardian\ChildLoginGate::withholdBooking() — every guardian-managed child
Auth\RegistrationLoginGate::withholdBookingWhilePending() — any self-signup not yet approved

So the panel offers both and then refuses them. Worse, it refuses with a message saying no student was chosen, when one was.

Withholding book_lesson is meant to stop those accounts booking in their own name — a child's account is never signed in to at all, and an unapproved signup is waiting on us. It was never meant to stop the studio booking for them, which is the entire purpose of the panel, and for a child is the only route to a lesson besides their guardian.

The docblock on AdminBooking::studentOptions() asserts the opposite of what the code does — "students and the children a guardian books for alike, since both hold book_lesson" — which is probably how this got in.

Expected: anyone the panel offers can be booked for. Guard the student role, not the capability.

2. A refused booking clears all five fields

templates/admin/lessons.php renders the panel with no repopulation from $_POST, so any refusal loses the student, time, lesson type, both tickboxes and the note. The panel is only ever reopened because something was refused, so this is the path that matters — and it still bites on the other refusals (slot_taken, offering_mismatch, not_weekly) even once #1 is fixed.

Expected: a refusal comes back with the fields as submitted; a success leaves an empty form.

3. Group enrolment does not check its ids at all

The mirror-image bug in GroupClassController. postedStudentIds() coerces the multi-select to positive ints and hands them straight to addDirect() and grantAccess() with no eligibility check whatsoever — so unlike private lessons it never refuses a child, but it will equally happily enrol an instructor, an administrator, or an account deleted since the page was drawn. For a priced class addDirect() then raises a real payment against them.

Reachable from a stale page as well as a tampered POST.

Expected: both controls vet each id and skip anything that is not a student — while still accepting children and unapproved signups, who are students.

Suggested shape

One shared predicate for "may the studio register this person?", used by both the pickers and the guards so they cannot drift apart again — which is precisely what caused #1.

Found while testing the new **Book a lesson for a student** panel: picking a student and submitting gave **"Choose a student to book for."** and wiped the form. Three separate defects behind it, all in the staff-registers-someone-else paths. ## 1. The picker and the guard disagree about who is a student `AdminBooking::formData()` builds the student select from the `us_student` **role**: ```php get_users( [ 'role' => RoleManager::STUDENT, ... ] ) ``` but `AdminBooking::book()` vets the submitted id with the **capability**: ```php ! user_can( $studentId, RoleManager::CAP_BOOK_LESSON ) ``` Two `user_has_cap` filters strip `book_lesson` from accounts that keep the role: - `Guardian\ChildLoginGate::withholdBooking()` — every guardian-managed child - `Auth\RegistrationLoginGate::withholdBookingWhilePending()` — any self-signup not yet approved So the panel offers both and then refuses them. Worse, it refuses with a message saying no student was chosen, when one was. Withholding `book_lesson` is meant to stop *those accounts* booking in their own name — a child's account is never signed in to at all, and an unapproved signup is waiting on us. It was never meant to stop the studio booking **for** them, which is the entire purpose of the panel, and for a child is the only route to a lesson besides their guardian. The docblock on `AdminBooking::studentOptions()` asserts the opposite of what the code does — *"students and the children a guardian books for alike, since both hold `book_lesson`"* — which is probably how this got in. **Expected:** anyone the panel offers can be booked for. Guard the student role, not the capability. ## 2. A refused booking clears all five fields `templates/admin/lessons.php` renders the panel with no repopulation from `$_POST`, so any refusal loses the student, time, lesson type, both tickboxes and the note. The panel is only ever reopened *because* something was refused, so this is the path that matters — and it still bites on the other refusals (`slot_taken`, `offering_mismatch`, `not_weekly`) even once #1 is fixed. **Expected:** a refusal comes back with the fields as submitted; a success leaves an empty form. ## 3. Group enrolment does not check its ids at all The mirror-image bug in `GroupClassController`. `postedStudentIds()` coerces the multi-select to positive ints and hands them straight to `addDirect()` and `grantAccess()` with no eligibility check whatsoever — so unlike private lessons it never refuses a child, but it will equally happily enrol an **instructor**, an **administrator**, or an account **deleted since the page was drawn**. For a priced class `addDirect()` then raises a real payment against them. Reachable from a stale page as well as a tampered POST. **Expected:** both controls vet each id and skip anything that is not a student — while still accepting children and unapproved signups, who are students. ## Suggested shape One shared predicate for "may the studio register this person?", used by both the pickers and the guards so they cannot drift apart again — which is precisely what caused #1.

thatguygriff added the bug security labels 2026-08-24 21:42:24 +00:00

thatguygriff referenced this issue from a commit

2026-08-24 21:43:07 +00:00

Let the studio register the students who cannot register themselves

thatguygriff referenced a pull request that will close this issue

2026-08-24 21:43:29 +00:00

Let the studio register the students who cannot register themselves #186

thatguygriff closed this issue

2026-08-24 22:03:12 +00:00

Sign in to join this conversation.