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.

Sign up for GitHub

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

Jump to bottom

Add tenant filtering for templates in provisioning and summary pages #17851

Merged

gtanzillo merged 1 commit into ManageIQ:master from alexander-demicev:named-scopes-templates

Oct 10, 2018

alexander-demicev commented Aug 13, 2018 •

edited

Loading

This PR adds tenant filtering for templates in provisioning and summary pages.
I added new condition which finds all templates in case if user is admin, in case if user in not admin only private images that belong to user's tenant and public images should be shown.

https://bugzilla.redhat.com/show_bug.cgi?id=1524368
https://bugzilla.redhat.com/show_bug.cgi?id=1598520
https://bugzilla.redhat.com/show_bug.cgi?id=1546539

aufi reviewed

View reviewed changes

app/models/miq_template.rb Outdated

@@ @@ -36,6 +36,19 @@ def self.without_volume_templates @@
                                       "ManageIQ::Providers::Openstack::CloudManager::VolumeSnapshotTemplate"])
                 end
+                def self.applied_filtering
+                  tenant = User.current_user.current_group.tenant
+                  if tenant.source_id

Member

aufi Aug 14, 2018

PR works good for me (at least from OpenStack provider point of view).

Can we get MIQ-core eyes on this PR? @Ladas @agrare

Contributor

Ladas Aug 15, 2018

@lpichler hm, looks this this should be done via the tenant mapping? This looks like a wrong place.

@aufi Also I am confused about which tenant is in use here. Cause it looks like we use MIQ tenant, but we should rather use the mapped tenants? Or there should be 2 paths, with tenant mapping on&off?

alexander-demicev mentioned this pull request

Add tenant filtering for templates in provisioning and summary pages ManageIQ/manageiq-ui-classic#4465

Closed

Member

aufi commented Aug 28, 2018

current_group.tenant is MIQ Tenant of current user, if it was created by tenant-mapping feature, tenant.source_id points to OpenStack-mapped tenant (CloudTenant), otherwise it is nil. (assuming that source_id attribute is not used for other purposes by other providers or core which I'm not sure)

Contributor

lpichler commented Aug 28, 2018

@alexander-demichev isn't logic described in description covered by #17058 ? If not, it should be added in RBAC call (like you did in #17058 ) or in some mixin.

I think that when what to list templates (maybe some special type) we want to get same result, independently where we call it, especially when it depends on user.

Also I noticed that you are not asking for admin user you stated in description. (or is it accomplished by source_id condition ? )

assuming that source_id attribute is not used for other purposes by other providers or core which I'm not sure
that's right.

so we need to get in to RBAC call:

Rbac.filtered(MiqTemplate or do you need it only for any special model Cloud:: ? )

and then we need to spec as well.

if you have any other questions, let me know !

thanks!

alexander-demicev force-pushed the named-scopes-templates branch 2 times, most recently from e3f88b0 to 0f70bd0 Compare

September 6, 2018 12:43

Author

alexander-demicev commented Sep 6, 2018

@lpichler I added new tests, can you review PR? :)

alexander-demicev force-pushed the named-scopes-templates branch from 0f70bd0 to f4e65e2 Compare

September 6, 2018 13:29

Author

alexander-demicev commented Oct 2, 2018

@lpichler Hi, can I get a review?

lpichler reviewed

View reviewed changes

spec/lib/rbac/filterer_spec.rb Outdated

-                          it "returns all public cloud templates" do
+                          it "finds tenant's private cloud templates" do
+                            User.current_user = user_2
+                            tenant_2.source_id = 1

Contributor

lpichler Oct 3, 2018 •

edited

Loading

why there is hardcoded value 1 ? Isn't possible to establish relation ?

lpichler reviewed

View reviewed changes

spec/lib/rbac/filterer_spec.rb Outdated

+                        let(:group_2) { FactoryGirl.create(:miq_group, :tenant => tenant_2) } # T1
+                        let(:user_2) { FactoryGirl.create(:user, :miq_groups => [group_2]) }
+                        context "when tenant doesn't have source_id" do

Contributor

lpichler Oct 3, 2018

can you add something like when tenant is not mapped ? + on other places

lpichler reviewed

View reviewed changes

spec/lib/rbac/filterer_spec.rb Outdated

+                              expect(results).to match_array([cloud_template, cloud_template_root])
+                            end
+                            context "should ignore" do

Contributor

lpichler Oct 3, 2018

please write reason why it should ignore instead of, is it because of publicly_available => false ?

lpichler reviewed

View reviewed changes

spec/lib/rbac/filterer_spec.rb Outdated

+                            context "should ignore" do
+                              let!(:cloud_template) { FactoryGirl.create(:template_cloud, :tenant => tenant_3, :publicly_available => false) }
+                              it "private cloud templates" do

Contributor

lpichler Oct 3, 2018

always it "verb something" like "returns .." as you have it on other places

lpichler reviewed

View reviewed changes

spec/lib/rbac/filterer_spec.rb Outdated

+                            expect(results).to match_array([cloud_template_2, cloud_template_3])
+                          end
+                          let(:cloud_template_5) { FactoryGirl.create(:template_cloud, :tenant => default_tenant, :publicly_available => true) }

Contributor

lpichler Oct 3, 2018

it have to be evaluated before RBAC is called. And can you put it together after line context "when tenant has source_id "

            let!(:cloud_template_2) { FactoryGirl.create(:template_cloud, :tenant => tenant_2, :publicly_available => false) }
            let!(:cloud_template_3) { FactoryGirl.create(:template_cloud, :tenant => tenant_2, :publicly_available => true) }
            let!(:cloud_template_4) { FactoryGirl.create(:template_cloud, :tenant => default_tenant, :publicly_available => false) }
            let!(:cloud_template_5) { FactoryGirl.create(:template_cloud, :tenant => default_tenant, :publicly_available => true) }

Contributor

lpichler Oct 3, 2018

with this, it is failing for me, so you probably need more changes here, but now you it should cause sporadic failures.

thanks

Member

JPrause commented Oct 3, 2018

@miq-bot add_label blocker

miq-bot added the blocker label

Member

JPrause commented Oct 3, 2018

@alexander-demichev if this can be backported, can you add the gaprindashvili/yes and the hammer/yes labels

alexander-demicev force-pushed the named-scopes-templates branch from f4e65e2 to f0d226d Compare

October 4, 2018 13:01

Author

alexander-demicev commented Oct 4, 2018

@lpichler I refactored tests, is ok now? I tried to play with let!, but seems that the only way to make tests work is to create factories right in it block

alexander-demicev force-pushed the named-scopes-templates branch from f0d226d to 280fd75 Compare

October 5, 2018 07:42

Member

miq-bot commented Oct 5, 2018

Checked commit alexander-demicev@280fd75 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

lpichler approved these changes

View reviewed changes

Contributor

lpichler commented Oct 5, 2018

@miq-bot assign @gtanzillo

miq-bot assigned gtanzillo

Author

alexander-demicev commented Oct 9, 2018

@gtanzillo hi, can you merge/review?

gtanzillo reviewed

View reviewed changes

app/models/manageiq/providers/cloud_manager/template.rb Outdated

@@ @@ -124,9 +124,13 @@ def self.display_name(number = 1) @@
                 def self.tenant_id_clause(user_or_group)
                   template_tenant_ids = MiqTemplate.accessible_tenant_ids(user_or_group, Rbac.accessible_tenant_ids_strategy(self))
-                  return if template_tenant_ids.empty?
+                  tenant = User.current_user.current_group.tenant

Member

gtanzillo Oct 9, 2018

This can be simplified to User.current_user.current_tenant

Member

gtanzillo Oct 9, 2018

Also, should this be looking at user_or_group that is passed in instead of User.current_user?


          Add template filtering by tenant and visibility

560a98c

alexander-demicev force-pushed the named-scopes-templates branch from 280fd75 to 560a98c Compare

October 9, 2018 14:25

Member

JPrause commented Oct 10, 2018

@gtanzillo is this ready to merge?
@alexander-demichev can you add the gaprindashvili/yes and the hammer/yes labels

Author

alexander-demicev commented Oct 10, 2018

@miq-bot add_label gaprindashvili/yes

Author

alexander-demicev commented Oct 10, 2018

@miq-bot add_label hammer/yes

miq-bot added gaprindashvili/yes labels

gtanzillo approved these changes

View reviewed changes

gtanzillo added this to the Sprint 97 Ending Oct 22, 2018 milestone

gtanzillo merged commit 142a184 into ManageIQ:master

simaishi pushed a commit that referenced this pull request


          Merge pull request #17851 from alexander-demichev/named-scopes-templates

15058e5

Add tenant filtering for templates in provisioning and summary pages

(cherry picked from commit 142a184)

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

Contributor

simaishi commented Oct 11, 2018

Hammer backport details:

$ git log -1
commit 15058e549987cba58b342ba3d675719093212ffe
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Wed Oct 10 14:08:21 2018 -0400

    Merge pull request #17851 from alexander-demichev/named-scopes-templates
    
    Add tenant filtering for templates in provisioning and summary pages
    
    (cherry picked from commit 142a184d4619fd9d3ecebb6cff71ccd13667832e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1524368

simaishi added hammer/backported and removed hammer/yes labels

simaishi pushed a commit that referenced this pull request


          Merge pull request #17851 from alexander-demichev/named-scopes-templates

119d269

Add tenant filtering for templates in provisioning and summary pages

(cherry picked from commit 142a184)

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

Contributor

simaishi commented Oct 11, 2018

Gaprindashvili backport details:

$ git log -1
commit 119d2691ca63f5730e7f7cfa37b8b7c029e7fa2a
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Wed Oct 10 14:08:21 2018 -0400

    Merge pull request #17851 from alexander-demichev/named-scopes-templates
    
    Add tenant filtering for templates in provisioning and summary pages
    
    (cherry picked from commit 142a184d4619fd9d3ecebb6cff71ccd13667832e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1623561

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels

Contributor

simaishi commented Oct 11, 2018

oops... wrong BZ in the git commit msg for G backport... The correct one is: https://bugzilla.redhat.com/show_bug.cgi?id=1598520

lpichler mentioned this pull request

Fix RBAC call for templates and vms #18128

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocker gaprindashvili/backported hammer/backported