Plugins: Filter Plugins During Search and Install (#3458)

* Unit and functional tests for a reject list facility
* Implementation of plugin reject facility
* Initial draft of reject list
* Add option to search to hide test fixture by default
* Fix test
* PR feedback

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>

etc/plugin_filters.json

@@ -0,0 +1,25 @@
+{
+  "file_version": "1.0.0",
+  "exclude": [
+    {
+      "plugin_name": "inspec-core",
+      "rationale": "This gem is a stripped-down alternate packaging of InSpec. It is not a plugin."
+    },
+    {
+      "plugin_name": "inspec-multi-server",
+      "rationale": "This gem is a script that attempts to drive a parallel execution of InSpec by wrapping and forking. It is not a plugin."
+    },
+    {
+      "plugin_name": "train-tax-calculator",
+      "rationale": "This gem is a tax calculation tool for the Philippines. It has nothing to do the Chef Train remote execution framework, or the InSpec project."
+    },
+    {
+      "plugin_name": "inspec-plugin-example",
+      "rationale": "This gem is an early self-taught example of a v1 plugin. Please use inspec-resource-lister as an example for PluginV2 development."
+    },
+    {
+      "plugin_name": "train-core",
+      "rationale": "This gem is a stripped-down alternate packaging of Train. It is not a plugin."
+    }
+  ]
+}

lib/inspec/plugin/v2.rb

@@ -11,6 +11,9 @@ module Inspec
         attr_accessor :version
       end
       class InstallError < Inspec::Plugin::V2::GemActionError; end
+      class PluginExcludedError < Inspec::Plugin::V2::InstallError
+        attr_accessor :details
+      end
       class UpdateError < Inspec::Plugin::V2::GemActionError
         attr_accessor :from_version, :to_version
       end

lib/inspec/plugin/v2/filter.rb

@@ -0,0 +1,62 @@
+require 'singleton'
+require 'json'
+require 'inspec/globals'
+
+module Inspec::Plugin::V2
+  Exclusion = Struct.new(:plugin_name, :rationale)
+
+  class PluginFilter
+    include Singleton
+    def initialize
+      read_filter_data
+    end
+
+    def self.exclude?(plugin_name)
+      instance.exclude?(plugin_name)
+    end
+
+    def exclude?(plugin_name)
+      # Currently, logic is very simple: is there an exact match?
+      # In the future, we might add regexes on names, or exclude version ranges
+      return false unless @filter_data[:exclude].detect { |e| e.plugin_name == plugin_name }
+
+      # OK, return entire data structure.
+      @filter_data[:exclude].detect { |e| e.plugin_name == plugin_name }
+    end
+
+    private
+
+    def read_filter_data
+      path = File.join(Inspec.src_root, 'etc', 'plugin_filters.json')
+      @filter_data = JSON.parse(File.read(path))
+
+      unless @filter_data['file_version'] == '1.0.0'
+        raise Inspec::Plugin::V2::ConfigError, "Unknown plugin fillter file format at #{path}"
+      end
+
+      validate_plugin_filter_file('1.0.0')
+
+      @filter_data[:exclude] = @filter_data['exclude'].map do |entry|
+        Exclusion.new(entry['plugin_name'], entry['rationale'])
+      end
+      @filter_data.delete('exclude')
+    end
+
+    def validate_plugin_filter_file(_file_version)
+      unless @filter_data.key?('exclude') && @filter_data['exclude'].is_a?(Array)
+        raise Inspec::Plugin::V2::ConfigError, 'Unknown plugin fillter file format: expected "exclude" to be an array'
+      end
+      @filter_data['exclude'].each_with_index do |entry, idx|
+        unless entry.is_a? Hash
+          raise Inspec::Plugin::V2::ConfigError, "Unknown plugin fillter file format: expected entry #{idx} to be a Hash / JS Object"
+        end
+        unless entry.key?('plugin_name')
+          raise Inspec::Plugin::V2::ConfigError, "Unknown plugin fillter file format: expected entry #{idx} to have a \"plugin_name\" field"
+        end
+        unless entry.key?('rationale')
+          raise Inspec::Plugin::V2::ConfigError, "Unknown plugin fillter file format: expected entry #{idx} to have a \"rationale\" field"
+        end
+      end
+    end
+  end
+end

lib/inspec/plugin/v2/installer.rb

@@ -9,6 +9,8 @@ require 'rubygems/package'
 require 'rubygems/name_tuple'
 require 'rubygems/uninstaller'
 
+require 'inspec/plugin/v2/filter'
+
 module Inspec::Plugin::V2
   # Handles all actions modifying the user's plugin set:
   # * Modifying the plugins.json file
@@ -127,7 +129,7 @@ module Inspec::Plugin::V2
       else
         regex = Regexp.new('^' + plugin_query + '.*')
         matched_tuples = fetcher.detect(opts[:scope]) do |tuple|
-          tuple.name != 'inspec-core' && tuple.name =~ regex
+          tuple.name =~ regex && !Inspec::Plugin::V2::PluginFilter.exclude?(tuple.name)
         end
       end
 
@@ -193,6 +195,13 @@ module Inspec::Plugin::V2
           raise InstallError, "#{plugin_name} is already installed. Use 'inspec plugin update' to change version."
         end
       end
+
+      reason = Inspec::Plugin::V2::PluginFilter.exclude?(plugin_name)
+      if reason
+        ex = PluginExcludedError.new("Refusing to install #{plugin_name}.  It is on the Plugin Exclusion List.  Rationale: #{reason.rationale}")
+        ex.details = reason
+        raise ex
+      end
     end
     # rubocop: enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/AbcSize
 

lib/plugins/inspec-plugin-manager-cli/lib/inspec-plugin-manager-cli/cli_command.rb

@@ -1,5 +1,6 @@
 require 'term/ansicolor'
 require 'pathname'
+require 'inspec/plugin/v2'
 require 'inspec/plugin/v2/installer'
 
 module InspecPlugins
@@ -35,16 +36,30 @@ module InspecPlugins
       #                        inspec plugin search
       #==================================================================#
 
-      desc 'search [options] PATTERN', 'Searches rubygems.org for InSpec plugins. Exits 0 on a search hit, exits 2 on a search miss.'
+      desc 'search [options] PATTERN', 'Searches rubygems.org for plugins.'
+      long_desc <<~EOLD
+        Searches rubygems.org for InSpec plugins. Exits 0 on a search hit, 1 on user error,
+        2 on a search miss. PATTERN is a simple string; a wildcard will be added as
+        a suffix, unless -e is used.
+      EOLD
       option :all, desc: 'List all available versions, not just the latest one.', type: :boolean, aliases: [:a]
       option :exact, desc: 'Assume PATTERN is exact; do not add a wildcard to the end', type: :boolean, aliases: [:e]
+      option :'include-test-fixture', type: :boolean, desc: 'Internal use', hide: true
       # Justification for disabling ABC: currently at 33.51/33
       def search(search_term) # rubocop: disable Metrics/AbcSize
         search_results = installer.search(search_term, exact: options[:exact])
+        # The search results have already been filtered by the reject list.  But the
+        # RejectList doesn't filter {inspec, train}-test-fixture because we need those
+        # for testing.  We want to hide those from users, so unless we know we're in
+        # test mode, remove them.
+        unless options[:'include-test-fixture']
+          search_results.delete('inspec-test-fixture')
+          search_results.delete('train-test-fixture')
+        end
 
         # TODO: ui object support
         puts
-        puts(bold { format(' %-30s%-50s%', 'Plugin Name', 'Versions Available') })
+        puts(bold { format(' %-30s%-50s', 'Plugin Name', 'Versions Available') })
         puts '-' * 55
         search_results.keys.sort.each do |plugin_name|
           versions = options[:all] ? search_results[plugin_name] : [search_results[plugin_name].first]
@@ -342,8 +357,15 @@ module InspecPlugins
         exit 2
       end
 
-      def install_attempt_install(plugin_name)
+      # Rationale for RuboCop variance: This is a one-line method with heavy UX-focused error handling.
+      def install_attempt_install(plugin_name) # rubocop: disable Metrics/AbcSize
         installer.install(plugin_name, version: options[:version])
+      rescue Inspec::Plugin::V2::PluginExcludedError => ex
+        puts(red { 'Plugin on Exclusion List' } + " - #{plugin_name} is listed as an incompatible gem - refusing to install.")
+        puts "Rationale: #{ex.details.rationale}"
+        puts 'Exclusion list location: ' + File.join(Inspec.src_root, 'etc', 'plugin_filters.json')
+        puts 'If you disagree with this determination, please accept our apologies for the misunderstanding, and open an issue at https://github.com/inspec/inspec/issues/new'
+        exit 2
       rescue Inspec::Plugin::V2::InstallError
         results = installer.search(plugin_name, exact: true)
         if results.empty?

lib/plugins/inspec-plugin-manager-cli/test/functional/inspec-plugin_test.rb

@@ -143,8 +143,14 @@ class PluginManagerCliSearch < MiniTest::Test
   include CorePluginFunctionalHelper
   include PluginManagerHelpers
 
+  # TODO: Thor can't hide options, but we wish it could.
+  # def test_search_include_fixture_hidden_option
+  #   result = run_inspec_process_with_this_plugin('plugin help search')
+  #   refute_includes result.stdout, '--include-test-fixture'
+  # end
+
   def test_search_for_a_real_gem_with_full_name_no_options
-    result = run_inspec_process('plugin search inspec-test-fixture')
+    result = run_inspec_process('plugin search --include-test-fixture inspec-test-fixture')
     assert_equal 0, result.exit_status, 'Search should exit 0 on a hit'
     assert_includes result.stdout, 'inspec-test-fixture', 'Search result should contain the gem name'
     assert_includes result.stdout, '1 plugin(s) found', 'Search result should find 1 plugin'
@@ -153,7 +159,7 @@ class PluginManagerCliSearch < MiniTest::Test
   end
 
   def test_search_for_a_real_gem_with_stub_name_no_options
-    result = run_inspec_process('plugin search inspec-test-')
+    result = run_inspec_process('plugin search --include-test-fixture inspec-test-')
     assert_equal 0, result.exit_status, 'Search should exit 0 on a hit'
     assert_includes result.stdout, 'inspec-test-fixture', 'Search result should contain the gem name'
     assert_includes result.stdout, '1 plugin(s) found', 'Search result should find 1 plugin'
@@ -163,26 +169,26 @@ class PluginManagerCliSearch < MiniTest::Test
   end
 
   def test_search_for_a_real_gem_with_full_name_and_exact_option
-    result = run_inspec_process('plugin search --exact inspec-test-fixture')
+    result = run_inspec_process('plugin search --exact --include-test-fixture inspec-test-fixture')
     assert_equal 0, result.exit_status, 'Search should exit 0 on a hit'
     assert_includes result.stdout, 'inspec-test-fixture', 'Search result should contain the gem name'
     assert_includes result.stdout, '1 plugin(s) found', 'Search result should find 1 plugin'
 
-    result = run_inspec_process('plugin search -e inspec-test-fixture')
+    result = run_inspec_process('plugin search -e --include-test-fixture inspec-test-fixture')
     assert_equal 0, result.exit_status, 'Search should exit 0 on a hit'
   end
 
   def test_search_for_a_real_gem_with_stub_name_and_exact_option
-    result = run_inspec_process('plugin search --exact inspec-test-')
+    result = run_inspec_process('plugin search --exact --include-test-fixture inspec-test-')
     assert_equal 2, result.exit_status, 'Search should exit 2 on a miss'
     assert_includes result.stdout, '0 plugin(s) found', 'Search result should find 0 plugins'
 
-    result = run_inspec_process('plugin search -e inspec-test-')
+    result = run_inspec_process('plugin search -e --include-test-fixture inspec-test-')
     assert_equal 2, result.exit_status, 'Search should exit 2 on a miss'
   end
 
   def test_search_for_a_real_gem_with_full_name_and_all_option
-    result = run_inspec_process('plugin search --all inspec-test-fixture')
+    result = run_inspec_process('plugin search --all --include-test-fixture inspec-test-fixture')
     assert_equal 0, result.exit_status, 'Search should exit 0 on a hit'
     assert_includes result.stdout, 'inspec-test-fixture', 'Search result should contain the gem name'
     assert_includes result.stdout, '1 plugin(s) found', 'Search result should find 1 plugin'
@@ -190,24 +196,24 @@ class PluginManagerCliSearch < MiniTest::Test
     line = result.stdout.split("\n").grep(/inspec-test-fixture/).first
     assert_match(/\s*inspec-test-fixture\s+\((\d+\.\d+\.\d+(,\s)?){2,}\)/,line,'Plugin line should include name and at least two versions')
 
-    result = run_inspec_process('plugin search -a inspec-test-fixture')
+    result = run_inspec_process('plugin search -a --include-test-fixture inspec-test-fixture')
     assert_equal 0, result.exit_status, 'Search should exit 0 on a hit'
   end
 
   def test_search_for_a_gem_with_missing_prefix
-    result = run_inspec_process('plugin search test-fixture')
+    result = run_inspec_process('plugin search --include-test-fixture test-fixture')
     assert_equal 1, result.exit_status, 'Search should exit 1 on user error'
     assert_includes result.stdout, "All inspec plugins must begin with either 'inspec-' or 'train-'"
   end
 
   def test_search_for_a_gem_that_does_not_exist
-    result = run_inspec_process('plugin search inspec-test-fixture-nonesuch')
+    result = run_inspec_process('plugin search --include-test-fixture inspec-test-fixture-nonesuch')
     assert_equal 2, result.exit_status, 'Search should exit 2 on a miss'
     assert_includes result.stdout, '0 plugin(s) found', 'Search result should find 0 plugins'
   end
 
   def test_search_for_a_real_gem_with_full_name_no_options_and_train_name
-    result = run_inspec_process('plugin search train-test-fixture')
+    result = run_inspec_process('plugin search --include-test-fixture train-test-fixture')
     assert_equal 0, result.exit_status, 'Search should exit 0 on a hit'
     assert_includes result.stdout, 'train-test-fixture', 'Search result should contain the gem name'
     assert_includes result.stdout, '1 plugin(s) found', 'Search result should find 1 plugin'
@@ -215,6 +221,28 @@ class PluginManagerCliSearch < MiniTest::Test
     assert_match(/\s*train-test-fixture\s+\((\d+\.\d+\.\d+){1}\)/,line,'Plugin line should include name and exactly one version')
   end
 
+  def test_search_omit_excluded_inspec_plugins
+    result = run_inspec_process('plugin search --include-test-fixture inspec-')
+    assert_equal 0, result.exit_status, 'Search should exit 0'
+    assert_includes result.stdout, 'inspec-test-fixture', 'Search result should contain the test gem'
+    [
+      'inspec-core',
+      'inspec-multi-server',
+    ].each do |plugin_name|
+      refute_includes result.stdout, plugin_name, 'Search result should not contain excluded gems'
+    end
+  end
+  def test_search_for_a_real_gem_with_full_name_no_options_filter_fixtures
+    result = run_inspec_process('plugin search inspec-test-fixture')
+    refute_includes result.stdout, 'inspec-test-fixture', 'Search result should not contain the fixture gem name'
+  end
+
+  def test_search_for_a_real_gem_with_full_name_no_options_filter_fixtures_train
+    result = run_inspec_process('plugin search train-test-fixture')
+    refute_includes result.stdout, 'train-test-fixture', 'Search result should not contain the fixture gem name'
+  end
+
+
 end
 
 #-----------------------------------------------------------------------------------------#
@@ -513,6 +541,32 @@ class PluginManagerCliInstall < MiniTest::Test
     refute_nil itf_line, 'train-test-fixture should now appear in the output of inspec list'
     assert_match(/\s*train-test-fixture\s+0.1.0\s+gem\s+/, itf_line, 'list output should show that it is a gem installation with version')
   end
+
+  def test_refuse_install_when_plugin_on_exclusion_list
+
+    # Here, 'inspec-core', 'inspec-multi-server', and 'train-tax-collector'
+    # are the names of real rubygems.  They are not InSpec/Train plugins, though,
+    # and installing them would be a jam-up.
+    # This is configured in 'etc/plugin-filter.json'.
+    [
+      'inspec-core',
+      'inspec-multi-server',
+      'train-tax-calculator',
+    ].each do |plugin_name|
+      install_result = run_inspec_process_with_this_plugin("plugin install #{plugin_name}")
+      assert_empty install_result.stderr
+      assert_equal 2, install_result.exit_status, 'Exit status should be 2'
+
+      refusal_message = install_result.stdout
+      refute_nil refusal_message, 'Should find a failure message at the end'
+      assert_includes refusal_message, plugin_name
+      assert_includes refusal_message, 'Plugin on Exclusion List'
+      assert_includes refusal_message, 'refusing to install'
+      assert_includes refusal_message, 'Rationale:'
+      assert_includes refusal_message, 'etc/plugin_filters.json'
+      assert_includes refusal_message, 'github.com/inspec/inspec/issues/new'
+    end
+  end
 end
 
 

lib/plugins/inspec-plugin-manager-cli/test/unit/cli_args_test.rb

@@ -26,7 +26,7 @@ class PluginManagerCliOptions < MiniTest::Test
 
   def test_search_args
     arg_config = cli_class.all_commands['search'].options
-    assert_equal 2, arg_config.count, 'The search command should have 2 options'
+    assert_equal 3, arg_config.count, 'The search command should have 3 options'
 
     assert_includes arg_config.keys, :all, 'The search command should have an --all option'
     assert_equal :boolean, arg_config[:all].type, 'The --all option should be boolean'
@@ -40,6 +40,10 @@ class PluginManagerCliOptions < MiniTest::Test
     refute_nil arg_config[:exact].description, 'The --exact option should have a description'
     refute arg_config[:exact].required, 'The --exact option should not be required'
 
+    assert_includes arg_config.keys, :'include-test-fixture', 'The search command should have an --include-test-fixture option'
+    assert_equal :boolean, arg_config[:'include-test-fixture'].type, 'The --include-test-fixture option should be boolean'
+    refute arg_config[:'include-test-fixture'].required, 'The --include-test-fixture option should not be required'
+
     assert_equal 1, cli_class.instance_method(:search).arity, 'The search command should take one argument'
   end
 

test/unit/plugin/v2/installer_test.rb

@@ -239,6 +239,24 @@ class PluginInstallerInstallationTests < MiniTest::Test
     assert_includes entry.keys, 'installation_path', 'plugins.json should include installation_path key'
     assert_equal @plugin_fixture_src_path, entry['installation_path'], 'plugins.json should include correct value for installation path'
   end
+
+  def test_refuse_to_install_gem_whose_name_is_on_the_reject_list
+    ENV['INSPEC_CONFIG_DIR'] = File.join(@config_dir_path, 'empty')
+
+    # Here, 'inspec-core', 'inspec-multi-server', and 'train-tax-collector'
+    # are the names of real rubygems.  They are not InSPec/Train plugins, though,
+    # and installing them would be a jam-up.
+    # This is configured in 'etc/plugin-filter.json'.
+    [
+      'inspec-core',
+      'inspec-multi-server',
+      'train-tax-calculator',
+    ].each do |plugin_name|
+      ex = assert_raises(Inspec::Plugin::V2::InstallError) { @installer.install(plugin_name)}
+      assert_includes(ex.message, 'on the Plugin Exclusion List')
+      assert_includes(ex.message, 'Rationale:')
+    end
+  end
 end
 
 #-----------------------------------------------------------------------#
@@ -455,5 +473,36 @@ class PluginInstallerSearchTests < MiniTest::Test
     assert_includes version_list, '0.1.0', 'Version list should contain 0.1.0'
     assert_includes version_list, '0.2.0', 'Version list should contain 0.2.0'
   end
+
+  def test_search_omits_inspec_gem_on_the_reject_list
+    results = @installer.search('inspec-')
+    assert results.key?('inspec-test-fixture')
+
+    # Here, 'inspec-core', 'inspec-multi-server'
+    # are the names of real rubygems.  They are not InSpec/Train plugins, though,
+    # and installing them would be a jam-up.
+    # This is configured in 'etc/plugin_filters.json'.
+    [
+      'inspec-core',
+      'inspec-multi-server',
+    ].each do |plugin_name|
+      refute results.key(plugin_name)
+    end
+  end
+
+  def test_search_omits_train_gem_on_the_reject_list
+    results = @installer.search('train-')
+    assert results.key?('train-test-fixture')
+
+    # Here, train-tax-calculator'
+    # is the name of a real rubygem.  It is not a InSpec/Train plugin, though,
+    # and installing it would be a jam-up.
+    # This is configured in 'etc/plugin_filters.json'.
+    [
+      'train-tax-calculator'
+    ].each do |plugin_name|
+      refute results.key(plugin_name)
+    end
+  end
 end