Cleanup RBAC testing methods for checked items

[romanblanco]

At the moment of creating this issue, we are using 5 different methods for doing RBAC check, that basically do the same thing, just for different cases:

• find_checked_ids_with_rbac: returns array of checked ids tested on RBAC ( ❗️ the records are being thrown away after the RBAC check, and later are loaded again)

rg find_checked_ids_with_rbac -c

controllers/cloud_volume_snapshot_controller.rb:1
controllers/application_controller.rb:2
controllers/cloud_network_controller.rb:1
controllers/provider_foreman_controller.rb:1
controllers/ems_common.rb:2
controllers/pxe_controller/pxe_image_types.rb:1
controllers/application_controller/policy_support.rb:1
controllers/application_controller/ci_processing.rb:3
controllers/mixins/containers_common_mixin.rb:2
controllers/ops_controller/settings/analysis_profiles.rb:2

• find_id_with_rbac: returns id of single record checked in UI and tested on RBAC ( ❗️ again, the records are being thrown away after the RBAC check, and later are loaded again)

rg find_id_with_rbac -c

controllers/cloud_volume_snapshot_controller.rb:1
controllers/application_controller.rb:2
controllers/cloud_network_controller.rb:1
controllers/miq_ae_class_controller.rb:2
controllers/provider_foreman_controller.rb:1
controllers/ems_common.rb:1
controllers/pxe_controller/pxe_image_types.rb:2
controllers/application_controller/policy_support.rb:1
controllers/application_controller/ci_processing.rb:2
controllers/mixins/containers_common_mixin.rb:1

• find_checked_records_with_rbac: returns array of checked records tested on RBAC

rg find_checked_records_with_rbac -c

controllers/application_controller.rb:2
controllers/auth_key_pair_cloud_controller.rb:1
controllers/cloud_tenant_controller.rb:1
controllers/report_controller/schedules.rb:1
controllers/ops_controller/ops_rbac.rb:2

• find_record_with_rbac: returns single record tested on RBAC (reused)

• find_record_with_rbac_flash: basically the same as find_record_with_rbac, except it's showing a flash, instead of rising an exception in case the unauthorized record has been accessed (gone)

[romanblanco]

cc/ @PanSpagetka @martinpovolny

[romanblanco]

@miq-bot assign @romanblanco
@miq-bot add_label refactoring, rbac

[miq-bot]

This issue has been automatically marked as stale because it has not been updated for at least 6 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions!

[romanblanco]

@miq-bot remove_label stale

[romanblanco]

mentioned in https://github.com/ManageIQ/guides/blob/master/reviewers_guidelines.md#testing-rbac-on-checked-items

[miq-bot]

This issue has been automatically marked as stale because it has not been updated for at least 6 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions!

[martinpovolny]

@romanblanco : can you sum up the status of this work, please?

[romanblanco]

@martinpovolny method find_record_with_rbac has been reused, method find_record_with_rbac_flash is not in the codebase anymore.

Other methods are still present, but some of the students PRs removing them are still opened:

rg find_checked_ids_with_rbac -c

controllers/cloud_volume_snapshot_controller.rb:1
controllers/application_controller.rb:2
controllers/cloud_network_controller.rb:1
controllers/provider_foreman_controller.rb:1
controllers/ems_common.rb:2
controllers/pxe_controller/pxe_image_types.rb:1
controllers/application_controller/policy_support.rb:1
controllers/application_controller/ci_processing.rb:3
controllers/mixins/containers_common_mixin.rb:2
controllers/ops_controller/settings/analysis_profiles.rb:2

rg find_id_with_rbac -c

controllers/cloud_volume_snapshot_controller.rb:1
controllers/application_controller.rb:2
controllers/cloud_network_controller.rb:1
controllers/miq_ae_class_controller.rb:2
controllers/provider_foreman_controller.rb:1
controllers/ems_common.rb:1
controllers/pxe_controller/pxe_image_types.rb:2
controllers/application_controller/policy_support.rb:1
controllers/application_controller/ci_processing.rb:2
controllers/mixins/containers_common_mixin.rb:1

rg find_checked_records_with_rbac -c

controllers/application_controller.rb:2
controllers/auth_key_pair_cloud_controller.rb:1
controllers/cloud_tenant_controller.rb:1
controllers/report_controller/schedules.rb:1
controllers/ops_controller/ops_rbac.rb:2

[romanblanco]

@miq-bot remove_label stale

[martinpovolny]

@romanblanco, can you estimate the amount of work needed to get this finished, please? One sprint? Two sprints?

[romanblanco]

@martinpovolny I believe it is doable in one sprint. The changes are not somewhat complicated, and there is a lot of examples available already.

Making sure the changes are correct take the most of the time.

[martinpovolny]

The new code is simpler and unified. It's less error-prone. Previously it was really fragile, fixes and changes where bringing new BZs.
Also you have added a number of tests where there where none previously.

If it's a sprint or a bit more of work I would say we should get it finished and off the table.

@dclarizio, what do you think?

[dclarizio]

Let's finish this project since it's mostly done already and definitely improves the code base. Thx!