From 56069d8abe8a66bc228c31c48c22de0886870b95 Mon Sep 17 00:00:00 2001 From: Cody Cutrer Date: Fri, 12 Aug 2011 10:12:13 -0600 Subject: [PATCH] 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 Tested-by: Selenium Reviewed-by: Bracken Mosbacker --- app/models/account.rb | 37 ++++++++++++++----------------------- app/models/course.rb | 17 +++++++++++++---- spec/models/user_spec.rb | 3 ++- 3 files changed, 29 insertions(+), 28 deletions(-) diff --git a/app/models/account.rb b/app/models/account.rb index 91ef91826ee..d158a330d0a 100644 --- a/app/models/account.rb +++ b/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| diff --git a/app/models/course.rb b/app/models/course.rb index 62b83ea9ac1..e0fa0500bdf 100644 --- a/app/models/course.rb +++ b/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? diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index d64c6e1abb6..c29b64d36d4 100644 --- a/spec/models/user_spec.rb +++ b/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