Refactor RBAC permissions logic to centralize and test without external gems

app/controllers/application_controller.rb

@@ -1,4 +1,6 @@
 class ApplicationController < ActionController::Base
+  include Authorization
+
   before_action :authenticated!, unless: -> { excluded_controller_action? }
 
   rescue_from LmsFacade::LmsAPIError, with: :handle_lms_api_error
@@ -18,7 +20,6 @@ def excluded_controller_action?
     excluded_actions[controller]&.include?(action)
   end
 
-  # TODO: Refactor all auth methods
   helper_method :current_user
   def current_user
     if defined?(@current_user)
@@ -79,8 +80,8 @@ def handle_lms_api_error(error)
 
   def set_pending_request_count
     return unless defined?(@course) && @course.present? && defined?(@user) && @user.present?
-    # only calculating pending requests count if the role is instructor so we don't show it to students
-    return unless @course.user_role(@user) == 'instructor'
+
+    return unless current_policy.staff?
 
     @pending_requests_count = @course.requests.where(status: 'pending').count
   end
@@ -99,7 +100,7 @@ def render_role_based_view(options = {})
     instructor_view = "#{ctrl}/instructor_#{act}"
     student_view = "#{ctrl}/student_#{act}"
 
-    case @role
+    case current_policy.view_role
     when 'instructor'
       render instructor_view
     when 'student'
@@ -122,9 +123,6 @@ def set_course
   end
 
   def ensure_instructor_role
-    return if @role == 'instructor'
-
-    flash[:alert] = 'You do not have access to this page.'
-    redirect_to courses_path
+    authorize! :staff?
   end
 end

app/controllers/assignments_controller.rb

@@ -1,20 +1,17 @@
 class AssignmentsController < ApplicationController
   def toggle_enabled
     @assignment = Assignment.find(params[:id])
-    course = @assignment.course_to_lms.course
-    @role = params[:role] || course&.user_role(@user)
+    @course = @assignment.course_to_lms.course
+    @role = @course&.user_role(current_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! :can_toggle_assignment?, format: :json
+    return if performed?
 
     if @assignment.update(enabled: params[:enabled])
       render json: { success: true }, status: :ok
     else
       flash[:alert] = "Failed to update assignment: #{@assignment.errors.full_messages.to_sentence}"
-      render json: { redirect_to: course_path(course) }, status: :unprocessable_content
+      render json: { redirect_to: course_path(@course) }, status: :unprocessable_content
     end
   end
 end

app/controllers/concerns/authorization.rb

@@ -0,0 +1,47 @@
+module Authorization
+  extend ActiveSupport::Concern
+
+  included do
+    helper_method :current_policy
+  end
+
+  private
+
+  def current_policy
+    @current_policy ||= CoursePolicy.new(current_user, @course)
+  end
+
+  # Authorize an action using CoursePolicy. Redirects or renders 403 on failure.
+  #
+  # When used as a before_action, Rails automatically halts the filter chain
+  # after a redirect/render. When used inline in an action method, the caller
+  # must add `return if performed?` after the call to prevent double renders.
+  #
+  # Usage:
+  #   before_action -> { authorize! :can_manage_settings? }
+  #   # or define a named method:
+  #   def authorize_manage_settings = authorize!(:can_manage_settings?)
+  #
+  # Inline usage:
+  #   authorize! :can_edit_course?
+  #   return if performed?
+  def authorize!(permission, format: nil)
+    return if current_policy.public_send(permission)
+
+    deny_access!(format: format)
+  end
+
+  def deny_access!(format: nil, message: 'You do not have permission to perform this action.')
+    format ||= request.format.symbol
+
+    if format == :json
+      render json: { error: message, redirect_to: fallback_redirect_path }, status: :forbidden
+    else
+      redirect_to(fallback_redirect_path, alert: message)
+    end
+  end
+
+  def fallback_redirect_path
+    @course ? course_path(@course) : courses_path
+  end
+end

app/controllers/course_settings_controller.rb

@@ -2,7 +2,7 @@ class CourseSettingsController < ApplicationController
   before_action :authenticated!
   before_action :authenticate_user
   before_action :set_course
-  before_action :ensure_instructor_role
+  before_action :authorize_manage_settings
   before_action :set_pending_request_count
 
   # Default template settings
@@ -76,6 +76,10 @@ def course_settings_params
     )
   end
 
+  def authorize_manage_settings
+    authorize! :can_manage_settings?
+  end
+
   def set_pending_request_count
     @pending_requests_count = Request.where(course_id: @course&.id, status: 'pending').count
   end

app/controllers/courses_controller.rb

@@ -5,7 +5,7 @@ class CoursesController < ApplicationController
   before_action :determine_user_role
 
   def index
-    @teacher_courses = UserToCourse.includes(:course).where(user: @user, role: %w[teacher ta])
+    @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')
@@ -22,7 +22,7 @@ def show
     @side_nav = 'show'
     @course.regenerate_readonly_api_token_if_blank
 
-    if @role == 'student'
+    if current_policy.student? && !current_policy.staff?
       course_settings = @course.course_settings
       return redirect_to courses_path, alert: 'Extensions are not enabled for this course.' unless course_settings&.enable_extensions
 
@@ -43,8 +43,6 @@ def new
     @selected_semester = params[:semester]
 
     teacher_enrollment_types = %w[teacher ta]
-    # TODO: Add spec for when a course is created, but the user is not enrolled in it.
-    # TODO: Why do some courses have empty enrollments?
     existing_canvas_ids = @user.courses.pluck(:canvas_id)
     @courses_teacher = filter_courses(@courses, teacher_enrollment_types, existing_canvas_ids)
     @courses_student = filter_courses(@courses, [ 'student' ], existing_canvas_ids)
@@ -57,7 +55,7 @@ def new
 
   def edit
     @side_nav = 'edit'
-    redirect_to course_path(@course.id), alert: 'You do not have access to this page.' unless @role == 'instructor'
+    authorize! :can_edit_course?
   end
 
   def create
@@ -71,27 +69,35 @@ def create
   def sync_assignments
     return render json: { error: 'Course not found.' }, status: :not_found unless @course
 
+    authorize! :can_sync_assignments?, format: :json
+    return if performed?
+
     @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! :can_sync_enrollments?, format: :json
+    return if performed?
+
     @course.sync_all_enrollments_from_canvas(@user.id)
     render json: { message: 'Users synced successfully.' }, status: :ok
   end
 
   def enrollments
     @side_nav = 'enrollments'
-    return redirect_to courses_path, alert: 'You do not have access to this page.' unless @role == 'instructor'
+    authorize! :can_view_enrollments?
+    return if performed?
 
     @enrollments = @course.user_to_courses.includes(:user)
-    @is_course_admin = @course.user_to_courses.find_by(user: @user)&.course_admin?
+    @is_course_admin = current_policy.course_admin?
   end
 
   def delete
-    return redirect_to courses_path, alert: 'You do not have access to this page.' unless @role == 'instructor'
+    authorize! :can_delete_course?
+    return if performed?
     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,7 +2,7 @@ class FormSettingsController < ApplicationController
   before_action :authenticated!
   before_action :authenticate_user
   before_action :set_course
-  before_action :ensure_instructor_role
+  before_action :authorize_manage_form_settings
   before_action :set_pending_request_count
 
   def edit
@@ -44,6 +44,10 @@ def form_setting_params
     )
   end
 
+  def authorize_manage_form_settings
+    authorize! :can_manage_form_settings?
+  end
+
   def set_pending_request_count
     @pending_requests_count = Request.where(course_id: @course&.id, status: 'pending').count
   end

app/controllers/requests_controller.rb

@@ -11,11 +11,11 @@ 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]
+  before_action :authorize_approve_deny, only: %i[approve reject mass_approve mass_reject]
 
   def index
     @side_nav = 'requests'
-    if @role == 'student'
+    if !current_policy.staff?
       @requests = @course.requests.for_user(@user)
     elsif params[:show_all] == 'true'
       @requests = @course.requests.includes(:assignment)
@@ -41,10 +41,10 @@ def new
     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?
 
-    if @role == 'instructor'
+    if current_policy.staff?
       prepare_instructor_new_request(course_to_lmss)
       render :new_for_student and return
-    elsif @role == 'student'
+    elsif current_policy.student?
       redirected = prepare_student_new_request(course_to_lmss)
       return if redirected
 
@@ -56,7 +56,7 @@ def new
 
   def new_for_student
     @side_nav = 'form'
-    return redirect_to course_requests_path(@course), alert: 'You do not have permission to access this page.' unless @role == 'instructor'
+    return redirect_to course_requests_path(@course), alert: 'You do not have permission to access this page.' unless current_policy.staff?
 
     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 +91,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'
+    return redirect_to course_requests_path(@course), alert: 'You do not have permission to perform this action.' unless current_policy.staff?
 
     student = User.find_by(id: params[:request][:user_id])
     return redirect_to new_course_request_path(@course), alert: 'Student not found.' unless student
@@ -126,6 +126,9 @@ def update
   end
 
   def cancel
+    authorize! :can_cancel_request?
+    return if performed?
+
     if @request.reject(@user)
       redirect_to course_requests_path(@course), notice: 'Request canceled successfully.'
     else
@@ -196,9 +199,8 @@ 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
+  def authorize_approve_deny
+    authorize! :can_approve_or_deny_requests?
   end
 
   def handle_request_error
@@ -248,7 +250,7 @@ def ensure_request_is_pending
   end
 
   def check_extensions_enabled_for_students
-    result = RequestService.check_extensions_enabled_for_students(@role, @course, courses_path)
+    result = RequestService.check_extensions_enabled_for_students(current_policy, @course, courses_path)
     redirect_to result[:redirect_to], alert: result[:alert] if result != true
   end
 

app/controllers/user_to_courses_controller.rb

@@ -1,9 +1,11 @@
 class UserToCoursesController < ApplicationController
   before_action :authenticate_user
   before_action :set_course
-  before_action :ensure_course_admin
 
   def toggle_allow_extended_requests
+    authorize! :can_manage_extended_circumstances?, format: :json
+    return if performed?
+
     @enrollment = @course.user_to_courses.find(params[:id])
 
     if @enrollment.update(allow_extended_requests: params[:allow_extended_requests])
@@ -13,13 +15,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

@@ -62,11 +62,9 @@ def enabled_assignments
     assignments.where(enabled: true)
   end
 
-  # TODO: Replace this with staff_role?(user) or student_role?(user)
-  # 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 & UserToCourse.staff_roles).any?
     return 'student' if roles.include?('student')
 
     nil