Week 21: Access Has to Be Checked Where the Data Moves

Eleven pull requests opened this week: five for subgroups on the 19th and 20th, four for circuit templates on the 20th, and two more
for templates on the 23rd and the 25th. The subject changed halfway through. The review findings did not. Almost every comment, on both features,
was the same question in a different place: someone is about to be given something, and was anyone checking whether they should have
it at that moment?

The subgroup screen, with no new JavaScript

#8053 is the part an instructor actually touches. A top level group's page gets a subgroups section: a list, an Add Subgroup button,
and for each subgroup a Manage Members dialog with checkboxes of the parent's members and a Delete confirmation. It is 441 added
lines, and it adds no JavaScript, no routes and no controllers, because the actions already existed from the previous pull requests.

The two alternatives were a Stimulus controller with one shared dialog and a separate page for managing members. The shared dialog is
less markup and needs script to rewrite the form's target for each subgroup, which is more code to test. A separate page avoids dialogs
and adds a route, an action and a view for a short checkbox list. Rendering one dialog per subgroup, with ids like
manage-subgroup-<id>, uses Bootstrap's data-bs-toggle and data-bs-target and the styles the app already has.

The section is gated by manage_subgroups? in the view as well as the server, so a student never sees a control the server would
refuse. That is the habit I would defend: a button the server rejects is a UI bug, and the check in the view and the check in the
controller come from one policy method so they cannot disagree.

AccessLint found the one real defect in the markup: a <ul> whose direct children were <div>s, which breaks the list semantics a
screen reader relies on.

The user who left the class and kept the team

The security finding of the week came on that pull request and again on the progress one:

This filter checks IDs submitted to update_members, but it does not handle an existing subgroup member removed from the parent
group. That user retains the subgroup membership and can still pass GroupPolicy#show_access? for the subgroup.

A subgroup member is a GroupMember row on the subgroup. Removing someone from the parent group deletes the parent's row and nothing
else. The model has no callback that follows:

class GroupMember < ApplicationRecord
  belongs_to :group, counter_cache: true
  belongs_to :user
  after_commit :send_welcome_email, on: :create, unless: -> { group.subgroup? }
end

So a student dropped from the class, whether by a mentor or by the roster sync from last week, is still a member of a team inside it and
can still open its page and its shared project. I checked that against the branch rather than trusting the comment, and it is true. It
is not in this pull request and I am not going to pretend it is. The fix is its own change, one that removes the user's subgroup
memberships when the parent membership goes, and it has to cover the API destroy path too. Two ways to remove a member, and only
the one I thought of while writing the code was safe.

A missing parameter that deletes a roster

Subgroups got a JSON API on the 19th (#8056): list and create under a group, and show, delete and update members by the subgroup's
own id. Reads go through the group's existing show_access?. Writes authorize the parent with manage_subgroups?, and the members
endpoint calls the same Group#sync_subgroup_members as the web controller, so the two cannot drift. Index and create are nested
because they only make sense with a parent. Show, destroy and members use the subgroup id alone so a client that already holds one does
not have to know the parent.

The review found what happens when the request is sloppy:

params[:user_ids] is passed unfiltered to sync_subgroup_members. A request without the key removes every non-mentor member of the
subgroup. A request with a hash or nested value raises on to_i, because Hash#to_i does not exist and the request returns 500. Use
params.expect(user_ids: []) so the API returns 400 for a missing or malformed key.

"Set the members to this list" with a missing list is not a no-op. It is "set the members to nothing." An API call that forgets a field
had the same effect as one that meant to empty the team. The web controller does the same thing in one line,
Array(params[:user_ids]).map(&:to_i), so the bug is in both. Making the parameter required changes a silent destructive default into
a 400, which is the right trade for an endpoint whose whole job is replacing a list.

A smaller one on the web side, from the shared team project pull request: manage_subgroups? lets a parent group's co-mentor act, but
show_access? does not let them view a subgroup they are not a member of. A co-mentor could update the members and then be redirected
to a page they are not allowed to see. The redirect should stay on the parent page. Two policy methods that answer slightly different
questions will always eventually be used in the same sentence.

Progress and the team project

The progress rollup (#8058) puts a line next to each subgroup, such as "2 assignments, 5 submitted, 3 graded", so an instructor can see
how teams are doing without opening each one. It is a model method with three counts:

def progress
  submitted = Project.where(assignment_id: assignments.select(:id), project_submission: true)
  { assignments: assignments.count, submissions: submitted.count,
    graded: Grade.where(project_id: submitted.select(:id)).count }
end

I did not add cached counters. They can drift when a project is un-submitted or a grade is deleted, and they need callbacks and a
migration for numbers read on exactly one page. A bug on that page was a fair one: "1 assignments". The fix on the 20th is a plural
form in the locale file, not a conditional in the view.

The team project (#8064) lets a subgroup designate one project and have everyone in the subgroup collaborate on it. Rather than a new
table or a group level collaborator type, it reuses Collaboration:

belongs_to :team_project, class_name: "Project", optional: true
validate :team_project_author_must_be_member, if: :team_project_id?

def share_team_project
  group_members.where.not(user_id: team_project.author_id).pluck(:user_id).each do |user_id|
    Collaboration.find_or_create_by!(project_id: team_project_id, user_id: user_id)
  end
end

Reusing Collaboration means every existing project access rule applies unchanged, and the alternative, teaching the project policy about
groups, would have touched rules that matter far more than this feature. The validation says the project's author must be a member
of the subgroup. Without it, a mentor could attach someone else's private circuit to a team and share it with everyone in it, so the
rule is an access check disguised as data validation. find_or_create_by! makes sharing idempotent, so running it twice, as happens
when members are updated, does not duplicate collaborations. Deleting the project clears the link rather than failing or deleting the
subgroup, because the foreign key is on_delete: :nullify.

Templates: three places a circuit can be handed to someone

On the 20th the template stack went up, four small pull requests in a chain: a flag and a visibility enum on Project, a
save-as-template action, a gallery, and a link from an assignment to a template. The fifth, on the 23rd, is the one where a student
actually receives a copy. Every one of them is a different answer to "who may receive this circuit".

The rules start small. An assignment project cannot be a template, because it is a student's working copy and publishing it would
expose submitted work. Saving as a template is allowed for the author, an admin, and a mentor of a group the author belongs to, and
refused for collaborators, because a collaborator can edit a circuit without owning it. is_template and template_visibility are
separate from project_access_type: access decides who can view a project and visibility decides who can find and copy it, and a
project can be public without being offered.

That separation is exactly what the first review attacked, on the gallery:

If a private project is saved with template_visibility: "public", the gallery selects it, but check_view_access denies its detail
page to anonymous visitors. The save action permits this combination. Apply one access rule across template eligibility, the
gallery, and the detail page.

Two independent switches create a combination nobody designed. The template could be listed, with a card, and refuse to open. The
direction of the fix is a single rule that every path uses, rather than a patch for each page.

The copy that has to ask again

#8070 lets an instructor pick a base template for an assignment. It stores a reference, not a copy, and a controller check limits
the choice to templates the instructor could use, with the same Project.templates_for scope that fills the dropdown, so what the
form offers and what the server accepts are always the same list. CodeQL flagged the first version of the check as a SQL query built
from user input, which was fair, and the check became exists?(id: template_id), which lets ActiveRecord bind the value.

#8084 makes the link do something: when a student opens an assignment, their project starts from the template's circuit. It copies
only the circuit data rather than calling Project#fork, because a fork also notifies the template's author and attaches a preview
image, and a student opening an assignment is not a fork and a template with no preview would raise. It copies when the student
opens it, not when the instructor saves, so each student gets the template as it stands when they start and the instructor can still
fix it before anyone begins.

That choice created the review's best finding:

template_visibility: private alone does not make the underlying project private. The privacy issue is reachable when the author
changes the linked template's project_access_type to "Private". The assignment keeps its existing template link, and both
AssignmentsController#start and LtiController#create_project_if_student_present copy the linked template's current project data
without checking its current access.

Because the assignment stores a reference and the copy happens later, access has to be decided at the later time, and I had only
decided it at the earlier one. The instructor chose a legitimate template on Monday, its author made the project private on Tuesday,
and on Wednesday a student received its data. The copy now asks:

def template_circuit_data
  return unless circuit_template
  return if circuit_template.project_access_type == "Private" && !template_author_mentors_group?

  circuit_template.project_datum&.data
end

A private template is still copied when its author is a mentor of the assignment's group, since in that case the student is receiving
it from their own teacher, and refused otherwise. Both the web start action and the LTI launch call the same method, so there is one
place that answers.

A template that carries its own tests

The last pull request of the week, on the 25th, lets a template carry a test suite and hands it to the assignments that use it. It adds
a template_testbench column to projects and validates it by building a Testbench around it, so a suite on a template has to pass
the same shape, label and pin checks as any other suite instead of getting a second, weaker validator:

validate :template_testbench_is_runnable, if: :template_testbench?

def template_testbench_is_runnable
  suite = Testbench.new(assignment: Assignment.new, data: template_testbench)
  suite.validate
  errors.add(:template_testbench, suite.errors[:data].to_sentence) if suite.errors.key?(:data)
end

When an assignment's template changes, the suite is copied into a testbench, but only if the assignment does not already have one:

after_save :copy_template_testbench, if: :saved_change_to_circuit_template_id?

def copy_template_testbench
  suite = circuit_template&.template_testbench
  create_testbench!(data: suite) if suite.present? && testbench.nil?
end

An instructor who has already written their own tests keeps them. Nothing in the interface sets template_testbench yet, so this is
the plumbing for a later change, and I would rather say that than imply a feature.

Where the project stands

Subgroups can be created, filled, shown, progress-tracked, exposed through an API and given a shared project. One of the largest gaps,
a user removed from the parent keeping their team, is real and open, and so is the destructive empty parameter on the web path. Templates
can be saved, browsed, linked to an assignment and copied to a student, with access checked at the copy, and they can carry a
test suite that follows the link.

The lesson I would write on the wall is that a reference is a promise about later, and every promise about later needs a check at later.

Next week: template visibility stops being a label on a column and becomes a rule, and a template gets a fork button.