DEV: remove calls to guardian from GroupActionLogger (#13835)

DEV: remove calls to guardian from GroupActionLogger (#13835)

We shouldn’t be checking if a user is allowed to do an action in the logger. We should be checking it just before we perform the action. In fact, guardians in the logger can make things even worse in case of a security bug. Let’s say we forgot to check user’s permissions before performing some action, but we still have a call to the guardian in the logger. In this case, a user would perform the action anyway, and this action wouldn’t even be logged!

I’ve checked all cases and I confirm that we’re safe to delete this calls from the logger.

I’ve added two calls to guardians in admin/user_controller. We didn’t have security bugs there, because regular users can’t access admin/… routes at all. But it’s good to have calls to guardian in these methods anyway, neighboring methods have them.

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index e25d4d9..2fb4469 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -211,9 +211,10 @@ class Admin::UsersController < Admin::AdminController
 
   def add_group
     group = Group.find(params[:group_id].to_i)
-
     raise Discourse::NotFound unless group
+
     return render_json_error(I18n.t('groups.errors.can_not_modify_automatic')) if group.automatic
+    guardian.ensure_can_edit!(group)
 
     group.add(@user)
     GroupActionLogger.new(current_user, group).log_add_user_to_group(@user)
@@ -223,12 +224,14 @@ class Admin::UsersController < Admin::AdminController
 
   def remove_group
     group = Group.find(params[:group_id].to_i)
-
     raise Discourse::NotFound unless group
+
     return render_json_error(I18n.t('groups.errors.can_not_modify_automatic')) if group.automatic
+    guardian.ensure_can_edit!(group)
 
-    group.remove(@user)
-    GroupActionLogger.new(current_user, group).log_remove_user_from_group(@user)
+    if group.remove(@user)
+      GroupActionLogger.new(current_user, group).log_remove_user_from_group(@user)
+    end
 
     render body: nil
   end
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 440deb4..462d73d 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -170,7 +170,7 @@ class GroupsController < ApplicationController
     end
 
     if group.update(params_with_permitted)
-      GroupActionLogger.new(current_user, group, skip_guardian: true).log_change_group_settings
+      GroupActionLogger.new(current_user, group).log_change_group_settings
       group.record_email_setting_changes!(current_user)
       group.expire_imap_mailbox_cache
       update_existing_users(group.group_users, categories, tags) if categories.present? || tags.present?
diff --git a/app/services/group_action_logger.rb b/app/services/group_action_logger.rb
index 2688ab8..9fb65af 100644
--- a/app/services/group_action_logger.rb
+++ b/app/services/group_action_logger.rb
@@ -2,15 +2,12 @@
 
 class GroupActionLogger
 
-  def initialize(acting_user, group, opts = {})
+  def initialize(acting_user, group)
     @acting_user = acting_user
     @group = group
-    @opts = opts
   end
 
   def log_make_user_group_owner(target_user)
-    can_edit?
-
     GroupHistory.create!(default_params.merge(
       action: GroupHistory.actions[:make_user_group_owner],
       target_user: target_user
@@ -18,8 +15,6 @@ class GroupActionLogger
   end
 
   def log_remove_user_as_group_owner(target_user)
-    can_edit?
-
     GroupHistory.create!(default_params.merge(
       action: GroupHistory.actions[:remove_user_as_group_owner],
       target_user: target_user
@@ -27,8 +22,6 @@ class GroupActionLogger
   end
 
   def log_add_user_to_group(target_user)
-    (target_user == @acting_user && @group.public_admission) || can_edit?
-
     GroupHistory.create!(default_params.merge(
       action: GroupHistory.actions[:add_user_to_group],
       target_user: target_user
@@ -36,8 +29,6 @@ class GroupActionLogger
   end
 
   def log_remove_user_from_group(target_user)
-    (target_user == @acting_user && @group.public_exit) || can_edit?
-
     GroupHistory.create!(default_params.merge(
       action: GroupHistory.actions[:remove_user_from_group],
       target_user: target_user
@@ -45,8 +36,6 @@ class GroupActionLogger
   end
 
   def log_change_group_settings
-    @opts[:skip_guardian] || can_edit?
-
     @group.previous_changes.except(*excluded_attributes).each do |attribute_name, value|
       next if value[0].blank? && value[1].blank?
 
@@ -73,9 +62,4 @@ class GroupActionLogger
   def default_params
     { group: @group, acting_user: @acting_user }
   end
-
-  def can_edit?
-    raise Discourse::InvalidParameters.new unless Guardian.new(@acting_user).can_log_group_changes?(@group)
-  end
-
 end
diff --git a/lib/guardian/group_guardian.rb b/lib/guardian/group_guardian.rb
index 7aa0f9e..36d4efc 100644
--- a/lib/guardian/group_guardian.rb
+++ b/lib/guardian/group_guardian.rb
@@ -16,11 +16,8 @@ module GroupGuardian
   # Automatic groups are not represented in the GROUP_USERS
   # table and thus do not allow membership changes.
   def can_edit_group?(group)
-    !group.automatic && can_log_group_changes?(group)
-  end
-
-  def can_log_group_changes?(group)
-    can_admin_group?(group) || group.users.where('group_users.owner').include?(user)
+    !group.automatic &&
+      (can_admin_group?(group) || group.users.where('group_users.owner').include?(user))
   end
 
   def can_admin_group?(group)
diff --git a/spec/services/group_action_logger_spec.rb b/spec/services/group_action_logger_spec.rb
index 865138e..ccb50ac 100644
--- a/spec/services/group_action_logger_spec.rb
+++ b/spec/services/group_action_logger_spec.rb
@@ -53,27 +53,18 @@ RSpec.describe GroupActionLogger do
     context 'as a normal user' do
       subject { described_class.new(user, group) }
 
-      describe 'user cannot freely exit group' do
-        it 'should not be allowed to log the action' do
-          expect { subject.log_add_user_to_group(user) }
-            .to raise_error(Discourse::InvalidParameters)
-        end
+      before do
+        group.update!(public_admission: true)
       end
 
-      describe 'user can freely exit group' do
-        before do
-          group.update!(public_admission: true)
-        end
-
-        it 'should create the right record' do
-          subject.log_add_user_to_group(user)
+      it 'should create the right record' do
+        subject.log_add_user_to_group(user)
 
-          group_history = GroupHistory.last
+        group_history = GroupHistory.last
 
-          expect(group_history.action).to eq(GroupHistory.actions[:add_user_to_group])
-          expect(group_history.acting_user).to eq(user)
-          expect(group_history.target_user).to eq(user)
-        end
+        expect(group_history.action).to eq(GroupHistory.actions[:add_user_to_group])
+        expect(group_history.acting_user).to eq(user)
+        expect(group_history.target_user).to eq(user)
       end
     end
   end
@@ -94,27 +85,18 @@ RSpec.describe GroupActionLogger do
     context 'as a normal user' do
       subject { described_class.new(user, group) }
 
-      describe 'user cannot freely exit group' do
-        it 'should not be allowed to log the action' do
-          expect { subject.log_remove_user_from_group(user) }
-            .to raise_error(Discourse::InvalidParameters)
-        end
+      before do
+        group.update!(public_exit: true)
       end
 
-      describe 'user can freely exit group' do
-        before do
-          group.update!(public_exit: true)
-        end
-
-        it 'should create the right record' do
-          subject.log_remove_user_from_group(user)
+      it 'should create the right record' do
+        subject.log_remove_user_from_group(user)
 
-          group_history = GroupHistory.last
+        group_history = GroupHistory.last
 
-          expect(group_history.action).to eq(GroupHistory.actions[:remove_user_from_group])
-          expect(group_history.acting_user).to eq(user)
-          expect(group_history.target_user).to eq(user)
-        end
+        expect(group_history.action).to eq(GroupHistory.actions[:remove_user_from_group])
+        expect(group_history.acting_user).to eq(user)
+        expect(group_history.target_user).to eq(user)
       end
     end
   end

GitHub sha: 5a2ad7e386ebe1cda41699c27fb9056dc6ad87d5

This commit appears in #13835 which was approved by eviltrout. It was merged by AndrewPrigorshnev.