Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rbac no references array #19318

Merged
merged 1 commit into from
Oct 3, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/models/application_record.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ class ApplicationRecord < ActiveRecord::Base
include ToModelHash

extend ArTableLock
extend ArReferences

# FIXME: UI code - decorator support
if defined?(ManageIQ::Decorators::Engine)
Expand Down
2 changes: 1 addition & 1 deletion app/models/miq_report/generator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ def generate_daily_metric_rollup_results(options = {})
.where(options[:where_clause])
.where(:timestamp => time_range)
.includes(db_includes)
.references(db_includes)
.references(db_klass.includes_to_references(db_includes))
.includes(exp_includes || [])
.limit(options[:limit])
results = Rbac.filtered(results, :class => db,
Expand Down
4 changes: 4 additions & 0 deletions lib/acts_as_ar_model.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ def self.base_class
superclass == ActsAsArModel ? self : superclass.base_class
end

def self.includes_to_references(_inc)
[]
end

class << self; alias_method :base_model, :base_class; end

#
Expand Down
21 changes: 21 additions & 0 deletions lib/extensions/ar_references.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
module ArReferences
# Given a nested hash of associations (used by includes)
# convert into an array of table names (used by references)
# If given an array of table names, will output the same array
def includes_to_references(inc)
return [] unless inc

inc = Array(inc) unless inc.kind_of?(Hash)
inc.flat_map do |n, v|
if (ref = reflect_on_association(n.to_sym)) && !ref.polymorphic?
n_table = ref.table_name
v_tables = v ? ref.klass.try(:includes_to_references, v) : []
[n_table] + v_tables
elsif reflection_with_virtual(n.to_sym) # ignore polymorphic and virtual attribute
[]
else # it is probably a table name - keep it
n
end
end
end
end
4 changes: 2 additions & 2 deletions lib/rbac/filterer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ def self.accessible_tenant_ids_strategy(klass)
# @option options :conditions [Hash|String|Array<String>]
# @option options :where_clause []
# @option options :sub_filter
# @option options :include_for_find [Array<Symbol>]
# @option options :include_for_find [Array<Symbol>, Hash{Symbol => Symbol,Hash,Array}] models included but not in query
# @option options :filter [MiqExpression] (optional)

# @option options :user [User] (default: current_user)
Expand Down Expand Up @@ -413,7 +413,7 @@ def include_references(scope, klass, include_for_find, exp_includes, skip)

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)
scope = scope.references(klass.includes_to_references(include_for_find)).references(klass.includes_to_references(exp_includes))
end
scope
end
Expand Down
23 changes: 23 additions & 0 deletions spec/lib/extensions/ar_references_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
describe "ar_references" do
describe ".includes_to_references" do
it "supports none" do
expect(Vm.includes_to_references(nil)).to eq([])
expect(Vm.includes_to_references([])).to eq([])
end

it "supports arrays" do
expect(Vm.includes_to_references(%w[host operating_system])).to eq(%w[hosts operating_systems])
expect(Vm.includes_to_references(%i[host])).to eq(%w[hosts])
end

it "supports hashes" do
expect(Hardware.includes_to_references(:vm => {})).to eq(%w[vms])
expect(Hardware.includes_to_references(:vm => :ext_management_system)).to eq(%w[vms ext_management_systems])
expect(Hardware.includes_to_references(:vm => {:ext_management_system => {}})).to eq(%w[vms ext_management_systems])
end

it "supports table array" do
expect(Vm.includes_to_references(%w[hosts operating_systems])).to eq(%w[hosts operating_systems])
end
end
end
24 changes: 8 additions & 16 deletions spec/lib/rbac/filterer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -621,11 +621,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt
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 ["", "{}"]
expect(results.references_values).to eq []
end

context "with :include_for_find" do
Expand All @@ -638,7 +634,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt
end

it "adds references" do
expect(results.references_values).to match_array ["{:evm_owner=>{}}", "{}"]
expect(results.references_values).to match_array %w[users]
end
end
end
Expand All @@ -655,11 +651,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt
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 [""]
expect(results.references_values).to eq []
end

context "with :include_for_find" do
Expand All @@ -671,7 +663,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt
end

it "adds references" do
expect(results.references_values).to match_array ["", "{:evm_owner=>{}}"]
expect(results.references_values).to match_array %w[users]
end
end
end
Expand Down Expand Up @@ -2461,21 +2453,21 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects)
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=>{}}", ""])
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]
resulting_scope = subject.send(:include_references, *method_args)

expect(resulting_scope.references_values).to eq(["", "{:host=>{}}"])
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]
resulting_scope = subject.send(:include_references, *method_args)

expect(resulting_scope.references_values).to eq(["{:miq_server=>{}}", "{:host=>{}}"])
expect(resulting_scope.references_values).to eq(%w[miq_servers hosts])
end

context "if the include is polymorphic" do
Expand Down Expand Up @@ -2516,7 +2508,7 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects)

it "adds .references to the scope" do
allow(subject).to receive(:warn)
expect(resulting_scope.references_values).to eq(["{:miq_server=>{}}", "{:host=>{}}"])
expect(resulting_scope.references_values).to eq(%w[miq_servers hosts])
Copy link
Member Author

@kbrock kbrock Sep 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically the fix:

- expect(resulting_scope.references_values).to eq(["{:miq_server=>{}}", "{:host=>{}}"])
+ expect(resulting_scope.references_values).to eq(%w[miq_servers hosts])

Before: there were 2 tables with very ugly names
After: there are 2 tables (with valid names)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was it working before? Is this where it would reference all?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would reference "all"

just looking at it it looks wrong. right?

end

it "warns that there was an issue in test mode" do
Expand Down