From 80d9049fb6e6c7bf5eb262612e6ad3e8a541c051 Mon Sep 17 00:00:00 2001 From: Romain Hardouin Date: Thu, 3 Apr 2014 23:52:13 +0200 Subject: [PATCH] Allow all printable ASCII characters in security group names Thus far, the security group name was checked with Django's validate_slug. Consequently, some characters like spaces (bug #1224576) and @ (bug #1233501) were forbidden. This fix removes validate_slug and provides a less restrictive validation. Now, all printable ASCII characters -- i.e. from 0x20 to 0x7E -- are valid. Note that diacritics are disallowed. On top of that the length is now checked and limited to 255 characters in accordance with Nova. Security group forms have been factorized in one abstract base class: GroupBase. CreateGroup and UpdateGroup subclass GroupBase in order to be more DRY. Change-Id: Ifc8e5f75419a73a353b2138641773d36138ecea8 Closes-Bug: #1224576 Closes-Bug: #1233501 --- horizon/utils/validators.py | 11 ++ .../security_groups/forms.py | 91 +++++----- .../security_groups/tests.py | 157 ++++++++++-------- 3 files changed, 145 insertions(+), 114 deletions(-) diff --git a/horizon/utils/validators.py b/horizon/utils/validators.py index 27d7f1d78a..d3baa96ef1 100644 --- a/horizon/utils/validators.py +++ b/horizon/utils/validators.py @@ -12,7 +12,10 @@ # License for the specific language governing permissions and limitations # under the License. +import re + from django.core.exceptions import ValidationError # noqa +from django.core import validators # noqa from django.utils.translation import ugettext_lazy as _ from horizon import conf @@ -47,3 +50,11 @@ def validate_port_or_colon_separated_port_range(port_range): raise ValidationError(_("Not a valid port number")) except ValueError: raise ValidationError(_("Port number must be integer")) + +# Same as POSIX [:print:]. Accordingly, diacritics are disallowed. +PRINT_REGEX = re.compile(r'^[\x20-\x7E]*$') + +validate_printable_ascii = validators.RegexValidator( + PRINT_REGEX, + _("The string may only contain ASCII printable characters."), + "invalid_characters") diff --git a/openstack_dashboard/dashboards/project/access_and_security/security_groups/forms.py b/openstack_dashboard/dashboards/project/access_and_security/security_groups/forms.py index 08e80ce90c..0e39e33d9b 100644 --- a/openstack_dashboard/dashboards/project/access_and_security/security_groups/forms.py +++ b/openstack_dashboard/dashboards/project/access_and_security/security_groups/forms.py @@ -20,7 +20,6 @@ import netaddr from django.conf import settings from django.core.urlresolvers import reverse -from django.core import validators from django.forms import ValidationError # noqa from django.utils.translation import ugettext_lazy as _ @@ -33,62 +32,68 @@ from openstack_dashboard import api from openstack_dashboard.utils import filters -class CreateGroup(forms.SelfHandlingForm): +class GroupBase(forms.SelfHandlingForm): + """Base class to handle creation and update of security groups. + + Children classes must define two attributes: + + .. attribute:: success_message + + A success message containing the placeholder %s, + which will be replaced by the group name. + + .. attribute:: error_message + + An error message containing the placeholder %s, + which will be replaced by the error message. + """ name = forms.CharField(label=_("Name"), max_length=255, - error_messages={ - 'required': _('This field is required.'), - 'invalid': _("The string may only contain" - " ASCII characters and numbers.")}, - validators=[validators.validate_slug]) + validators=[ + utils_validators.validate_printable_ascii]) description = forms.CharField(label=_("Description"), required=False, widget=forms.Textarea(attrs={'rows': 4})) + def _call_network_api(self, request, data): + """Call the underlying network API: Nova-network or Neutron. + + Used in children classes to create or update a group. + """ + raise NotImplementedError() + def handle(self, request, data): try: - sg = api.network.security_group_create(request, - data['name'], - data['description']) - messages.success(request, - _('Successfully created security group: %s') - % data['name']) + sg = self._call_network_api(request, data) + messages.success(request, self.success_message % sg.name) return sg - except Exception: + except Exception as e: redirect = reverse("horizon:project:access_and_security:index") - exceptions.handle(request, - _('Unable to create security group.'), - redirect=redirect) + error_msg = self.error_message % e + exceptions.handle(request, error_msg, redirect=redirect) -class UpdateGroup(forms.SelfHandlingForm): +class CreateGroup(GroupBase): + success_message = _('Successfully created security group: %s') + error_message = _('Unable to create security group: %s') + + def _call_network_api(self, request, data): + return api.network.security_group_create(request, + data['name'], + data['description']) + + +class UpdateGroup(GroupBase): + success_message = _('Successfully updated security group: %s') + error_message = _('Unable to update security group: %s') + id = forms.CharField(widget=forms.HiddenInput()) - name = forms.CharField(label=_("Name"), - max_length=255, - error_messages={ - 'required': _('This field is required.'), - 'invalid': _("The string may only contain" - " ASCII characters and numbers.")}, - validators=[validators.validate_slug]) - description = forms.CharField(label=_("Description"), - required=False, - widget=forms.Textarea(attrs={'rows': 4})) - def handle(self, request, data): - try: - sg = api.network.security_group_update(request, - data['id'], - data['name'], - data['description']) - messages.success(request, - _('Successfully updated security group: %s') - % data['name']) - return sg - except Exception: - redirect = reverse("horizon:project:access_and_security:index") - exceptions.handle(request, - _('Unable to update security group.'), - redirect=redirect) + def _call_network_api(self, request, data): + return api.network.security_group_update(request, + data['id'], + data['name'], + data['description']) class AddRule(forms.SelfHandlingForm): diff --git a/openstack_dashboard/dashboards/project/access_and_security/security_groups/tests.py b/openstack_dashboard/dashboards/project/access_and_security/security_groups/tests.py index 7713668395..395dbd2a9b 100644 --- a/openstack_dashboard/dashboards/project/access_and_security/security_groups/tests.py +++ b/openstack_dashboard/dashboards/project/access_and_security/security_groups/tests.py @@ -35,6 +35,16 @@ INDEX_URL = reverse('horizon:project:access_and_security:index') SG_CREATE_URL = reverse('horizon:project:access_and_security:' 'security_groups:create') +SG_VIEW_PATH = 'horizon:project:access_and_security:security_groups:%s' +SG_DETAIL_VIEW = SG_VIEW_PATH % 'detail' +SG_UPDATE_VIEW = SG_VIEW_PATH % 'update' +SG_ADD_RULE_VIEW = SG_VIEW_PATH % 'add_rule' + +SG_TEMPLATE_PATH = 'project/access_and_security/security_groups/%s' +SG_DETAIL_TEMPLATE = SG_TEMPLATE_PATH % 'detail.html' +SG_CREATE_TEMPLATE = SG_TEMPLATE_PATH % 'create.html' +SG_UPDATE_TEMPLATE = SG_TEMPLATE_PATH % '_update.html' + def strip_absolute_base(uri): return uri.split(settings.TESTSERVER, 1)[-1] @@ -46,12 +56,9 @@ class SecurityGroupsViewTests(test.TestCase): def setUp(self): super(SecurityGroupsViewTests, self).setUp() sec_group = self.security_groups.first() - self.detail_url = reverse('horizon:project:access_and_security:' - 'security_groups:detail', - args=[sec_group.id]) - self.edit_url = reverse('horizon:project:access_and_security:' - 'security_groups:add_rule', - args=[sec_group.id]) + self.detail_url = reverse(SG_DETAIL_VIEW, args=[sec_group.id]) + self.edit_url = reverse(SG_ADD_RULE_VIEW, args=[sec_group.id]) + self.update_url = reverse(SG_UPDATE_VIEW, args=[sec_group.id]) @test.create_stubs({api.network: ('security_group_rule_create', 'security_group_list', @@ -83,66 +90,77 @@ class SecurityGroupsViewTests(test.TestCase): api.network.security_group_get(IsA(http.HttpRequest), sec_group.id).AndReturn(sec_group) self.mox.ReplayAll() - - res = self.client.get(reverse('horizon:project:access_and_security:' - 'security_groups:update', - args=[sec_group.id])) - self.assertTemplateUsed( - res, 'project/access_and_security/security_groups/_update.html') + res = self.client.get(self.update_url) + self.assertTemplateUsed(res, SG_UPDATE_TEMPLATE) self.assertEqual(res.context['security_group'].name, sec_group.name) @test.create_stubs({api.network: ('security_group_update', 'security_group_get')}) def test_update_security_groups_post(self): - sec_group = self.security_groups.get(name="other_group") - api.network.security_group_update(IsA(http.HttpRequest), - str(sec_group.id), - sec_group.name, - sec_group.description) \ - .AndReturn(sec_group) - api.network.security_group_get(IsA(http.HttpRequest), - sec_group.id).AndReturn(sec_group) + """Ensure that we can change a group name. + + The name must not be restricted to alphanumeric characters. + bug #1233501 Security group names cannot contain at characters + bug #1224576 Security group names cannot contain spaces + """ + sec_group = self.security_groups.first() + sec_group.name = "@new name" + api.network.security_group_update( + IsA(http.HttpRequest), + str(sec_group.id), + sec_group.name, + sec_group.description).AndReturn(sec_group) + api.network.security_group_get( + IsA(http.HttpRequest), sec_group.id).AndReturn(sec_group) self.mox.ReplayAll() - - formData = {'method': 'UpdateGroup', - 'id': sec_group.id, - 'name': sec_group.name, - 'description': sec_group.description} - - update_url = reverse('horizon:project:access_and_security:' - 'security_groups:update', - args=[sec_group.id]) - res = self.client.post(update_url, formData) + form_data = {'method': 'UpdateGroup', + 'id': sec_group.id, + 'name': sec_group.name, + 'description': sec_group.description} + res = self.client.post(self.update_url, form_data) self.assertRedirectsNoFollow(res, INDEX_URL) def test_create_security_groups_get(self): res = self.client.get(SG_CREATE_URL) - self.assertTemplateUsed( - res, 'project/access_and_security/security_groups/create.html') + self.assertTemplateUsed(res, SG_CREATE_TEMPLATE) - @test.create_stubs({api.network: ('security_group_create',)}) def test_create_security_groups_post(self): sec_group = self.security_groups.first() - api.network.security_group_create(IsA(http.HttpRequest), - sec_group.name, - sec_group.description) \ - .AndReturn(sec_group) + self._create_security_group(sec_group) + + def test_create_security_groups_special_chars(self): + """Ensure that a group name is not restricted to alphanumeric + characters. + + bug #1233501 Security group names cannot contain at characters + bug #1224576 Security group names cannot contain spaces + """ + sec_group = self.security_groups.first() + sec_group.name = '@group name' + self._create_security_group(sec_group) + + @test.create_stubs({api.network: ('security_group_create',)}) + def _create_security_group(self, sec_group): + api.network.security_group_create( + IsA(http.HttpRequest), + sec_group.name, + sec_group.description).AndReturn(sec_group) self.mox.ReplayAll() - formData = {'method': 'CreateGroup', - 'name': sec_group.name, - 'description': sec_group.description} - res = self.client.post(SG_CREATE_URL, formData) + form_data = {'method': 'CreateGroup', + 'name': sec_group.name, + 'description': sec_group.description} + res = self.client.post(SG_CREATE_URL, form_data) self.assertRedirectsNoFollow(res, INDEX_URL) @test.create_stubs({api.network: ('security_group_create',)}) def test_create_security_groups_post_exception(self): sec_group = self.security_groups.first() - api.network.security_group_create(IsA(http.HttpRequest), - sec_group.name, - sec_group.description) \ - .AndRaise(self.exceptions.nova) + api.network.security_group_create( + IsA(http.HttpRequest), + sec_group.name, + sec_group.description).AndRaise(self.exceptions.nova) self.mox.ReplayAll() formData = {'method': 'CreateGroup', @@ -153,17 +171,22 @@ class SecurityGroupsViewTests(test.TestCase): self.assertRedirectsNoFollow(res, INDEX_URL) @test.create_stubs({api.network: ('security_group_create',)}) - def test_create_security_groups_post_wrong_name(self): + def test_create_security_groups_non_printable(self): + """Ensure that group names can only contain printable + ASCII characters. + + Only 95 characters are allowed: from 0x20 (space) to 0x7E (~). + """ sec_group = self.security_groups.first() - fail_name = sec_group.name + ' invalid' + # 0x7F is a control character (DELETE) + fail_name = sec_group.name + ' \x7F' self.mox.ReplayAll() - formData = {'method': 'CreateGroup', - 'name': fail_name, - 'description': sec_group.description} - res = self.client.post(SG_CREATE_URL, formData) - self.assertTemplateUsed( - res, 'project/access_and_security/security_groups/create.html') + form_data = {'method': 'CreateGroup', + 'name': fail_name, + 'description': sec_group.description} + res = self.client.post(SG_CREATE_URL, form_data) + self.assertTemplateUsed(res, SG_CREATE_TEMPLATE) self.assertContains(res, "ASCII") @test.create_stubs({api.network: ('security_group_get',)}) @@ -174,17 +197,15 @@ class SecurityGroupsViewTests(test.TestCase): sec_group.id).AndReturn(sec_group) self.mox.ReplayAll() res = self.client.get(self.detail_url) - self.assertTemplateUsed( - res, 'project/access_and_security/security_groups/detail.html') + self.assertTemplateUsed(res, SG_DETAIL_TEMPLATE) @test.create_stubs({api.network: ('security_group_get',)}) def test_detail_get_exception(self): sec_group = self.security_groups.first() - api.network.security_group_get(IsA(http.HttpRequest), - sec_group.id) \ - .AndRaise(self.exceptions.nova) - + api.network.security_group_get( + IsA(http.HttpRequest), + sec_group.id).AndRaise(self.exceptions.nova) self.mox.ReplayAll() res = self.client.get(self.detail_url) @@ -630,12 +651,9 @@ class SecurityGroupsNovaNeutronDriverTests(SecurityGroupsViewTests): self.security_group_rules = self.security_group_rules_uuid sec_group = self.security_groups.first() - self.detail_url = reverse('horizon:project:access_and_security:' - 'security_groups:detail', - args=[sec_group.id]) - self.edit_url = reverse('horizon:project:access_and_security:' - 'security_groups:add_rule', - args=[sec_group.id]) + self.detail_url = reverse(SG_DETAIL_VIEW, args=[sec_group.id]) + self.edit_url = reverse(SG_ADD_RULE_VIEW, args=[sec_group.id]) + self.update_url = reverse(SG_UPDATE_VIEW, args=[sec_group.id]) def tearDown(self): self.security_groups = self._sec_groups_orig @@ -656,12 +674,9 @@ class SecurityGroupsNeutronTests(SecurityGroupsViewTests): self.security_group_rules = self.q_secgroup_rules sec_group = self.security_groups.first() - self.detail_url = reverse('horizon:project:access_and_security:' - 'security_groups:detail', - args=[sec_group.id]) - self.edit_url = reverse('horizon:project:access_and_security:' - 'security_groups:add_rule', - args=[sec_group.id]) + self.detail_url = reverse(SG_DETAIL_VIEW, args=[sec_group.id]) + self.edit_url = reverse(SG_ADD_RULE_VIEW, args=[sec_group.id]) + self.update_url = reverse(SG_UPDATE_VIEW, args=[sec_group.id]) def tearDown(self): self.security_groups = self._sec_groups_orig