From c930638fcf58b43d53903cfc30e06fd6919bdad6 Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Tue, 23 Jun 2020 07:12:12 -0700 Subject: [PATCH] Check authorization before import for image Right now we only check to see if the user can see the image before we kick off an import operation. However, that will never work unless the user is the *owner* of the image (or an admin) which means we return a 202 to the API caller and then the task fails immediately. This change makes us check that authorization up front and return an appropriate error to the user so they know it failed, and avoid starting a task destined for failure. Note that there was already a check for a Forbidden result when calling the import API. However, that used a context.owner=None which could never happen in reality. A more suitable check would have been to use a context with a different real owner, but it turns out that the task creation would have succeeded in that case as well. This test is changed to use an alternate owner and ensure that we get the forbidden result from the new check immediately. Change-Id: I385f222c5e3b46978b40bdefdc28fcb20d9c67d3 Closes-Bug: #1884587 --- glance/api/v2/images.py | 4 +++ glance/tests/unit/v2/test_images_resource.py | 29 +++++++++++--------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/glance/api/v2/images.py b/glance/api/v2/images.py index fd5611c207..8acb8e81f0 100644 --- a/glance/api/v2/images.py +++ b/glance/api/v2/images.py @@ -30,6 +30,7 @@ from six.moves import http_client as http import six.moves.urllib.parse as urlparse import webob.exc +from glance.api import authorization from glance.api import common from glance.api import policy from glance.common import exception @@ -131,6 +132,9 @@ class ImagesController(object): if not getattr(image, 'disk_format', None): msg = _("'disk_format' needs to be set before import") raise exception.Conflict(msg) + if not authorization.is_image_mutable(req.context, image): + raise webob.exc.HTTPForbidden( + explanation=_("Operation not permitted")) stores = [None] if CONF.enabled_backends: diff --git a/glance/tests/unit/v2/test_images_resource.py b/glance/tests/unit/v2/test_images_resource.py index 74911b99e7..75cf61a694 100644 --- a/glance/tests/unit/v2/test_images_resource.py +++ b/glance/tests/unit/v2/test_images_resource.py @@ -139,6 +139,7 @@ class FakeImage(object): self.container_format = container_format self.disk_format = disk_format self.locations = locations + self.owner = unit_test_utils.TENANT1 class TestImagesController(base.IsolatedUnitTest): @@ -2944,18 +2945,20 @@ class TestImagesController(base.IsolatedUnitTest): self.assertEqual(UUID4, output) - def test_image_import_not_allowed(self): - request = unit_test_utils.get_fake_request() - # NOTE(abhishekk): For coverage purpose setting tenant to - # None. It is not expected to do in normal scenarios. - request.context.project_id = None - with mock.patch.object( - glance.api.authorization.ImageRepoProxy, 'get') as mock_get: - mock_get.return_value = FakeImage(status='uploading') - self.assertRaises(webob.exc.HTTPForbidden, - self.controller.import_image, - request, UUID4, {'method': {'name': - 'glance-direct'}}) + @mock.patch.object(glance.domain.TaskFactory, 'new_task') + @mock.patch.object(glance.api.authorization.ImageRepoProxy, 'get') + def test_image_import_not_allowed(self, mock_get, mock_new_task): + # NOTE(danms): FakeImage is owned by utils.TENANT1. Try to do the + # import as TENANT2 and we should get an HTTPForbidden + request = unit_test_utils.get_fake_request(tenant=TENANT2) + mock_get.return_value = FakeImage(status='uploading') + self.assertRaises(webob.exc.HTTPForbidden, + self.controller.import_image, + request, UUID4, {'method': {'name': + 'glance-direct'}}) + # NOTE(danms): Make sure we failed early and never even created + # a task + mock_new_task.assert_not_called() def test_delete_encryption_key_no_encryption_key(self): request = unit_test_utils.get_fake_request() @@ -5433,7 +5436,7 @@ class TestMultiImagesController(base.MultiIsolatedUnitTest): 'stores': ["fast"]}) def test_copy_image_in_existing_store(self): - request = unit_test_utils.get_fake_request() + request = unit_test_utils.get_fake_request(tenant=TENANT3) self.assertRaises(webob.exc.HTTPBadRequest, self.controller.import_image, request, UUID6, {'method': {'name': 'copy-image'},