Merge pull request #1256 from concord-consortium/187154175-researcher…

…-class-access

Give researchers access to class page and anonymize student names [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,36 +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]
-    role = auth[:role]
-
     clazz = Portal::Clazz.find_by_id(params[:id])
     if !clazz
       return error('The requested class was not found')
     end
 
-    # NOTE: these checks are set so only one of these can be true and researcher access is checked before teacher access
-    student_in_class = user.portal_student && user.portal_student.has_clazz?(clazz)
-    teacher_in_class = !student_in_class || (user.portal_teacher && user.portal_teacher.has_clazz?(clazz))
-    researcher_in_class = !teacher_in_class || (role[:is_project_researcher] && user.is_researcher_for_clazz?(clazz))
+    authorize clazz, :api_show?
 
-    if (!student_in_class && !teacher_in_class && !researcher_in_class)
-      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, researcher_in_class)
+    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|
@@ -56,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 => {
@@ -87,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!
 
@@ -131,8 +114,8 @@ def render_info(clazz, anonymize)
           :id => url_for(student.user),
           :user_id => student.user.id,
           :email => student.user.email,
-          :first_name => anonymize ? "Student" : student.user.first_name,
-          :last_name => anonymize ? "#{student.id}" : student.user.last_name
+          :first_name => anonymize ? student.anonymized_first_name : student.user.first_name,
+          :last_name => anonymize ? student.anonymized_last_name : student.user.last_name
         }
       },
       :offerings => clazz.teacher_visible_offerings.map { |offering|
@@ -155,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

@@ -12,8 +12,12 @@ def show
     unless offering
       return error('offering not found', 404)
     end
+
     authorize offering, :api_show?
-    offering_api = API::V1::Offering.new(offering, request.protocol, request.host_with_port, current_user, params[:add_external_report])
+
+    anonymize_students = !current_user.has_full_access_to_student_data?(offering.clazz)
+
+    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/controllers/portal/clazzes_controller.rb

@@ -15,9 +15,7 @@ class Portal::ClazzesController < ApplicationController
   # accessing.
   #
   include RestrictedTeacherController
-  before_action :check_teacher_owns_clazz, :only => [   :roster,
-                                                        :materials,
-                                                        :fullstatus ]
+  before_action :check_teacher_owns_clazz, :only => [ :roster ]
 
   def current_clazz
     # PUNDIT_REVIEW_AUTHORIZE
@@ -303,20 +301,12 @@ def manage_classes
   end
 
   def materials
-    # PUNDIT_REVIEW_AUTHORIZE
-    # PUNDIT_CHOOSE_AUTHORIZE
-    # no authorization needed ...
-    # authorize Portal::Clazz
-    # authorize @clazz
-    # authorize Portal::Clazz, :new_or_create?
-    # authorize @clazz, :update_edit_or_destroy?
-
-
     @portal_clazz = Portal::Clazz.includes(:offerings => :learners, :students => :user).find(params[:id])
 
+    authorize @portal_clazz, :materials?
+
     # Save the left pane sub-menu item
     Portal::Teacher.save_left_pane_submenu_item(current_visitor, Portal::Teacher.LEFT_PANE_ITEM['MATERIALS'])
-
   end
 
 

rails/app/helpers/navigation_helper.rb

@@ -195,12 +195,6 @@ def clazz_links_for_teacher
         label: nav_label("class_setup"),
         url: url_for([:edit, clazz])
       }
-      # TODO: Delete this one, its not used any more:
-      # clazz_links << {
-      #   id: "#{section_id}/status",
-      #   label: nav_label("full_status"),
-      #   url: url_for([:fullstatus, clazz])
-      # }
       clazz_links << {
         id: "#{section_id}/links",
         label: nav_label("links"),

rails/app/models/api/v1/offering.rb

@@ -28,10 +28,10 @@ class OfferingStudent
     attribute :total_progress, Float
     attribute :detailed_progress, Array
 
-    def initialize(student, offering, protocol, host_with_port)
+    def initialize(student, offering, protocol, host_with_port, anonymize = false)
       self.name = student.user.name
-      self.first_name = student.user.first_name
-      self.last_name = student.user.last_name
+      self.first_name = anonymize ? student.anonymized_first_name : student.user.first_name
+      self.last_name = anonymize ? student.anonymized_last_name : student.user.last_name
       self.username = student.user.login
       self.user_id = student.user.id
       learner = offering.learners.find { |l| l.student_id === student.id }
@@ -76,7 +76,7 @@ def report_attributes(report, offering, protocol, host_with_port)
     }
   end
 
-  def initialize(offering, protocol, host_with_port, current_user, additional_external_report_id)
+  def initialize(offering, protocol, host_with_port, current_user, additional_external_report_id, anonymize_students = false)
     runnable = offering.runnable
     self.id = offering.id
     self.teacher = offering.clazz.teacher.name
@@ -111,6 +111,6 @@ def initialize(offering, protocol, host_with_port, current_user, additional_exte
       end
     end
 
-    self.students = offering.clazz.students.map { |s| OfferingStudent.new(s, offering, protocol, host_with_port) }
+    self.students = offering.clazz.students.map { |s| OfferingStudent.new(s, offering, protocol, host_with_port, anonymize_students) }
   end
 end

rails/app/models/portal/student.rb

@@ -67,6 +67,14 @@ def self.generate_user_login(first_name, last_name)
     return generated_login
   end
 
+  def anonymized_first_name
+    "Student"
+  end
+
+  def anonymized_last_name
+    id.to_s
+  end
+
   def status(offerings_updated_after=0)
     # If offerings_updated_after is provided, all the offerings that haven't been updated
     # after this timestamp will be filtered out from the results (performance optimization).

rails/app/models/user.rb

@@ -436,6 +436,28 @@ def is_researcher_for_clazz?(clazz)
       .count > 0
   end
 
+  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)
     role_attribute = "is_#{role}"
     project_user = project_users.find_by_project_id project.id

rails/app/policies/portal/clazz_policy.rb

@@ -19,4 +19,44 @@ def resolve
       end
     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
 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