Ensure a users own tasks are the only ones returned when the users role has View/My Tasks

[jvlcek]

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

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

[jvlcek]

@miq-bot add_label bug

[jvlcek]

@miq-bot assign @abellotti

[jvlcek]

@miq-bot add_label hammer/yes

[jvlcek]

@bdunne Please take a look if you can.

[bdunne]

I'm not too familiar with how the product features stuff works, but isn't this already checked somewhere?

[jvlcek]

Thank you for the feedback @bdunne. What is checked now is if there is a role that allows access to Tasks. There are 2 product features that allow access to Tasks, miq_task_all_ui and miq_task_my_ui. The current logic allows access to all tasks if either of these product features is set on the role associated with the group assigned to the user. The result being that even if only miq_task_my_ui product feature is set the API returns all tasks. The code changes introduced this PR adds logic to only access the tasks owned by the current user when only miq_task_my_ui product features is set.

[bdunne]

You could do something like this to remove the enumeration and extra database queries

expect(MiqTask.where(:id => args.collect(&:id)).length).to eq(args.length)

[jvlcek]

👍 I like it. Thank you! Will do.

[gtanzillo]

@jvlcek At first glance, this seems to blur the lines of the two sides of our RBAC implementation -

1. What a user can see (filtering)
2. What a user can do (product features)
   Historically those two have been separate, meaning, given a product feature like edit that a user has in his role would allow that user to edit any object that is visible to him.

This PR is attempting to limit what a user can see based on product features and not filtering. Looking at the BZ, I can understand this approach. I need to give it some thought to see if there is another way to handle this case.

It seems very similar to the Access Restriction for Services, VMs, and Templates options in user roles where an admin can restrict users to Only User or Group Owned or Only User Owned The implementation of that option is solely on the filtering side of RBAC

[gtanzillo]

@jvlcek After doing a little more research I think your approach is ok. The UI does a similar thing, indirectly. They are looking at the same product features to determine which tabs a user will see here

    if role_allows?(:feature => "miq_task_my_ui")
      @tabs.push(["1", _("My Tasks")])
    end
    if role_allows?(:feature => "miq_task_all_ui")
      @tabs.push(["2", _("All Tasks")])
    end

This follows the "what a use can do" part of RBAC. But the filtering is done by virtue of the tab the user gets based on his role. The API does not have this luxury.

We do something similar to you approach with report results here. But instead of hard coding the product feature, it's added to a method in MiqUserRole here called report_admin_user?

I think we should follow that pattern

[jvlcek]

@gtanzillo Thank you! Stay tuned while I try out that approach.

[jvlcek]

@miq-bot assign @gtanzillo

[jvlcek]

This PR now has a companion PR which needs to be merged before this PR, ManageIQ/manageiq#18311

[jvlcek]

The continuous integration tests will fail until companion PR, ManageIQ/manageiq#18311, is merged.

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[miq-bot]

Checked commits jvlcek/manageiq-api@905f429~...fed0506 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆