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

Adding global filters only possible by (super)admin #6253

Closed
wants to merge 1 commit into from
Closed

Adding global filters only possible by (super)admin #6253

wants to merge 1 commit into from

Conversation

Nadkine
Copy link
Contributor

@Nadkine Nadkine commented Oct 1, 2019

Our tenants are able to add global filters for VM's by loading a global filter (which is already added by an admin) in the 'advanced search ' tab, then editing and saving it. The fact that other tenants can see these filters obviously shouldn't be possible. I think the adding of global filters should only be able for admin and super admin users. The proposed change might not be the most elegant one, but fixes the described bug.

@@ -48,7 +48,7 @@ def adv_search_button_saveid
add_flash(_("Search Name is required"), :error) if params[:button] == 'saveit'
false
else
s = @edit[@expkey].build_search(@edit[:new_search_name], @edit[:search_type], session[:userid])
s = @edit[@expkey].build_search(@edit[:new_search_name], current_user.super_admin_user? || current_user.super_admin_user? ? @edit[:search_type] : nil, session[:userid])
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is current_user.super_admin_user? || current_user.super_admin_user? ? @edit[:search_type] : nil needed 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.

It solves the following bug:

User can load a custom filter made by the admin ..
image

.. and save a changed version of this.
image

Now the filter will be seen by all users, not only of his own tenant.
image

This is because the loaded filter will adopt the search type of the used filter. So when a user loads an global filter, his changed version will also be global. This proposed change fixes this, by only allowing admins and superadmins to save filters as global filters

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. But why would you need to do

current_user.super_admin_user? || current_user.super_admin_user?

?

i.e. why do you need true || true (or false || false) here?

Copy link
Contributor Author

@Nadkine Nadkine Oct 2, 2019

Choose a reason for hiding this comment

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

Oops.. Of course that had to be admin and super_admin. I overlooked it both after your comment and after writing it. I changed it

Adding global filters only for (super)admin
@miq-bot
Copy link
Member

miq-bot commented Oct 2, 2019

Checked commit https://github.com/Nadkine/manageiq-ui-classic/commit/af9b2bc0f8709aac88e65f182f017bb90a1770c4 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

@chessbyte
Copy link
Member

@mzazrivec what is the status of this PR?

@chessbyte
Copy link
Member

@h-kataria if you have a chance, please review
@mzazrivec bump

@@ -48,7 +48,7 @@ def adv_search_button_saveid
add_flash(_("Search Name is required"), :error) if params[:button] == 'saveit'
false
else
s = @edit[@expkey].build_search(@edit[:new_search_name], @edit[:search_type], session[:userid])
s = @edit[@expkey].build_search(@edit[:new_search_name], current_user.admin_user? || current_user.super_admin_user? ? @edit[:search_type] : nil, session[:userid])
Copy link
Contributor

Choose a reason for hiding this comment

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

@Nadkine admin_user? method does not exist, you should only check for current_user.super_admin_user?

@h-kataria
Copy link
Contributor

@Nadkine also need to make sure that the checkbox "Global Search" on the "Save" filter screen is only visible to super admin or an admin user
Screenshot from 2020-02-27 13-54-39

@Nadkine
Copy link
Contributor Author

Nadkine commented Feb 27, 2020

Unfortunately I'm not able to work on this issue anymore

h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request Feb 28, 2020
@h-kataria
Copy link
Contributor

closing in favor of #6723

@h-kataria h-kataria closed this Feb 28, 2020
h-kataria added a commit to h-kataria/manageiq that referenced this pull request Mar 3, 2020
- Any features that are common thru out the UI, are accessible from multiple screens in UI can live in this section. Example of such feature is Advanced search that is available thru out the UI and cannot be added as an individual feature under any one section.
- New feature added in this PR addresses an issue where in the past only Super Admin user and Admin user could add Global filters for everyone to see, but now as we have different types of Admin users it is better to have this as a product feature. Without this fix currently anyone can add global filters in UI.

This PR addresses issues mention in ManageIQ/manageiq-ui-classic#6253
h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request Mar 3, 2020
Changed code to check for newly added product feature to determine whether a user is allowed to add global filters or not, instead of checking if a user is super admin or admin user.

This PR addresses issues mention in ManageIQ#6253
h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request Mar 3, 2020
Changed code to check for newly added product feature to determine whether a user is allowed to add global filters or not, instead of checking if a user is super admin or admin user.

This PR addresses issues mention in ManageIQ#6253
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants