Refactor permissions logic with Pundit gem and write policies for user roles

Gemfile

@@ -54,6 +54,9 @@ gem 'omniauth'
 gem 'omniauth-canvas'
 gem 'omniauth-oauth2'
 
+# Authorization
+gem 'pundit'
+
 # Audit for potentially unsafe database migrations
 gem 'strong_migrations'
 
@@ -111,6 +114,7 @@ group :test do
   gem 'rack_session_access'
   gem 'rspec-retry'
   gem 'selenium-webdriver'
+  gem 'pundit-matchers', '~> 3.1'
   gem 'shoulda-matchers', '~> 7.0'
   gem 'simplecov', '~> 0.22.0', require: false
   gem 'simplecov_json_formatter'

Gemfile.lock

@@ -382,6 +382,13 @@ GEM
     public_suffix (7.0.5)
     puma (7.2.0)
       nio4r (~> 2.0)
+    pundit (2.5.2)
+      activesupport (>= 3.0.0)
+    pundit-matchers (3.1.2)
+      rspec-core (~> 3.12)
+      rspec-expectations (~> 3.12)
+      rspec-mocks (~> 3.12)
+      rspec-support (~> 3.12)
     racc (1.8.1)
     rack (3.2.6)
     rack-protection (4.2.1)
@@ -643,6 +650,8 @@ DEPENDENCIES
   ostruct
   pg
   puma (>= 6.0)
+  pundit
+  pundit-matchers (~> 3.1)
   rack_session_access
   rails (~> 7.2.3.1)
   rails-controller-testing (~> 1.0)

app/controllers/application_controller.rb

@@ -1,7 +1,10 @@
 class ApplicationController < ActionController::Base
+  include Pundit::Authorization
+
   before_action :authenticated!, unless: -> { excluded_controller_action? }
 
   rescue_from LmsFacade::LmsAPIError, with: :handle_lms_api_error
+  rescue_from Pundit::NotAuthorizedError, with: :user_not_authorized
 
   def excluded_controller_action?
     # Actions and controllers that do NOT require authentication
@@ -127,4 +130,13 @@ def ensure_instructor_role
     flash[:alert] = 'You do not have access to this page.'
     redirect_to courses_path
   end
+
+  def pundit_user
+    current_user
+  end
+
+  def user_not_authorized
+    flash[:alert] = 'You do not have access to this page.'
+    redirect_back_or_to(courses_path)
+  end
 end

app/controllers/assignments_controller.rb

@@ -1,18 +1,12 @@
 class AssignmentsController < ApplicationController
   def toggle_enabled
     @assignment = Assignment.find(params[:id])
-    course = @assignment.course_to_lms.course
-    @role = params[:role] || course&.user_role(@user)
-
-    unless @role == 'instructor'
-      Rails.logger.error "Role #{@role} does not have permission to toggle assignment enabled status"
-      flash.now[:alert] = 'You do not have permission to perform this action.'
-      return render json: { redirect_to: course_path(course) }, status: :forbidden
-    end
+    authorize @assignment
 
     if @assignment.update(enabled: params[:enabled])
       render json: { success: true }, status: :ok
     else
+      course = @assignment.course_to_lms.course
       flash[:alert] = "Failed to update assignment: #{@assignment.errors.full_messages.to_sentence}"
       render json: { redirect_to: course_path(course) }, status: :unprocessable_content
     end

app/controllers/course_settings_controller.rb

@@ -2,7 +2,6 @@ class CourseSettingsController < ApplicationController
   before_action :authenticated!
   before_action :authenticate_user
   before_action :set_course
-  before_action :ensure_instructor_role
   before_action :set_pending_request_count
 
   # Default template settings
@@ -21,6 +20,7 @@ class CourseSettingsController < ApplicationController
   def update
     @side_nav = 'course_settings'
     @course_settings = @course.course_settings || @course.build_course_settings
+    authorize @course_settings
 
     if params[:reset_email_template].present?
       reset_email_templates

app/controllers/courses_controller.rb

@@ -5,7 +5,8 @@ class CoursesController < ApplicationController
   before_action :determine_user_role
 
   def index
-    @teacher_courses = UserToCourse.includes(:course).where(user: @user, role: %w[teacher ta])
+    authorize Course
+    @teacher_courses = UserToCourse.includes(:course).where(user: @user, role: UserToCourse.staff_roles)
 
     # Only show courses to students if extensions are enabled at the course level
     student_courses = UserToCourse.includes(course: :course_settings).where(user: @user, role: 'student')
@@ -17,6 +18,8 @@ def index
 
   def show
     return redirect_to courses_path, alert: 'Course not found.' unless @course
+
+    authorize @course
     return redirect_to courses_path, alert: 'No Canvas LMS data found for this course.' unless @course.has_canvas_linked?
 
     @side_nav = 'show'
@@ -34,6 +37,7 @@ def show
   end
 
   def new
+    authorize Course
     token = @user.lms_credentials.first.token
     @courses = Course.fetch_courses(token)
     flash[:alert] = 'No courses found.' if @courses.empty?
@@ -56,11 +60,12 @@ def new
   end
 
   def edit
+    authorize @course
     @side_nav = 'edit'
-    redirect_to course_path(@course.id), alert: 'You do not have access to this page.' unless @role == 'instructor'
   end
 
   def create
+    authorize Course
     token = @user.lms_credentials.first.token
     filter_courses(Course.fetch_courses(token), %w[teacher ta])
       .select { |c| params[:courses]&.include?(c['id'].to_s) }
@@ -71,27 +76,29 @@ def create
   def sync_assignments
     return render json: { error: 'Course not found.' }, status: :not_found unless @course
 
+    authorize @course
     @course.sync_assignments(@user)
     render json: { message: 'Assignments synced successfully.' }, status: :ok
   end
 
   def sync_enrollments
     return render json: { error: 'Course not found.' }, status: :not_found unless @course
 
+    authorize @course
     @course.sync_all_enrollments_from_canvas(@user.id)
     render json: { message: 'Users synced successfully.' }, status: :ok
   end
 
   def enrollments
+    authorize @course
     @side_nav = 'enrollments'
-    return redirect_to courses_path, alert: 'You do not have access to this page.' unless @role == 'instructor'
 
     @enrollments = @course.user_to_courses.includes(:user)
     @is_course_admin = @course.user_to_courses.find_by(user: @user)&.course_admin?
   end
 
   def delete
-    return redirect_to courses_path, alert: 'You do not have access to this page.' unless @role == 'instructor'
+    authorize @course
     return redirect_to courses_path, alert: 'Extensions are enabled for this course.' if @course.course_settings&.enable_extensions
 
     assignments = Assignment.where(course_to_lms_id: CourseToLms.where(course_id: @course.id).select(:id))

app/controllers/form_settings_controller.rb

@@ -2,17 +2,18 @@ class FormSettingsController < ApplicationController
   before_action :authenticated!
   before_action :authenticate_user
   before_action :set_course
-  before_action :ensure_instructor_role
   before_action :set_pending_request_count
 
   def edit
-    @side_nav = 'form_settings'
     @form_setting = @course.form_setting
+    authorize @form_setting
+    @side_nav = 'form_settings'
   end
 
   def update
     @side_nav = 'form_settings'
     @form_setting = @course.form_setting || @course.build_form_setting
+    authorize @form_setting
 
     permitted = form_setting_params.to_h
     defaulted = {

app/controllers/requests_controller.rb

@@ -11,9 +11,9 @@ class RequestsController < ApplicationController
   before_action :check_extensions_enabled_for_students, except: [ :export ]
   before_action :ensure_request_is_pending, only: %i[update approve reject]
   before_action :set_request, only: %i[show edit cancel]
-  before_action :check_instructor_permission, only: %i[approve reject mass_approve mass_reject]
 
   def index
+    authorize @course, :show?
     @side_nav = 'requests'
     if @role == 'student'
       @requests = @course.requests.for_user(@user)
@@ -30,12 +30,14 @@ def index
   end
 
   def show
+    authorize @request
     @assignment = @request.assignment
     @number_of_days = @request.calculate_days_difference if @request.requested_due_date.present? && @assignment&.due_date.present?
     render_role_based_view
   end
 
   def new
+    authorize @course, :show?
     @side_nav = 'form'
     # course_to_lms = @course.course_to_lms(1)
     course_to_lmss = @course.all_linked_lmss.pluck(:id)
@@ -55,8 +57,8 @@ def new
   end
 
   def new_for_student
+    authorize @course, policy_class: RequestPolicy
     @side_nav = 'form'
-    return redirect_to course_requests_path(@course), alert: 'You do not have permission to access this page.' unless @role == 'instructor'
 
     course_to_lmss = @course.all_linked_lmss.pluck(:id)
     return redirect_to courses_path, alert: 'No Canvas LMS data found for this course.' unless course_to_lmss.any?
@@ -91,7 +93,7 @@ def create
   end
 
   def create_for_student
-    return redirect_to course_requests_path(@course), alert: 'You do not have permission to perform this action.' unless @role == 'instructor'
+    authorize @course, policy_class: RequestPolicy
 
     student = User.find_by(id: params[:request][:user_id])
     return redirect_to new_course_request_path(@course), alert: 'Student not found.' unless student
@@ -134,6 +136,7 @@ def cancel
   end
 
   def approve
+    authorize @request
     @assignment = Assignment.find_by(id: @request.assignment_id)
     lms_facade = @assignment.lms_facade
     if @request.approve(lms_facade.from_user(@user), @user)
@@ -152,6 +155,7 @@ def approve
   end
 
   def reject
+    authorize @request
     if @request.reject(@user)
       notice = 'Request denied successfully.'
       respond_to do |format|
@@ -168,10 +172,12 @@ def reject
   end
 
   def mass_approve
+    authorize @course, policy_class: RequestPolicy
     process_mass_action(:approve)
   end
 
   def mass_reject
+    authorize @course, policy_class: RequestPolicy
     process_mass_action(:reject)
   end
 
@@ -196,11 +202,6 @@ def set_request
     redirect_to course_path(@course), alert: 'Request not found.' unless @request
   end
 
-  def check_instructor_permission
-    result = RequestService.check_instructor_permission(@role, course_path(@course))
-    redirect_to result[:redirect_to], alert: result[:alert] if result != true
-  end
-
   def handle_request_error
     flash.now[:alert] = 'There was a problem submitting your request.'
     @assignments = Assignment.where(course_to_lms_id: @course.course_to_lms(1).id, enabled: true).order(:name)

app/controllers/user_to_courses_controller.rb

@@ -1,10 +1,10 @@
 class UserToCoursesController < ApplicationController
   before_action :authenticate_user
   before_action :set_course
-  before_action :ensure_course_admin
 
   def toggle_allow_extended_requests
     @enrollment = @course.user_to_courses.find(params[:id])
+    authorize @enrollment
 
     if @enrollment.update(allow_extended_requests: params[:allow_extended_requests])
       render json: { success: true }, status: :ok
@@ -13,13 +13,4 @@ def toggle_allow_extended_requests
       render json: { redirect_to: course_path(@course) }, status: :unprocessable_content
     end
   end
-
-  private
-
-  def ensure_course_admin
-    enrollment = @course.user_to_courses.find_by(user: @user)
-    return if enrollment&.course_admin?
-
-    render json: { error: 'You must be an instructor or Lead TA.', redirect_to: course_path(@course) }, status: :forbidden
-  end
 end

app/models/course.rb

@@ -66,7 +66,7 @@ def enabled_assignments
   # Or is user.staff_role?(course) or user.student_role?(course) better?
   def user_role(user)
     roles = UserToCourse.where(user_id: user.id, course_id: id).pluck(:role)
-    return 'instructor' if roles.include?('teacher') || roles.include?('ta')
+    return 'instructor' if roles.include?('teacher') || roles.include?('ta') || roles.include?('leadta')
     return 'student' if roles.include?('student')
 
     nil

app/policies/application_policy.rb

@@ -0,0 +1,42 @@
+class ApplicationPolicy
+  attr_reader :user, :record
+
+  def initialize(user, record)
+    @user = user
+    @record = record
+  end
+
+  def index?
+    false
+  end
+
+  def show?
+    false
+  end
+
+  def create?
+    false
+  end
+
+  def new?
+    create?
+  end
+
+  def update?
+    false
+  end
+
+  def edit?
+    update?
+  end
+
+  def destroy?
+    false
+  end
+
+  private
+
+  def site_admin?
+    user&.admin?
+  end
+end

app/policies/assignment_policy.rb

@@ -0,0 +1,22 @@
+class AssignmentPolicy < ApplicationPolicy
+  # record is an Assignment
+
+  # Only course admins (teacher/leadta) can toggle assignment enabled status
+  def toggle_enabled?
+    course_admin? || site_admin?
+  end
+
+  private
+
+  def course
+    record.course_to_lms.course
+  end
+
+  def enrollment
+    @enrollment ||= UserToCourse.find_by(user: user, course_id: course.id)
+  end
+
+  def course_admin?
+    enrollment&.course_admin? || false
+  end
+end