Add tenant scoping for resources of performance reports in RBAC #14095

lpichler · 2017-02-28T14:18:53Z

second part of
https://bugzilla.redhat.com/show_bug.cgi?id=1418961
first part (#14041)

RBAC is supported for MetricRollups class and his childs also MetricRollup#resource but there is no support also for tenant scoping, so this PR is adding it:

cc @kbrock

@miq-bot assign @gtanzillo

kbrock

this is great.
Will help right across the board

have been working so hard on consolidating that set logic, so would prefer to have just a single point where set logic is performed

kbrock · 2017-03-02T18:47:42Z

lib/rbac/filterer.rb

@@ -445,7 +445,12 @@ def scope_targets(klass, scope, rbac_filters, user, miq_group)
        # if subclasses of MetricRollup or Metric, use the associated
        # model to derive permissions from
        associated_class = rbac_class(scope)
+        if associated_class.respond_to?(:scope_by_tenant?) && associated_class.scope_by_tenant?


if associated_class.try(:scope_by_tenant?)

kbrock · 2017-03-02T18:50:34Z

lib/rbac/filterer.rb

@@ -445,7 +445,12 @@ def scope_targets(klass, scope, rbac_filters, user, miq_group)
        # if subclasses of MetricRollup or Metric, use the associated
        # model to derive permissions from
        associated_class = rbac_class(scope)
+        if associated_class.respond_to?(:scope_by_tenant?) && associated_class.scope_by_tenant?
+          scope_associated_ids = scope_to_tenant(associated_class, user, miq_group).ids


please leave this as a scope and rename to something like tenant_filter

kbrock · 2017-03-02T18:51:35Z

lib/rbac/filterer.rb

        filtered_ids = calc_filtered_ids(associated_class, rbac_filters, user, miq_group)
+        filtered_ids = [scope_associated_ids, filtered_ids].compact.flatten


please pass this scope into filtered_ids.

Trying hard to keep that set logic in there so we can move to the database in the future.

(all but one of those params are currently scopes)

I cannot pass it to the first parameter because scope tenant condition in scope_tenant_filter would be lost, so I added it as new parameter.
Is this ok for now ?

For now we need to make 1 query to determine scope_tenant_filter.ids, in follow up PR I would like to look if it is possible to merge it to main scope and skip this query.

thanks for review @kbrock

kbrock · 2017-03-03T19:34:10Z

lib/rbac/filterer.rb

      klass = scope.respond_to?(:klass) ? scope.klass : scope
      u_filtered_ids = pluck_ids(get_self_service_objects(user, miq_group, klass))
      b_filtered_ids = get_belongsto_filter_object_ids(klass, user_filters['belongsto'])
      m_filtered_ids = pluck_ids(get_managed_filter_object_ids(scope, user_filters['managed']))
      d_filtered_ids = pluck_ids(matches_via_descendants(rbac_class(klass), user_filters['match_via_descendants'],
                                                         :user => user, :miq_group => miq_group))

-      combine_filtered_ids(u_filtered_ids, b_filtered_ids, m_filtered_ids, d_filtered_ids)
+      combine_filtered_ids(u_filtered_ids, b_filtered_ids, m_filtered_ids, d_filtered_ids) ||


Would it be difficult to get this into combine_filtered_ids?
(just say too difficult if so. I can fix when I'm going in there to convert to a sql scope from raw ids)

yes done 👍

https://github.com/ManageIQ/manageiq/pull/14095/files#diff-8d948055de14ced0e63abf9637a9a788R383 this maybe look sort complex but there is needed previous values it returns. (nil should not be confused with an empty field)

kbrock

Looking good. just the one question

also, the tenant_scope can probably be always passed in, even if it is a nil. I'm guessing we probably need to change our code to always have a tenant scope (unless this makes testing harder - then leave the nil as is)

lpichler · 2017-03-06T14:20:53Z

@kbrock

... so we can move to the database in the future.

What do you mean by this ? Do you mean somehow convert RBAC to sequence of rules or DSL ?

kbrock · 2017-03-06T22:53:55Z

@lpichler example

Current vm RBAC combine_filtered_ids workflow:

Bring back ids for 4 different queries of vms
Perform post processing on ids (down to only 1 id list being manipulated)
Do set logic in memory
send that list of ids back to the server to bring back vms.

Since sql's strength is set logic (e.g. using AND and OR), it makes no sense to bring this back into ruby. Just need to remove post processing step.

Was hoping we could keep the set logic in the combine_filtered_ids method.
That way the combination will be more straight forward.

But if that doesn't work for you, we can move it into that method when we are fixing step 2.

- visible ids of resource class are determined for tenant - those ids are used for filtering as well

miq-bot · 2017-03-07T12:47:45Z

Checked commits lpichler/manageiq@39d0469~...dce0e5d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🍪

kbrock

Thanks. this looks great

gtanzillo

👍 Thanks @lpichler @kbrock

…omance_reports Add tenant scoping for resources of performance reports in RBAC (cherry picked from commit f46d363) https://bugzilla.redhat.com/show_bug.cgi?id=1431168

simaishi · 2017-03-10T14:34:29Z

Euwe backport details:

$ git log -1
commit 997b55cb7fa847993d81c379def3dd77077e520e
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Tue Mar 7 16:05:03 2017 -0500

    Merge pull request #14095 from lpichler/add_rbac_to_resouces_for_perfomance_reports
    
    Add tenant scoping for resources of performance reports in RBAC
    (cherry picked from commit f46d363c250b386a9783dc10e6a98bb8e4413442)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1431168

simaishi · 2017-03-14T18:38:14Z

Backported to Darga via #14308

…for_perfomance_reports Add tenant scoping for resources of performance reports in RBAC (cherry picked from commit f46d363) https://bugzilla.redhat.com/show_bug.cgi?id=1431168

miq-bot added the wip label Feb 28, 2017

chessbyte added core/chargeback enhancement core/rbac labels Feb 28, 2017

chessbyte assigned gtanzillo Feb 28, 2017

lpichler force-pushed the add_rbac_to_resouces_for_perfomance_reports branch from 6a36e31 to 03569d2 Compare March 2, 2017 14:08

lpichler changed the title ~~[WIP] Add rbac to resources for performance reports~~ Add tenant scoping for resources of performance reports in RBAC Mar 2, 2017

miq-bot removed the wip label Mar 2, 2017

lpichler force-pushed the add_rbac_to_resouces_for_perfomance_reports branch from 03569d2 to 7013bfa Compare March 2, 2017 15:00

kbrock reviewed Mar 2, 2017

View reviewed changes

lpichler force-pushed the add_rbac_to_resouces_for_perfomance_reports branch from 7013bfa to 4521c0b Compare March 3, 2017 17:58

kbrock reviewed Mar 3, 2017

View reviewed changes

lpichler force-pushed the add_rbac_to_resouces_for_perfomance_reports branch 2 times, most recently from 206f5db to 28298d3 Compare March 6, 2017 13:49

lpichler force-pushed the add_rbac_to_resouces_for_perfomance_reports branch from 28298d3 to 6b47b81 Compare March 6, 2017 14:31

lpichler added 3 commits March 7, 2017 13:45

Add scoping to tenant for resources in performance reports

39d0469

- visible ids of resource class are determined for tenant - those ids are used for filtering as well

Add factory for VmPerfomance

25d89dc

Specs for tenant scoping for VmPerformance

dce0e5d

lpichler force-pushed the add_rbac_to_resouces_for_perfomance_reports branch from 6b47b81 to dce0e5d Compare March 7, 2017 12:45

kbrock approved these changes Mar 7, 2017

View reviewed changes

lpichler closed this Mar 7, 2017

lpichler reopened this Mar 7, 2017

gtanzillo approved these changes Mar 7, 2017

View reviewed changes

gtanzillo added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 7, 2017

gtanzillo merged commit f46d363 into ManageIQ:master Mar 7, 2017

lpichler deleted the add_rbac_to_resouces_for_perfomance_reports branch March 9, 2017 19:26

gtanzillo added the euwe/yes label Mar 10, 2017

simaishi added euwe/backported and removed euwe/yes labels Mar 10, 2017

lpichler mentioned this pull request Mar 14, 2017

[DARGA] Add rbac to resouces for perfomance reports #14316

Closed

simaishi added the darga/yes label Mar 14, 2017

simaishi added darga/backported and removed darga/yes labels Mar 14, 2017

lpichler mentioned this pull request May 15, 2017

Fix tag filtering for indirect RBAC #15088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tenant scoping for resources of performance reports in RBAC #14095

Add tenant scoping for resources of performance reports in RBAC #14095

lpichler commented Feb 28, 2017 •

edited

Loading

kbrock left a comment

kbrock Mar 2, 2017

kbrock Mar 2, 2017

kbrock Mar 2, 2017

lpichler Mar 3, 2017

kbrock Mar 3, 2017

lpichler Mar 6, 2017

kbrock left a comment •

edited

Loading

lpichler commented Mar 6, 2017 •

edited

Loading

kbrock commented Mar 6, 2017

miq-bot commented Mar 7, 2017

kbrock left a comment

gtanzillo left a comment

simaishi commented Mar 10, 2017

simaishi commented Mar 14, 2017

		filtered_ids = calc_filtered_ids(associated_class, rbac_filters, user, miq_group)
		filtered_ids = [scope_associated_ids, filtered_ids].compact.flatten

Add tenant scoping for resources of performance reports in RBAC #14095

Add tenant scoping for resources of performance reports in RBAC #14095

Conversation

lpichler commented Feb 28, 2017 • edited Loading

kbrock left a comment

Choose a reason for hiding this comment

kbrock Mar 2, 2017

Choose a reason for hiding this comment

kbrock Mar 2, 2017

Choose a reason for hiding this comment

kbrock Mar 2, 2017

Choose a reason for hiding this comment

lpichler Mar 3, 2017

Choose a reason for hiding this comment

kbrock Mar 3, 2017

Choose a reason for hiding this comment

lpichler Mar 6, 2017

Choose a reason for hiding this comment

kbrock left a comment • edited Loading

Choose a reason for hiding this comment

lpichler commented Mar 6, 2017 • edited Loading

kbrock commented Mar 6, 2017

miq-bot commented Mar 7, 2017

kbrock left a comment

Choose a reason for hiding this comment

gtanzillo left a comment

Choose a reason for hiding this comment

simaishi commented Mar 10, 2017

simaishi commented Mar 14, 2017

lpichler commented Feb 28, 2017 •

edited

Loading

kbrock left a comment •

edited

Loading

lpichler commented Mar 6, 2017 •

edited

Loading