Fixes #31159 - Add authorization for HostBulkActionsController #9006
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Issues: #31159 |
jturel
commented
Oct 23, 2020
| @@ -314,7 +314,9 @@ def find_errata | |||
| end | |||
|
|
|||
| def find_host_collections | |||
| @host_collections = HostCollection.where(:id => params[:host_collection_ids]) | |||
| throw_each_resource_not_found(name: 'host collection', expected_ids: params[:host_collection_ids]) do | |||
There was a problem hiding this comment.
@jlsherrill thoughts on using the block pattern for throw_resource_not_found ?
There was a problem hiding this comment.
whats the alternative? Passing the expected_ids as parameter ?
parthaa
reviewed
Nov 6, 2020
| found_ids = resources.map(&:id) | ||
| missing_ids = expected_ids.map(&:to_i) - found_ids | ||
| missing_ids.each do |id| | ||
| throw_resource_not_found(name: name, id: id) |
There was a problem hiding this comment.
I'd rather have all the missing ids and one error thrown.
jturel
force-pushed
the
host_bulk_actions_auth
branch
from
235064f
to
383755a
Compare
|
@parthaa updated! |
parthaa
reviewed
Nov 10, 2020
…, applicable_errata routes
jturel
commented
Nov 11, 2020
| @@ -3,37 +3,41 @@ module Concerns | |||
| module Api::V2::BulkHostsExtensions | |||
| extend ActiveSupport::Concern | |||
|
|
|||
| def bulk_hosts_relation(permission, org) | |||
There was a problem hiding this comment.
@parthaa after testing it, I don't think it makes much sense to throw error about the missing ids here. The reason being is that the scoped search or restrict parameters can alter the output and so ids which were included may ultimately be excluded. Opted to keep current behavior, but I did a few things to clean this concern up:
- fail at the start if no ids or search given
- framed a few of the
unlessintoiffor comprehension - de-duped some code for the hosts relation
- used
mergefor the query when a scoped search is given + other improvement in that area
None of the tests broke, and I added another one for the .merge usage since we didn't have coverage in that case
jturel
commented
Nov 11, 2020
| search_hosts = search_hosts.where(:organization_id => @organization.id) if @organization | ||
| search_hosts = search_hosts.search_for(bulk_params[:included][:search]) | ||
| if @hosts.any? | ||
| @hosts = ::Host.where(id: @hosts).or(::Host.where(id: search_hosts)) |
There was a problem hiding this comment.
I believe .merge gives us the same functionality here without the ugliness
parthaa
approved these changes
Nov 11, 2020
jturel
added a commit
to jturel/katello
that referenced
this pull request
Nov 17, 2020
…lo#9006) * Fixes #31159 - Add authorization for HostBulkActionsController * Refs #31159 - join ids together in error * Refs #31159 - remove redundant find_hosts call for installable_errata, applicable_errata routes * Refs #31159 - refactor find_bulk_hosts * Refs #31159 - clean up bulk host extensions tests * Refs #31159 - add test for bulk host extensions when no params given (cherry picked from commit 686c9c8)
jturel
added a commit
that referenced
this pull request
Nov 18, 2020
* Fixes #31159 - Add authorization for HostBulkActionsController * Refs #31159 - join ids together in error * Refs #31159 - remove redundant find_hosts call for installable_errata, applicable_errata routes * Refs #31159 - refactor find_bulk_hosts * Refs #31159 - clean up bulk host extensions tests * Refs #31159 - add test for bulk host extensions when no params given (cherry picked from commit 686c9c8)
ranjan
pushed a commit
to ranjan/katello
that referenced
this pull request
Feb 9, 2021
…lo#9006) * Fixes #31159 - Add authorization for HostBulkActionsController * Refs #31159 - join ids together in error * Refs #31159 - remove redundant find_hosts call for installable_errata, applicable_errata routes * Refs #31159 - refactor find_bulk_hosts * Refs #31159 - clean up bulk host extensions tests * Refs #31159 - add test for bulk host extensions when no params given
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses the various resources in HostBulkActionsController and adds a new helper method
throw_each_resource_not_found