Merge pull request ManageIQ#15472 from isimluk/CVE-2016-7047-final

CVE-2016-7047: API leaks any MiqReportResult

app/controllers/api/reports_controller.rb

@@ -7,8 +7,16 @@ class ReportsController < BaseController
 
     before_action :set_additional_attributes, :only => [:index, :show]
 
-    def run_resource(_type, id, _data)
-      report = MiqReport.find(id)
+    def reports_search_conditions
+      MiqReport.for_user(User.current_user).where_clause.ast unless User.current_user.admin?
+    end
+
+    def find_reports(id)
+      MiqReport.for_user(User.current_user).find(id)
+    end
+
+    def run_resource(type, id, _data)
+      report = resource_search(id, type, MiqReport)
       report_result = MiqReportResult.find(report.queue_generate_table)
       run_report_result(true,
                         "running report #{report.id}",

app/controllers/api/results_controller.rb

@@ -2,6 +2,14 @@ module Api
   class ResultsController < BaseController
     before_action :set_additional_attributes, :only => [:index, :show]
 
+    def results_search_conditions
+      MiqReportResult.for_user(User.current_user).where_clause.ast
+    end
+
+    def find_results(id)
+      MiqReportResult.for_user(User.current_user).find(id)
+    end
+
     private
 
     def set_additional_attributes

app/controllers/api/subcollections/results.rb

@@ -1,8 +1,12 @@
 module Api
   module Subcollections
     module Results
+      def find_results(id)
+        MiqReportResult.for_user(User.current_user).find(id)
+      end
+
       def results_query_resource(object)
-        object.miq_report_results
+        object.miq_report_results.for_user(User.current_user)
       end
     end
   end

app/models/miq_report.rb

@@ -53,17 +53,18 @@ class MiqReport < ApplicationRecord
   GROUPINGS = [[:min, "Minimum"], [:avg, "Average"], [:max, "Maximum"], [:total, "Total"]]
   PIVOTS    = [[:min, "Minimum"], [:avg, "Average"], [:max, "Maximum"], [:total, "Total"]]
 
-  def self.filter_with_report_results_by(miq_group_ids)
-    miq_group_condition = {:miq_report_results => {:miq_group_id => miq_group_ids}}
-
-    if miq_group_ids.nil?
-      miq_group_relation = where.not(miq_group_condition)
+  scope :for_user, lambda { |user|
+    if user.admin_user?
+      all
     else
-      miq_group_relation = where(miq_group_condition)
+      where(
+        arel_table[:rpt_type].eq('Custom').and(arel_table[:miq_group_id].eq(user.current_group_id))
+        .or(
+          arel_table[:rpt_type].eq('Default')
+        )
+      )
     end
-
-    miq_group_relation.joins(:miq_report_results).distinct
-  end
+  }
 
   # Scope on reports that have report results.
   #
@@ -75,8 +76,7 @@ def self.having_report_results(options = {})
     miq_group_ids = options[:miq_groups].collect(&:id) unless options[:miq_groups].nil?
 
     miq_group_ids ||= options[:miq_group_ids]
-
-    q = filter_with_report_results_by(miq_group_ids)
+    q = joins(:miq_report_results).merge(MiqReportResult.for_groups(miq_group_ids)).distinct
 
     if options[:select]
       cols = options[:select].to_miq_a

app/models/miq_report_result.rb

@@ -15,6 +15,18 @@ class MiqReportResult < ApplicationRecord
   virtual_column :status_message,        :type => :string, :uses => :miq_task
   virtual_has_one :result_set,           :class_name => "Hash"
 
+  scope :for_groups, lambda { |group_ids|
+    condition = {:miq_group_id => group_ids}
+    if group_ids.nil?
+      where.not(condition)
+    else
+      where(condition)
+    end
+  }
+  scope :for_user, lambda { |user|
+    for_groups(user.admin_user? ? nil : user.miq_group_ids)
+  }
+
   before_save do
     user_info = userid.to_s.split("|")
     if user_info.length == 1

spec/factories/miq_report.rb

@@ -21,7 +21,7 @@
   end
 
   factory :miq_report_with_results, :parent => :miq_report do
-    miq_report_results { [FactoryGirl.create(:miq_report_result)] }
+    miq_report_results { [FactoryGirl.create(:miq_report_result, :miq_group => miq_group)] }
   end
 
   factory :miq_report_chargeback, :parent => :miq_report do

spec/requests/api/collections_spec.rb

@@ -185,7 +185,7 @@ def test_collection_bulk_query(collection, collection_url, klass, id = nil)
     end
 
     it "query Report Results" do
-      FactoryGirl.create(:miq_report_result)
+      FactoryGirl.create(:miq_report_result, :miq_group => @user.current_group)
       test_collection_query(:results, results_url, MiqReportResult)
     end
 
@@ -438,7 +438,7 @@ def test_collection_bulk_query(collection, collection_url, klass, id = nil)
     end
 
     it "bulk query Report Results" do
-      FactoryGirl.create(:miq_report_result)
+      FactoryGirl.create(:miq_report_result, :miq_group => @user.current_group)
       test_collection_bulk_query(:results, results_url, MiqReportResult)
     end
 

spec/requests/api/reports_spec.rb

@@ -33,71 +33,97 @@
     expect(response).to have_http_status(:ok)
   end
 
-  it "can fetch a report's results" do
-    report = FactoryGirl.create(:miq_report_with_results)
-    report_result = report.miq_report_results.first
+  context 'authorized to see its own report results' do
+    let(:group) { FactoryGirl.create(:miq_group) }
+    let(:user) do
+      @user.current_group ||= group
+      @user
+    end
+    let(:report) { FactoryGirl.create(:miq_report_with_results, :miq_group => user.current_group) }
 
-    api_basic_authorize
-    run_get "#{reports_url(report.id)}/results"
+    it "can fetch a report's results" do
+      report_result = report.miq_report_results.first
 
-    expect_result_resources_to_include_hrefs(
-      "resources",
-      [
-        "#{reports_url(report.id)}/results/#{report_result.to_param}"
-      ]
-    )
-    expect(response.parsed_body["resources"]).not_to be_any { |resource| resource.key?("result_set") }
-    expect(response).to have_http_status(:ok)
-  end
+      api_basic_authorize
+      run_get "#{reports_url(report.id)}/results"
 
-  it "can fetch a report's result" do
-    report = FactoryGirl.create(:miq_report_with_results)
-    report_result = report.miq_report_results.first
-    table = Ruport::Data::Table.new(
-      :column_names => %w(foo),
-      :data         => [%w(bar), %w(baz)]
-    )
-    allow(report).to receive(:table).and_return(table)
-    allow_any_instance_of(MiqReportResult).to receive(:report_results).and_return(report)
+      expect_result_resources_to_include_hrefs(
+        "resources",
+        [
+          "#{reports_url(report.id)}/results/#{report_result.to_param}"
+        ]
+      )
+      expect(response.parsed_body["resources"]).not_to be_any { |resource| resource.key?("result_set") }
+      expect(response).to have_http_status(:ok)
+    end
 
-    api_basic_authorize
-    run_get "#{reports_url(report.id)}/results/#{report_result.to_param}"
+    it "can fetch a report's result" do
+      report_result = report.miq_report_results.first
+      table = Ruport::Data::Table.new(
+        :column_names => %w(foo),
+        :data         => [%w(bar), %w(baz)]
+      )
+      allow(report).to receive(:table).and_return(table)
+      allow_any_instance_of(MiqReportResult).to receive(:report_results).and_return(report)
 
-    expect_result_to_match_hash(response.parsed_body, "result_set" => [{"foo" => "bar"}, {"foo" => "baz"}])
-    expect(response).to have_http_status(:ok)
-  end
+      api_basic_authorize
+      run_get "#{reports_url(report.id)}/results/#{report_result.to_param}"
 
-  it "can fetch all the results" do
-    report = FactoryGirl.create(:miq_report_with_results)
-    result = report.miq_report_results.first
+      expect_result_to_match_hash(response.parsed_body, "result_set" => [{"foo" => "bar"}, {"foo" => "baz"}])
+      expect(response).to have_http_status(:ok)
+    end
 
-    api_basic_authorize collection_action_identifier(:results, :read, :get)
-    run_get results_url
+    it "can fetch all the results" do
+      result = report.miq_report_results.first
 
-    expect_result_resources_to_include_hrefs(
-      "resources",
-      [
-        results_url(result.id).to_s
-      ]
-    )
-    expect(response).to have_http_status(:ok)
-  end
+      api_basic_authorize collection_action_identifier(:results, :read, :get)
+      run_get results_url
 
-  it "can fetch a specific result as a primary collection" do
-    report = FactoryGirl.create(:miq_report_with_results)
-    report_result = report.miq_report_results.first
-    table = Ruport::Data::Table.new(
-      :column_names => %w(foo),
-      :data         => [%w(bar), %w(baz)]
-    )
-    allow(report).to receive(:table).and_return(table)
-    allow_any_instance_of(MiqReportResult).to receive(:report_results).and_return(report)
+      expect_result_resources_to_include_hrefs(
+        "resources",
+        [
+          results_url(result.id).to_s
+        ]
+      )
+      expect(response).to have_http_status(:ok)
+    end
 
-    api_basic_authorize action_identifier(:results, :read, :resource_actions, :get)
-    run_get results_url(report_result.id)
+    it "can fetch a specific result as a primary collection" do
+      report_result = report.miq_report_results.first
+      table = Ruport::Data::Table.new(
+        :column_names => %w(foo),
+        :data         => [%w(bar), %w(baz)]
+      )
+      allow(report).to receive(:table).and_return(table)
+      allow_any_instance_of(MiqReportResult).to receive(:report_results).and_return(report)
 
-    expect_result_to_match_hash(response.parsed_body, "result_set" => [{"foo" => "bar"}, {"foo" => "baz"}])
-    expect(response).to have_http_status(:ok)
+      api_basic_authorize action_identifier(:results, :read, :resource_actions, :get)
+      run_get results_url(report_result.id)
+
+      expect_result_to_match_hash(response.parsed_body, "result_set" => [{"foo" => "bar"}, {"foo" => "baz"}])
+      expect(response).to have_http_status(:ok)
+    end
+
+    it "returns an empty result set if none has been run" do
+      report_result = report.miq_report_results.first
+
+      api_basic_authorize
+      run_get "#{reports_url(report.id)}/results/#{report_result.id}"
+
+      expect_result_to_match_hash(response.parsed_body, "result_set" => [])
+      expect(response).to have_http_status(:ok)
+    end
+
+    it "returns an empty result set if none has been run" do
+      report = FactoryGirl.create(:miq_report_with_results, :miq_group => user.current_group)
+      report_result = report.miq_report_results.first
+
+      api_basic_authorize
+      run_get "#{reports_url(report.id)}/results/#{report_result.id}"
+
+      expect_result_to_match_hash(response.parsed_body, "result_set" => [])
+      expect(response).to have_http_status(:ok)
+    end
   end
 
   it "can fetch all the schedule" do
@@ -166,17 +192,6 @@
     expect(response).to have_http_status(:forbidden)
   end
 
-  it "returns an empty result set if none has been run" do
-    report = FactoryGirl.create(:miq_report_with_results)
-    report_result = report.miq_report_results.first
-
-    api_basic_authorize
-    run_get "#{reports_url(report.id)}/results/#{report_result.id}"
-
-    expect_result_to_match_hash(response.parsed_body, "result_set" => [])
-    expect(response).to have_http_status(:ok)
-  end
-
   context "with an appropriate role" do
     it "can run a report" do
       report = FactoryGirl.create(:miq_report)