Check selected Cloud Volumes for RBAC

[romanblanco]

Storage -> Block Storage -> Volumes

[romanblanco]

I'm not sure how to test this, is there a way to restrict some Cloud Volumes to some users?

[miq-bot]

Checked commit romanblanco@451475a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🏆

[romanblanco]

Also:

It seems strange to me, that there is Unauthorized exception handling in cloud_volume_controller.rb:

  459         begin                                                                                                                   
  460           valid_delete = volume.validate_delete_volume                                                                          
  461           if valid_delete[:available]                                                                                           
  462             volumes_to_delete.push(volume)                                                                                      
  463           else                                                                                                                  
  464             add_flash(_("Couldn't initiate deletion of %{model} \"%{name}\": %{details}") % {                                   
  465               :model   => ui_lookup(:table => 'cloud_volume'),                                                                  
  466               :name    => volume.name,                                                                                          
  467               :details => valid_delete[:message]}, :error)                                                                      
  468           end                                                                                                                   
  469         rescue Excon::Error::Unauthorized => e                                                                                  
  470           add_flash(_("Couldn't initiate deletion of %{model} \"%{name}\": %{details}") % {                                     
  471             :model   => ui_lookup(:table => 'cloud_volume'),                                                                    
  472             :name    => volume.name,                                                                                            
  473             :details => e}, :error)                                                                                             
  474         end    

when the validate_delete_volume method is only checking for provider status:

 45   def validate_delete_volume                                                                                                      
 46     msg = validate_volume                                                                                                         
 47     return {:available => msg[:available], :message => msg[:message]} unless msg[:available]                                      
 48     if with_provider_object(&:status) == "in-use"                                                                                 
 49       return validation_failed("Create Volume", "Can't delete volume that is in use.")                                            
 50     end                                                                                                                           
 51     {:available => true, :message => nil}                                                                                         
 52   end 

maybe I'm missing something, but it seems to me that the code can never get to the exception. ❓

[ZitaNemeckova]

@romanblanco I'm pretty sure I was getting Excon::Error::Unauthorized exception at some point. But I agree that checking for authorization in validate_delete_volume would be nicer.

[martinpovolny]

This is probably a question for the OpenStackers. @mansam, @tzumainn. If I remember correctly they used to call the OpenStack API directly so the exception handling might be a remnant of that.

[romanblanco]

@martinpovolny it's not remnant of that i think, it was added by @ZitaNemeckova in ManageIQ/manageiq@6ada628, but I believe I can remove it now.

[simaishi]

Fine backport details:

$ git log -1
commit c1fdd88bfc51d59af9f43bc9e35bf560273c832d
Author: Martin Povolny <[email protected]>
Date:   Fri Mar 31 09:08:31 2017 +0200

    Merge pull request #770 from romanblanco/fix_rbac_cloud_volume
    
    Check selected Cloud Volumes for RBAC
    (cherry picked from commit 690aed4c06f46eed1509e720b1f7d9ba99ca8eff)