Skip to content

Commit

Permalink
Fix invalid count query
Browse files Browse the repository at this point in the history
The fix here simply matches up the `if` checks used in:

- `Api::ReportsController#reports_search_conditions`
- `MiqReport.for_user`

to both use `User#report_admin_user?`

* * *

Further background info:

This bug is the result of a collision between the following two PRs:

- ManageIQ/manageiq#15472
- ManageIQ/manageiq#17444

The first PR, ManageIQ/manageiq#15472, introduces:

`Api::ReportsController#reports_search_conditions`

Which uses `unless User.current_user.admin?` to check if it should use
the `where_clause` from the `MiqReport.for_user` for the search
conditions (passed into a `ActiveRecord` `.where` farther up the stack).
The idea for this initial check is building this where clause only makes
sense if it isn't an `all`, otherwise where it is used will cause an
error in Postgres syntax because of `... WHERE ()` being passed

This, however, doesn't match the `user.admin_user?`(at the time of
ManageIQ/manageiq#15472) check that is done in `.for_user` method,
which means it would be called for users that don't match the `admin`
username, but still are part of either of the following roles

- `"EvmRole-super_administrator"`
- `"EvmRole-administrator"`

The bug in question:

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

tested this with a custom role, which doesn't match either of those two
roles, so the `User#admin?` check and `User#admin_user?` check happened
to be equivalent in `5.9` (see comment ManageIQ#4 in BZ link), but not `5.10`.
But, if the replication steps had instead said to:

- Create a user, "User1", that is part of the "EvmGroup-administrator"
  group

This would have also failed in the same way on `5.9` as it does on
`5.10` currently.

This fails on 5.10, though, because in ManageIQ/manageiq#17444, the
`admin_user?` method changes to `report_admin_user?`, and it's
functionality also now checks if the role is valid for querying the
group role's product features the user has access to, and it only has an
eject for the role that happens to be `admin?`.  Meaning that the
`admin` "super user" still has no problem with this query, but any other
user that could still count as a `report_admin_user?` would (both users
in the `EvmGroup-administrator` role and just having the proper features
enabled)
  • Loading branch information
NickLaMuro committed Dec 5, 2018
1 parent 7cec205 commit 43f00d6
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 1 deletion.
2 changes: 1 addition & 1 deletion app/controllers/api/reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class ReportsController < BaseController
before_action :set_additional_attributes, :only => [:index, :show]

def reports_search_conditions
MiqReport.for_user(User.current_user).where_clause.ast unless User.current_user.admin?
MiqReport.for_user(User.current_user).where_clause.ast unless User.current_user.report_admin_user?
end

def find_reports(id)
Expand Down
24 changes: 24 additions & 0 deletions spec/requests/reports_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,30 @@
end

context "with an appropriate role" do
# Setup in a similar was to the reproduction steps in this BZ:
#
# https://bugzilla.redhat.com/show_bug.cgi?id=1650531
#
it "can fetch all the reports" do
report_1 = FactoryGirl.create(:miq_report_with_results)
report_2 = FactoryGirl.create(:miq_report_with_results)

# Includes roles "API" and "Cloud Intel"
MiqProductFeature.seed
api_basic_authorize :dashboard, :miq_report, :chargeback, :timeline, :rss, :api_exclusive
get api_reports_url

expect_result_resources_to_include_hrefs(
"resources",
[
api_report_url(nil, report_1),
api_report_url(nil, report_2)
]
)
expect_result_to_match_hash(response.parsed_body, "count" => 2, "name" => "reports")
expect(response).to have_http_status(:ok)
end

it "can run a report" do
report = FactoryGirl.create(:miq_report)

Expand Down

0 comments on commit 43f00d6

Please sign in to comment.