From 270f30e2cef7099f29271c4ca80a6e9a6f98e3d9 Mon Sep 17 00:00:00 2001 From: Joe Rafaniello Date: Mon, 9 Apr 2018 16:23:21 -0400 Subject: [PATCH] Merge pull request #17141 from NickLaMuro/optional_report_references Skip query references in Rbac when not needed (cherry picked from commit 05ab3d0c7be3206541d52a2b950b0623b48f1939) https://bugzilla.redhat.com/show_bug.cgi?id=1565678 --- app/models/cloud_tenant.rb | 3 +- app/models/flavor.rb | 3 +- app/models/mixins/cloud_tenancy_mixin.rb | 8 +- lib/rbac/filterer.rb | 46 +++- spec/lib/rbac/filterer_spec.rb | 239 +++++++++++++++++- .../models/mixins/cloud_tenancy_mixin_spec.rb | 25 ++ 6 files changed, 313 insertions(+), 11 deletions(-) diff --git a/app/models/cloud_tenant.rb b/app/models/cloud_tenant.rb index 417af2ad0a2b..29f10f9dec78 100644 --- a/app/models/cloud_tenant.rb +++ b/app/models/cloud_tenant.rb @@ -162,6 +162,7 @@ def self.post_refresh_ems(ems_id, _) end def self.tenant_joins_clause(scope) - scope.includes(:source_tenant).includes(:ext_management_system) + scope.includes(:source_tenant, :ext_management_system) + .references(:source_tenant, :ext_management_system) end end diff --git a/app/models/flavor.rb b/app/models/flavor.rb index bf37910f2fd4..fa93e8d845fd 100644 --- a/app/models/flavor.rb +++ b/app/models/flavor.rb @@ -36,6 +36,7 @@ def name_with_details end def self.tenant_joins_clause(scope) - scope.includes(:cloud_tenants => "source_tenant").includes(:ext_management_system) + scope.includes(:cloud_tenants => "source_tenant", :ext_management_system => {}) + .references(:cloud_tenants => "source_tenant", :ext_management_system => {}) end end diff --git a/app/models/mixins/cloud_tenancy_mixin.rb b/app/models/mixins/cloud_tenancy_mixin.rb index 75313422f477..e57306b6699f 100644 --- a/app/models/mixins/cloud_tenancy_mixin.rb +++ b/app/models/mixins/cloud_tenancy_mixin.rb @@ -1,6 +1,11 @@ module CloudTenancyMixin extend ActiveSupport::Concern + QUERY_REFERENCES = { + :cloud_tenant => "source_tenant", + :ext_management_system => {} + }.freeze + module ClassMethods include TenancyCommonMixin @@ -13,7 +18,8 @@ def tenant_id_clause_format(tenant_ids) end def tenant_joins_clause(scope) - scope.includes(:cloud_tenant => "source_tenant").includes(:ext_management_system) + scope.includes(QUERY_REFERENCES) + .references(QUERY_REFERENCES) # needed for the where to work end end end diff --git a/lib/rbac/filterer.rb b/lib/rbac/filterer.rb index dd642ec0a771..d8134fb84a3c 100644 --- a/lib/rbac/filterer.rb +++ b/lib/rbac/filterer.rb @@ -244,6 +244,7 @@ def search(options = {}) exp_sql, exp_includes, exp_attrs = search_filter.to_sql(tz) if search_filter && !klass.try(:instances_are_derived?) attrs[:apply_limit_in_sql] = (exp_attrs.nil? || exp_attrs[:supported_by_sql]) && user_filters["belongsto"].blank? + skip_references = skip_references?(options, attrs) # for belongs_to filters, scope_targets uses scope to make queries. want to remove limits for those. # if you note, the limits are put back into scope a few lines down from here @@ -253,7 +254,7 @@ def search(options = {}) .includes(include_for_find).includes(exp_includes) .order(order) - scope = include_references(scope, klass, include_for_find, exp_includes) + scope = include_references(scope, klass, include_for_find, exp_includes, skip_references) scope = scope.limit(limit).offset(offset) if attrs[:apply_limit_in_sql] targets = scope @@ -287,7 +288,48 @@ def is_sti?(klass) klass.respond_to?(:finder_needs_type_condition?) ? klass.finder_needs_type_condition? : false end - def include_references(scope, klass, include_for_find, exp_includes) + # This is a very primitive way of determining whether we want to skip + # adding references to the query. + # + # For now, basically it checks if the caller has not provided :extra_cols, + # or if the MiqExpression can't apply the limit in SQL. If both of those + # are true, then we don't add `.references` to the scope. + # + # If still invalid, there is an EXPLAIN check in #include_references that + # will make sure the query is valid and if not, will include the references + # as done previously. + def skip_references?(options, attrs) + options[:extra_cols].blank? && !attrs[:apply_limit_in_sql] + end + + def include_references(scope, klass, include_for_find, exp_includes, skip) + if skip + # If we are in a transaction, we don't want to polute that + # transaction with a failed EXPLAIN. We use a SQL SAVEPOINT (which is + # created via `transaction(:requires_new => true)`) to prevent that + # from being an issue (happens in tests with transactional fixtures) + # + # See https://stackoverflow.com/a/31146267/3574689 + valid_skip = MiqDatabase.transaction(:requires_new => true) do + begin + ActiveRecord::Base.connection.explain(scope.to_sql) + rescue ActiveRecord::StatementInvalid => e + unless Rails.env.production? + warn "There was an issue with the Rbac filter without references!" + warn "Consider trying to fix this edge case in Rbac::Filterer! Error Below:" + warn e.message + warn e.backtrace + end + # returns nil + raise ActiveRecord::Rollback + end + end + # If the result of the transaction is non-nil, then the block was + # successful and didn't trigger the ActiveRecord::Rollback, so we can + # return the scope as is. + return scope if valid_skip + end + ref_includes = Hash(include_for_find).merge(Hash(exp_includes)) unless polymorphic_include?(klass, ref_includes) scope = scope.references(include_for_find).references(exp_includes) diff --git a/spec/lib/rbac/filterer_spec.rb b/spec/lib/rbac/filterer_spec.rb index 06b1b7808333..e75ed9f0b32a 100644 --- a/spec/lib/rbac/filterer_spec.rb +++ b/spec/lib/rbac/filterer_spec.rb @@ -167,6 +167,120 @@ end end + context "with non-sql filter" do + subject { described_class.new } + + let(:expression) { MiqExpression.new("=" => {"field" => "Vm-vendor_display", "value" => "VMware"}) } + let(:search_attributes) { { :class => "Vm", :filter => expression } } + let(:results) { subject.search(search_attributes).first } + + before { [owned_vm, other_vm] } + + it "finds the Vms" do + expect(results.to_a).to match_array [owned_vm, other_vm] + expect(results.count).to eq 2 + end + + it "does not add references without includes" do + expect(subject).to receive(:include_references).with(anything, Vm, nil, nil, true).and_call_original + results + end + + context "with :include_for_find" do + let(:include_search) { search_attributes.merge(:include_for_find => {:evm_owner => {}}) } + let(:results) { subject.search(include_search).first } + + it "finds the Vms" do + expect(results.to_a).to match_array [owned_vm, other_vm] + expect(results.count).to eq 2 + end + + it "does not add references since there isn't a SQL filter" do + expect(subject).to receive(:include_references).with(anything, Vm, {:evm_owner => {}}, nil, true).and_call_original + results + end + + context "with a references based where_clause" do + let(:search_with_where) { include_search.merge(:where_clause => ['"users"."id" = ?', owner_user.id]) } + let(:results) { subject.search(search_with_where).first } + + it "will try to skip references to begin with" do + expect(subject).to receive(:include_references).with(anything, Vm, {:evm_owner => {}}, nil, true).and_call_original + expect(subject).to receive(:warn).exactly(4).times + results + end + end + end + end + + context "with a miq_expression filter on vms" do + let(:expression) { MiqExpression.new("=" => {"field" => "Vm-vendor", "value" => "vmware"}) } + let(:search_attributes) { { :class => "Vm", :filter => expression } } + let(:results) { described_class.search(search_attributes).first } + + before { [owned_vm, other_vm] } + + it "finds the Vms" do + expect(results.to_a).to match_array [owned_vm, other_vm] + expect(results.count).to eq 2 + end + + it "does not add references without includes" do + # empty string here is basically passing `.references(nil)`, and the + # extra empty hash here is from the MiqExpression (which will result in + # the same), both of which will no-op to when determining if there are + # joins in ActiveRecord, and will not create a JoinDependency query + expect(results.references_values).to match_array ["", "{}"] + end + + context "with :include_for_find" do + let(:include_search) { search_attributes.merge(:include_for_find => {:evm_owner => {}}) } + let(:results) { described_class.search(include_search).first } + + it "finds the Service" do + expect(results.to_a).to match_array [owned_vm, other_vm] + expect(results.count).to eq 2 + end + + it "adds references" do + expect(results.references_values).to match_array ["{:evm_owner=>{}}", "{}"] + end + end + end + + context "with :extra_cols on a Service" do + let(:extra_cols) { [:owned_by_current_user] } + let(:search_attributes) { { :class => "Service", :extra_cols => extra_cols } } + let(:results) { described_class.search(search_attributes).first } + + before { FactoryGirl.create :service, :evm_owner => owner_user } + + it "finds the Service" do + expect(results.first.attributes["owned_by_current_user"]).to be false + end + + it "does not add references with no includes" do + # The single empty string is the result of a nil from both the lack of + # a MiqExpression filter and the user filter, which is deduped in + # ActiveRecord's internals and results in a `.references(nil)` + # effectively + expect(results.references_values).to match_array [""] + end + + context "with :include_for_find" do + let(:include_search) { search_attributes.merge(:include_for_find => {:evm_owner => {}}) } + let(:results) { described_class.search(include_search).first } + + it "finds the Service" do + expect(results.first.attributes["owned_by_current_user"]).to be false + end + + it "adds references" do + expect(results.references_values).to match_array ["", "{:evm_owner=>{}}"] + end + end + end + describe "with find_options_for_tenant filtering" do before do owned_vm # happy path @@ -200,12 +314,6 @@ expect(results).to match_array [owned_vm] end - it "with :extra_cols finds Service" do - FactoryGirl.create :service, :evm_owner => owner_user - results = described_class.search(:class => "Service", :extra_cols => [:owned_by_current_user]).first - expect(results.first.attributes["owned_by_current_user"]).to be false - end - it "leaving tenant doesnt find Vm" do owner_user.update_attributes(:miq_groups => [other_user.current_group]) User.with_user(owner_user) do @@ -1612,6 +1720,125 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects) end end + describe "#include_references (private)" do + subject { described_class.new } + + let(:skip) { false } + let(:klass) { VmOrTemplate } + let(:scope) { klass.all } + let(:include_for_find) { { :miq_server => {} } } + let(:exp_includes) { { :host => {} } } + + it "adds include_for_find .references to the scope" do + method_args = [scope, klass, include_for_find, nil, skip] + resulting_scope = subject.send(:include_references, *method_args) + + expect(resulting_scope.references_values).to eq(["{:miq_server=>{}}", ""]) + end + + it "adds exp_includes .references to the scope" do + method_args = [scope, klass, nil, exp_includes, skip] + resulting_scope = subject.send(:include_references, *method_args) + + expect(resulting_scope.references_values).to eq(["", "{:host=>{}}"]) + end + + it "adds include_for_find and exp_includes .references to the scope" do + method_args = [scope, klass, include_for_find, exp_includes, skip] + resulting_scope = subject.send(:include_references, *method_args) + + expect(resulting_scope.references_values).to eq(["{:miq_server=>{}}", "{:host=>{}}"]) + end + + context "if the include is polymorphic" do + let(:klass) { MetricRollup } + let(:include_for_find) { { :resource => {} } } + + it "does not add .references to the scope" do + method_args = [scope, klass, include_for_find, nil, skip] + resulting_scope = subject.send(:include_references, *method_args) + + expect(resulting_scope.references_values).to eq([]) + end + end + + context "when skip is passed as true" do + let(:skip) { true } + + it "does not add .references to the scope" do + method_args = [scope, klass, include_for_find, exp_includes, skip] + resulting_scope = subject.send(:include_references, *method_args) + + expect(resulting_scope.references_values).to eq([]) + end + + context "when the scope is invalid without .references" do + let(:scope) { klass.where("hosts.name = 'foo'") } + let(:method_args) { [scope, klass, include_for_find, exp_includes, skip] } + let(:resulting_scope) { subject.send(:include_references, *method_args) } + + let(:explain_error_match) do + Regexp.new(Regexp.escape(<<~PG_ERR.chomp)) + PG::UndefinedTable: ERROR: missing FROM-clause entry for table "hosts" + LINE 1: EXPLAIN SELECT "vms".* FROM "vms" WHERE (hosts.name = 'foo') + ^ + : EXPLAIN SELECT "vms".* FROM "vms" WHERE (hosts.name = 'foo') + PG_ERR + end + + it "adds .references to the scope" do + allow(subject).to receive(:warn) + expect(resulting_scope.references_values).to eq(["{:miq_server=>{}}", "{:host=>{}}"]) + end + + it "warns that there was an issue in test mode" do + # This next couple of lines is just used to check that some of the + # backtrace that we are dumping into the logs is what we expect will + # for sure be there, and not try to match the entire trace. + # + # Does a bit of line addition to avoid this being too brittle and + # breaking easily, but expect it to break if you update + # Rbac::Filterer#include_references + method_file, method_line = subject.method(:include_references).source_location + explain_stacktrace_includes = [ + "#{method_file}:#{method_line + 10}:in `block in include_references'", + Thread.current.backtrace[1].gsub(/:\d*:/) { |sub| ":#{sub.tr(":", "").to_i + 7}:" } + ] + + expect(subject).to receive(:warn).with("There was an issue with the Rbac filter without references!").ordered + expect(subject).to receive(:warn).with("Consider trying to fix this edge case in Rbac::Filterer! Error Below:").ordered + expect(subject).to receive(:warn).with(explain_error_match).ordered + expect(subject).to receive(:warn).with(array_including(explain_stacktrace_includes)).ordered + resulting_scope + end + + it "warns that there was an issue in development mode" do + expect(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new("developement")) + + # See above + method_file, method_line = subject.method(:include_references).source_location + explain_stacktrace_includes = [ + "#{method_file}:#{method_line + 10}:in `block in include_references'", + Thread.current.backtrace[1].gsub(/:\d*:/) { |sub| ":#{sub.tr(":", "").to_i + 7}:" } + ] + + expect(subject).to receive(:warn).with("There was an issue with the Rbac filter without references!").ordered + expect(subject).to receive(:warn).with("Consider trying to fix this edge case in Rbac::Filterer! Error Below:").ordered + expect(subject).to receive(:warn).with(explain_error_match).ordered + expect(subject).to receive(:warn).with(array_including(explain_stacktrace_includes)).ordered + resulting_scope + end + + it "does not warn that there was an issue in production mode" do + expect(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new("production")) + + expect(subject).to receive(:warn).never + resulting_scope + end + end + end + end + it ".apply_rbac_directly?" do expect(described_class.new.send(:apply_rbac_directly?, Vm)).to be_truthy expect(described_class.new.send(:apply_rbac_directly?, Rbac)).not_to be diff --git a/spec/models/mixins/cloud_tenancy_mixin_spec.rb b/spec/models/mixins/cloud_tenancy_mixin_spec.rb index 349c74135770..9026ffe411dc 100644 --- a/spec/models/mixins/cloud_tenancy_mixin_spec.rb +++ b/spec/models/mixins/cloud_tenancy_mixin_spec.rb @@ -16,10 +16,35 @@ it "finds correct tenant id clause for regular tenants" do expect(VmOrTemplate.tenant_id_clause(user)).to eql ["(vms.template = true AND vms.tenant_id IN (?)) OR (vms.template = false AND vms.tenant_id IN (?))", [default_tenant.id, tenant.id], [tenant.id]] + + query = VmOrTemplate.where(VmOrTemplate.tenant_id_clause(user)) + expect { query.load }.not_to raise_error end it "finds correct tenant id clause for cloud tenants" do expect(CloudVolume.tenant_id_clause(user)).to eql ["(tenants.id IN (?) AND ext_management_systems.tenant_mapping_enabled IS TRUE) OR ext_management_systems.tenant_mapping_enabled IS FALSE OR ext_management_systems.tenant_mapping_enabled IS NULL", [tenant.id]] + + query = CloudVolume.tenant_joins_clause(CloudVolume.all) + .where(CloudVolume.tenant_id_clause(user)) + expect { query.load }.not_to raise_error + end + + # Overwrites CloudTenancyMixin.tenant_joins_clause + it "finds correct tenant id clause for flavors" do + expect(Flavor.tenant_id_clause(user)).to eql ["(tenants.id IN (?) AND ext_management_systems.tenant_mapping_enabled IS TRUE) OR ext_management_systems.tenant_mapping_enabled IS FALSE OR ext_management_systems.tenant_mapping_enabled IS NULL", [tenant.id]] + + query = Flavor.tenant_joins_clause(Flavor.all) + .where(Flavor.tenant_id_clause(user)) + expect { query.load }.not_to raise_error + end + + # Overwrites CloudTenancyMixin.tenant_joins_clause + it "finds correct tenant id clause for cloud tenants" do + expect(CloudTenant.tenant_id_clause(user)).to eql ["(tenants.id IN (?) AND ext_management_systems.tenant_mapping_enabled IS TRUE) OR ext_management_systems.tenant_mapping_enabled IS FALSE OR ext_management_systems.tenant_mapping_enabled IS NULL", [tenant.id]] + + query = CloudTenant.tenant_joins_clause(CloudTenant.all) + .where(CloudTenant.tenant_id_clause(user)) + expect { query.load }.not_to raise_error end end end