From 0784d40275e1babaa4fc1c1008bd01a515ec968a Mon Sep 17 00:00:00 2001 From: Michael Ball Date: Fri, 8 May 2026 13:56:57 +0000 Subject: [PATCH 1/4] fix(security): derive role server-side in AssignmentsController#toggle_enabled The role check trusted a client-supplied params[:role], which the Stimulus controller passed through from a data-* attribute. Any authenticated user could PATCH /assignments/:id/toggle_enabled with role=instructor and toggle assignments in courses where they were a student or had no enrollment at all. Now the role is looked up exclusively via course.user_role(current_user), the JS no longer sends role/user_id, and the view drops the data-role / data-user-id attributes that fed the bypass. Adds spec coverage for students, foreign-course instructors, and the no-enrollment case all attempting role=instructor. Co-Authored-By: Claude Opus 4.7 --- app/controllers/assignments_controller.rb | 6 +- .../controllers/assignment_controller.js | 4 - app/views/courses/instructor_show.html.erb | 4 +- .../assignments_controller_spec.rb | 122 +++++++++++------- 4 files changed, 81 insertions(+), 55 deletions(-) diff --git a/app/controllers/assignments_controller.rb b/app/controllers/assignments_controller.rb index 1655b05d..290e7421 100644 --- a/app/controllers/assignments_controller.rb +++ b/app/controllers/assignments_controller.rb @@ -1,11 +1,13 @@ class AssignmentsController < ApplicationController + before_action :authenticate_user + def toggle_enabled @assignment = Assignment.find(params[:id]) course = @assignment.course_to_lms.course - @role = params[:role] || course&.user_role(@user) + @role = course&.user_role(@user) unless @role == 'instructor' - Rails.logger.error "Role #{@role} does not have permission to toggle assignment enabled status" + Rails.logger.warn "User #{@user&.id} (role=#{@role.inspect}) attempted to toggle assignment #{@assignment.id} in course #{course&.id}" flash.now[:alert] = 'You do not have permission to perform this action.' return render json: { redirect_to: course_path(course) }, status: :forbidden end diff --git a/app/javascript/controllers/assignment_controller.js b/app/javascript/controllers/assignment_controller.js index 8112cfb5..11bbb097 100644 --- a/app/javascript/controllers/assignment_controller.js +++ b/app/javascript/controllers/assignment_controller.js @@ -28,8 +28,6 @@ export default class extends Controller { const assignmentId = checkbox.dataset.assignmentId; const url = checkbox.dataset.url; const enabled = checkbox.checked; - const role = checkbox.dataset.role; // Get the role - const userId = checkbox.dataset.userId; // Get the user ID try { const token = document.querySelector('meta[name="csrf-token"]').content; @@ -42,8 +40,6 @@ export default class extends Controller { }, body: JSON.stringify({ enabled: enabled, - role: role, // Pass the role - user_id: userId, // Pass the user ID }), }); diff --git a/app/views/courses/instructor_show.html.erb b/app/views/courses/instructor_show.html.erb index 3f0ed8ef..2286e2e6 100644 --- a/app/views/courses/instructor_show.html.erb +++ b/app/views/courses/instructor_show.html.erb @@ -36,9 +36,7 @@ <%= 'checked' if assignment.enabled %> data-assignment-target="checkbox" data-assignment-id="<%= assignment.id %>" - data-url="<%= toggle_enabled_assignment_path(assignment) %>" - data-role="<%= @role %>" - data-user-id="<%= @user.id %>"> + data-url="<%= toggle_enabled_assignment_path(assignment) %>"> diff --git a/spec/controllers/assignments_controller_spec.rb b/spec/controllers/assignments_controller_spec.rb index 37545ca0..bae53b38 100644 --- a/spec/controllers/assignments_controller_spec.rb +++ b/spec/controllers/assignments_controller_spec.rb @@ -2,32 +2,34 @@ require 'rails_helper' RSpec.describe AssignmentsController, type: :controller do + let!(:user) { User.create!(name: 'Test User', email: 'test@example.com', canvas_uid: '123') } + let!(:course) { Course.create!(course_name: 'Test Course', canvas_id: '123') } + let!(:course_to_lms) { CourseToLms.create!(course: course, lms_id: 1, external_course_id: '123') } + let!(:course_settings) { CourseSettings.create!(course: course, enable_extensions: true) } + let!(:assignment) do + Assignment.create!( + name: 'Test Assignment', + course_to_lms: course_to_lms, + due_date: 3.days.from_now, + external_assignment_id: 'abc123', + enabled: false + ) + end + before do - session[:user_id] = '123' + session[:user_id] = user.canvas_uid + user.lms_credentials.create!( + lms_name: 'canvas', token: 't', refresh_token: 'r', + expire_time: 1.hour.from_now + ) end describe 'POST #toggle_enabled' do - let!(:user) { User.create!(name: 'Test User', email: 'test@example.com') } - let!(:course) { Course.create!(course_name: 'Test Course', canvas_id: '123') } - let!(:course_to_lms) { CourseToLms.create!(course: course, lms_id: 1, external_course_id: '123') } - let!(:course_settings) { CourseSettings.create!(course: course, enable_extensions: true) } - let!(:assignment) do - Assignment.create!( - name: 'Test Assignment', - course_to_lms: course_to_lms, - due_date: 3.days.from_now, - external_assignment_id: 'abc123', - enabled: false - ) - end - - context 'when the user is an instructor' do - before do - allow(course).to receive(:user_role).with(user).and_return('instructor') - end + context 'when the user is an instructor in the course' do + before { UserToCourse.create!(user: user, course: course, role: 'teacher') } it 'updates the enabled status to true' do - post :toggle_enabled, params: { id: assignment.id, enabled: true, role: 'instructor', user_id: user.id } + post :toggle_enabled, params: { id: assignment.id, enabled: true } expect(response).to have_http_status(:ok) expect(assignment.reload.enabled).to be true @@ -36,63 +38,91 @@ it 'updates the enabled status to false' do assignment.update!(enabled: true) - post :toggle_enabled, params: { id: assignment.id, enabled: false, role: 'instructor', user_id: user.id } + post :toggle_enabled, params: { id: assignment.id, enabled: false } expect(response).to have_http_status(:ok) expect(assignment.reload.enabled).to be false end end - context 'when the user is not an instructor' do - before do - allow(course).to receive(:user_role).with(user).and_return('student') - end - - it 'returns a forbidden status' do - post :toggle_enabled, params: { id: assignment.id, enabled: true, role: 'student', user_id: user.id } - - expect(response).to have_http_status(:forbidden) - expect(assignment.reload.enabled).to be false - end - end - context 'when course-level extensions are disabled' do before do + UserToCourse.create!(user: user, course: course, role: 'teacher') course_settings.update!(enable_extensions: false) - allow(course).to receive(:user_role).with(user).and_return('instructor') end it 'still allows enabling the assignment and returns ok status' do - post :toggle_enabled, params: { id: assignment.id, enabled: true, role: 'instructor', user_id: user.id } + post :toggle_enabled, params: { id: assignment.id, enabled: true } expect(response).to have_http_status(:ok) expect(assignment.reload.enabled).to be true end end - context 'when there is no due_date on an Assignment' do + context 'when the assignment has no due_date' do before do + UserToCourse.create!(user: user, course: course, role: 'teacher') assignment.update!(due_date: nil) end - it 'returns a bad request status' do - post :toggle_enabled, params: { id: assignment.id, enabled: true, role: 'instructor', user_id: user.id } + it 'returns an unprocessable status' do + post :toggle_enabled, params: { id: assignment.id, enabled: true } expect(response).to have_http_status(:unprocessable_content) expect(flash[:alert]).to include('Due date must be present if assignment is enabled') end end - context 'when user_id is not provided' do - before do - allow(course).to receive(:user_role).and_return('instructor') + # Authorization regression coverage: a client-supplied role parameter + # must not bypass server-side role lookup. See security audit C1. + describe 'authorization' do + context 'when the user is a student in the course' do + before { UserToCourse.create!(user: user, course: course, role: 'student') } + + it 'rejects the toggle' do + post :toggle_enabled, params: { id: assignment.id, enabled: true } + + expect(response).to have_http_status(:forbidden) + expect(assignment.reload.enabled).to be false + end + + it 'rejects the toggle even when the request claims role=instructor' do + post :toggle_enabled, params: { id: assignment.id, enabled: true, role: 'instructor' } + + expect(response).to have_http_status(:forbidden) + expect(assignment.reload.enabled).to be false + end end - it 'uses the session user and updates the assignment' do - post :toggle_enabled, params: { id: assignment.id, enabled: true, role: 'instructor' } + context "when the user is an instructor of a different course" do + let!(:other_course) { Course.create!(course_name: 'Other Course', canvas_id: '999') } - expect(response).to have_http_status(:ok) - expect(assignment.reload.enabled).to be true + before do + UserToCourse.create!(user: user, course: other_course, role: 'teacher') + end + + it 'rejects the toggle on the foreign assignment' do + post :toggle_enabled, params: { id: assignment.id, enabled: true } + + expect(response).to have_http_status(:forbidden) + expect(assignment.reload.enabled).to be false + end + + it 'rejects the toggle even when the request claims role=instructor' do + post :toggle_enabled, params: { id: assignment.id, enabled: true, role: 'instructor' } + + expect(response).to have_http_status(:forbidden) + expect(assignment.reload.enabled).to be false + end + end + + context 'when the user has no enrollment in the course' do + it 'rejects the toggle even when the request claims role=instructor' do + post :toggle_enabled, params: { id: assignment.id, enabled: true, role: 'instructor' } + + expect(response).to have_http_status(:forbidden) + expect(assignment.reload.enabled).to be false + end end end end From 21cd76cf504cc9cdb67fbc08ccb891ec2ae51696 Mon Sep 17 00:00:00 2001 From: Michael Ball Date: Fri, 8 May 2026 13:57:17 +0000 Subject: [PATCH 2/4] chore(security): replace example Gradescope credentials with placeholders The previous .env.example shipped a plausible-looking Gradescope password (KoH-z92-oVJ-yqr) and a real-looking service-account email. Even though those values were not the live credential, anything that looks like a secret in a sample file invites confusion (and gets indexed by secret scanners as a leak). Replace both with obvious placeholders and add a warning not to commit live values. Co-Authored-By: Claude Opus 4.7 --- .env.example | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.env.example b/.env.example index 0fe7a7ee..0f413904 100644 --- a/.env.example +++ b/.env.example @@ -21,8 +21,9 @@ CANVAS_URL='https://ucberkeleysandbox.instructure.com' # We use a single username/password for Gradescope # This should be a "service account" that can be used to set course settings # This email must be invited to each Gradescope course as a TA or Instructor -GRADESCOPE_EMAIL='michael@gradescope.com' -GRADESCOPE_PASSWORD=KoH-z92-oVJ-yqr +# NEVER commit a real credential here. Generate the real values out-of-band. +GRADESCOPE_EMAIL='example-service-account@example.com' +GRADESCOPE_PASSWORD='REPLACE_ME_WITH_REAL_GRADESCOPE_PASSWORD' # This is required to be set. DEFAULT_FROM_EMAIL='flextensions@berkeley.edu' # Generally recommended / best practices From 584888dbeb85c62ef2b529bf5a40a72a809c4d90 Mon Sep 17 00:00:00 2001 From: Michael Ball Date: Fri, 8 May 2026 14:00:37 +0000 Subject: [PATCH 3/4] fix(security): split developer/canvas account-linking and gate developer login The find_or_create_user logic always matched first by email and silently re-keyed the existing user's canvas_uid. That is intentional for the developer omniauth strategy (whose whole point is "log in as an existing user without Canvas"), but for the real Canvas provider it would let any auth response carrying a victim's email rewrite that user's identity. Split the two paths: - Canvas (and any non-developer provider) now matches strictly by canvas_uid, updates email/name on that user, and refuses to merge two identities when the email is already taken by a different account. - Developer keeps the email-first masquerade behavior, but only runs when Rails.env is development or test. The omniauth_callback action now hard-fails developer-provider responses in any other environment, backing up the existing initializer-level guard. Adds spec coverage for: canvas-provider email collision (refused), canvas-provider canvas_uid match (still updates email), developer masquerade (still works), and developer-provider rejection in production. Co-Authored-By: Claude Opus 4.7 --- app/controllers/session_controller.rb | 106 +++++++++++++++++--- config/initializers/omniauth.rb | 5 + spec/controllers/session_controller_spec.rb | 87 +++++++++++----- 3 files changed, 159 insertions(+), 39 deletions(-) diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index eeac56a9..68dfe802 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -46,6 +46,15 @@ def omniauth_callback return end + # Defense in depth: the developer provider is also gated in + # config/initializers/omniauth.rb, but block it here too so a misconfigured + # production environment cannot accept a developer-strategy callback. + if auth.provider.to_s == 'developer' && !developer_login_allowed? + Rails.logger.error("Refusing developer-provider login in #{Rails.env}") + redirect_to root_path, alert: 'Authentication failed.' + return + end + user_data = { 'id' => auth.uid, 'name' => auth.info.name, @@ -66,7 +75,12 @@ def omniauth_callback ) # Persist / update the user just like `create` - user = find_or_create_user(user_data, access_token) + user = find_or_create_user(user_data, access_token, provider: auth.provider.to_s) + + if user.nil? + redirect_to root_path, alert: 'Authentication failed. This account is already linked to another login.' + return + end # Auto-enroll developer login users in test courses if auth.provider == 'developer' @@ -89,6 +103,16 @@ def destroy private + # The developer omniauth strategy is intended ONLY for local development / + # tests, where you may want to log in "as" an existing user (matched by + # email) without going through Canvas. It must never be reachable in + # production. The strategy itself is only registered when + # Rails.env.development? || Rails.env.test? (see config/initializers/omniauth.rb); + # this method is the callback-side guard that backs that up. + def developer_login_allowed? + Rails.env.development? || Rails.env.test? + end + def ensure_developer_test_enrollments(user) # Find the test course test_course = Course.find_by(course_code: 'DEV101') @@ -101,23 +125,28 @@ def ensure_developer_test_enrollments(user) end end - # TODO: Refactor. - def find_or_create_user(user_data, auth_token) - auth_token.token - user = nil - if User.exists?(email: user_data['primary_email']) - user = User.find_by(email: user_data['primary_email']) - user.canvas_uid = user_data['id'] - elsif User.exists?(canvas_uid: user_data['id']) - user = User.find_by(canvas_uid: user_data['id']) - user.email = user_data['email'] + # Find or create the user for the given omniauth identity. + # + # For real LMS providers (Canvas) the canvas_uid is the canonical identity. + # If we find an existing User with that canvas_uid we update their email; + # otherwise we create a new User. We refuse to silently re-key an existing + # account by email match: that pathway would let anyone who can present an + # auth response carrying a victim's email take over the account. + # + # The developer provider is the deliberate exception: its whole purpose is + # to let a local developer log in as an existing user by email without + # round-tripping through Canvas, so when the developer provider is in use + # we keep the legacy "match by email and overwrite canvas_uid" behavior. + # That branch is only reachable in dev/test (gated above and in the + # initializer). + def find_or_create_user(user_data, auth_token, provider: 'canvas') + if provider == 'developer' + user = developer_lookup_or_create(user_data) else - user = User.find_or_initialize_by(canvas_uid: user_data['id']) - user.assign_attributes( - email: user_data['email'], - name: user_data['name'] - ) + user = canvas_lookup_or_create(user_data) + return nil if user.nil? end + user.save! update_user_credential(user, auth_token) @@ -128,6 +157,51 @@ def find_or_create_user(user_data, auth_token) user end + # Production / Canvas path: never re-key an existing user by email. + # Returns nil if the email is already taken by a different canvas_uid so the + # caller can surface a "linked to another login" error instead of merging + # two distinct identities. + def canvas_lookup_or_create(user_data) + by_uid = User.find_by(canvas_uid: user_data['id']) + if by_uid + by_uid.assign_attributes(email: user_data['email'], name: user_data['name']) + return by_uid + end + + if User.exists?(email: user_data['primary_email']) + Rails.logger.warn( + "Refusing to link canvas_uid=#{user_data['id']} to existing email " \ + "#{user_data['primary_email']}: a different account already owns it" + ) + return nil + end + + User.new( + canvas_uid: user_data['id'], + email: user_data['email'], + name: user_data['name'] + ) + end + + # Developer-only path: matches by email first so a developer can log in + # "as" an existing user without Canvas. Intentionally rewrites canvas_uid + # on email match — see find_or_create_user docstring. + def developer_lookup_or_create(user_data) + if User.exists?(email: user_data['primary_email']) + user = User.find_by(email: user_data['primary_email']) + user.canvas_uid = user_data['id'] + user + elsif User.exists?(canvas_uid: user_data['id']) + user = User.find_by(canvas_uid: user_data['id']) + user.email = user_data['email'] + user + else + User.find_or_initialize_by(canvas_uid: user_data['id']).tap do |u| + u.assign_attributes(email: user_data['email'], name: user_data['name']) + end + end + end + # TODO: Move this to a Canvas API libarary or user service # TODO: Find credentals for the right LMS, not just the first one. def update_user_credential(user, token) diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb index bfffa61e..3117a4dc 100644 --- a/config/initializers/omniauth.rb +++ b/config/initializers/omniauth.rb @@ -18,6 +18,11 @@ def build_access_token # URL-encode the scopes defined in CanvasFacade encoded_scopes = CGI.escape(CanvasFacade::CANVAS_API_SCOPES) + # The `developer` strategy lets a local developer sign in as any existing + # user by entering their email (no Canvas round-trip needed). It is + # deliberately registered ONLY in development and test, and the session + # callback (`SessionController#omniauth_callback`) double-checks the env + # before honoring a developer-provider response. provider :developer, fields: [:email] if Rails.env.development? || Rails.env.test? provider :canvas, diff --git a/spec/controllers/session_controller_spec.rb b/spec/controllers/session_controller_spec.rb index 6acae4d2..dc6b5b40 100644 --- a/spec/controllers/session_controller_spec.rb +++ b/spec/controllers/session_controller_spec.rb @@ -75,42 +75,83 @@ expires_at: 2.hours.from_now.to_i) end - context 'when user exists by email' do - let!(:existing_user) { User.create!(email: 'test@example.com', canvas_uid: 'old_uid') } + context 'with the canvas provider' do + context 'when user exists by canvas_uid' do + let!(:existing_user) { User.create!(email: 'old@example.com', canvas_uid: '12345') } + + it 'updates email and LMS credentials' do + expect do + controller.send(:find_or_create_user, user_data, mock_token, provider: 'canvas') + end.to change { existing_user.reload.email }.from('old@example.com').to('test@example.com') + end + end + + context 'when an existing user has the email but a different canvas_uid' do + let!(:existing_user) { User.create!(email: 'test@example.com', canvas_uid: 'old_uid') } - it 'updates canvas_uid and LMS credentials' do - expect do - controller.send(:find_or_create_user, user_data, mock_token) - end.to change { existing_user.reload.canvas_uid }.from('old_uid').to('12345') + it 'refuses to re-key the existing account and returns nil' do + result = controller.send(:find_or_create_user, user_data, mock_token, provider: 'canvas') - creds = existing_user.reload.lms_credentials.first - expect(creds.token).to eq('new-token') + expect(result).to be_nil + expect(existing_user.reload.canvas_uid).to eq('old_uid') + expect(existing_user.lms_credentials).to be_empty + end end - end - context 'when user exists by canvas_uid' do - let!(:existing_user) { User.create!(email: 'old@example.com', canvas_uid: '12345') } + context 'when user is new' do + it 'creates the user and LMS credentials' do + expect do + controller.send(:find_or_create_user, user_data, mock_token, provider: 'canvas') + end.to change(User, :count).by(1) - it 'updates email and LMS credentials' do - expect do - controller.send(:find_or_create_user, user_data, mock_token) - end.to change { existing_user.reload.email }.from('old@example.com').to('test@example.com') + user = User.find_by(canvas_uid: '12345') + expect(user.email).to eq('test@example.com') + expect(user.lms_credentials.first.token).to eq('new-token') + end end end - context 'when user is new' do - it 'creates the user and LMS credentials' do - expect do - controller.send(:find_or_create_user, user_data, mock_token) - end.to change(User, :count).by(1) + context 'with the developer provider (dev/test only)' do + context 'when user exists by email' do + let!(:existing_user) { User.create!(email: 'test@example.com', canvas_uid: 'old_uid') } - user = User.find_by(canvas_uid: '12345') - expect(user.email).to eq('test@example.com') - expect(user.lms_credentials.first.token).to eq('new-token') + it 'updates canvas_uid and LMS credentials (intentional masquerade)' do + expect do + controller.send(:find_or_create_user, user_data, mock_token, provider: 'developer') + end.to change { existing_user.reload.canvas_uid }.from('old_uid').to('12345') + + creds = existing_user.reload.lms_credentials.first + expect(creds.token).to eq('new-token') + end end end end + describe 'GET #omniauth_callback (developer provider in production)' do + let(:dev_auth_hash) do + OmniAuth::AuthHash.new( + provider: 'developer', + uid: 'attacker@example.com', + info: OpenStruct.new(name: 'Attacker', email: 'attacker@example.com'), + credentials: { token: 'dev-token', refresh_token: nil, expires_at: nil } + ) + end + + before { request.env['omniauth.auth'] = dev_auth_hash } + + it 'is refused when Rails.env is production' do + allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new('production')) + + expect do + get :omniauth_callback, params: { provider: 'developer' } + end.not_to change(User, :count) + + expect(session[:user_id]).to be_nil + expect(response).to redirect_to(root_path) + expect(flash[:alert]).to eq('Authentication failed.') + end + end + describe 'GET #omniauth_callback (developer provider)' do let(:dev_auth_hash) do OmniAuth::AuthHash.new( From 69941ee87363da0e63074b6c3e8f5efc0803d475 Mon Sep 17 00:00:00 2001 From: Michael Ball Date: Fri, 8 May 2026 14:05:39 +0000 Subject: [PATCH 4/4] fix(security): scope request params and validate assignment-in-course request_params permitted :user_id, which on PATCH #update would let a student reassign their request to another user (the create flow merged user: @user so it was incidentally safe; update did not). The assignment_id param was also never checked against @course, so a request could be filed against an assignment from a course the submitter doesn't belong to. - Drop :user_id from request_params. Both create paths already set the owner explicitly (`user: @user` for self-service, `user: student` in the instructor `create_for_student` flow), so this only removes the mass-assignment escape hatch. - Add assignment_in_course? and reject create/update/create_for_student when the assignment_id resolves to a different course's CourseToLms. - Add student_enrolled_in_course for create_for_student so an instructor cannot file requests on behalf of someone outside their course roster. Adds spec coverage for the user_id mass-assignment attempt on both create and update, and for the cross-course assignment_id attempt on both. Also bumps a few hardcoded canvas_uids in pre-existing tests to string-prefixed values so they do not collide with the user factory's numeric sequence as more tests are added. Co-Authored-By: Claude Opus 4.7 --- app/controllers/requests_controller.rb | 58 +++++++++-- spec/controllers/requests_controller_spec.rb | 102 ++++++++++++++++++- 2 files changed, 150 insertions(+), 10 deletions(-) diff --git a/app/controllers/requests_controller.rb b/app/controllers/requests_controller.rb index 463f809d..ab5a1bcd 100644 --- a/app/controllers/requests_controller.rb +++ b/app/controllers/requests_controller.rb @@ -73,11 +73,18 @@ def edit def create Request.merge_date_and_time!(params[:request]) - @request = @course.requests.new(request_params.merge(user: @user)) + permitted = request_params + + if permitted[:assignment_id].present? && !assignment_in_course?(permitted[:assignment_id]) + redirect_to course_requests_path(@course), alert: 'Invalid assignment for this course.' + return + end + + @request = @course.requests.new(permitted.merge(user: @user)) # Check if the assignment already has a pending request, but only if assignment_id exists - if request_params[:assignment_id].present? && - Assignment.find_by(id: request_params[:assignment_id])&.has_pending_request_for_user?(@user, @course) + if permitted[:assignment_id].present? && + Assignment.find_by(id: permitted[:assignment_id])&.has_pending_request_for_user?(@user, @course) redirect_to course_requests_path(@course), alert: 'You already have a pending request for this assignment.' return end @@ -93,10 +100,15 @@ def create def create_for_student return redirect_to course_requests_path(@course), alert: 'You do not have permission to perform this action.' unless @role == 'instructor' - student = User.find_by(id: params[:request][:user_id]) + student = student_enrolled_in_course(params[:request][:user_id]) return redirect_to new_course_request_path(@course), alert: 'Student not found.' unless student assignment_id = params[:request][:assignment_id] + if assignment_id.present? && !assignment_in_course?(assignment_id) + redirect_to new_course_request_path(@course), alert: 'Invalid assignment for this course.' + return + end + reject_other_student_requests(student, assignment_id) if assignment_id.present? Request.merge_date_and_time!(params[:request]) @@ -116,7 +128,13 @@ def update Request.merge_date_and_time!(params[:request]) - if @request.update(request_params) + permitted = request_params + if permitted[:assignment_id].present? && !assignment_in_course?(permitted[:assignment_id]) + redirect_to course_requests_path(@course), alert: 'Invalid assignment for this course.' + return + end + + if @request.update(permitted) result = @request.process_update(@user) redirect_to result[:redirect_to], notice: result[:notice] else @@ -221,8 +239,36 @@ def set_course_role_from_settings @form_settings = result[:form_settings] end + # Note: :user_id is intentionally NOT permitted here. The owning user is + # set explicitly by the caller (`user: @user` for the student flow, + # `user: student` for the instructor `create_for_student` flow). Allowing + # :user_id through mass-assignment would let a student submit or + # reassign a request as another user. def request_params - params.require(:request).permit(:assignment_id, :reason, :documentation, :custom_q1, :custom_q2, :requested_due_date, :user_id) + params.require(:request).permit(:assignment_id, :reason, :documentation, :custom_q1, :custom_q2, :requested_due_date) + end + + # Returns true if the given assignment id resolves to an assignment that + # belongs to one of @course's CourseToLms records. Without this, a student + # could attach a request to an assignment from a course they don't belong + # to (the request would still be saved against @course, but the + # assignment_id would point elsewhere). + def assignment_in_course?(assignment_id) + Assignment.joins(:course_to_lms) + .where(course_to_lms: { course_id: @course.id }) + .exists?(id: assignment_id) + end + + # Looks up a student by id and confirms they are actually enrolled in + # @course as a student. Used by the instructor `create_for_student` flow + # so an instructor cannot file a request on behalf of someone outside + # their roster. + def student_enrolled_in_course(user_id) + return nil if user_id.blank? + + User.joins(:user_to_courses) + .where(user_to_courses: { course_id: @course.id, role: 'student' }) + .find_by(id: user_id) end def authenticate_user diff --git a/spec/controllers/requests_controller_spec.rb b/spec/controllers/requests_controller_spec.rb index 723cac10..f765590e 100644 --- a/spec/controllers/requests_controller_spec.rb +++ b/spec/controllers/requests_controller_spec.rb @@ -1,8 +1,8 @@ require 'rails_helper' RSpec.describe RequestsController, type: :controller do - let(:user) { User.create!(email: 'student@example.com', canvas_uid: '123', name: 'Student') } - let(:instructor) { User.create!(email: 'instructor@example.com', canvas_uid: '566', name: 'Instructor') } + let(:user) { User.create!(email: 'student@example.com', canvas_uid: 'fixed-student', name: 'Student') } + let(:instructor) { User.create!(email: 'instructor@example.com', canvas_uid: 'fixed-instructor', name: 'Instructor') } let(:course) { create(:course, :with_staff, course_name: 'Test Course', canvas_id: '456', course_code: 'TST101') } let(:teacher_course) { Course.create!(course_name: 'Instructor Course', canvas_id: '999', course_code: 'INST101') } let(:assignment) do @@ -222,6 +222,57 @@ expect(response).to render_template(:new) expect(flash[:alert]).to eq('There was a problem submitting your request.') end + + # Regression coverage for security audit H2. + describe 'authorization hardening' do + let(:other_course) { create(:course, course_name: 'Other Course', canvas_id: '777', course_code: 'OTHER101') } + let(:other_course_to_lms) { CourseToLms.find_by(course: other_course, lms_id: 1) || CourseToLms.create!(course: other_course, lms_id: 1) } + let(:foreign_assignment) do + Assignment.create!( + name: 'Foreign A1', + course_to_lms_id: other_course_to_lms.id, + due_date: 2.days.from_now, + external_assignment_id: 'foreign-1', + enabled: true + ) + end + let(:other_student) { User.create!(email: 'other_student@example.com', canvas_uid: 'fixed-other-student', name: 'Other Student') } + + it 'rejects creating a request for an assignment in a different course' do + expect do + post :create, params: { + course_id: course.id, + request: { + assignment_id: foreign_assignment.id, + reason: 'Sick', + requested_due_date: Date.tomorrow.to_s, + due_time: '10:00' + } + } + end.not_to change(Request, :count) + + expect(response).to redirect_to(course_requests_path(course)) + expect(flash[:alert]).to eq('Invalid assignment for this course.') + end + + it 'ignores a client-supplied user_id and files the request under the session user' do + UserToCourse.create!(user: other_student, course: course, role: 'student') + + post :create, params: { + course_id: course.id, + request: { + assignment_id: assignment.id, + user_id: other_student.id, + reason: 'Sick', + requested_due_date: Date.tomorrow.to_s, + due_time: '10:00' + } + } + + expect(response).to redirect_to(course_request_path(course, Request.last)) + expect(Request.last.user_id).to eq(user.id) + end + end end describe 'GET #edit' do @@ -275,6 +326,49 @@ expect(response).to redirect_to(course_request_path(course, request)) expect(flash[:notice]).to match(/updated/) end + + # Regression coverage for security audit H2. + it 'ignores a client-supplied user_id and does not reassign the request' do + other_student = User.create!(email: 'other_student@example.com', canvas_uid: 'fixed-other-student', name: 'Other Student') + + patch :update, params: { + course_id: course.id, + id: request.id, + request: { + user_id: other_student.id, + reason: 'Updated reason', + requested_due_date: Date.tomorrow.to_s, + due_time: '12:00' + } + } + + expect(request.reload.user_id).to eq(user.id) + end + + # Regression coverage for security audit H2. + it 'rejects updating to an assignment in a different course' do + other_course = create(:course, course_name: 'Other Course', canvas_id: '777', course_code: 'OTHER101') + other_ctl = CourseToLms.find_by(course: other_course, lms_id: 1) || CourseToLms.create!(course: other_course, lms_id: 1) + foreign_assignment = Assignment.create!( + name: 'Foreign A1', course_to_lms_id: other_ctl.id, + due_date: 2.days.from_now, external_assignment_id: 'foreign-1', enabled: true + ) + + patch :update, params: { + course_id: course.id, + id: request.id, + request: { + assignment_id: foreign_assignment.id, + reason: 'Updated reason', + requested_due_date: Date.tomorrow.to_s, + due_time: '12:00' + } + } + + expect(response).to redirect_to(course_requests_path(course)) + expect(flash[:alert]).to eq('Invalid assignment for this course.') + expect(request.reload.assignment_id).to eq(assignment.id) + end end describe 'POST #cancel' do @@ -428,7 +522,7 @@ end let!(:pending_request_two) do Request.create!( - user: User.create!(email: 'student2@example.com', canvas_uid: '124', name: 'Student Two'), + user: User.create!(email: 'student2@example.com', canvas_uid: 'fixed-student-two', name: 'Student Two'), course: course, assignment: assignment, reason: 'Still need more time', @@ -494,7 +588,7 @@ end let!(:pending_request_two) do Request.create!( - user: User.create!(email: 'student3@example.com', canvas_uid: '125', name: 'Student Three'), + user: User.create!(email: 'student3@example.com', canvas_uid: 'fixed-student-three', name: 'Student Three'), course: course, assignment: assignment, reason: 'Still need more time',