simplify permission policies for Course and Account

Basically, change from using recursive calls to a single flattened call.

Interestingly, this change caught a bug in the become user spec, so
apparently the old code wasn't quite working as intended.

Change-Id: I20fce8dbc5ca7c4797a4f8bd929202aaf8a96f9c
Reviewed-on: https://gerrit.instructure.com/5062
Tested-by: Hudson <hudson@instructure.com>
Tested-by: Selenium <selenium@instructure.com>
Reviewed-by: Bracken Mosbacker <bracken@instructure.com>

app/models/account.rb

@@ -358,7 +358,7 @@ class Account < ActiveRecord::Base
     Canvas::Security.decrypt_password(self.turnitin_crypted_secret, self.turnitin_salt, 'instructure_turnitin_secret_shared')
   end
   
-  def account_chain
+  def account_chain(opts = {})
     res = [self]
     account = self
     while account.parent_account
@@ -366,6 +366,7 @@ class Account < ActiveRecord::Base
       res << account
     end
     res << self.root_account unless res.include?(self.root_account)
+    res << Account.site_admin if opts[:include_site_admin] && !self.site_admin?
     res.compact
   end
   
@@ -483,13 +484,7 @@ class Account < ActiveRecord::Base
     self.membership_types = self.account_membership_types.select{|t| t != type}.join(',')
     self.save
   end
-  
-  def membership_allows(user, permission)
-    return false unless user && permission
-    account_users = AccountUser.for_user(user).select{|a| a.account_id == self.id}
-    result = account_users.any?{|e| e.has_permission_to?(permission) }
-  end
-  
+
   def account_authorization_config
     # We support multiple auth configs per account, but several places we assume there is only one.
     # This is for compatibility with those areas. TODO: migrate everything to supporting multiple
@@ -518,26 +513,22 @@ class Account < ActiveRecord::Base
     state :active
     state :deleted
   end
+
+  def account_users_for(user)
+    @account_chain_ids ||= self.account_chain(:include_site_admin => true).map { |a| a.active? ? a.id : nil }.compact
+    @account_users_cache ||= {}
+    @account_users_cache[user] ||= AccountUser.find(:all, :conditions => { :account_id => @account_chain_ids, :user_id => user.id }) if user
+    @account_users_cache[user] ||= []
+    @account_users_cache[user]
+  end
   
   set_policy do
-    RoleOverride.permissions.each do |permission, params|
-      given {|user, session| self.membership_allows(user, permission) }
-      can permission
-      
-      given {|user, session| self.parent_account && self.parent_account.grants_right?(user, session, permission) }
-      can permission
-
-      given {|user, session| !site_admin? && Account.site_admin.grants_right?(user, session, permission) }
+    RoleOverride.permissions.each_key do |permission|
+      given { |user| self.account_users_for(user).any? { |au| au.has_permission_to?(permission) } }
       can permission
     end
 
-    given { |user| self.active? && self.users.include?(user) }
-    can :read and can :manage and can :update and can :delete
-
-    given { |user| self.parent_account && self.parent_account.grants_right?(user, nil, :manage) }
-    can :read and can :manage and can :update and can :delete
-
-    given { |user| !site_admin? && Account.site_admin_user?(user, :manage) }
+    given { |user| !self.account_users_for(user).empty? }
     can :read and can :manage and can :update and can :delete
 
     given { |user|

app/models/course.rb

@@ -755,7 +755,7 @@ class Course < ActiveRecord::Base
     given { |user| self.available? && self.is_public }
     can :read
     
-    RoleOverride.permissions.each do |permission, params|
+    RoleOverride.permissions.each_key do |permission|
       given {|user, session| self.enrollment_allows(user, session, permission) || self.account_membership_allows(user, session, permission) }
       can permission
     end
@@ -890,12 +890,21 @@ class Course < ActiveRecord::Base
     return (self.account || self.root_account).name
   end
   memoize :institution_name
-  
+
+  def account_users_for(user)
+    @associated_account_ids ||= (self.associated_accounts + [Account.site_admin]).map { |a| a.active? ? a.id: nil }.compact
+    @account_users ||= {}
+    @account_users[user] ||= AccountUser.find(:all, :conditions => { :account_id => @associated_account_ids, :user_id => user.id }) if user
+    @account_users[user] ||= nil
+    @account_users[user]
+  end
+
   def account_membership_allows(user, session, permission)
-    return false unless user && permission && AccountUser.any_for?(user) #.for_user(user).length > 0
+    return false unless user && permission
     return false if session && session["role_course_#{self.id}"]
+
     @membership_allows ||= {}
-    @membership_allows[[user.id, permission]] ||= (self.associated_accounts + [Account.site_admin]).uniq.any?{|a| a.membership_allows(user, permission) }
+    @membership_allows[[user.id, permission]] ||= self.account_users_for(user).any? { |au| au.has_permission_to?(permission) }
   end
   
   def teacherless?

spec/models/user_spec.rb

@@ -356,7 +356,8 @@ describe User do
       user.grants_right?(@admin, nil, :become_user).should be_false
       user.grants_right?(@site_admin, nil, :become_user).should be_true
       @account2.add_user(@admin)
-      user.grants_right?(@admin, nil, :become_user).should be_true
+      user.grants_right?(@admin, nil, :become_user).should be_false
+      user.grants_right?(@site_admin, nil, :become_user).should be_true
     end
 
     it "should not grant become_user for dis-associated users" do