commit 943bc1a277e543d13893ded9ead1459b3c664789
Author: Ondrej Prazak <oprazak@redhat.com>
Date:   Thu Jun 21 10:29:17 2018 +0200

    Fixes #23994 - Do not update templates out of scope

diff --git a/app/models/concerns/taxonomy_collision_finder.rb b/app/models/concerns/taxonomy_collision_finder.rb
new file mode 100644
index 000000000..3ca074995
--- /dev/null
+++ b/app/models/concerns/taxonomy_collision_finder.rb
@@ -0,0 +1,12 @@
+module TaxonomyCollisionFinder
+  extend ActiveSupport::Concern
+
+  module ClassMethods
+    def find_without_collision(attribute, value)
+      from_local = find_or_initialize_by attribute => value
+      from_global = unscoped.find_or_initialize_by attribute => value
+      from_local.errors.add(attribute, _("cannot be used, please choose another")) if from_local.new_record? && from_global.persisted?
+      from_local
+    end
+  end
+end
diff --git a/app/models/provisioning_template.rb b/app/models/provisioning_template.rb
index 684bc51dc..d35dbda35 100644
--- a/app/models/provisioning_template.rb
+++ b/app/models/provisioning_template.rb
@@ -7,6 +7,7 @@ class ProvisioningTemplate < Template
   friendly_id :name
   include Parameterizable::ByIdName
   include DirtyAssociations
+  include TaxonomyCollisionFinder
 
   class << self
     # we have to override the base_class because polymorphic associations does not detect it correctly, more details at
diff --git a/app/models/ptable.rb b/app/models/ptable.rb
index 0f381358b..639b7ace2 100644
--- a/app/models/ptable.rb
+++ b/app/models/ptable.rb
@@ -12,6 +12,7 @@ class Ptable < Template
   include Parameterizable::ByIdName
   include ValidateOsFamily
   include DirtyAssociations
+  include TaxonomyCollisionFinder
 
   class << self
     # we have to override the base_class because polymorphic associations does not detect it correctly, more details at
diff --git a/app/models/template.rb b/app/models/template.rb
index 592fc0b51..639bb719b 100644
--- a/app/models/template.rb
+++ b/app/models/template.rb
@@ -94,7 +94,7 @@ class Template < ApplicationRecord
   # based on +name it either finds existing template or builds a new one
   # then it applies changes to it and return this object, note no changes were saved at this point
   def self.import_without_save(name, text, options = {})
-    template = self.unscoped.find_or_initialize_by(:name => name)
+    template = self.find_without_collision :name, name
     Foreman::Logging.logger('app').debug "#{template.new_record? ? 'building new' : 'updating existing'} template"
     template.import_without_save(text, options)
   end
@@ -117,6 +117,7 @@ class Template < ApplicationRecord
   #   :default - default flag value (false by default)
   def self.import!(name, text, options = {})
     template = import_without_save(name, text, options)
+    return template unless template.valid?
     if options[:force]
       template.ignore_locking { template.save! }
     else
@@ -125,6 +126,11 @@ class Template < ApplicationRecord
     template
   end
 
+  # override in subclass to handle taxonomy scope, see TaxonomyCollisionFinder
+  def self.find_without_collision(attribute, name)
+    self.find_or_initialize_by :name => name
+  end
+
   private
 
   # This method can be overridden in Template children classes to import additional attributes
diff --git a/test/models/template_test.rb b/test/models/template_test.rb
index 0ff842ed8..0364fc46c 100644
--- a/test/models/template_test.rb
+++ b/test/models/template_test.rb
@@ -177,6 +177,7 @@ EOS
     describe '.import!' do
       test 'by default it does not ignore locking' do
         template = Minitest::Mock.new
+        template.expect(:valid?, true)
         template.expect(:save!, true)
         Template.expects(:import_without_save => template)
         Template.import!('test', '')
@@ -185,6 +186,7 @@ EOS
 
       test 'locking can be overriden by force option' do
         template = Minitest::Mock.new
+        template.expect(:valid?, true)
         template.expect(:ignore_locking, true)
         Template.expects(:import_without_save => template)
         Template.import!('test', '', { :force => true })
@@ -411,5 +413,38 @@ EOS
         refute_includes @template.operatingsystem_ids, @os2.id
       end
     end
+
+    describe '::find_without_name_collision in subclasses' do
+      setup do
+        @org = FactoryBot.create(:organization, :name => 'TemplateOrg')
+        @empty = FactoryBot.create(:organization, :name => 'EmptyOrg')
+        @regular_template = FactoryBot.create(:provisioning_template, :name => 'regular template', :organizations => [@org])
+        @collision_template = FactoryBot.create(:provisioning_template, :name => 'collision template', :organizations => [@empty])
+        @common_template = FactoryBot.create(:provisioning_template, :name => 'common template', :organizations => [@empty, @org])
+        @before_org = Organization.current
+        Organization.current = @org
+      end
+
+      test 'should initialize a new template' do
+        template = ProvisioningTemplate.find_without_collision(:name, 'new template')
+        assert template.new_record?
+        assert template.errors.empty?
+      end
+
+      test 'should return existing template in current context' do
+        assert_equal @regular_template, ProvisioningTemplate.find_without_collision(:name, 'regular template')
+      end
+
+      test 'should return existing template if persent in multiple contexts' do
+        assert_equal @common_template, ProvisioningTemplate.find_without_collision(:name, 'common template')
+      end
+
+      test 'should return new instance with error when outside of current context' do
+        template = ProvisioningTemplate.find_without_collision(:name, 'collision template')
+        assert template.new_record?
+        refute template.errors.empty?
+        assert_equal "cannot be used, please choose another", template.errors.messages[:name].first
+      end
+    end
   end
 end
