From d7a37e8ff374e040fc946cc6246f038baa18823a Mon Sep 17 00:00:00 2001 From: Yuri Rudman Date: Thu, 26 Apr 2018 09:08:38 -0400 Subject: [PATCH 1/3] do not change current_group for super admin user when executing Rbac#lookup_user_group Example when updating user.current_group in group's look-up is bad: if widget set-up for different group than during content generation (triggered manually from UI) the last group will become current group for super user and this may throw unexpectd errors ( like failing ApplicationController.assert_privileges(widget_refresh) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1564986 --- lib/rbac/filterer.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/rbac/filterer.rb b/lib/rbac/filterer.rb index 2e170809eaf..f343627d9a8 100644 --- a/lib/rbac/filterer.rb +++ b/lib/rbac/filterer.rb @@ -573,16 +573,18 @@ def lookup_user_group(user, userid, miq_group, miq_group_id) miq_group_id ||= miq_group.try!(:id) return [user, user.current_group] if user && user.current_group_id.to_s == miq_group_id.to_s + found_group = user.try(:current_group) if user if miq_group_id && (detected_group = user.miq_groups.detect { |g| g.id.to_s == miq_group_id.to_s }) user.current_group = detected_group + found_group = detected_group elsif miq_group_id && user.super_admin_user? - user.current_group = miq_group || MiqGroup.find_by(:id => miq_group_id) + found_group = miq_group || MiqGroup.find_by(:id => miq_group_id) end else - miq_group ||= miq_group_id && MiqGroup.find_by(:id => miq_group_id) + found_group = miq_group || (miq_group_id && MiqGroup.find_by(:id => miq_group_id)) end - [user, user.try(:current_group) || miq_group] + [user, found_group] end # for reports, user is currently nil, so use the group filter From 911af7e52a2a6f4f69578d0dad8254c2e25bb5df Mon Sep 17 00:00:00 2001 From: Yuri Rudman Date: Thu, 26 Apr 2018 11:18:15 -0400 Subject: [PATCH 2/3] added rspec to check that use.currentuser is not updated if user is superadmin --- spec/lib/rbac/filterer_spec.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/lib/rbac/filterer_spec.rb b/spec/lib/rbac/filterer_spec.rb index 3f97479d19c..85b6a4c6435 100644 --- a/spec/lib/rbac/filterer_spec.rb +++ b/spec/lib/rbac/filterer_spec.rb @@ -2151,6 +2151,14 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects) _, group = filter.send(:lookup_user_group, admin, nil, nil, random_group.id) expect(group).to eq(random_group) end + + it "does not update user.current_group if user is super admin" do + admin = FactoryGirl.create(:user_admin) + admin_group = admin.current_group + random_group = FactoryGirl.create(:miq_group) + filter.send(:lookup_user_group, admin, nil, nil, random_group.id) + expect(admin.current_group).to eq(admin_group) + end end context "user" do From c798fc501270ec7523ff1cd0a0f504ea49ef06df Mon Sep 17 00:00:00 2001 From: Yuri Rudman Date: Tue, 1 May 2018 08:26:08 -0400 Subject: [PATCH 3/3] litle bit of DRY --- lib/rbac/filterer.rb | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/rbac/filterer.rb b/lib/rbac/filterer.rb index f343627d9a8..442fc2ea085 100644 --- a/lib/rbac/filterer.rb +++ b/lib/rbac/filterer.rb @@ -573,18 +573,18 @@ def lookup_user_group(user, userid, miq_group, miq_group_id) miq_group_id ||= miq_group.try!(:id) return [user, user.current_group] if user && user.current_group_id.to_s == miq_group_id.to_s - found_group = user.try(:current_group) - if user - if miq_group_id && (detected_group = user.miq_groups.detect { |g| g.id.to_s == miq_group_id.to_s }) - user.current_group = detected_group - found_group = detected_group - elsif miq_group_id && user.super_admin_user? - found_group = miq_group || MiqGroup.find_by(:id => miq_group_id) - end - else - found_group = miq_group || (miq_group_id && MiqGroup.find_by(:id => miq_group_id)) - end - [user, found_group] + group = if user + if miq_group_id && (detected_group = user.miq_groups.detect { |g| g.id.to_s == miq_group_id.to_s }) + user.current_group = detected_group + elsif miq_group_id && user.super_admin_user? + miq_group || MiqGroup.find_by(:id => miq_group_id) + else + user.try(:current_group) + end + else + miq_group || (miq_group_id && MiqGroup.find_by(:id => miq_group_id)) + end + [user, group] end # for reports, user is currently nil, so use the group filter