Skip to content

Commit

Permalink
drop rbac skip_references
Browse files Browse the repository at this point in the history
  • Loading branch information
@kbrock
kbrock committed Sep 12, 2019
1 parent b3f5682 commit c8cb376
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 142 deletions.
56 changes: 2 additions & 54 deletions lib/rbac/filterer.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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))
Expand Down
97 changes: 9 additions & 88 deletions spec/lib/rbac/filterer_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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

Expand All @@ -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

Expand All @@ -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
Expand Down Expand Up @@ -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])
Expand All @@ -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
Expand Down

0 comments on commit c8cb376

Please sign in to comment.