implement plugin system review suggestsions

fdroidserver/__main__.py

@@ -32,7 +32,7 @@ from argparse import ArgumentError
 from collections import OrderedDict
 
 
-commands = OrderedDict([
+COMMANDS = OrderedDict([
     ("build", _("Build a package from source")),
     ("init", _("Quickly start a new repository")),
     ("publish", _("Sign and place packages in the repo")),
@@ -57,16 +57,16 @@ commands = OrderedDict([
 ])
 
 
-def print_help(fdroid_modules=None):
+def print_help(available_plugins=None):
     print(_("usage: ") + _("fdroid [<command>] [-h|--help|--version|<args>]"))
     print("")
     print(_("Valid commands are:"))
-    for cmd, summary in commands.items():
+    for cmd, summary in COMMANDS.items():
         print("   " + cmd + ' ' * (15 - len(cmd)) + summary)
-    if fdroid_modules:
+    if available_plugins:
         print(_('commands from plugin modules:'))
-        for command in sorted(fdroid_modules.keys()):
+        for command in sorted(available_plugins.keys()):
-            print('   {:15}{}'.format(command, fdroid_modules[command]['summary']))
+            print('   {:15}{}'.format(command, available_plugins[command]['summary']))
     print("")
 
 
@@ -109,12 +109,12 @@ def preparse_plugin(module_name, module_dir):
 
 def find_plugins():
     found_plugins = [{'name': x[1], 'dir': x[0].path} for x in pkgutil.iter_modules() if x[1].startswith('fdroid_')]
-    commands = {}
+    plugin_infos = {}
     for plugin_def in found_plugins:
         command_name = plugin_def['name'][7:]
         try:
-            commands[command_name] = preparse_plugin(plugin_def['name'],
+            plugin_infos[command_name] = preparse_plugin(plugin_def['name'],
-                                                     plugin_def['dir'])
+                                                         plugin_def['dir'])
         except Exception as e:
             # We need to keep module lookup fault tolerant because buggy
             # modules must not prevent fdroidserver from functioning
@@ -122,20 +122,20 @@ def find_plugins():
                 # only raise exeption when a user specifies the broken
                 # plugin in explicitly in command line
                 raise e
-    return commands
+    return plugin_infos
 
 
 def main():
-    fdroid_modules = find_plugins()
+    available_plugins = find_plugins()
 
     if len(sys.argv) <= 1:
-        print_help(fdroid_modules=fdroid_modules)
+        print_help(available_plugins=available_plugins)
         sys.exit(0)
 
     command = sys.argv[1]
-    if command not in commands and command not in fdroid_modules.keys():
+    if command not in COMMANDS and command not in available_plugins.keys():
         if command in ('-h', '--help'):
-            print_help(fdroid_modules=fdroid_modules)
+            print_help(available_plugins=available_plugins)
             sys.exit(0)
         elif command == '--version':
             output = _('no version info found!')
@@ -162,11 +162,11 @@ def main():
             else:
                 from pkg_resources import get_distribution
                 output = get_distribution('fdroidserver').version + '\n'
-            print(output),
+            print(output)
             sys.exit(0)
         else:
             print(_("Command '%s' not recognised.\n" % command))
-            print_help(fdroid_modules=fdroid_modules)
+            print_help(available_plugins=available_plugins)
             sys.exit(1)
 
     verbose = any(s in sys.argv for s in ['-v', '--verbose'])
@@ -196,10 +196,10 @@ def main():
     sys.argv[0] += ' ' + command
 
     del sys.argv[1]
-    if command in commands.keys():
+    if command in COMMANDS.keys():
         mod = __import__('fdroidserver.' + command, None, None, [command])
     else:
-        mod = __import__(fdroid_modules[command]['name'], None, None, [command])
+        mod = __import__(available_plugins[command]['name'], None, None, [command])
 
     system_langcode, system_encoding = locale.getdefaultlocale()
     if system_encoding is None or system_encoding.lower() not in ('utf-8', 'utf8'):

tests/main.TestCase

@@ -24,9 +24,9 @@ import fdroidserver.__main__
 class MainTest(unittest.TestCase):
     '''this tests fdroid.py'''
 
-    def test_commands(self):
+    def test_COMMANDS_check(self):
         """make sure the built in sub-command defs didn't change unintentionally"""
-        self.assertListEqual([x for x in fdroidserver.__main__.commands.keys()],
+        self.assertListEqual([x for x in fdroidserver.__main__.COMMANDS.keys()],
                              ['build',
                               'init',
                               'publish',
@@ -55,6 +55,8 @@ class MainTest(unittest.TestCase):
             with mock.patch('fdroidserver.init.main', co):
                 with mock.patch('sys.exit') as exit_mock:
                     fdroidserver.__main__.main()
+                    # note: this is sloppy, if `init` changes
+                    # this might need changing too
                     exit_mock.assert_called_once_with(0)
         co.assert_called_once_with()
 
@@ -64,51 +66,102 @@ class MainTest(unittest.TestCase):
             with mock.patch('fdroidserver.server.main', co):
                 with mock.patch('sys.exit') as exit_mock:
                     fdroidserver.__main__.main()
+                    # note: this is sloppy, if `deploy` changes
+                    # this might need changing too
                     exit_mock.assert_called_once_with(0)
         co.assert_called_once_with()
 
     def test_find_plugins(self):
         with tempfile.TemporaryDirectory() as tmpdir, TmpCwd(tmpdir):
-            with open('fdroid_testy.py', 'w') as f:
+            with open('fdroid_testy1.py', 'w') as f:
                 f.write(textwrap.dedent("""\
                         fdroid_summary = "ttt"
                         main = lambda: 'all good'"""))
             with TmpPyPath(tmpdir):
                 plugins = fdroidserver.__main__.find_plugins()
-                self.assertIn('testy', plugins.keys())
+                self.assertIn('testy1', plugins.keys())
-                self.assertEqual(plugins['testy']['summary'], 'ttt')
+                self.assertEqual(plugins['testy1']['summary'], 'ttt')
-                self.assertEqual(__import__(plugins['testy']['name'],
+                self.assertEqual(__import__(plugins['testy1']['name'],
                                             None,
                                             None,
-                                            ['testy'])
+                                            ['testy1'])
                                  .main(),
                                  'all good')
 
     def test_main_plugin_lambda(self):
         with tempfile.TemporaryDirectory() as tmpdir, TmpCwd(tmpdir):
-            with open('fdroid_testy.py', 'w') as f:
+            with open('fdroid_testy2.py', 'w') as f:
                 f.write(textwrap.dedent("""\
                         fdroid_summary = "ttt"
-                        main = lambda: pritn('all good')"""))
+                        main = lambda: print('all good')"""))
             with TmpPyPath(tmpdir):
-                with mock.patch('sys.argv', ['', 'testy']):
+                with mock.patch('sys.argv', ['', 'testy2']):
                     with mock.patch('sys.exit') as exit_mock:
                         fdroidserver.__main__.main()
                         exit_mock.assert_called_once_with(0)
 
     def test_main_plugin_def(self):
         with tempfile.TemporaryDirectory() as tmpdir, TmpCwd(tmpdir):
-            with open('fdroid_testy.py', 'w') as f:
+            with open('fdroid_testy3.py', 'w') as f:
                 f.write(textwrap.dedent("""\
                         fdroid_summary = "ttt"
                         def main():
-                            pritn('all good')"""))
+                            print('all good')"""))
             with TmpPyPath(tmpdir):
-                with mock.patch('sys.argv', ['', 'testy']):
+                with mock.patch('sys.argv', ['', 'testy3']):
                     with mock.patch('sys.exit') as exit_mock:
                         fdroidserver.__main__.main()
                         exit_mock.assert_called_once_with(0)
 
+    def test_main_broken_plugin(self):
+        """making sure broken plugins get their exceptions through"""
+        with tempfile.TemporaryDirectory() as tmpdir, TmpCwd(tmpdir):
+            with open('fdroid_testy4.py', 'w') as f:
+                f.write(textwrap.dedent("""\
+                        fdroid_summary = "ttt"
+                        def main():
+                            raise Exception("this plugin is broken")"""))
+            with TmpPyPath(tmpdir):
+                with mock.patch('sys.argv', ['', 'testy4']):
+                    with self.assertRaisesRegex(Exception, "this plugin is broken"):
+                        fdroidserver.__main__.main()
+
+    def test_main_malicious_plugin(self):
+        """The purpose of this test is to make sure code in plugins
+        doesn't get executed unintentionally.
+        """
+        with tempfile.TemporaryDirectory() as tmpdir, TmpCwd(tmpdir):
+            with open('fdroid_testy5.py', 'w') as f:
+                f.write(textwrap.dedent("""\
+                        fdroid_summary = "ttt"
+                        raise Exception("this plugin is malicious")
+                        def main():
+                            print("evil things")"""))
+            with TmpPyPath(tmpdir):
+                with mock.patch('sys.argv', ['', 'lint']):
+                    with mock.patch('sys.exit') as exit_mock:
+                        fdroidserver.__main__.main()
+                        # note: this is sloppy, if `lint` changes
+                        # this might need changing too
+                        exit_mock.assert_called_once_with(0)
+
+    def test_main_prevent_plugin_override(self):
+        """making sure build-in subcommands cannot be overridden by plugins
+        """
+        with tempfile.TemporaryDirectory() as tmpdir, TmpCwd(tmpdir):
+            with open('fdroid_signatures.py', 'w') as f:
+                f.write(textwrap.dedent("""\
+                        fdroid_summary = "ttt"
+                        def main():
+                            raise("plugin overrides don't get prevent!")"""))
+            with TmpPyPath(tmpdir):
+                with mock.patch('sys.argv', ['', 'signatures']):
+                    with mock.patch('sys.exit') as exit_mock:
+                        fdroidserver.__main__.main()
+                        # note: this is sloppy, if `signatures` changes
+                        # this might need changing too
+                        self.assertEqual(exit_mock.call_count, 2)
+
     def test_preparse_plugin_lookup_bad_name(self):
         self.assertRaises(ValueError,
                           fdroidserver.__main__.preparse_plugin,
@@ -121,7 +174,7 @@ class MainTest(unittest.TestCase):
 
     def test_preparse_plugin_lookup_summary_missing(self):
         with tempfile.TemporaryDirectory() as tmpdir, TmpCwd(tmpdir):
-            with open('fdroid_testy.py', 'w') as f:
+            with open('fdroid_testy6.py', 'w') as f:
                 f.write(textwrap.dedent("""\
                         main = lambda: print('all good')"""))
             with TmpPyPath(tmpdir):
@@ -134,7 +187,7 @@ class MainTest(unittest.TestCase):
 
     def test_preparse_plugin_lookup_module_file(self):
         with tempfile.TemporaryDirectory() as tmpdir, TmpCwd(tmpdir):
-            with open('fdroid_testy.py', 'w') as f:
+            with open('fdroid_testy7.py', 'w') as f:
                 f.write(textwrap.dedent("""\
                         fdroid_summary = "ttt"
                         main = lambda: pritn('all good')"""))
@@ -143,24 +196,24 @@ class MainTest(unittest.TestCase):
                 module_path = p[0][0].path
                 module_name = p[0][1]
                 d = fdroidserver.__main__.preparse_plugin(module_name, module_path)
-            self.assertDictEqual(d, {'name': 'fdroid_testy',
+            self.assertDictEqual(d, {'name': 'fdroid_testy7',
                                      'summary': 'ttt'})
 
     def test_preparse_plugin_lookup_module_dir(self):
         with tempfile.TemporaryDirectory() as tmpdir, TmpCwd(tmpdir):
-            os.mkdir(os.path.join(tmpdir, 'fdroid_testy'))
+            os.mkdir(os.path.join(tmpdir, 'fdroid_testy8'))
-            with open('fdroid_testy/__main__.py', 'w') as f:
+            with open('fdroid_testy8/__main__.py', 'w') as f:
                 f.write(textwrap.dedent("""\
                         fdroid_summary = "ttt"
                         main = lambda: print('all good')"""))
-            with open('fdroid_testy/__init__.py', 'w') as f:
+            with open('fdroid_testy8/__init__.py', 'w') as f:
                 pass
             with TmpPyPath(tmpdir):
                 p = [x for x in pkgutil.iter_modules() if x[1].startswith('fdroid_')]
                 module_path = p[0][0].path
                 module_name = p[0][1]
                 d = fdroidserver.__main__.preparse_plugin(module_name, module_path)
-            self.assertDictEqual(d, {'name': 'fdroid_testy',
+            self.assertDictEqual(d, {'name': 'fdroid_testy8',
                                      'summary': 'ttt'})