Complete security audit of Flextensioons, identifying key vulnerabilities and areas for improvement.

.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

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

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

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)

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
         }),
       });
 

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) %>">
                     <label class="visually-hidden" for="assignment-<%= assignment.id %>-enabled">
                       Enable <%= assignment.name %> assignment
                     </label>

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,