diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index adadf988..f7076fec 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2021-06-09 10:26:01 UTC using RuboCop version 1.16.0. +# on 2021-07-28 22:19:11 UTC using RuboCop version 1.16.0. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new @@ -18,7 +18,7 @@ Lint/RequireParentheses: Exclude: - 'app/controllers/concerns/foreman_puppet/extensions/hosts_controller_extensions.rb' -# Offense count: 42 +# Offense count: 44 # Configuration parameters: IgnoredMethods, CountRepeatedAttributes. Metrics/AbcSize: Max: 41 @@ -34,17 +34,17 @@ Metrics/BlockLength: Metrics/ClassLength: Max: 250 -# Offense count: 16 +# Offense count: 18 # Configuration parameters: IgnoredMethods. Metrics/CyclomaticComplexity: Max: 17 -# Offense count: 38 +# Offense count: 39 # Configuration parameters: CountComments, CountAsOne, ExcludedMethods, IgnoredMethods. Metrics/MethodLength: Max: 23 -# Offense count: 3 +# Offense count: 4 # Configuration parameters: CountComments, CountAsOne. Metrics/ModuleLength: Max: 171 @@ -54,16 +54,17 @@ Metrics/ModuleLength: Metrics/ParameterLists: MaxOptionalParameters: 4 -# Offense count: 15 +# Offense count: 17 # Configuration parameters: IgnoredMethods. Metrics/PerceivedComplexity: Max: 18 -# Offense count: 1 +# Offense count: 6 # Configuration parameters: EnforcedStyle, IgnoredPatterns. # SupportedStyles: snake_case, camelCase Naming/MethodName: Exclude: + - 'app/models/concerns/foreman_puppet/orchestration/puppetca.rb' - 'app/models/foreman_puppet/environment.rb' # Offense count: 2 @@ -96,7 +97,7 @@ Rails/SkipsModelValidations: Exclude: - 'test/controllers/foreman_puppet/api/v2/environments_controller_test.rb' -# Offense count: 198 +# Offense count: 213 # Cop supports --auto-correct. # Configuration parameters: EnforcedStyle. # SupportedStyles: always, always_true, never @@ -130,7 +131,7 @@ Style/SymbolProc: Exclude: - 'app/views/foreman_puppet/api/v2/import_puppetclasses/show.json.rabl' -# Offense count: 272 +# Offense count: 279 # Cop supports --auto-correct. # Configuration parameters: AutoCorrect, AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns. # URISchemes: http, https diff --git a/app/models/concerns/foreman_puppet/extensions/host.rb b/app/models/concerns/foreman_puppet/extensions/host.rb index 39416982..95ad45a3 100644 --- a/app/models/concerns/foreman_puppet/extensions/host.rb +++ b/app/models/concerns/foreman_puppet/extensions/host.rb @@ -6,9 +6,7 @@ module Host included do prepend PrependedMethods - if SETTINGS[:unattended] - include ForemanPuppet::Orchestration::Puppetca - end + include ForemanPuppet::Orchestration::Puppetca if SETTINGS[:unattended] if ForemanPuppet.extracted_from_core? has_one :environment, through: :puppet, class_name: 'ForemanPuppet::Environment' @@ -20,7 +18,7 @@ module Host host_classes_assoc&.instance_variable_set(:@class_name, 'ForemanPuppet::HostClass') end - smart_proxy_reference :self => [:puppet_proxy_id, :puppet_ca_proxy_id] + smart_proxy_reference self: %i[puppet_proxy_id puppet_ca_proxy_id] before_save :check_puppet_ca_proxy_is_required? @@ -34,7 +32,6 @@ module Host scoped_search relation: :config_groups, on: :name, complete_value: true, rename: :config_group, only_explicit: true, operators: ['= ', '~ '], ext_method: :search_by_config_group - apipie :class do property :puppetca_token, 'Token::Puppetca', desc: 'Returns Puppet CA token for this host' end @@ -44,10 +41,8 @@ module Host # fall back to our puppet proxy in case our puppet ca is not defined/used. def check_puppet_ca_proxy_is_required? return true if puppet_ca_proxy_id.present? || puppet_proxy_id.blank? - if puppet_proxy.has_feature?('Puppet CA') - self.puppet_ca_proxy ||= puppet_proxy - end - rescue + self.puppet_ca_proxy ||= puppet_proxy if puppet_proxy.has_feature?('Puppet CA') + rescue StandardError true # we don't want to break anything, so just skipping. end end diff --git a/app/models/concerns/foreman_puppet/orchestration/puppetca.rb b/app/models/concerns/foreman_puppet/orchestration/puppetca.rb index bb4bb040..7114207f 100644 --- a/app/models/concerns/foreman_puppet/orchestration/puppetca.rb +++ b/app/models/concerns/foreman_puppet/orchestration/puppetca.rb @@ -1,111 +1,112 @@ module ForemanPuppet - module Orchestration::Puppetca - extend ActiveSupport::Concern - include Orchestration::Common - - included do - attr_reader :puppetca - after_validation :initialize_puppetca, :unless => :skip_orchestration? - after_validation :queue_puppetca - before_destroy :initialize_puppetca, :queue_puppetca_destroy - end + module Orchestration + module Puppetca + extend ActiveSupport::Concern + include Orchestration::Common - protected + included do + attr_reader :puppetca - def initialize_puppetca - return unless puppetca? - return unless Setting[:manage_puppetca] - @puppetca = ProxyAPI::Puppetca.new :url => puppet_ca_proxy.url - true - rescue => e - failure _("Failed to initialize the PuppetCA proxy: %s") % e, e - end + after_validation :initialize_puppetca, unless: :skip_orchestration? + after_validation :queue_puppetca + before_destroy :initialize_puppetca, :queue_puppetca_destroy + end - # Removes the host's puppet certificate from the puppetmaster's CA - def delCertificate - logger.info "Remove puppet certificate for #{name}" - puppetca.del_certificate certname - end + protected - # Empty method for rollbacks - maybe in the future we would support creating the certificates directly - def setCertificate - end + def initialize_puppetca + return unless puppetca? + return unless Setting[:manage_puppetca] + @puppetca = ProxyAPI::Puppetca.new url: puppet_ca_proxy.url + true + rescue StandardError => e + failure _('Failed to initialize the PuppetCA proxy: %s') % e, e + end - # Reset certname based on whether to use uuids or the hostname - def resetCertname - logger.info "Resetting certname for #{name}" - self.certname = Setting[:use_uuid_for_certificates] ? Foreman.uuid : hostname - end + # Removes the host's puppet certificate from the puppetmaster's CA + def delCertificate + logger.info "Remove puppet certificate for #{name}" + puppetca.del_certificate certname + end - # Adds the host's name to the autosign.conf file - def setAutosign - logger.info "Adding autosign entry for #{name}" - response = puppetca.set_autosign certname - # return if puppetca is using basic autosigning - return response if response.in? [true, false] - unless response.is_a?(Hash) && response['generated_token'].present? - logger.warn "Received an unexpected smart proxy response: #{response}" - return false + # Empty method for rollbacks - maybe in the future we would support creating the certificates directly + def setCertificate end - puppet.create_puppetca_token value: response['generated_token'] - end - # Removes the host's name from the autosign.conf file - def delAutosign - logger.info "Delete the autosign entry for #{name}" - puppetca_token.destroy! if puppetca_token.present? - puppetca.del_autosign certname - end + # Reset certname based on whether to use uuids or the hostname + def resetCertname + logger.info "Resetting certname for #{name}" + self.certname = Setting[:use_uuid_for_certificates] ? Foreman.uuid : hostname + end - private + # Adds the host's name to the autosign.conf file + def setAutosign + logger.info "Adding autosign entry for #{name}" + response = puppetca.set_autosign certname + # return if puppetca is using basic autosigning + return response if response.in? [true, false] + unless response.is_a?(Hash) && response['generated_token'].present? + logger.warn "Received an unexpected smart proxy response: #{response}" + return false + end + puppet.create_puppetca_token value: response['generated_token'] + end - def queue_puppetca - return log_orchestration_errors unless puppetca? && errors.empty? - return unless Setting[:manage_puppetca] - new_record? ? queue_puppetca_create : queue_puppetca_update - end + # Removes the host's name from the autosign.conf file + def delAutosign + logger.info "Delete the autosign entry for #{name}" + puppetca_token.destroy! if puppetca_token.present? + puppetca.del_autosign certname + end - def queue_puppetca_certname_reset - post_queue.create(:name => _("Reset PuppetCA certname for %s") % self, :priority => 49, - :action => [self, :resetCertname]) - end + private - def queue_puppetca_create - post_queue.create(:name => _("Cleanup PuppetCA certificates for %s") % self, :priority => 51, - :action => [self, :delCertificate]) - post_queue.create(:name => _("Enable PuppetCA autosigning for %s") % self, :priority => 55, - :action => [self, :setAutosign]) - end + def queue_puppetca + return log_orchestration_errors unless puppetca? && errors.empty? + return unless Setting[:manage_puppetca] + new_record? ? queue_puppetca_create : queue_puppetca_update + end - def queue_puppetca_update - if old.build? && !build? - # Host has been built --> remove auto sign - queue_puppetca_autosign_destroy - elsif !old.build? && build? - # Host was set to build mode - # If use_uuid_for_certificates is true, reuse the certname UUID value. - # If false, then reset the certname if it does not match the hostname. - if (Setting[:use_uuid_for_certificates] ? !Foreman.is_uuid?(certname) : certname != hostname) - queue_puppetca_certname_reset + def queue_puppetca_certname_reset + post_queue.create(name: _('Reset PuppetCA certname for %s') % self, priority: 49, + action: [self, :resetCertname]) + end + + def queue_puppetca_create + post_queue.create(name: _('Cleanup PuppetCA certificates for %s') % self, priority: 51, + action: [self, :delCertificate]) + post_queue.create(name: _('Enable PuppetCA autosigning for %s') % self, priority: 55, + action: [self, :setAutosign]) + end + + def queue_puppetca_update + if old.build? && !build? + # Host has been built --> remove auto sign + queue_puppetca_autosign_destroy + elsif !old.build? && build? + # Host was set to build mode + # If use_uuid_for_certificates is true, reuse the certname UUID value. + # If false, then reset the certname if it does not match the hostname. + queue_puppetca_certname_reset if Setting[:use_uuid_for_certificates] ? !Foreman.is_uuid?(certname) : certname != hostname + queue_puppetca_autosign_destroy + queue_puppetca_create end - queue_puppetca_autosign_destroy - queue_puppetca_create + true end - true - end - def queue_puppetca_destroy - return unless puppetca? && errors.empty? - return unless Setting[:manage_puppetca] - post_queue.create(:name => _("Delete PuppetCA certificates for %s") % self, :priority => 59, - :action => [self, :delCertificate]) - queue_puppetca_autosign_destroy - true - end + def queue_puppetca_destroy + return unless puppetca? && errors.empty? + return unless Setting[:manage_puppetca] + post_queue.create(name: _('Delete PuppetCA certificates for %s') % self, priority: 59, + action: [self, :delCertificate]) + queue_puppetca_autosign_destroy + true + end - def queue_puppetca_autosign_destroy - post_queue.create(:name => _("Disable PuppetCA autosigning for %s") % self, :priority => 50, - :action => [self, :delAutosign]) + def queue_puppetca_autosign_destroy + post_queue.create(name: _('Disable PuppetCA autosigning for %s') % self, priority: 50, + action: [self, :delAutosign]) + end end end end diff --git a/app/models/foreman_puppet/host_puppet_facet.rb b/app/models/foreman_puppet/host_puppet_facet.rb index 269fdb6d..3dfd92a3 100644 --- a/app/models/foreman_puppet/host_puppet_facet.rb +++ b/app/models/foreman_puppet/host_puppet_facet.rb @@ -6,7 +6,7 @@ class HostPuppetFacet < ApplicationRecord include Facets::Base include ForemanPuppet::PuppetFacetCommon - has_one :puppetca_token, :foreign_key => :host_id, :dependent => :destroy, :inverse_of => :host, :class_name => 'ForemanPuppet::Token::Puppetca' + has_one :puppetca_token, foreign_key: :host_id, dependent: :destroy, inverse_of: :host, class_name: 'ForemanPuppet::Token::Puppetca' has_many :host_classes, dependent: :destroy, class_name: 'ForemanPuppet::HostClass' has_many :puppetclasses, through: :host_classes diff --git a/app/models/foreman_puppet/token/puppetca.rb b/app/models/foreman_puppet/token/puppetca.rb index 032b8488..2f9cc32c 100644 --- a/app/models/foreman_puppet/token/puppetca.rb +++ b/app/models/foreman_puppet/token/puppetca.rb @@ -1,5 +1,7 @@ module ForemanPuppet - class Token::Puppetca < ::Token - validates :value, uniqueness: true + module Token + class Puppetca < ::Token + validates :value, uniqueness: true + end end end diff --git a/test/models/foreman_puppet/orchestration/puppet_ca_orchestration_test.rb b/test/models/foreman_puppet/orchestration/puppet_ca_orchestration_test.rb index 55870185..42f3d333 100644 --- a/test/models/foreman_puppet/orchestration/puppet_ca_orchestration_test.rb +++ b/test/models/foreman_puppet/orchestration/puppet_ca_orchestration_test.rb @@ -2,7 +2,7 @@ class PuppetCaOrchestrationTest < ActiveSupport::TestCase def setup - users(:one).roles << Role.find_by_name('Manager') + users(:one).roles << Role.find_by(name: 'Manager') User.current = users(:one) disable_orchestration Setting[:manage_puppetca] = true @@ -15,7 +15,7 @@ def teardown context 'a host with puppetca orchestration' do context 'when entering build mode on creation' do - let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, :build => true) } + let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, build: true) } test 'should queue puppetca autosigning' do assert_valid host @@ -45,7 +45,7 @@ def teardown end context 'when reentering build mode' do - let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, :build => false) } + let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, build: false) } setup do @host = host @@ -64,7 +64,7 @@ def teardown end context 'when reentering build mode after certname setting was changed' do - let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, :build => false) } + let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, build: false) } test 'should reset certname when changing from hostname to uuid' do assert_valid host @@ -98,7 +98,7 @@ def teardown end context 'when host leaves build mode' do - let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, :build => true) } + let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, build: true) } setup do @host = host @@ -115,19 +115,19 @@ def teardown end context 'when host is updated' do - let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, :build => false) } + let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, build: false) } test 'should not queue anything if build mode is not changed' do assert_valid host host.post_queue.clear - host.comment = "updated" + host.comment = 'updated' host.save! assert_equal 0, host.post_queue.all.size end end context 'when host gets destroyed' do - let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, :build => false) } + let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, build: false) } test 'should queue puppetca destroy' do assert_valid host @@ -141,7 +141,7 @@ def teardown end context 'handles smart proxy responses correctly' do - let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, :build => true) } + let(:host) { FactoryBot.create(:host, :managed, :with_puppet_ca, build: true) } setup do @host = host @@ -156,7 +156,7 @@ def teardown test 'when autosigning fails' do @host.puppetca.stubs(:set_autosign).with(@host.certname).returns(false) - refute @host.send(:setAutosign) + assert_not @host.send(:setAutosign) assert_nil @host.puppetca_token end @@ -165,19 +165,19 @@ def teardown @host.puppetca.stubs(:set_autosign).with(@host.certname).returns(spresponse) assert @host.send(:setAutosign) assert_valid @host.puppetca_token - assert_equal @host.puppetca_token.value, 'foo42' + assert_equal('foo42', @host.puppetca_token.value) end test 'when it gets an invalid hash response' do spresponse = { 'not_a_token' => '' } @host.puppetca.stubs(:set_autosign).with(@host.certname).returns(spresponse) - refute @host.send(:setAutosign) + assert_not @host.send(:setAutosign) assert_nil @host.puppetca_token end test 'when it gets an invalid nil response' do @host.puppetca.stubs(:set_autosign).with(@host.certname).returns(nil) - refute @host.send(:setAutosign) + assert_not @host.send(:setAutosign) assert_nil @host.puppetca_token end end diff --git a/test/models/foreman_puppet/token/puppetca_test.rb b/test/models/foreman_puppet/token/puppetca_test.rb index 5fcb7cb2..2f5fc254 100644 --- a/test/models/foreman_puppet/token/puppetca_test.rb +++ b/test/models/foreman_puppet/token/puppetca_test.rb @@ -1,24 +1,26 @@ require 'test_puppet_helper' module ForemanPuppet - class Token::PuppetcaTest < ActiveSupport::TestCase - should validate_uniqueness_of(:value) + module Token + class PuppetcaTest < ActiveSupport::TestCase + should validate_uniqueness_of(:value) - let(:host) { FactoryBot.create(:host) } + let(:host) { FactoryBot.create(:host) } - test "a host can create a puppetca-token" do - host.create_puppetca_token value: 'foo.bar.baz' - assert_instance_of Token::Puppetca, host.puppetca_token - assert_equal Token::Puppetca.first.host_id, host.id - assert_equal 'foo.bar.baz', host.puppetca_token.value - end + test 'a host can create a puppetca-token' do + host.create_puppetca_token value: 'foo.bar.baz' + assert_instance_of Token::Puppetca, host.puppetca_token + assert_equal Token::Puppetca.first.host_id, host.id + assert_equal 'foo.bar.baz', host.puppetca_token.value + end - test "a host can delete its puppetca-token" do - host.create_puppetca_token value: 'aaaa' - assert_equal host.puppetca_token.value, 'aaaa' - host.puppetca_token = nil - assert_nil host.puppetca_token - assert_equal Token::Puppetca.all, [] + test 'a host can delete its puppetca-token' do + host.create_puppetca_token value: 'aaaa' + assert_equal('aaaa', host.puppetca_token.value) + host.puppetca_token = nil + assert_nil host.puppetca_token + assert_equal([], Token::Puppetca.all) + end end end end