EMS Cloud Refresh is missing #428
Conversation
|
Even with |
|
@miq-bot assign @gtanzillo |
|
@gtanzillo I need to add some tests to confirm/deny my comment |
|
Also I don't like where I've added And the same could be true for other provider types too. |
|
@miq-bot add_label bug, gaprindashvili/yes |
config/api.yml
Outdated
| @@ -2073,6 +2073,8 @@ | |||
| :identifier: ems_infra_new | |||
| - :name: edit | |||
| :identifier: ems_infra_edit | |||
| - :name: refresh | |||
| :identifier: ems_cloud_refresh | |||
There was a problem hiding this comment.
I've not seen multiple "names" in the list before, so I'm not sure what happens... Is there an example of this elsewhere in the file? @abellotti ?
There was a problem hiding this comment.
@Fryguy Yeah exactly, I just put it there "for now" so it was easier for me tbh to comment out ems_infra_refresh during testing 😂
|
@Fryguy @abellotti I've moved the cloud refresh lines now. |
|
We need tests. |
|
@abellotti Good reminder thanks. I assume there are existing tests I can look at to create new ones for this. Do you know what/where those tests are? Sorry I'm still learning the API :) |
config/api.yml
Outdated
| @@ -2081,6 +2081,8 @@ | |||
| :identifier: ems_infra_edit | |||
| - :name: resume | |||
| :identifier: ems_infra_edit | |||
| - :name: refresh | |||
There was a problem hiding this comment.
this is off, we already have a refresh action, maybe your intent was to add the identifier to the refresh above on line 2076, similar to what query does to allow for 2 identifiers. so they can so a refresh if they have the ems_infra_refresh or ems_cloud_refresh role. In which case we have the refresh test, we could add another test to make sure refresh works with ems_cloud_refresh too.
There was a problem hiding this comment.
I moved to here after a comment by Jason. Should I put it underneath identifier: ems_infra_refresh
Yes my intent was to allow a user to refresh if they have either ems_infra_refresh or ems_cloud_refresh role
There was a problem hiding this comment.
yes please, that's the intent of the multiple identifiers in the actions list (user must have at least one such role).
spec/requests/providers_spec.rb
Outdated
| @@ -935,6 +948,17 @@ def failed_auth_action(id) | |||
| expect_single_action_result(failed_auth_action(provider.id.to_s).symbolize_keys) | |||
| end | |||
|
|
|||
| it "supports cloud provider refresh" do | |||
| api_basic_authorize collection_action_identifier(:providers, :refresh) | |||
There was a problem hiding this comment.
I think for this we want the specific identifier, maybe api_basic_authorize('ems_cloud_refresh')
spec/requests/providers_spec.rb
Outdated
| @@ -935,6 +948,17 @@ def failed_auth_action(id) | |||
| expect_single_action_result(failed_auth_action(provider.id.to_s).symbolize_keys) | |||
| end | |||
|
|
|||
| it "supports cloud provider refresh" do | |||
| api_basic_authorize 'ems_cloud_refresh', collection_action_identifier(:providers, :refresh) | |||
There was a problem hiding this comment.
Does this look right @abellotti?
There was a problem hiding this comment.
If we're just testing the cloud refresh, we only want to test the ems_cloud_refresh identifier so probably drop the , collection_action_identifier(:providers, :refresh).
When a user role only allows cloud and not infra refresh, that user is denied refreshing as we only check for infra refresh.
|
@gtanzillo The issues breaking the API repo have been fixed, so this is now green, if @abellotti agrees, should we merge this? |
|
Ping @gtanzillo |
|
@abellotti You good with merging? |
|
Checked commits https://github.com/juliancheal/manageiq-api/compare/8fba92b1f01b253902411d2abfc7e5ced4d09120~...55df8349b68d728db4f3c508b54ccc3404eae893 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
@abellotti @gtanzillo change made, all green ❇️ |
|
Thanks @juliancheal for the update, LGTM!! 👍 |
When a user role only allows cloud and not infra refresh, that user
is denied refreshing as we only check for infra refresh.
Fixes BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1602413
This shows the roles defined:
