refactor guardian class for clarity & correctness

introduce NullUser to avoid type-checking
DRY up code
reduce number of multiple returns
remove some redundant/impossible logic branches
add pending test for possible bug
add test & fix for ability to flag archived posts
add #secure_category? method to topic class
Fix bug that prevented flagging of archived topics
Rename NullUser to AnonymousUser
DRY up can_<action>? methods
Fix some ownership logic, and a test, for Guardian

app/models/topic.rb

@@ -745,4 +745,7 @@ class Topic < ActiveRecord::Base
     self.auto_close_at = (num_days and num_days.to_i > 0.0 ? num_days.to_i.days.from_now : nil)
   end
 
+  def secure_category?
+    category && category.secure
+  end
 end

lib/guardian.rb

@@ -1,60 +1,66 @@
 # The guardian is responsible for confirming access to various site resources and operations
 class Guardian
 
-  attr_reader :user
+  class AnonymousUser
-
+    def blank?; true; end
-  def initialize(user=nil)
+    def admin?; false; end
-    @user = user
+    def staff?; false; end
+    def approved?; false; end
+    def secure_category_ids; []; end
+    def has_trust_level?(level); false; end
   end
 
-  def current_user
+  def initialize(user=nil)
-    @user
+    @user = user.presence || AnonymousUser.new
+  end
+
+  def user
+    @user.presence
+  end
+  alias :current_user :user
+
+  def anonymous?
+    !authenticated?
+  end
+
+  def authenticated?
+    @user.present?
   end
 
   def is_admin?
-    @user && @user.admin?
+    @user.admin?
   end
 
   def is_staff?
-    @user && @user.staff?
+    @user.staff?
   end
 
   # Can the user see the object?
   def can_see?(obj)
-    return false if obj.blank?
+    if obj
-
+      see_method = method_name_for :see, obj
-    see_method = :"can_see_#{obj.class.name.underscore}?"
+      return (see_method ? send(see_method, obj) : true)
-    return send(see_method, obj) if respond_to?(see_method)
+    end
-
-    return true
   end
 
   # Can the user edit the obj
   def can_edit?(obj)
-    return false if obj.blank?
+    if obj && authenticated?
-    return false if @user.blank?
+      edit_method = method_name_for :edit, obj
-
+      return (edit_method ? send(edit_method, obj) : true)
-    edit_method = :"can_edit_#{obj.class.name.underscore}?"
+    end
-    return send(edit_method, obj) if respond_to?(edit_method)
-
-    true
   end
 
   # Can we delete the object
   def can_delete?(obj)
-    return false if obj.blank?
+    if obj && authenticated?
-    return false if @user.blank?
+      delete_method = method_name_for :delete, obj
-
+      return (delete_method ? send(delete_method, obj) : true)
-    delete_method = :"can_delete_#{obj.class.name.underscore}?"
+    end
-    return send(delete_method, obj) if respond_to?(delete_method)
-
-    true
   end
 
   def can_moderate?(obj)
-    return false if obj.blank?
+    obj && is_staff?
-    return false if @user.blank?
-    @user.staff?
   end
   alias :can_move_posts? :can_moderate?
   alias :can_see_flags? :can_moderate?
@@ -62,8 +68,7 @@ class Guardian
 
   # Can the user create a topic in the forum
   def can_create?(klass, parent=nil)
-    return false if klass.blank?
+    return false unless authenticated? && klass
-    return false if @user.blank?
 
     # If no parent is provided, we look for a can_i_create_klass?
     # custom method.
@@ -84,79 +89,52 @@ class Guardian
 
   # Can we impersonate this user?
   def can_impersonate?(target)
-    return false if target.blank?
+    target &&
-    return false if @user.blank?
 
     # You must be an admin to impersonate
-    return false unless @user.admin?
+    is_admin? &&
 
     # You may not impersonate other admins
-    return false if target.admin?
+    not(target.admin?)
 
-    # You may not impersonate yourself
+    # Additionally, you may not impersonate yourself;
-    return false if @user == target
+    # but the two tests for different admin statuses
-
+    # make it impossible to be the same user.
-    true
   end
 
   # Can we approve it?
   def can_approve?(target)
-    return false if target.blank?
+    is_staff? && target && not(target.approved?)
-    return false if @user.blank?
-    return false if target.approved?
-    @user.staff?
   end
   alias :can_activate? :can_approve?
 
   def can_ban?(user)
-    is_staff? && user && !user.staff?
+    user && is_staff? && not(user.staff?)
   end
-
   alias :can_deactivate? :can_ban?
 
   def can_clear_flags?(post)
-    return false if @user.blank?
+    is_staff? && post
-    return false if post.blank?
-    @user.staff?
   end
 
   def can_revoke_admin?(admin)
-    return false unless @user.try(:admin?)
+    can_administer_user?(admin) && admin.admin?
-    return false if admin.blank?
-    return false if @user.id == admin.id
-    return false unless admin.admin?
-    true
   end
 
   def can_grant_admin?(user)
-    return false unless @user.try(:admin?)
+    can_administer_user?(user) && not(user.admin?)
-    return false if user.blank?
-    return false if @user.id == user.id
-    return false if user.admin?
-    true
   end
 
   def can_revoke_moderation?(moderator)
-    return false unless is_admin?
+    can_administer?(moderator) && moderator.moderator?
-    return false if moderator.blank?
-    return false if @user.id == moderator.id && !is_admin?
-    return false unless moderator.moderator?
-    true
   end
 
   def can_grant_moderation?(user)
-    return false unless is_admin?
+    can_administer?(user) && not(user.moderator?)
-    return false unless user
-    return false if @user.id == user.id && !is_admin?
-    return false if user.moderator?
-    true
   end
 
   def can_delete_user?(user_to_delete)
-    return false unless is_admin?
+    can_administer?(user_to_delete) && user_to_delete.post_count <= 0
-    return false unless user_to_delete
-    return false if user_to_delete.post_count > 0
-    true
   end
 
   # Can we see who acted on a post in a particular way?
@@ -187,35 +165,26 @@ class Guardian
   end
 
   def can_see_pending_invites_from?(user)
-    return false unless user && @user
+    is_me?(user)
-    return user == @user
   end
 
   # For now, can_invite_to is basically can_see?
   def can_invite_to?(object)
-    return false unless @user
+    authenticated? && can_see?(object) &&
-    return false unless can_see?(object)
+    not(SiteSetting.must_approve_users?) &&
-    return false if SiteSetting.must_approve_users?
+    (@user.has_trust_level?(:regular) || is_staff?)
-    @user.has_trust_level?(:regular) || @user.staff?
   end
 
-
   def can_see_deleted_posts?
-    return true if is_staff?
+    is_staff?
-    false
   end
 
   def can_see_private_messages?(user_id)
-    return true if is_staff?
+    is_staff? || (authenticated? && @user.id == user_id)
-    return false unless @user
-    @user.id == user_id
   end
 
   def can_delete_all_posts?(user)
-    return false unless is_staff?
+    is_staff? && user.created_at >= 7.days.ago
-    return false if user.created_at < 7.days.ago
-
-    true
   end
 
   # Support for ensure_{blah}! methods.
@@ -243,10 +212,7 @@ class Guardian
   end
 
   def can_create_post_on_topic?(topic)
-    return true if is_staff?
+    is_staff? || not(topic.closed? || topic.archived?)
-    return false if topic.closed?
-    return false if topic.archived?
-    true
   end
 
   # Editing Methods
@@ -255,20 +221,15 @@ class Guardian
   end
 
   def can_edit_post?(post)
-    return true if is_staff?
+    is_staff? || (not(post.topic.archived?) && is_my_own?(post))
-    return false if post.topic.archived?
-    (post.user == @user)
   end
 
   def can_edit_user?(user)
-    return true if user == @user
+    is_me?(user) || is_staff?
-    is_staff?
   end
 
   def can_edit_topic?(topic)
-    return true if is_staff?
+    is_staff? || is_my_own?(topic)
-    return true if topic.user == @user
-    false
   end
 
   # Deleting Methods
@@ -277,92 +238,69 @@ class Guardian
     return false if post.post_number == 1
 
     # You can delete your own posts
-    return !post.user_deleted? if post.user == @user
+    return !post.user_deleted? if is_my_own?(post)
 
     is_staff?
   end
 
   # Recovery Method
   def can_recover_post?(post)
-    return false unless @user
     is_staff?
   end
 
   def can_delete_category?(category)
-    return false unless is_staff?
+    is_staff? && category.topic_count == 0
-    return category.topic_count == 0
   end
 
   def can_delete_topic?(topic)
-    return false unless is_staff?
+    is_staff? && not(Category.exists?(topic_id: topic.id))
-    return false if Category.exists?(topic_id: topic.id)
-    true
   end
 
   def can_delete_post_action?(post_action)
-
     # You can only undo your own actions
-    return false unless @user
+    is_my_own?(post_action) && not(post_action.is_private_message?) &&
-    return false unless post_action.user_id == @user.id
-    return false if post_action.is_private_message?
 
     # Make sure they want to delete it within the window
-    return post_action.created_at > SiteSetting.post_undo_action_window_mins.minutes.ago
+    post_action.created_at > SiteSetting.post_undo_action_window_mins.minutes.ago
   end
 
   def can_send_private_message?(target)
-    return false unless User === target || Group === target
+    (User === target || Group === target) &&
-    return false unless @user
+    authenticated? &&
 
     # Can't send message to yourself
-    return false if User === target && @user.id == target.id
+    is_not_me?(target) &&
 
     # Have to be a basic level at least
-    return false unless @user.has_trust_level?(:basic)
+    @user.has_trust_level?(:basic) &&
 
     SiteSetting.enable_private_messages
   end
 
   def can_reply_as_new_topic?(topic)
-    return false unless @user
+    authenticated? && topic && not(topic.private_message?) && @user.has_trust_level?(:basic)
-    return false unless topic
-    return false if topic.private_message?
-
-    @user.has_trust_level?(:basic)
   end
 
   def can_see_topic?(topic)
-    return false unless topic
+    if topic
+      is_staff? ||
 
-    return true if @user && is_staff?
+      topic.deleted_at.nil? &&
-    return false if topic.deleted_at
 
-    if topic.category && topic.category.secure
+      # not secure, or I can see it
-      return false unless @user && can_see_category?(topic.category)
+      (not(topic.secure_category?) || can_see_category?(topic.category)) &&
+
+      # not private, or I am allowed (or an admin)
+      (not(topic.private_message?) || authenticated? && (topic.all_allowed_users.where(id: @user.id).exists? || is_admin?))
     end
-
-    if topic.private_message?
-      return false unless @user
-      return true if topic.all_allowed_users.where(id: @user.id).exists?
-      return is_admin?
-    end
-    true
   end
 
   def can_see_post?(post)
-    return false unless post
+    post.present? && (is_staff? || (!post.deleted_at.present? && can_see_topic?(post.topic)))
-
-    return true if @user && is_staff?
-    return false if post.deleted_at.present?
-
-    can_see_topic?(post.topic)
   end
 
   def can_see_category?(category)
-    return true unless category.secure
+    not(category.secure) || secure_category_ids.include?(category.id)
-    return false unless @user
-
-    secure_category_ids.include?(category.id)
   end
 
   def can_vote?(post, opts={})
@@ -372,37 +310,62 @@ class Guardian
   # Can the user act on the post in a particular way.
   #  taken_actions = the list of actions the user has already taken
   def post_can_act?(post, action_key, opts={})
-    return false if @user.blank?
-    return false if post.blank?
 
-    taken = opts[:taken_actions]
+    taken = opts[:taken_actions].try(:keys).to_a
-    taken = taken.keys if taken
+    is_flag = PostActionType.is_flag?(action_key)
+    already_taken_this_action = taken.any? && taken.include?(PostActionType.types[action_key])
+    already_did_flagging      = taken.any? && (taken & PostActionType.flag_types.values).any?
 
-    # we always allow flagging
+    if  authenticated? && post
-    if PostActionType.is_flag?(action_key)
+      # we always allow flagging - NOTE: this does not seem true, see specs. (MVH)
-      return false unless @user.has_trust_level?(:basic)
+      (is_flag && @user.has_trust_level?(:basic) && not(already_did_flagging)) ||
 
-      if taken
+      # not a flagging action, and haven't done it already
-        return false unless (taken & PostActionType.flag_types.values).empty?
+      not(is_flag || already_taken_this_action) &&
-      end
+
-    else
+      # nothing except flagging on archived posts
-      return false if taken && taken.include?(PostActionType.types[action_key])
+      not(post.topic.archived?) &&
+
+      # don't like your own stuff
+      not(action_key == :like && is_my_own?(post)) &&
+
+      # no voting more than once on single vote topics
+      not(action_key == :vote && opts[:voted_in_topic] && post.topic.has_meta_data_boolean?(:single_vote))
     end
-
-    # nothing else on archived posts
-    return false if post.topic.archived?
-
-    case action_key
-    when :like
-      return false if post.user == @user
-    when :vote then
-      return false if opts[:voted_in_topic] && post.topic.has_meta_data_boolean?(:single_vote)
-    end
-
-    return true
   end
 
   def secure_category_ids
-    @secure_category_ids ||= @user ? @user.secure_category_ids : []
+    @secure_category_ids ||= @user.secure_category_ids
   end
+
+  private
+
+  def is_my_own?(obj)
+    @user.present? &&
+    (obj.respond_to?(:user) || obj.respond_to?(:user_id)) &&
+    (obj.respond_to?(:user) ? obj.user == @user : true) &&
+    (obj.respond_to?(:user_id) ? (obj.user_id == @user.id) : true)
+  end
+
+  def is_me?(other)
+    other && authenticated? && User === other && @user == other
+  end
+
+  def is_not_me?(other)
+    @user.blank? || !is_me?(other)
+  end
+
+  def can_administer?(obj)
+    is_admin? && obj.present?
+  end
+
+  def can_administer_user?(other_user)
+    can_administer?(other_user) && is_not_me?(other_user)
+  end
+
+  def method_name_for(action, obj)
+    method_name = :"can_#{action}_#{obj.class.name.underscore}?"
+    return method_name if respond_to?(method_name)
+  end
+
 end

spec/components/guardian_spec.rb

@@ -38,6 +38,11 @@ describe Guardian do
       Guardian.new(user).post_can_act?(post, :like).should be_false
     end
 
+    it "always allows flagging" do
+      post.topic.archived = true
+      Guardian.new(user).post_can_act?(post, :spam).should be_true
+    end
+
     it "returns false when liking yourself" do
       Guardian.new(post.user).post_can_act?(post, :like).should be_false
     end
@@ -694,7 +699,7 @@ describe Guardian do
         user.id = 1
         post.id = 1
 
-        a = PostAction.new(user_id: user.id, post_id: post.id, post_action_type_id: 1)
+        a = PostAction.new(user: user, post: post, post_action_type_id: 1)
         a.created_at = 1.minute.ago
         a
       }
@@ -794,7 +799,7 @@ describe Guardian do
       Guardian.new.can_grant_moderation?(user).should be_false
     end
 
-    it "wont allow a regular user to revoke an modearator's access" do
+    it "wont allow a regular user to revoke an moderator's access" do
       Guardian.new(user).can_grant_moderation?(moderator).should be_false
     end
 

spec/models/topic_spec.rb

@@ -1104,4 +1104,21 @@ describe Topic do
     end
   end
 
+  describe '#secure_category?' do
+    let(:category){ Category.new }
+
+    it "is true if the category is secure" do
+      category.stubs(:secure).returns(true)
+      Topic.new(:category => category).should be_secure_category
+    end
+
+    it "is false if the category is not secure" do
+      category.stubs(:secure).returns(false)
+      Topic.new(:category => category).should_not be_secure_category
+    end
+
+    it "is false if there is no category" do
+      Topic.new(:category => nil).should_not be_secure_category
+    end
+  end
 end