Merge lp:~zyga/checkbox/fix-1297746 into lp:checkbox

[Zygmunt Krynicki]

5a7415f plainbox:job: don't require the id field
3a845c8 plainbox:job: validate the presence of job.partial_id
eddc7e0 plainbox:job: drop validator_cls argument
17f4314 plainbox:job: add Problem.deprecated
ad51337 plainbox:job: add strictness control to job validator
3f05db4 plainbox:secure:providers: drop useless import
c1974b4 plainbox:secure:providers: expose control for job validation
b2e6e20 plainbox:provider_manager: enforce strong validation in `manage.py validate`
dd9036e plainbox:provider_manager: handle deprecated fields

[lp:~zyga/checkbox/fix-1297746]

2848. By Zygmunt Krynicki on 2014-03-28

plainbox:job: add strictness control to job validator

This patch adds two optional arguments to the validator, strict, which
effectively runs the validator at maximum strictness and deprecated
which makes it find deprecated fields or values.

The default settings will ignore harmless errors (thus making the
validator do no evil) while still reporting (and preventing) serious
problems that would definitely fail at job runtime.

Signed-off-by: Zygmunt Krynicki <email address hidden>

2849. By Zygmunt Krynicki on 2014-03-28

plainbox:secure:providers: drop useless import

Signed-off-by: Zygmunt Krynicki <email address hidden>

2850. By Zygmunt Krynicki on 2014-03-28

plainbox:secure:providers: expose control for job validation

This patch allows two settings (validate and validation_kwargs) to propagate
from Provider1 to JobDefinitionPlugIn. By default validation is enabled
but no validation keywords are supplied so only serious problems are
"reported" (as in preventing a job definition from being loaded).
Those can be changed by passing the two new keyword-only arguments.

They are passed from Provider1.from_definition(),
through Provider1.__init__() and to JobDefinitionPlugIn where they control
the validation block. The second argument (validation_kwargs) is passed
directly to the validator to control its behavior.

Some tests were altered not to overly validate everything where it is
obvious we don't care about valid jobs, just about getting some dummy
data across.

Signed-off-by: Zygmunt Krynicki <email address hidden>

2851. By Zygmunt Krynicki on 2014-03-28

plainbox:provider_manager: enforce strong validation in `manage.py validate`

This patch ironically allows the validate command to load broken job
definitions and inspect them manually inside the command core.

Signed-off-by: Zygmunt Krynicki <email address hidden>

2852. By Zygmunt Krynicki on 2014-03-28

plainbox:provider_manager: handle deprecated fields

This patch makes the `manage.py validate` command support the
Problem.deprecated problem which is used to report 'name' being used
inside job definitions.

Signed-off-by: Zygmunt Krynicki <email address hidden>

[Daniel Manrique (roadmr)]

Awesome, maybe next we can plug this validation into the tarmac tests to protect against borked jobs?

review: Approve

=== modified file 'plainbox/plainbox/impl/job.py'
--- plainbox/plainbox/impl/job.py	2014-03-27 13:57:14 +0000
+++ plainbox/plainbox/impl/job.py	2014-03-28 22:08:58 +0000
@@ -50,6 +50,7 @@
     missing = 'missing'
     wrong = 'wrong'
     useless = 'useless'
+    deprecated = 'deprecated'
 
 
 class ValidationError(ValueError):
@@ -75,15 +76,27 @@
     """
 
     @staticmethod
-    def validate(job):
+    def validate(job, strict=False, deprecated=False):
         """
         Validate the specified job
+
+        :param strict:
+            Enforce strict validation. Non-conforming jobs will be rejected.
+            This is off by default to ensure that non-critical errors don't
+            prevent jobs from running.
+        :param deprecated:
+            Enforce deprecation validation. Jobs having deprecated fields will
+            be rejected. This is off by default to allow backwards compatible
+            jobs to be used without any changes.
         """
-        # Check if id is empty
-        if job.id is None:
+        # Check if name is still being used, if running in strict mode
+        if deprecated and job.name is not None:
+            raise ValidationError(job.fields.name, Problem.deprecated)
+        # Check if the partial_id field is empty
+        if job.partial_id is None:
             raise ValidationError(job.fields.id, Problem.missing)
-        # Check if summary is empty
-        if job.summary is None:
+        # Check if summary is empty, if running in strict mode
+        if strict and job.summary is None:
             raise ValidationError(job.fields.summary, Problem.missing)
         # Check if plugin is empty
         if job.plugin is None:
@@ -91,11 +104,13 @@
         # Check if plugin has a good value
         if job.plugin not in JobDefinition.plugin.get_all_symbols():
             raise ValidationError(job.fields.plugin, Problem.wrong)
-        # Check if user is given without a command to run
-        if job.user is not None and job.command is None:
+        # Check if user is given without a command to run, if running in strict
+        # mode
+        if strict and job.user is not None and job.command is None:
             raise ValidationError(job.fields.user, Problem.useless)
-        # Check if environ is given without a command to run
-        if job.environ is not None and job.command is None:
+        # Check if environ is given without a command to run, if running in
+        # strict mode
+        if strict and job.environ is not None and job.command is None:
             raise ValidationError(job.fields.environ, Problem.useless)
         # Verify that command is present on a job within the subset that should
         # really have them (shell, local, resource, attachment, user-verify and
@@ -121,8 +136,9 @@
             if job.description is None:
                 raise ValidationError(
                     job.fields.description, Problem.missing)
-            # Ensure that manual jobs don't have command
-            if job.command is not None:
+            # Ensure that manual jobs don't have command, if running in strict
+            # mode
+            if strict and job.command is not None:
                 raise ValidationError(job.fields.command, Problem.useless)
 
 
@@ -481,16 +497,12 @@
     @classmethod
     def from_rfc822_record(cls, record):
         """
-        Create a JobDefinition instance from rfc822 record
+        Create a JobDefinition instance from rfc822 record. The resulting
+        instance may not be valid but will always be created. Only valid jobs
+        should be executed.
 
         The record must be a RFC822Record instance.
-
-        Only the 'id' and 'plugin' keys are required.
-        All other data is stored as is and is entirely optional.
         """
-        if 'id' not in record.data and 'name' not in record.data:
-            # TRANSLATORS: don't translate id or translate it as 'id field'
-            raise ValueError(_("Cannot create job without an id"))
         # Strip the trailing newlines form all the raw values coming from the
         # RFC822 parser. We don't need them and they don't match gettext keys
         # (xgettext strips out those newlines)
@@ -498,14 +510,17 @@
             key: value.rstrip('\n')
             for key, value in record.raw_data.items()})
 
-    def validate(self, validator_cls=CheckBoxJobValidator):
+    def validate(self, **validation_kwargs):
         """
         Validate this job definition with the specified validator
 
+        :param validation_kwargs:
+            Keyword arguments to pass to the
+            :meth:`CheckBoxJobValidator.validate()`
         :raises ValidationError:
             If the job has any problems that make it unsuitable for execution.
         """
-        validator_cls.validate(self)
+        CheckBoxJobValidator.validate(self, **validation_kwargs)
 
     def create_child_job_from_record(self, record):
         """
 
=== modified file 'plainbox/plainbox/impl/secure/providers/test_v1.py'
--- plainbox/plainbox/impl/secure/providers/test_v1.py	2014-03-28 13:02:24 +0000
+++ plainbox/plainbox/impl/secure/providers/test_v1.py	2014-03-28 22:08:58 +0000
@@ -25,7 +25,6 @@
 """
 
 from unittest import TestCase
-import os
 
 from plainbox.impl.job import JobDefinition
 from plainbox.impl.secure.config import Unset
@@ -649,7 +648,10 @@
         self.provider = Provider1(
             self.NAME, self.VERSION, self.DESCRIPTION, self.SECURE,
             self.GETTEXT_DOMAIN, self.JOBS_DIR, self.WHITELISTS_DIR,
-            self.DATA_DIR, self.BIN_DIR, self.LOCALE_DIR, self.BASE_DIR)
+            self.DATA_DIR, self.BIN_DIR, self.LOCALE_DIR, self.BASE_DIR,
+            # We are using dummy job definitions so let's not shout about those
+            # being invalid in each test
+            validate=False)
 
     def test_repr(self):
         self.assertEqual(
@@ -792,11 +794,11 @@
             JobDefinitionPlugIn("/path/to/jobs1.txt", (
                 "id: a2\n"
                 "\n"
-                "id: a1\n"), self.provider),
+                "id: a1\n"), self.provider, validate=False),
             JobDefinitionPlugIn("/path/to/jobs2.txt", (
                 "id: a3\n"
                 "\n"
-                "id: a4\n"), self.provider)
+                "id: a4\n"), self.provider, validate=False)
         ]
         with self.provider._job_collection.fake_plugins(fake_plugins):
             job_list = self.provider.get_builtin_jobs()
@@ -840,11 +842,11 @@
             JobDefinitionPlugIn("/path/to/jobs1.txt", (
                 "id: a2\n"
                 "\n"
-                "id: a1\n"), self.provider),
+                "id: a1\n"), self.provider, validate=False),
             JobDefinitionPlugIn("/path/to/jobs2.txt", (
                 "id: a3\n"
                 "\n"
-                "id: a4\n"), self.provider)
+                "id: a4\n"), self.provider, validate=False)
         ]
         with self.provider._job_collection.fake_plugins(fake_plugins):
             job_list, problem_list = self.provider.load_all_jobs()
@@ -862,7 +864,8 @@
         """
         fake_plugins = [
             JobDefinitionPlugIn(
-                "/path/to/jobs1.txt", "id: working\n", self.provider)
+                "/path/to/jobs1.txt", "id: working\n", self.provider,
+                validate=False)
         ]
         fake_problems = [
             PlugInError("some problem"),
 
=== modified file 'plainbox/plainbox/impl/secure/providers/v1.py'
--- plainbox/plainbox/impl/secure/providers/v1.py	2014-03-28 13:02:24 +0000
+++ plainbox/plainbox/impl/secure/providers/v1.py	2014-03-28 22:08:58 +0000
@@ -31,6 +31,7 @@
 from plainbox.abc import IProvider1, IProviderBackend1
 from plainbox.i18n import gettext as _
 from plainbox.impl.job import JobDefinition
+from plainbox.impl.job import ValidationError
 from plainbox.impl.secure.config import Config, Variable
 from plainbox.impl.secure.config import IValidator
 from plainbox.impl.secure.config import NotEmptyValidator
@@ -87,12 +88,30 @@
     list of :class:`plainbox.impl.job.JobDefinition` instances from a file.
     """
 
-    def __init__(self, filename, text, provider):
+    def __init__(self, filename, text, provider, *,
+                 validate=True, validation_kwargs=None):
         """
         Initialize the plug-in with the specified name text
+
+        :param filename:
+            Name of the file with job definitions
+        :param text:
+            Full text of the file with job definitions
+        :param provider:
+            A provider object to which those jobs belong to
+        :param validate:
+            Enable job validation. Incorrect job definitions will not be loaded
+            and will abort the process of loading of the remainder of the jobs.
+            This is ON by default to prevent broken job definitions from being
+            used. This is a keyword-only argument.
+        :param validation_kwargs:
+            Keyword arguments to pass to the JobDefinition.validate().  Note,
+            this is a single argument. This is a keyword-only argument.
         """
         self._filename = filename
         self._job_list = []
+        if validation_kwargs is None:
+            validation_kwargs = {}
         logger.debug(_("Loading jobs definitions from %r..."), filename)
         try:
             records = load_rfc822_records(
@@ -108,10 +127,16 @@
                 raise PlugInError(
                     _("Cannot define job from record {!r}: {}").format(
                         record, exc))
-            else:
-                job._provider = provider
-                self._job_list.append(job)
-                logger.debug(_("Loaded %r"), job)
+            job._provider = provider
+            if validate:
+                try:
+                    job.validate(**validation_kwargs)
+                except ValidationError as exc:
+                    raise PlugInError(
+                        _("Problem in job definition, field {}: {}").format(
+                            exc.field, exc.problem))
+            self._job_list.append(job)
+            logger.debug(_("Loaded %r"), job)
 
     @property
     def plugin_name(self):
@@ -142,7 +167,7 @@
 
     def __init__(self, name, version, description, secure, gettext_domain,
                  jobs_dir, whitelists_dir, data_dir, bin_dir, locale_dir,
-                 base_dir):
+                 base_dir, *, validate=True, validation_kwargs=None):
         """
         Initialize a provider with a set of meta-data and directories.
 
@@ -187,6 +212,16 @@
             path of the directory with (perhaps) all of jobs_dir,
             whitelist_dir, data_dir, bin_dir, locale_dir. This may be None.
             This is also the effective value of $CHECKBOX_SHARE
+
+        :param validate:
+            Enable job validation. Incorrect job definitions will not be loaded
+            and will abort the process of loading of the remainder of the jobs.
+            This is ON by default to prevent broken job definitions from being
+            used. This is a keyword-only argument.
+
+        :param validation_kwargs:
+            Keyword arguments to pass to the JobDefinition.validate().  Note,
+            this is a single argument. This is a keyword-only argument.
         """
         # Meta-data
         self._name = name
@@ -215,22 +250,31 @@
             jobs_dir_list = []
         self._job_collection = FsPlugInCollection(
             jobs_dir_list, ext=(".txt", ".txt.in"),
-            wrapper=JobDefinitionPlugIn, provider=self)
+            wrapper=JobDefinitionPlugIn, provider=self,
+            validate=validate, validation_kwargs=validation_kwargs)
         # Setup translations
         if gettext_domain and locale_dir:
             gettext.bindtextdomain(self._gettext_domain, self._locale_dir)
 
     @classmethod
-    def from_definition(cls, definition, secure):
+    def from_definition(cls, definition, secure, *, validate=True,
+                        validation_kwargs=None):
         """
         Initialize a provider from Provider1Definition object
 
         :param definition:
             A Provider1Definition object to use as reference
-
         :param secure:
             Value of the secure flag. This cannot be expressed by a definition
             object.
+        :param validate:
+            Enable job validation. Incorrect job definitions will not be loaded
+            and will abort the process of loading of the remainder of the jobs.
+            This is ON by default to prevent broken job definitions from being
+            used. This is a keyword-only argument.
+        :param validation_kwargs:
+            Keyword arguments to pass to the JobDefinition.validate().  Note,
+            this is a single argument. This is a keyword-only argument.
 
         This method simplifies initialization of a Provider1 object where the
         caller already has a Provider1Definition object. Depending on the value
@@ -248,7 +292,8 @@
             secure, definition.effective_gettext_domain,
             definition.effective_jobs_dir, definition.effective_whitelists_dir,
             definition.effective_data_dir, definition.effective_bin_dir,
-            definition.effective_locale_dir, definition.location or None)
+            definition.effective_locale_dir, definition.location or None,
+            validate=validate, validation_kwargs=validation_kwargs)
 
     def __repr__(self):
         return "<{} name:{!r}>".format(self.__class__.__name__, self.name)
 
=== modified file 'plainbox/plainbox/impl/test_job.py'
--- plainbox/plainbox/impl/test_job.py	2014-03-27 13:57:24 +0000
+++ plainbox/plainbox/impl/test_job.py	2014-03-28 22:08:58 +0000
@@ -42,6 +42,19 @@
 
 class CheckBoxJobValidatorTests(TestCase):
 
+    def test_validate_checks_for_deprecated_name(self):
+        """
+        verify that validate() checks if jobs have a value for the 'id'
+        field.
+        """
+        job = JobDefinition({
+            'name': 'name'
+        })
+        with self.assertRaises(ValidationError) as boom:
+            CheckBoxJobValidator.validate(job, deprecated=True)
+        self.assertEqual(boom.exception.field, JobDefinition.fields.name)
+        self.assertEqual(boom.exception.problem, Problem.deprecated)
+
     def test_validate_checks_for_missing_id(self):
         """
         verify that validate() checks if jobs have a value for the 'id'
@@ -91,7 +104,7 @@
             'user': 'root'
         })
         with self.assertRaises(ValidationError) as boom:
-            CheckBoxJobValidator.validate(job)
+            CheckBoxJobValidator.validate(job, strict=True)
         self.assertEqual(boom.exception.field, JobDefinition.fields.user)
         self.assertEqual(boom.exception.problem, Problem.useless)
 
@@ -106,7 +119,7 @@
             'environ': 'VAR_NAME'
         })
         with self.assertRaises(ValidationError) as boom:
-            CheckBoxJobValidator.validate(job)
+            CheckBoxJobValidator.validate(job, strict=True)
         self.assertEqual(boom.exception.field, JobDefinition.fields.environ)
         self.assertEqual(boom.exception.problem, Problem.useless)
 
@@ -137,7 +150,7 @@
             'command': 'run_some_test'
         })
         with self.assertRaises(ValidationError) as boom:
-            CheckBoxJobValidator.validate(job)
+            CheckBoxJobValidator.validate(job, strict=True)
         self.assertEqual(boom.exception.field, JobDefinition.fields.command)
         self.assertEqual(boom.exception.problem, Problem.useless)
 
@@ -254,11 +267,6 @@
         self.assertEqual(job.command, None)
         self.assertEqual(job.description, None)
 
-    def test_from_rfc822_record_missing_id(self):
-        record = RFC822Record({'plugin': 'plugin'})
-        with self.assertRaises(ValueError):
-            JobDefinition.from_rfc822_record(record)
-
     def test_str(self):
         job = JobDefinition(self._min_record.data)
         self.assertEqual(str(job), "id")
 
=== modified file 'plainbox/plainbox/provider_manager.py'
--- plainbox/plainbox/provider_manager.py	2014-03-28 13:22:04 +0000
+++ plainbox/plainbox/provider_manager.py	2014-03-28 22:08:58 +0000
@@ -623,7 +623,7 @@
         problem_list = []
         for job in job_list:
             try:
-                job.validate()
+                job.validate(strict=True, deprecated=True)
             except JobValidationError as exc:
                 problem_list.append((job, exc))
         return problem_list
@@ -636,6 +636,7 @@
             Problem.missing: _("missing definition of required field"),
             Problem.wrong: _("incorrect value supplied"),
             Problem.useless: _("useless field in this context"),
+            Problem.deprecated: _("usage of deprecated field"),
         }
         for job, error in problem_list:
             if isinstance(error, JobValidationError):
@@ -664,6 +665,17 @@
         else:
             print(_("All jobs seem to be valid"))
 
+    def get_provider(self):
+        """
+        Get a Provider1 that describes the current provider
+
+        This version disables all validation so that we can see totally broken
+        providers and let us validate them and handle the errors explicitly.
+        """
+        return Provider1.from_definition(
+            # NOTE: don't validate, we want to validate manually
+            self.definition, secure=False, validate=False)
+
 
 @docstring(
     # TRANSLATORS: please leave various options (both long and short forms),
 
=== modified file 'plainbox/plainbox/test_provider_manager.py'
--- plainbox/plainbox/test_provider_manager.py	2014-03-28 13:22:04 +0000
+++ plainbox/plainbox/test_provider_manager.py	2014-03-28 22:08:58 +0000
@@ -120,7 +120,7 @@
             self.tmpdir + os.path.join("/foo", "lib", "plainbox-providers-1",
                                        "2014.com.example:test", "jobs",
                                        "jobs.txt"),
-            "name: dummy\nplugin: shell\ncommand: true\n")
+            "id: dummy\nplugin: shell\ncommand: true\n")
 
     def test_install__flat_partial(self):
         """
@@ -155,7 +155,7 @@
         self.assertFileContent(
             self.tmpdir + os.path.join(
                 prefix, "share", "2014.com.example:test", "jobs", "jobs.txt"),
-            "name: dummy\nplugin: shell\ncommand: true\n")
+            "id: dummy\nplugin: shell\ncommand: true\n")
         self.assertFileContent(
             self.tmpdir + os.path.join(
                 prefix, "share",  "2014.com.example:test", "whitelists",
@@ -198,7 +198,7 @@
             self.tmpdir, "dist", "2014.com.example.test-1.0.tar.gz")
         self.assertTarballContent(
             tarball, "2014.com.example.test-1.0/jobs/jobs.txt",
-            "name: dummy\nplugin: shell\ncommand: true\n")
+            "id: dummy\nplugin: shell\ncommand: true\n")
         self.assert_common_sdist(tarball)
 
     def test_sdist__partial(self):
@@ -287,7 +287,7 @@
         """
         filename = os.path.join(self.tmpdir, "jobs", "broken.txt")
         with open(filename, "wt", encoding='UTF-8') as stream:
-            print("name: broken", file=stream)
+            print("id: broken", file=stream)
             print("plugin: shell", file=stream)
         with TestIO() as test_io:
             self.tool.main(["validate"])
@@ -303,7 +303,7 @@
         """
         filename = os.path.join(self.tmpdir, "jobs", "broken.txt")
         with open(filename, "wt", encoding='UTF-8') as stream:
-            print("name: broken", file=stream)
+            print("id: broken", file=stream)
             print("plugin: magic", file=stream)
         with TestIO() as test_io:
             self.tool.main(["validate"])
@@ -321,7 +321,7 @@
         """
         filename = os.path.join(self.tmpdir, "jobs", "broken.txt")
         with open(filename, "wt", encoding='UTF-8') as stream:
-            print("name: broken", file=stream)
+            print("id: broken", file=stream)
             print("plugin: manual", file=stream)
             print("description: broken job definition", file=stream)
             print("command: true", file=stream)
@@ -332,6 +332,23 @@
                 "jobs/broken.txt:1-4: job '2014.com.example::broken', field 'command': "
                 "useless field in this context\n"))
 
+    def test_validate__broken_deprecated_field(self):
+        """
+        verify that ./manage.py validate shows information about deprecated fields
+        """
+        filename = os.path.join(self.tmpdir, "jobs", "broken.txt")
+        with open(filename, "wt", encoding='UTF-8') as stream:
+            print("name: broken", file=stream)
+            print("plugin: manual", file=stream)
+            print("description: broken job definition", file=stream)
+            print("command: true", file=stream)
+        with TestIO() as test_io:
+            self.tool.main(["validate"])
+        self.assertEqual(
+            test_io.stdout, (
+                "jobs/broken.txt:1-4: job '2014.com.example::broken', field 'name': "
+                "usage of deprecated field\n"))
+
     def test_info(self):
         """
         verify that ./manage.py info shows basic provider information
@@ -359,7 +376,7 @@
         os.mkdir(os.path.join(tmpdir, "jobs"))
         filename = os.path.join(tmpdir, "jobs", "jobs.txt")
         with open(filename, "wt", encoding='UTF-8') as stream:
-            print("name: dummy", file=stream)
+            print("id: dummy", file=stream)
             print("plugin: shell", file=stream)
             print("command: true", file=stream)
         os.mkdir(os.path.join(tmpdir, "whitelists"))