Merge lp:~blake-rouse/maas/license-key-form into lp:~maas-committers/maas/trunk

[Blake Rouse]

Add LicenseKeyForm for the LicenseKey model.

This is the third part of a series of changes to allow a global registry of license key's in MAAS. The end result will allow a user to set a license key for each operating system and series globally. When a booting operating system and series requires a license key, first the node will be check to see if a specific key has been set in the license_key field. If not then this LicenseKey model will be used to see if one is set globally for the selected operating system and series.

This branch includes the form that will be used in the WebUI and API.

[Raphaël Badin (rvb)]

I see what you're trying to do here and you're on the right path… but I've spotted problems in the form you're adding here, see my comments inline. Also, the code of the list_all_* utilities is really starting to look like spaghetti code; since these methods are well-encapsulated helpers I wouldn't mind so much if they were clearly documented and well tested but it doesn't look like it's the case; I'd like you to have another look at this and see if you can improve the situation and reply to my questions: see the inline comments.

review: Needs Fixing

[Blake Rouse (blake-rouse)]

Quick question before I work on the fixes, question is in the inline comments.

[Raphaël Badin (rvb)]

Replies inline…

[Blake Rouse (blake-rouse)]

I have fixed the items you requested. I did not add any more tests for the utils/osystems.py, as I will move the tests from test_forms into utils/tests/test_osystems.py and update the tests for the new optional parameters in another branch.

[Raphaël Badin (rvb)]

A couple more remarks but nothing major. Thanks for all the work on this branch… I'm looking forward to the follow-up branch with the cleanups ;).

review: Approve

[Blake Rouse (blake-rouse)]

Updated! Comment inline about the clean fields check.

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-06-27 20:02:49 +0000
+++ src/maasserver/forms.py	2014-07-02 14:55:33 +0000
@@ -98,6 +98,7 @@
     BootSourceSelection,
     Config,
     DownloadProgress,
+    LicenseKey,
     MACAddress,
     Network,
     Node,
@@ -119,6 +120,7 @@
 from maasserver.utils.osystems import (
     get_distro_series_inital,
     get_release_requires_key,
+    list_all_releases_requiring_keys,
     list_all_usable_osystems,
     list_all_usable_releases,
     list_osystem_choices,
@@ -1826,3 +1828,110 @@
         if kwargs.get('commit', True):
             boot_source_selection.save(*args, **kwargs)
         return boot_source_selection
+
+
+class LicenseKeyForm(ModelForm):
+    """Form for global license keys."""
+
+    class Meta:
+        model = LicenseKey
+        fields = (
+            'osystem',
+            'distro_series',
+            'license_key',
+            )
+
+    def __init__(self, *args, **kwargs):
+        super(LicenseKeyForm, self).__init__(*args, **kwargs)
+        self.set_up_osystem_and_distro_series_fields(kwargs.get('instance'))
+
+    def set_up_osystem_and_distro_series_fields(self, instance):
+        """Create the `osystem` and `distro_series` fields.
+
+        This needs to be done on the fly so that we can pass a dynamic list of
+        usable operating systems and distro_series.
+        """
+        osystems = list_all_usable_osystems(have_images=False)
+        releases = list_all_releases_requiring_keys(osystems)
+
+        # Remove the operating systems that do not have any releases that
+        # require license keys. Don't want them to show up in the UI or be
+        # used in the API.
+        osystems = [
+            osystem
+            for osystem in osystems
+            if osystem.name in releases
+            ]
+
+        os_choices = list_osystem_choices(osystems, include_default=False)
+        distro_choices = list_release_choices(
+            releases, include_default=False, include_latest=False,
+            with_key_required=False)
+        invalid_osystem_message = compose_invalid_choice_text(
+            'osystem', os_choices)
+        invalid_distro_series_message = compose_invalid_choice_text(
+            'distro_series', distro_choices)
+        self.fields['osystem'] = forms.ChoiceField(
+            label="OS", choices=os_choices, required=True,
+            error_messages={'invalid_choice': invalid_osystem_message})
+        self.fields['distro_series'] = forms.ChoiceField(
+            label="Release", choices=distro_choices, required=True,
+            error_messages={'invalid_choice': invalid_distro_series_message})
+        if instance is not None:
+            initial_value = get_distro_series_inital(
+                instance, with_key_required=False)
+            if instance is not None:
+                self.initial['distro_series'] = initial_value
+
+    def full_clean(self):
+        # When this form is used from the API, the distro_series field will
+        # not be formatted correctly. This is to make it easy on the user, and
+        # not have to call the api with distro_series=os/series. This occurs
+        # in full_clean, so the value is correct before validation occurs on
+        # the distro_series field.
+        if 'distro_series' in self.data and 'osystem' in self.data:
+            if '/' not in self.data['distro_series']:
+                self.data['distro_series'] = '%s/%s' % (
+                    self.data['osystem'],
+                    self.data['distro_series'],
+                    )
+        super(LicenseKeyForm, self).full_clean()
+
+    def clean(self):
+        """Validate distro_series and osystem match, and license_key is valid
+        for selected operating system and series."""
+        # Get the clean_data, check that all of the fields we need are
+        # present. If not then the form will error, so no reason to continue.
+        cleaned_data = super(LicenseKeyForm, self).clean()
+        required_fields = ['license_key', 'osystem', 'distro_series']
+        for field in required_fields:
+            if field not in cleaned_data:
+                return cleaned_data
+        cleaned_data['distro_series'] = self.clean_osystem_distro_series_field(
+            cleaned_data)
+        self.validate_license_key(cleaned_data)
+        return cleaned_data
+
+    def clean_osystem_distro_series_field(self, cleaned_data):
+        """Validate that os/distro_series matches osystem, and update the
+        distro_series field, to remove the leading os/."""
+        cleaned_osystem = cleaned_data['osystem']
+        cleaned_series = cleaned_data['distro_series']
+        series_os, release = cleaned_series.split('/', 1)
+        if series_os != cleaned_osystem:
+            raise ValidationError(
+                "%s in distro_series does not match with "
+                "operating system %s" % (release, cleaned_osystem))
+        return release
+
+    def validate_license_key(self, cleaned_data):
+        """Validates that the license key is valid."""
+        cleaned_key = cleaned_data['license_key']
+        cleaned_osystem = cleaned_data['osystem']
+        cleaned_series = cleaned_data['distro_series']
+        os_obj = OperatingSystemRegistry.get_item(cleaned_osystem)
+        if os_obj is None:
+            raise ValidationError(
+                "Failed to retrieve %s from os registry." % cleaned_osystem)
+        elif not os_obj.validate_license_key(cleaned_series, cleaned_key):
+            raise ValidationError("Invalid license key.")

=== modified file 'src/maasserver/models/licensekey.py'
--- src/maasserver/models/licensekey.py	2014-06-26 14:09:40 +0000
+++ src/maasserver/models/licensekey.py	2014-07-02 14:55:33 +0000
@@ -74,10 +74,21 @@
     distro_series = CharField(max_length=255, blank=False)
 
     # License key for the osystem/distro_series combo.
-    license_key = CharField(max_length=255, blank=False)
+    license_key = CharField(
+        max_length=255, blank=False, verbose_name="License Key", help_text=(
+            "License key for operating system"))
 
     def __repr__(self):
         return "<LicenseKey %s/%s>" % (
             self.osystem,
             self.distro_series,
             )
+
+    def unique_error_message(self, model_class, unique_check):
+        if unique_check == ('osystem', 'distro_series'):
+            return '%s %s' % (
+                "License key with this operating system and distro series",
+                "already exists.",
+                )
+        return super(
+            LicenseKey, self).unique_error_message(model_class, unique_check)

=== modified file 'src/maasserver/models/tests/test_licensekey.py'
--- src/maasserver/models/tests/test_licensekey.py	2014-06-26 13:17:00 +0000
+++ src/maasserver/models/tests/test_licensekey.py	2014-07-02 14:55:33 +0000
@@ -14,6 +14,7 @@
 __metaclass__ = type
 __all__ = []
 
+from django.db import IntegrityError
 from maasserver.models import LicenseKey
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
@@ -46,3 +47,11 @@
         self.assertFalse(
             LicenseKey.objects.has_license_key(
                 osystem, series))
+
+    def test_errors_on_not_unique(self):
+        key = factory.make_license_key()
+        new_key = factory.make_name('key')
+        self.assertRaises(
+            IntegrityError,
+            LicenseKey.objects.create, osystem=key.osystem,
+            distro_series=key.distro_series, license_key=new_key)

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2014-07-02 07:05:24 +0000
+++ src/maasserver/tests/test_forms.py	2014-07-02 14:55:33 +0000
@@ -56,6 +56,7 @@
     initialize_node_group,
     InstanceListField,
     INTERFACES_VALIDATION_ERROR_MESSAGE,
+    LicenseKeyForm,
     list_all_usable_architectures,
     list_all_usable_osystems,
     list_all_usable_releases,
@@ -83,6 +84,7 @@
     )
 from maasserver.models import (
     Config,
+    LicenseKey,
     MACAddress,
     Network,
     Node,
@@ -120,6 +122,7 @@
     IPRange,
     )
 from provisioningserver import tasks
+from provisioningserver.drivers.osystem import OperatingSystemRegistry
 from provisioningserver.drivers.osystem.ubuntu import UbuntuOS
 from testtools import TestCase
 from testtools.matchers import (
@@ -2253,3 +2256,188 @@
             }
         form = DeployForm(data=params)
         self.assertTrue(form.is_valid())
+
+
+class TestLicenseKeyForm(MAASServerTestCase):
+    """Tests for `LicenseKeyForm`."""
+
+    def make_os_with_license_key(self, osystem=None, has_key=True,
+                                 key_is_valid=True, patch_registry=True):
+        """Makes a fake operating system that requires a license key, and
+        validates to key_is_valid."""
+        if osystem is None:
+            osystem = make_usable_osystem(self)
+        self.patch(osystem, 'requires_license_key').return_value = has_key
+        self.patch(osystem, 'validate_license_key').return_value = key_is_valid
+        if patch_registry:
+            self.patch(
+                OperatingSystemRegistry, 'get_item').return_value = osystem
+        return osystem
+
+    def make_all_osystems_require_key(self, key_is_valid=True):
+        """Patches all operating systems in the registry, to require a key."""
+        for _, osystem in OperatingSystemRegistry:
+            self.patch(
+                osystem, 'requires_license_key').return_value = True
+            self.patch(
+                osystem, 'validate_license_key').return_value = key_is_valid
+
+    def make_only_one_osystem_require_key(self, key_is_valid=True):
+        """Patches a random operating system from the registry, to require a
+        key. Patches the remaining operating systems to not require a key."""
+        osystem = factory.getRandomOS()
+        self.patch(
+            osystem, 'requires_license_key').return_value = True
+        self.patch(
+            osystem, 'validate_license_key').return_value = key_is_valid
+        for _, other_osystem in OperatingSystemRegistry:
+            if osystem == other_osystem:
+                continue
+            self.patch(
+                other_osystem, 'requires_license_key').return_value = False
+        return osystem
+
+    def test_creates_license_key(self):
+        osystem = self.make_os_with_license_key()
+        series = factory.getRandomRelease(osystem)
+        key = factory.make_name('key')
+        definition = {
+            'osystem': osystem.name,
+            'distro_series': series,
+            'license_key': key,
+            }
+        data = definition.copy()
+        data['distro_series'] = '%s/%s' % (osystem.name, series)
+        form = LicenseKeyForm(data=data)
+        form.save()
+        license_key_obj = LicenseKey.objects.get(
+            osystem=osystem.name, distro_series=series)
+        self.assertAttributes(license_key_obj, definition)
+
+    def test_updates_license_key(self):
+        osystem = self.make_os_with_license_key()
+        series = factory.getRandomRelease(osystem)
+        license_key = factory.make_license_key(
+            osystem=osystem.name, distro_series=series,
+            license_key=factory.make_name('key'))
+        new_key = factory.make_name('key')
+        form = LicenseKeyForm(
+            data={'license_key': new_key}, instance=license_key)
+        form.save()
+        license_key = reload_object(license_key)
+        self.assertEqual(new_key, license_key.license_key)
+
+    def test_validates_license_key(self):
+        osystem = self.make_os_with_license_key(key_is_valid=False)
+        series = factory.getRandomRelease(osystem)
+        license_key = factory.make_license_key(
+            osystem=osystem.name, distro_series=series,
+            license_key=factory.make_name('key'))
+        new_key = factory.make_name('key')
+        form = LicenseKeyForm(
+            data={'license_key': new_key}, instance=license_key)
+        self.assertFalse(form.is_valid(), form.errors)
+        self.assertEqual(
+            {'__all__': ['Invalid license key.']},
+            form.errors)
+
+    def test_handles_missing_osystem_in_distro_series(self):
+        osystem = self.make_os_with_license_key()
+        series = factory.getRandomRelease(osystem)
+        key = factory.make_name('key')
+        definition = {
+            'osystem': osystem.name,
+            'distro_series': series,
+            'license_key': key,
+            }
+        form = LicenseKeyForm(data=definition.copy())
+        form.save()
+        license_key_obj = LicenseKey.objects.get(
+            osystem=osystem.name, distro_series=series)
+        self.assertAttributes(license_key_obj, definition)
+
+    def test_requires_all_fields(self):
+        form = LicenseKeyForm(data={})
+        self.assertFalse(form.is_valid(), form.errors)
+        self.assertItemsEqual(
+            ['osystem', 'distro_series', 'license_key'],
+            form.errors.keys())
+
+    def test_errors_on_not_unique(self):
+        osystem = self.make_os_with_license_key()
+        series = factory.getRandomRelease(osystem)
+        key = factory.make_name('key')
+        factory.make_license_key(
+            osystem=osystem.name, distro_series=series, license_key=key)
+        definition = {
+            'osystem': osystem.name,
+            'distro_series': series,
+            'license_key': key,
+            }
+        form = LicenseKeyForm(data=definition)
+        self.assertFalse(form.is_valid(), form.errors)
+        self.assertEqual({
+            '__all__': ['%s %s' % (
+                "License key with this operating system and distro series",
+                "already exists.")]},
+            form.errors)
+
+    def test_doesnt_include_default_osystem(self):
+        form = LicenseKeyForm()
+        self.assertNotIn(('', 'Default OS'), form.fields['osystem'].choices)
+
+    def test_includes_all_osystems(self):
+        self.make_all_osystems_require_key()
+        osystems = [osystem for _, osystem in OperatingSystemRegistry]
+        expected = [
+            (osystem.name, osystem.title)
+            for osystem in osystems
+            ]
+        form = LicenseKeyForm()
+        self.assertItemsEqual(expected, form.fields['osystem'].choices)
+
+    def test_includes_all_osystems_sorted(self):
+        self.make_all_osystems_require_key()
+        osystems = [osystem for _, osystem in OperatingSystemRegistry]
+        osystems = sorted(osystems, key=lambda osystem: osystem.title)
+        expected = [
+            (osystem.name, osystem.title)
+            for osystem in osystems
+            ]
+        form = LicenseKeyForm()
+        self.assertEqual(expected, form.fields['osystem'].choices)
+
+    def test_includes_only_osystems_that_require_license_keys(self):
+        osystem = self.make_only_one_osystem_require_key()
+        expected = [(osystem.name, osystem.title)]
+        form = LicenseKeyForm()
+        self.assertEquals(expected, form.fields['osystem'].choices)
+
+    def test_doesnt_include_default_distro_series(self):
+        form = LicenseKeyForm()
+        self.assertNotIn(
+            ('', 'Default OS Release'), form.fields['distro_series'].choices)
+
+    def test_includes_all_distro_series(self):
+        self.make_all_osystems_require_key()
+        osystems = [osystem for _, osystem in OperatingSystemRegistry]
+        expected = []
+        for osystem in osystems:
+            releases = osystem.get_supported_releases()
+            for name, title in osystem.format_release_choices(releases):
+                expected.append((
+                    '%s/%s' % (osystem.name, name),
+                    title
+                    ))
+        form = LicenseKeyForm()
+        self.assertItemsEqual(expected, form.fields['distro_series'].choices)
+
+    def test_includes_only_distro_series_that_require_license_keys(self):
+        osystem = self.make_only_one_osystem_require_key()
+        releases = osystem.get_supported_releases()
+        expected = [
+            ('%s/%s' % (osystem.name, name), title)
+            for name, title in osystem.format_release_choices(releases)
+            ]
+        form = LicenseKeyForm()
+        self.assertEquals(expected, form.fields['distro_series'].choices)

=== modified file 'src/maasserver/utils/osystems.py'
--- src/maasserver/utils/osystems.py	2014-06-16 14:25:56 +0000
+++ src/maasserver/utils/osystems.py	2014-07-02 14:55:33 +0000
@@ -14,6 +14,7 @@
 __all__ = [
     'get_distro_series_inital',
     'get_release_requires_key',
+    'list_all_releases_requiring_keys',
     'list_all_usable_osystems',
     'list_all_usable_releases',
     'list_osystem_choices',
@@ -28,31 +29,42 @@
 from provisioningserver.drivers.osystem import OperatingSystemRegistry
 
 
-def list_all_usable_osystems():
+def list_all_usable_osystems(have_images=True):
     """Return all operating systems that can be used for nodes.
 
-    These are the operating systems for which any nodegroup has the boot images
-    required to boot the node.
+    :param have_images: If set to true then its only the operating systems for
+    which any nodegroup has the boot images available for that operating
+    system.
     """
-    # The Node edit form offers all usable operating systems as options for the
-    # osystem field.  Not all of these may be available in the node's
-    # nodegroup, but to represent that accurately in the UI would depend on
-    # the currently selected nodegroup.  Narrowing the options down further
-    # would have to happen browser-side.
-    osystems = set()
-    for nodegroup in NodeGroup.objects.all():
-        osystems = osystems.union(
-            BootImage.objects.get_usable_osystems(nodegroup))
-    osystems = [
-        OperatingSystemRegistry[osystem] for osystem in osystems
-        if osystem in OperatingSystemRegistry
-        ]
+    if not have_images:
+        osystems = set([osystem for _, osystem in OperatingSystemRegistry])
+    else:
+        # The Node edit form offers all usable operating systems as options for
+        # the osystem field.  Not all of these may be available in the node's
+        # nodegroup, but to represent that accurately in the UI would depend on
+        # the currently selected nodegroup.  Narrowing the options down further
+        # would have to happen browser-side.
+        osystems = set()
+        for nodegroup in NodeGroup.objects.all():
+            osystems = osystems.union(
+                BootImage.objects.get_usable_osystems(nodegroup))
+        osystems = [
+            OperatingSystemRegistry[osystem] for osystem in osystems
+            if osystem in OperatingSystemRegistry
+            ]
     return sorted(osystems, key=lambda osystem: osystem.title)
 
 
-def list_osystem_choices(osystems):
-    """Return Django "choices" list for `osystem`."""
-    choices = [('', 'Default OS')]
+def list_osystem_choices(osystems, include_default=True):
+    """Return Django "choices" list for `osystem`.
+
+    :param include_default: When true includes the 'Default OS' in choice
+        selection.
+    """
+    if include_default:
+        choices = [('', 'Default OS')]
+    else:
+        choices = []
     choices += [
         (osystem.name, osystem.title)
         for osystem in osystems
@@ -60,19 +72,42 @@
     return choices
 
 
-def list_all_usable_releases(osystems):
-    """Return dictionary of usable `releases` for each opertaing system."""
+def list_all_usable_releases(osystems, have_images=True):
+    """Return dictionary of usable `releases` for each operating system.
+
+    :param have_images: If set to true then its only the releases for
+    which any nodegroup has the boot images available for that release.
+    """
     distro_series = {}
     nodegroups = list(NodeGroup.objects.all())
     for osystem in osystems:
         releases = set()
-        for nodegroup in nodegroups:
-            releases = releases.union(
-                BootImage.objects.get_usable_releases(nodegroup, osystem.name))
+        if not have_images:
+            releases = releases.union(osystem.get_supported_releases())
+        else:
+            for nodegroup in nodegroups:
+                releases = releases.union(
+                    BootImage.objects.get_usable_releases(
+                        nodegroup, osystem.name))
         distro_series[osystem.name] = sorted(releases)
     return distro_series
 
 
+def list_all_releases_requiring_keys(osystems):
+    """Return dictionary of OS name mapping to `releases` that require
+    license keys."""
+    distro_series = {}
+    for osystem in osystems:
+        releases = [
+            release
+            for release in osystem.get_supported_releases()
+            if osystem.requires_license_key(release)
+            ]
+        if len(releases) > 0:
+            distro_series[osystem.name] = sorted(releases)
+    return distro_series
+
+
 def get_release_requires_key(osystem, release):
     """Return astrisk for any release that requires
     a license key.
@@ -84,39 +119,58 @@
     return ''
 
 
-def list_release_choices(releases):
-    """Return Django "choices" list for `releases`."""
-    choices = [('', 'Default OS Release')]
+def list_release_choices(releases, include_default=True, include_latest=True,
+                         with_key_required=True):
+    """Return Django "choices" list for `releases`.
+
+    :param include_default: When true includes the 'Default OS Release' in
+        choice selection.
+    :param include_latest: When true includes the 'Latest OS Release' in
+        choice selection.
+    :param with_key_required: When true includes the release_requires_key in
+        the choice.
+    """
+    if include_default:
+        choices = [('', 'Default OS Release')]
+    else:
+        choices = []
     for key, value in releases.items():
         osystem = OperatingSystemRegistry[key]
         options = osystem.format_release_choices(value)
-        requires_key = get_release_requires_key(osystem, '')
-        choices += [(
-            '%s/%s' % (osystem.name, requires_key),
-            'Latest %s Release' % osystem.title
-            )]
-        choices += [(
-            '%s/%s%s' % (
-                osystem.name,
-                name,
-                get_release_requires_key(osystem, name)
-                ),
-            title
-            )
-            for name, title in options
-            ]
+        if with_key_required:
+            requires_key = get_release_requires_key(osystem, '')
+        else:
+            requires_key = ''
+        if include_latest:
+            choices.append((
+                '%s/%s' % (osystem.name, requires_key),
+                'Latest %s Release' % osystem.title
+                ))
+        for name, title in options:
+            if with_key_required:
+                requires_key = get_release_requires_key(osystem, name)
+            else:
+                requires_key = ''
+            choices.append((
+                '%s/%s%s' % (osystem.name, name, requires_key),
+                title
+                ))
     return choices
 
 
-def get_distro_series_inital(instance):
-    """Returns the distro_series initial value for the instance."""
+def get_distro_series_inital(instance, with_key_required=True):
+    """Returns the distro_series initial value for the instance.
+
+    :param with_key_required: When true includes the release_requires_key in
+        the choice.
+    """
     osystem = instance.osystem
     series = instance.distro_series
     os_obj = OperatingSystemRegistry.get_item(osystem)
-    if os_obj is not None:
+    if not with_key_required:
+        key_required = ''
+    elif os_obj is not None:
         key_required = get_release_requires_key(os_obj, series)
-    else:
-        key_required = ''
     if osystem is not None and osystem != '':
         if series is None:
             series = ''