Fixes the failure of updating or deleting image empty property
When property protection feature enabled, end user couldn't uses Glance v2 API to update or delete a property of an image successfully which value is empty, client will receive a http 500 error and a relevant error log could be found in glance-api service log. Closes-bug: #1332103 Change-Id: I1f9f181cea714e6ba26388d125bb7023e7a14305 Signed-off-by: Zhi Yan Liu <zhiyanl@cn.ibm.com>
This commit is contained in:
parent
5148c9648f
commit
fe172d6114
|
@ -102,7 +102,7 @@ class ExtraPropertiesProxy(glance.domain.ExtraProperties):
|
||||||
# A user cannot request to read a specific property, hence reads do
|
# A user cannot request to read a specific property, hence reads do
|
||||||
# raise an exception
|
# raise an exception
|
||||||
try:
|
try:
|
||||||
if self.__getitem__(key):
|
if self.__getitem__(key) is not None:
|
||||||
if self.property_rules.check_property_rules(key, 'update',
|
if self.property_rules.check_property_rules(key, 'update',
|
||||||
self.context):
|
self.context):
|
||||||
return dict.__setitem__(self, key, value)
|
return dict.__setitem__(self, key, value)
|
||||||
|
@ -116,7 +116,7 @@ class ExtraPropertiesProxy(glance.domain.ExtraProperties):
|
||||||
raise exception.ReservedProperty(property=key)
|
raise exception.ReservedProperty(property=key)
|
||||||
|
|
||||||
def __delitem__(self, key):
|
def __delitem__(self, key):
|
||||||
if not super(ExtraPropertiesProxy, self).__getitem__(key):
|
if key not in super(ExtraPropertiesProxy, self).keys():
|
||||||
raise KeyError
|
raise KeyError
|
||||||
|
|
||||||
if self.property_rules.check_property_rules(key, 'delete',
|
if self.property_rules.check_property_rules(key, 'delete',
|
||||||
|
|
|
@ -40,6 +40,12 @@ read = admin,spl_role
|
||||||
update = admin
|
update = admin
|
||||||
delete = admin,spl_role
|
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.*]
|
[^x_all_permitted.*]
|
||||||
create = @
|
create = @
|
||||||
read = @
|
read = @
|
||||||
|
|
|
@ -694,7 +694,8 @@ class TestImages(functional.FunctionalTest):
|
||||||
'spl_create_prop_policy': 'create_policy_bar',
|
'spl_create_prop_policy': 'create_policy_bar',
|
||||||
'spl_read_prop': 'read_bar',
|
'spl_read_prop': 'read_bar',
|
||||||
'spl_update_prop': 'update_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)
|
response = requests.post(path, headers=headers, data=data)
|
||||||
self.assertEqual(201, response.status_code)
|
self.assertEqual(201, response.status_code)
|
||||||
image = jsonutils.loads(response.text)
|
image = jsonutils.loads(response.text)
|
||||||
|
@ -725,14 +726,15 @@ class TestImages(functional.FunctionalTest):
|
||||||
response = requests.patch(path, headers=headers, data=data)
|
response = requests.patch(path, headers=headers, data=data)
|
||||||
self.assertEqual(403, response.status_code, response.text)
|
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)
|
path = self._url('/v2/images/%s' % image_id)
|
||||||
media_type = 'application/openstack-images-v2.1-json-patch'
|
media_type = 'application/openstack-images-v2.1-json-patch'
|
||||||
headers = self._headers({'content-type': media_type,
|
headers = self._headers({'content-type': media_type,
|
||||||
'X-Roles': 'spl_role'})
|
'X-Roles': 'spl_role'})
|
||||||
data = jsonutils.dumps([
|
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': 'replace', 'path': '/spl_update_prop', 'value': 'u'},
|
||||||
{'op': 'remove', 'path': '/spl_delete_prop'},
|
|
||||||
])
|
])
|
||||||
response = requests.patch(path, headers=headers, data=data)
|
response = requests.patch(path, headers=headers, data=data)
|
||||||
self.assertEqual(200, response.status_code, response.text)
|
self.assertEqual(200, response.status_code, response.text)
|
||||||
|
@ -744,9 +746,26 @@ class TestImages(functional.FunctionalTest):
|
||||||
# hence the value has changed
|
# hence the value has changed
|
||||||
self.assertEqual('u', image['spl_update_prop'])
|
self.assertEqual('u', image['spl_update_prop'])
|
||||||
|
|
||||||
# 'spl_delete_prop' has delete permission for spl_role
|
# Attempt to remove properties
|
||||||
# hence the property has been deleted
|
path = self._url('/v2/images/%s' % image_id)
|
||||||
self.assertTrue('spl_delete_prop' not in image.keys())
|
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
|
# Image Deletion should work
|
||||||
path = self._url('/v2/images/%s' % image_id)
|
path = self._url('/v2/images/%s' % image_id)
|
||||||
|
@ -834,6 +853,8 @@ class TestImages(functional.FunctionalTest):
|
||||||
headers = self._headers({'content-type': media_type,
|
headers = self._headers({'content-type': media_type,
|
||||||
'X-Roles': 'admin'})
|
'X-Roles': 'admin'})
|
||||||
data = jsonutils.dumps([
|
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'},
|
{'op': 'replace', 'path': '/spl_creator_policy', 'value': 'r'},
|
||||||
])
|
])
|
||||||
response = requests.patch(path, headers=headers, data=data)
|
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'.
|
# 'random_role' is forbidden to read 'spl_creator_policy'.
|
||||||
self.assertFalse('spl_creator_policy' in image)
|
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)
|
path = self._url('/v2/images/%s' % image_id)
|
||||||
media_type = 'application/openstack-images-v2.1-json-patch'
|
media_type = 'application/openstack-images-v2.1-json-patch'
|
||||||
headers = self._headers({'content-type': media_type,
|
headers = self._headers({'content-type': media_type,
|
||||||
'X-Roles': 'admin'})
|
'X-Roles': 'admin'})
|
||||||
data = jsonutils.dumps([
|
data = jsonutils.dumps([
|
||||||
|
# Deleting an empty property to verify bug #1332103.
|
||||||
|
{'op': 'replace', 'path': '/spl_creator_policy', 'value': ''},
|
||||||
{'op': 'remove', 'path': '/spl_creator_policy'},
|
{'op': 'remove', 'path': '/spl_creator_policy'},
|
||||||
])
|
])
|
||||||
response = requests.patch(path, headers=headers, data=data)
|
response = requests.patch(path, headers=headers, data=data)
|
||||||
|
|
|
@ -168,6 +168,14 @@ class TestExtraPropertiesProxy(utils.BaseTestCase):
|
||||||
extra_prop_proxy.__setitem__, 'spl_create_prop',
|
extra_prop_proxy.__setitem__, 'spl_create_prop',
|
||||||
'par')
|
'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):
|
def test_create_extra_property_admin(self):
|
||||||
extra_properties = {}
|
extra_properties = {}
|
||||||
context = glance.context.RequestContext(roles=['admin'])
|
context = glance.context.RequestContext(roles=['admin'])
|
||||||
|
@ -218,6 +226,14 @@ class TestExtraPropertiesProxy(utils.BaseTestCase):
|
||||||
self.assertRaises(KeyError,
|
self.assertRaises(KeyError,
|
||||||
extra_prop_proxy.__delitem__, 'spl_read_prop')
|
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):
|
class TestProtectedImageFactoryProxy(utils.BaseTestCase):
|
||||||
def setUp(self):
|
def setUp(self):
|
||||||
|
|
|
@ -29,6 +29,7 @@ CONFIG_SECTIONS = [
|
||||||
'spl_update_prop',
|
'spl_update_prop',
|
||||||
'spl_update_only_prop',
|
'spl_update_only_prop',
|
||||||
'spl_delete_prop',
|
'spl_delete_prop',
|
||||||
|
'spl_delete_empty_prop',
|
||||||
'^x_all_permitted.*',
|
'^x_all_permitted.*',
|
||||||
'^x_none_permitted.*',
|
'^x_none_permitted.*',
|
||||||
'x_none_read',
|
'x_none_read',
|
||||||
|
|
Loading…
Reference in New Issue