diff --git a/lib/rbac/filterer.rb b/lib/rbac/filterer.rb index 770a2d46e9c..1206ec4b409 100644 --- a/lib/rbac/filterer.rb +++ b/lib/rbac/filterer.rb @@ -258,7 +258,6 @@ 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?(scope, options, attrs, exp_sql, exp_includes) # 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 @@ -268,7 +267,7 @@ def search(options = {}) .includes(include_for_find).includes(exp_includes) .order(order) - scope = include_references(scope, klass, references, exp_includes, skip_references) + scope = include_references(scope, klass, references, exp_includes) scope = scope.limit(limit).offset(offset) if attrs[:apply_limit_in_sql] if inline_view?(options, scope) @@ -380,58 +379,7 @@ def self.includes_to_references(klass, inc) end end - # 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. - # - # Also, if for whatever reason we are passed a - # `ActiveRecord::NullRelation`, make sure that we don't skip references. - # This will cause the EXPLAIN to blow up since `.to_sql` gets changed to - # always return `""`... even though at the end of the day, we will always - # get back zero records from the query. - # - # 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?(scope, options, attrs, exp_sql, exp_includes) - return false if scope.singleton_class.included_modules.include?(ActiveRecord::NullRelation) - options[:skip_references] || - (options[:extra_cols].blank? && - !options.key?(:references) && - (!attrs[:apply_limit_in_sql] && (exp_sql.nil? || exp_includes.nil?))) - 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 - + def include_references(scope, klass, include_for_find, exp_includes) ref_includes = Hash(include_for_find).merge(Hash(exp_includes)) unless polymorphic_include?(klass, ref_includes) scope = scope.references(self.class.includes_to_references(klass, include_for_find)).references(self.class.includes_to_references(klass, exp_includes)) diff --git a/spec/lib/rbac/filterer_spec.rb b/spec/lib/rbac/filterer_spec.rb index 34ad9ba955d..144f417c9bb 100644 --- a/spec/lib/rbac/filterer_spec.rb +++ b/spec/lib/rbac/filterer_spec.rb @@ -511,7 +511,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt end it "does not add references without includes" do - expect(subject).to receive(:include_references).with(anything, Vm, nil, nil, true).and_call_original + expect(subject).to receive(:include_references).with(anything, Vm, nil, nil).and_call_original results end @@ -525,7 +525,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt end it "includes references" do - expect(subject).to receive(:include_references).with(anything, ::Vm, nil, {:host => {}}, false) + expect(subject).to receive(:include_references).with(anything, ::Vm, nil, {:host => {}}) .and_call_original expect(subject).to receive(:warn).never results @@ -542,7 +542,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt 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 + expect(subject).to receive(:include_references).with(anything, Vm, {:evm_owner => {}}, nil).and_call_original results end @@ -551,8 +551,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt 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 + expect(subject).to receive(:include_references).with(anything, Vm, {:evm_owner => {}}, nil).and_call_original results end @@ -562,8 +561,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt let(:results) { subject.search(null_search).first } it "will not try to skip references" do - expect(subject).to receive(:include_references).with(anything, Vm, {:evm_owner => {}}, nil, false).and_call_original - expect(subject).to receive(:warn).never + expect(subject).to receive(:include_references).with(anything, Vm, {:evm_owner => {}}, nil).and_call_original results end end @@ -2406,28 +2404,27 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects) 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] + method_args = [scope, klass, include_for_find, nil] resulting_scope = subject.send(:include_references, *method_args) expect(resulting_scope.references_values).to eq(%w[miq_servers]) end it "adds exp_includes .references to the scope" do - method_args = [scope, klass, nil, exp_includes, skip] + method_args = [scope, klass, nil, exp_includes] resulting_scope = subject.send(:include_references, *method_args) expect(resulting_scope.references_values).to eq(%w[hosts]) end it "adds include_for_find and exp_includes .references to the scope" do - method_args = [scope, klass, include_for_find, exp_includes, skip] + method_args = [scope, klass, include_for_find, exp_includes] resulting_scope = subject.send(:include_references, *method_args) expect(resulting_scope.references_values).to eq(%w[miq_servers hosts]) @@ -2438,88 +2435,12 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects) let(:include_for_find) { { :resource => {} } } it "does not add .references to the scope" do - method_args = [scope, klass, include_for_find, nil, skip] + method_args = [scope, klass, include_for_find, nil] 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(%w[miq_servers hosts]) - 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 describe "using right RBAC rules to VM and Templates" do