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 4b465d8c1..f6c11f995 100644 --- a/rails/app/controllers/api/v1/classes_controller.rb +++ b/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 diff --git a/rails/app/controllers/api/v1/offerings_controller.rb b/rails/app/controllers/api/v1/offerings_controller.rb index a3cd81491..9d9173723 100644 --- a/rails/app/controllers/api/v1/offerings_controller.rb +++ b/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 diff --git a/rails/app/models/user.rb b/rails/app/models/user.rb index f06efeb00..a78058d3c 100644 --- a/rails/app/models/user.rb +++ b/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) diff --git a/rails/app/policies/portal/clazz_policy.rb b/rails/app/policies/portal/clazz_policy.rb index 4cee9d2fd..1cbc82455 100644 --- a/rails/app/policies/portal/clazz_policy.rb +++ b/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 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