diff --git a/glance/api/v2/images.py b/glance/api/v2/images.py index 72cc39fa2b..37a06dd602 100644 --- a/glance/api/v2/images.py +++ b/glance/api/v2/images.py @@ -274,12 +274,15 @@ class ImagesController(object): "invisible.") raise webob.exc.HTTPForbidden(explanation=msg) + if image.status not in ('active', 'queued'): + msg = _("It's not allowed to replace locations if image status is " + "%s.") % image.status + raise webob.exc.HTTPConflict(explanation=msg) + try: # NOTE(flwang): _locations_proxy's setattr method will check if # the update is acceptable. image.locations = value - if image.status == 'queued': - image.status = 'active' except (exception.BadStoreUri, exception.DuplicateLocation) as e: raise webob.exc.HTTPBadRequest(explanation=e.msg) except ValueError as ve: # update image status failed. @@ -318,6 +321,11 @@ class ImagesController(object): "invisible.") raise webob.exc.HTTPForbidden(explanation=msg) + if image.status not in ('active'): + msg = _("It's not allowed to remove locations if image status is " + "%s.") % image.status + raise webob.exc.HTTPConflict(explanation=msg) + if len(image.locations) == 1: LOG.debug("User forbidden to remove last location of image %s", image.image_id) diff --git a/glance/tests/unit/v2/test_images_resource.py b/glance/tests/unit/v2/test_images_resource.py index a0fe9f2515..6f81bd705b 100644 --- a/glance/tests/unit/v2/test_images_resource.py +++ b/glance/tests/unit/v2/test_images_resource.py @@ -1571,90 +1571,113 @@ class TestImagesController(base.IsolatedUnitTest): self.assertEqual(2, len(output.locations)) self.assertEqual(new_location, output.locations[1]) - def test_update_add_locations_status_saving(self): + def test_replace_location_possible_on_queued(self): + self.config(show_multiple_locations=True) + self.images = [ + _db_fixture('1', owner=TENANT1, checksum=CHKSUM, + name='1', + is_public=True, + disk_format='raw', + container_format='bare', + status='queued'), + ] + self.db.image_create(None, self.images[0]) + request = unit_test_utils.get_fake_request() + new_location = {'url': '%s/fake_location_1' % BASE_URI, 'metadata': {}} + changes = [{'op': 'replace', 'path': ['locations'], + 'value': [new_location]}] + output = self.controller.update(request, '1', changes) + self.assertEqual('1', output.image_id) + self.assertEqual(1, len(output.locations)) + self.assertEqual(new_location, output.locations[0]) + + def test_add_location_possible_on_queued(self): + self.config(show_multiple_locations=True) + self.images = [ + _db_fixture('1', owner=TENANT1, checksum=CHKSUM, + name='1', + is_public=True, + disk_format='raw', + container_format='bare', + status='queued'), + ] + self.db.image_create(None, self.images[0]) + request = unit_test_utils.get_fake_request() + new_location = {'url': '%s/fake_location_1' % BASE_URI, 'metadata': {}} + changes = [{'op': 'add', 'path': ['locations', '-'], + 'value': new_location}] + output = self.controller.update(request, '1', changes) + self.assertEqual('1', output.image_id) + self.assertEqual(1, len(output.locations)) + self.assertEqual(new_location, output.locations[0]) + + def _test_update_locations_status(self, image_status, update): self.config(show_multiple_locations=True) self.images = [ _db_fixture('1', owner=TENANT1, checksum=CHKSUM, name='1', disk_format='raw', container_format='bare', - status='saving'), + status=image_status), ] - self.db.image_create(None, self.images[0]) - new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} request = unit_test_utils.get_fake_request() - changes = [{'op': 'add', 'path': ['locations', '-'], + if image_status == 'deactivated': + self.db.image_create(request.context, self.images[0]) + else: + self.db.image_create(None, self.images[0]) + new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} + changes = [{'op': update, 'path': ['locations', '-'], 'value': new_location}] self.assertRaises(webob.exc.HTTPConflict, self.controller.update, request, '1', changes) - def test_update_add_locations_status_deactivated(self): - self.config(show_multiple_locations=True) - self.images = [ - _db_fixture('1', owner=TENANT1, checksum=CHKSUM, - name='1', - disk_format='raw', - container_format='bare', - status='deactivated'), - ] - request = unit_test_utils.get_fake_request() - self.db.image_create(request.context, self.images[0]) - new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} - changes = [{'op': 'add', 'path': ['locations', '-'], - 'value': new_location}] - self.assertRaises(webob.exc.HTTPConflict, - self.controller.update, request, '1', changes) + def test_location_add_not_permitted_status_saving(self): + self._test_update_locations_status('saving', 'add') - def test_update_add_locations_status_deleted(self): - self.config(show_multiple_locations=True) - self.images = [ - _db_fixture('1', owner=TENANT1, checksum=CHKSUM, - name='1', - disk_format='raw', - container_format='bare', - status='deleted'), - ] - self.db.image_create(None, self.images[0]) - new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} - request = unit_test_utils.get_fake_request() - changes = [{'op': 'add', 'path': ['locations', '-'], - 'value': new_location}] - self.assertRaises(webob.exc.HTTPConflict, - self.controller.update, request, '1', changes) + def test_location_add_not_permitted_status_deactivated(self): + self._test_update_locations_status('deactivated', 'add') - def test_update_add_locations_status_pending_delete(self): - self.config(show_multiple_locations=True) - self.images = [ - _db_fixture('1', owner=TENANT1, checksum=CHKSUM, - name='1', - disk_format='raw', - container_format='bare', - status='pending_delete'), - ] - self.db.image_create(None, self.images[0]) - new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} - request = unit_test_utils.get_fake_request() - changes = [{'op': 'add', 'path': ['locations', '-'], - 'value': new_location}] - self.assertRaises(webob.exc.HTTPConflict, - self.controller.update, request, '1', changes) + def test_location_add_not_permitted_status_deleted(self): + self._test_update_locations_status('deleted', 'add') - def test_update_add_locations_status_killed(self): - self.config(show_multiple_locations=True) - self.images = [ - _db_fixture('1', owner=TENANT1, checksum=CHKSUM, - name='1', - disk_format='raw', - container_format='bare', - status='killed'), - ] - self.db.image_create(None, self.images[0]) - new_location = {'url': '%s/fake_location' % BASE_URI, 'metadata': {}} - request = unit_test_utils.get_fake_request() - changes = [{'op': 'add', 'path': ['locations', '-'], - 'value': new_location}] - self.assertRaises(webob.exc.HTTPConflict, - self.controller.update, request, '1', changes) + def test_location_add_not_permitted_status_pending_delete(self): + self._test_update_locations_status('pending_delete', 'add') + + def test_location_add_not_permitted_status_killed(self): + self._test_update_locations_status('killed', 'add') + + def test_location_remove_not_permitted_status_saving(self): + self._test_update_locations_status('saving', 'remove') + + def test_location_remove_not_permitted_status_deactivated(self): + self._test_update_locations_status('deactivated', 'remove') + + def test_location_remove_not_permitted_status_deleted(self): + self._test_update_locations_status('deleted', 'remove') + + def test_location_remove_not_permitted_status_pending_delete(self): + self._test_update_locations_status('pending_delete', 'remove') + + def test_location_remove_not_permitted_status_killed(self): + self._test_update_locations_status('killed', 'remove') + + def test_location_remove_not_permitted_status_queued(self): + self._test_update_locations_status('queued', 'remove') + + def test_location_replace_not_permitted_status_saving(self): + self._test_update_locations_status('saving', 'replace') + + def test_location_replace_not_permitted_status_deactivated(self): + self._test_update_locations_status('deactivated', 'replace') + + def test_location_replace_not_permitted_status_deleted(self): + self._test_update_locations_status('deleted', 'replace') + + def test_location_replace_not_permitted_status_pending_delete(self): + self._test_update_locations_status('pending_delete', 'replace') + + def test_location_replace_not_permitted_status_killed(self): + self._test_update_locations_status('killed', 'replace') def test_update_add_locations_insertion(self): self.config(show_multiple_locations=True) diff --git a/releasenotes/notes/restrict_location_updates-05454bb765a8c92c.yaml b/releasenotes/notes/restrict_location_updates-05454bb765a8c92c.yaml new file mode 100644 index 0000000000..20bd3bebcf --- /dev/null +++ b/releasenotes/notes/restrict_location_updates-05454bb765a8c92c.yaml @@ -0,0 +1,22 @@ +--- +prelude: > + Location updates for images are now restricted to + images in ``active`` or ``queued`` status. Please + refer to the "Fixes" section for more information. + +fixes: + - | + Image location updates to an image not in status + ``active`` or ``queued`` can introduce race + conditions and security issues and hence a bad + experience for users and operators. As a result, + we have restricted image location updates in this + release. Users will now observe the following: + + * HTTP Response Code 409 (Conflict) will be returned + in response to an attempt to remove an image + location when the image status is not ``active`` + * HTTP Response Code 409 (Conflict) will be returned + in response to an attempt to replace an image + location when the image status is not ``active`` or + ``queued``