Fix storage_manager_id when adding a new cloud volume

[AparnaKarve]

The error below is seen when a new Cloud Volume is added from the Storage Manager Summary screen,

[----] F, [2017-04-18T17:33:46.472740 #2526:3fedbc66de1c] FATAL -- : Error caught: [RuntimeError] Unauthorized object or action
/Users/aparnakarve/rh/master/manageiq/plugins/manageiq-ui-classic/app/controllers/application_controller.rb:2057:in `assert_rbac'
/Users/aparnakarve/rh/master/manageiq/plugins/manageiq-ui-classic/app/controllers/mixins/checked_id_mixin.rb:83:in `find_id_with_rbac'
/Users/aparnakarve/rh/master/manageiq/plugins/manageiq-ui-classic/app/controllers/ems_common.rb:460:in `button'

Issue introduced in 9f513f9

@romanblanco Please review.

https://bugzilla.redhat.com/show_bug.cgi?id=1445095

[miq-bot]

Checked commit AparnaKarve@b04abb2 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍰

[gberginc]

Thanks @AparnaKarve; I've tested and it works nicely.

[romanblanco]

@AparnaKarve this does fix the failing, but this way, the RBAC check is not done.

assert_rbac(CloudVolume, Array.wrap(params[:id]))

I believe that this code should pass without such issues 🤔 . I'm trying to find out, why is the error happening.

EDIT: The class should be probably different, It seems that ManageIQ::Providers::StorageManager::CinderManager is the right one, still testing

[martinpovolny]

Please, don't remove the check. Fix it by passing in the correct class instead.

[gberginc]

@romanblanco CinderManager is not ok. We now also have Amazon EBS storage manager. So either ManageIQ::Providers::StorageManager or even ExtManagementSystem work.

diff --git a/app/controllers/ems_common.rb b/app/controllers/ems_common.rb
index d1c9ee8d4..d510b14ee 100644
--- a/app/controllers/ems_common.rb
+++ b/app/controllers/ems_common.rb
@@ -457,7 +457,7 @@ module EmsCommon
     elsif params[:pressed] == "cloud_volume_new"
       javascript_redirect :controller         => "cloud_volume",
                           :action             => "new",
-                          :storage_manager_id => find_id_with_rbac(CloudVolume, params[:id])
+                          :storage_manager_id => find_id_with_rbac(ExtManagementSystem, params[:id])
     elsif params[:pressed] == "cloud_volume_attach"
       javascript_redirect :controller => "cloud_volume",
                           :action     => "attach",

[martinpovolny]

Oh well, this is the redirect code only. If we have RBAC handled correctly on the receiving side -- cloud_volume_attach then we can probably skip the check here (at least for now). Otherwise we can look for the code to identify the correct model there (in cloud_volume_attach).

[gberginc]

Yes, this code is used just to pre-set the storage manager for which cloud volumes are currently being displayed. This can be seen at ~2:05 in this video: https://xlab.koofr.net/links/f1bc8422-1606-4652-8ee7-dff76dd8339d (the storage manager is set and disabled based on the storage_manager_id).

The receiving end uses

    if params[:storage_manager_id]
      @storage_manager = find_record_with_rbac(ExtManagementSystem, params[:storage_manager_id])
    end

[martinpovolny]

Thanks, @gberginc!

@romanblanco, @AparnaKarve : can you, please, fix this as @gberginc indicated?

Thx!

[AparnaKarve]

@martinpovolny @romanblanco, As @gberginc indicated, the receiving end already does the find_record_with_rbac for us here --
https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/cloud_volume_controller.rb#L272-L275

So the way I see it, there is no reason to duplicate find_record_with_rbac for this case.
That said, the current change is sufficient. No other changes are required.

[AparnaKarve]

@martinpovolny bump!

This should be categorized as a blocker because:
(a) it's fixing a UI blow-up
(b) it's related to another blocker BZ https://bugzilla.redhat.com/show_bug.cgi?id=1444172

[simaishi]

Fine backport details:

$ git log -1
commit ad2bc25eea02d4e2a3356f3bb038eb2ee6583997
Author: Dan Clarizio <[email protected]>
Date:   Mon Apr 24 16:17:30 2017 -0700

    Merge pull request #1061 from AparnaKarve/fix_storage_manager_id
    
    Fix storage_manager_id when adding a new cloud volume
    (cherry picked from commit f491b455a5d50068bc9160e8fc9b47fdb2f3429d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1445111