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

Jump to bottom

Add orchestration stacks to RBAC #18308

Merged
merged 1 commit into from
Jul 17, 2019
+35 −11
Merged

Add orchestration stacks to RBAC #18308

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
2 changes: 2 additions & 0 deletions app/models/orchestration_stack.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@ class OrchestrationStack < ApplicationRecord
include CustomActionsMixin
include SupportsFeatureMixin
include CiFeatureMixin
include CloudTenancyMixin

acts_as_miq_taggable

has_ancestry

belongs_to :ext_management_system, :foreign_key => :ems_id
belongs_to :tenant
belongs_to :cloud_tenant

has_many :authentication_orchestration_stacks
has_many :authentications, :through => :authentication_orchestration_stacks
Expand Down
1 change: 1 addition & 0 deletions lib/rbac/filterer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ class Filterer
'MiqRequest' => :descendant_ids,
'MiqRequestTask' => nil, # tenant only
'MiqTemplate' => :ancestor_ids,
'OrchestrationStack' => nil,
Copy link
Member

Choose a reason for hiding this comment

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

Seems templates / generic objects tend to be ancestor_ids,
and core objects tend to be descendant_ids.

I expected this one to be descendant_ids

Copy link
Member

Choose a reason for hiding this comment

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

ok, can we remove this?

a. I think this array is only for tenancy and not cloud tenancy
b. Having a nil in here will not do anything

Also, can you verify that we are not calling into accessible_tenant_ids_strategy for OrchestrationStack?
I'm pretty sure we don't need that

Copy link
Contributor

Choose a reason for hiding this comment

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

let me make clear those things...

a. I think this array is only for tenancy and not cloud tenancy

It works togethers. We need find ids from tenants and then find their equivalents in cloud tenants.

in filterer:

  1. it goes to scope_to_cloud_tenant

  2. then to the TenancyCommonMixin#tenant_id_clause

  3. and then in TenancyCommonMixin#tenant_id_clause it calls tenant_id_clause_format but from CloudTenancyMixin

b. Having a nil in here will not do anything
so form previous answer (2) it is needed to have nil here - it means "current" strategy (it is considering just tenant from logged user - no ancestors or descendants)

Also, can you verify that we are not calling into accessible_tenant_ids_strategy for OrchestrationStack?
I'm pretty sure we don't need that

Yes I verified it when I was looking for previous answers. accessible_tenant_ids_strategy is called also from OrchestrationStack and it is called here

@kbrock does it make sense now ? If you other questions let me know!

thanks!

'Provider' => :ancestor_ids,
'Service' => :descendant_ids,
'ServiceTemplate' => :ancestor_ids,
Expand Down
43 changes: 32 additions & 11 deletions spec/lib/rbac/filterer_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2464,23 +2464,26 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects)
let(:project1_user) { FactoryBot.create(:user, :miq_groups => [project1_group]) }
let(:project1_volume) { FactoryBot.create(:cloud_volume, :ext_management_system => ems_openstack, :cloud_tenant => project1_cloud_tenant) }
let(:project1_flavor) { FactoryBot.create(:flavor, :ext_management_system => ems_openstack) }
let(:project1_orchestration_stack) { FactoryBot.create(:orchestration_stack, :ext_management_system => ems_openstack, :cloud_tenant => project1_cloud_tenant) }
let(:project1_c_t_flavor) { FactoryBot.create(:cloud_tenant_flavor, :cloud_tenant => project1_cloud_tenant, :flavor => project1_flavor) }
let(:project2_tenant) { FactoryBot.create(:tenant, :source_type => 'CloudTenant') }
let(:project2_cloud_tenant) { FactoryBot.create(:cloud_tenant, :source_tenant => project2_tenant, :ext_management_system => ems_openstack) }
let(:project2_group) { FactoryBot.create(:miq_group, :tenant => project2_tenant) }
let(:project2_user) { FactoryBot.create(:user, :miq_groups => [project2_group]) }
let(:project2_volume) { FactoryBot.create(:cloud_volume, :ext_management_system => ems_openstack, :cloud_tenant => project2_cloud_tenant) }
let(:project2_flavor) { FactoryBot.create(:flavor, :ext_management_system => ems_openstack) }
let(:project2_orchestration_stack) { FactoryBot.create(:orchestration_stack, :ext_management_system => ems_openstack, :cloud_tenant => project2_cloud_tenant) }
let(:project2_c_t_flavor) { FactoryBot.create(:cloud_tenant_flavor, :cloud_tenant => project2_cloud_tenant, :flavor => project2_flavor) }
let(:ems_other) { FactoryBot.create(:ems_cloud, :name => 'ems_other', :tenant_mapping_enabled => false) }
let(:volume_other) { FactoryBot.create(:cloud_volume, :ext_management_system => ems_other) }
let(:tenant_other) { FactoryBot.create(:tenant, :source_type => 'CloudTenant') }
let(:cloud_tenant_other) { FactoryBot.create(:cloud_tenant, :source_tenant => tenant_other, :ext_management_system => ems_other) }
let(:flavor_other) { FactoryBot.create(:flavor, :ext_management_system => ems_other) }
let(:orchestration_stack_other) { FactoryBot.create(:orchestration_stack, :ext_management_system => ems_other, :cloud_tenant => cloud_tenant_other) }
let(:c_t_flavor_other) { FactoryBot.create(:cloud_tenant_flavor, :cloud_tenant => cloud_tenant_other, :flavor => flavor_other) }
let!(:all_objects) { [project1_volume, project2_volume, volume_other, cloud_tenant_other, project1_c_t_flavor, project2_c_t_flavor, c_t_flavor_other] }
let!(:all_objects) { [project1_volume, project2_volume, volume_other, cloud_tenant_other, project1_c_t_flavor, project2_c_t_flavor, c_t_flavor_other, project1_orchestration_stack, project2_orchestration_stack, orchestration_stack_other] }

it "lists its own project's objects and other objects where tenant_mapping is not enabled" do
it "lists its own project's objects and other objects where tenant_mapping is enabled" do
ems_openstack.tenant_mapping_enabled = true
ems_openstack.save!
results = described_class.search(:class => CloudVolume, :user => project1_user).first
Expand Down Expand Up @@ -2509,37 +2512,55 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects)

results = described_class.search(:class => Flavor, :user => other_user).first
expect(results).to match_array [flavor_other]

results = described_class.search(:class => OrchestrationStack, :user => project1_user).first
expect(results).to match_array [project1_orchestration_stack, orchestration_stack_other]

results = described_class.search(:class => OrchestrationStack, :user => project2_user).first
expect(results).to match_array [project2_orchestration_stack, orchestration_stack_other]

results = described_class.search(:class => OrchestrationStack, :user => other_user).first
expect(results).to match_array [orchestration_stack_other]
kbrock marked this conversation as resolved.
Show resolved Hide resolved
end

it "all objects are visible to all users when tenant_mapping is not enabled" do
ems_openstack.tenant_mapping_enabled = false
ems_openstack.save!
results = described_class.search(:class => CloudVolume, :user => project1_user).first
expect(results).to match_array [project1_volume, project2_volume, volume_other]
expect(results).to match_array CloudVolume.all

results = described_class.search(:class => CloudVolume, :user => project2_user).first
expect(results).to match_array [project1_volume, project2_volume, volume_other]
expect(results).to match_array CloudVolume.all

results = described_class.search(:class => CloudVolume, :user => owner_user).first
expect(results).to match_array [project1_volume, project2_volume, volume_other]
expect(results).to match_array CloudVolume.all

results = described_class.search(:class => CloudTenant, :user => project1_user).first
expect(results).to match_array [project1_cloud_tenant, project2_cloud_tenant, cloud_tenant_other]
expect(results).to match_array CloudTenant.all

results = described_class.search(:class => CloudTenant, :user => project2_user).first
expect(results).to match_array [project1_cloud_tenant, project2_cloud_tenant, cloud_tenant_other]
expect(results).to match_array CloudTenant.all

results = described_class.search(:class => CloudTenant, :user => other_user).first
expect(results).to match_array [project1_cloud_tenant, project2_cloud_tenant, cloud_tenant_other]
expect(results).to match_array CloudTenant.all

results = described_class.search(:class => Flavor, :user => project1_user).first
expect(results).to match_array [project1_flavor, project2_flavor, flavor_other]
expect(results).to match_array Flavor.all

results = described_class.search(:class => Flavor, :user => project2_user).first
expect(results).to match_array [project1_flavor, project2_flavor, flavor_other]
expect(results).to match_array Flavor.all

results = described_class.search(:class => Flavor, :user => other_user).first
expect(results).to match_array [project1_flavor, project2_flavor, flavor_other]
expect(results).to match_array Flavor.all

results = described_class.search(:class => OrchestrationStack, :user => project1_user).first
expect(results).to match_array OrchestrationStack.all

results = described_class.search(:class => OrchestrationStack, :user => project2_user).first
expect(results).to match_array OrchestrationStack.all

results = described_class.search(:class => OrchestrationStack, :user => other_user).first
expect(results).to match_array OrchestrationStack.all
end
end

Expand Down