diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index e25d4d903bd..2fb44695b40 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 - group.remove(@user) - GroupActionLogger.new(current_user, group).log_remove_user_from_group(@user) + return render_json_error(I18n.t('groups.errors.can_not_modify_automatic')) if group.automatic + guardian.ensure_can_edit!(group) + + 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 440deb4894d..462d73d4117 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 2688ab85e91..9fb65afbdec 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 7aa0f9ebfcc..36d4efc4cc4 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 865138e87ff..ccb50ac5f31 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