diff --git a/rails/app/controllers/api/api_controller.rb b/rails/app/controllers/api/api_controller.rb index 84156ef8a..57c1e5aed 100644 --- a/rails/app/controllers/api/api_controller.rb +++ b/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 diff --git a/rails/app/controllers/api/v1/classes_controller.rb b/rails/app/controllers/api/v1/classes_controller.rb index 524a2ab30..f6c11f995 100644 --- a/rails/app/controllers/api/v1/classes_controller.rb +++ b/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 diff --git a/rails/app/controllers/api/v1/offerings_controller.rb b/rails/app/controllers/api/v1/offerings_controller.rb index d4d377e02..9d9173723 100644 --- a/rails/app/controllers/api/v1/offerings_controller.rb +++ b/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 diff --git a/rails/app/controllers/portal/clazzes_controller.rb b/rails/app/controllers/portal/clazzes_controller.rb index 43fa5ba71..f76f5b846 100644 --- a/rails/app/controllers/portal/clazzes_controller.rb +++ b/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 diff --git a/rails/app/helpers/navigation_helper.rb b/rails/app/helpers/navigation_helper.rb index aaefc0f63..a5d4c0f59 100644 --- a/rails/app/helpers/navigation_helper.rb +++ b/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"), diff --git a/rails/app/models/api/v1/offering.rb b/rails/app/models/api/v1/offering.rb index fea4e728e..9e6e38da9 100644 --- a/rails/app/models/api/v1/offering.rb +++ b/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 diff --git a/rails/app/models/portal/student.rb b/rails/app/models/portal/student.rb index 65ab302f3..8c9880a40 100644 --- a/rails/app/models/portal/student.rb +++ b/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). diff --git a/rails/app/models/user.rb b/rails/app/models/user.rb index 582676266..a78058d3c 100644 --- a/rails/app/models/user.rb +++ b/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 diff --git a/rails/app/policies/portal/clazz_policy.rb b/rails/app/policies/portal/clazz_policy.rb index fa4c0cb26..1cbc82455 100644 --- a/rails/app/policies/portal/clazz_policy.rb +++ b/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 diff --git a/rails/spec/controllers/api/v1/classes_controller_spec.rb b/rails/spec/controllers/api/v1/classes_controller_spec.rb index 66f023368..7abbe4b7d 100644 --- a/rails/spec/controllers/api/v1/classes_controller_spec.rb +++ b/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