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 #30244 - Authorizer error raised when Filter does not have a search condition #7787

Merged
merged 1 commit into from Jul 10, 2020

Conversation

jturel
Copy link
Contributor

@jturel jturel commented Jun 29, 2020

I was testing out a permissions related pr in katello where I had a user configured with the viewer role and the following error was raised:

ScopedSearch::QueryNotSupported: Field '1' not recognized for searching!                                                                                                                      

I'm having trouble figuring out whether this is legitimate, a regression, or something else. I've written a test case showing the failure, and some commented code which fixes the problem, but makes some other tests fail. Ultimately, the error is because we pass a search string of '(1=1)' to the scoped search query builder built from the filter search_condition and it can not parse it. Was able to reproduce as far back as 1.18 (that's as far back as I went)

@jturel jturel force-pushed the build_scoped_search_condition branch from af0ed4b to fc9b483 Compare June 29, 2020 19:53
@jturel jturel changed the title Field '1' not available for search Fixes #30244 - Authorizer error raised when Filter does not have a search condition Jun 29, 2020
@jturel jturel force-pushed the build_scoped_search_condition branch from fc9b483 to 1ee77c7 Compare June 29, 2020 21:14
@jturel
Copy link
Contributor Author

jturel commented Jul 8, 2020

@ezr-ondrej I see you're assigned here ;) any thoughts on this?

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.

This makes sense to me. Scoped search doesn't have any "always true" condition so just omitting seems like the way to go

app/services/authorizer.rb Outdated Show resolved Hide resolved
Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

I think we should never hit the ('1 = 1') case, because of

search_string = build_scoped_search_condition(all_filters.select(&:limited?))

I believe the tests are somehow not running the Filter#nilify_empty_searches, what is your usecase, where do you hit this failure?

app/services/authorizer.rb Outdated Show resolved Hide resolved
app/services/authorizer.rb Outdated Show resolved Hide resolved
app/models/filter.rb Outdated Show resolved Hide resolved
@jturel
Copy link
Contributor Author

jturel commented Jul 8, 2020

@adamruzicka @ezr-ondrej updated. I think I found the root cause. see my comments

Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

One nit, apart of it looks good 👍

@@ -0,0 +1,5 @@
class SetEmptyFilterTaxonomySearchNil < ActiveRecord::Migration[6.0]
def change
Filter.where(taxonomy_search: '').update(taxonomy_search: nil)
Copy link
Member

Choose a reason for hiding this comment

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

I believe we would be better off with update_all as it doesn't go throught the whole ActiveRecord callback hell. It's better to be avoided in migrations.

Suggested change
Filter.where(taxonomy_search: '').update(taxonomy_search: nil)
Filter.where(taxonomy_search: '').update_all(taxonomy_search: nil)

@jturel
Copy link
Contributor Author

jturel commented Jul 10, 2020

@ezr-ondrej updated :)

Comment on lines 107 to 108
filters.reject! { |f| f.search_condition.blank? }
filters.map { |f| "(#{f.search_condition})" }.join(' OR ')
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to avoid modification in place? Might as well chain it altogether, right?

@ezr-ondrej
Copy link
Member

[test foreman] shouldn't be related, but just to be sure...

Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

Thanks @jturel 👍 Will wait for the Jenkins.
I will fix the commit message once merging :)

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