Security Group Rule enhancements.

  * Corrects inconcsistent capitalization. Fixes bug 956760.
  * Adds a default value for CIDR and updates handling code. Fixes bug 956771.
  * Corrects the way SelfHandlingForm access the cleaned data. Fixes bug 958971.

Change-Id: I66afeb4b530be350f33f63c8f9a60bd4a20e01bf

horizon/dashboards/nova/access_and_security/security_groups/forms.py

@@ -40,7 +40,6 @@
 class CreateGroup(forms.SelfHandlingForm):
     name = forms.CharField(validators=[validators.validate_slug])
     description = forms.CharField()
-    tenant_id = forms.CharField(widget=forms.HiddenInput())
 
     def handle(self, request, data):
         try:
@@ -56,13 +55,13 @@ def handle(self, request, data):
 
 
 class AddRule(forms.SelfHandlingForm):
-    ip_protocol = forms.ChoiceField(label=_('IP protocol'),
-                                    choices=[('tcp', 'tcp'),
-                                             ('udp', 'udp'),
-                                             ('icmp', 'icmp')],
+    ip_protocol = forms.ChoiceField(label=_('IP Protocol'),
+                                    choices=[('tcp', 'TCP'),
+                                             ('udp', 'UDP'),
+                                             ('icmp', 'ICMP')],
                                     widget=forms.Select(attrs={'class':
                                                                'switchable'}))
-    from_port = forms.IntegerField(label=_("From port"),
+    from_port = forms.IntegerField(label=_("From Port"),
                                    help_text=_("TCP/UDP: Enter integer value "
                                                "between 1 and 65535. ICMP: "
                                                "enter a value for ICMP type "
@@ -71,7 +70,7 @@ class AddRule(forms.SelfHandlingForm):
                                           attrs={'data': _('From port'),
                                                  'data-icmp': _('Type')}),
                                    validators=[validate_port_range])
-    to_port = forms.IntegerField(label=_("To port"),
+    to_port = forms.IntegerField(label=_("To Port"),
                                  help_text=_("TCP/UDP: Enter integer value "
                                              "between 1 and 65535. ICMP: "
                                              "enter a value for ICMP code "
@@ -82,13 +81,14 @@ class AddRule(forms.SelfHandlingForm):
                                  validators=[validate_port_range])
 
     source_group = forms.ChoiceField(label=_('Source Group'), required=False)
-    cidr = forms.CharField(label=_("CIDR"), required=False,
+    cidr = forms.CharField(label=_("CIDR"),
+                           required=False,
+                           initial="0.0.0.0/0",
                            help_text=_("Classless Inter-Domain Routing "
                                        "(i.e. 192.168.0.0/24"),
                            validators=[validate_ipv4_cidr])
 
     security_group_id = forms.IntegerField(widget=forms.HiddenInput())
-    tenant_id = forms.CharField(widget=forms.HiddenInput())
 
     def __init__(self, *args, **kwargs):
         super(AddRule, self).__init__(*args, **kwargs)
@@ -120,11 +120,16 @@ def clean(self):
                     'the "from" port number.')
             raise ValidationError(msg)
 
-        if cidr and source_group:
-            msg = _('Only either "CIDR" or "Source Group" may be specified')
+        if source_group and cidr != self.fields['cidr'].initial:
+            # Specifying a source group *and* a custom CIDR is invalid.
+            msg = _('Either CIDR or Source Group may be specified, '
+                    'but not both.')
             raise ValidationError(msg)
-        if cidr:
-            # if only cidr is specified, make sure source_group is cleaned
+        elif source_group:
+            # If a source group is specified, clear the CIDR from its default
+            cleaned_data['cidr'] = None
+        else:
+            # If only cidr is specified, clear the source_group entirely
             cleaned_data['source_group'] = None
 
         return cleaned_data

horizon/dashboards/nova/access_and_security/security_groups/tests.py

@@ -59,7 +59,6 @@ def test_create_security_groups_post(self):
         self.mox.ReplayAll()
 
         formData = {'method': 'CreateGroup',
-                    'tenant_id': self.tenant.id,
                     'name': sec_group.name,
                     'description': sec_group.description}
         res = self.client.post(SG_CREATE_URL, formData)
@@ -75,7 +74,6 @@ def test_create_security_groups_post_exception(self):
         self.mox.ReplayAll()
 
         formData = {'method': 'CreateGroup',
-                    'tenant_id': self.tenant.id,
                     'name': sec_group.name,
                     'description': sec_group.description}
         res = self.client.post(SG_CREATE_URL, formData)
@@ -137,7 +135,6 @@ def test_edit_rules_add_rule_cidr(self):
         self.mox.ReplayAll()
 
         formData = {'method': 'AddRule',
-                    'tenant_id': self.tenant.id,
                     'security_group_id': sec_group.id,
                     'from_port': rule.from_port,
                     'to_port': rule.to_port,
@@ -147,6 +144,32 @@ def test_edit_rules_add_rule_cidr(self):
         res = self.client.post(self.edit_url, formData)
         self.assertRedirectsNoFollow(res, INDEX_URL)
 
+    def test_edit_rules_add_rule_cidr_and_source_group(self):
+        sec_group = self.security_groups.first()
+        sec_group_other = self.security_groups.get(id=2)
+        sec_group_list = self.security_groups.list()
+        rule = self.security_group_rules.first()
+
+        self.mox.StubOutWithMock(api, 'security_group_get')
+        self.mox.StubOutWithMock(api, 'security_group_list')
+        api.security_group_get(IsA(http.HttpRequest),
+                               sec_group.id).AndReturn(sec_group)
+        api.security_group_list(
+                        IsA(http.HttpRequest)).AndReturn(sec_group_list)
+        self.mox.ReplayAll()
+
+        formData = {'method': 'AddRule',
+                    'security_group_id': sec_group.id,
+                    'from_port': rule.from_port,
+                    'to_port': rule.to_port,
+                    'ip_protocol': rule.ip_protocol,
+                    'cidr': "127.0.0.1/32",
+                    'source_group': sec_group_other.id}
+        res = self.client.post(self.edit_url, formData)
+        self.assertNoMessages()
+        msg = 'Either CIDR or Source Group may be specified, but not both.'
+        self.assertFormErrors(res, count=1, message=msg)
+
     def test_edit_rules_invalid_port_range(self):
         sec_group = self.security_groups.first()
         sec_group_list = self.security_groups.list()
@@ -161,7 +184,6 @@ def test_edit_rules_invalid_port_range(self):
         self.mox.ReplayAll()
 
         formData = {'method': 'AddRule',
-                    'tenant_id': self.tenant.id,
                     'security_group_id': sec_group.id,
                     'from_port': rule.from_port,
                     'to_port': int(rule.from_port) - 1,
@@ -192,7 +214,6 @@ def test_edit_rules_add_rule_exception(self):
         self.mox.ReplayAll()
 
         formData = {'method': 'AddRule',
-                    'tenant_id': self.tenant.id,
                     'security_group_id': sec_group.id,
                     'from_port': rule.from_port,
                     'to_port': rule.to_port,

horizon/dashboards/nova/access_and_security/security_groups/views.py

@@ -56,12 +56,16 @@ def get_data(self):
         return rules
 
     def handle_form(self):
-        tenant_id = self.request.user.tenant_id
-        security_groups = [(group.id, group.name)
-                            for group in api.security_group_list(self.request)]
+        try:
+            groups = api.security_group_list(self.request)
+        except:
+            groups = []
+            exceptions.handle(self.request,
+                              _("Unable to retrieve security groups."))
+
+        security_groups = [(group.id, group.name) for group in groups]
 
-        initial = {'tenant_id': tenant_id,
-                   'security_group_id': self.kwargs['security_group_id'],
+        initial = {'security_group_id': self.kwargs['security_group_id'],
                    'security_group_list': security_groups}
         return AddRule.maybe_handle(self.request, initial=initial)
 

horizon/forms/base.py

@@ -84,10 +84,8 @@ def maybe_handle(cls, request, *args, **kwargs):
         if not form.is_valid():
             return form, None
 
-        data = form.clean()
-
         try:
-            return form, form.handle(request, data)
+            return form, form.handle(request, form.cleaned_data)
         except:
             exceptions.handle(request)
             return form, None

horizon/test.py

@@ -199,7 +199,8 @@ def assertNoFormErrors(self, response, context_name="form"):
         assert len(errors) == 0, \
                "Unexpected errors were found on the form: %s" % errors
 
-    def assertFormErrors(self, response, count=0, context_name="form"):
+    def assertFormErrors(self, response, count=0, message=None,
+                         context_name="form"):
         """
         Asserts that the response does contain a form in it's
         context, and that form has errors, if count were given,
@@ -213,6 +214,10 @@ def assertFormErrors(self, response, count=0, context_name="form"):
             assert len(errors) == count, \
                "%d errors were found on the form, %d expected" % \
                (len(errors), count)
+            if message and message not in unicode(errors):
+                self.fail("Expected message not found, instead found: %s"
+                          % ["%s: %s" % (key, [e for e in field_errors]) for
+                             (key, field_errors) in errors.items()])
         else:
             assert len(errors) > 0, "No errors were found on the form"