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.

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

RBAC is not working correctly for models with :cloud_tenant #13343

Closed
rwsu opened this issue Jan 4, 2017 · 4 comments
Closed

RBAC is not working correctly for models with :cloud_tenant #13343

rwsu opened this issue Jan 4, 2017 · 4 comments

Comments

@rwsu
Copy link
Contributor

rwsu commented Jan 4, 2017

RBAC is working correctly for models that have "belongs_to :tenant" because they include TenancyMixin which defines a "tenant_id_clause" method that provides the correct where clause to Filterer.scope_to_tenant.

RBAC is not working correctly for cloud-based models, or models that have a :cloud_tenant association instead of a :tenant association. For example, cloud tenants who belong to OpenStack project A can see CloudVolumes for all projects. They should only be able to see CloudVolumes that belong to project A only. At the moment, CloudVolumes doesn't implement TenancyMixin so no tenant based filtering are applied.

For Cloud-based models to be scoped correctly to tenants, we can't simply have it include TenancyMixin, because it breaks the SQL generated by Filterer. I think we need to update TenancyMixin to provide the correct scope, filtered to the correct tenant id. The scope needs to be inner joined to cloud_tenant and then to its source_tenant. And then the scope needs to be filtered by the tenant table's id and not by the source model's tenant_id.

@jrafanie
Copy link
Member

jrafanie commented Jan 4, 2017

@rwsu I don't think cloud_tenants behave exactly like our tenants in the other models and unfortunately share the tenant word but aren't the same thing. For example, I'm not sure if the "what you can see in rbac up/down/sidewise in the tenant hierarchy" in the cloud_tenant follows what we have currently for tenant.

If it makes sense, we'll have to extract common features from cloud_tenants and tenants so they can share common behavior.

cc @Ladas @gtanzillo

@Ladas
Copy link
Contributor

Ladas commented Jan 4, 2017

The CloudTenant objects are mapped to Tenant (if you switch it on), then the tenancy should be based on that. I think it should work as you say @rwsu

E.g. going to tenant -> cloud_tenant -> vms, or tenant -> cloud_tenant -> flavors

Where flavors are a rare case we do not cover,

clout tenant has_many :flavors, :through => :cloud_tenant_flavors (so there is a mapping table, not just cloud_tenant_id in flavor)

other models have the mapping table, e.g. cloud_networks, templates, etc.

@lpichler
Copy link
Contributor

lpichler commented Jan 5, 2017

The CloudTenant objects are mapped to Tenant (if you switch it on), then the tenancy should be based on that.

Yes but only for classes in

'CloudSnapshot' => :descendant_ids,
'CloudTenant' => :descendant_ids,
'CloudVolume' => :descendant_ids,
'ExtManagementSystem' => :ancestor_ids,
'MiqAeNamespace' => :ancestor_ids,
'MiqGroup' => :descendant_ids,
'MiqRequest' => :descendant_ids,
'MiqRequestTask' => nil, # tenant only
'MiqTemplate' => :ancestor_ids,
'Provider' => :ancestor_ids,
'ServiceTemplateCatalog' => :ancestor_ids,
'ServiceTemplate' => :ancestor_ids,
'Service' => :descendant_ids,
'Tenant' => :descendant_ids,
'User' => :descendant_ids,
'Vm' => :descendant_ids
and which have TenancyMixin.
(This is not true about CloudSnapshot, CloudTenant, CloudVolume - I think that they are useless here at this moment - they don't have TenancyMixin as well as they are not tied to tenants thru relationship)

I'll will probably need (for one BZ) restrict CloudTenants according to mapped tenants.

I think that it makes sense to use CloudTenant in RBAC similarly as Tenant for objects with cloud_tenants. (but only when tenant mapping is enabled)

@miq-bot
Copy link
Member

miq-bot commented Sep 30, 2017

This issue has been automatically marked as stale because it has not been updated for at least 6 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions!

@miq-bot miq-bot added the stale label Sep 30, 2017
@rwsu rwsu closed this as completed Oct 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants