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 acts_as_miq_taggable to AuthPrivateKey #16828

Merged
merged 1 commit into from
Jan 18, 2018

Conversation

syncrou
Copy link
Contributor

@syncrou syncrou commented Jan 16, 2018

The RBAC filtering was not 100% applied to the AuthPrivate key class which
caused tagging to fail. This change allows key_pairs to return correctly when
run through RBAC when being tagged

https://bugzilla.redhat.com/show_bug.cgi?id=1514237

@syncrou
Copy link
Contributor Author

syncrou commented Jan 16, 2018

@miq-bot add_label wip, bug, blocker, providers

@miq-bot assign @gmcculloug

cc - @gtanzillo

@miq-bot miq-bot changed the title Add acts_as_miq_taggable to AuthPrivateKey [WIP] Add acts_as_miq_taggable to AuthPrivateKey Jan 16, 2018
@syncrou
Copy link
Contributor Author

syncrou commented Jan 16, 2018

@gtanzillo - Would you mind taking a look at this PR. The Authentication class is already included in lib/rbac/filterer.rb CLASSES_THAT_PARTICIPATE_IN_RBAC constant, however the inclusion of acts_as_miq_taggable was not. With that change I was able to tag AuthPrivateKey instances in the specs, and the UI would filter key_pairs as expected.

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.

@syncrou Looks good!

@gmcculloug
Copy link
Member

@gtanzillo If Authentication class is already included in CLASSES_THAT_PARTICIPATE_IN_RBAC does it makes sense to move the acts_as_miq_taggable to the base Authentication class or keep it in the subclass AuthPrivateKey?

@gtanzillo
Copy link
Member

@gmcculloug

@gtanzillo If Authentication class is already included in CLASSES_THAT_PARTICIPATE_IN_RBAC does it makes sense to move the acts_as_miq_taggable to the base Authentication class or keep it in the subclass AuthPrivateKey?

I was thinking about that too but didn't see a use case for universally enabling tagging on all authentications. Like provider credentials. That said, I don't think it would hurt anything either.

@syncrou
Copy link
Contributor Author

syncrou commented Jan 16, 2018

@gtanzillo - Thanks GT!

@miq-bot remove_label wip

@miq-bot miq-bot changed the title [WIP] Add acts_as_miq_taggable to AuthPrivateKey Add acts_as_miq_taggable to AuthPrivateKey Jan 16, 2018
@miq-bot miq-bot removed the wip label Jan 16, 2018
@@ -74,7 +74,10 @@
FactoryGirl.create(:classification_cost_center_with_tags)
admin.current_group.entitlement = Entitlement.create!(:filters => {'managed' => [['/managed/cc/001']],
'belongsto' => []})

2.times do
kp = ManageIQ::Providers::CloudManager::AuthKeyPair.create(:name => "auth_1")
Copy link
Member

Choose a reason for hiding this comment

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

I realize this won't matter but if we are going to name the elements with a number should we give them unique names?

Maybe:

2.times do |i|
  kp = ManageIQ::Providers::CloudManager::AuthKeyPair.create(:name => "auth_#{i}")
end

You can even use i + 1 here if you don't want a zero element name. 😄

@syncrou syncrou force-pushed the add_auth_private_key_to_rbac branch from 58cda8c to 0e7c31e Compare January 17, 2018 15:40
The RBAC filtering was not 100% applied to the AuthPrivate key class which
caused tagging to fail.  This change allows key_pairs to return correctly when
run through RBAC when being tagged

https://bugzilla.redhat.com/show_bug.cgi?id=1514237
@syncrou syncrou force-pushed the add_auth_private_key_to_rbac branch from 0e7c31e to feadd7a Compare January 17, 2018 15:42
@miq-bot
Copy link
Member

miq-bot commented Jan 17, 2018

Checked commit syncrou@feadd7a with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

@syncrou syncrou closed this Jan 17, 2018
@syncrou syncrou reopened this Jan 17, 2018
@gmcculloug gmcculloug merged commit fad3529 into ManageIQ:master Jan 18, 2018
@gmcculloug gmcculloug added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 18, 2018
simaishi pushed a commit that referenced this pull request Jan 18, 2018
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit e3e95363ef8eb12cf3937112f4c7b6e01144441d
Author: Greg McCullough <[email protected]>
Date:   Wed Jan 17 19:59:00 2018 -0500

    Merge pull request #16828 from syncrou/add_auth_private_key_to_rbac
    
    Add acts_as_miq_taggable to AuthPrivateKey
    (cherry picked from commit fad3529c00ab1797a8633bc090c964d92b244248)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1532646

simaishi pushed a commit that referenced this pull request Jan 18, 2018
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit bb43558c8e9f9805fecce622281c726fb8ead81d
Author: Greg McCullough <[email protected]>
Date:   Wed Jan 17 19:59:00 2018 -0500

    Merge pull request #16828 from syncrou/add_auth_private_key_to_rbac
    
    Add acts_as_miq_taggable to AuthPrivateKey
    (cherry picked from commit fad3529c00ab1797a8633bc090c964d92b244248)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1533139

syncrou added a commit to syncrou/manageiq that referenced this pull request Jan 22, 2018
The Authentication class is needed in the CLASSES_THAT_PARTICIPATE_IN_RBAC
array in order for the AuthPrivateKey class to participate in RBAC.

PR where this was introduced: ManageIQ#16828
d-m-u pushed a commit to d-m-u/manageiq that referenced this pull request Jun 6, 2018
d-m-u pushed a commit to d-m-u/manageiq that referenced this pull request Jun 6, 2018
The Authentication class is needed in the CLASSES_THAT_PARTICIPATE_IN_RBAC
array in order for the AuthPrivateKey class to participate in RBAC.

PR where this was introduced: ManageIQ#16828
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.

5 participants