Ensure that `base_class` of first target is used for RBAC scope #16178

gtanzillo · 2017-10-11T21:23:51Z

When targets are passed and the instances are of different subclasses through STI, the base class needs to be used for building the scope to prevent ActiveRecord from also scoping to the subclass and limiting the results to only instances of that class.

For example - lets say that targets were passed in as instances of these classes that derive from ExtManagementSystem

["ManageIQ::Providers::Redhat::InfraManager",
 "ManageIQ::Providers::Google::CloudManager",
 "ManageIQ::Providers::StorageManager::CinderManager",
 "ManageIQ::Providers::Vmware::InfraManager",
 "ManageIQ::Providers::Openstack::NetworkManager"]

Without this change RBAC would only return the targets of the first class ManageIQ::Providers::Redhat::InfraManager in the results

https://bugzilla.redhat.com/show_bug.cgi?id=1480812
https://bugzilla.redhat.com/show_bug.cgi?id=1467756

/cc @jrafanie, @yrudman

jrafanie · 2017-10-11T22:01:41Z

spec/lib/rbac/filterer_spec.rb

+            results = described_class.search(:targets => targets, :user => user)
+            objects = results.first
+            expect(objects.length).to eq(4)
+            expect(objects).to eq(targets)


Ugh, sorry I didn't see this when we were writing it. Are these guaranteed to be in the same order? Maybe we should use contain_exactly or it's similar form match_array

jrafanie

Looks good other than my comment above

gtanzillo · 2017-10-12T13:40:38Z

@jrafanie I made the change to use match_array. Please take another 👀

lpichler · 2017-10-12T13:48:30Z

lib/rbac/filterer.rb

@@ -279,6 +280,10 @@ def search(options = {})
      return targets, attrs
    end

+    def is_sti?(klass)
+      klass.respond_to?(:finder_needs_type_condition?) ? klass.finder_needs_type_condition? : false


@gtanzillo @jrafanie

I am just thinking if it is covered also behaviour when you will pass objects with resource_type column like

[MetricRollup.first, VmPerformance.first]

because as

ManageIQ::Providers::Redhat::InfraManager.finder_needs_type_condition? => true is true
the
VmPerformance.finder_needs_type_condition? => false is not true

Thanks @lpichler. Let me make a test for that case and see.

@gtanzillo what did you find? Is this a test case we need to worry about?

@jrafanie, I discussed it with @yrudman and we added 2 more tests for this case. It turned out to be the same case that @lpichler previously did for MiqAeDomain.

Like we discussed yesterday, the case where multiple classes are represented in targets, like [MetricRollup, Vmperformance], and they are not STI would not be supported.

/cc @lpichler, @yrudman

That makes sense to me.

jrafanie · 2017-10-12T20:44:04Z

spec/lib/rbac/filterer_spec.rb

        User.with_user(admin_user) do
          results = described_class.search(:targets => [miq_ae_domain]).first
          expect(results.first).to be_an_instance_of(MiqAeDomain)
          expect(results).to match_array [miq_ae_domain]
        end
      end
+
+      it 'returns the same class as input for parents clast that is not STI' do


typo: clast

jrafanie · 2017-10-12T20:46:36Z

spec/lib/rbac/filterer_spec.rb

+        end
+      end
+
+      it 'returns the same class as input for subclass that is not STI' do


^ what does that mean? Is it returning a base class or subclass?

It's returning the subclass. But, the point is that it's returning the same class that was passed to targets.

jrafanie · 2017-10-12T20:48:16Z

spec/lib/rbac/filterer_spec.rb

@@ -830,6 +870,18 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects)
            expect(objects).to eq(targets)
          end

+          it "returns the correct results when targets are of different types" do


maybe this should be "returns the correct class for different classes of targets"

When targets are passed and the instances are of different subclasses through STI, the base class needs to be used for building the scope to prevent ActiveRecord from also scoping to the subclass and limiting the results to only instances of that class. https://bugzilla.redhat.com/show_bug.cgi?id=1480812 https://bugzilla.redhat.com/show_bug.cgi?id=1467756

miq-bot · 2017-10-13T11:03:11Z

Checked commit gtanzillo@cb3cb72 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 1 offense detected

lib/rbac/filterer.rb

❗ - Line 283, Col 9 - Style/PredicateName - Rename is_sti? to sti?.

Ensure that `base_class` of first target is used for RBAC scope (cherry picked from commit b02bfce) https://bugzilla.redhat.com/show_bug.cgi?id=1511135 https://bugzilla.redhat.com/show_bug.cgi?id=1511130

simaishi · 2017-11-13T21:36:31Z

Fine backport details:

$ git log -1
commit 997fa5b8dbf22aab13d327dd0375b40679b5281c
Author: Oleg Barenboim <[email protected]>
Date:   Fri Oct 13 09:46:11 2017 -0400

    Merge pull request #16178 from gtanzillo/rbac-sti-base-class
    
    Ensure that `base_class` of first target is used for RBAC scope
    (cherry picked from commit b02bfcea291aec1d2a2e08e54b360e5cface35c6)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1511135
    https://bugzilla.redhat.com/show_bug.cgi?id=1511130

Ensure that `base_class` of first target is used for RBAC scope (cherry picked from commit b02bfce) https://bugzilla.redhat.com/show_bug.cgi?id=1511135 https://bugzilla.redhat.com/show_bug.cgi?id=1511130

gtanzillo added bug fine/yes core/rbac labels Oct 11, 2017

gtanzillo requested a review from jrafanie October 11, 2017 21:26

jrafanie reviewed Oct 11, 2017

View reviewed changes

jrafanie requested changes Oct 11, 2017

View reviewed changes

gtanzillo force-pushed the rbac-sti-base-class branch from 257e1a0 to 80bbc59 Compare October 12, 2017 13:39

jrafanie approved these changes Oct 12, 2017

View reviewed changes

lpichler reviewed Oct 12, 2017

View reviewed changes

gtanzillo force-pushed the rbac-sti-base-class branch from 80bbc59 to 7cda928 Compare October 12, 2017 20:13

jrafanie reviewed Oct 12, 2017

View reviewed changes

gtanzillo force-pushed the rbac-sti-base-class branch 2 times, most recently from 4d9aaa0 to 3896845 Compare October 12, 2017 21:53

gtanzillo force-pushed the rbac-sti-base-class branch from 3896845 to cb3cb72 Compare October 13, 2017 10:58

chessbyte self-assigned this Oct 13, 2017

chessbyte merged commit b02bfce into ManageIQ:master Oct 13, 2017

chessbyte added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 13, 2017

simaishi added fine/backported and removed fine/yes labels Nov 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that `base_class` of first target is used for RBAC scope #16178

Ensure that `base_class` of first target is used for RBAC scope #16178

gtanzillo commented Oct 11, 2017

jrafanie Oct 11, 2017 •

edited

Loading

jrafanie left a comment

gtanzillo commented Oct 12, 2017

lpichler Oct 12, 2017

gtanzillo Oct 12, 2017

jrafanie Oct 12, 2017

gtanzillo Oct 12, 2017

jrafanie Oct 12, 2017

jrafanie Oct 12, 2017

jrafanie Oct 12, 2017

gtanzillo Oct 12, 2017

jrafanie Oct 12, 2017

miq-bot commented Oct 13, 2017

simaishi commented Nov 13, 2017

Ensure that base_class of first target is used for RBAC scope #16178

Ensure that base_class of first target is used for RBAC scope #16178

Conversation

gtanzillo commented Oct 11, 2017

jrafanie Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

jrafanie left a comment

Choose a reason for hiding this comment

gtanzillo commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Oct 13, 2017

simaishi commented Nov 13, 2017

Ensure that `base_class` of first target is used for RBAC scope #16178

Ensure that `base_class` of first target is used for RBAC scope #16178

jrafanie Oct 11, 2017 •

edited

Loading