Make all public images be visible for provisioning. #17058

alexander-demicev · 2018-02-27T17:22:02Z

This PR makes all public images visible for tenants. The issue is that when we have MiQ user with visibility to 1 cloud tenant public images of admin user are not visible.
https://bugzilla.redhat.com/show_bug.cgi?id=1524368

ping @aufi

alexander-demicev · 2018-03-01T13:05:03Z

@lpichler Can you take a look to it?

lpichler · 2018-03-07T14:15:30Z

@alexander-demichev so far looks good but can you add some specs to see RBAC cases ?
thanks!

alexander-demicev · 2018-03-07T14:19:32Z

@lpichler Sure, I will do it

alexander-demicev · 2018-03-12T12:45:50Z

@lpichler Hi, I added some tests, is it ok now?

miq-bot · 2018-04-04T15:11:07Z

This pull request is not mergeable. Please rebase and repush.

mansam · 2018-04-10T17:13:34Z

Hi @lpichler, is there anything else @alexander-demichev needs to do on this? Thanks! :)

lpichler · 2018-04-11T08:25:01Z

Hi @alexander-demichev, your test are good but as you nicely built objects user - group - role, can you also add some test with RBAC call ? (you will need to create some template cloud )
I mean:

 let (:cloud_template_1) { } 
 let (:cloud_template_2) { } 

  it 'returns all cloud templates when user is admin ' do 

      results  = described_class.search(:class => CloudTemplate, :user => any_user)
      expect(results).to (CloudTemplate.all)
  end

  it 'returns only cloud templates related to restricted user is admin' do 
    ... 
  end

and such specs what I am suggesting are placed usually in filterer_spec.rb

miq-bot · 2018-06-07T22:26:56Z

This pull request is not mergeable. Please rebase and repush.

JPrause · 2018-06-22T17:18:59Z

@alexander-demichev are there further review/edits needed?
Who can merged is all is good. (although I see it's unmergable now)

lpichler · 2018-06-29T15:13:54Z

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

@@ -122,6 +122,13 @@ def self.display_name(number = 1)
    n_('Image', 'Images', number)
  end

+  def self.tenant_id_clause(user_or_group)
+    template_tenant_ids = MiqTemplate.accessible_tenant_ids(user_or_group, Rbac.accessible_tenant_ids_strategy(MiqTemplate))


this can be just

accessible_tenant_ids(user_or_group, Rbac.accessible_tenant_ids_strategy(self))

lpichler · 2018-06-29T15:14:56Z

spec/lib/rbac/filterer_spec.rb

+            let!(:cloud_template) { FactoryGirl.create(:template_cloud, :tenant => tenant_3, :publicly_available => true) }
+
+            context "returns all public cloud templates" do
+              it "" do


missing description for it ""

lpichler · 2018-06-29T15:16:03Z

spec/lib/rbac/filterer_spec.rb

+            let(:tenant_3) { FactoryGirl.create(:tenant, :parent => tenant_2) } # T3
+            let!(:cloud_template) { FactoryGirl.create(:template_cloud, :tenant => tenant_3, :publicly_available => true) }
+
+            context "returns all public cloud templates" do


context is not needed here, it looks like that message should in related 'it'

you can learn more here: http://www.betterspecs.org

lpichler · 2018-06-29T15:16:39Z

spec/lib/rbac/filterer_spec.rb

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


missing description

lpichler · 2018-06-29T15:18:31Z

@alexander-demichev I put some comments here, otherwise LGTM 👍

@miq-bot assign @gtanzillo

miq-bot · 2018-07-02T09:33:20Z

Checked commit alexander-demicev@e5d2edd 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. ⭐

Fryguy · 2018-07-02T17:48:01Z

Something about this API feels off...I don't understand why a model would have clause like this as opposed to an ActiveRecord scope. @NickLaMuro @kbrock Thoughts here?

NickLaMuro · 2018-07-02T17:59:16Z

@Fryguy at first glance, I have seen this before:

https://github.com/ManageIQ/manageiq/blob/6627b3ba/app/models/mixins/cloud_tenancy_mixin.rb#L16-L18

So it is a method in this case as well, but defined in a mixin in this case.

I mean, as far as whether it is a scope or not, I guess I don't have an opinion (since one is just an shorthand for the other). And I think you are more concerned about the pattern that is in place for tenancy plus RBAC, and for that, I don't know what I think without giving it some more thought.

This seems to be be at least following the pattern in place for the TenancyCommonMixin:

https://github.com/ManageIQ/manageiq/blob/6627b3ba704820e9cd540dfbfc8ef48aa197f784/app/models/mixins/tenancy_common_mixin.rb

But unsure if that pattern is a good thing overall.

gtanzillo · 2018-07-02T18:20:28Z

As @NickLaMuro pointed out, that is the pattern we've been using for cases where the generic tenant access doesn't fit a particular class. It may not be ideal but we've done this for a handful of classes already.

simaishi · 2018-07-05T17:30:04Z

@alexander-demichev @gtanzillo Can this be gaprindashvili/yes ?

gtanzillo · 2018-07-06T12:57:44Z

@simaishi, Yes, I think so. I'll set the label

Make all public images be visible for provisioning. (cherry picked from commit 178f498) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1598520

simaishi · 2018-07-06T13:52:58Z

Thanks @gtanzillo

Gaprindashvili backport details:

$ git log -1
commit 3f9e0e78e3f84f81a990862224e46002c62c52fe
Author: Gregg Tanzillo <[email protected]>
Date:   Tue Jul 3 05:16:55 2018 -0400

    Merge pull request #17058 from alexander-demichev/public-images-tenants
    
    Make all public images be visible for provisioning.
    (cherry picked from commit 178f498dd0a4e62e2d80c100504b55bf347ebc22)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1598520

miq-bot added the wip label Feb 27, 2018

alexander-demicev force-pushed the public-images-tenants branch from 39a8f3d to f78b823 Compare March 12, 2018 12:44

alexander-demicev changed the title ~~[WIP] Make all public images be visible for provisioning.~~ Make all public images be visible for provisioning. Mar 12, 2018

miq-bot removed the wip label Mar 12, 2018

miq-bot added the unmergeable label Apr 4, 2018

alexander-demicev force-pushed the public-images-tenants branch 2 times, most recently from 0194b99 to a2f4837 Compare April 10, 2018 16:43

miq-bot removed the unmergeable label Apr 10, 2018

alexander-demicev force-pushed the public-images-tenants branch from a2f4837 to 509926c Compare May 14, 2018 13:29

miq-bot added the unmergeable label Jun 7, 2018

alexander-demicev force-pushed the public-images-tenants branch 2 times, most recently from d627e41 to 1be1571 Compare June 28, 2018 13:12

miq-bot removed the unmergeable label Jun 28, 2018

alexander-demicev force-pushed the public-images-tenants branch from 1be1571 to 46053ba Compare June 29, 2018 11:58

lpichler reviewed Jun 29, 2018

View reviewed changes

miq-bot assigned gtanzillo Jun 29, 2018

add publicity of images to query

e5d2edd

alexander-demicev force-pushed the public-images-tenants branch from 46053ba to e5d2edd Compare July 2, 2018 09:28

gtanzillo added enhancement providers labels Jul 3, 2018

gtanzillo added this to the Sprint 89 Ending Jul 2, 2018 milestone Jul 3, 2018

gtanzillo merged commit 178f498 into ManageIQ:master Jul 3, 2018

gtanzillo added the gaprindashvili/yes label Jul 6, 2018

simaishi added blocker gaprindashvili/backported and removed gaprindashvili/yes labels Jul 6, 2018

lpichler mentioned this pull request Aug 28, 2018

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make all public images be visible for provisioning. #17058

Make all public images be visible for provisioning. #17058

alexander-demicev commented Feb 27, 2018

alexander-demicev commented Mar 1, 2018

lpichler commented Mar 7, 2018

alexander-demicev commented Mar 7, 2018 •

edited

Loading

alexander-demicev commented Mar 12, 2018

miq-bot commented Apr 4, 2018

mansam commented Apr 10, 2018

lpichler commented Apr 11, 2018 •

edited

Loading

miq-bot commented Jun 7, 2018

JPrause commented Jun 22, 2018

lpichler Jun 29, 2018

lpichler Jun 29, 2018

lpichler Jun 29, 2018

lpichler Jun 29, 2018

lpichler Jun 29, 2018

lpichler commented Jun 29, 2018

miq-bot commented Jul 2, 2018

Fryguy commented Jul 2, 2018

NickLaMuro commented Jul 2, 2018

gtanzillo commented Jul 2, 2018

simaishi commented Jul 5, 2018

gtanzillo commented Jul 6, 2018

simaishi commented Jul 6, 2018

Make all public images be visible for provisioning. #17058

Make all public images be visible for provisioning. #17058

Conversation

alexander-demicev commented Feb 27, 2018

alexander-demicev commented Mar 1, 2018

lpichler commented Mar 7, 2018

alexander-demicev commented Mar 7, 2018 • edited Loading

alexander-demicev commented Mar 12, 2018

miq-bot commented Apr 4, 2018

mansam commented Apr 10, 2018

lpichler commented Apr 11, 2018 • edited Loading

miq-bot commented Jun 7, 2018

JPrause commented Jun 22, 2018

lpichler Jun 29, 2018

Choose a reason for hiding this comment

lpichler Jun 29, 2018

Choose a reason for hiding this comment

lpichler Jun 29, 2018

Choose a reason for hiding this comment

lpichler Jun 29, 2018

Choose a reason for hiding this comment

lpichler Jun 29, 2018

Choose a reason for hiding this comment

lpichler commented Jun 29, 2018

miq-bot commented Jul 2, 2018

Fryguy commented Jul 2, 2018

NickLaMuro commented Jul 2, 2018

gtanzillo commented Jul 2, 2018

simaishi commented Jul 5, 2018

gtanzillo commented Jul 6, 2018

simaishi commented Jul 6, 2018

alexander-demicev commented Mar 7, 2018 •

edited

Loading

lpichler commented Apr 11, 2018 •

edited

Loading