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

Fix editing Ansible Credential/Repository for restricted user #17244

Conversation

hstastna
Copy link

@hstastna hstastna commented Apr 3, 2018

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


Move acts_as_miq_taggable to base Authentication class to prevent 500 Internal Server Error when
editing Ansible Credential for restricted user, in Automation > Ansible > Credentials.
The same for Ansible Repository, in Automation > Ansible > Repositories page. The reason is that it looks like that API does not work with classes like ManageIQ::Providers::AutomationManager well so it is needed to add acts_as_miq_taggable to base classes.

Note: This PR addresses new changes related to #17049 (see the comments, there is one unanswered question about the UI and ManageIQ::Providers::AutomationManager class)

Before reproducing:

  • you need to enable Embedded Ansible: https://github.com/himdel/guides/blob/5b4723bc5361d66e344d134e67db58561f154c56/providers/embedded_ansible.md
    or you have to make disabled? method return false, in app/helpers/application_helper/button/embedded_ansible.rb (not recommended, only for urgent needs!)
  • as an admin, tag some credential(s)/repository(ies) from Automation > Ansible > Credentials/Repositories, remember those tags
  • then in Configuration > Access Control > Groups, create a group so that under Assigned Filters in the first tab called My Company Tags, you choose the same tags as tags from previous step
  • then set EvmRole-administrator as a role of that group (not super administrator!) and save the group
  • then create a user with that group from the previous step

Steps to reproduce the bug:

  1. Login as a restricted user, created by the steps from the previous section above
  2. Go to Automation > Ansible > Credentials/Repositories page
  3. Select some credential/repository by checking its checkbox in the left in the list
  4. Under Configuration, choose Edit this Credential/Repository:
    edit_credential

Before: (error occurs)
edit_credential_bad
edit_repo_bad

After: (no error occurred, editing page successfully opened)
edit_credential_ok
edit_credential_save

edit_repo_ok
edit_repo-save

@hstastna
Copy link
Author

hstastna commented Apr 3, 2018

@miq-bot add_label bug, blocker

@hstastna
Copy link
Author

hstastna commented Apr 3, 2018

@miq-bot add_label gaprindashvili/yes

@hstastna hstastna force-pushed the 500_Error_restricted_user_edit_credential_repo branch 6 times, most recently from c6e7bbc to c0e591c Compare April 3, 2018 10:30
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1560525

Move acts_as_miq_taggable to Authentication base class to prevent 500 Internal Server Error
when editing Ansible Credential for restricted user, in Automation > Ansible > Credentials.
@hstastna hstastna force-pushed the 500_Error_restricted_user_edit_credential_repo branch 3 times, most recently from 2673c3a to 83ff177 Compare April 3, 2018 10:37
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1560525

Move acts_as_miq_taggable to ConfigurationScriptSource base class to prevent 500 Internal Server Error
when editing Ansible Repository for restricted user, in Automation > Ansible > Repositories page.
@hstastna hstastna force-pushed the 500_Error_restricted_user_edit_credential_repo branch from 83ff177 to 625f38d Compare April 3, 2018 10:38
Move acts_as_miq_taggable to ConfigurationScriptPayload class to prevent any potential errors
in Automation > Ansible > Playbooks, similarly as in Ansible Credentials/Repositories pages.
@hstastna hstastna force-pushed the 500_Error_restricted_user_edit_credential_repo branch from 625f38d to 5978e08 Compare April 3, 2018 10:39
@miq-bot
Copy link
Member

miq-bot commented Apr 3, 2018

Checked commits hstastna/manageiq@7378ebe~...5978e08 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. ⭐

@lpichler
Copy link
Contributor

lpichler commented Apr 3, 2018

@miq-bot assign @gtanzillo

@agrare agrare merged commit dcde0d6 into ManageIQ:master Apr 3, 2018
@agrare agrare added this to the Sprint 83 Ending Apr 9, 2018 milestone Apr 3, 2018
@agrare agrare assigned agrare and unassigned gtanzillo Apr 3, 2018
simaishi pushed a commit that referenced this pull request Apr 3, 2018
…t_credential_repo

Fix editing Ansible Credential/Repository for restricted user
(cherry picked from commit dcde0d6)

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

simaishi commented Apr 3, 2018

Gaprindashvili backport details:

$ git log -1
commit fc8e1832be9beee0e58865d15981fc3556b513ba
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Apr 3 08:47:34 2018 -0400

    Merge pull request #17244 from hstastna/500_Error_restricted_user_edit_credential_repo
    
    Fix editing Ansible Credential/Repository for restricted user
    (cherry picked from commit dcde0d68c9206129ec53a65609b1035d6a1de76b)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1563263

@jameswnl
Copy link
Contributor

jameswnl commented Apr 3, 2018

I have no problem with moving this to the base models if we do need it.

What's not clear to me is: If UI is operating at the abstraction level of Authentications/ConfigurationScriptSource, then yes, we need to make the base classes tag-able. However, in this case, UI is clearly operating in the space of Ansible/AutomationManager.

Is this is a limitation from the backend (general groups/tags management) or UI is not targeting the right abstraction level?

@lpichler
Copy link
Contributor

lpichler commented Apr 4, 2018

@jameswnl

when edit screen is opened for credentials, this request is fired up

curl --user user:smartvm -i -H "Accept: application/json" -H "Content-Type: application/json" -X GET http://localhost:3000/api/authentications/123

and API works only with base model Authentication(etc.) and this model is pass to RBAC call where method find_tags_by_grouping on the model is called. This is the reason why we need it in base class.

@jameswnl
Copy link
Contributor

jameswnl commented Apr 4, 2018

@lpichler ok, thanks

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.

7 participants