-
Notifications
You must be signed in to change notification settings - Fork 303
Fixes #38656 - Make autocompletion honor permissions #11469
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
base: master
Are you sure you want to change the base?
Conversation
Reviewer's GuideThis patch ensures autocomplete search results respect user view permissions by adding an authorized(:view) scope to the query and includes a test to verify only permitted resources are suggested. Sequence diagram for permission-enforced autocomplete searchsequenceDiagram
actor User
participant Controller
participant ResourceClass
User->>Controller: Request autocomplete suggestions
Controller->>ResourceClass: completer_scope_options(search)
Controller->>ResourceClass: authorized(:view)
Controller->>ResourceClass: where(id: index_relation)
Controller->>ResourceClass: complete_for(search, options)
ResourceClass-->>Controller: Permitted autocomplete items
Controller-->>User: Return filtered suggestions
Class diagram for updated FilteredAutoCompleteSearch concernclassDiagram
class FilteredAutoCompleteSearch {
+auto_complete_search()
}
class ResourceClass {
+completer_scope_options(search)
+authorized(permission)
+where(id)
+complete_for(search, options)
+find_permission_name(:view)
}
FilteredAutoCompleteSearch --> ResourceClass: uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @adamruzicka - I've reviewed your changes - here's some feedback:
- Add a safe fallback in case
find_permission_name(:view)
returns nil so the autocomplete still works for resources without a defined view permission. - Consider chaining
authorized
beforewhere
to let the database prune unauthorized rows earlier and improve query performance. - Include a test for a user with no view permissions to confirm the endpoint returns an empty array rather than raising an error.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add a safe fallback in case `find_permission_name(:view)` returns nil so the autocomplete still works for resources without a defined view permission.
- Consider chaining `authorized` before `where` to let the database prune unauthorized rows earlier and improve query performance.
- Include a test for a user with no view permissions to confirm the endpoint returns an empty array rather than raising an error.
## Individual Comments
### Comment 1
<location> `test/controllers/api/v2/concerns/filtered_auto_complete_search_test.rb:11` </location>
<code_context>
+ setup_controller_defaults_api
+ end
+
+ test "only suggests options the user is allowed to see" do
+ user = users(:one)
+ org = user.organizations.first
+ product1 = FactoryBot.create(:katello_product, :with_provider, organization_id: org.id)
+ _product2 = FactoryBot.create(:katello_product, :with_provider, organization_id: org.id)
+ setup_user('view', 'products', "name = \"#{product1.name}\"")
+
+ get :auto_complete_search, session: set_session_user(:one), params: { search: "name =", organization_id: org.id }
+ assert_predicate response, :successful?
+ suggestions = ActiveSupport::JSON.decode(response.body)
+ assert_equal 1, suggestions.length
+ assert_equal suggestions.first['part'], "name = \"#{product1.name}\""
+ end
+end
</code_context>
<issue_to_address>
Consider adding tests for users with no permissions and for users with full permissions.
Please add tests for users with no view permissions to verify no suggestions are returned, and for users with full permissions to confirm all suggestions are shown. This will ensure comprehensive coverage of permission scenarios.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
test "only suggests options the user is allowed to see" do
user = users(:one)
org = user.organizations.first
product1 = FactoryBot.create(:katello_product, :with_provider, organization_id: org.id)
_product2 = FactoryBot.create(:katello_product, :with_provider, organization_id: org.id)
setup_user('view', 'products', "name = \"#{product1.name}\"")
get :auto_complete_search, session: set_session_user(:one), params: { search: "name =", organization_id: org.id }
assert_predicate response, :successful?
suggestions = ActiveSupport::JSON.decode(response.body)
assert_equal 1, suggestions.length
assert_equal suggestions.first['part'], "name = \"#{product1.name}\""
end
end
=======
test "only suggests options the user is allowed to see" do
user = users(:one)
org = user.organizations.first
product1 = FactoryBot.create(:katello_product, :with_provider, organization_id: org.id)
_product2 = FactoryBot.create(:katello_product, :with_provider, organization_id: org.id)
setup_user('view', 'products', "name = \"#{product1.name}\"")
get :auto_complete_search, session: set_session_user(:one), params: { search: "name =", organization_id: org.id }
assert_predicate response, :successful?
suggestions = ActiveSupport::JSON.decode(response.body)
assert_equal 1, suggestions.length
assert_equal suggestions.first['part'], "name = \"#{product1.name}\""
end
test "returns no suggestions for user with no view permissions" do
user = users(:one)
org = user.organizations.first
product1 = FactoryBot.create(:katello_product, :with_provider, organization_id: org.id)
product2 = FactoryBot.create(:katello_product, :with_provider, organization_id: org.id)
# User has no view permissions
setup_user(nil, 'products', nil)
get :auto_complete_search, session: set_session_user(:one), params: { search: "name =", organization_id: org.id }
assert_predicate response, :successful?
suggestions = ActiveSupport::JSON.decode(response.body)
assert_equal 0, suggestions.length
end
test "returns all suggestions for user with full permissions" do
user = users(:one)
org = user.organizations.first
product1 = FactoryBot.create(:katello_product, :with_provider, organization_id: org.id)
product2 = FactoryBot.create(:katello_product, :with_provider, organization_id: org.id)
# User has full view permissions
setup_user('view', 'products', nil)
get :auto_complete_search, session: set_session_user(:one), params: { search: "name =", organization_id: org.id }
assert_predicate response, :successful?
suggestions = ActiveSupport::JSON.decode(response.body)
# Should return both products
expected_parts = [
"name = \"#{product1.name}\"",
"name = \"#{product2.name}\""
]
assert_equal 2, suggestions.length
assert_equal expected_parts.sort, suggestions.map { |s| s['part'] }.sort
end
end
>>>>>>> REPLACE
</suggested_fix>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
c1ed210
to
700aa89
Compare
700aa89
to
49e9c41
Compare
Loose adaptation of theforeman/foreman#10645
What are the changes introduced in this pull request?
Users could use autocompletion to get details about resources they're not permitted to see, this PR ensures even autocomplete runs through the
authorized(:view_...)
scope.Considerations taken when implementing this change?
Mostly just replicating what's in the Foreman counterpart.
What are the testing steps for this pull request?
See the test
Summary by Sourcery
Enforce authorization in Katello autocompletion so that only view-permitted resources are suggested and add a test to verify this behavior.
Bug Fixes:
authorized(:view)
scope to filter out unauthorized resources during autocompletion.Tests: