diff --git a/glance/api/property_protections.py b/glance/api/property_protections.py index 9e6ca05392..ddcdca1e38 100644 --- a/glance/api/property_protections.py +++ b/glance/api/property_protections.py @@ -102,7 +102,7 @@ class ExtraPropertiesProxy(glance.domain.ExtraProperties): # A user cannot request to read a specific property, hence reads do # raise an exception try: - if self.__getitem__(key): + if self.__getitem__(key) is not None: if self.property_rules.check_property_rules(key, 'update', self.context): return dict.__setitem__(self, key, value) @@ -116,7 +116,7 @@ class ExtraPropertiesProxy(glance.domain.ExtraProperties): raise exception.ReservedProperty(property=key) def __delitem__(self, key): - if not super(ExtraPropertiesProxy, self).__getitem__(key): + if key not in super(ExtraPropertiesProxy, self).keys(): raise KeyError if self.property_rules.check_property_rules(key, 'delete', diff --git a/glance/tests/etc/property-protections.conf b/glance/tests/etc/property-protections.conf index 97ef1f642d..b18ded5466 100644 --- a/glance/tests/etc/property-protections.conf +++ b/glance/tests/etc/property-protections.conf @@ -40,6 +40,12 @@ read = admin,spl_role update = admin delete = admin,spl_role +[spl_delete_empty_prop] +create = admin,spl_role +read = admin,spl_role +update = admin +delete = admin,spl_role + [^x_all_permitted.*] create = @ read = @ diff --git a/glance/tests/functional/v2/test_images.py b/glance/tests/functional/v2/test_images.py index efd067abc8..dcde476542 100644 --- a/glance/tests/functional/v2/test_images.py +++ b/glance/tests/functional/v2/test_images.py @@ -694,7 +694,8 @@ class TestImages(functional.FunctionalTest): 'spl_create_prop_policy': 'create_policy_bar', 'spl_read_prop': 'read_bar', 'spl_update_prop': 'update_bar', - 'spl_delete_prop': 'delete_bar'}) + 'spl_delete_prop': 'delete_bar', + 'spl_delete_empty_prop': ''}) response = requests.post(path, headers=headers, data=data) self.assertEqual(201, response.status_code) image = jsonutils.loads(response.text) @@ -725,14 +726,15 @@ class TestImages(functional.FunctionalTest): response = requests.patch(path, headers=headers, data=data) self.assertEqual(403, response.status_code, response.text) - # Attempt to replace, add and remove properties + # Attempt to replace properties path = self._url('/v2/images/%s' % image_id) media_type = 'application/openstack-images-v2.1-json-patch' headers = self._headers({'content-type': media_type, 'X-Roles': 'spl_role'}) data = jsonutils.dumps([ + # Updating an empty property to verify bug #1332103. + {'op': 'replace', 'path': '/spl_update_prop', 'value': ''}, {'op': 'replace', 'path': '/spl_update_prop', 'value': 'u'}, - {'op': 'remove', 'path': '/spl_delete_prop'}, ]) response = requests.patch(path, headers=headers, data=data) self.assertEqual(200, response.status_code, response.text) @@ -744,9 +746,26 @@ class TestImages(functional.FunctionalTest): # hence the value has changed self.assertEqual('u', image['spl_update_prop']) - # 'spl_delete_prop' has delete permission for spl_role - # hence the property has been deleted - self.assertTrue('spl_delete_prop' not in image.keys()) + # Attempt to remove properties + path = self._url('/v2/images/%s' % image_id) + media_type = 'application/openstack-images-v2.1-json-patch' + headers = self._headers({'content-type': media_type, + 'X-Roles': 'spl_role'}) + data = jsonutils.dumps([ + {'op': 'remove', 'path': '/spl_delete_prop'}, + # Deleting an empty property to verify bug #1332103. + {'op': 'remove', 'path': '/spl_delete_empty_prop'}, + ]) + response = requests.patch(path, headers=headers, data=data) + self.assertEqual(200, response.status_code, response.text) + + # Returned image entity should reflect the changes + image = jsonutils.loads(response.text) + + # 'spl_delete_prop' and 'spl_delete_empty_prop' have delete + # permission for spl_role hence the property has been deleted + self.assertNotIn('spl_delete_prop', image.keys()) + self.assertNotIn('spl_delete_empty_prop', image.keys()) # Image Deletion should work path = self._url('/v2/images/%s' % image_id) @@ -834,6 +853,8 @@ class TestImages(functional.FunctionalTest): headers = self._headers({'content-type': media_type, 'X-Roles': 'admin'}) data = jsonutils.dumps([ + # Updating an empty property to verify bug #1332103. + {'op': 'replace', 'path': '/spl_creator_policy', 'value': ''}, {'op': 'replace', 'path': '/spl_creator_policy', 'value': 'r'}, ]) response = requests.patch(path, headers=headers, data=data) @@ -870,12 +891,14 @@ class TestImages(functional.FunctionalTest): # 'random_role' is forbidden to read 'spl_creator_policy'. self.assertFalse('spl_creator_policy' in image) - # Attempt to add and remove properties which are permitted + # Attempt to replace and remove properties which are permitted path = self._url('/v2/images/%s' % image_id) media_type = 'application/openstack-images-v2.1-json-patch' headers = self._headers({'content-type': media_type, 'X-Roles': 'admin'}) data = jsonutils.dumps([ + # Deleting an empty property to verify bug #1332103. + {'op': 'replace', 'path': '/spl_creator_policy', 'value': ''}, {'op': 'remove', 'path': '/spl_creator_policy'}, ]) response = requests.patch(path, headers=headers, data=data) diff --git a/glance/tests/unit/api/test_property_protections.py b/glance/tests/unit/api/test_property_protections.py index 7f26cbf45d..9875f947e2 100644 --- a/glance/tests/unit/api/test_property_protections.py +++ b/glance/tests/unit/api/test_property_protections.py @@ -168,6 +168,14 @@ class TestExtraPropertiesProxy(utils.BaseTestCase): extra_prop_proxy.__setitem__, 'spl_create_prop', 'par') + def test_update_empty_extra_property(self): + extra_properties = {'foo': ''} + context = glance.context.RequestContext(roles=['admin']) + extra_prop_proxy = property_protections.ExtraPropertiesProxy( + context, extra_properties, self.property_rules) + extra_prop_proxy['foo'] = 'bar' + self.assertEqual('bar', extra_prop_proxy['foo']) + def test_create_extra_property_admin(self): extra_properties = {} context = glance.context.RequestContext(roles=['admin']) @@ -218,6 +226,14 @@ class TestExtraPropertiesProxy(utils.BaseTestCase): self.assertRaises(KeyError, extra_prop_proxy.__delitem__, 'spl_read_prop') + def test_delete_empty_extra_property(self): + extra_properties = {'foo': ''} + context = glance.context.RequestContext(roles=['admin']) + extra_prop_proxy = property_protections.ExtraPropertiesProxy( + context, extra_properties, self.property_rules) + del extra_prop_proxy['foo'] + self.assertNotIn('foo', extra_prop_proxy) + class TestProtectedImageFactoryProxy(utils.BaseTestCase): def setUp(self): diff --git a/glance/tests/unit/common/test_property_utils.py b/glance/tests/unit/common/test_property_utils.py index 8cb28aa6a1..216fcd1460 100644 --- a/glance/tests/unit/common/test_property_utils.py +++ b/glance/tests/unit/common/test_property_utils.py @@ -29,6 +29,7 @@ CONFIG_SECTIONS = [ 'spl_update_prop', 'spl_update_only_prop', 'spl_delete_prop', + 'spl_delete_empty_prop', '^x_all_permitted.*', '^x_none_permitted.*', 'x_none_read',