Skip to content
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

Fix RBAC call for templates and vms #18128

Merged
merged 1 commit into from
Dec 18, 2018
Merged

Fix RBAC call for templates and vms #18128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions app/models/manageiq/providers/cloud_manager/template.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,17 +122,6 @@ 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(self))
tenant = user_or_group.current_tenant

if tenant.source_id
["(vms.template = true AND (vms.tenant_id = (?) AND vms.publicly_available = false OR vms.publicly_available = true))", tenant.id]
else
["(vms.template = true AND (vms.tenant_id IN (?) OR vms.publicly_available = true))", template_tenant_ids] unless template_tenant_ids.empty?
end
end

private

def raise_created_event
Expand Down
16 changes: 14 additions & 2 deletions app/models/vm_or_template.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1761,13 +1761,25 @@ def self.reconfigurable?(ids)
vms.all?(&:reconfigurable?)
end

PUBLIC_TEMPLATE_CLASSES = %w(ManageIQ::Providers::Openstack::CloudManager::Template).freeze
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why the base class is knowledgeable about a public template class from openstack. Why does this need to be in the base class?

because it breaks filtering in UI, problem is in model inheritance.

I don't know what this means. can you please explain?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @lpichler explained here #18128 (comment) why inheritance breaks RBAC filtering for UI, also when using explorer view the RBAC call will be made for the base model, which is vm_or_template. We are trying to apply some custom RBAC rules for openstack provider, so we placed the RBAC call in base model and we want to not change it's behaviour for anything except ManageIQ::Providers::Openstack::CloudManager::Template

@lpichler correct me if I'm wrong :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexander-demichev maybe you can add #18128 (comment) to the description.

@jrafanie if we will leave this RBAC rule (it is restriction) only in TemplateCloud model, we will get this restriction only for API call

/api/<request_for_list_of_template_cloud>

but it will not prevent list of restricted TemplateClouds(restricted by this rule) in this api call:

/api/<request_for_list_of_vm_or_template> - which can any client call.

so to have it in base class is about security and basically it implies general RBAC rule (cc @himdel ):
so that it has a basic RBAC rule (learned from @himdel):
Do not implement RBAC behavior in special models beucase api can call basic models

Copy link
Contributor

@himdel himdel Dec 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, /api/vms will ignore any restriction defined in subclasses of Vm.

I think the same goes for Rbac.filtered(Vm.where(...))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation.

For a future PR... would it make sense to have the subclass register this class into the base class? It seems odd to me for a base class to know about a subclass.

Copy link
Contributor

@himdel himdel Dec 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable 👍 (though I don't know that much about RBAC internals)

Some kind of registration mechanism would keep the logic in the right place, while allowing it to work even when asking for the basest class.

(Assuming different subclasses don't have conflicting ideas about these restrictions, I guess.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, very good point about different subclasses treating this registration thing differently. I'm fine with doing this for now and if we need to do something "like" this for another class, we should really consider a way to register this subclass with the base class.

Alternatively, you could have the subclass implement a method publicly_visible? or something that the base class can then collect to get a list of publicly visible classes. It would require all subclasses are loaded first, but could be a way to "register" them automatically based on an interface.

Either way, this is good for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @jrafanie I am making a note for it.


def self.tenant_id_clause(user_or_group)
template_tenant_ids = MiqTemplate.accessible_tenant_ids(user_or_group, Rbac.accessible_tenant_ids_strategy(MiqTemplate))
vm_tenant_ids = Vm.accessible_tenant_ids(user_or_group, Rbac.accessible_tenant_ids_strategy(Vm))
return if template_tenant_ids.empty? && vm_tenant_ids.empty?

["(vms.template = true AND vms.tenant_id IN (?)) OR (vms.template = false AND vms.tenant_id IN (?))",
template_tenant_ids, vm_tenant_ids]
tenant = user_or_group.current_tenant
tenant_vms = "vms.template = false AND vms.tenant_id IN (?)"
public_templates = "vms.template = true AND vms.publicly_available = true AND vms.type IN (?)"
tenant_templates = "vms.template = true AND vms.tenant_id IN (?)"

if tenant.source_id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexander-demichev

looks great and I wrote summary and during I had a idea to write these condition in more understandable way - i am just using variables:

git diff:

diff --git a/app/models/vm_or_template.rb b/app/models/vm_or_template.rb
index e3babcf6c8..05ef73f54b 100644
--- a/app/models/vm_or_template.rb
+++ b/app/models/vm_or_template.rb
@@ -1766,12 +1766,19 @@ class VmOrTemplate < ApplicationRecord
     vm_tenant_ids       = Vm.accessible_tenant_ids(user_or_group, Rbac.accessible_tenant_ids_strategy(Vm))
     return if template_tenant_ids.empty? && vm_tenant_ids.empty?
     tenant = user_or_group.current_tenant
+
+    tenant_vms       = "vms.template = false AND vms.tenant_id IN (?)"
+    public_templates = "vms.template = true AND vms.publicly_available = true"
+
     if tenant.source_id
-      ["(vms.template = true AND (vms.tenant_id = (?) AND vms.publicly_available = false OR vms.publicly_available = true)) OR (vms.template = false AND vms.tenant_id IN (?))", tenant.id, vm_tenant_ids]
+      private_tenant_templates = "vms.template = true AND vms.tenant_id = (?) AND vms.publicly_available = false"
+
+      ["#{private_tenant_templates} OR #{public_templates} OR #{tenant_vms}", tenant.id, vm_tenant_ids]
     elsif template_tenant_ids.empty?
-      ["vms.template = false AND vms.tenant_id IN (?)", vm_tenant_ids]
+      [tenant_vms, vm_tenant_ids]
     else
-      ["(vms.template = true AND (vms.tenant_id IN (?) OR vms.publicly_available = true)) OR (vms.template = false AND vms.tenant_id IN (?))", template_tenant_ids, vm_tenant_ids]

a there is how it looks like

    tenant_vms       = "vms.template = false AND vms.tenant_id IN (?)"
    public_templates = "vms.template = true AND vms.publicly_available = true"

    if tenant.source_id
      private_tenant_templates = "vms.template = true AND vms.tenant_id = (?) AND vms.publicly_available = false"

      ["#{private_tenant_templates} OR #{public_templates} OR #{tenant_vms}", tenant.id, vm_tenant_ids]
    elsif template_tenant_ids.empty?
      [tenant_vms, vm_tenant_ids]
    else
      tenant_templates = "vms.template = true AND vms.tenant_id IN (?)"
      ["#{tenant_templates} OR #{public_templates} OR #{tenant_vms}", template_tenant_ids, vm_tenant_ids]
    end

it looks to me more understandable, if you like also can you use it ?

I also tried remove each condition and run specs to confirm if we have test coverage - and you did great job!

the only thing which I found that it not covered is if leg:

    elsif template_tenant_ids.empty?
      [tenant_vms, vm_tenant_ids]

so if can also add covered for this it would be great!

thanks !!!

private_tenant_templates = "vms.template = true AND vms.tenant_id = (?) AND vms.publicly_available = false"
tenant_templates += " AND vms.type NOT IN (?)"
["#{private_tenant_templates} OR #{tenant_vms} OR #{tenant_templates} OR #{public_templates}", tenant.id, vm_tenant_ids, template_tenant_ids, PUBLIC_TEMPLATE_CLASSES, PUBLIC_TEMPLATE_CLASSES]
else
["#{tenant_templates} OR #{public_templates} OR #{tenant_vms}", template_tenant_ids, PUBLIC_TEMPLATE_CLASSES, vm_tenant_ids]
end
end

def self.with_ownership
Expand Down
4 changes: 4 additions & 0 deletions spec/factories/vm_or_template.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@
vendor "openstack"
end

factory :volume_template_openstack, :class => "ManageIQ::Providers::Openstack::CloudManager::VolumeTemplate", :parent => :template_cloud do
vendor "openstack"
end

factory :miq_template do
name "ubuntu-16.04-stable"
location "Minneapolis, MN"
Expand Down
74 changes: 58 additions & 16 deletions spec/lib/rbac/filterer_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,7 +799,7 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt
context "searching CloudTemplate" do
let(:group) { FactoryBot.create(:miq_group, :tenant => default_tenant) } # T1
let(:admin_user) { FactoryBot.create(:user, :role => "super_administrator") }
let!(:cloud_template_root) { FactoryBot.create(:template_cloud, :publicly_available => false) }
let!(:cloud_template_root) { FactoryBot.create(:template_openstack, :publicly_available => false) }

let(:tenant_2) { FactoryBot.create(:tenant, :parent => default_tenant, :source_type => 'CloudTenant') } # T2
let(:group_2) { FactoryBot.create(:miq_group, :tenant => tenant_2) } # T1
Expand All @@ -814,20 +814,26 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt

context "when user is restricted user" do
let(:tenant_3) { FactoryBot.create(:tenant, :parent => tenant_2) } # T3
let!(:cloud_template) { FactoryBot.create(:template_cloud, :tenant => tenant_3, :publicly_available => true) }
let!(:cloud_template) { FactoryBot.create(:template_openstack, :tenant => tenant_3, :publicly_available => true) }
let!(:volume_template_openstack_1) { FactoryBot.create(:template_openstack, :tenant => tenant_3, :publicly_available => true) }

it "returns all public cloud templates" do
it "returns all public cloud templates and its descendants" do
User.current_user = user_2

results = described_class.filtered(VmOrTemplate, :user => user_2)
expect(results).to match_array([cloud_template, cloud_template_root, volume_template_openstack_1])

results = described_class.filtered(TemplateCloud, :user => user_2)
expect(results).to match_array([cloud_template, cloud_template_root])
expect(results).to match_array([cloud_template, cloud_template_root, volume_template_openstack_1])
end

context "should ignore other tenant's private cloud templates" do
let!(:cloud_template) { FactoryBot.create(:template_cloud, :tenant => tenant_3, :publicly_available => false) }
let!(:cloud_template) { FactoryBot.create(:template_openstack, :tenant => tenant_3, :publicly_available => false) }
let!(:volume_template_openstack_2) { FactoryBot.create(:template_openstack, :tenant => tenant_3, :publicly_available => false) }
it "returns public templates" do
User.current_user = user_2
results = described_class.filtered(TemplateCloud, :user => user_2)
expect(results).to match_array([cloud_template_root])
expect(results).to match_array([cloud_template_root, volume_template_openstack_1])
end
end
end
Expand All @@ -837,34 +843,34 @@ def combine_filtered_ids(user_filtered_ids, belongsto_filtered_ids, managed_filt
let(:tenant_2) { FactoryBot.create(:tenant, :parent => default_tenant, :source_type => 'CloudTenant', :source_id => 1) }

it "finds tenant's private cloud templates" do
cloud_template2 = FactoryBot.create(:template_cloud, :tenant => tenant_2, :publicly_available => false)
cloud_template2 = FactoryBot.create(:template_openstack, :tenant => tenant_2, :publicly_available => false)
User.current_user = user_2
results = described_class.filtered(TemplateCloud, :user => user_2)
expect(results).to match_array([cloud_template2])
end

it "finds tenant's private and public cloud templates" do
cloud_template2 = FactoryBot.create(:template_cloud, :tenant => tenant_2, :publicly_available => false)
cloud_template3 = FactoryBot.create(:template_cloud, :tenant => tenant_2, :publicly_available => true)
cloud_template2 = FactoryBot.create(:template_openstack, :tenant => tenant_2, :publicly_available => false)
cloud_template3 = FactoryBot.create(:template_openstack, :tenant => tenant_2, :publicly_available => true)
User.current_user = user_2
results = described_class.filtered(TemplateCloud, :user => user_2)
expect(results).to match_array([cloud_template2, cloud_template3])
end

it "ignores other tenant's private templates" do
cloud_template2 = FactoryBot.create(:template_cloud, :tenant => tenant_2, :publicly_available => false)
cloud_template3 = FactoryBot.create(:template_cloud, :tenant => tenant_2, :publicly_available => true)
FactoryBot.create(:template_cloud, :tenant => default_tenant, :publicly_available => false)
cloud_template2 = FactoryBot.create(:template_openstack, :tenant => tenant_2, :publicly_available => false)
cloud_template3 = FactoryBot.create(:template_openstack, :tenant => tenant_2, :publicly_available => true)
FactoryBot.create(:template_openstack, :tenant => default_tenant, :publicly_available => false)
User.current_user = user_2
results = described_class.filtered(TemplateCloud, :user => user_2)
expect(results).to match_array([cloud_template2, cloud_template3])
end

it "finds other tenant's public templates" do
cloud_template2 = FactoryBot.create(:template_cloud, :tenant => tenant_2, :publicly_available => false)
cloud_template3 = FactoryBot.create(:template_cloud, :tenant => tenant_2, :publicly_available => true)
cloud_template4 = FactoryBot.create(:template_cloud, :tenant => default_tenant, :publicly_available => true)
FactoryBot.create(:template_cloud, :tenant => default_tenant, :publicly_available => false)
cloud_template2 = FactoryBot.create(:template_openstack, :tenant => tenant_2, :publicly_available => false)
cloud_template3 = FactoryBot.create(:template_openstack, :tenant => tenant_2, :publicly_available => true)
cloud_template4 = FactoryBot.create(:template_openstack, :tenant => default_tenant, :publicly_available => true)
FactoryBot.create(:template_openstack, :tenant => default_tenant, :publicly_available => false)
User.current_user = user_2
results = described_class.filtered(TemplateCloud, :user => user_2)
expect(results).to match_array([cloud_template2, cloud_template3, cloud_template4])
Expand Down Expand Up @@ -2288,6 +2294,42 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects)
end
end

describe "using right RBAC rules to VM and Templates" do
let(:ems_openstack) { FactoryBot.create(:ems_cloud) }
let(:tenant_1) { FactoryBot.create(:tenant, :source => project1_cloud_tenant) }
let(:project1_cloud_tenant) { FactoryBot.create(:cloud_tenant, :ext_management_system => ems_openstack) }

let(:tenant_2) { FactoryBot.create(:tenant, :source => project2_cloud_tenant, :parent => tenant_1) }
let(:project2_cloud_tenant) { FactoryBot.create(:cloud_tenant, :ext_management_system => ems_openstack) }

let(:project2_group) { FactoryBot.create(:miq_group, :tenant => tenant_2) }
let(:user_2) { FactoryBot.create(:user, :miq_groups => [project2_group]) }

let(:tenant_2_without_mapping) { FactoryBot.create(:tenant, :parent => tenant_1) }
let(:project2_group_without_mapping) { FactoryBot.create(:miq_group, :tenant => tenant_2_without_mapping) }
let(:user_2_without_mapping) { FactoryBot.create(:user, :miq_groups => [project2_group_without_mapping]) }

let(:tenant_3) { FactoryBot.create(:tenant, :source => project3_cloud_tenant, :parent => tenant_2) }
let(:project3_cloud_tenant) { FactoryBot.create(:cloud_tenant, :ext_management_system => ems_openstack) }

let(:ems_infra) { FactoryBot.create(:ems_vmware) }
let!(:vm_tenant_1) { FactoryBot.create(:vm_infra, :ext_management_system => ems_infra, :tenant => tenant_1) }
let!(:vm_tenant_2) { FactoryBot.create(:vm_infra, :ext_management_system => ems_infra, :tenant => tenant_2) }
let!(:vm_tenant_3) { FactoryBot.create(:vm_infra, :ext_management_system => ems_infra, :tenant => tenant_3) }

let!(:template_tenant_1) { FactoryBot.create(:template_infra, :ext_management_system => ems_infra, :tenant => tenant_1) }
let!(:template_tenant_2) { FactoryBot.create(:template_infra, :ext_management_system => ems_infra, :tenant => tenant_2) }
let!(:template_tenant_3) { FactoryBot.create(:template_infra, :ext_management_system => ems_infra, :tenant => tenant_3) }

it "finds records according to RBAC" do
results = described_class.filtered(Vm, :user => user_2)
expect(results.ids).to match_array([vm_tenant_2.id, vm_tenant_3.id])

results = described_class.filtered(MiqTemplate, :user => user_2)
expect(results.ids).to match_array([template_tenant_1.id, template_tenant_2.id])
end
end

it ".apply_rbac_directly?" do
expect(described_class.new.send(:apply_rbac_directly?, Vm)).to be_truthy
expect(described_class.new.send(:apply_rbac_directly?, Rbac)).not_to be
Expand Down
28 changes: 0 additions & 28 deletions spec/models/manageiq/providers/cloud_manager/template_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,32 +8,4 @@
subject.post_create_actions
end
end

let(:root_tenant) do
Tenant.seed
end

let(:default_tenant) do
root_tenant
Tenant.default_tenant
end

describe "miq_group" do
let(:user) { FactoryBot.create(:user, :userid => 'user', :miq_groups => [tenant_group]) }
let(:tenant) { FactoryBot.build(:tenant, :parent => default_tenant) }
let(:tenant_users) { FactoryBot.create(:miq_user_role, :name => "tenant-users") }
let(:tenant_group) { FactoryBot.create(:miq_group, :miq_user_role => tenant_users, :tenant => tenant) }
let(:cloud_template_1) { FactoryBot.create(:class => "TemplateCloud") }

it "finds correct tenant id clause when tenant doesn't have source_id" do
User.current_user = user
expect(TemplateCloud.tenant_id_clause(user)).to eql ["(vms.template = true AND (vms.tenant_id IN (?) OR vms.publicly_available = true))", [default_tenant.id, tenant.id]]
end

it "finds correct tenant id clause when tenant has source_id" do
User.current_user = user
tenant.source_id = 1
expect(TemplateCloud.tenant_id_clause(user)).to eql ["(vms.template = true AND (vms.tenant_id = (?) AND vms.publicly_available = false OR vms.publicly_available = true))", tenant.id]
end
end
end
28 changes: 28 additions & 0 deletions spec/models/manageiq/providers/cloud_manager/vm_or_template_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,32 @@
expect(described_class.archived).to match_array([vm, t])
end
end

let(:root_tenant) do
Tenant.seed
end

let(:default_tenant) do
root_tenant
Tenant.default_tenant
end

describe "miq_group" do
let(:user) { FactoryGirl.create(:user, :userid => 'user', :miq_groups => [tenant_group]) }
let(:tenant) { FactoryGirl.build(:tenant, :parent => default_tenant) }
let(:tenant_users) { FactoryGirl.create(:miq_user_role, :name => "tenant-users") }
let(:tenant_group) { FactoryGirl.create(:miq_group, :miq_user_role => tenant_users, :tenant => tenant) }
let(:cloud_template_1) { FactoryGirl.create(:class => "TemplateCloud") }

it "finds correct tenant id clause when tenant has source_id" do
User.current_user = user
tenant.source_id = 1
expect(VmOrTemplate.tenant_id_clause(user)).to eql ["vms.template = true AND vms.tenant_id = (?) AND vms.publicly_available = false OR vms.template = false AND vms.tenant_id IN (?) OR vms.template = true AND vms.tenant_id IN (?) AND vms.type NOT IN (?) OR vms.template = true AND vms.publicly_available = true AND vms.type IN (?)", tenant.id, [tenant.id], [root_tenant.id, tenant.id], ["ManageIQ::Providers::Openstack::CloudManager::Template"], ["ManageIQ::Providers::Openstack::CloudManager::Template"]]
end

it "finds correct tenant id clause when tenant doesn't have source_id" do
User.current_user = user
expect(VmOrTemplate.tenant_id_clause(user)).to eql ["vms.template = true AND vms.tenant_id IN (?) OR vms.template = true AND vms.publicly_available = true AND vms.type IN (?) OR vms.template = false AND vms.tenant_id IN (?)", [root_tenant.id, tenant.id], ["ManageIQ::Providers::Openstack::CloudManager::Template"], [tenant.id]]
end
end
end
2 changes: 1 addition & 1 deletion spec/models/mixins/cloud_tenancy_mixin_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
let(:tenant_group) { FactoryBot.create(:miq_group, :miq_user_role => tenant_users, :tenant => tenant) }

it "finds correct tenant id clause for regular tenants" do
expect(VmOrTemplate.tenant_id_clause(user)).to eql ["(vms.template = true AND vms.tenant_id IN (?)) OR (vms.template = false AND vms.tenant_id IN (?))", [default_tenant.id, tenant.id], [tenant.id]]
expect(VmOrTemplate.tenant_id_clause(user)).to eql ["vms.template = true AND vms.tenant_id IN (?) OR vms.template = true AND vms.publicly_available = true AND vms.type IN (?) OR vms.template = false AND vms.tenant_id IN (?)", [default_tenant.id, tenant.id], ["ManageIQ::Providers::Openstack::CloudManager::Template"], [tenant.id]]

query = VmOrTemplate.where(VmOrTemplate.tenant_id_clause(user))
expect { query.load }.not_to raise_error
Expand Down