Filter tenants and groups in Access Control by current region #4831

ZitaNemeckova · 2018-10-25T12:02:00Z

Closes #4720

Have a bunch of tenants
If you have all tenants in one region Tenant.count == Tenant.in_my_region.count set id of one of tenants to something outside current region(multiple its id by 3 or something)
Go to Configuration -> Access Control -> click a region

In this case cloudusers tenant is not in Tenant.in_my_region

Before:

After:

@miq-bot add_label bug, hammer/no,

mzazrivec · 2018-10-26T11:37:41Z

@lpichler is this change OK with you?

lpichler · 2018-10-26T12:09:11Z

spec/presenters/tree_builder_ops_rbac_spec.rb

+
+  describe "#x_get_tree_tenant_kids" do
+    let!(:child_tenant) { FactoryGirl.create(:tenant, :parent => Tenant.root_tenant) }
+    let!(:child_tenant_out_of_region) { FactoryGirl.create(:tenant, :parent => Tenant.root_tenant, :id => Tenant.root_tenant.id * 3) }


@ZitaNemeckova

you created child tenant in default region and his child which is "probably"( * 3) in different region and this situation doesn't happen in real.

we need to create tenant structure in default region and then whole tenant structure in other tenant region

and then check whether it is listing only tenant in current region.

you can get some inspiration from https://github.com/ManageIQ/manageiq/pull/17453/files#diff-de9adf0bd8114667190fecb643922d79R941 .

Usingid_in_region sounds better 👍

ZitaNemeckova · 2018-10-26T15:46:00Z

@lpichler Changed it as you suggested so Tenant is in a MiqRegion that actually exists and not some random "of by three" non-existent one :) Is it ok with you now?

lpichler · 2018-10-29T05:56:59Z

app/presenters/tree_builder_ops_rbac.rb

@@ -58,6 +58,6 @@ def x_get_tree_custom_kids(object_hash, count_only, _options)
  end

  def x_get_tree_tenant_kids(object, count_only)
-    count_only_or_objects(count_only, object.children, "name")
+    count_only_or_objects(count_only, object.children.in_my_region, "name")


this is not needed, because object is already in one region and his children will be also the same region. I can show it you in my env.

So there's no way for a Tenant to have a child that's in another region. Noted :)

lpichler · 2018-10-29T06:09:06Z

spec/presenters/tree_builder_ops_rbac_spec.rb

@@ -83,4 +83,17 @@ def x_get_tree_custom_kids_for_and_expect_objects(type_of_model, expected_object
      end
    end
  end
+
+  describe "#x_get_tree_tenant_kids" do


better however you are testing a code which I suggested to removed - you can still reuse the test for the "count" part.

and tests are better but it is still not reflect DB with 2 region, you can image it like two duplicated DB with different IDs, so one object in one region exists like duplicated object in other region.
You tests are mixing it together(like one tenant has children from other region) which is probably testing issue but it is not same as in real. SO I am suggesting to do it like it could be in real.

So one thing what you need to have is Tenant.root in other region.

let!(:child_tenant) { FactoryGirl.create(:tenant, :parent => Tenant.root_tenant) } let(:inactive_region) { FactoryGirl.create(:miq_region) } let(:tenant_root_in_inactive_region) do root_in_inactive_region = Tenant.root_tenant.dup root_in_inactive_region.id = Tenant.id_in_region(Tenant.count + 1_000_000, inactive_region.region) # you can create a method (DRY) root_in_inactive_region.save root_in_inactive_region end let!(:child_tenant_out_of_region) { FactoryGirl.create(:tenant, :parent => tenant_root_in_inactive_region, :id => Tenant.id_in_region(Tenant.count + 1_000_000, inactive_region.region)) }

I guess you didn't try that out or I'm missing something :) It's impossible to create second root Tenant in specs. It fails at Validation failed: Tenant: Parent required. I'll use a tenant that's in different region but has a parent from active region. Spec checks that count is correct not their relationship.

End removing the tree code because parent and child tenant have to be in the same region as you said earlier :)

Is that ok with you? :)

I see, you are right. It about that validator use current region. I think we can skip validation with
root_in_inactive_region.save!(:validate => false) in this case and you can leave parent as nil.

thanks!

lpichler · 2018-10-29T06:10:19Z

@lpichler Changed it as you suggested so Tenant is in a MiqRegion that actually exists and not some random "of by three" non-existent one :) Is it ok with you now?

Much better and I gave you other suggestions :)

lpichler · 2018-10-30T09:39:39Z

spec/controllers/ops_controller/ops_rbac_spec.rb

+    end
+
+    it 'counts only Tenants in active region' do
+      MiqRegion.seed


you can put this to the before block

miq-bot · 2018-10-30T13:46:42Z

Checked commit ZitaNemeckova@6893b69 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

Closes ManageIQ#4720

ZitaNemeckova · 2018-10-31T10:58:36Z

@miq-bot remove_label hammer/no
@miq-bot add_label hammer/yes

Filter tenants and groups in Access Control by current region (cherry picked from commit 79ac457)

simaishi · 2018-11-01T14:53:24Z

Hammer backport details:

$ git log -1
commit 056aac1f922ee57edb9abfb64f86250882e367c1
Author: Milan Zázrivec <[email protected]>
Date:   Thu Nov 1 11:05:12 2018 +0100

    Merge pull request #4831 from ZitaNemeckova/tenants_in_my_region
    
    Filter tenants and groups in Access Control by current region
    
    (cherry picked from commit 79ac45765a1da8f54e806ff923f8430a677a9e98)

miq-bot added bug hammer/no labels Oct 25, 2018

ZitaNemeckova force-pushed the tenants_in_my_region branch from c508f65 to e88c149 Compare October 25, 2018 12:14

mzazrivec added the settings label Oct 26, 2018

lpichler reviewed Oct 26, 2018

View reviewed changes

ZitaNemeckova force-pushed the tenants_in_my_region branch from e88c149 to 9fa2e98 Compare October 26, 2018 15:41

lpichler reviewed Oct 29, 2018

View reviewed changes

ZitaNemeckova force-pushed the tenants_in_my_region branch 2 times, most recently from 89a5046 to 1cb0c58 Compare October 30, 2018 08:57

lpichler reviewed Oct 30, 2018

View reviewed changes

ZitaNemeckova force-pushed the tenants_in_my_region branch 2 times, most recently from b68ed5b to 5201bc8 Compare October 30, 2018 10:17

ZitaNemeckova changed the title ~~Filter tenants in Access Control by current region~~ Filter tenants and groups in Access Control by current region Oct 30, 2018

ZitaNemeckova force-pushed the tenants_in_my_region branch 2 times, most recently from 759e1bf to 6893b69 Compare October 30, 2018 13:43

ZitaNemeckova force-pushed the tenants_in_my_region branch 2 times, most recently from 7ce0a85 to 57c4300 Compare October 31, 2018 10:46

Filter tenants and groups in Access Control by current region

57c4300

Closes ManageIQ#4720

miq-bot added hammer/yes and removed hammer/no labels Oct 31, 2018

lpichler approved these changes Oct 31, 2018

View reviewed changes

mzazrivec self-assigned this Nov 1, 2018

mzazrivec added this to the Sprint 98 Ending Nov 5, 2018 milestone Nov 1, 2018

mzazrivec merged commit 79ac457 into ManageIQ:master Nov 1, 2018

simaishi pushed a commit that referenced this pull request Nov 1, 2018

Merge pull request #4831 from ZitaNemeckova/tenants_in_my_region

056aac1

Filter tenants and groups in Access Control by current region (cherry picked from commit 79ac457)

simaishi added hammer/backported and removed hammer/yes labels Nov 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter tenants and groups in Access Control by current region #4831

Filter tenants and groups in Access Control by current region #4831

ZitaNemeckova commented Oct 25, 2018

mzazrivec commented Oct 26, 2018

lpichler Oct 26, 2018

ZitaNemeckova Oct 26, 2018

ZitaNemeckova commented Oct 26, 2018

lpichler Oct 29, 2018

ZitaNemeckova Oct 29, 2018

lpichler Oct 29, 2018

ZitaNemeckova Oct 29, 2018 •

edited

Loading

lpichler Oct 30, 2018

lpichler commented Oct 29, 2018 •

edited

Loading

lpichler Oct 30, 2018

miq-bot commented Oct 30, 2018

ZitaNemeckova commented Oct 31, 2018

simaishi commented Nov 1, 2018

Filter tenants and groups in Access Control by current region #4831

Filter tenants and groups in Access Control by current region #4831

Conversation

ZitaNemeckova commented Oct 25, 2018

mzazrivec commented Oct 26, 2018

lpichler Oct 26, 2018

Choose a reason for hiding this comment

ZitaNemeckova Oct 26, 2018

Choose a reason for hiding this comment

ZitaNemeckova commented Oct 26, 2018

lpichler Oct 29, 2018

Choose a reason for hiding this comment

ZitaNemeckova Oct 29, 2018

Choose a reason for hiding this comment

lpichler Oct 29, 2018

Choose a reason for hiding this comment

ZitaNemeckova Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

lpichler Oct 30, 2018

Choose a reason for hiding this comment

lpichler commented Oct 29, 2018 • edited Loading

lpichler Oct 30, 2018

Choose a reason for hiding this comment

miq-bot commented Oct 30, 2018

ZitaNemeckova commented Oct 31, 2018

simaishi commented Nov 1, 2018

ZitaNemeckova Oct 29, 2018 •

edited

Loading

lpichler commented Oct 29, 2018 •

edited

Loading