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 <svc.cloudjenkins@instructure.com>
Reviewed-by: Simon Williams <simon@instructure.com>
QA-Review: Simon Williams <simon@instructure.com>
Product-Review: Simon Williams <simon@instructure.com>

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

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
 

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

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
 

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)

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)

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