IntenseWebs/discourse
3
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2025-02-25 18:55:32 -06:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
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.
This commit is contained in:
Andrei Prigorshnev
2021-07-28 15:04:04 +04:00
committed by GitHub
parent 32951ca2f4
commit 5a2ad7e386
5 changed files with 28 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
app/services/group_action_logger.rb
View File

@@ -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