Fixes #16982 - Scope properly when no taxonomies are set

The default scope for hosts and other objects did not restrict
properly by taxonomies. An user without organizations or
locations, could do anything it's permissions allow to.
The list of hosts was unrestricted and showed hosts in
any location or organization.

This is fixed to work so that:

Users without taxonomies, when set to 'any context' cannot see
anything (at all)

Users with taxonomies, when set to 'any context' can see
everything within all of their taxonomies context (including
children taxonomies).

Admins set to 'any context' can see everything - regardless
of whether it has a taxonomy or not.

Users or admins set to some organization/location scope
can only see stuff within scope.

app/controllers/concerns/foreman/controller/smart_proxy_auth.rb

@@ -30,7 +30,11 @@ def add_smart_proxy_filters(actions, options = {})
   # Permits registered Smart Proxies or a user with permission
   def require_smart_proxy_or_login(features = nil)
     features = features.call if features.respond_to?(:call)
-    allowed_smart_proxies = features.blank? ? SmartProxy.all : SmartProxy.with_features(*features)
+    allowed_smart_proxies = if features.blank?
+                              SmartProxy.unscoped.all
+                            else
+                              SmartProxy.unscoped.with_features(*features)
+                            end
 
     if !Setting[:restrict_registered_smart_proxies] || auth_smart_proxy(allowed_smart_proxies, Setting[:require_ssl_smart_proxies])
       set_admin_user
@@ -47,7 +51,7 @@ def require_smart_proxy_or_login(features = nil)
 
   # Filter requests to only permit from hosts with a registered smart proxy
   # Uses rDNS of the request to match proxy hostnames
-  def auth_smart_proxy(proxies = SmartProxy.all, require_cert = true)
+  def auth_smart_proxy(proxies = SmartProxy.unscoped.all, require_cert = true)
     request_hosts = nil
     if request.ssl?
       # If we have the client certficate in the request environment we can extract the dn and sans from there

app/controllers/users_controller.rb

@@ -158,7 +158,7 @@ def parameter_filter_context
   end
 
   def verify_active_session
-    if !request.post? && params[:status].blank? && User.exists?(session[:user].presence)
+    if !request.post? && params[:status].blank? && User.unscoped.exists?(session[:user].presence)
       warning _("You have already logged in")
       redirect_back_or_to hosts_path
       return

app/models/concerns/taxonomix.rb

@@ -1,15 +1,15 @@
 module Taxonomix
   extend ActiveSupport::Concern
   include DirtyAssociations
+  TAXONOMY_JOIN_TABLE = :taxable_taxonomies
 
   included do
-    taxonomy_join_table = :taxable_taxonomies
-    has_many taxonomy_join_table.to_sym, :dependent => :destroy, :as => :taxable
+    has_many TAXONOMY_JOIN_TABLE, :dependent => :destroy, :as => :taxable
     has_many :locations, -> { where(:type => 'Location') },
-             :through => taxonomy_join_table, :source => :taxonomy,
+             :through => TAXONOMY_JOIN_TABLE, :source => :taxonomy,
              :validate => false
     has_many :organizations, -> { where(:type => 'Organization') },
-             :through => taxonomy_join_table, :source => :taxonomy,
+             :through => TAXONOMY_JOIN_TABLE, :source => :taxonomy,
              :validate => false
     after_initialize :set_current_taxonomy
 
@@ -28,10 +28,11 @@ module ClassMethods
 
     # default inner_method includes children (subtree_ids)
     def with_taxonomy_scope(loc = Location.current, org = Organization.current, inner_method = :subtree_ids)
+      scope = block_given? ? yield : where(nil)
+      return scope unless Taxonomy.enabled_taxonomies.present?
       self.which_ancestry_method = inner_method
       self.which_location        = Location.expand(loc) if SETTINGS[:locations_enabled]
       self.which_organization    = Organization.expand(org) if SETTINGS[:organizations_enabled]
-      scope = block_given? ? yield : where('1=1')
       scope = scope_by_taxable_ids(scope)
       scope.readonly(false)
     end
@@ -69,26 +70,45 @@ def enforce_default
     end
 
     def taxable_ids(loc = which_location, org = which_organization, inner_method = which_ancestry_method)
-      if SETTINGS[:locations_enabled] && loc.present?
-        inner_ids_loc = if Location.ignore?(self.to_s)
-                          self.unscoped.pluck("#{table_name}.id")
-                        else
-                          inner_select(loc, inner_method)
-                        end
+      # Return everything (represented by nil), including objects without
+      # taxonomies. This value should only be returned for admin users.
+      return nil if any_context?(loc) && any_context?(org) &&
+        User.current.try(:admin?)
+
+      ids = unscoped.pluck(:id)
+      ids &= inner_ids(loc, Location, inner_method) if SETTINGS[:locations_enabled]
+      ids &= inner_ids(org, Organization, inner_method) if SETTINGS[:organizations_enabled]
+
+      if self == User
+        # In the case of users we want the taxonomy scope to get both the users
+        # of the taxonomy, admins, and the current user.
+        ids.concat(admin_ids)
+        ids << User.current.id if User.current.present?
       end
-      if SETTINGS[:organizations_enabled] && org.present?
-        inner_ids_org = if Organization.ignore?(self.to_s)
-                          self.unscoped.pluck("#{table_name}.id")
-                        else
-                          inner_select(org, inner_method)
-                        end
-      end
-      inner_ids   = inner_ids_loc & inner_ids_org if (inner_ids_loc && inner_ids_org)
-      inner_ids ||= inner_ids_loc if inner_ids_loc
-      inner_ids ||= inner_ids_org if inner_ids_org
-      # In the case of users we want the taxonomy scope to get both the users of the taxonomy and admins.
-      inner_ids.concat(admin_ids) if inner_ids && self == User
-      inner_ids
+
+      ids
+    end
+
+    # Returns the IDs available for the passed context.
+    # Passing a nil or [] value as taxonomy equates to "Any context".
+    # Any other value will be understood as 'IDs available in this taxonomy'.
+    def inner_ids(taxonomy, taxonomy_class, inner_method)
+      return unscoped.pluck("#{table_name}.id") if taxonomy_class.ignore?(to_s)
+      return inner_select(taxonomy, inner_method) if taxonomy.present?
+      return [] unless User.current.present?
+      # Any available taxonomy to the current user
+      return unscoped.pluck("#{table_name}.id") if User.current.admin?
+
+      taxonomy_relation = taxonomy_class.to_s.underscore.pluralize
+      any_context_taxonomies = User.current.
+        taxonomy_and_child_ids(:"#{taxonomy_relation}")
+      unscoped.joins(TAXONOMY_JOIN_TABLE).
+        where("#{TAXONOMY_JOIN_TABLE}.taxonomy_id" => any_context_taxonomies).
+        pluck(:id)
+    end
+
+    def any_context?(taxonomy)
+      taxonomy.blank?
     end
 
     # taxonomy can be either specific taxonomy object or array of these objects

app/models/host/base.rb

@@ -49,8 +49,8 @@ def self.taxonomy_conditions
       org = Organization.expand(Organization.current) if SETTINGS[:organizations_enabled]
       loc = Location.expand(Location.current) if SETTINGS[:locations_enabled]
       conditions = {}
-      conditions[:organization_id] = Array(org).map { |o| o.subtree_ids }.flatten.uniq if org.present?
-      conditions[:location_id] = Array(loc).map { |l| l.subtree_ids }.flatten.uniq if loc.present?
+      conditions[:organization_id] = Array(org).map { |o| o.subtree_ids }.flatten.uniq unless org.nil?
+      conditions[:location_id] = Array(loc).map { |l| l.subtree_ids }.flatten.uniq unless loc.nil?
       conditions
     end
 

app/services/sso/base.rb

@@ -41,7 +41,7 @@ def authenticate!
     end
 
     def current_user
-      User.except_hidden.find_by_login(self.user)
+      User.unscoped.except_hidden.find_by_login(self.user)
     end
   end
 end

app/services/sso/oauth.rb

@@ -20,7 +20,7 @@ def authenticate!
       if OAuth::Signature.verify(request, :consumer_secret => Setting['oauth_consumer_secret'])
         if Setting['oauth_map_users']
           user_name = request.headers['HTTP_FOREMAN_USER'].to_s
-          User.find_by_login(user_name).tap do |obj|
+          User.unscoped.find_by_login(user_name).tap do |obj|
             Rails.logger.warn "Oauth: mapping to user '#{user_name}' failed" if obj.nil?
           end.try(:login)
         else

app/services/tax_host.rb

@@ -27,11 +27,11 @@ def selected_ids
     ids = default_ids_hash
     #types NOT ignored - get ids that are selected
     hash_keys.each do |col|
-      ids[col] = Array(taxonomy.send(col))
+      ids[col] = Array(taxonomy.send(col)).uniq
     end
     #types that ARE ignored - get ALL ids for object
     Array(taxonomy.ignore_types).each do |taxonomy_type|
-      ids["#{taxonomy_type.tableize.singularize}_ids"] = taxonomy_type.constantize.pluck(:id)
+      ids["#{taxonomy_type.tableize.singularize}_ids"] = taxonomy_type.constantize.pluck(:id).uniq
     end
 
     ids["#{opposite_taxonomy_type}_ids"] = Array(taxonomy.send("#{opposite_taxonomy_type}_ids"))
@@ -213,7 +213,7 @@ def substract_deep_hashes(h1, h2)
   def default_ids_hash(populate_values = false)
     ids = HashWithIndifferentAccess.new
     hash_keys.each do |col|
-      ids[col] = populate_values ? Array(self.send(col)) : []
+      ids[col] = populate_values ? Array(self.send(col)).uniq : []
     end
     ids
   end

db/migrate/20130908100439_delete_orphaned_records.rb

@@ -37,23 +37,23 @@ def up
     TrendCounter.where("trend_id NOT IN (?)", Trend.pluck(:id)).delete_all
 
     # NULLIFY FOREIGN KEY VALUE IF IT HAS AN ORPHANED FOREIGN KEY
-    Audit.unscoped.where("user_id NOT IN (?)", User.pluck(:id)).update_all(:user_id => nil)
+    Audit.unscoped.where("user_id NOT IN (?)", User.unscoped.pluck(:id)).update_all(:user_id => nil)
     FakeConfigTemplate.where("template_kind_id NOT IN (?)", TemplateKind.pluck(:id)).update_all(:template_kind_id => nil)
-    Domain.where("dns_id NOT IN (?)", SmartProxy.pluck(:id)).update_all(:dns_id => nil)
-    Subnet.where("dhcp_id NOT IN (?)", SmartProxy.pluck(:id)).update_all(:dhcp_id => nil)
-    Subnet.where("dns_id NOT IN (?)", SmartProxy.pluck(:id)).update_all(:dns_id => nil)
-    Subnet.where("tftp_id NOT IN (?)", SmartProxy.pluck(:id)).update_all(:tftp_id => nil)
-    Image.where("architecture_id NOT IN (?)", Architecture.pluck(:id)).update_all(:architecture_id => nil)
-    Image.where("compute_resource_id NOT IN (?)", ComputeResource.pluck(:id)).update_all(:compute_resource_id => nil)
+    Domain.where("dns_id NOT IN (?)", SmartProxy.unscoped.pluck(:id)).update_all(:dns_id => nil)
+    Subnet.where("dhcp_id NOT IN (?)", SmartProxy.unscoped.pluck(:id)).update_all(:dhcp_id => nil)
+    Subnet.where("dns_id NOT IN (?)", SmartProxy.unscoped.pluck(:id)).update_all(:dns_id => nil)
+    Subnet.where("tftp_id NOT IN (?)", SmartProxy.unscoped.pluck(:id)).update_all(:tftp_id => nil)
+    Image.where("architecture_id NOT IN (?)", Architecture.unscoped.pluck(:id)).update_all(:architecture_id => nil)
+    Image.where("compute_resource_id NOT IN (?)", ComputeResource.unscoped.pluck(:id)).update_all(:compute_resource_id => nil)
     Image.where("operatingsystem_id NOT IN (?)", Operatingsystem.unscoped.pluck(:id)).update_all(:operatingsystem_id => nil)
-    Nic::Base.where("domain_id NOT IN (?)", Domain.pluck(:id)).update_all(:domain_id => nil)
-    Nic::Base.where("host_id NOT IN (?)", Host::Base.pluck(:id)).update_all(:host_id => nil)
-    Nic::Base.where("subnet_id NOT IN (?)", Subnet.pluck(:id)).update_all(:subnet_id => nil)
-    OsDefaultTemplate.where("config_template_id NOT IN (?)", FakeConfigTemplate.pluck(:id)).update_all(:config_template_id => nil)
+    Nic::Base.where("domain_id NOT IN (?)", Domain.unscoped.pluck(:id)).update_all(:domain_id => nil)
+    Nic::Base.where("host_id NOT IN (?)", Host::Base.unscoped.pluck(:id)).update_all(:host_id => nil)
+    Nic::Base.where("subnet_id NOT IN (?)", Subnet.unscoped.pluck(:id)).update_all(:subnet_id => nil)
+    OsDefaultTemplate.where("config_template_id NOT IN (?)", FakeConfigTemplate.unscoped.pluck(:id)).update_all(:config_template_id => nil)
     OsDefaultTemplate.where("operatingsystem_id NOT IN (?)", Operatingsystem.unscoped.pluck(:id)).update_all(:operatingsystem_id => nil)
-    OsDefaultTemplate.where("template_kind_id NOT IN (?)", TemplateKind.pluck(:id)).update_all(:template_kind_id => nil)
-    TemplateCombination.where("config_template_id NOT IN (?)", FakeConfigTemplate.pluck(:id)).update_all(:config_template_id => nil)
-    TemplateCombination.where("environment_id NOT IN (?)", Environment.pluck(:id)).update_all(:environment_id => nil)
+    OsDefaultTemplate.where("template_kind_id NOT IN (?)", TemplateKind.unscoped.pluck(:id)).update_all(:template_kind_id => nil)
+    TemplateCombination.where("config_template_id NOT IN (?)", FakeConfigTemplate.unscoped.pluck(:id)).update_all(:config_template_id => nil)
+    TemplateCombination.where("environment_id NOT IN (?)", Environment.unscoped.pluck(:id)).update_all(:environment_id => nil)
     TemplateCombination.where("hostgroup_id NOT IN (?)", Hostgroup.unscoped.pluck(:id)).update_all(:hostgroup_id => nil)
 
     host_groups_up

db/seeds.d/07-provisioning_templates.rb

@@ -7,14 +7,14 @@
 # Template kinds
 kinds = {}
 TemplateKind.default_template_labels.keys.collect(&:to_sym).each do |type|
-  kinds[type] = TemplateKind.find_by_name(type)
-  kinds[type] ||= TemplateKind.create(:name => type)
+  kinds[type] = TemplateKind.unscoped.find_by_name(type)
+  kinds[type] ||= TemplateKind.unscoped.create(:name => type)
   raise "Unable to create template kind: #{format_errors kinds[type]}" if kinds[type].nil? || kinds[type].errors.any?
 end
 
 # Provisioning templates
-organizations = Organization.all
-locations = Location.all
+organizations = Organization.unscoped.all
+locations = Location.unscoped.all
 ProvisioningTemplate.without_auditing do
   [
     # Generic PXE files
@@ -81,7 +81,6 @@
     { :name => 'puppet.conf', :source => 'snippets/_puppet.conf.erb', :snippet => true },
     { :name => 'puppet_setup', :source => 'snippets/_puppet_setup.erb', :snippet => true },
     { :name => 'puppetlabs_repo', :source => 'snippets/_puppetlabs_repo.erb', :snippet => true },
-    { :name => 'pxelinux_discovery', :source => 'snippets/_pxelinux_discovery.erb', :snippet => true },
     { :name => 'redhat_register', :source => 'snippets/_redhat_register.erb', :snippet => true },
     { :name => 'remote_execution_ssh_keys', :source => 'snippets/_remote_execution_ssh_keys.erb', :snippet => true },
     { :name => 'saltstack_minion', :source => 'snippets/_saltstack_minion.erb', :snippet => true },
@@ -95,7 +94,7 @@
   ].each do |input|
     contents = File.read(File.join("#{Rails.root}/app/views/unattended", input.delete(:source)))
 
-    if (t = ProvisioningTemplate.find_by_name(input[:name])) && !audit_modified?(ProvisioningTemplate, input[:name])
+    if (t = ProvisioningTemplate.unscoped.find_by_name(input[:name])) && !audit_modified?(ProvisioningTemplate, input[:name])
       if t.template != contents
         t.template = contents
         raise "Unable to update template #{t.name}: #{format_errors t}" unless t.save

db/seeds.d/08-partition_tables.rb

@@ -1,6 +1,6 @@
 # Partition tables
-organizations = Organization.all
-locations = Location.all
+organizations = Organization.unscoped.all
+locations = Location.unscoped.all
 Ptable.without_auditing do
   [
     { :name => 'AutoYaST entire SCSI disk', :os_family => 'Suse', :source => 'autoyast/disklayout_scsi.erb' },
@@ -19,7 +19,7 @@
   ].each do |input|
     contents = File.read(File.join("#{Rails.root}/app/views/unattended", input.delete(:source)))
 
-    if (p = Ptable.find_by_name(input[:name])) && !audit_modified?(Ptable, input[:name])
+    if (p = Ptable.unscoped.find_by_name(input[:name])) && !audit_modified?(Ptable, input[:name])
       if p.layout != contents
         p.layout = contents
         raise "Unable to update partition table: #{format_errors p}" unless p.save

db/seeds.d/10-installation_media.rb

@@ -1,4 +1,4 @@
-os_suse = Operatingsystem.where(:type => "Suse") || Operatingsystem.where("name LIKE ?", "suse")
+os_suse = Operatingsystem.unscoped.where(:type => "Suse") || Operatingsystem.unscoped.where("name LIKE ?", "suse")
 
 # Installation media: default mirrors
 Medium.without_auditing do
@@ -12,7 +12,7 @@
     { :name => "Ubuntu mirror", :os_family => "Debian", :path => "http://archive.ubuntu.com/ubuntu" },
     { :name => "CoreOS mirror", :os_family => "Coreos", :path => "http://$release.release.core-os.net" }
   ].each do |input|
-    next if Medium.where(['name = ? OR path = ?', input[:name], input[:path]]).any?
+    next if Medium.unscoped.where(['name = ? OR path = ?', input[:name], input[:path]]).any?
     next if audit_modified? Medium, input[:name]
     m = Medium.create input
     raise "Unable to create medium: #{format_errors m}" if m.nil? || m.errors.any?

test/active_support_test_case_helper.rb

@@ -9,6 +9,7 @@ class ActiveSupport::TestCase
   setup :begin_gc_deferment
   setup :reset_setting_cache
   setup :skip_if_plugin_asked_to
+  setup :set_admin
 
   teardown :reconsider_gc_deferment
   teardown :clear_current_user
@@ -40,6 +41,10 @@ def skip_if_plugin_asked_to
     end
   end
 
+  def set_admin
+    User.current = users(:admin)
+  end
+
   def clear_current_user
     User.current = nil
   end
@@ -125,7 +130,8 @@ def setup_users
   def setup_user(operation, type = "", search = nil, user = :one)
     @one = users(user)
     as_admin do
-      permission = Permission.find_by_name("#{operation}_#{type}") || FactoryGirl.create(:permission, :name => "#{operation}_#{type}")
+      permission = Permission.find_by_name("#{operation}_#{type}") ||
+        FactoryGirl.create(:permission, :name => "#{operation}_#{type}")
       filter = FactoryGirl.build(:filter, :search => search)
       filter.permissions = [ permission ]
       role = Role.where(:name => "#{operation}_#{type}").first_or_create

test/controllers/api/v1/auth_source_ldaps_controller_test.rb

@@ -19,7 +19,7 @@ class Api::V1::AuthSourceLdapsControllerTest < ActionController::TestCase
   end
 
   test "should create auth_source_ldap" do
-    assert_difference('AuthSourceLdap.count', 1) do
+    assert_difference('AuthSourceLdap.unscoped.count', 1) do
       post :create, { :auth_source_ldap => valid_attrs }
     end
     assert_response :success
@@ -31,7 +31,7 @@ class Api::V1::AuthSourceLdapsControllerTest < ActionController::TestCase
   end
 
   test "should destroy auth_source_ldap" do
-    assert_difference('AuthSourceLdap.count', -1) do
+    assert_difference('AuthSourceLdap.unscoped.count', -1) do
       auth = auth_sources(:one)
       User.where(:auth_source_id => auth.id).update_all(:auth_source_id => nil)
       delete :destroy, { :id => auth.id }

test/controllers/api/v1/compute_resources_controller_test.rb

@@ -35,12 +35,13 @@ def teardown
 
   test "should update compute resource" do
     put :update, { :id => compute_resources(:mycompute).to_param, :compute_resource => { :description => "new_description" } }
-    assert_equal "new_description", ComputeResource.find_by_name('mycompute').description
+    assert_equal "new_description",
+      ComputeResource.unscoped.find_by_name('mycompute').description
     assert_response :success
   end
 
   test "should destroy compute resource" do
-    assert_difference('ComputeResource.count', -1) do
+    assert_difference('ComputeResource.unscoped.count', -1) do
       delete :destroy, { :id => compute_resources(:yourcompute).id }
     end
     assert_response :success
@@ -66,12 +67,13 @@ def teardown
   test "should update compute resource for owner" do
     setup_user 'edit', 'compute_resources', "id = #{compute_resources(:mycompute).id}"
     put :update, { :id => compute_resources(:mycompute).to_param, :compute_resource => { :description => "new_description" } }
-    assert_equal "new_description", ComputeResource.find_by_name('mycompute').description
+    assert_equal "new_description",
+      ComputeResource.unscoped.find_by_name('mycompute').description
     assert_response :success
   end
 
   test "should destroy compute resource for owner" do
-    assert_difference('ComputeResource.count', -1) do
+    assert_difference('ComputeResource.unscoped.count', -1) do
       setup_user 'destroy', 'compute_resources', "id = #{compute_resources(:mycompute).id}"
       delete :destroy, { :id => compute_resources(:mycompute).id }
     end

test/controllers/api/v1/config_templates_controller_test.rb

@@ -51,15 +51,15 @@ class Api::V1::ConfigTemplatesControllerTest < ActionController::TestCase
     config_template = templates(:pxekickstart)
     delete :destroy, { :id => config_template.to_param }
     assert_response :unprocessable_entity
-    assert ProvisioningTemplate.exists?(config_template.id)
+    assert ProvisioningTemplate.unscoped.exists?(config_template.id)
   end
 
   test "should destroy" do
     config_template = templates(:pxekickstart)
     config_template.os_default_templates.clear
     delete :destroy, { :id => config_template.to_param }
     assert_response :success
-    refute ProvisioningTemplate.exists?(config_template.id)
+    refute ProvisioningTemplate.unscoped.exists?(config_template.id)
   end
 
   test "should build pxe menu" do