refactor grants_rights?, add cache expiration, refs #6058, #9957

clean up grants_right? and grants_rights?, and add cache expiration so
that stale data eventually clears. the cache-related changed are:

1. we now cache "nobody" lookups for course permissions
2. course permission lookups are cached for no more than a day

additionally, there is a slight behavior fix for non-course grants_rights?
calls that care about the session. previously, the session argument to a
"given" block would be set to nil unless the :session_affects_permissions
flag was set. this was *not* the case for grants_right?. so that means
that grants_rights? calls for a non-context could be more restrictive than
the equivalent grants_right? one(s).

a code audit reveals there should be no places where this was actually
happenening today, so the fix shouldn't affect any current permissions
checks in canvas. rubric_assessment.rb would be susceptible, but the
corresponding controller code is unused. eportfolio.rb has some session-
based policy checks, but it was setting :session_affects_permissions.

test plan:
1. use course-related functionality in canvas
2. confirm that the course permissions are cached (rails log) and used
3. do something that sets :session_affects_permissions (e.g. get a
   course invitation and go look at the course)
4. confirm that course permissions are no longer cached, but work
   correctly
5. confirm that non-course permission checks work correctly and are not
   cached (rails log)

Change-Id: Ie7f79054f48f6a9f168510349c3d1f1ef453deb4
Reviewed-on: https://gerrit.instructure.com/12933
Reviewed-by: Cody Cutrer <cody@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>

vendor/plugins/adheres_to_policy/lib/adheres_to_policy.rb

@@ -111,38 +111,23 @@ module Instructure #:nodoc:
           session = sought_rights.shift
         end
         sought_rights = (sought_rights || []).compact.uniq
-        check_rights = sought_rights
+
+        cache_lookup = is_a?(Course)
         # If you're going to add something to the user session that
         # affects permissions, you'd durn well better set :session_affects_permissions
         # to true as well
-        session = nil if session && !session[:session_affects_permissions]
-        # If this checking a course policy (which is the most expensive lookup)
-        # then just grab all the permissions at once instead of just the one
-        # being asked for.
-        if user && (self.is_a_context?) && !session
-          check_rights = []
-        end
-        # Generate cache key for memcached lookup
-        cache_lookup = ['course_permissions', self.cache_key, (user.cache_key rescue 'nobody'), check_rights.join('/')].join('/')
-        # Don't memcache the value unless it's checking all values for a course
-        cache_lookup = nil unless self.is_a?(Course) && check_rights.empty?
-        # Sometimes the session object holds information that can affect
-        # permission policies.  If this is true, we shouldn't cache the
-        # result, either.
-        cache_lookup = nil if session && session[:session_affects_permissions]
-        user_id = user ? user.id : nil
-        cache_param = [user_id, sought_rights].flatten
+        cache_lookup = false if session && session[:session_affects_permissions]
 
-        # According to my understanding, calling Rails.cache.fetch with an
-        # empty string will always return the contents of the block, and
-        # will not cache the value at all
-        granted_rights = []
-        if cache_lookup && RAILS_ENV != "test"
-          granted_rights = Rails.cache.fetch(cache_lookup) do
-            check_policy(user, session, *check_rights)
+        # Cache the lookup, iff this is a course and the session doesn't affect
+        # the policies. Since course policy lookups are expensive, we grab all
+        # the permissions at once
+        granted_rights = if cache_lookup
+          # Check and cache all the things!
+          Rails.cache.fetch(['course_permissions', self, user].cache_key, :expires_in => 1.day) do
+            check_policy(user)
           end
         else
-          granted_rights = check_policy(user, session, *check_rights)
+          check_policy(user, session, *sought_rights)
         end
 
         sought_rights = granted_rights if sought_rights.empty?
@@ -150,22 +135,12 @@ module Instructure #:nodoc:
         res
       end
 
-      def grants_right?(user, *sought_rights)
-        session = nil
-        if !sought_rights[0].is_a? Symbol
-          session = sought_rights.shift
-        end
-        sought_right = sought_rights[0].to_sym rescue nil
+      # user [, session], [, sought_right]
+      def grants_right?(user, *args)
+        sought_right = args.first.is_a?(Symbol) ? args.first : args[1].to_sym rescue nil
         return false unless sought_right
 
-        # if this is a course, call grants_rights which has specific logic to
-        # cache course rights lookups. otherwise we lose the benefits of that cache.
-        if self.is_a?(Course) && !(session && session[:session_affects_permissions])
-          return !!(self.grants_rights?(user, *[session].compact)[sought_right])
-        end
-
-        granted_rights = check_policy(user, session, *sought_rights)
-        granted_rights.include?(sought_right)
+        grants_rights?(user, *args)[sought_right]
       end
 
       # Used for a more-natural: user.has_rights?(@account, :destroy)

vendor/plugins/adheres_to_policy/spec_canvas/adheres_to_policy_spec.rb

@@ -248,6 +248,69 @@ describe Instructure::AdheresToPolicy::InstanceMethods do
     b.total.should == 2
   end
 
+  context "grants_right?" do
+    it "should check the policy" do
+      course(:active_all => true)
+      @course.grants_right?(@teacher, :read).should be_true
+      @course.grants_right?(@teacher, :asdf).should be_false
+    end
+  end
+
+  context "grants_rights?" do
+    it "should return all granted rights if no specific ones are sought" do
+      course(:active_all => true)
+      rights = @course.grants_rights?(@teacher)
+      rights.should_not be_empty
+    end
+
+    context "caching" do
+      it "should cache for courses" do
+        enable_cache do
+          Rails.cache.expects(:fetch).twice.with{ |p,| p =~ /course_permissions.*course/ }.returns([])
+          course().grants_rights?(user(:name => 'bob'))
+          # cache lookups for "nobody" as well
+          course().grants_rights?(nil)
+        end
+      end
+  
+      it "should not cache for courses if session[:session_affects_permissions]" do
+        enable_cache do
+          Rails.cache.expects(:fetch).never.with{ |p,| p =~ /course_permissions.*course/ }
+          Rails.cache.stubs(:fetch).with{ |p,| p !~ /course_permissions.*course/ }.returns([])
+          course().grants_rights?(user(:name => 'bob'), {:session_affects_permissions => true})
+        end
+      end
+  
+      it "should not cache for non-courses" do
+        enable_cache do
+          class B
+            extend Instructure::AdheresToPolicy::ClassMethods
+            set_policy {}
+          end
+          Rails.cache.expects(:fetch).never
+          B.new.grants_rights?(user(:name => 'bob'))
+        end
+      end
+
+      it "should not nil the session argument when not caching" do
+        enable_cache do
+          class B
+            attr_reader :session
+            extend Instructure::AdheresToPolicy::ClassMethods
+            set_policy {
+              given { |user, session| @session = session }
+              can :read
+            }
+          end
+          Rails.cache.expects(:fetch).never
+          b = B.new
+          b.grants_rights?(user(:name => 'bob'), {})
+          b.session.should_not be_nil
+        end
+      end
+    end
+  end
+
   after(:all) do
     Object.send(:remove_const, :A)
   end