From 826715eef4eabdebe048e74befa6214cb45efeae Mon Sep 17 00:00:00 2001 From: Manuel Laug Date: Tue, 7 May 2024 10:30:44 +0200 Subject: [PATCH] Support Foreman >= 3.9 - migrate to settings DSL - drop old Foreman version support - switch to upstream CI - switch to upstream rubocop - add access permission test --- .github/workflows/ci.yml | 105 +++++------------- .rubocop.yml | 14 ++- Rakefile | 4 +- .../api/v2/vault_connections_controller.rb | 3 +- .../vault_connections_controller.rb | 3 +- .../orchestration/vault_policy.rb | 6 +- app/models/vault_connection.rb | 6 +- .../foreman_vault/vault_auth_method.rb | 3 +- app/services/foreman_vault/vault_policy.rb | 1 + ...2504_fix_vault_settings_category_to_dsl.rb | 2 - db/seeds.d/103-provisioning_templates.rb | 4 +- foreman_vault.gemspec | 4 +- lib/foreman_vault/engine.rb | 46 ++++---- lib/tasks/foreman_vault_tasks.rake | 53 +++------ .../foreman_vault/access_permissions_test.rb | 18 +++ test/unit/lib/foreman_vault/macros_test.rb | 2 +- .../foreman_vault/vault_auth_method_test.rb | 8 +- .../foreman_vault/vault_client_test.rb | 8 +- 18 files changed, 118 insertions(+), 172 deletions(-) mode change 100644 => 100755 Rakefile create mode 100644 test/unit/foreman_vault/access_permissions_test.rb diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4ce3512..70a7e35 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,85 +1,34 @@ -name: CI -on: [push, pull_request] -env: - RAILS_ENV: test - DATABASE_URL: postgresql://postgres:@localhost/test - DATABASE_CLEANER_ALLOW_REMOTE_DATABASE_URL: true +name: Ruby test + +on: + pull_request: + push: + branches: + - master + +concurrency: + group: ${{ github.ref_name }}-${{ github.workflow }} + cancel-in-progress: true + jobs: rubocop: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v3 - - name: Setup Ruby - uses: ruby/setup-ruby@v1 - with: - ruby-version: 2.7 - - name: Setup - run: | - gem install bundler - echo "gem 'rdoc', '< 6.4'" >> Gemfile - bundle install --jobs=3 --retry=3 - - name: Run rubocop - run: bundle exec rubocop + uses: theforeman/actions/.github/workflows/rubocop.yml@v0 + with: + command: bundle exec rubocop --parallel --format github + test: - runs-on: ubuntu-latest + name: Ruby needs: rubocop - services: - postgres: - image: postgres:12.1 - ports: ['5432:5432'] - options: --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 5 strategy: fail-fast: false matrix: - foreman-core-branch: [3.1-stable, 3.2-stable, 3.3-stable, 3.4-stable, 3.5-stable, 3.6-stable, develop] - ruby-version: [2.7] - node-version: [14] - steps: - - name: Install dependencies - run: | - sudo apt-get update - sudo apt-get install build-essential libcurl4-openssl-dev libvirt-dev ruby-libvirt zlib1g-dev libpq-dev - - uses: actions/checkout@v3 - with: - repository: theforeman/foreman - ref: ${{ matrix.foreman-core-branch }} - - uses: actions/checkout@v3 - with: - path: foreman_vault - - name: Setup Ruby - uses: ruby/setup-ruby@v1 - with: - ruby-version: ${{ matrix.ruby-version }} - - name: Setup Node - uses: actions/setup-node@v3 - with: - node-version: ${{ matrix.node-version }} - - name: Setup Bundler - run: | - gem install bundler - bundle config path vendor/bundle - bundle config set without journald development console mysql2 sqlite - echo "gem 'foreman_vault', path: './foreman_vault'" > bundler.d/foreman_vault.local.rb - bundle lock --update - - name: Cache gems - uses: actions/cache@v3 - with: - path: vendor/bundle - key: ${{ runner.os }}-gems-${{ hashFiles('**/Gemfile.lock') }} - restore-keys: | - ${{ runner.os }}-gems- - - name: Setup Plugin - run: | - bundle install --jobs=3 --retry=3 - bundle exec rake db:create - bundle exec rake db:migrate - npm install - bundle exec rake webpack:compile - - name: Run plugin tests - run: | - bundle exec rake test:foreman_vault - bundle exec rake test TEST="test/unit/foreman/access_permissions_test.rb" - - name: Precompile plugin assets - run: bundle exec rake 'plugin:assets:precompile[foreman_vault]' - env: - RAILS_ENV: production + foreman: + - "develop" + - "3.10-stable" + - "3.9-stable" + uses: theforeman/actions/.github/workflows/foreman_plugin.yml@v0 + with: + plugin: foreman_vault + foreman_version: ${{ matrix.foreman }} + environment_variables: | + FOREMAN_VERSION=${{ matrix.foreman }} diff --git a/.rubocop.yml b/.rubocop.yml index 6c6dee8..79c97a8 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,8 +1,6 @@ -AllCops: - TargetRubyVersion: 2.3 - TargetRailsVersion: 5.1 - Exclude: - - 'Rakefile' +inherit_gem: + theforeman-rubocop: + - default.yml Rails: Enabled: true @@ -11,7 +9,7 @@ Rails: Style/Documentation: Enabled: false -Metrics/LineLength: +Layout/LineLength: Max: 190 Metrics/ParameterLists: @@ -34,6 +32,10 @@ Metrics/BlockLength: - lib/foreman_vault/engine.rb - 'test/**/*' +Style/FrozenStringLiteralComment: + Exclude: + - Rakefile + Metrics/ClassLength: Exclude: - 'test/**/*' diff --git a/Rakefile b/Rakefile old mode 100644 new mode 100755 index 9800b98..73e4bc0 --- a/Rakefile +++ b/Rakefile @@ -20,7 +20,7 @@ RDoc::Task.new(:rdoc) do |rdoc| rdoc.rdoc_files.include('lib/**/*.rb') end -APP_RAKEFILE = File.expand_path('../test/dummy/Rakefile', __FILE__) +APP_RAKEFILE = File.expand_path('test/dummy/Rakefile', __dir__) Bundler::GemHelper.install_tasks @@ -38,7 +38,7 @@ task default: :test begin require 'rubocop/rake_task' RuboCop::RakeTask.new -rescue => _ +rescue StandardError => _e puts 'Rubocop not loaded.' end diff --git a/app/controllers/api/v2/vault_connections_controller.rb b/app/controllers/api/v2/vault_connections_controller.rb index 0741c33..f718d5a 100644 --- a/app/controllers/api/v2/vault_connections_controller.rb +++ b/app/controllers/api/v2/vault_connections_controller.rb @@ -16,7 +16,8 @@ def index api :GET, '/vault_connections/:id', N_('Show VaultConnection details') param :id, :identifier, required: true - def show; end + def show + end def_param_group :vault_connection do param :vault_connection, Hash, action_aware: true, required: true do diff --git a/app/controllers/vault_connections_controller.rb b/app/controllers/vault_connections_controller.rb index 9756242..30c32e8 100644 --- a/app/controllers/vault_connections_controller.rb +++ b/app/controllers/vault_connections_controller.rb @@ -22,7 +22,8 @@ def create end end - def edit; end + def edit + end def update if @vault_connection.update(vault_connection_params) diff --git a/app/models/concerns/foreman_vault/orchestration/vault_policy.rb b/app/models/concerns/foreman_vault/orchestration/vault_policy.rb index f2ed05b..0141284 100644 --- a/app/models/concerns/foreman_vault/orchestration/vault_policy.rb +++ b/app/models/concerns/foreman_vault/orchestration/vault_policy.rb @@ -21,7 +21,7 @@ def queue_vault_push return unless vault_auth_method.valid? queue.create(name: _('Push %s data to Vault') % self, priority: 100, - action: [self, :set_vault]) + action: [self, :set_vault]) end def queue_vault_destroy @@ -30,10 +30,9 @@ def queue_vault_destroy return unless vault_auth_method.valid? queue.create(name: _('Clear %s Vault data') % self, priority: 60, - action: [self, :del_vault]) + action: [self, :del_vault]) end - # rubocop:disable Metrics/AbcSize def set_vault logger.info "Pushing #{name} data to Vault" @@ -44,7 +43,6 @@ def set_vault Foreman::Logging.exception("Failed to push #{name} data to Vault.", e) failure format(_('Failed to push %{name} data to Vault: %{message}\n '), name: name, message: e.message), e end - # rubocop:enable Metrics/AbcSize def del_vault logger.info "Clearing #{name} Vault data" diff --git a/app/models/vault_connection.rb b/app/models/vault_connection.rb index caf2322..b2300ec 100644 --- a/app/models/vault_connection.rb +++ b/app/models/vault_connection.rb @@ -7,7 +7,7 @@ class VaultConnection < ApplicationRecord validates :name, presence: true, uniqueness: true validates :name, inclusion: { in: ->(i) { [i.name_was] }, message: _('cannot be changed after creation') }, on: :update validates :url, presence: true - validates :url, format: URI.regexp(['http', 'https']) + validates :url, format: URI::DEFAULT_PARSER.make_regexp(['http', 'https']) validates :token, presence: true, if: -> { role_id.nil? || secret_id.nil? } validates :token, inclusion: { in: [nil], message: _('AppRole or token must be blank') }, unless: -> { role_id.nil? || secret_id.nil? } @@ -25,8 +25,8 @@ class VaultConnection < ApplicationRecord scope :with_valid_token, -> { with_token.where(vault_error: nil).where('expire_time > ?', Time.zone.now) } delegate :fetch_expire_time, :fetch_secret, :issue_certificate, - :policy, :policies, :put_policy, :delete_policy, - :set_certificate, :certificates, :delete_certificate, to: :client + :policy, :policies, :put_policy, :delete_policy, + :set_certificate, :certificates, :delete_certificate, to: :client def with_token? token.present? diff --git a/app/services/foreman_vault/vault_auth_method.rb b/app/services/foreman_vault/vault_auth_method.rb index 1183116..7fb46f8 100644 --- a/app/services/foreman_vault/vault_auth_method.rb +++ b/app/services/foreman_vault/vault_auth_method.rb @@ -31,6 +31,7 @@ def delete private attr_reader :host + delegate :vault_policy, :vault_connection, :fqdn, to: :host delegate :name, to: :vault_policy, prefix: true delegate :set_certificate, :delete_certificate, to: :vault_connection @@ -39,7 +40,7 @@ def options { certificate: certificate, token_policies: vault_policy_name, - allowed_common_names: allowed_common_names + allowed_common_names: allowed_common_names, } end diff --git a/app/services/foreman_vault/vault_policy.rb b/app/services/foreman_vault/vault_policy.rb index 0815609..c21012f 100644 --- a/app/services/foreman_vault/vault_policy.rb +++ b/app/services/foreman_vault/vault_policy.rb @@ -37,6 +37,7 @@ def delete private attr_reader :host + delegate :params, :render_template, :vault_connection, to: :host delegate :policy, :policies, :put_policy, :delete_policy, to: :vault_connection diff --git a/db/migrate/20230309072504_fix_vault_settings_category_to_dsl.rb b/db/migrate/20230309072504_fix_vault_settings_category_to_dsl.rb index 49d2495..fd1ce71 100644 --- a/db/migrate/20230309072504_fix_vault_settings_category_to_dsl.rb +++ b/db/migrate/20230309072504_fix_vault_settings_category_to_dsl.rb @@ -2,8 +2,6 @@ class FixVaultSettingsCategoryToDsl < ActiveRecord::Migration[6.0] def up - # rubocop:disable Rails/SkipsModelValidations Setting.where(category: 'Setting::Vault').update_all(category: 'Setting') if column_exists?(:settings, :category) - # rubocop:enable Rails/SkipsModelValidations end end diff --git a/db/seeds.d/103-provisioning_templates.rb b/db/seeds.d/103-provisioning_templates.rb index feb5084..3e61cba 100644 --- a/db/seeds.d/103-provisioning_templates.rb +++ b/db/seeds.d/103-provisioning_templates.rb @@ -5,8 +5,8 @@ { name: 'Default Vault Policy', source: 'VaultPolicy/default.erb', - template_kind: TemplateKind.find_or_create_by(name: 'VaultPolicy') - } + template_kind: TemplateKind.find_or_create_by(name: 'VaultPolicy'), + }, ] templates.each do |template| diff --git a/foreman_vault.gemspec b/foreman_vault.gemspec index f9f994d..0617293 100644 --- a/foreman_vault.gemspec +++ b/foreman_vault.gemspec @@ -14,8 +14,10 @@ Gem::Specification.new do |s| s.files = Dir['{app,config,db,lib,locale}/**/*'] + ['LICENSE', 'Rakefile', 'README.md'] s.test_files = Dir['test/**/*'] + s.required_ruby_version = '>= 2.5', '< 4' + s.add_dependency 'vault', '~> 0.1' s.add_development_dependency 'rdoc' - s.add_development_dependency 'rubocop', '0.54.0' + s.add_development_dependency 'theforeman-rubocop', '~> 0.1.2' end diff --git a/lib/foreman_vault/engine.rb b/lib/foreman_vault/engine.rb index 4e2cea0..ee26621 100644 --- a/lib/foreman_vault/engine.rb +++ b/lib/foreman_vault/engine.rb @@ -40,24 +40,24 @@ class Engine < ::Rails::Engine settings do category(:vault, N_('Vault')) do setting('vault_connection', - full_name: N_('Default Vault connection'), - type: :string, - description: N_('Default Vault Connection that can be override using parameters'), - default: VaultConnection.table_exists? && VaultConnection.unscoped.count == 1 ? VaultConnection.unscoped.first.name : nil, - collection: VaultConnection.table_exists? ? proc { Hash[VaultConnection.unscoped.all.map { |vc| [vc.name, vc.name] }] } : [], - include_blank: _('Select Vault Connection')) + full_name: N_('Default Vault connection'), + type: :string, + description: N_('Default Vault Connection that can be override using parameters'), + default: VaultConnection.table_exists? && VaultConnection.unscoped.count == 1 ? VaultConnection.unscoped.first.name : nil, + collection: VaultConnection.table_exists? ? proc { Hash[VaultConnection.unscoped.all.map { |vc| [vc.name, vc.name] }] } : [], + include_blank: _('Select Vault Connection')) setting('vault_policy_template', - full_name: N_('Vault Policy template name'), - type: :string, - description: N_('The name of the ProvisioningTemplate that will be used for Vault Policy'), - default: ProvisioningTemplate.unscoped.of_kind(:VaultPolicy).find_by(name: 'Default Vault Policy')&.name, - collection: proc { Hash[ProvisioningTemplate.unscoped.of_kind(:VaultPolicy).map { |tmpl| [tmpl.name, tmpl.name] }] }, - include_blank: _('Select Template')) + full_name: N_('Vault Policy template name'), + type: :string, + description: N_('The name of the ProvisioningTemplate that will be used for Vault Policy'), + default: ProvisioningTemplate.unscoped.of_kind(:VaultPolicy).find_by(name: 'Default Vault Policy')&.name, + collection: proc { Hash[ProvisioningTemplate.unscoped.of_kind(:VaultPolicy).map { |tmpl| [tmpl.name, tmpl.name] }] }, + include_blank: _('Select Template')) setting('vault_orchestration_enabled', - full_name: N_('Vault Orchestration enabled'), - type: :boolean, - description: N_('Enable or disable the Vault orchestration step for managing policies and auth methods'), - default: false) + full_name: N_('Vault Orchestration enabled'), + type: :boolean, + description: N_('Enable or disable the Vault orchestration step for managing policies and auth methods'), + default: false) end end @@ -69,14 +69,12 @@ class Engine < ::Rails::Engine end config.to_prepare do - begin - ::Host::Managed.include(ForemanVault::HostExtensions) - ::ProvisioningTemplate.include(ForemanVault::ProvisioningTemplateExtensions) - ::Foreman::Renderer::Scope::Base.include(ForemanVault::Macros) - ::Foreman::Renderer.configure { |c| c.allowed_generic_helpers += [:vault_secret, :vault_issue_certificate] } - rescue StandardError => e - Rails.logger.warn "ForemanVault: skipping engine hook (#{e})" - end + ::Host::Managed.include(ForemanVault::HostExtensions) + ::ProvisioningTemplate.include(ForemanVault::ProvisioningTemplateExtensions) + ::Foreman::Renderer::Scope::Base.include(ForemanVault::Macros) + ::Foreman::Renderer.configure { |c| c.allowed_generic_helpers += [:vault_secret, :vault_issue_certificate] } + rescue StandardError => e + Rails.logger.warn "ForemanVault: skipping engine hook (#{e})" end initializer 'foreman_vault.register_gettext', after: :load_config_initializers do |_app| diff --git a/lib/tasks/foreman_vault_tasks.rake b/lib/tasks/foreman_vault_tasks.rake index c5add60..6193815 100644 --- a/lib/tasks/foreman_vault_tasks.rake +++ b/lib/tasks/foreman_vault_tasks.rake @@ -11,16 +11,14 @@ namespace :foreman_vault do # rubocop:disable Metrics/BlockLength hosts = Host::Managed.where(managed: true) hosts.each_with_index do |host, index| - begin - result = host.reload.vault_auth_method.save - if result - puts "[#{index + 1}/#{hosts.count}] Auth-Method of \"#{host.name}\" pushed to Vault server \"#{host.vault_connection.url}\"" - else - puts "[#{index + 1}/#{hosts.count}] Failed to push \"#{host.name}\": #{result}" - end - rescue StandardError => err - puts "[#{index + 1}/#{hosts.count}] Failed to push \"#{host.name}\": #{err}" + result = host.reload.vault_auth_method.save + if result + puts "[#{index + 1}/#{hosts.count}] Auth-Method of \"#{host.name}\" pushed to Vault server \"#{host.vault_connection.url}\"" + else + puts "[#{index + 1}/#{hosts.count}] Failed to push \"#{host.name}\": #{result}" end + rescue StandardError => e + puts "[#{index + 1}/#{hosts.count}] Failed to push \"#{host.name}\": #{e}" end end end @@ -33,16 +31,14 @@ namespace :foreman_vault do # rubocop:disable Metrics/BlockLength hosts = Host::Managed.where(managed: true) hosts.each_with_index do |host, index| - begin - result = host.reload.vault_policy.save - if result - puts "[#{index + 1}/#{hosts.count}] Policy of \"#{host.name}\" pushed to Vault server \"#{host.vault_connection.url}\"" - else - puts "[#{index + 1}/#{hosts.count}] Failed to push \"#{host.name}\": #{result}" - end - rescue StandardError => err - puts "[#{index + 1}/#{hosts.count}] Failed to push \"#{host.name}\": #{err}" + result = host.reload.vault_policy.save + if result + puts "[#{index + 1}/#{hosts.count}] Policy of \"#{host.name}\" pushed to Vault server \"#{host.vault_connection.url}\"" + else + puts "[#{index + 1}/#{hosts.count}] Failed to push \"#{host.name}\": #{result}" end + rescue StandardError => e + puts "[#{index + 1}/#{hosts.count}] Failed to push \"#{host.name}\": #{e}" end end end @@ -61,25 +57,4 @@ namespace :test do end end -namespace :foreman_vault do - task :rubocop do - begin - require 'rubocop/rake_task' - RuboCop::RakeTask.new(:rubocop_foreman_vault) do |task| - task.patterns = ["#{ForemanVault::Engine.root}/app/**/*.rb", - "#{ForemanVault::Engine.root}/lib/**/*.rb", - "#{ForemanVault::Engine.root}/test/**/*.rb"] - end - rescue StandardError - puts 'Rubocop not loaded.' - end - - Rake::Task['rubocop_foreman_vault'].invoke - end -end - Rake::Task[:test].enhance ['test:foreman_vault'] - -load 'tasks/jenkins.rake' - -Rake::Task['jenkins:unit'].enhance ['test:foreman_vault', 'foreman_vault:rubocop'] if Rake::Task.task_defined?(:'jenkins:unit') diff --git a/test/unit/foreman_vault/access_permissions_test.rb b/test/unit/foreman_vault/access_permissions_test.rb new file mode 100644 index 0000000..3dc31f3 --- /dev/null +++ b/test/unit/foreman_vault/access_permissions_test.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +require 'test_plugin_helper' +require 'unit/shared/access_permissions_test_base' + +# Permissions are added in AccessPermissions with lists of controllers and +# actions that they enable access to. For non-admin users, we need to test +# that there are permissions available that cover every controller action, else +# it can't be delegated and this will lead to parts of the application that +# aren't functional for non-admin users. +# +# In particular, it's important that actions for AJAX requests are added to +# an appropriate permission so views using those requests function. +class AccessPermissionsTest < ActiveSupport::TestCase + include AccessPermissionsTestBase + + check_routes(ForemanVault::Engine.routes, []) +end diff --git a/test/unit/lib/foreman_vault/macros_test.rb b/test/unit/lib/foreman_vault/macros_test.rb index d661bae..ddd8e65 100644 --- a/test/unit/lib/foreman_vault/macros_test.rb +++ b/test/unit/lib/foreman_vault/macros_test.rb @@ -22,7 +22,7 @@ class TestScope < Foreman::Renderer::Scope::Base subject = TestScope.new(host: host, source: source) - assert subject.respond_to?(:vault_secret) + assert_respond_to subject, :vault_secret assert_equal response.data, subject.vault_secret(vault_connection.name, secret_path) end end diff --git a/test/unit/services/foreman_vault/vault_auth_method_test.rb b/test/unit/services/foreman_vault/vault_auth_method_test.rb index 9436a4d..8e979f1 100644 --- a/test/unit/services/foreman_vault/vault_auth_method_test.rb +++ b/test/unit/services/foreman_vault/vault_auth_method_test.rb @@ -59,9 +59,11 @@ class VaultAuthMethodTest < ActiveSupport::TestCase subject.expects(:set_certificate).once.with( 'name', - certificate: 'cert', - token_policies: 'vault_policy_name', - allowed_common_names: [host.fqdn] + { + certificate: 'cert', + token_policies: 'vault_policy_name', + allowed_common_names: [host.fqdn], + } ) subject.save end diff --git a/test/unit/services/foreman_vault/vault_client_test.rb b/test/unit/services/foreman_vault/vault_client_test.rb index a943b16..e342d56 100644 --- a/test/unit/services/foreman_vault/vault_client_test.rb +++ b/test/unit/services/foreman_vault/vault_client_test.rb @@ -23,15 +23,15 @@ class VaultClientTest < ActiveSupport::TestCase stub_request(:post, "#{base_url}/v1/auth/approle/login").with( body: { role_id: role_id, - secret_id: secret_id + secret_id: secret_id, } ).to_return( status: 200, headers: { 'Content-Type': 'application/json' }, body: { auth: { - client_token: token - } + client_token: token, + }, }.to_json ) end @@ -82,7 +82,7 @@ class VaultClientTest < ActiveSupport::TestCase issuing_ca: 'CA_CERTIFICATE_DATA', private_key: 'PRIVATE_KEY_DATA', private_key_type: 'rsa', - serial_number: '7e:2d:c8:dd:df:da:fe:1f:39:da:39:23:4f:74:c8:1f:1d:4a:db:a7' + serial_number: '7e:2d:c8:dd:df:da:fe:1f:39:da:39:23:4f:74:c8:1f:1d:4a:db:a7', } response = OpenStruct.new(data: @data)