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

Fixes #37531 - Autocomplete feature for search shows content from forbidden organization for user #10197

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Thorben-D
Copy link

Currently, the suggestions provided by the auto-complete feature of the search-bars include content from different organizations. This PR fixes this by using the (yet to be merged) enhanced_filter function of scoped_search.

Detailed problem description:

The auto-complete functionality is handled by the action auto_complete_search of AutoCompleteSearch.
The action works by first inferring the Model (DB-Table) from the name of the controller it has been called from and then forming valid SQL queries, which are shown in its own query-language.
Since permissions are handled on a per-action basis, if a role permits a user to call auto_complete_search from a controller X, he is given de-facto read access to the whole DB-Table X.
It has not been tested, whether the scope may be expanded past X by injecting joins, etc..

Changes:
This PR filters the auto-complete suggestions by organization and only displays suggestions with content belonging to the user's current organization.
This does NOT fix the root issue, which is scoped_search not properly leveraging the Foreman permission system, but provides an interim solution until an implementation is found that respects the Foreman permission model.

Relations:

…bidden organization for user

Fix an issue that caused content from all organizations to be shown
through the auto-complete feature by using "enhanced_filter" of scoped_search.
Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

I was curious why this isn't an issue for other resources, such as hostgroups (going to Hosts > All hosts and searching for hostgroup = only gives you hostgroups from the current organization). Turns out hostgroups define a default_scope[1], which honors currently set taxonomies. This is also honored by scoped_search's autocomplete.

In general I would like to avoid solving the same problem in two different ways within a single codebase. Considering probably all katello models should belong to exactly one organization and probably all of them inherit from the same superclass, it should be relatively straightforward to craft a single default scope that would universally apply the scoping we expect and would be honored by scoped_search's autocomplete.

On the other hand, the default_scope way would affect much larger area as all active record queries would go through it, unless explicitly unscoped.

[1] - https://github.com/theforeman/foreman/blob/develop/app/models/hostgroup.rb#L52

@@ -9,7 +9,7 @@ module Foreman::Controller::AutoCompleteSearch
def auto_complete_search
begin
model = (controller_name == "hosts") ? Host::Managed : model_of_controller
@items = model.complete_for(params[:search], {:controller => controller_name})
@items = model.complete_for(params[:search], {:controller => controller_name, :enhanced_filter => {:has_column => "organization_id", :filter => {:organization_id => Organization.current.id}}})
Copy link
Contributor

Choose a reason for hiding this comment

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

Katello has its own auto_complete_search definition, this change would need to happen there as well https://github.com/Katello/katello/blob/16440c3907d6b1f64c6edc2a2dedc213085436e0/app/controllers/katello/concerns/filtered_auto_complete_search.rb#L8 . Things like Content > Subscriptions and search for "name = " still offers subscriptions from other organizations.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Thorben-D can you please have a look at the last comments?

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