fix: cleanup authorization in Classes and Offering API [PT-187154175]

rails/app/controllers/api/api_controller.rb

@@ -157,32 +157,4 @@ def auth_teacher(params)
 
     return auth
   end
-
-  def auth_student_or_teacher(params)
-    auth = auth_not_anonymous(params)
-    return auth if auth[:error]
-    user = auth[:user]
-
-    if !user.portal_student && !user.portal_teacher
-      auth[:error] = 'You must be logged in as a student or teacher to use this endpoint'
-    end
-
-    return auth
-  end
-
-  def auth_student_or_teacher_or_researcher(params)
-    auth = auth_not_anonymous(params)
-    return auth if auth[:error]
-    user = auth[:user]
-
-    # Check if the user is a researcher of ANY project - the controller will check for a specific resource
-    auth[:role] ||= {}
-    auth[:role][:is_project_researcher] = user && user.is_project_researcher?
-
-    if !user.portal_student && !user.portal_teacher && !auth[:role][:is_project_researcher]
-      auth[:error] = 'You must be logged in as a student or teacher or researcher to use this endpoint'
-    end
-
-    return auth
-  end
 end

rails/app/controllers/api/v1/classes_controller.rb

@@ -2,32 +2,24 @@ class API::V1::ClassesController < API::APIController
 
   # GET api/v1/classes/:id
   def show
-    auth = auth_student_or_teacher_or_researcher(params)
-    return error(auth[:error]) if auth[:error]
-    user = auth[:user]
-
     clazz = Portal::Clazz.find_by_id(params[:id])
     if !clazz
       return error('The requested class was not found')
     end
 
-    role_in_clazz = user.role_in_clazz(clazz)
+    authorize clazz, :api_show?
 
-    if (!role_in_clazz[:student] && !role_in_clazz[:teacher] && !role_in_clazz[:researcher])
-      return error('You are not a student or teacher or researcher of the requested class')
-    end
+    anonymize_students = !current_user.has_full_access_to_student_data?(clazz)
 
-    render_info(clazz, role_in_clazz[:researcher])
+    render_info(clazz, anonymize_students)
   end
 
   # GET api/v1/classes/mine
   # lists the users classes
   def mine
-    auth = auth_student_or_teacher(params)
-    return error(auth[:error]) if auth[:error]
-    user = auth[:user]
+    authorize Portal::Clazz, :mine?
 
-    user_with_clazzes = user.portal_student || user.portal_teacher
+    user_with_clazzes = current_user.portal_student || current_user.portal_teacher
 
     render :json => {
       classes: user_with_clazzes.clazzes.map do |clazz|
@@ -52,14 +44,13 @@ def info
   end
 
   def log_links
-    # allow only admins for now
-    return error('You must be an admin to use this endpoint') unless current_user && current_user.has_role?("admin")
-
     clazz = Portal::Clazz.find_by_id(params[:id])
     if !clazz
       return error('The requested class was not found')
     end
 
+    authorize clazz, :log_links?
+
     base_report_url = ENV["BASE_LOG_REPORT_URL"] || "https://log-puller.herokuapp.com"
 
     render :json => {
@@ -83,14 +74,10 @@ def log_links
   end
 
   def set_is_archived
-    auth = auth_teacher(params)
-    return error(auth[:error]) if auth[:error]
-    user = auth[:user]
+    clazz = Portal::Clazz.find_by_id(params[:id])
 
-    class_ownership = verify_teacher_class_ownership(user, params)
-    return error(class_ownership[:error]) if class_ownership[:error]
+    authorize clazz, :set_is_archived?
 
-    clazz = Portal::Clazz.find_by_id(params[:id])
     clazz.is_archived = ActiveModel::Type::Boolean.new.cast(params[:is_archived])
     clazz.save!
 
@@ -151,19 +138,6 @@ def render_info(clazz, anonymize)
     }
   end
 
-  def verify_teacher_class_ownership(user, params)
-    clazz = Portal::Clazz.find(params[:id])
-    if !clazz
-      return {error: 'The requested class was not found'}
-    end
-
-    if !clazz.is_teacher?(user)
-      return {error: 'You are not a teacher of the requested class'}
-    end
-
-    return {clazz: clazz}
-  end
-
   def render_ok
     render :json => { success: true }, :status => :ok
   end

rails/app/controllers/api/v1/offerings_controller.rb

@@ -5,10 +5,6 @@
 class API::V1::OfferingsController < API::APIController
 
   def show
-    auth = auth_student_or_teacher_or_researcher(params)
-    return error(auth[:error]) if auth[:error]
-    user = auth[:user]
-
     offering = Portal::Offering
                    .where(id: params[:id])
                    .includes(API::V1::Offering::INCLUDES_DEF)
@@ -17,13 +13,10 @@ def show
       return error('offering not found', 404)
     end
 
-    role_in_clazz = user.role_in_clazz(offering.clazz)
+    authorize offering, :api_show?
 
-    if (!role_in_clazz[:student] && !role_in_clazz[:teacher] && !role_in_clazz[:researcher])
-      return error('You are not a student or teacher or researcher of the requested offerings class')
-    end
+    anonymize_students = !current_user.has_full_access_to_student_data?(offering.clazz)
 
-    anonymize_students = role_in_clazz[:researcher]
     offering_api = API::V1::Offering.new(offering, request.protocol, request.host_with_port, current_user, params[:add_external_report], anonymize_students)
     render :json => offering_api.to_json, :callback => params[:callback]
   end

rails/app/models/user.rb

@@ -436,16 +436,26 @@ def is_researcher_for_clazz?(clazz)
       .count > 0
   end
 
-  def role_in_clazz(clazz)
-    # NOTE: these checks are set so only one of these can be true and teacher access is checked before researcher access
-    student_in_class = portal_student && portal_student.has_clazz?(clazz)
-    teacher_in_class = !student_in_class && (portal_teacher && portal_teacher.has_clazz?(clazz))
-    researcher_in_class = !teacher_in_class && is_researcher_for_clazz?(clazz)
-    {
-      student: student_in_class,
-      teacher: teacher_in_class,
-      researcher: researcher_in_class
-    }
+  def is_project_admin_for_clazz?(clazz)
+    # check if class has teacher in a cohort of a project the user is a admin of using a explicit join to avoid a
+    # bunch of unneeded object instantiation
+    admin_for_projects
+      .joins("INNER JOIN admin_cohorts __ac ON __ac.project_id = admin_projects.id")
+      .joins("INNER JOIN admin_cohort_items __aci ON __aci.admin_cohort_id = __ac.id AND __aci.item_type = 'Portal::Teacher'")
+      .joins("INNER JOIN portal_teachers __pt ON __pt.id = __aci.item_id")
+      .joins("INNER JOIN portal_teacher_clazzes __ptc ON __ptc.teacher_id = __pt.id")
+      .where("__ptc.clazz_id = ?", clazz.id)
+      .count > 0
+  end
+
+  def has_full_access_to_student_data?(clazz)
+    # Only admins, class students, teachers and project admins have full access to student data. Start checks from
+    # the "cheapest" queries so we don't need to execute complex queries if not necessary (e.g. is_project_admin_for_clazz).
+    return true if has_role?('admin')
+    return true if portal_student&.has_clazz?(clazz)
+    return true if portal_teacher&.has_clazz?(clazz)
+    return true if is_project_admin_for_clazz?(clazz)
+    false
   end
 
   def add_role_for_project(role, project)

rails/app/policies/portal/clazz_policy.rb

@@ -20,16 +20,42 @@ def resolve
     end
   end
 
+  # Used by API::V1::ClassesController:
+  def api_show?
+    class_teacher_or_admin? || class_student? || class_researcher?
+  end
+
+  def mine?
+    teacher? || student?
+  end
+
+  def log_links?
+    admin?
+  end
+
+  def set_is_archived?
+    class_teacher_or_admin?
+  end
+
+  # Used by Portal::ClazzesController:
   def materials?
     class_teacher? || class_researcher? || admin?
   end
 
   private
 
+  def class_student?
+    user && record && record.is_student?(user)
+  end
+
   def class_teacher?
     user && record && record.is_teacher?(user)
   end
 
+  def class_teacher_or_admin?
+    class_teacher? || admin?
+  end
+
   def class_researcher?
     user && record && user.is_researcher_for_clazz?(record)
   end

rails/spec/controllers/api/v1/classes_controller_spec.rb

@@ -44,8 +44,8 @@
 
     it "should fail when id is a class that the teacher doesn't own" do
       post :set_is_archived, params: { id: other_clazz.id }
-      expect(response).to have_http_status(:bad_request)
-      expect(JSON.parse(response.body)["message"]).to eq "You are not a teacher of the requested class"
+      expect(response).to have_http_status(:forbidden)
+      expect(JSON.parse(response.body)["message"]).to eq "Not authorized"
     end
 
     it "should succeed when the id is a class the teacher owns" do
@@ -66,7 +66,7 @@
     it 'GET mine' do
       get :mine
 
-      expect(response).to have_http_status(:bad_request)
+      expect(response).to have_http_status(:forbidden)
     end
   end