fix: with_role_excluding has bad subquery #17930
Conversation
app/models/miq_group.rb
Outdated
| @@ -61,7 +61,7 @@ def settings=(new_settings) | |||
| end | |||
|
|
|||
| def self.with_roles_excluding(identifier) | |||
| where.not(:id => MiqGroup.joins(:miq_product_features) | |||
| where.not(:id => MiqGroup.unscoped.joins(:miq_product_features) | |||
There was a problem hiding this comment.
Should this be scoped to in_my_region?
app/models/user.rb
Outdated
|
|
||
| def self.with_roles_excluding(identifier) | ||
| where.not(:id => User.joins(:miq_groups => :miq_product_features) | ||
| where.not(:id => User.unscoped.joins(:miq_groups => :miq_product_features) |
spec/models/miq_group_spec.rb
Outdated
| b = FactoryGirl.create(:miq_group, :features => %w(good everything), :description => "combo") | ||
| c = FactoryGirl.create(:miq_group, :features => "everything", :description => "admins") | ||
|
|
||
| expect(MiqGroup.select(:id, :description).with_roles_excluding("everything")).to include(a) |
There was a problem hiding this comment.
Should this be a single expectation using match_array?
There was a problem hiding this comment.
some other groups were coming back - I'll double check.
spec/models/miq_user_role_spec.rb
Outdated
| b = FactoryGirl.create(:miq_user_role, :features => %w(good everything), :name => "combo") | ||
| c = FactoryGirl.create(:miq_user_role, :features => "everything", :name => "admins") | ||
|
|
||
| expect(MiqUserRole.select(:id, :name).with_roles_excluding("everything")).to include(a) |
spec/models/user_spec.rb
Outdated
| u3 = FactoryGirl.create(:user, :miq_groups => [a1, b]) # nope | ||
| u4 = FactoryGirl.create(:user, :miq_groups => [c]) # nope | ||
|
|
||
| expect(User.with_roles_excluding("everything").select(:id, :name)).to include(u1, u2) |
spec/models/user_spec.rb
Outdated
|
|
||
| describe ".with_roles_excluding" do | ||
| it "handles multiple columns" do | ||
| a1 = FactoryGirl.create(:miq_group, :features => "good", :description => "simple") |
There was a problem hiding this comment.
description is sequenced in the factory, maybe those would be better as variable names?
There was a problem hiding this comment.
I can remove. I just had them in for when I was debugging
app/models/miq_group.rb
Outdated
| @@ -61,7 +61,7 @@ def settings=(new_settings) | |||
| end | |||
|
|
|||
| def self.with_roles_excluding(identifier) | |||
| where.not(:id => MiqGroup.joins(:miq_product_features) | |||
| where.not(:id => MiqGroup.unscoped.joins(:miq_product_features) | |||
There was a problem hiding this comment.
How do we know we want to remove the scopes here unconditionally?
There was a problem hiding this comment.
So this is saying, "bring back all groups except those associated with the product feature of 'everything'"
@bdunne very good question. That had slipped my mind.
In this case, I think it is best to not have in_my_region.
This filter is removing entries. If a user asked for all users in all regions, we would want to filter out the groups with high privileged features in other regions as well.
If we add that scope we would have ALL_GROUPS_ALL_REGIONS - SUPER_GROUPS_MY_REGION - Mis-displaying SUPER_GROUPS_OTHER_REGIONS
though this is probably a stretch case
There was a problem hiding this comment.
@jrafanie ok - I changed to only remove the select portion since that is what was blowing up
|
@kbrock Wow, I'll need to review this more but I think I follow what happens. I don't know how we know we want to unscope in all cases. |
kbrock
force-pushed
the
bz_1623464
branch
2 times, most recently
from
b532c0e to
3edea92
Compare
spec/models/user_spec.rb
Outdated
|
|
||
| describe ".with_same_userid" do | ||
| # this is testing the select does not break and in general, the scope works | ||
| it "properly unsopes" do |
Rbac.filtered/search uses this method to filter by tenant admin Unfortunately, the subquery was using the select from the main query and producing bad queries It is now properly isolating the queries and tests have been added to protect against regressions https://bugzilla.redhat.com/show_bug.cgi?id=1623464
|
Checked commit kbrock@9a9b5dd with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
@jrafanie thanks for pointing out typo - this good for you now? |
|
LGTM, please add backport labels... @kbrock how confident are you for backporting this? |
Rbac.filteredandsearchuseswith_role_excludingto filter for non tenant_admin users.Unfortunately, the subquery was using the select from the main query and
was blowing up.
It is now properly isolating the queries and tests have been added to
protect against regressions.
A similar issue exists for
User.with_same_userid. That was corrected here as well.Thanks @yrudman for the teamwork.
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1623464