Check get_image(s) in the API
This includes a change to catch Forbidden and convert to NotFound. The previous Forbidden handler was not only correct (it shoud hide the permissions error with "not found") but it was actually dead code, since the DB was performing its own checks and would never raise Forbidden. This also includes a change of the default policy for get_images to include the other states, like get_image does. I think this was just an oversight in the original RBAC patches, which didn't matter because they weren't really being honored strictly. Partially implements: blueprint policy-refactor Change-Id: I70100cd7f01da803e9740cea1f7ce7ae18ad6919
This commit is contained in:
parent
3825d2111a
commit
ba37ea3227
@ -514,14 +514,31 @@ class ImagesController(object):
|
|||||||
limit = CONF.limit_param_default
|
limit = CONF.limit_param_default
|
||||||
limit = min(CONF.api_limit_max, limit)
|
limit = min(CONF.api_limit_max, limit)
|
||||||
|
|
||||||
image_repo = self.gateway.get_repo(req.context)
|
image_repo = self.gateway.get_repo(req.context,
|
||||||
|
authorization_layer=False)
|
||||||
try:
|
try:
|
||||||
|
# NOTE(danms): This is just a "do you have permission to
|
||||||
|
# list images" check. Each image is checked against
|
||||||
|
# get_image below.
|
||||||
|
target = {'project_id': req.context.project_id}
|
||||||
|
self.policy.enforce(req.context, 'get_images', target)
|
||||||
|
|
||||||
images = image_repo.list(marker=marker, limit=limit,
|
images = image_repo.list(marker=marker, limit=limit,
|
||||||
sort_key=sort_key,
|
sort_key=sort_key,
|
||||||
sort_dir=sort_dir,
|
sort_dir=sort_dir,
|
||||||
filters=filters,
|
filters=filters,
|
||||||
member_status=member_status)
|
member_status=member_status)
|
||||||
if len(images) != 0 and len(images) == limit:
|
db_image_count = len(images)
|
||||||
|
images = [image for image in images
|
||||||
|
if api_policy.ImageAPIPolicy(req.context, image,
|
||||||
|
self.policy
|
||||||
|
).check('get_image')]
|
||||||
|
|
||||||
|
# NOTE(danms): we need to include the next marker if the DB
|
||||||
|
# paginated. Since we filter images based on policy, we can
|
||||||
|
# not determine if pagination happened from the final list,
|
||||||
|
# so use the original count.
|
||||||
|
if len(images) != 0 and db_image_count == limit:
|
||||||
result['next_marker'] = images[-1].image_id
|
result['next_marker'] = images[-1].image_id
|
||||||
except (exception.NotFound, exception.InvalidSortKey,
|
except (exception.NotFound, exception.InvalidSortKey,
|
||||||
exception.InvalidFilterRangeValue,
|
exception.InvalidFilterRangeValue,
|
||||||
@ -537,12 +554,13 @@ class ImagesController(object):
|
|||||||
return result
|
return result
|
||||||
|
|
||||||
def show(self, req, image_id):
|
def show(self, req, image_id):
|
||||||
image_repo = self.gateway.get_repo(req.context)
|
image_repo = self.gateway.get_repo(req.context,
|
||||||
|
authorization_layer=False)
|
||||||
try:
|
try:
|
||||||
return image_repo.get(image_id)
|
image = image_repo.get(image_id)
|
||||||
except exception.Forbidden as e:
|
api_policy.ImageAPIPolicy(req.context, image,
|
||||||
LOG.debug("User not permitted to show image '%s'", image_id)
|
self.policy).get_image()
|
||||||
raise webob.exc.HTTPForbidden(explanation=e.msg)
|
return image
|
||||||
except exception.NotFound as e:
|
except exception.NotFound as e:
|
||||||
raise webob.exc.HTTPNotFound(explanation=e.msg)
|
raise webob.exc.HTTPNotFound(explanation=e.msg)
|
||||||
except exception.NotAuthenticated as e:
|
except exception.NotAuthenticated as e:
|
||||||
|
@ -107,3 +107,6 @@ class ImageAPIPolicy(APIPolicyBase):
|
|||||||
|
|
||||||
def get_image(self):
|
def get_image(self):
|
||||||
self._enforce('get_image')
|
self._enforce('get_image')
|
||||||
|
|
||||||
|
def get_images(self):
|
||||||
|
self._enforce('get_images')
|
||||||
|
@ -29,14 +29,18 @@ IMAGE_MEMBER_CHECK = 'project_id:%(member_id)s'
|
|||||||
COMMUNITY_VISIBILITY_CHECK = '"community":%(visibility)s'
|
COMMUNITY_VISIBILITY_CHECK = '"community":%(visibility)s'
|
||||||
# Check if the visibility of the image supplied in the target matches "public"
|
# Check if the visibility of the image supplied in the target matches "public"
|
||||||
PUBLIC_VISIBILITY_CHECK = '"public":%(visibility)s'
|
PUBLIC_VISIBILITY_CHECK = '"public":%(visibility)s'
|
||||||
|
# Check if the visibility of the image supplied in the target matches "shared"
|
||||||
|
SHARED_VISIBILITY_CHECK = '"shared":%(visibility)s'
|
||||||
|
|
||||||
PROJECT_MEMBER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC = (
|
PROJECT_MEMBER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC_OR_SHARED = (
|
||||||
f'role:member and (project_id:%(project_id)s or {IMAGE_MEMBER_CHECK} '
|
f'role:member and (project_id:%(project_id)s or {IMAGE_MEMBER_CHECK} '
|
||||||
f'or {COMMUNITY_VISIBILITY_CHECK} or {PUBLIC_VISIBILITY_CHECK})'
|
f'or {COMMUNITY_VISIBILITY_CHECK} or {PUBLIC_VISIBILITY_CHECK} '
|
||||||
|
f'or {SHARED_VISIBILITY_CHECK})'
|
||||||
)
|
)
|
||||||
PROJECT_READER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC = (
|
PROJECT_READER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC_OR_SHARED = (
|
||||||
f'role:reader and (project_id:%(project_id)s or {IMAGE_MEMBER_CHECK} '
|
f'role:reader and (project_id:%(project_id)s or {IMAGE_MEMBER_CHECK} '
|
||||||
f'or {COMMUNITY_VISIBILITY_CHECK} or {PUBLIC_VISIBILITY_CHECK})'
|
f'or {COMMUNITY_VISIBILITY_CHECK} or {PUBLIC_VISIBILITY_CHECK} '
|
||||||
|
f'or {SHARED_VISIBILITY_CHECK})'
|
||||||
)
|
)
|
||||||
|
|
||||||
# FIXME(lbragstad): These are composite check strings that represents glance's
|
# FIXME(lbragstad): These are composite check strings that represents glance's
|
||||||
@ -58,10 +62,12 @@ PROJECT_READER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC = (
|
|||||||
ADMIN_OR_PROJECT_MEMBER = f'role:admin or ({PROJECT_MEMBER})'
|
ADMIN_OR_PROJECT_MEMBER = f'role:admin or ({PROJECT_MEMBER})'
|
||||||
ADMIN_OR_PROJECT_READER = f'role:admin or ({PROJECT_READER})'
|
ADMIN_OR_PROJECT_READER = f'role:admin or ({PROJECT_READER})'
|
||||||
ADMIN_OR_PROJECT_READER_GET_IMAGE = (
|
ADMIN_OR_PROJECT_READER_GET_IMAGE = (
|
||||||
f'role:admin or ({PROJECT_READER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC})'
|
f'role:admin or '
|
||||||
|
f'({PROJECT_READER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC_OR_SHARED})'
|
||||||
)
|
)
|
||||||
ADMIN_OR_PROJECT_MEMBER_DOWNLOAD_IMAGE = (
|
ADMIN_OR_PROJECT_MEMBER_DOWNLOAD_IMAGE = (
|
||||||
f'role:admin or ({PROJECT_MEMBER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC})'
|
f'role:admin or '
|
||||||
|
f'({PROJECT_MEMBER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC_OR_SHARED})'
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
@ -6857,7 +6857,7 @@ class TestCopyImagePermissions(functional.MultipleBackendFunctionalTest):
|
|||||||
'content-type': 'application/json',
|
'content-type': 'application/json',
|
||||||
})
|
})
|
||||||
headers = get_auth_header(TENANT2, TENANT2,
|
headers = get_auth_header(TENANT2, TENANT2,
|
||||||
role='member', headers=headers)
|
role='reader,member', headers=headers)
|
||||||
data = jsonutils.dumps(
|
data = jsonutils.dumps(
|
||||||
{'method': {'name': 'copy-image'},
|
{'method': {'name': 'copy-image'},
|
||||||
'stores': ['file2']})
|
'stores': ['file2']})
|
||||||
|
@ -160,3 +160,47 @@ class TestImagesPolicy(functional.SynchronousAPIBase):
|
|||||||
self.assertEqual(2,
|
self.assertEqual(2,
|
||||||
len(self.api_get(
|
len(self.api_get(
|
||||||
'/v2/images/%s' % image_id).json['locations']))
|
'/v2/images/%s' % image_id).json['locations']))
|
||||||
|
|
||||||
|
def test_image_get(self):
|
||||||
|
self.start_server()
|
||||||
|
|
||||||
|
image_id = self._create_and_upload()
|
||||||
|
|
||||||
|
# Make sure we can get the image
|
||||||
|
image = self.api_get('/v2/images/%s' % image_id).json
|
||||||
|
self.assertEqual(image_id, image['id'])
|
||||||
|
|
||||||
|
# Make sure we can list the image
|
||||||
|
images = self.api_get('/v2/images').json['images']
|
||||||
|
self.assertEqual(1, len(images))
|
||||||
|
self.assertEqual(image_id, images[0]['id'])
|
||||||
|
|
||||||
|
# Now disable get_images but allow get_image
|
||||||
|
self.set_policy_rules({'get_images': '!',
|
||||||
|
'get_image': ''})
|
||||||
|
|
||||||
|
# We should not be able to list, but still fetch the image by id
|
||||||
|
resp = self.api_get('/v2/images')
|
||||||
|
self.assertEqual(403, resp.status_code)
|
||||||
|
image = self.api_get('/v2/images/%s' % image_id).json
|
||||||
|
self.assertEqual(image_id, image['id'])
|
||||||
|
|
||||||
|
# Now disable get_image but allow get_images
|
||||||
|
self.set_policy_rules({'get_images': '',
|
||||||
|
'get_image': '!'})
|
||||||
|
|
||||||
|
# We should be able to list, but not actually see the image in the list
|
||||||
|
images = self.api_get('/v2/images').json['images']
|
||||||
|
self.assertEqual(0, len(images))
|
||||||
|
resp = self.api_get('/v2/images/%s' % image_id)
|
||||||
|
self.assertEqual(404, resp.status_code)
|
||||||
|
|
||||||
|
# Now disable both get_image and get_images
|
||||||
|
self.set_policy_rules({'get_images': '!',
|
||||||
|
'get_image': '!'})
|
||||||
|
|
||||||
|
# We should not be able to list or fetch by id
|
||||||
|
resp = self.api_get('/v2/images')
|
||||||
|
self.assertEqual(403, resp.status_code)
|
||||||
|
resp = self.api_get('/v2/images/%s' % image_id)
|
||||||
|
self.assertEqual(404, resp.status_code)
|
||||||
|
@ -275,8 +275,9 @@ class TestPolicyEnforcer(base.IsolatedUnitTest):
|
|||||||
def test_policy_file_default_rules_default_location(self):
|
def test_policy_file_default_rules_default_location(self):
|
||||||
enforcer = glance.api.policy.Enforcer()
|
enforcer = glance.api.policy.Enforcer()
|
||||||
|
|
||||||
context = glance.context.RequestContext(roles=[])
|
context = glance.context.RequestContext(roles=['reader'])
|
||||||
enforcer.enforce(context, 'get_image', {})
|
enforcer.enforce(context, 'get_image',
|
||||||
|
{'project_id': context.project_id})
|
||||||
|
|
||||||
def test_policy_file_custom_rules_default_location(self):
|
def test_policy_file_custom_rules_default_location(self):
|
||||||
rules = {"get_image": '!'}
|
rules = {"get_image": '!'}
|
||||||
@ -1073,11 +1074,12 @@ class TestDefaultPolicyCheckStrings(base.IsolatedUnitTest):
|
|||||||
expected = (
|
expected = (
|
||||||
'role:member and (project_id:%(project_id)s or '
|
'role:member and (project_id:%(project_id)s or '
|
||||||
'project_id:%(member_id)s or "community":%(visibility)s or '
|
'project_id:%(member_id)s or "community":%(visibility)s or '
|
||||||
'"public":%(visibility)s)'
|
'"public":%(visibility)s or "shared":%(visibility)s)'
|
||||||
)
|
)
|
||||||
self.assertEqual(
|
self.assertEqual(
|
||||||
expected,
|
expected,
|
||||||
base_policy.PROJECT_MEMBER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC
|
base_policy.
|
||||||
|
PROJECT_MEMBER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC_OR_SHARED
|
||||||
)
|
)
|
||||||
|
|
||||||
def test_project_reader_check_string(self):
|
def test_project_reader_check_string(self):
|
||||||
@ -1092,11 +1094,12 @@ class TestDefaultPolicyCheckStrings(base.IsolatedUnitTest):
|
|||||||
expected = (
|
expected = (
|
||||||
'role:reader and (project_id:%(project_id)s or '
|
'role:reader and (project_id:%(project_id)s or '
|
||||||
'project_id:%(member_id)s or "community":%(visibility)s or '
|
'project_id:%(member_id)s or "community":%(visibility)s or '
|
||||||
'"public":%(visibility)s)'
|
'"public":%(visibility)s or "shared":%(visibility)s)'
|
||||||
)
|
)
|
||||||
self.assertEqual(
|
self.assertEqual(
|
||||||
expected,
|
expected,
|
||||||
base_policy.PROJECT_READER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC
|
base_policy.
|
||||||
|
PROJECT_READER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC_OR_SHARED
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
@ -66,7 +66,7 @@ def sort_url_by_qs_keys(url):
|
|||||||
def get_fake_request(path='', method='POST', is_admin=False, user=USER1,
|
def get_fake_request(path='', method='POST', is_admin=False, user=USER1,
|
||||||
roles=None, tenant=TENANT1):
|
roles=None, tenant=TENANT1):
|
||||||
if roles is None:
|
if roles is None:
|
||||||
roles = ['member']
|
roles = ['member', 'reader']
|
||||||
|
|
||||||
req = wsgi.Request.blank(path)
|
req = wsgi.Request.blank(path)
|
||||||
req.method = method
|
req.method = method
|
||||||
|
@ -388,6 +388,30 @@ class TestImagesController(base.IsolatedUnitTest):
|
|||||||
self.assertEqual(expected, actual)
|
self.assertEqual(expected, actual)
|
||||||
self.assertNotIn('next_marker', output)
|
self.assertNotIn('next_marker', output)
|
||||||
|
|
||||||
|
def test_index_marker_would_be_disallowed(self):
|
||||||
|
self.config(limit_param_default=1, api_limit_max=10)
|
||||||
|
request = unit_test_utils.get_fake_request(is_admin=True)
|
||||||
|
|
||||||
|
def fake_enforce(context, action, target=None, **kw):
|
||||||
|
assert target is not None
|
||||||
|
if target['project_id'] != TENANT1:
|
||||||
|
raise exception.Forbidden()
|
||||||
|
|
||||||
|
# As admin, list three images. By default, this should leave
|
||||||
|
# us on UUID3 (and as next_marker), which is owned by TENANT3
|
||||||
|
output = self.controller.index(request, sort_dir=['asc'], limit=3)
|
||||||
|
self.assertEqual(UUID3, output['next_marker'])
|
||||||
|
self.assertEqual(3, len(output['images']))
|
||||||
|
|
||||||
|
# Now sub in our fake policy that restricts us to TENANT1 images only.
|
||||||
|
# Even though we list with limit=3, we should only get two images back,
|
||||||
|
# and our next_marker should be UUID2 because we couldn't see UUID3.
|
||||||
|
with mock.patch.object(self.controller.policy, 'enforce',
|
||||||
|
new=fake_enforce):
|
||||||
|
output = self.controller.index(request, sort_dir=['asc'], limit=3)
|
||||||
|
self.assertEqual(UUID2, output['next_marker'])
|
||||||
|
self.assertEqual(2, len(output['images']))
|
||||||
|
|
||||||
def test_index_with_id_filter(self):
|
def test_index_with_id_filter(self):
|
||||||
request = unit_test_utils.get_fake_request('/images?id=%s' % UUID1)
|
request = unit_test_utils.get_fake_request('/images?id=%s' % UUID1)
|
||||||
output = self.controller.index(request, filters={'id': UUID1})
|
output = self.controller.index(request, filters={'id': UUID1})
|
||||||
@ -775,6 +799,19 @@ class TestImagesController(base.IsolatedUnitTest):
|
|||||||
self.assertRaises(webob.exc.HTTPNotFound,
|
self.assertRaises(webob.exc.HTTPNotFound,
|
||||||
self.controller.show, request, UUID4)
|
self.controller.show, request, UUID4)
|
||||||
|
|
||||||
|
def test_show_not_allowed_by_policy(self):
|
||||||
|
# Use admin so that we get past the check buried in the DB and
|
||||||
|
# only hit the policy check we are mocking.
|
||||||
|
request = unit_test_utils.get_fake_request(is_admin=True)
|
||||||
|
with mock.patch.object(self.controller.policy, 'enforce') as mock_enf:
|
||||||
|
mock_enf.side_effect = webob.exc.HTTPForbidden()
|
||||||
|
# Make sure we get NotFound instead of Forbidden
|
||||||
|
exc = self.assertRaises(webob.exc.HTTPNotFound,
|
||||||
|
self.controller.show, request, UUID4)
|
||||||
|
# Make sure we did not leak details of the original Forbidden
|
||||||
|
# error into the NotFound returned to the client.
|
||||||
|
self.assertEqual('The resource could not be found.', str(exc))
|
||||||
|
|
||||||
def test_get_task_info(self):
|
def test_get_task_info(self):
|
||||||
request = unit_test_utils.get_fake_request()
|
request = unit_test_utils.get_fake_request()
|
||||||
output = self.controller.get_task_info(request, image_id=UUID1)
|
output = self.controller.get_task_info(request, image_id=UUID1)
|
||||||
@ -1621,7 +1658,8 @@ class TestImagesController(base.IsolatedUnitTest):
|
|||||||
created_image = self.controller.create(request, image=image,
|
created_image = self.controller.create(request, image=image,
|
||||||
extra_properties=extra_props,
|
extra_properties=extra_props,
|
||||||
tags=[])
|
tags=[])
|
||||||
another_request = unit_test_utils.get_fake_request(roles=['member'])
|
another_request = unit_test_utils.get_fake_request(roles=['reader',
|
||||||
|
'member'])
|
||||||
output = self.controller.show(another_request, created_image.image_id)
|
output = self.controller.show(another_request, created_image.image_id)
|
||||||
self.assertEqual('bar', output.extra_properties['x_owner_foo'])
|
self.assertEqual('bar', output.extra_properties['x_owner_foo'])
|
||||||
|
|
||||||
@ -1638,7 +1676,8 @@ class TestImagesController(base.IsolatedUnitTest):
|
|||||||
created_image = self.controller.create(request, image=image,
|
created_image = self.controller.create(request, image=image,
|
||||||
extra_properties=extra_props,
|
extra_properties=extra_props,
|
||||||
tags=[])
|
tags=[])
|
||||||
another_request = unit_test_utils.get_fake_request(roles=['fake_role'])
|
another_request = unit_test_utils.get_fake_request(roles=['reader',
|
||||||
|
'fake_role'])
|
||||||
output = self.controller.show(another_request, created_image.image_id)
|
output = self.controller.show(another_request, created_image.image_id)
|
||||||
self.assertRaises(KeyError, output.extra_properties.__getitem__,
|
self.assertRaises(KeyError, output.extra_properties.__getitem__,
|
||||||
'x_owner_foo')
|
'x_owner_foo')
|
||||||
@ -1759,7 +1798,8 @@ class TestImagesController(base.IsolatedUnitTest):
|
|||||||
created_image = self.controller.create(request, image=image,
|
created_image = self.controller.create(request, image=image,
|
||||||
extra_properties=extra_props,
|
extra_properties=extra_props,
|
||||||
tags=[])
|
tags=[])
|
||||||
another_request = unit_test_utils.get_fake_request(roles=['member'])
|
another_request = unit_test_utils.get_fake_request(roles=['reader',
|
||||||
|
'member'])
|
||||||
output = self.controller.show(another_request, created_image.image_id)
|
output = self.controller.show(another_request, created_image.image_id)
|
||||||
self.assertEqual('1', output.extra_properties['x_case_insensitive'])
|
self.assertEqual('1', output.extra_properties['x_case_insensitive'])
|
||||||
|
|
||||||
|
@ -139,3 +139,15 @@ class APIImagePolicy(APIPolicyBase):
|
|||||||
mock_enf.assert_has_calls([
|
mock_enf.assert_has_calls([
|
||||||
mock.call(mock.ANY, 'modify_image', mock.ANY),
|
mock.call(mock.ANY, 'modify_image', mock.ANY),
|
||||||
mock.call(mock.ANY, 'get_image', mock.ANY)])
|
mock.call(mock.ANY, 'get_image', mock.ANY)])
|
||||||
|
|
||||||
|
def test_get_image(self):
|
||||||
|
self.policy.get_image()
|
||||||
|
self.enforcer.enforce.assert_called_once_with(self.context,
|
||||||
|
'get_image',
|
||||||
|
mock.ANY)
|
||||||
|
|
||||||
|
def test_get_images(self):
|
||||||
|
self.policy.get_images()
|
||||||
|
self.enforcer.enforce.assert_called_once_with(self.context,
|
||||||
|
'get_images',
|
||||||
|
mock.ANY)
|
||||||
|
Loading…
Reference in New Issue
Block a user