Merge pull request #4631 from tgxworld/prevent_users_from_changing_permissions_of_non_real_users

FIX: Do not allow admins to meddle with admin and moderation access o…

lib/guardian.rb

@@ -177,7 +177,7 @@ class Guardian
   end
 
   def can_grant_admin?(user)
-    can_administer_user?(user) && not(user.admin?)
+    can_administer_user?(user) && !user.admin?
   end
 
   def can_revoke_moderation?(moderator)
@@ -185,7 +185,7 @@ class Guardian
   end
 
   def can_grant_moderation?(user)
-    can_administer?(user) && not(user.moderator?)
+    can_administer?(user) && !user.moderator?
   end
 
   def can_grant_title?(user)
@@ -313,7 +313,7 @@ class Guardian
   end
 
   def can_administer?(obj)
-    is_admin? && obj.present?
+    is_admin? && obj.present? && obj.id&.positive?
   end
 
   def can_administer_user?(other_user)

spec/components/guardian_spec.rb

@@ -4,9 +4,9 @@ require_dependency 'post_destroyer'
 
 describe Guardian do
 
-  let(:user) { build(:user) }
-  let(:moderator) { build(:moderator) }
-  let(:admin) { build(:admin) }
+  let(:user) { Fabricate(:user) }
+  let(:moderator) { Fabricate(:moderator) }
+  let(:admin) { Fabricate(:admin) }
   let(:trust_level_2) { build(:user, trust_level: 2) }
   let(:trust_level_3) { build(:user, trust_level: 3) }
   let(:trust_level_4)  { build(:user, trust_level: 4) }
@@ -1558,6 +1558,11 @@ describe Guardian do
       user.id = 2
       expect(Guardian.new(admin).can_grant_admin?(user)).to be_truthy
     end
+
+    it 'should not allow an admin to grant admin access to a non real user' do
+      Discourse.system_user.update!(admin: false)
+      expect(Guardian.new(admin).can_grant_admin?(Discourse.system_user)).to be(false)
+    end
   end
 
   context 'can_revoke_admin?' do
@@ -1579,6 +1584,11 @@ describe Guardian do
 
       expect(Guardian.new(admin).can_revoke_admin?(another_admin)).to be_truthy
     end
+
+    it "should not allow an admin to revoke a no real user's admin access" do
+      Discourse.system_user.update!(admin: true)
+      expect(Guardian.new(admin).can_revoke_admin?(Discourse.system_user)).to be(false)
+    end
   end
 
   context 'can_grant_moderation?' do
@@ -1602,6 +1612,11 @@ describe Guardian do
     it "allows an admin to grant a regular user access" do
       expect(Guardian.new(admin).can_grant_moderation?(user)).to be_truthy
     end
+
+    it "should not allow an admin to grant moderation to a non real user" do
+      Discourse.system_user.update!(moderator: false)
+      expect(Guardian.new(admin).can_grant_moderation?(Discourse.system_user)).to be(false)
+    end
   end
 
   context 'can_revoke_moderation?' do
@@ -1629,6 +1644,11 @@ describe Guardian do
     it "does not allow revoke from non moderators" do
       expect(Guardian.new(admin).can_revoke_moderation?(admin)).to be_falsey
     end
+
+    it "should not allow an admin to revoke moderation from a non real user" do
+      Discourse.system_user.update!(moderator: true)
+      expect(Guardian.new(admin).can_revoke_moderation?(Discourse.system_user)).to be(false)
+    end
   end
 
   context "can_see_invite_details?" do