From b5f6c2864f5ca829854af5c12f37a3d49ccc9d5f Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Thu, 14 Dec 2017 02:24:29 +0000 Subject: [PATCH] Fix volume image metadata endpoints returning None This commit fixes the following volume image metadata endpoints returning None following policy enforcement failure: * ``os-set_image_metadata`` * ``os-unset_image_metadata`` The endpoints will now correctly raise a 403 Forbidden instead. The kwarg `fatal=False` was dropped from `context.authorize` for these APIs because the kwarg is only useful when adding additional information to the response body (if the user is authorized). This commit: * makes the fix for the two endpoints above * adds unit tests for validating the new, correct behavior (as a side note, policy overriding in tests can be more easily accomplished via adoption of something like [0]) Also note that since the default policy rule for these endpoints is "admin_or_owner" Tempest doesn't validate this behavior by default. [0] https://github.com/openstack/nova/blob/e599b13e4940fb9654f0e0c0f43077a6979eaabe/nova/tests/unit/policy_fixture.py#L30 Change-Id: Icc286d529609165e5f14cb506342660d7bc2ae9f Closes-Bug: #1737609 --- cinder/api/contrib/volume_image_metadata.py | 4 +- .../api/contrib/test_volume_image_metadata.py | 44 +++++++++++++++++++ ...oints-returning-none-ba0590e6c6757b0c.yaml | 10 +++++ 3 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 releasenotes/notes/fix-vol-image-metadata-endpoints-returning-none-ba0590e6c6757b0c.yaml diff --git a/cinder/api/contrib/volume_image_metadata.py b/cinder/api/contrib/volume_image_metadata.py index 2a7b8ae9be1..faab8e5056a 100644 --- a/cinder/api/contrib/volume_image_metadata.py +++ b/cinder/api/contrib/volume_image_metadata.py @@ -85,7 +85,7 @@ class VolumeImageMetadataController(wsgi.Controller): @wsgi.action("os-set_image_metadata") def create(self, req, id, body): context = req.environ['cinder.context'] - if context.authorize(policy.IMAGE_METADATA_POLICY, fatal=False): + if context.authorize(policy.IMAGE_METADATA_POLICY): try: metadata = body['os-set_image_metadata']['metadata'] except (KeyError, TypeError): @@ -128,7 +128,7 @@ class VolumeImageMetadataController(wsgi.Controller): def delete(self, req, id, body): """Deletes an existing image metadata.""" context = req.environ['cinder.context'] - if context.authorize(policy.IMAGE_METADATA_POLICY, fatal=False): + if context.authorize(policy.IMAGE_METADATA_POLICY): try: key = body['os-unset_image_metadata']['key'] except (KeyError, TypeError): diff --git a/cinder/tests/unit/api/contrib/test_volume_image_metadata.py b/cinder/tests/unit/api/contrib/test_volume_image_metadata.py index cc8c88f2fa9..7ac4f133716 100644 --- a/cinder/tests/unit/api/contrib/test_volume_image_metadata.py +++ b/cinder/tests/unit/api/contrib/test_volume_image_metadata.py @@ -14,6 +14,7 @@ import uuid +from oslo_policy import policy as oslo_policy from oslo_serialization import jsonutils from oslo_utils import timeutils from six.moves import http_client @@ -25,6 +26,9 @@ from cinder import db from cinder import exception from cinder import objects from cinder.objects import fields +from cinder.policies import base as base_policy +from cinder.policies import volume_metadata as metadata_policy +from cinder import policy from cinder import test from cinder.tests.unit.api import fakes from cinder.tests.unit import fake_constants as fake @@ -216,6 +220,26 @@ class VolumeImageMetadataTest(test.TestCase): self.assertEqual(fake_image_metadata, jsonutils.loads(res.body)["metadata"]) + def test_create_image_metadata_policy_not_authorized(self): + rules = { + metadata_policy.IMAGE_METADATA_POLICY: base_policy.RULE_ADMIN_API + } + policy.set_rules(oslo_policy.Rules.from_dict(rules)) + self.addCleanup(policy.reset) + + req = fakes.HTTPRequest.blank('/v2/%s/volumes/%s/action' % ( + fake.PROJECT_ID, fake.VOLUME_ID), use_admin_context=False) + + req.method = 'POST' + req.content_type = "application/json" + body = {"os-set_image_metadata": { + "metadata": {"image_name": "fake"}} + } + req.body = jsonutils.dump_as_bytes(body) + + self.assertRaises(exception.PolicyNotAuthorized, + self.controller.create, req, fake.VOLUME_ID, None) + def test_create_with_keys_case_insensitive(self): # If the keys in uppercase_and_lowercase, should return the one # which server added @@ -320,6 +344,26 @@ class VolumeImageMetadataTest(test.TestCase): fake_auth_context=self.user_ctxt)) self.assertEqual(http_client.OK, res.status_int) + def test_delete_image_metadata_policy_not_authorized(self): + rules = { + metadata_policy.IMAGE_METADATA_POLICY: base_policy.RULE_ADMIN_API + } + policy.set_rules(oslo_policy.Rules.from_dict(rules)) + self.addCleanup(policy.reset) + + req = fakes.HTTPRequest.blank('/v2/%s/volumes/%s/action' % ( + fake.PROJECT_ID, fake.VOLUME_ID), use_admin_context=False) + + req.method = 'POST' + req.content_type = "application/json" + body = {"os-unset_image_metadata": { + "metadata": {"image_name": "fake"}} + } + req.body = jsonutils.dump_as_bytes(body) + + self.assertRaises(exception.PolicyNotAuthorized, + self.controller.delete, req, fake.VOLUME_ID, None) + def test_delete_meta_not_found(self): data = {"os-unset_image_metadata": { "key": "invalid_id"} diff --git a/releasenotes/notes/fix-vol-image-metadata-endpoints-returning-none-ba0590e6c6757b0c.yaml b/releasenotes/notes/fix-vol-image-metadata-endpoints-returning-none-ba0590e6c6757b0c.yaml new file mode 100644 index 00000000000..0c40560a3f3 --- /dev/null +++ b/releasenotes/notes/fix-vol-image-metadata-endpoints-returning-none-ba0590e6c6757b0c.yaml @@ -0,0 +1,10 @@ +--- +fixes: + - | + Fix the following volume image metadata endpoints returning None following + policy enforcement failure: + + * ``os-set_image_metadata`` + * ``os-unset_image_metadata`` + + The endpoints will now correctly raise a 403 Forbidden instead.