Merge ~ltrager/maas:lp1835289_2.6 into maas:2.6

[Lee Trager]

Backport of 36d5dd0 - LP: #1835289 - Fix error handling on the commissioning API.

The commissioning API was returning an exception instead or raising it. This
caused a 500 internal error with no readable error message. Fixing this
revealed the commissioning API cannot be used with embedded script YAML as
the commissioning API sets the script_type to commissioning. The form now
allows fields to be set as long as they match what is in the script YAML.

[Lee Trager (ltrager)]

Approved in https://code.launchpad.net/~ltrager/maas/+git/maas/+merge/369863

review: Approve

diff --git a/src/maasserver/api/commissioning_scripts.py b/src/maasserver/api/commissioning_scripts.py
index 16791fc..906e3ee 100644
--- a/src/maasserver/api/commissioning_scripts.py
+++ b/src/maasserver/api/commissioning_scripts.py
@@ -1,4 +1,4 @@
-# Copyright 2014-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """API handlers: `CommissioningScript`."""
@@ -92,11 +92,14 @@ class CommissioningScriptsHandler(OperationsHandler):
             return {
                 'name': script.name,
                 'content': b64encode(script.script.data.encode()),
+                'deprecated': (
+                    'The commissioning-scripts endpoint is deprecated. '
+                    'Please use the node-scripts endpoint.'),
                 'resource_uri': reverse(
                     'commissioning_script_handler', args=[script.name]),
             }
         else:
-            return MAASAPIValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
     @classmethod
     def resource_uri(cls):
@@ -142,7 +145,7 @@ class CommissioningScriptHandler(OperationsHandler):
             form.save(request)
             return rc.ALL_OK
         else:
-            return MAASAPIValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
     @classmethod
     def resource_uri(cls, script=None):
diff --git a/src/maasserver/api/tests/test_commissioning.py b/src/maasserver/api/tests/test_commissioning.py
index 1cfa07b..49dcce2 100644
--- a/src/maasserver/api/tests/test_commissioning.py
+++ b/src/maasserver/api/tests/test_commissioning.py
@@ -1,4 +1,4 @@
-# Copyright 2013-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2013-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the commissioning-related portions of the MAAS API."""
@@ -74,6 +74,22 @@ class AdminCommissioningScriptsAPITest(APITestCase.ForAdmin):
         self.assertEqual(content, stored_script.script.data)
         self.assertEqual(SCRIPT_TYPE.COMMISSIONING, stored_script.script_type)
 
+    def test_POST_creates_errors(self):
+        content = factory.make_script_content(
+            yaml_content={'name': factory.make_name('name')})
+        response = self.client.post(
+            self.get_url(),
+            {
+                'name': factory.make_name('name'),
+                'content': factory.make_file_upload(content=content.encode()),
+            })
+        self.assertThat(response, HasStatusCode(http.client.BAD_REQUEST))
+
+        ret = json_load_bytes(response.content)
+        self.assertDictEqual(
+            {'name': ['May not override values defined in embedded YAML.']},
+            ret)
+
 
 class CommissioningScriptsAPITest(APITestCase.ForUser):
 
@@ -118,6 +134,28 @@ class AdminCommissioningScriptAPITest(APITestCase.ForAdmin):
         self.assertEqual(
             new_content.decode('utf-8'), reload_object(script).script.data)
 
+    def test_PUT_errors(self):
+        old_content = factory.make_script_content(
+            yaml_content={'name': factory.make_name('name')})
+        old_content = old_content.encode('ascii')
+        script = factory.make_Script(
+            script_type=SCRIPT_TYPE.COMMISSIONING, script=old_content)
+        new_content = factory.make_script_content(
+            yaml_content={'name': factory.make_name('name')})
+        new_content = new_content.encode('ascii')
+
+        response = self.client.put(
+            self.get_url(script.name),
+            {
+                'name': factory.make_name('name'),
+                'content': factory.make_file_upload(content=new_content)})
+        self.assertThat(response, HasStatusCode(http.client.BAD_REQUEST))
+
+        ret = json_load_bytes(response.content)
+        self.assertDictEqual(
+            {'name': ['May not override values defined in embedded YAML.']},
+            ret)
+
     def test_DELETE_deletes_script(self):
         script = factory.make_Script(script_type=SCRIPT_TYPE.COMMISSIONING)
         self.client.delete(self.get_url(script.name))
diff --git a/src/maasserver/forms/script.py b/src/maasserver/forms/script.py
index 4555831..ab03f36 100644
--- a/src/maasserver/forms/script.py
+++ b/src/maasserver/forms/script.py
@@ -169,6 +169,73 @@ class ScriptForm(ModelForm):
             valid = False
         return valid
 
+    def _clean_script(self, parsed_yaml):
+        """Clean script data and validate input."""
+        # Tags and timeout may not be updated from new embedded YAML. This
+        # allows users to receive updated scripts from an upstream maintainer,
+        # such as Canonical, while maintaining user defined tags and timeout.
+
+        # Tags must be a comma seperated string for the form.
+        tags = parsed_yaml.pop('tags', None)
+        if (tags is not None and self.instance.id is None and
+                'tags' not in self.data):
+            tags_valid = True
+            if isinstance(tags, str):
+                self.data['tags'] = tags
+            elif isinstance(tags, list):
+                for tag in tags:
+                    if not isinstance(tag, str):
+                        tags_valid = False
+                        continue
+                if tags_valid:
+                    self.data['tags'] = ','.join(tags)
+            else:
+                tags_valid = False
+            if not tags_valid:
+                set_form_error(
+                    self, 'tags',
+                    'Embedded tags must be a string of comma seperated '
+                    'values, or a list of strings.')
+
+        # Timeout must be a string for the form.
+        timeout = parsed_yaml.pop('timeout', None)
+        if (timeout is not None and self.instance.id is None and
+                'timeout' not in self.data):
+            self.data['timeout'] = str(timeout)
+
+        # Packages and for_hardware must be a JSON string for the form.
+        for key in ['packages', 'for_hardware']:
+            value = parsed_yaml.pop(key, None)
+            if value is not None and key not in self.data:
+                self.data[key] = json.dumps(value)
+
+        for key, value in parsed_yaml.items():
+            if key in self.fields:
+                error = False
+                if key not in self.data:
+                    self.data[key] = value
+                elif key == 'script_type':
+                    # The deprecated Commissioning API always sets the
+                    # script_type to commissioning as it has always only
+                    # accepted commissioning scripts while the form sets
+                    # the default type to testing. If the YAML matches the
+                    # type allow it.
+                    try:
+                        if (translate_script_type(value) !=
+                                translate_script_type(self.data[key])):
+                            error = True
+                    except ValidationError:
+                        error = True
+                elif value != self.data[key]:
+                    # Only allow form data for fields defined in the YAML if
+                    # the data matches.
+                    error = True
+
+                if error:
+                    set_form_error(
+                        self, key,
+                        'May not override values defined in embedded YAML.')
+
     def _read_script(self):
         """Read embedded YAML configuration in a script.
 
@@ -224,52 +291,7 @@ class ScriptForm(ModelForm):
         self.instance.results = parsed_yaml.pop('results', {})
         self.instance.parameters = parsed_yaml.pop('parameters', {})
 
-        # Tags and timeout may not be updated from new embedded YAML. This
-        # allows users to receive updated scripts from an upstream maintainer,
-        # such as Canonical, while maintaining user defined tags and timeout.
-
-        # Tags must be a comma seperated string for the form.
-        tags = parsed_yaml.pop('tags', None)
-        if (tags is not None and self.instance.id is None and
-                'tags' not in self.data):
-            tags_valid = True
-            if isinstance(tags, str):
-                self.data['tags'] = tags
-            elif isinstance(tags, list):
-                for tag in tags:
-                    if not isinstance(tag, str):
-                        tags_valid = False
-                        continue
-                if tags_valid:
-                    self.data['tags'] = ','.join(tags)
-            else:
-                tags_valid = False
-            if not tags_valid:
-                set_form_error(
-                    self, 'tags',
-                    'Embedded tags must be a string of comma seperated '
-                    'values, or a list of strings.')
-
-        # Timeout must be a string for the form.
-        timeout = parsed_yaml.pop('timeout', None)
-        if (timeout is not None and self.instance.id is None and
-                'timeout' not in self.data):
-            self.data['timeout'] = str(timeout)
-
-        # Packages and for_hardware must be a JSON string for the form.
-        for key in ['packages', 'for_hardware']:
-            value = parsed_yaml.pop(key, None)
-            if value is not None and key not in self.data:
-                self.data[key] = json.dumps(value)
-
-        for key, value in parsed_yaml.items():
-            if key in self.fields:
-                if key not in self.data:
-                    self.data[key] = value
-                else:
-                    set_form_error(
-                        self, key,
-                        'May not override values defined in embedded YAML.')
+        self._clean_script(parsed_yaml)
 
     def clean_packages(self):
         if self.cleaned_data['packages'] == '':
diff --git a/src/maasserver/forms/tests/test_script.py b/src/maasserver/forms/tests/test_script.py
index b7df349..23f8c69 100644
--- a/src/maasserver/forms/tests/test_script.py
+++ b/src/maasserver/forms/tests/test_script.py
@@ -618,6 +618,19 @@ class TestScriptForm(MAASServerTestCase):
         })
         self.assertFalse(form.is_valid())
 
+    def test__user_option_can_match_yaml_value(self):
+        name = factory.make_name('name')
+        script_type = factory.pick_choice(SCRIPT_TYPE_CHOICES)
+        form = ScriptForm(data={
+            'name': name,
+            'script_type': script_type,
+            'script': factory.make_script_content({
+                'name': name,
+                'script_type': script_type,
+            })
+        })
+        self.assertTrue(form.is_valid())
+
     def test__errors_on_bad_yaml(self):
         form = ScriptForm(data={
             'name': factory.make_name('name'),