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

Tenant admin should not be able to create groups in other tenants. #13483

Merged
merged 1 commit into from Jan 17, 2017

Conversation

martinpovolny
Copy link

@martinpovolny martinpovolny commented Jan 13, 2017

ManageIQ/manageiq-ui-classic#134

This is only a part of the fix. The 2nd part needs fixing on the
manageiq-ui-classic side.

Issue: ManageIQ/manageiq-ui-classic#134

ManageIQ/manageiq-ui-classic#151

@miq-bot
Copy link
Member

miq-bot commented Jan 13, 2017

Checked commit martinpovolny@f60f606 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 4 offenses detected

app/models/tenant.rb

spec/models/tenant_spec.rb

@martinpovolny
Copy link
Author

The test failure is unrelated. Ping @kbrock

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

This looks nice martin.

Just a few minor changes.
Do we test where you have access to the child but not the parent (I don't think this will ever be the case, but the double query in tenant_and_project_names suggests it may be so)

all_tenants_and_projects = Tenant.in_my_region.select(:id, :ancestry, :divisible, :use_config_for_attributes, :name)
tenants_by_id = all_tenants_and_projects.index_by(&:id)

tenants_and_projects = Rbac.filtered(Tenant.in_my_region.select(:id, :ancestry, :divisible, :use_config_for_attributes, :name))
Copy link
Member

Choose a reason for hiding this comment

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

I think it is better to pass the local variable into rbac (could you verify it is not worse?):

tenants_and_projects = Rbac.filtered(all_tenants_and_projects)

bummed about the double query here, but looks necessary

Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me why all_tenants_and_projects can't be passed into Rbac.filtered. @martinpovolny, @kbrock?

@@ -854,16 +859,22 @@
stub_settings(:server => {:company => "root"})
end

#before(:all) do
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer create_guid_miq_server_zone over

allow(user).to receive(:get_timezone).and_return("UTC")

if that fixes things

@martinpovolny
Copy link
Author

I am using the parent to construct the name path to the child. To keep the functionality I need to access the names of the parents that the user does not have access to.

Therefor I test the RBAC in one branch and not the other one.

And for the same reason I need the two queries.

@martinpovolny
Copy link
Author

I changed the spec.

ManageIQ/manageiq-ui-classic#134

This is only a part of the fix. The 2nd part needs fixing on the
manageiq-ui-classic side.
@martinpovolny
Copy link
Author

martinpovolny commented Jan 14, 2017

@kbrock : I reverted the changed spec, because the method that you suggested seeds some tenants and breaks many of the tests.

@kbrock
Copy link
Member

kbrock commented Jan 16, 2017

👍 LGTM

@martinpovolny
Copy link
Author

@Fryguy, @dclarizio : merge, please?

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@gtanzillo gtanzillo added this to the Sprint 53 Ending Jan 30, 2017 milestone Jan 17, 2017
@gtanzillo gtanzillo merged commit be680ff into ManageIQ:master Jan 17, 2017
@simaishi
Copy link
Contributor

@martinpovolny Can you please create a BZ if it doesn't already exist?

simaishi pushed a commit that referenced this pull request Jan 23, 2017
Tenant admin should not be able to create groups in other tenants.
(cherry picked from commit be680ff)

https://bugzilla.redhat.com/show_bug.cgi?id=1415217
@simaishi
Copy link
Contributor

Euwe backport details:

$ git log -1
commit a4ba5f23a4e77e631dca2f9f877608a088ad8d0c
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Tue Jan 17 08:59:45 2017 -0500

    Merge pull request #13483 from martinpovolny/group_tenant_limit
    
    Tenant admin should not be able to create groups in other tenants.
    (cherry picked from commit be680ff7bd88736d2a1d0a2f6343bc233a1dc944)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1415217

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants