Ensure a users own tasks are the only ones returned when the users role has View/My Tasks #18311
Conversation
|
@miq-bot add_label bug |
|
@miq-bot add_label hammer/yes |
|
@miq-bot assign @gtanzillo |
|
@bdunne Please take a look. This PR addresses comments made by @gtanzillo in companion PR |
app/models/miq_product_feature.rb
Outdated
| REPORT_ADMIN_FEATURE = "miq_report_superadmin".freeze | ||
| REQUEST_ADMIN_FEATURE = "miq_request_approval".freeze | ||
| TENANT_ADMIN_FEATURE = "rbac_tenant".freeze | ||
| REPORT_MY_TASKS = "miq_task_my_ui".freeze |
There was a problem hiding this comment.
Following the existing convention I think these two should be named with *_FEATURE.
Also, the REPORT prefix is misleading because these features are not reporting feature, afaik. Perhaps you can name them MY_TASKS_FEATURE and ALL_TASKS_FEATURE?
app/models/miq_user_role.rb
Outdated
| @@ -112,6 +112,10 @@ def tenant_admin_user? | |||
| allows?(:identifier => MiqProductFeature::TENANT_ADMIN_FEATURE) | |||
| end | |||
|
|
|||
| def report_only_my_user_tasks? | |||
There was a problem hiding this comment.
Following the convention of the above methods, how about naming this one my_tasks_user?
There was a problem hiding this comment.
@gtanzillo Thank you for the feedback.
I named this method report_only_my_user_tasks? because it is possible that miq_task_my_ui product feature is enabled and miq_task_all_ui in which case naming this method my_tasks_user? would be somewhat inaccurate. One might expect my_tasks_user? to return true if miq_task_my_ui is set but that is not the functionality we want. We want a method to return true when only miq_task_my_ui is set and not miq_task_all_ui.
Would you be OK if I renamed this method from report_only_my_user_tasks? to only_my_user_tasks? ?
|
@gtanzillo I made a commit using |
|
Checked commits jvlcek/manageiq@485a2df~...cbce61d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1639387
This is a companion PR for, and must be merged before, the manageiq-api repo PR:
ManageIQ/manageiq-api#526
A role can be configured to view all tasks or only my tasks. When a user is assigned to
a role that is configured to view only "My Tasks" The API would incorrectly returning all
tasks for all users.
And when querying a single task by ID would return the task even if not owned by the
current user.
This PR corrects this erroneous condition by adding two new methods to the
base_controller/renderer.rb #find_resource and #find_collection. Each of these
two new methods is overridden in the task_controller.rb where logic is adding to
correctly handle this condition.
To test:
Assign a group to a user where the group has a role configured with
View / My Tasks and without View / All Tasks
Then exercise the API for this user to query the task collection ensuring only tasks
owned by the specified user are returned:
e.g.:
curl -k "https://:@/api/tasks/"
This should only return tasks owned by the specified user and should match what the UI
shows when that specified user is logged in and lists their tasks.
and queries for individual task do not return tasks owned by another user.
e.g.:
curl -k "https://:@/api/tasks/"
This should only return the task if it is owned by the specified user