Added ActAsTaggable concern to MiqRequest model

[yrudman]

MiqRequest was added to RBAC in #17208 without adding ActAsTaggable. It cause raising

[----] F, [2018-05-08T19:34:15.481475 #7215:1428924] FATAL -- : Error caught: [NoMethodError] undefined method `find_tags_by_grouping' for #<MiqRequest::ActiveRecord_Relation:0x0000000012b154b0>
/opt/rh/cfme-gemset/gems/activerecord-5.0.6/lib/active_record/relation/delegation.rb:124:in `method_missing'
/opt/rh/cfme-gemset/gems/activerecord-5.0.6/lib/active_record/relation/delegation.rb:94:in `method_missing'
/var/www/miq/vmdb/lib/rbac/filterer.rb:493:in `get_managed_filter_object_ids'
/var/www/miq/vmdb/lib/rbac/filterer.rb:426:in `calc_filtered_ids'

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

BEFORE:

AFTER:

@miq-bot add-label bug, core, gaprindashvili/yes, fine/yes

[miq-bot]

@yrudman unrecognized command 'ad-label', ignoring...

Accepted commands are: add_label, add_reviewer, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

[miq-bot]

Checked commit yrudman@582d4ab with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

[lpichler]

LGTM 👍 thanks @yrudman

[yrudman]

@miq-bot add-label bug, core, gaprindashvili/yes, fine/yes

[yrudman]

@miq-bot assign @jrafanie

[yrudman]

@miq-bot add-label blocker

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit ac40681930507ccde0ffc6ced0321a1eed542010
Author: Joe Rafaniello <[email protected]>
Date:   Wed May 23 11:14:23 2018 -0400

    Merge pull request #17466 from yrudman/make-miq_request-act-as-taggable
    
    Added ActAsTaggable concern to MiqRequest model
    (cherry picked from commit 5923577ea5370b94c05efe74ce85b65982925593)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1583710

[simaishi]

Fine backport details:

$ git log -1
commit fb18f4d2a3b33cd6edbb98ce0f277c791f11acfc
Author: Joe Rafaniello <[email protected]>
Date:   Wed May 23 11:14:23 2018 -0400

    Merge pull request #17466 from yrudman/make-miq_request-act-as-taggable
    
    Added ActAsTaggable concern to MiqRequest model
    (cherry picked from commit 5923577ea5370b94c05efe74ce85b65982925593)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1583711

[LorkScorguar]

After this fix, I don't have error on requests page, but with a account tied to a subtenant, i'm unable to see any requests even my own.

[jrafanie]

@LorkScorguar a subtenant can't see parent tenant requests via the tenant access strategy, only descendant tenant requests. Would you be able to draw a tree/picture of how your tenant structure looks, who created the request (and where they are in the tenant structure), and who's failing to "see" the request (and where they are in the tenant structure). Thanks!

[LorkScorguar]

This is what I have:
rootTenant
|
subTenant
/ |
sub2Tenant sub2Tenant2 sub2Tenant3

if a user from sub2Tenant create a request, he can't see other tenant requests (normal) but he can't see request on his tenant (abnormal), even he can't see his own requests.
User from subTenant are also unable to see requests from descendant tenant.

All users get rights from custom roles, not the default ones. I don't know if this can be the problem.

[jrafanie]

@LorkScorguar I am not able to recreate this in test. Is it possible the user's role doesn't include the various miq_request_* identifiers, see here.

I wrote the following tests and am able to get your tests to pass with no code changes:

diff --git a/spec/lib/rbac/filterer_spec.rb b/spec/lib/rbac/filterer_spec.rb
index 045ff9a204..7c9eba65cc 100644
--- a/spec/lib/rbac/filterer_spec.rb
+++ b/spec/lib/rbac/filterer_spec.rb
@@ -122,6 +122,7 @@ describe Rbac::Filterer do

   let(:child_tenant)       { FactoryGirl.create(:tenant, :divisible => false, :parent => owner_tenant) }
   let(:child_group)        { FactoryGirl.create(:miq_group, :tenant => child_tenant) }
+  let(:child_user)         { FactoryGirl.create(:user, :miq_groups => [child_group]) }
   let(:child_openstack_vm) { FactoryGirl.create(:vm_openstack, :tenant => child_tenant, :miq_group => child_group) }

   describe ".search" do
@@ -621,6 +622,35 @@ describe Rbac::Filterer do
         end
       end

+      context "with accessible_tenant_ids filtering (strategy = :descendants_id)" do
+        it "can see own request" do
+          request = FactoryGirl.create(:miq_host_provision_request, :tenant => owner_tenant, :requester => owner_user)
+          results = described_class.search(:class => "MiqRequest", :user => owner_user).first
+          expect(results).to match_array [request]
+        end
+
+        it "can see other's request in same tenant" do
+          group = FactoryGirl.create(:miq_group, :tenant => owner_tenant)
+          user  = FactoryGirl.create(:user, :miq_groups => [group])
+
+          request = FactoryGirl.create(:miq_host_provision_request, :tenant => owner_tenant, :requester => owner_user)
+          results = described_class.search(:class => "MiqRequest", :user => user).first
+          expect(results).to match_array [request]
+        end
+
+        it "can't see parent tenant's request" do
+          FactoryGirl.create(:miq_host_provision_request, :tenant => owner_tenant, :requester => owner_user)
+          results = described_class.search(:class => "MiqRequest", :miq_group => child_group).first
+          expect(results).to match_array []
+        end
+
+        it "can see descendant tenant's request" do
+          request = FactoryGirl.create(:miq_host_provision_request, :tenant => child_tenant, :requester => child_user)
+          results = described_class.search(:class => "MiqRequest", :miq_group => owner_group).first
+          expect(results).to match_array [request]
+        end
+      end
+
       context "tenant access strategy VMs and Templates" do
         let(:owned_template) { FactoryGirl.create(:template_vmware, :tenant => owner_tenant) }
         let(:child_tenant)   { FactoryGirl.create(:tenant, :divisible => false, :parent => owner_tenant) }

11:28:28 ~/Code/manageiq (master) (2.4.4) + be rspec -f d spec/lib/rbac/filterer_spec.rb:625
/Users/joerafaniello/.gem/ruby/2.4.4/bundler/gems/manageiq-providers-redfish-ffa8cf8146bc/spec/factories/physical_server.rb:3: warning: toplevel constant PhysicalServer referenced by ManageIQ::Providers::Redfish::PhysicalInfraManager::PhysicalServer
Run options: include {:locations=>{"./spec/lib/rbac/filterer_spec.rb"=>[625]}}

Randomized with seed 9385

Rbac::Filterer
  .search
    with find_options_for_tenant filtering
      with accessible_tenant_ids filtering (strategy = :descendants_id)
        can see other's request in same tenant
        can't see parent tenant's request
        can see descendant tenant's request
        can see own request

Finished in 3.81 seconds (files took 9.03 seconds to load)
4 examples, 0 failures

Randomized with seed 9385 

[LorkScorguar]

@jrafanie I found the problem. It's not related to role, but to my group. On my group I have a filter by tag which is used to restrict view on some object, but miq_requests doesn't have tags, so users are unable to see their requests.

[jrafanie]

Interesting @LorkScorguar. Would you be able to share the exact tag filter? I want to write a test for this. Please change the name of the filter if it's something you don't want to share.

What do you think about this @yrudman @gtanzillo @lpichler?

[LorkScorguar]

@jrafanie Here are the informations:
Using the same tenant tree as before, this is what I have:

           rootTenant
               |
            subTenant
   /           |               \
sub2Tenant sub2Tenant2 sub2Tenant3

2Tags:

• business_group, can be: team1, team2, team3
• business_unit, can be: fr, de, uk

We have 3 user groups:
groupA, attached to rootTenant and no filter, custom role full access
groupB, attached to rootTenant and filter business_unit=fr, custom role access to catalog, services and machines
groupC, attached to sub2Tenant and filter business_unit=fr & business_group=teamC, custom role access to catalog, services and machines (same as for groupB)

groupA have no problem seeing requests
groupB and groupC can see any requests even their own
I tried to remove the filter tag on groupB and then they are able to see requests, but I can't keep it like that.

I experiment with requests and giving group right to approve/deny + tag_assign on request allow user to see the request, but tag_assign only didn't help.

[jrafanie]

@LorkScorguar thank you for reporting this regression. I have recreated it in large part because of the details you provided. I hope to get a PR opened today to address it.

The fix will look like this, I just need to add more tests:

diff --git a/app/models/miq_request.rb b/app/models/miq_request.rb
index 14e069e993..8608963093 100644
--- a/app/models/miq_request.rb
+++ b/app/models/miq_request.rb
@@ -12,8 +12,6 @@ class MiqRequest < ApplicationRecord
   has_many   :miq_approvals,     :dependent   => :destroy
   has_many   :miq_request_tasks, :dependent   => :destroy

-  acts_as_miq_taggable
-
   alias_attribute :state, :request_state

   serialize   :options, Hash
diff --git a/lib/rbac/filterer.rb b/lib/rbac/filterer.rb
index b48ebb7e7b..fc68cbc937 100644
--- a/lib/rbac/filterer.rb
+++ b/lib/rbac/filterer.rb
@@ -54,7 +54,7 @@ module Rbac
       VmOrTemplate
     )

-    TAGGABLE_FILTER_CLASSES = CLASSES_THAT_PARTICIPATE_IN_RBAC - %w(EmsFolder) + %w(MiqGroup User Tenant)
+    TAGGABLE_FILTER_CLASSES = CLASSES_THAT_PARTICIPATE_IN_RBAC - %w(EmsFolder MiqRequest) + %w(MiqGroup User Tenant)

     NETWORK_MODELS_FOR_BELONGSTO_FILTER = %w(
       CloudNetwork

[jrafanie]

Fixed the followup bug reported by @LorkScorguar in #17656