Implement project personas for volume metadata API

Implements the 3 Xena personas (no system scope).  Includes new, more
granular policies for volume image metadata.

Co-authored-by: Lance Bragstad <lbragstad@gmail.com>
Co-authored-by: Brian Rosmaita <rosmaita.fossdev@gmail.com>
Co-authored-by: Alan Bishop <abishop@redhat.com>

Change-Id: I82a3daa0224132b59ba14c6181500f1f492c566a

cinder/api/contrib/volume_image_metadata.py

@@ -72,13 +72,13 @@ class VolumeImageMetadataController(wsgi.Controller):
     @wsgi.extends
     def show(self, req, resp_obj, id):
         context = req.environ['cinder.context']
-        if context.authorize(policy.IMAGE_METADATA_POLICY, fatal=False):
+        if context.authorize(policy.IMAGE_METADATA_SHOW_POLICY, fatal=False):
             self._add_image_metadata(context, [resp_obj.obj['volume']])
 
     @wsgi.extends
     def detail(self, req, resp_obj):
         context = req.environ['cinder.context']
-        if context.authorize(policy.IMAGE_METADATA_POLICY, fatal=False):
+        if context.authorize(policy.IMAGE_METADATA_SHOW_POLICY, fatal=False):
             # Just get the image metadata of those volumes in response.
             volumes = list(resp_obj.obj.get('volumes', []))
             if volumes:
@@ -89,7 +89,7 @@ class VolumeImageMetadataController(wsgi.Controller):
     def create(self, req, id, body):
         context = req.environ['cinder.context']
         volume = objects.Volume.get_by_id(context, id)
-        if context.authorize(policy.IMAGE_METADATA_POLICY,
+        if context.authorize(policy.IMAGE_METADATA_SET_POLICY,
                              target_obj=volume):
             metadata = body['os-set_image_metadata']['metadata']
             new_metadata = self._update_volume_image_metadata(context,
@@ -131,7 +131,8 @@ class VolumeImageMetadataController(wsgi.Controller):
         """Deletes an existing image metadata."""
         context = req.environ['cinder.context']
         volume = objects.Volume.get_by_id(context, id)
-        if context.authorize(policy.IMAGE_METADATA_POLICY, target_obj=volume):
+        if context.authorize(policy.IMAGE_METADATA_REMOVE_POLICY,
+                             target_obj=volume):
             key = body['os-unset_image_metadata']['key']
 
             vol, metadata = self._get_image_metadata(context, id)

cinder/policies/volume_metadata.py

@@ -22,16 +22,45 @@ CREATE_POLICY = "volume:create_volume_metadata"
 DELETE_POLICY = "volume:delete_volume_metadata"
 UPDATE_POLICY = "volume:update_volume_metadata"
 IMAGE_METADATA_POLICY = "volume_extension:volume_image_metadata"
+IMAGE_METADATA_SHOW_POLICY = "volume_extension:volume_image_metadata:show"
+IMAGE_METADATA_SET_POLICY = "volume_extension:volume_image_metadata:set"
+IMAGE_METADATA_REMOVE_POLICY = "volume_extension:volume_image_metadata:remove"
 UPDATE_ADMIN_METADATA_POLICY = "volume:update_volume_admin_metadata"
 
 
 BASE_POLICY_NAME = 'volume:volume_metadata:%s'
 
 
+deprecated_get_volume_metadata = base.CinderDeprecatedRule(
+    name=GET_POLICY,
+    check_str=base.RULE_ADMIN_OR_OWNER
+)
+deprecated_create_volume_metadata = base.CinderDeprecatedRule(
+    name=CREATE_POLICY,
+    check_str=base.RULE_ADMIN_OR_OWNER
+)
+deprecated_update_volume_metadata = base.CinderDeprecatedRule(
+    name=UPDATE_POLICY,
+    check_str=base.RULE_ADMIN_OR_OWNER
+)
+deprecated_delete_volume_metadata = base.CinderDeprecatedRule(
+    name=DELETE_POLICY,
+    check_str=base.RULE_ADMIN_OR_OWNER
+)
+# this is being replaced in Xena by 3 more granular policies
+deprecated_image_metadata = base.CinderDeprecatedRule(
+    name=IMAGE_METADATA_POLICY,
+    check_str=base.RULE_ADMIN_OR_OWNER,
+    deprecated_reason=(
+        f'{IMAGE_METADATA_POLICY} has been replaced by more granular '
+        'policies that separately govern show, set, and remove operations.')
+)
+
+
 volume_metadata_policies = [
     policy.DocumentedRuleDefault(
         name=GET_POLICY,
-        check_str=base.RULE_ADMIN_OR_OWNER,
+        check_str=base.SYSTEM_READER_OR_PROJECT_READER,
         description="Show volume's metadata or one specified metadata "
                     "with a given key.",
         operations=[
@@ -42,23 +71,32 @@ volume_metadata_policies = [
             {
                 'method': 'GET',
                 'path': '/volumes/{volume_id}/metadata/{key}'
+            },
+            {
+                'method': 'POST',
+                'path': '/volumes/{volume_id}/action  (os-show_image_metadata)'
             }
-        ]),
+        ],
+        deprecated_rule=deprecated_get_volume_metadata,
+    ),
     policy.DocumentedRuleDefault(
         name=CREATE_POLICY,
-        check_str=base.RULE_ADMIN_OR_OWNER,
+        check_str=base.SYSTEM_ADMIN_OR_PROJECT_MEMBER,
         description="Create volume metadata.",
         operations=[
             {
                 'method': 'POST',
                 'path': '/volumes/{volume_id}/metadata'
             }
-        ]),
+        ],
+        deprecated_rule=deprecated_create_volume_metadata,
+    ),
     policy.DocumentedRuleDefault(
         name=UPDATE_POLICY,
-        check_str=base.RULE_ADMIN_OR_OWNER,
+        check_str=base.SYSTEM_ADMIN_OR_PROJECT_MEMBER,
-        description="Update volume's metadata or one specified "
+        description=(
-                    "metadata with a given key.",
+            "Replace a volume's metadata dictionary or update a single "
+            "metadatum with a given key."),
         operations=[
             {
                 'method': 'PUT',
@@ -68,22 +106,27 @@ volume_metadata_policies = [
                 'method': 'PUT',
                 'path': '/volumes/{volume_id}/metadata/{key}'
             }
-        ]),
+        ],
+        deprecated_rule=deprecated_update_volume_metadata,
+    ),
     policy.DocumentedRuleDefault(
         name=DELETE_POLICY,
-        check_str=base.RULE_ADMIN_OR_OWNER,
+        check_str=base.SYSTEM_ADMIN_OR_PROJECT_MEMBER,
-        description="Delete volume's specified metadata with a given key.",
+        description="Delete a volume's metadatum with the given key.",
         operations=[
             {
                 'method': 'DELETE',
                 'path': '/volumes/{volume_id}/metadata/{key}'
             }
-        ]),
+        ],
+        deprecated_rule=deprecated_delete_volume_metadata,
+    ),
     policy.DocumentedRuleDefault(
-        name=IMAGE_METADATA_POLICY,
+        name=IMAGE_METADATA_SHOW_POLICY,
-        check_str=base.RULE_ADMIN_OR_OWNER,
+        check_str=base.SYSTEM_READER_OR_PROJECT_READER,
-        description="Volume's image metadata related operation, create, "
+        description=(
-                    "delete, show and list.",
+            "Include a volume's image metadata in volume detail responses.  "
+            "The ability to make these calls is governed by other policies."),
         operations=[
             {
                 'method': 'GET',
@@ -92,21 +135,45 @@ volume_metadata_policies = [
             {
                 'method': 'GET',
                 'path': '/volumes/{volume_id}'
-            },
+            }
+        ],
+        # TODO: will need its own deprecated rule in Yoga
+        deprecated_rule=deprecated_image_metadata,
+    ),
+    policy.DocumentedRuleDefault(
+        name=IMAGE_METADATA_SET_POLICY,
+        check_str=base.SYSTEM_ADMIN_OR_PROJECT_MEMBER,
+        description="Set image metadata for a volume",
+        operations=[
             {
                 'method': 'POST',
                 'path': '/volumes/{volume_id}/action (os-set_image_metadata)'
-            },
+            }
+        ],
+        # TODO: will need its own deprecated rule in Yoga
+        deprecated_rule=deprecated_image_metadata,
+    ),
+    policy.DocumentedRuleDefault(
+        name=IMAGE_METADATA_REMOVE_POLICY,
+        check_str=base.SYSTEM_ADMIN_OR_PROJECT_MEMBER,
+        description="Remove specific image metadata from a volume",
+        operations=[
             {
                 'method': 'POST',
                 'path': '/volumes/{volume_id}/action (os-unset_image_metadata)'
             }
-        ]),
+        ],
+        # TODO: will need its own deprecated rule in Yoga
+        deprecated_rule=deprecated_image_metadata,
+    ),
     policy.DocumentedRuleDefault(
         name=UPDATE_ADMIN_METADATA_POLICY,
+        # TODO: deprecate checkstring in Yoga
         check_str=base.RULE_ADMIN_API,
-        description="Update volume admin metadata. It's used in `attach` "
+        description=(
-                    "and `os-update_readonly_flag` APIs",
+            "Update volume admin metadata. This permission is required "
+            "to complete these API calls, though the ability to make these "
+            "calls is governed by other policies."),
         operations=[
             {
                 'method': 'POST',

cinder/tests/unit/policies/test_volume_metadata.py

@@ -14,14 +14,327 @@
 from http import HTTPStatus
 from unittest import mock
 
+import ddt
+
+from cinder.api.contrib import volume_image_metadata as image_metadata
+from cinder.api import microversions as mv
+from cinder.api.v3 import volume_metadata
+from cinder import db
+from cinder import exception
+from cinder.policies import volume_metadata as policy
+from cinder.tests.unit.api import fakes as fake_api
+from cinder.tests.unit.policies import base
 from cinder.tests.unit.policies import test_base
+from cinder.tests.unit import utils as test_utils
 from cinder.volume import api as volume_api
 
 
-# TODO(yikun): The below policy test cases should be added:
+@ddt.ddt
-# * IMAGE_METADATA_POLICY
+class VolumeMetadataPolicyTest(base.BasePolicyTest):
-class VolumePolicyTests(test_base.CinderPolicyTests):
+    authorized_readers = [
+        'legacy_admin',
+        'legacy_owner',
+        'system_admin',
+        'project_admin',
+        'project_member',
+        'project_reader',
+        'project_foo',
+    ]
 
+    unauthorized_readers = [
+        'system_member',
+        'system_reader',
+        'system_foo',
+        'other_project_member',
+        'other_project_reader',
+    ]
+
+    authorized_members = [
+        'legacy_admin',
+        'legacy_owner',
+        'system_admin',
+        'project_admin',
+        'project_member',
+        'project_reader',
+        'project_foo',
+    ]
+
+    unauthorized_members = [
+        'system_member',
+        'system_reader',
+        'system_foo',
+        'other_project_member',
+        'other_project_reader',
+    ]
+
+    authorized_admins = [
+        'legacy_admin',
+        'system_admin',
+        'project_admin',
+    ]
+
+    unauthorized_admins = [
+        'legacy_owner',
+        'system_member',
+        'system_reader',
+        'system_foo',
+        'project_member',
+        'project_reader',
+        'project_foo',
+        'other_project_member',
+        'other_project_reader',
+    ]
+
+    # DB validations will throw VolumeNotFound for some contexts
+    unauthorized_exceptions = [
+        exception.VolumeNotFound,
+    ]
+
+    # Basic policy test is without enforcing scope (which cinder doesn't
+    # yet support) and deprecated rules enabled.
+    def setUp(self, enforce_scope=False, enforce_new_defaults=False,
+              *args, **kwargs):
+        super().setUp(enforce_scope, enforce_new_defaults, *args, **kwargs)
+        self.controller = volume_metadata.Controller()
+        self.image_controller = image_metadata.VolumeImageMetadataController()
+        self.api_path = '/v3/%s/volumes' % (self.project_id)
+        self.api_version = mv.BASE_VERSION
+
+    def _create_volume(self, image_metadata=None, **kwargs):
+        vol_type = test_utils.create_volume_type(self.project_admin_context,
+                                                 name='fake_vol_type',
+                                                 testcase_instance=self)
+
+        volume = test_utils.create_volume(self.project_member_context,
+                                          volume_type_id=vol_type.id,
+                                          testcase_instance=self, **kwargs)
+
+        for (k, v) in (image_metadata.items() if image_metadata else []):
+            db.volume_glance_metadata_create(self.project_admin_context,
+                                             volume.id, k, v)
+        return volume
+
+    @ddt.data(*base.all_users)
+    def test_get_policy(self, user_id):
+        volume = self._create_volume()
+        rule_name = policy.GET_POLICY
+        url = '%s/%s/metadata' % (self.api_path, volume.id)
+        req = fake_api.HTTPRequest.blank(url, version=self.api_version)
+
+        self.common_policy_check(user_id, self.authorized_readers,
+                                 self.unauthorized_readers,
+                                 self.unauthorized_exceptions,
+                                 rule_name, self.controller.index, req,
+                                 volume_id=volume.id)
+
+    @ddt.data(*base.all_users)
+    def test_create_policy(self, user_id):
+        volume = self._create_volume()
+        rule_name = policy.CREATE_POLICY
+        url = '%s/%s/metadata' % (self.api_path, volume.id)
+        req = fake_api.HTTPRequest.blank(url, version=self.api_version)
+        req.method = 'POST'
+        body = {
+            "metadata": {
+                "name": "metadata0"
+            }
+        }
+
+        self.common_policy_check(user_id, self.authorized_members,
+                                 self.unauthorized_members,
+                                 self.unauthorized_exceptions,
+                                 rule_name, self.controller.create, req,
+                                 volume_id=volume.id, body=body)
+
+    @ddt.data(*base.all_users)
+    def test_update_policy(self, user_id):
+        volume = self._create_volume(metadata={"foo": "bar"})
+        rule_name = policy.UPDATE_POLICY
+        url = '%s/%s/metadata' % (self.api_path, volume.id)
+        req = fake_api.HTTPRequest.blank(url, version=self.api_version)
+        req.method = 'PUT'
+        body = {
+            # Not sure why, but the API code expects the body to contain
+            # a "meta" (not "metadata") dict.
+            "meta": {
+                "foo": "zap"
+            }
+        }
+
+        self.common_policy_check(user_id, self.authorized_members,
+                                 self.unauthorized_members,
+                                 self.unauthorized_exceptions,
+                                 rule_name, self.controller.update, req,
+                                 volume_id=volume.id, id='foo', body=body)
+
+    @ddt.data(*base.all_users)
+    def test_delete_policy(self, user_id):
+        volume = self._create_volume(metadata={"foo": "bar"})
+        rule_name = policy.DELETE_POLICY
+        url = '%s/%s/metadata/foo' % (self.api_path, volume.id)
+        req = fake_api.HTTPRequest.blank(url, version=self.api_version)
+        req.method = 'DELETE'
+
+        # Relax the GET_POLICY in order to get past that check.
+        self.policy.set_rules({policy.GET_POLICY: ""},
+                              overwrite=False)
+
+        self.common_policy_check(user_id, self.authorized_members,
+                                 self.unauthorized_members,
+                                 self.unauthorized_exceptions,
+                                 rule_name, self.controller.delete, req,
+                                 volume_id=volume.id, id='foo')
+
+    @ddt.data(*base.all_users)
+    def test_image_metadata_show_policy(self, user_id):
+        image_metadata = {
+            "up": "down",
+            "left": "right"
+        }
+        volume = self._create_volume(image_metadata)
+        volume = volume.obj_to_primitive()['versioned_object.data']
+        rule_name = policy.IMAGE_METADATA_SHOW_POLICY
+        url = '%s/%s' % (self.api_path, volume['id'])
+        req = fake_api.HTTPRequest.blank(url, version=self.api_version)
+        req.get_db_volume = mock.MagicMock()
+        req.get_db_volume.return_value = volume
+        resp_obj = mock.MagicMock(obj={'volume': volume})
+
+        self.assertNotIn('volume_image_metadata', volume.keys())
+
+        self.common_policy_check(user_id, self.authorized_readers,
+                                 self.unauthorized_readers,
+                                 self.unauthorized_exceptions,
+                                 rule_name,
+                                 self.image_controller.show, req,
+                                 resp_obj, id=volume['id'], fatal=False)
+
+        if user_id in self.authorized_readers:
+            self.assertDictEqual(image_metadata,
+                                 volume['volume_image_metadata'])
+
+    @ddt.data(*base.all_users)
+    def test_image_metadata_set_policy(self, user_id):
+        volume = self._create_volume()
+        rule_name = policy.IMAGE_METADATA_SET_POLICY
+        url = '%s/%s/action' % (self.api_path, volume.id)
+        req = fake_api.HTTPRequest.blank(url, version=self.api_version)
+        req.method = 'POST'
+        body = {
+            "os-set_image_metadata": {
+                "metadata": {
+                    "image_name": "my_image",
+                }
+            }
+        }
+
+        self.common_policy_check(user_id, self.authorized_members,
+                                 self.unauthorized_members,
+                                 self.unauthorized_exceptions,
+                                 rule_name,
+                                 self.image_controller.create, req,
+                                 id=volume.id, body=body)
+
+    @ddt.data(*base.all_users)
+    def test_image_metadata_remove_policy(self, user_id):
+        volume = self._create_volume(image_metadata={"foo": "bar"})
+        rule_name = policy.IMAGE_METADATA_REMOVE_POLICY
+        url = '%s/%s/action' % (self.api_path, volume.id)
+        req = fake_api.HTTPRequest.blank(url, version=self.api_version)
+        req.method = 'POST'
+        body = {
+            "os-unset_image_metadata": {
+                "key": "foo"
+            }
+        }
+
+        self.common_policy_check(user_id, self.authorized_members,
+                                 self.unauthorized_members,
+                                 self.unauthorized_exceptions,
+                                 rule_name,
+                                 self.image_controller.delete, req,
+                                 id=volume.id, body=body)
+
+    # NOTE(abishop):
+    # The following code is a work in progress, and work is deferred until
+    # Yoga. This is because the UPDATE_ADMIN_METADATA_POLICY rule is
+    # unchanged in Xena (it's RULE_ADMIN_API). This test will be necessary
+    # when RULE_ADMIN_API is deprecated in Yoga.
+    #
+    # @ddt.data(*base.all_users)
+    # def test_update_admin_metadata_policy(self, user_id):
+    #     volume = self._create_volume()
+    #     rule_name = policy.UPDATE_ADMIN_METADATA_POLICY
+    #     url = '%s/%s/action' % (self.api_path, volume.id)
+    #     req = fake_api.HTTPRequest.blank(url, version=self.api_version)
+    #     req.method = 'POST'
+    #     body = {
+    #         "os-update_readonly_flag": {
+    #             "readonly": True
+    #         }
+    #     }
+    #
+    #     # Only this test needs a VolumeActionsController
+    #     ext_mgr = extensions.ExtensionManager()
+    #     controller = volume_actions.VolumeActionsController(ext_mgr)
+    #
+    #     # Relax the UPDATE_READONLY_POLICY in order to get past that check.
+    #     self.policy.set_rules({va_policy.UPDATE_READONLY_POLICY: ""},
+    #                           overwrite=False)
+    #
+    #     self.common_policy_check(user_id, self.authorized_admins,
+    #                              self.unauthorized_admins,
+    #                              self.unauthorized_exceptions,
+    #                              rule_name,
+    #                              controller._volume_readonly_update, req,
+    #                              id=volume.id, body=body)
+
+
+class VolumeMetadataPolicySecureRbacTest(VolumeMetadataPolicyTest):
+    authorized_readers = [
+        'legacy_admin',
+        'system_admin',
+        'project_admin',
+        'project_member',
+        'project_reader',
+    ]
+
+    unauthorized_readers = [
+        'legacy_owner',
+        'system_member',
+        'system_reader',
+        'system_foo',
+        'project_foo',
+        'other_project_member',
+        'other_project_reader',
+    ]
+
+    authorized_members = [
+        'legacy_admin',
+        'system_admin',
+        'project_admin',
+        'project_member',
+    ]
+
+    unauthorized_members = [
+        'legacy_owner',
+        'system_member',
+        'system_reader',
+        'system_foo',
+        'project_reader',
+        'project_foo',
+        'other_project_member',
+        'other_project_reader',
+    ]
+
+    def setUp(self, *args, **kwargs):
+        # Test secure RBAC by disabling deprecated policy rules (scope
+        # is still not enabled).
+        super().setUp(enforce_scope=False, enforce_new_defaults=True,
+                      *args, **kwargs)
+
+
+class VolumePolicyTests(test_base.CinderPolicyTests):
     def test_admin_can_get_metadata(self):
         admin_context = self.admin_context