From d24937cea221e1ea19f28cbd4ac6c0df9a834d2e Mon Sep 17 00:00:00 2001
From: Nick LaMuro <nicklamuro@gmail.com>
Date: Mon, 12 Mar 2018 19:54:48 -0500
Subject: [PATCH] Skip query references in Rbac when not needed

Makes calling `.references` on the Rbac scope more conservative, since
it has the possibility of causing some nasty join bombs.

The change to the CloudTenancyMixin was made so that it can be in charge
of it's own specific references instead of relying on a quirk of
`ActiveRecord`, in which `references(nil)` will call references for all
of the `includes` values.

This is a bandage for for the following issue:

* * *

Given roughly these kind of table counts

    container_images:       700
    containers:             2500
    openscap_results:       100
    openscap_rule_results:  70000
    custom_attributes:      20000

Where half of the custom_attributes are associated with
container_images.  If the following MiqReport is run:

    cols:
    - name
    - virtual_custom_attribute_build-date:SECTION:docker_labels
    - virtual_custom_attribute_io.openshift.build.name:SECTION:docker_labels
    - virtual_custom_attribute_io.openshift.build.namespace:SECTION:docker_labels
    include:
      openscap_rule_results:
        columns:
        - severity
      containers:
        columns:
        - state

(`:custom_attributes => {}` is also part of the resulting `include`, but
pretty sure that gets tacked on in the `MiqReport#generate` call)

Without needing a report, the query that gets executed can be replicated
by doing the following:

    irb> includes_for_find = {
           :openscap_rule_results => {},
           :containers => {},
           :custom_attributes => {}
         }
    irb> ContainerImages.includes(includes_for_find)
                        .references(includes_for_find)
                        .to_sql

There would be a "LEFT JOIN" bomb that would end up causing about 40GB
of data being returned from the database.  In this case as well, there
is nothing that is making use of the references for the result, but the
extra table data would be returned from the query prior to this commit
in a very inefficient manner with loads of duplicate data.
---
 app/models/mixins/cloud_tenancy_mixin.rb |  8 +++++++-
 lib/rbac/filterer.rb                     | 10 ++++++++--
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/app/models/mixins/cloud_tenancy_mixin.rb b/app/models/mixins/cloud_tenancy_mixin.rb
index 75313422f477..7d885d47c4bb 100644
--- a/app/models/mixins/cloud_tenancy_mixin.rb
+++ b/app/models/mixins/cloud_tenancy_mixin.rb
@@ -1,6 +1,11 @@
 module CloudTenancyMixin
   extend ActiveSupport::Concern
 
+  QUERY_REFERENCES = {
+    :cloud_tenant          => "source_tenant",
+    :ext_management_system => {}
+  }.freeze
+
   module ClassMethods
     include TenancyCommonMixin
 
@@ -13,7 +18,8 @@ def tenant_id_clause_format(tenant_ids)
     end
 
     def tenant_joins_clause(scope)
-      scope.includes(:cloud_tenant => "source_tenant").includes(:ext_management_system)
+      scope.includes(QUERY_REFERENCES)
+           .references(QUERY_REFERENCES) # needed for the above to work
     end
   end
 end
diff --git a/lib/rbac/filterer.rb b/lib/rbac/filterer.rb
index d5ad0e563867..b6c24c6e1f0a 100644
--- a/lib/rbac/filterer.rb
+++ b/lib/rbac/filterer.rb
@@ -240,6 +240,7 @@ def search(options = {})
 
       exp_sql, exp_includes, exp_attrs = search_filter.to_sql(tz) if search_filter && !klass.try(:instances_are_derived?)
       attrs[:apply_limit_in_sql] = (exp_attrs.nil? || exp_attrs[:supported_by_sql]) && user_filters["belongsto"].blank?
+      skip_references            = skip_references?(options, attrs)
 
       # for belongs_to filters, scope_targets uses scope to make queries. want to remove limits for those.
       # if you note, the limits are put back into scope a few lines down from here
@@ -249,7 +250,7 @@ def search(options = {})
               .includes(include_for_find).includes(exp_includes)
               .order(order)
 
-      scope = include_references(scope, klass, include_for_find, exp_includes)
+      scope = include_references(scope, klass, include_for_find, exp_includes, skip_references)
       scope = scope.limit(limit).offset(offset) if attrs[:apply_limit_in_sql]
       targets = scope
 
@@ -283,7 +284,12 @@ def is_sti?(klass)
       klass.respond_to?(:finder_needs_type_condition?) ? klass.finder_needs_type_condition? : false
     end
 
-    def include_references(scope, klass, include_for_find, exp_includes)
+    def skip_references?(options, attrs)
+      options[:extra_cols].blank? && !attrs[:apply_limit_in_sql]
+    end
+
+    def include_references(scope, klass, include_for_find, exp_includes, skip)
+      return scope if skip
       ref_includes = Hash(include_for_find).merge(Hash(exp_includes))
       unless polymorphic_include?(klass, ref_includes)
         scope = scope.references(include_for_find).references(exp_includes)