mirror of
https://github.com/discourse/discourse.git
synced 2024-12-12 23:26:17 +08:00
5a2ad7e386
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.
120 lines
3.5 KiB
Ruby
120 lines
3.5 KiB
Ruby
# frozen_string_literal: true
|
|
|
|
require 'rails_helper'
|
|
|
|
RSpec.describe GroupActionLogger do
|
|
fab!(:group_owner) { Fabricate(:user) }
|
|
fab!(:group) { Fabricate(:group) }
|
|
fab!(:user) { Fabricate(:user) }
|
|
|
|
subject { described_class.new(group_owner, group) }
|
|
|
|
before do
|
|
group.add_owner(group_owner)
|
|
end
|
|
|
|
describe '#log_make_user_group_owner' do
|
|
it 'should create the right record' do
|
|
subject.log_make_user_group_owner(user)
|
|
|
|
group_history = GroupHistory.last
|
|
|
|
expect(group_history.action).to eq(GroupHistory.actions[:make_user_group_owner])
|
|
expect(group_history.acting_user).to eq(group_owner)
|
|
expect(group_history.target_user).to eq(user)
|
|
end
|
|
end
|
|
|
|
describe '#log_remove_user_as_group_owner' do
|
|
it 'should create the right record' do
|
|
subject.log_remove_user_as_group_owner(user)
|
|
|
|
group_history = GroupHistory.last
|
|
|
|
expect(group_history.action).to eq(GroupHistory.actions[:remove_user_as_group_owner])
|
|
expect(group_history.acting_user).to eq(group_owner)
|
|
expect(group_history.target_user).to eq(user)
|
|
end
|
|
end
|
|
|
|
describe '#log_add_user_to_group' do
|
|
describe 'as a group owner' do
|
|
it 'should create the right record' do
|
|
subject.log_add_user_to_group(user)
|
|
|
|
group_history = GroupHistory.last
|
|
|
|
expect(group_history.action).to eq(GroupHistory.actions[:add_user_to_group])
|
|
expect(group_history.acting_user).to eq(group_owner)
|
|
expect(group_history.target_user).to eq(user)
|
|
end
|
|
end
|
|
|
|
context 'as a normal user' do
|
|
subject { described_class.new(user, group) }
|
|
|
|
before do
|
|
group.update!(public_admission: true)
|
|
end
|
|
|
|
it 'should create the right record' do
|
|
subject.log_add_user_to_group(user)
|
|
|
|
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
|
|
end
|
|
end
|
|
|
|
describe '#log_remove_user_from_group' do
|
|
describe 'as group owner' do
|
|
it 'should create the right record' do
|
|
subject.log_remove_user_from_group(user)
|
|
|
|
group_history = GroupHistory.last
|
|
|
|
expect(group_history.action).to eq(GroupHistory.actions[:remove_user_from_group])
|
|
expect(group_history.acting_user).to eq(group_owner)
|
|
expect(group_history.target_user).to eq(user)
|
|
end
|
|
end
|
|
|
|
context 'as a normal user' do
|
|
subject { described_class.new(user, group) }
|
|
|
|
before do
|
|
group.update!(public_exit: true)
|
|
end
|
|
|
|
it 'should create the right record' do
|
|
subject.log_remove_user_from_group(user)
|
|
|
|
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
|
|
end
|
|
end
|
|
|
|
describe '#log_change_group_settings' do
|
|
it 'should create the right record' do
|
|
group.update!(public_admission: true, created_at: Time.zone.now)
|
|
|
|
expect { subject.log_change_group_settings }.to change { GroupHistory.count }.by(1)
|
|
|
|
group_history = GroupHistory.last
|
|
|
|
expect(group_history.action).to eq(GroupHistory.actions[:change_group_setting])
|
|
expect(group_history.acting_user).to eq(group_owner)
|
|
expect(group_history.subject).to eq('public_admission')
|
|
expect(group_history.prev_value).to eq('f')
|
|
expect(group_history.new_value).to eq('t')
|
|
end
|
|
end
|
|
end
|