diff --git a/lib/rbac/filterer.rb b/lib/rbac/filterer.rb index 1d5d89dab504..86232afa07c0 100644 --- a/lib/rbac/filterer.rb +++ b/lib/rbac/filterer.rb @@ -307,7 +307,33 @@ def skip_references?(options, attrs) end def include_references(scope, klass, include_for_find, exp_includes, skip) - return scope if 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 cad0a7032312..d4f73040f0de 100644 --- a/spec/lib/rbac/filterer_spec.rb +++ b/spec/lib/rbac/filterer_spec.rb @@ -361,6 +361,17 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt 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 @@ -1969,6 +1980,71 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects) 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