Merge lp:~trapnine/maas/fix-1499062 into lp:~maas-committers/maas/trunk

[Jeffrey C Jones]

Fix lp:1499062. Use URLField for boot_source_url, strip whitespace on boot_source_keyring in BootSourceSettingsForm.

[Blake Rouse (blake-rouse)]

Code itself looks good. The unit testing needs to be improved. Overall you have good coverage just the format and how the tests are being done need to be improved. Comments inline about the testing.

review: Needs Fixing

[Blake Rouse (blake-rouse)]

Almost there. Just need to split out the tests.

review: Needs Fixing

[Blake Rouse (blake-rouse)]

Looks good. Thanks for the fixes.

review: Approve

=== modified file 'src/maasserver/fields.py'
--- src/maasserver/fields.py	2015-10-07 05:01:23 +0000
+++ src/maasserver/fields.py	2015-10-20 21:36:17 +0000
@@ -200,6 +200,14 @@
         return nodegroup
 
 
+class StrippedCharField(CharField):
+    """A CharField that will strip surrounding whitespace before validation."""
+
+    def clean(self, value):
+        value = self.to_python(value).strip()
+        return super(CharField, self).clean(value)
+
+
 class VerboseRegexField(CharField):
 
     def __init__(self, regex, message, *args, **kwargs):

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2015-09-24 16:22:12 +0000
+++ src/maasserver/forms.py	2015-10-20 21:36:17 +0000
@@ -105,6 +105,7 @@
     LargeObjectFile,
     MACAddressFormField,
     NodeGroupFormField,
+    StrippedCharField,
 )
 from maasserver.forms_settings import (
     CONFIG_ITEMS_KEYS,
@@ -1380,7 +1381,7 @@
     config_fields = ['boot_images_auto_import']
 
     boot_images_auto_import = get_config_field('boot_images_auto_import')
-    boot_source_url = forms.CharField(
+    boot_source_url = forms.URLField(
         label="Sync URL", required=True,
         help_text=(
             "URL to sync boot image from. E.g. "
@@ -1396,7 +1397,7 @@
         keyring_data."""
         boot_source = BootSource.objects.first()
         if boot_source is None or len(boot_source.keyring_data) == 0:
-            self.fields['boot_source_keyring'] = forms.CharField(
+            self.fields['boot_source_keyring'] = StrippedCharField(
                 label="Keyring Path", required=True,
                 help_text=(
                     "Path to the keyring to validate the sync URL. E.g. "

=== modified file 'src/maasserver/tests/test_forms_settings.py'
--- src/maasserver/tests/test_forms_settings.py	2015-07-01 19:02:36 +0000
+++ src/maasserver/tests/test_forms_settings.py	2015-10-20 21:36:17 +0000
@@ -16,6 +16,7 @@
 
 
 from django import forms
+from maasserver.forms import BootSourceSettingsForm
 from maasserver.forms_settings import (
     CONFIG_ITEMS,
     get_config_doc,
@@ -78,3 +79,41 @@
         ips2 = [factory.make_ip_address() for _ in range(3)]
         input = ' '.join(ips1) + ' ' + ','.join(ips2)
         self.assertEqual(' '.join(ips1 + ips2), field.clean(input))
+
+
+class TestBootSourceSettingsForm(MAASServerTestCase):
+
+    def setUp(self):
+        super(TestBootSourceSettingsForm, self).setUp()
+        self.form_data = {
+            'boot_source_url': 'http://example.com/good',
+            'boot_source_keyring': '/a/path'}
+
+    def test_happy_with_good_data(self):
+        form = BootSourceSettingsForm(data=self.form_data)
+        self.assertTrue(form.is_valid())
+        self.assertEqual(
+            "http://example.com/good",
+            form.cleaned_data['boot_source_url'])
+        self.assertEqual("/a/path", form.cleaned_data['boot_source_keyring'])
+
+    def test_unhappy_by_default(self):
+        form = BootSourceSettingsForm()
+        self.assertFalse(form.is_valid())
+
+    def test_reject_leading_spaces_in_boot_source_url(self):
+        # https://bugs.launchpad.net/maas/+bug/1499062
+        self.form_data['boot_source_url'] = ' http://example.com/leadingspace'
+        form = BootSourceSettingsForm(data=self.form_data)
+        self.assertFalse(form.is_valid())
+
+    def test_reject_non_url_in_boot_source_url(self):
+        self.form_data['boot_source_url'] = 'not_a_URL'
+        form = BootSourceSettingsForm(data=self.form_data)
+        self.assertFalse(form.is_valid())
+
+    def test_strips_boot_source_keyring(self):
+        self.form_data['boot_source_keyring'] = ' /a/path '
+        form = BootSourceSettingsForm(data=self.form_data)
+        self.assertTrue(form.is_valid())
+        self.assertEqual("/a/path", form.cleaned_data['boot_source_keyring'])