Check selected Service Catalog Item for RBAC

[romanblanco]

method: servicetemplate_edit
reproduce: Services -> Catalogs -> Catalog Item -> Select Service Catalog Items -> (Edit Selected Item / Add a New Catalog Item / Add a New Catalog Bundle)

[PanSpagetka]

- @record = checked[0] ? find_by_id_filtered(ServiceTemplate, checked[0]) : ServiceTemplate.new
this was right

[PanSpagetka]

this check is not needed here.

[romanblanco]

@PanSpagetka thanks for the review 👍

@martinpovolny after my changes I've realised, this method was not problematic, but anyway, I think we could use the changes, after I took back what was wrong.

I've introduced a method test_item_with_rbac, that will be useful later, and the code should be now readable, working same as before.

[romanblanco]

I'd like to change this methods name to find_item_with_rbac:

• to stop getting comments like

  " ❗️ - Line 64, Col 37 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id_filtered."

• to have similar name as find_checked_items_with_rbac, that does the same, except for 2 or more items

any ideas before I do the change? @martinpovolny @PanSpagetka

[martinpovolny]

I aggree. It's good to rename the method so that Brakeman does not get confused.

[martinpovolny]

@romanblanco : please, fix the failing spec, otherwise looks good 👍

[romanblanco]

@martinpovolny fixed.

[martinpovolny]

   raise _("Selected %{model_name} no longer exists") % {:model_name => ui_lookup(:model => klass.to_s)}
 +    end
 +    Rbac.filtered_object(tested_object, :user => current_user, :named_scope => options[:named_scope]) ||
 +      raise(_("User '%{user_id}' is not authorized to access '%{model}' record id '%{record_id}'") %

Please, unify the error messages for the 2 cases so that we do not leak any info to an unauthorized user.

[romanblanco]

@martinpovolny updated

[miq-bot]

Checked commits romanblanco/manageiq-ui-classic@19d9369~...59def9c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
39 files checked, 10 offenses detected

app/controllers/application_controller.rb

• ❗ - Line 2347, Col 15 - Style/TrailingUnderscoreVariable - Do not use trailing _s in parallel assignment. Prefer filtered, = Rbac.search( :targets => ids.map(&:to_i), :user => current_user, :class => klass, :results_format => :ids).
• ❗ - Line 2348, Col 7 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 2349, Col 7 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 2350, Col 7 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 2351, Col 30 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

app/controllers/catalog_controller.rb

• ❗ - Line 68, Col 7 - Style/MultilineOperationIndentation - Align the operands of an expression in an assignment spanning multiple lines.

app/controllers/host_controller.rb

• ❗ - Line 326, Col 50 - Style/MultilineTernaryOperator - Avoid multi-line ternary operators, use if or unless instead.

app/controllers/report_controller/reports/editor.rb

• ❗ - Line 52, Col 14 - Style/MultilineTernaryOperator - Avoid multi-line ternary operators, use if or unless instead.

app/controllers/vm_common.rb

• ❗ - Line 560, Col 63 - Style/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 721, Col 63 - Style/ExtraSpacing - Unnecessary spacing detected.

[simaishi]

Fine backport details:

$ git log -1
commit 1c67f4cc93c85bab606b9c59ba16098bf01304f9
Author: Martin Povolny <[email protected]>
Date:   Thu Mar 30 15:02:51 2017 +0200

    Merge pull request #779 from romanblanco/fix_rbac_catalog_servicetemplate_edit
    
    Check selected Service Catalog Item for RBAC
    (cherry picked from commit 37c1d94294958970a720c6aee196544562fd131e)

[martinpovolny]

to go with #857