Fixes #5261: Adds CRUD permissions for Products and Repositories. #4017
Conversation
| @@ -142,8 +142,7 @@ def available_puppet_modules | |||
| param :id, :identifier, :desc => "content view numeric identifier", :required => true | |||
| def available_puppet_module_names | |||
| current_names = @view.content_view_puppet_modules.map(&:name).reject{|p| p.nil?} | |||
| repo_ids = @view.organization.library.puppet_repositories.readable( | |||
| @view.organization.library).pluck(:pulp_id) | |||
| repo_ids = @view.organization.library.puppet_repositories.pluck(:pulp_id) | |||
There was a problem hiding this comment.
Ensuring these are readable is no longer required here?
There was a problem hiding this comment.
Was already being handled here -
katello/app/models/katello/kt_environment.rb
Line 177 in fc4c745
|
@ehelms -> Why did ya need a direct mapping between org and product. Considering the mapping was available from the provider ? .. Did you just feel it was cleaner/easier ? |
|
@parthaa In order for organization scoping to work with the permissions, the entity needs mapping to the organization directly and given the reduction of provider as an entity, it seemed to make sense that products should be attached to an organization directly. |
|
|
@ehelms sounds good.. Could you add a foreign key rule or a validation rule that some how maintains the the product and the provider map to the same org... |
|
@waldenraines from what page? can you give me a little more of a workflow |
|
Actually ignore the comment.. I see that in the plan for product create you are ensuring the they are the same... |
|
No more issues with this PR from me.. ACK once @waldenraines is satisfied ... |
|
@ehelms sorry, 403 when syncing from the product details page. |
|
@waldenraines updated |
|
@waldenraines forgot to address the index remove issue, please re-test with latest |
| deletable_products = @products.deletable | ||
| undeletable = @products - deletable_products | ||
|
|
||
| deletable_products.each{ |product| product.destroy } |
There was a problem hiding this comment.
tip: deletable_products.each(&:destroy)
|
thanks for updating |
| end | ||
|
|
||
| private | ||
|
|
||
| def find_products | ||
| params.require(:ids) | ||
| @products = params[:ids].map { |id| Product.find_by_cp_id!(id) } | ||
| @products = Product.where(:cp_id => params[:ids]) |
There was a problem hiding this comment.
Looking products up by cp_id is very very bad, at least without scoping on organization as well. They are not unique across organizations. Ideally the client would send the actual product id instead of the cp_id, and we should file an issue for that. In the meantime, we should add .where(:organization_id => @organization.id)
There was a problem hiding this comment.
We'll also have to lock these routes and actions down by requiring an organization_id won't we?
There was a problem hiding this comment.
Yeah, didn't realize that wasn't already available. Feel free leave it as is and file an issue for it. The result could be assignment of sync plans in other orgs!
There was a problem hiding this comment.
Looks like there is already an issue for it - http://projects.theforeman.org/issues/5343
There was a problem hiding this comment.
ah, i'll bump the priority and assign to 1.5
|
APJ pending a story being opened to cover fixing 'content' related items later. |
|
APJ from me as well. |
|
[test] |
1 similar comment
|
[test] |
Fixes #5261: Adds CRUD permissions for Products and Repositories.
No description provided.