openstack
/
glance
Code Issues Proposed changes

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
This commit is contained in:
Dan Smith 2020-06-23 07:12:12 -07:00
parent e6db0b10a7
commit c930638fcf
2 changed files with 20 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
glance/api/v2/images.py
View File

@ -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:

29
glance/tests/unit/v2/test_images_resource.py
View File

@ -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'},