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

Use base class only when it is supported by direct rbac #14665

Merged

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Apr 6, 2017

Use base only when it is supported by direct rbac

it is fixing that specialized class model is passed but
rbac is returing objects with the model's base_class.

for example:
Rbac.filtered_object(MiqAeDomain.first, :user => User.first)

before:
returned object is MiqAeNamespace (as parent of MiqAeDomain)
after:
returned object is MiqAeDomain

About fix

I did similar thing as we have here for format of rbac calling:
Rbac.search(:targets => MiqAeDomain, :user => User.first)

@miq-bot add_label rbac, core, bug
cc @PanSpagetka @martinpovolny
@miq-bot assign @gtanzillo

it is fixing that specialized class model is passed but
rbac is returing objects with specialized class model's baseclass.

for example:
Rbac.filtered_object(MiqAeDomain.first, :user => User.first)

before:
returned object is MiqAeNamespace (as parent of MiqAeDomain)

after:
returned object is MiqAeDomain
@lpichler
Copy link
Contributor Author

lpichler commented Apr 6, 2017

@kbrock @gtanzillo question:
as we have usage of base_class here in rbac

it looks like we have some cases when klass dont have method find (https://github.com/ManageIQ/manageiq/blob/master/lib/rbac/filterer.rb#L212 and https://github.com/ManageIQ/manageiq/blob/master/lib/rbac/filterer.rb#L202 )
and then we are trying to use klass.base_class.

Are you aware of such case of rbac call ?
because input class have to be inherit of some ActiveRecord model and
simultaneously it doesn't have respond to method find

class MyModel < ApplicationRecord
end

class MySpecialModel < MyModel
end

then it should be

MySpecialModel.respond_to?(:find) => false
MySpecialModel.respond_to?(:base_class) => true

I did not find such class method in Rbac::Filterer::CLASSES_THAT_PARTICIPATE_IN_RBAC

Rbac::Filterer::CLASSES_THAT_PARTICIPATE_IN_RBAC.map { |x| !x.constantize.respond_to?(:find) && x.constantize.respond_to?(:base_class) } .any?

some maybe we can remove these lines:
https://github.com/ManageIQ/manageiq/blob/master/lib/rbac/filterer.rb#L216
https://github.com/ManageIQ/manageiq/blob/master/lib/rbac/filterer.rb#L202

@@ -41,6 +41,18 @@
end
end

context 'when class is not participate in RBAC' do
Copy link
Member

Choose a reason for hiding this comment

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

when class does not participate...

@lpichler lpichler force-pushed the use_base_class_when_it_is_supported_by_rbac branch from 9a0e6e7 to e314f31 Compare April 7, 2017 11:23
@miq-bot
Copy link
Member

miq-bot commented Apr 7, 2017

Checked commits lpichler/manageiq@f32c23c~...e314f31 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🏆

@kbrock
Copy link
Member

kbrock commented Apr 7, 2017

/cc @mkanoor I was always confused why MiqAeDomain was passed into rbac while it was not in the list of supported rbac classes. Pretty sure you told me but it still confuses me.

@martinpovolny
Copy link
Member

martinpovolny commented Apr 9, 2017

bump ;-) this is crucial for RBAC fixes in the classic-ui as we heavily depend on Rbac.filtered and RBAC.filtered_object

please, review

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 58 Ending Apr 10, 2017 milestone Apr 10, 2017
@gtanzillo gtanzillo merged commit 81cf67c into ManageIQ:master Apr 10, 2017
@lpichler lpichler deleted the use_base_class_when_it_is_supported_by_rbac branch April 10, 2017 13:27
simaishi pushed a commit that referenced this pull request Apr 11, 2017
…ported_by_rbac

Use base class only when it is supported by direct rbac
(cherry picked from commit 81cf67c)
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit 8143a3bfce1f69a6e97c0f5035ecc28cc603425d
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Mon Apr 10 09:21:38 2017 -0400

    Merge pull request #14665 from lpichler/use_base_class_when_it_is_supported_by_rbac
    
    Use base class only when it is supported by direct rbac
    (cherry picked from commit 81cf67c728c3391734709f557ea7d800244d7367)

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.

6 participants