avoid several permission checks for course section visibility

* check read_roster first, as it's the mostly likely permission that a
   user will have, and if they don't, they won't have _any_ permissions,
   so you only need to check the one
 * for course_section_visibility, we don't care about the difference
   between :full and :limited, so pass that hint along so that we don't
   need to check any more permissions deep inside if we already know they
   can read_roster, and that they're not section limited

this is an optimization for planner, to avoid checking lots of permissions
in the common case of being passed courses that we can access, but only
needing to know the sections we can see in that course

Change-Id: I1ac7f98e5a825233569acf517d166fd38b1d8bfe
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/271732
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Simon Williams <simon@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>

app/helpers/search_helper.rb

@@ -109,7 +109,7 @@ module SearchHelper
 
       if context.is_a?(Course)
         add_courses.call [context], :current
-        visibility = context.enrollment_visibility_level_for(@current_user, context.section_visibilities_for(@current_user), true)
+        visibility = context.enrollment_visibility_level_for(@current_user, context.section_visibilities_for(@current_user), require_message_permission: true)
         sections = case visibility
                    when :sections, :sections_limited, :limited
           context.sections_visible_to(@current_user)
@@ -126,7 +126,7 @@ module SearchHelper
           add_courses.call [context.context], :current if context.context.is_a?(Course)
         end
       elsif context.is_a?(CourseSection)
-        visibility = context.course.enrollment_visibility_level_for(@current_user, context.course.section_visibilities_for(@current_user), true)
+        visibility = context.course.enrollment_visibility_level_for(@current_user, context.course.section_visibilities_for(@current_user), require_message_permission: true)
         sections = visibility == :restricted ? [] : [context]
         add_courses.call [context.course], :current
         add_sections.call context.course.sections_visible_to(@current_user, sections)

app/models/course.rb

@@ -2774,21 +2774,17 @@ class Course < ActiveRecord::Base
     end
   end
 
-  # returns :all, :none, or an array of section ids
+  # returns :all or an array of section ids
   def course_section_visibility(user, opts={})
     visibilities = section_visibilities_for(user, opts)
-    visibility = enrollment_visibility_level_for(user, visibilities)
-    if [:full, :limited, :restricted, :sections, :sections_limited].include?(visibility)
-      enrollment_types = ['StudentEnrollment', 'StudentViewEnrollment', 'ObserverEnrollment']
-      if [:restricted, :sections].include?(visibility) || (
-          visibilities.any? && visibilities.all? { |v| enrollment_types.include? v[:type] }
-        )
-        visibilities.map{ |s| s[:course_section_id] }.sort
-      else
-        :all
-      end
+    visibility = enrollment_visibility_level_for(user, visibilities, check_full: false)
+    enrollment_types = ['StudentEnrollment', 'StudentViewEnrollment', 'ObserverEnrollment']
+    if [:restricted, :sections].include?(visibility) || (
+        visibilities.any? && visibilities.all? { |v| enrollment_types.include? v[:type] }
+      )
+      visibilities.map { |s| s[:course_section_id] }.sort # rubocop:disable Rails/Pluck
     else
-      :none
+      :all
     end
   end
 
@@ -2806,29 +2802,43 @@ class Course < ActiveRecord::Base
     end
   end
 
-  def enrollment_visibility_level_for(user, visibilities = section_visibilities_for(user), require_message_permission = false)
+  # check_full is a hint that we don't care about the difference between :full and :limited,
+  # so don't bother with an extra permission check to see if they have :full. Just return :limited.
+  def enrollment_visibility_level_for(user,
+                                      visibilities = section_visibilities_for(user),
+                                      require_message_permission: false,
+                                      check_full: true)
     manage_perm = if self.root_account.feature_enabled? :granular_permissions_manage_users
       :allow_course_admin_actions
     else
       :manage_admin_users
     end
 
-    permissions = require_message_permission ?
-      [:send_messages] :
-      [:manage_grades, :manage_students, manage_perm, :read_roster, :view_all_grades, :read_as_admin]
-    granted_permissions = self.granted_rights(user, *permissions)
-    if granted_permissions.empty?
-      :restricted # e.g. observer, can only see admins in the course
-    elsif visibilities.present? && visibility_limited_to_course_sections?(user, visibilities)
-      if granted_permissions.eql? [:read_roster]
-        :sections_limited
-      else
-        :sections
-      end
-    elsif granted_permissions.eql? [:read_roster]
-      :limited
-    else
+    return :restricted if require_message_permission && !grants_right?(user, :send_messages)
+
+    has_read_roster = grants_right?(user, :read_roster) unless require_message_permission
+
+    visibility_limited_to_section = visibilities.present? && visibility_limited_to_course_sections?(user, visibilities)
+    if require_message_permission
+      has_admin = true
+    elsif visibility_limited_to_section || check_full || !has_read_roster
+      has_admin = grants_any_right?(user,
+                                    :read_as_admin,
+                                    :view_all_grades,
+                                    :manage_grades,
+                                    :manage_students,
+                                    manage_perm)
+    end
+
+    # e.g. observer, can only see admins in the course
+    return :restricted unless has_read_roster || has_admin
+ 
+    if visibility_limited_to_section
+      has_admin ? :sections : :sections_limited
+    elsif has_admin
       :full
+    else
+      :limited
     end
   end
 

lib/messageable_user/calculator.rb

@@ -972,7 +972,7 @@ class MessageableUser
     def course_visibility(course)
       @course_visibilities ||= {}
       @course_visibilities[course.global_id] ||=
-        course.enrollment_visibility_level_for(@user, course.section_visibilities_for(@user), true)
+        course.enrollment_visibility_level_for(@user, course.section_visibilities_for(@user), require_message_permission: true)
     end
 
     def all_courses_by_visibility(visibility)

spec/models/course_spec.rb

@@ -5087,11 +5087,11 @@ describe Course, "section_visibility" do
 
   context "require_message_permission" do
     it "should check the message permission" do
-      expect(@course.enrollment_visibility_level_for(@teacher, @course.section_visibilities_for(@teacher), true)).to eql :full
-      expect(@course.enrollment_visibility_level_for(@observer, @course.section_visibilities_for(@observer), true)).to eql :restricted
+      expect(@course.enrollment_visibility_level_for(@teacher, @course.section_visibilities_for(@teacher), require_message_permission: true)).to eql :full
+      expect(@course.enrollment_visibility_level_for(@observer, @course.section_visibilities_for(@observer), require_message_permission: true)).to eql :restricted
       RoleOverride.create!(:context => @course.account, :permission => 'send_messages',
                            :role => student_role, :enabled => false)
-      expect(@course.enrollment_visibility_level_for(@student1, @course.section_visibilities_for(@student1), true)).to eql :restricted
+      expect(@course.enrollment_visibility_level_for(@student1, @course.section_visibilities_for(@student1), require_message_permission: true)).to eql :restricted
     end
   end
 end