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

Add method to allow access for tenant quotas #17926

Merged
merged 1 commit into from
Sep 6, 2018

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Aug 30, 2018

In DB
Tenant has many Tenant Quotas

let's have tenant tree (in brackets are users)

My Company (S1)
 |-T1  (U1)
   |- T2 (U2)
   |- T3 (U3)

and users S1 (with super-admin role) and U1 (with tenant-admin role)

From BZ is requested that user with tenant-admin role should not be able manage quotas from his tenant and ancestor but only from his children otherwise as before.
manage means all operations:read, create, edit, delete.

Example:
U1 can manage quotas just for T2 and T3.

Technically

This rule doesn't fit to our existing models RBAC or MiqProductFeature permissions. It is like MiqProductFeature permission with some special RBAC behaviour. But maybe I just need different point of view.

Suggested next steps

This method is planned to be added to restrict all needed parts in the application:
UI: to toolbar buttons(create, edit, delete), templates(view)
API: CRUD

Note: to include it in RBAC could eventually help partly but after moment when quotas are created and quotas are created - at moment when you press button SAVE in form of tenant quotas - it is late. But we can include to ensure view operations for tenant_quotas like RBAC::Filterer.filtered(TenantQuotas)

At this moment I just created this backend method and I am opened discussion, ideas comments, if needed.

TODOs

  • add to RBAC
  • add to API
  • add to UI

Links

@miq-bot assign @gtanzillo

@lpichler lpichler force-pushed the restrict_tenant_quotas branch 3 times, most recently from ca7a0b2 to 92f916f Compare August 30, 2018 15:01
let(:tenant_admin_role) { FactoryGirl.create(:miq_user_role, :features => MiqProductFeature::TENANT_ADMIN_FEATURE) }

let(:tenant_1) { FactoryGirl.create(:tenant, :parent => root_tenant) }
let(:tenant_1_1) { FactoryGirl.create(:tenant, :name => 'Tenant 2', :parent => tenant_1) }
Copy link
Member

Choose a reason for hiding this comment

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

Please don't specify :name since there is a sequence on the factory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


let(:tenant_1) { FactoryGirl.create(:tenant, :parent => root_tenant) }
let(:tenant_1_1) { FactoryGirl.create(:tenant, :name => 'Tenant 2', :parent => tenant_1) }
let(:tenant_1_2) { FactoryGirl.create(:tenant, :name => 'Tenant 3', :parent => tenant_1, :divisible => false) }
Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

let(:tenant_1_2) { FactoryGirl.create(:tenant, :name => 'Tenant 3', :parent => tenant_1, :divisible => false) }

let(:group_tenant_1_tenant_admin) { FactoryGirl.create(:miq_group, :miq_user_role => tenant_admin_role, :tenant => tenant_1) }
let(:user_tenant_1_tenant_admin) { FactoryGirl.create(:user, :userid => 'user', :miq_groups => [group_tenant_1_tenant_admin]) }
Copy link
Member

Choose a reason for hiding this comment

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

Should :userid be moved to a sequence in the factory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is sequence for, so removing it from here

let(:user_tenant_1_tenant_admin) { FactoryGirl.create(:user, :userid => 'user', :miq_groups => [group_tenant_1_tenant_admin]) }

let(:group_tenant_1_super_admin) { FactoryGirl.create(:miq_group, :miq_user_role => super_admin_role, :tenant => tenant_1) }
let(:user_tenant_1_super_admin) { FactoryGirl.create(:user, :userid => 'user', :miq_groups => [group_tenant_1_super_admin]) }
Copy link
Member

Choose a reason for hiding this comment

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

Does this work? We have a validation that userid needs to be unique within the region

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was working because of lazy evaluation of let but we have sequence here for it so removing

@lpichler
Copy link
Contributor Author

@bdunne I added your suggestions, thanks!

@miq-bot
Copy link
Member

miq-bot commented Aug 31, 2018

Checked commit lpichler@4ec38c7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

@bdunne bdunne merged commit 7707eda into ManageIQ:master Sep 6, 2018
@bdunne bdunne added this to the Sprint 94 Ending Sept 10, 2018 milestone Sep 6, 2018
@bdunne bdunne assigned bdunne and unassigned gtanzillo Sep 6, 2018
@lpichler lpichler deleted the restrict_tenant_quotas branch September 6, 2018 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants