Authorise manage tenant quotas as tenant product feature in UI #5123

lpichler · 2019-01-03T16:13:10Z

WIP:

depends on Mark rbac_tenant_manage_quotas as MY TENANT FEATURE manageiq#18322
specs

Scenario:
Let's have tenant structure:

My Company
|--->  Alpha (user alpha)
       |---> Omega (user omega)

According to the BZ user alpha needs to have forbidden access to managing quotas for tenant Alpha (user alpha) and allowed to managing quotas.

We can use for this recent feature called Dynamic product features according to tenants
that we will authorise dynamic tenant product features (rbac_tenant_manage_quotas_tenant_<TENANT ID where we want/don't want to manage tenant quotas >) to access managing quotas of .

To accomplish this we need to change places where we are authorising rbac_tenant_manage_quotas_tenant to rbac_tenant_manage_quotas_tenant_<TENANT ID where we want/don't want to manage tenant quotas >

This places are in UI and API in the PRs above

User alpha has this setting in role (rbac_tenant_manage_quotas_tenant is checked just for Omega Tenant)

which will forbid managing for tenant alpha

and allow manage quotas for omega tenant

Links

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

@miq-bot add_label enhancement, blocker

lpichler · 2019-01-03T16:22:26Z

@miq-bot add_label hammer/yes

gtanzillo · 2019-01-03T16:57:58Z

app/controllers/ops_controller/ops_rbac.rb

@@ -166,7 +166,7 @@ def rbac_tenant_manage_quotas_reset
  end

  def rbac_tenant_manage_quotas
-    assert_privileges("rbac_tenant_manage_quotas")
+    assert_privileges(MiqProductFeature.tenant_identifier('rbac_tenant_manage_quotas', params[:id]))


Would be better if the call to MiqProductFeature.tenant_identifier lived in assert_privileges, if possible. This way UI developers don't necessarily need to be aware of it.

lpichler · 2019-01-04T14:07:23Z

@miq-bot assign @martinpovolny

himdel · 2019-01-04T14:14:04Z

app/controllers/application_controller.rb

@@ -2081,6 +2081,10 @@ def identify_tl_or_perf_record
  end

  def assert_privileges(feature)
+    if MiqProductFeature.my_root_tenant_identifier?(feature) && params&.[](:id)


params.key?(:id) ?

himdel · 2019-01-04T15:02:57Z

app/helpers/application_helper.rb

    # return feature that should be checked for the button that came in
    case pressed
    when "rbac_project_add", "rbac_tenant_add"
      "rbac_tenant_add"
+    when "rbac_tenant_manage_quotas"
+      MiqProductFeature.tenant_identifier('rbac_tenant_manage_quotas', resource_id)


Would it make sense to use the same logic as in assert_privileges here?

(As in, don't hardcode rbac_tenant_manage_quotas if this should be happening for every MiqProductFeature.my_root_tenant_identifier?(feature) when resource_id is not nil?)

This would also make it clear that calling tenant_identifier with nil as the second param shouldn't happen.

all of this method is a hack

there was some generic mechanism and then someone added an "if" there for a particular button

now there's one more of such "if"s.

Then the UI team has to support this for ever.

martinpovolny · 2019-01-04T15:34:18Z

app/controllers/application_controller.rb

@@ -1144,8 +1144,8 @@ def rbac_free_for_custom_button?(task, button_id)

  def check_button_rbac
    # buttons ids that share a common feature id
-    common_buttons = %w(rbac_project_add rbac_tenant_add)
-    task = common_buttons.include?(params[:pressed]) ? rbac_common_feature_for_buttons(params[:pressed]) : rbac_feature_id(params[:pressed])
+    common_buttons = %w(rbac_project_add rbac_tenant_add rbac_tenant_manage_quotas)


you are expanding an exception that was not supposed to be there at all

martinpovolny · 2019-01-04T15:39:48Z

app/controllers/application_controller.rb

@@ -2081,6 +2081,10 @@ def identify_tl_or_perf_record
  end

  def assert_privileges(feature)
+    if MiqProductFeature.my_root_tenant_identifier?(feature) && params.key?(:id)


assert_privileged was a rather clean method, not a pure function as it talks to a database, but given a state of the models it would return a result that would depend only on its single argument.

Not you are adding a side channel to it -- you talk to params. This goes directly against the cleanup efforts that we are having.

lpichler · 2019-01-07T09:46:08Z

@martinpovolny @himdel I overrode method role_allows? in ops_rbac controller - it works for buttons and accessing controller actions.

himdel · 2019-01-07T11:04:46Z

app/controllers/ops_controller/ops_rbac.rb

@@ -8,6 +8,14 @@ module OpsController::OpsRbac
    'Tenant'   => 'tenant'
  }.freeze

+  def role_allows?(**options)
+    if MiqProductFeature.my_root_tenant_identifier?(options[:feature]) && params.key?(:id)
+      options[:feature] = MiqProductFeature.tenant_identifier(options[:feature], params[:id].to_s.split("-").last)


why the split?

According to the tests, this is always just the id, are the tests wrong? ;)

(Or, if we're always dealing with trees, maybe something like ⬇️ ?)

_, _, id = TreeBuilder.extract_node_model_and_id(params[:id])

yes exactly I was looking for this, 👍

this goes for button (if it should display "Manage Tenant Quotas" button in toolbar) and for controller actions.

lpichler · 2019-01-07T12:16:31Z

@miq-bot remove_label wip

himdel · 2019-01-07T13:22:56Z

LGTM, merging now because build reasons 👍 ,
The test for "does it still render in the toolbar" will come as a separate PR. (#5129)

…_as_tenant_product_feature Authorise manage tenant quotas as tenant product feature in UI (cherry picked from commit ae4b7ad) https://bugzilla.redhat.com/show_bug.cgi?id=1468795

simaishi · 2019-01-07T16:16:22Z

Hammer backport details:

$ git log -1
commit 26035927639a636edd104b09ed00b03135d8956e
Author: Martin Hradil <himdel@seznam.cz>
Date:   Mon Jan 7 14:23:00 2019 +0100

    Merge pull request #5123 from lpichler/authorise_manage_tenant_quotas_as_tenant_product_feature
    
    Authorise manage tenant quotas as tenant product feature in UI
    
    (cherry picked from commit ae4b7add4a8f7d28bdac8a85fecb5cc882028c4e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1468795

miq-bot added blocker enhancement labels Jan 3, 2019

lpichler changed the title ~~[WIP] Authorise manage tenant quotas as tenant product feature~~ [WIP] Authorise manage tenant quotas as tenant product feature in UI Jan 3, 2019

miq-bot added the hammer/yes label Jan 3, 2019

lpichler mentioned this pull request Jan 3, 2019

Mark rbac_tenant_manage_quotas as MY TENANT FEATURE ManageIQ/manageiq#18322

Merged

2 tasks

gtanzillo reviewed Jan 3, 2019

View reviewed changes

lpichler force-pushed the authorise_manage_tenant_quotas_as_tenant_product_feature branch 2 times, most recently from 8ee9ce0 to 2b10a95 Compare January 4, 2019 08:26

mzazrivec added the wip label Jan 4, 2019

lpichler force-pushed the authorise_manage_tenant_quotas_as_tenant_product_feature branch 2 times, most recently from 6e97be5 to 46d9603 Compare January 4, 2019 14:06

miq-bot assigned martinpovolny Jan 4, 2019

himdel reviewed Jan 4, 2019

View reviewed changes

lpichler force-pushed the authorise_manage_tenant_quotas_as_tenant_product_feature branch from 46d9603 to 685a43f Compare January 4, 2019 14:41

himdel reviewed Jan 4, 2019

View reviewed changes

martinpovolny reviewed Jan 4, 2019

View reviewed changes

lpichler force-pushed the authorise_manage_tenant_quotas_as_tenant_product_feature branch 3 times, most recently from b9014e8 to 100db67 Compare January 7, 2019 08:54

lpichler changed the title ~~[WIP] Authorise manage tenant quotas as tenant product feature in UI~~ Authorise manage tenant quotas as tenant product feature in UI Jan 7, 2019

lpichler force-pushed the authorise_manage_tenant_quotas_as_tenant_product_feature branch from 100db67 to 838791c Compare January 7, 2019 09:43

lpichler closed this Jan 7, 2019

lpichler reopened this Jan 7, 2019

himdel reviewed Jan 7, 2019

View reviewed changes

Authorise action managing tenant quotas for according tenants

13a3f97

lpichler force-pushed the authorise_manage_tenant_quotas_as_tenant_product_feature branch from 838791c to 13a3f97 Compare January 7, 2019 11:35

miq-bot removed the wip label Jan 7, 2019

himdel merged commit ae4b7ad into ManageIQ:master Jan 7, 2019

himdel added this to the Sprint 102 Ending Jan 7, 2019 milestone Jan 7, 2019

lpichler deleted the authorise_manage_tenant_quotas_as_tenant_product_feature branch January 7, 2019 13:23

lpichler mentioned this pull request Jan 7, 2019

Add spec for button tenant manage quota in OpsController #5129

Merged

simaishi added hammer/backported and removed hammer/yes labels Jan 7, 2019

lpichler mentioned this pull request Jan 9, 2019

Fix usage of TreeBuilder.extract_node_model_and_id in OpcControler #5142

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authorise manage tenant quotas as tenant product feature in UI #5123

Authorise manage tenant quotas as tenant product feature in UI #5123

lpichler commented Jan 3, 2019 •

edited

Loading

lpichler commented Jan 3, 2019

gtanzillo Jan 3, 2019

lpichler commented Jan 4, 2019

himdel Jan 4, 2019

himdel Jan 4, 2019 •

edited

Loading

martinpovolny Jan 4, 2019

martinpovolny Jan 4, 2019 •

edited

Loading

martinpovolny Jan 4, 2019 •

edited

Loading

lpichler commented Jan 7, 2019

himdel Jan 7, 2019 •

edited

Loading

lpichler Jan 7, 2019

lpichler commented Jan 7, 2019

himdel commented Jan 7, 2019 •

edited

Loading

simaishi commented Jan 7, 2019

Authorise manage tenant quotas as tenant product feature in UI #5123

Authorise manage tenant quotas as tenant product feature in UI #5123

Conversation

lpichler commented Jan 3, 2019 • edited Loading

Links

lpichler commented Jan 3, 2019

gtanzillo Jan 3, 2019

Choose a reason for hiding this comment

lpichler commented Jan 4, 2019

himdel Jan 4, 2019

Choose a reason for hiding this comment

himdel Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

martinpovolny Jan 4, 2019

Choose a reason for hiding this comment

martinpovolny Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

martinpovolny Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

lpichler commented Jan 7, 2019

himdel Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

lpichler Jan 7, 2019

Choose a reason for hiding this comment

lpichler commented Jan 7, 2019

himdel commented Jan 7, 2019 • edited Loading

simaishi commented Jan 7, 2019

lpichler commented Jan 3, 2019 •

edited

Loading

himdel Jan 4, 2019 •

edited

Loading

martinpovolny Jan 4, 2019 •

edited

Loading

martinpovolny Jan 4, 2019 •

edited

Loading

himdel Jan 7, 2019 •

edited

Loading

himdel commented Jan 7, 2019 •

edited

Loading