From f8c05532cf654cff8e39bded83c25af8fbec281c Mon Sep 17 00:00:00 2001 From: Joe Rafaniello Date: Fri, 29 Jun 2018 10:59:11 -0400 Subject: [PATCH] Remove Request taggable and prevent tag filtering MiqRequest was changed to allow ownership for self service and limited self-service users in ManageIQ #17208, BZ #1545395 This caused a problem if you had tag filters assign to a user's group undefined method `find_tags_by_grouping'. This was fixed in ManageIQ #17466, BZ #1576129, and shipped with: Fine: BZ #1583711 Gaprindindashvili: BZ #1583710 Unfortunately, this second fix to add taggable caused a new bug: users in groups having tag filters could not see their own requests. This commit changes MiqRequest to no longer be taggable, since it's not even taggable in the UI and instead, we add MiqRequest to a list of models that are RBAC'able but not taggable so we don't try to filter MiqRequest based on a user's group tag filters. Credit goes to github user LorkScorguar who reported this issue and provided lots of diagnostics to help us fix this properly. To test this, simply assign managed filters to a user's group, such as /managed/environments/production, create a request for that user and try to see that user's request. They couldn't see it if they received the intermediate fix, #17466, or if they didn't receive that fix, they'd receive the `find_tags_by_grouping` error shown above. For gaprindashvili and fine: Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1596738 For hammer: Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1576129 --- app/models/miq_request.rb | 2 -- lib/rbac/filterer.rb | 2 +- spec/lib/rbac/filterer_spec.rb | 41 ++++++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 3 deletions(-) diff --git a/app/models/miq_request.rb b/app/models/miq_request.rb index 14e069e9931..8608963093a 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 b48ebb7e7ba..fc68cbc937c 100644 --- a/lib/rbac/filterer.rb +++ b/lib/rbac/filterer.rb @@ -54,7 +54,7 @@ class Filterer 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 diff --git a/spec/lib/rbac/filterer_spec.rb b/spec/lib/rbac/filterer_spec.rb index 045ff9a204d..bfa647218e5 100644 --- a/spec/lib/rbac/filterer_spec.rb +++ b/spec/lib/rbac/filterer_spec.rb @@ -122,6 +122,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt 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,46 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt end end + context "with accessible_tenant_ids filtering (strategy = :descendants_id) through" do + it "can see their own request in the same tenant" 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 "a child tenant user in a group with tag filters can see their own request" do + child_group.entitlement = Entitlement.new + child_group.entitlement.set_managed_filters([["/managed/environment/prod"], ["/managed/service_level/silver"]]) + child_group.entitlement.set_belongsto_filters([]) + child_group.save! + + request = FactoryGirl.create(:miq_host_provision_request, :tenant => child_tenant, :requester => child_user) + results = described_class.search(:class => "MiqRequest", :user => child_user).first + expect(results).to match_array [request] + end + + it "can see other's request in the 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) }