-
Notifications
You must be signed in to change notification settings - Fork 898
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.
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
Fix RBAC call for templates and vms #18128
Conversation
c281525
to
8d2d2e0
Compare
@miq-bot add_label blocker |
331205d
to
1fe68c1
Compare
Seems alright. @lpichler Any thoughts? |
This comment has been minimized.
This comment has been minimized.
["(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 | ||
if tenant.source_id |
There was a problem hiding this comment.
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 !!!
d8b32f5
to
1f0b303
Compare
@alexander-demichev @lpichler was there anything else needed for this PR prior to merge. This is needed for a blocker issue. |
app/models/vm_or_template.rb
Outdated
|
||
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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexander-demichev it means that
if the tenant came from cloud tenant mapping
it will restrict templates only on from current tenant's user and simultaneously with private templates (vms.publicly_available = false
).
and
all public templates will be displayed.
it means that it is change according origin RBAC rule - is it intended ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the behaviour we want, for tenant that comes from cloud tenant mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool and what about templates and VMs which not related to cloud openstack ?
These RBAC (descendant and ancestor) templates and vms (vms.publicly available = nil) should behave in origin way.
we can have also infra providers in Miq and when user will have mapped tenant, what will happen with their VMs ?
and I would expected test in this way:
let's have:
current user with tenant T2 from cloud tenant mapping in tenant tree - Root Tenant -> T1 -> T2 -> T3
VMs(from infra) in tenant T1, T2, T3
Templates(from infra) in tenant T1, T2, T3
expect:
user can see VMs from T2 and T3
and
user can see Templates from T2 and T1
I think you can use on existing test and update user to have mapped tenant.
thanks
@alexander-demichev if this can be backported, can you add the hammer/yes and gaprindashvili/yes |
@miq-bot add_label hammer/yes |
@miq-bot add_label gaprindashvili/yes |
@alexander-demichev @lpichler since this is for a blocker, can either of you say if this is ready to be merged. |
@alexander-demichev @lpichler ping. Was this ready to be merged? |
This may not be ready for merge, we need to ensure with @lpichler, that this PR will not break rules for other providers RBAC calls. |
@lpichler had you a chance to review? |
@alexander-demichev we need to get this to the PR: basically I am restricting your queries with model class and I added specs for it. Maybe it needs some minor changes like add description text,... other thins you can also look how I am pairing cloud tenant with tenant and you can use it in your specs. thanks diff --git a/app/models/vm_or_template.rb b/app/models/vm_or_template.rb
index 4b85e384d5..9d206ac752 100644
--- a/app/models/vm_or_template.rb
+++ b/app/models/vm_or_template.rb
@@ -1761,6 +1761,8 @@ class VmOrTemplate < ApplicationRecord
vms.all?(&:reconfigurable?)
end
+ PUBLIC_TEMPLATE_CLASSES = %w(ManageIQ::Providers::Openstack::CloudManager::Template).freeze
+
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))
@@ -1768,14 +1770,15 @@ class VmOrTemplate < ApplicationRecord
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"
+ 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
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]
+ 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 = "vms.template = true AND vms.tenant_id IN (?)"
- ["#{tenant_templates} OR #{public_templates} OR #{tenant_vms}", template_tenant_ids, vm_tenant_ids]
+ ["#{tenant_templates} OR #{public_templates} OR #{tenant_vms}", template_tenant_ids, PUBLIC_TEMPLATE_CLASSES, vm_tenant_ids]
end
end
diff --git a/spec/lib/rbac/filterer_spec.rb b/spec/lib/rbac/filterer_spec.rb
index 5c286867dc..8d1d946322 100644
--- a/spec/lib/rbac/filterer_spec.rb
+++ b/spec/lib/rbac/filterer_spec.rb
@@ -799,7 +799,7 @@ describe Rbac::Filterer do
context "searching CloudTemplate" do
let(:group) { FactoryGirl.create(:miq_group, :tenant => default_tenant) } # T1
let(:admin_user) { FactoryGirl.create(:user, :role => "super_administrator") }
- let!(:cloud_template_root) { FactoryGirl.create(:template_cloud, :publicly_available => false) }
+ let!(:cloud_template_root) { FactoryGirl.create(:template_openstack, :publicly_available => false) }
let(:tenant_2) { FactoryGirl.create(:tenant, :parent => default_tenant, :source_type => 'CloudTenant') } # T2
let(:group_2) { FactoryGirl.create(:miq_group, :tenant => tenant_2) } # T1
@@ -814,8 +814,8 @@ describe Rbac::Filterer do
context "when user is restricted user" do
let(:tenant_3) { FactoryGirl.create(:tenant, :parent => tenant_2) } # T3
- let!(:cloud_template) { FactoryGirl.create(:template_cloud, :tenant => tenant_3, :publicly_available => true) }
- let!(:volume_template_openstack_1) { FactoryGirl.create(:template_cloud, :tenant => tenant_3, :publicly_available => true) }
+ let!(:cloud_template) { FactoryGirl.create(:template_openstack, :tenant => tenant_3, :publicly_available => true) }
+ let!(:volume_template_openstack_1) { FactoryGirl.create(:template_openstack, :tenant => tenant_3, :publicly_available => true) }
it "returns all public cloud templates and its descendants" do
User.current_user = user_2
@@ -828,8 +828,8 @@ describe Rbac::Filterer do
end
context "should ignore other tenant's private cloud templates" do
- let!(:cloud_template) { FactoryGirl.create(:template_cloud, :tenant => tenant_3, :publicly_available => false) }
- let!(:volume_template_openstack_2) { FactoryGirl.create(:template_cloud, :tenant => tenant_3, :publicly_available => false) }
+ let!(:cloud_template) { FactoryGirl.create(:template_openstack, :tenant => tenant_3, :publicly_available => false) }
+ let!(:volume_template_openstack_2) { FactoryGirl.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)
@@ -843,34 +843,34 @@ describe Rbac::Filterer do
let(:tenant_2) { FactoryGirl.create(:tenant, :parent => default_tenant, :source_type => 'CloudTenant', :source_id => 1) }
it "finds tenant's private cloud templates" do
- cloud_template2 = FactoryGirl.create(:template_cloud, :tenant => tenant_2, :publicly_available => false)
+ cloud_template2 = FactoryGirl.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 = FactoryGirl.create(:template_cloud, :tenant => tenant_2, :publicly_available => false)
- cloud_template3 = FactoryGirl.create(:template_cloud, :tenant => tenant_2, :publicly_available => true)
+ cloud_template2 = FactoryGirl.create(:template_openstack, :tenant => tenant_2, :publicly_available => false)
+ cloud_template3 = FactoryGirl.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 = FactoryGirl.create(:template_cloud, :tenant => tenant_2, :publicly_available => false)
- cloud_template3 = FactoryGirl.create(:template_cloud, :tenant => tenant_2, :publicly_available => true)
- FactoryGirl.create(:template_cloud, :tenant => default_tenant, :publicly_available => false)
+ cloud_template2 = FactoryGirl.create(:template_openstack, :tenant => tenant_2, :publicly_available => false)
+ cloud_template3 = FactoryGirl.create(:template_openstack, :tenant => tenant_2, :publicly_available => true)
+ FactoryGirl.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 = FactoryGirl.create(:template_cloud, :tenant => tenant_2, :publicly_available => false)
- cloud_template3 = FactoryGirl.create(:template_cloud, :tenant => tenant_2, :publicly_available => true)
- cloud_template4 = FactoryGirl.create(:template_cloud, :tenant => default_tenant, :publicly_available => true)
- FactoryGirl.create(:template_cloud, :tenant => default_tenant, :publicly_available => false)
+ cloud_template2 = FactoryGirl.create(:template_openstack, :tenant => tenant_2, :publicly_available => false)
+ cloud_template3 = FactoryGirl.create(:template_openstack, :tenant => tenant_2, :publicly_available => true)
+ cloud_template4 = FactoryGirl.create(:template_openstack, :tenant => default_tenant, :publicly_available => true)
+ FactoryGirl.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])
@@ -2420,6 +2420,44 @@ describe Rbac::Filterer do
end
end
+
+ describe "" do
+ let(:ems_openstack) { FactoryGirl.create(:ems_cloud) }
+ let(:tenant_1) { FactoryGirl.create(:tenant, :source => project1_cloud_tenant) }
+ let(:project1_cloud_tenant) { FactoryGirl.create(:cloud_tenant, :ext_management_system => ems_openstack) }
+
+ let(:tenant_2) { FactoryGirl.create(:tenant, :source => project2_cloud_tenant, :parent => tenant_1) }
+ let(:project2_cloud_tenant) { FactoryGirl.create(:cloud_tenant, :ext_management_system => ems_openstack) }
+
+ let(:project2_group) { FactoryGirl.create(:miq_group, :tenant => tenant_2) }
+ let(:user_2) { FactoryGirl.create(:user, :miq_groups => [project2_group]) }
+
+ let(:tenant_2_without_mapping) { FactoryGirl.create(:tenant, :parent => tenant_1) }
+ let(:project2_group_without_mapping) { FactoryGirl.create(:miq_group, :tenant => tenant_2_without_mapping) }
+ let(:user_2_without_mapping) { FactoryGirl.create(:user, :miq_groups => [project2_group_without_mapping]) }
+
+ let(:tenant_3) { FactoryGirl.create(:tenant, :source => project3_cloud_tenant, :parent => tenant_2) }
+ let(:project3_cloud_tenant) { FactoryGirl.create(:cloud_tenant, :ext_management_system => ems_openstack) }
+
+ let(:ems_infra) { FactoryGirl.create(:ems_vmware) }
+ let!(:vm_tenant_1) { FactoryGirl.create(:vm_infra, :ext_management_system => ems_infra, :tenant => tenant_1) }
+ let!(:vm_tenant_2) { FactoryGirl.create(:vm_infra, :ext_management_system => ems_infra, :tenant => tenant_2) }
+ let!(:vm_tenant_3) { FactoryGirl.create(:vm_infra, :ext_management_system => ems_infra, :tenant => tenant_3) }
+
+ let!(:template_tenant_1) { FactoryGirl.create(:template_infra, :ext_management_system => ems_infra, :tenant => tenant_1) }
+ let!(:template_tenant_2) { FactoryGirl.create(:template_infra, :ext_management_system => ems_infra, :tenant => tenant_2) }
+ let!(:template_tenant_3) { FactoryGirl.create(:template_infra, :ext_management_system => ems_infra, :tenant => tenant_3) }
+
+
+ it "" 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
+ |
8248e2d
to
861aae9
Compare
@alexander-demichev @lpichler was anything else needed here prior to merge. |
Recap: follow up of #17851 but each model related RBAC rule needs to be in "base model classes" to ensure security for API calls. Api query are also on general models like this PR is moving RBAC rule(defined by What previous PR #17851 did#17851: Rule is updating tenant filtering for templates (this change pertains and could affect only
|
@gtanzillo this PR is ready for your review, see summary in #18128 (comment) |
@miq-bot assign @gtanzillo |
This pull request is not mergeable. Please rebase and repush. |
861aae9
to
1e2991f
Compare
Checked commit alexander-demicev@1e2991f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
@miq-bot remove_label unmergeable |
@@ -1761,13 +1761,25 @@ def self.reconfigurable?(ids) | |||
vms.all?(&:reconfigurable?) | |||
end | |||
|
|||
PUBLIC_TEMPLATE_CLASSES = %w(ManageIQ::Providers::Openstack::CloudManager::Template).freeze |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 TemplateCloud
s(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
There was a problem hiding this comment.
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(...))
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Fix RBAC call for templates and vms (cherry picked from commit cd982a4) https://bugzilla.redhat.com/show_bug.cgi?id=1524368
Hammer backport details:
|
Fix RBAC call for templates and vms (cherry picked from commit cd982a4) https://bugzilla.redhat.com/show_bug.cgi?id=1598520
Gaprindashvili backport details:
|
This PR moves RBAC call from template model to VmOrTemplate, because it breaks filtering in UI, problem is in model inheritance. I may not look 'clean', but in VmOrTemplate RBAC call we need to have a condition for handling Vms, Templates and it's descendants like
::VolumeTemplate
.@mansam @lpichler @aufi
https://bugzilla.redhat.com/show_bug.cgi?id=1524368