From cba25100761ce6860a1440ee5785e4d5091eb27c Mon Sep 17 00:00:00 2001 From: Cody Cutrer Date: Mon, 26 Apr 2021 09:04:54 -0600 Subject: [PATCH] exclude deleted users from being found in general but still let site admins see them Change-Id: I643e365c26061e3f7c9e4835d5b6300d9cc424a8 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/263659 Tested-by: Service Cloud Jenkins Reviewed-by: Simon Williams QA-Review: Simon Williams Product-Review: Simon Williams --- app/controllers/application_controller.rb | 5 +- app/controllers/users_controller.rb | 78 +++++++++++++---------- app/models/account.rb | 1 + spec/apis/v1/users_api_spec.rb | 20 +++++- spec/controllers/users_controller_spec.rb | 24 +++++++ spec/integration/security_spec.rb | 2 +- spec/integration/users_controller_spec.rb | 2 +- 7 files changed, 93 insertions(+), 39 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 46378fcd8f7..c13f68687c0 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -906,7 +906,7 @@ class ApplicationController < ActionController::Base # to. So /courses/5/assignments would have a @context=Course.find(5). # Also assigns @context_membership to the membership type of @current_user # if @current_user is a member of the context. - def get_context + def get_context(include_deleted: false) GuardRail.activate(:secondary) do unless @context if params[:course_id] @@ -934,7 +934,8 @@ class ApplicationController < ActionController::Base @context_enrollment = @context.group_memberships.where(user_id: @current_user).first if @context && @current_user @context_membership = @context_enrollment elsif params[:user_id] || (self.is_a?(UsersController) && params[:user_id] = params[:id]) - @context = api_find(User, params[:user_id]) + scope = include_deleted ? User : User.active + @context = api_find(scope, params[:user_id]) params[:context_id] = params[:user_id] params[:context_type] = "User" @context_membership = @context if @context == @current_user diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index c8ac53a70ae..55a4fb00ebb 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1198,43 +1198,52 @@ class UsersController < ApplicationController def show GuardRail.activate(:secondary) do - get_context + get_context(include_deleted: true) @context_account = @context.is_a?(Account) ? @context : @domain_root_account - @user = params[:id] && params[:id] != 'self' ? User.find(params[:id]) : @current_user - if authorized_action(@user, @current_user, :read_full_profile) - add_crumb(t('crumbs.profile', "%{user}'s profile", :user => @user.short_name), @user == @current_user ? user_profile_path(@current_user) : user_path(@user) ) + @user = if @context.is_a?(User) + @context + else + api_find(@context.all_users, params[:id]) + end + allowed = @user.grants_right?(@current_user, session, :read_full_profile) + + raise ActiveRecord::RecordNotFound unless allowed - @group_memberships = @user.cached_current_group_memberships_by_date + add_crumb(t('crumbs.profile', "%{user}'s profile", :user => @user.short_name), @user == @current_user ? user_profile_path(@current_user) : user_path(@user) ) - # course_section and enrollment term will only be used if the enrollment dates haven't been cached yet; - # maybe should just look at the first enrollment and check if it's cached to decide if we should include - # them here - @enrollments = @user.enrollments. - shard(@user). - where("enrollments.workflow_state<>'deleted' AND courses.workflow_state<>'deleted'"). - eager_load(:course). - preload(:associated_user, :course_section, :enrollment_state, course: { enrollment_term: :enrollment_dates_overrides }).to_a + @group_memberships = @user.cached_current_group_memberships_by_date - # restrict view for other users - if @user != @current_user - @enrollments = @enrollments.select{|e| e.grants_right?(@current_user, session, :read)} + # course_section and enrollment term will only be used if the enrollment dates haven't been cached yet; + # maybe should just look at the first enrollment and check if it's cached to decide if we should include + # them here + @enrollments = @user.enrollments + .shard(@user) + .where("enrollments.workflow_state<>'deleted' AND courses.workflow_state<>'deleted'") + .eager_load(:course) + .preload(:associated_user, :course_section, :enrollment_state, course: { enrollment_term: :enrollment_dates_overrides }).to_a + + # restrict view for other users + if @user != @current_user + @enrollments = @enrollments.select{|e| e.grants_right?(@current_user, session, :read)} + end + + @enrollments = @enrollments.sort_by {|e| [e.state_sortable, e.rank_sortable, e.course.name] } + # pre-populate the reverse association + @enrollments.each { |e| e.user = @user } + + status = @user.deleted? ? 404 : 200 + respond_to do |format| + format.html do + @google_analytics_page_title = "User" + @body_classes << 'full-width' + js_env(CONTEXT_USER_DISPLAY_NAME: @user.short_name, + USER_ID: @user.id) + render status: status end - - @enrollments = @enrollments.sort_by {|e| [e.state_sortable, e.rank_sortable, e.course.name] } - # pre-populate the reverse association - @enrollments.each { |e| e.user = @user } - - respond_to do |format| - format.html do - @google_analytics_page_title = "User" - @body_classes << 'full-width' - js_env(CONTEXT_USER_DISPLAY_NAME: @user.short_name, - USER_ID: @user.id) - end - format.json do - render :json => user_json(@user, @current_user, session, %w{locale avatar_url}, - @current_user.pseudonym.account) - end + format.json do + render json: user_json(@user, @current_user, session, %w{locale avatar_url}, + @current_user.pseudonym.account), + status: status end end end @@ -1276,9 +1285,10 @@ class UsersController < ApplicationController @user.last_login = User.with_last_login.find(@user.id).read_attribute(:last_login) end - render :json => user_json(@user, @current_user, session, includes, @domain_root_account) + render json: user_json(@user, @current_user, session, includes, @domain_root_account), + status: @user.deleted? ? 404 : 200 else - render_unauthorized_action + raise ActiveRecord::RecordNotFound end end diff --git a/app/models/account.rb b/app/models/account.rb index 9826456d154..6289a1c3f95 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -58,6 +58,7 @@ class Account < ActiveRecord::Base has_many :sis_batches has_many :abstract_courses, :class_name => 'AbstractCourse', :foreign_key => 'account_id' has_many :root_abstract_courses, :class_name => 'AbstractCourse', :foreign_key => 'root_account_id' + has_many :all_users, -> { distinct }, through: :user_account_associations, source: :user has_many :users, :through => :active_account_users has_many :user_past_lti_ids, as: :context, inverse_of: :context has_many :pseudonyms, -> { preload(:user) }, inverse_of: :account diff --git a/spec/apis/v1/users_api_spec.rb b/spec/apis/v1/users_api_spec.rb index 4abbdacb0de..b16b436d47f 100644 --- a/spec/apis/v1/users_api_spec.rb +++ b/spec/apis/v1/users_api_spec.rb @@ -874,7 +874,25 @@ describe "Users API", type: :request do account_admin_user_with_role_changes(:role_changes => {:read_roster => false, :manage_user_logins => false}) api_call(:get, "/api/v1/users/#{@other_user.id}", {:controller => 'users', :action => 'api_show', :id => @other_user.id.to_param, :format => 'json'}, - {}, {}, {:expected_status => 401}) + {}, {}, {:expected_status => 404}) + end + + it "404s on a deleted user" do + @other_user.destroy + account_admin_user + json = api_call(:get, "/api/v1/users/#{@other_user.id}", + { :controller => 'users', :action => 'api_show', :id => @other_user.id.to_param, :format => 'json' }, + {}, expected_status: 404) + expect(json.keys).to eq ["errors"] + end + + it "404s but still returns the user on a deleted user for a site admin" do + @other_user.destroy + account_admin_user(account: Account.site_admin) + json = api_call(:get, "/api/v1/users/#{@other_user.id}", + { :controller => 'users', :action => 'api_show', :id => @other_user.id.to_param, :format => 'json' }, + {}, expected_status: 404) + expect(json.keys).not_to be_include("errors") end end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 910c3856df6..5b558f15aa5 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -2095,6 +2095,30 @@ describe UsersController do expect(assigns[:enrollments].sort_by(&:id)).to eq [@enrollment1, @enrollment2] end + it "404s on a deleted user" do + course_with_teacher(:active_all => 1) + + account_admin_user + user_session(@admin) + @teacher.destroy + + get 'show', params: { id: @teacher.id } + expect(response.status).to eq 404 + expect(response).not_to render_template('users/show') + end + + it "404s, but still shows, on a deleted user for site admins" do + course_with_teacher(:active_all => 1) + + account_admin_user(account: Account.site_admin) + user_session(@admin) + @teacher.destroy + + get 'show', params: { id: @teacher.id } + expect(response.status).to eq 404 + expect(response).to render_template('users/show') + end + it "should respond to JSON request" do account = Account.create! course_with_student(:active_all => true, :account => account) diff --git a/spec/integration/security_spec.rb b/spec/integration/security_spec.rb index 9f1b6731763..50bcdd9ce86 100644 --- a/spec/integration/security_spec.rb +++ b/spec/integration/security_spec.rb @@ -1062,7 +1062,7 @@ describe "security" do expect(response).to be_successful get "/users/#{@student.id}" - assert_status(401) + assert_status(404) admin = account_admin_user :account => Account.site_admin user_session(admin) diff --git a/spec/integration/users_controller_spec.rb b/spec/integration/users_controller_spec.rb index 0242fda40ba..075ce192996 100644 --- a/spec/integration/users_controller_spec.rb +++ b/spec/integration/users_controller_spec.rb @@ -166,7 +166,7 @@ describe UsersController do course_factory(:account => account_model) student_in_course(:course => @course) get "/users/#{@student.id}" - assert_status(401) + assert_status(404) end it "should show user to account users that have the read_roster permission" do