From c806c74c80042d0504d26dc73a5d4b9a3c25ff7c Mon Sep 17 00:00:00 2001 From: Alan Bishop Date: Wed, 15 Sep 2021 08:29:05 -0700 Subject: [PATCH] Update policies related to user visible extra specs The policies for volume types was updated in [1][2] in order to implement project personas related to secure RBAC. However, that work did not take into account the new user visible extra specs feature [3]. This patch updates the policies related to [3] so that they now implement the proper project personas. [1] Id5ea2c81bedf51f3e0b4c5cb4ef569f255143a9e [2] I52de98d80e80e2c315c6e329a9ead6f83713eca2 [3] I5434ea4199cce8158b75771fb6127be001baf328 Closes-Bug: #1943733 Change-Id: I16ca60717dfc39a4b92300c3f4a88c402b65ff3c --- cinder/policies/type_extra_specs.py | 34 ++- cinder/policies/volume_type.py | 10 +- .../api/contrib/test_types_extra_specs.py | 60 ++-- .../unit/api/contrib/test_types_manage.py | 57 ++-- .../unit/policies/test_type_extra_specs.py | 285 ++++++++++++++++++ .../tests/unit/policies/test_volume_type.py | 50 ++- cinder/tests/unit/policy.yaml | 27 -- .../block-storage/policy-personas.rst | 53 ++-- 8 files changed, 479 insertions(+), 97 deletions(-) create mode 100644 cinder/tests/unit/policies/test_type_extra_specs.py diff --git a/cinder/policies/type_extra_specs.py b/cinder/policies/type_extra_specs.py index 206d20c1710..de97dab184b 100644 --- a/cinder/policies/type_extra_specs.py +++ b/cinder/policies/type_extra_specs.py @@ -31,17 +31,29 @@ GET_POLICY = "volume_extension:types_extra_specs:show" READ_SENSITIVE_POLICY = "volume_extension:types_extra_specs:read_sensitive" UPDATE_POLICY = "volume_extension:types_extra_specs:update" +deprecated_get_all_policy = base.CinderDeprecatedRule( + name=GET_ALL_POLICY, + check_str="" +) + +deprecated_get_policy = base.CinderDeprecatedRule( + name=GET_POLICY, + check_str="" +) + type_extra_specs_policies = [ policy.DocumentedRuleDefault( name=GET_ALL_POLICY, - check_str="", + check_str=base.SYSTEM_READER_OR_PROJECT_READER, description="List type extra specs.", operations=[ { 'method': 'GET', 'path': '/types/{type_id}/extra_specs' } - ]), + ], + deprecated_rule=deprecated_get_all_policy, + ), policy.DocumentedRuleDefault( name=CREATE_POLICY, check_str=base.RULE_ADMIN_API, @@ -51,17 +63,20 @@ type_extra_specs_policies = [ 'method': 'POST', 'path': '/types/{type_id}/extra_specs' } - ]), + ] + ), policy.DocumentedRuleDefault( name=GET_POLICY, - check_str="", + check_str=base.SYSTEM_READER_OR_PROJECT_READER, description="Show one specified type extra specs.", operations=[ { 'method': 'GET', 'path': '/types/{type_id}/extra_specs/{extra_spec_key}' } - ]), + ], + deprecated_rule=deprecated_get_policy, + ), policy.DocumentedRuleDefault( name=READ_SENSITIVE_POLICY, check_str=base.RULE_ADMIN_API, @@ -87,7 +102,8 @@ type_extra_specs_policies = [ 'method': 'GET', 'path': '/types/{type_id}/extra_specs/{extra_spec_key}' } - ]), + ] + ), policy.DocumentedRuleDefault( name=UPDATE_POLICY, check_str=base.RULE_ADMIN_API, @@ -97,7 +113,8 @@ type_extra_specs_policies = [ 'method': 'PUT', 'path': '/types/{type_id}/extra_specs/{extra_spec_key}' } - ]), + ] + ), policy.DocumentedRuleDefault( name=DELETE_POLICY, check_str=base.RULE_ADMIN_API, @@ -107,7 +124,8 @@ type_extra_specs_policies = [ 'method': 'DELETE', 'path': '/types/{type_id}/extra_specs/{extra_spec_key}' } - ]), + ] + ), ] diff --git a/cinder/policies/volume_type.py b/cinder/policies/volume_type.py index 76676e6aef4..f3bff66ce7b 100644 --- a/cinder/policies/volume_type.py +++ b/cinder/policies/volume_type.py @@ -50,6 +50,10 @@ deprecated_manage_policy = base.CinderDeprecatedRule( 'policies that separately govern POST, PUT, and DELETE ' 'operations.'), ) +deprecated_extra_spec_policy = base.CinderDeprecatedRule( + name=EXTRA_SPEC_POLICY, + check_str=base.RULE_ADMIN_API +) deprecated_encryption_create_policy = base.CinderDeprecatedRule( name=CREATE_ENCRYPTION_POLICY, # TODO: change to base.RULE_ADMIN_API in Yoga & remove dep_reason @@ -150,7 +154,7 @@ volume_type_policies = [ ), policy.DocumentedRuleDefault( name=EXTRA_SPEC_POLICY, - check_str=base.RULE_ADMIN_API, + check_str=base.SYSTEM_READER_OR_PROJECT_READER, description=( "Include the volume type's extra_specs attribute in the volume " "type list or show requests. The ability to make these calls " @@ -164,7 +168,9 @@ volume_type_policies = [ 'method': 'GET', 'path': '/types' } - ]), + ], + deprecated_rule=deprecated_extra_spec_policy, + ), policy.DocumentedRuleDefault( name=QOS_POLICY, check_str=base.RULE_ADMIN_API, diff --git a/cinder/tests/unit/api/contrib/test_types_extra_specs.py b/cinder/tests/unit/api/contrib/test_types_extra_specs.py index bcb43df392a..df3eed23c76 100644 --- a/cinder/tests/unit/api/contrib/test_types_extra_specs.py +++ b/cinder/tests/unit/api/contrib/test_types_extra_specs.py @@ -132,7 +132,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): self.mock_object(cinder.db, 'volume_type_extra_specs_delete') self.assertEqual(0, len(self.notifier.notifications)) - req = fakes.HTTPRequest.blank(self.api_path + '/key5') + req = fakes.HTTPRequest.blank(self.api_path + '/key5', + use_admin_context=True) self.controller.delete(req, fake.VOLUME_ID, 'key5') self.assertEqual(1, len(self.notifier.notifications)) self.assertIn('created_at', self.notifier.notifications[0]['payload']) @@ -144,7 +145,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): side_effect=exception.VolumeTypeExtraSpecsNotFound( "Not Found")) - req = fakes.HTTPRequest.blank(self.api_path + '/key6') + req = fakes.HTTPRequest.blank(self.api_path + '/key6', + use_admin_context=True) self.assertRaises(exception.VolumeTypeExtraSpecsNotFound, self.controller.delete, req, fake.VOLUME_ID, 'key6') @@ -155,7 +157,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): body = {"extra_specs": {"key1": "value1"}} self.assertEqual(0, len(self.notifier.notifications)) - req = fakes.HTTPRequest.blank(self.api_path) + req = fakes.HTTPRequest.blank(self.api_path, + use_admin_context=True) res_dict = self.controller.create(req, fake.VOLUME_ID, body=body) self.assertEqual(1, len(self.notifier.notifications)) self.assertIn('created_at', self.notifier.notifications[0]['payload']) @@ -179,7 +182,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): body = {"extra_specs": {"image_service:store_id": "cheap"}} self.assertEqual(0, len(self.notifier.notifications)) - req = fakes.HTTPRequest.blank(self.api_path) + req = fakes.HTTPRequest.blank(self.api_path, + use_admin_context=True) res_dict = self.controller.create(req, fake.VOLUME_ID, body=body) self.assertEqual(1, len(self.notifier.notifications)) self.assertIn('created_at', self.notifier.notifications[0]['payload']) @@ -199,7 +203,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): }] } body = {"extra_specs": {"image_service:store_id": "fast"}} - req = fakes.HTTPRequest.blank(self.api_path) + req = fakes.HTTPRequest.blank(self.api_path, + use_admin_context=True) self.assertRaises(cinder.exception.GlanceStoreNotFound, self.controller.create, req, fake.VOLUME_ID, body=body) @@ -216,7 +221,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): }] } body = {"extra_specs": {"image_service:store_id": "read_only_store"}} - req = fakes.HTTPRequest.blank(self.api_path) + req = fakes.HTTPRequest.blank(self.api_path, + use_admin_context=True) self.assertRaises(cinder.exception.GlanceStoreReadOnly, self.controller.create, req, fake.VOLUME_ID, body=body) @@ -236,7 +242,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): self.assertEqual(0, len(self.notifier.notifications)) - req = fakes.HTTPRequest.blank(self.api_path) + req = fakes.HTTPRequest.blank(self.api_path, + use_admin_context=True) res_dict = self.controller.create(req, fake.VOLUME_ID, body=body) self.assertEqual(1, len(self.notifier.notifications)) self.assertEqual('value1', @@ -259,7 +266,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): self.assertEqual(0, len(self.notifier.notifications)) - req = fakes.HTTPRequest.blank(self.api_path) + req = fakes.HTTPRequest.blank(self.api_path, + use_admin_context=True) res_dict = self.controller.create(req, fake.VOLUME_ID, body=body) self.assertEqual(1, len(self.notifier.notifications)) self.assertEqual('value1', @@ -290,7 +298,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): self.assertEqual(0, len(self.notifier.notifications)) req = fakes.HTTPRequest.blank( - self.api_path + "/image_service:store_id") + self.api_path + "/image_service:store_id", + use_admin_context=True) res_dict = self.controller.update(req, fake.VOLUME_ID, "image_service:store_id", body=body) @@ -321,7 +330,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): self.assertEqual(0, len(self.notifier.notifications)) req = fakes.HTTPRequest.blank( - self.api_path + "/image_service:store_id") + self.api_path + "/image_service:store_id", + use_admin_context=True) self.assertRaises(cinder.exception.GlanceStoreNotFound, self.controller.update, req, fake.VOLUME_ID, @@ -349,7 +359,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): self.assertEqual(0, len(self.notifier.notifications)) req = fakes.HTTPRequest.blank( - self.api_path + "/image_service:store_id") + self.api_path + "/image_service:store_id", + use_admin_context=True) self.assertRaises(cinder.exception.GlanceStoreReadOnly, self.controller.update, req, fake.VOLUME_ID, @@ -363,7 +374,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): body = {"key1": "value1"} self.assertEqual(0, len(self.notifier.notifications)) - req = fakes.HTTPRequest.blank(self.api_path + '/key1') + req = fakes.HTTPRequest.blank(self.api_path + '/key1', + use_admin_context=True) res_dict = self.controller.update(req, fake.VOLUME_ID, 'key1', body=body) self.assertEqual(1, len(self.notifier.notifications)) @@ -378,7 +390,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): return_create_volume_type_extra_specs) body = {"key1": "value1", "key2": "value2"} - req = fakes.HTTPRequest.blank(self.api_path + '/key1') + req = fakes.HTTPRequest.blank(self.api_path + '/key1', + use_admin_context=True) self.assertRaises(exception.ValidationError, self.controller.update, req, fake.VOLUME_ID, 'key1', body=body) @@ -388,13 +401,15 @@ class VolumeTypesExtraSpecsTest(test.TestCase): return_create_volume_type_extra_specs) body = {"key1": "value1"} - req = fakes.HTTPRequest.blank(self.api_path + '/bad') + req = fakes.HTTPRequest.blank(self.api_path + '/bad', + use_admin_context=True) self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update, req, fake.VOLUME_ID, 'bad', body=body) def _extra_specs_empty_update(self, body): req = fakes.HTTPRequest.blank('/v3/%s/types/%s/extra_specs' % ( - fake.PROJECT_ID, fake.VOLUME_TYPE_ID)) + fake.PROJECT_ID, fake.VOLUME_TYPE_ID), + use_admin_context=True) req.method = 'POST' self.assertRaises(exception.ValidationError, @@ -409,7 +424,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): def _extra_specs_create_bad_body(self, body): req = fakes.HTTPRequest.blank('/v3/%s/types/%s/extra_specs' % ( - fake.PROJECT_ID, fake.VOLUME_TYPE_ID)) + fake.PROJECT_ID, fake.VOLUME_TYPE_ID), + use_admin_context=True) req.method = 'POST' self.assertRaises(exception.ValidationError, @@ -440,17 +456,20 @@ class VolumeTypesExtraSpecsTest(test.TestCase): 'volume_type_extra_specs_update_or_create', return_create_volume_type_extra_specs) body = {"extra_specs": {"key1": "value1"}} - req = fakes.HTTPRequest.blank(self.api_path) + req = fakes.HTTPRequest.blank(self.api_path, + use_admin_context=True) with mock.patch.object( cinder.db, 'volume_get_all', return_value=['a']): req = fakes.HTTPRequest.blank('/v3/%s/types/%s/extra_specs' % ( - fake.PROJECT_ID, fake.VOLUME_TYPE_ID)) + fake.PROJECT_ID, fake.VOLUME_TYPE_ID), + use_admin_context=True) req.method = 'POST' body = {"extra_specs": {"key1": "value1"}} - req = fakes.HTTPRequest.blank(self.api_path) + req = fakes.HTTPRequest.blank(self.api_path, + use_admin_context=True) self.assertRaises(webob.exc.HTTPBadRequest, self.controller.create, req, @@ -462,7 +481,8 @@ class VolumeTypesExtraSpecsTest(test.TestCase): {'extra_specs': {' ': 'a'}}) def test_create_with_invalid_extra_specs(self, body): req = fakes.HTTPRequest.blank('/v3/%s/types/%s/extra_specs' % ( - fake.PROJECT_ID, fake.VOLUME_TYPE_ID)) + fake.PROJECT_ID, fake.VOLUME_TYPE_ID), + use_admin_context=True) req.method = 'POST' self.assertRaises(exception.ValidationError, diff --git a/cinder/tests/unit/api/contrib/test_types_manage.py b/cinder/tests/unit/api/contrib/test_types_manage.py index 9ded23a57e8..2482f8ae622 100644 --- a/cinder/tests/unit/api/contrib/test_types_manage.py +++ b/cinder/tests/unit/api/contrib/test_types_manage.py @@ -151,7 +151,8 @@ class VolumeTypesManageApiTest(test.TestCase): return_volume_types_destroy) req = fakes.HTTPRequest.blank('/v3/%s/types/%s' % ( - fake.PROJECT_ID, DEFAULT_VOLUME_TYPE)) + fake.PROJECT_ID, DEFAULT_VOLUME_TYPE), + use_admin_context=True) self.assertEqual(0, len(self.notifier.notifications)) self.controller._delete(req, DEFAULT_VOLUME_TYPE) self.assertEqual(1, len(self.notifier.notifications)) @@ -164,7 +165,8 @@ class VolumeTypesManageApiTest(test.TestCase): self.assertEqual(0, len(self.notifier.notifications)) req = fakes.HTTPRequest.blank('/v3/%s/types/%s' % ( - fake.PROJECT_ID, NOT_FOUND_VOLUME_TYPE)) + fake.PROJECT_ID, NOT_FOUND_VOLUME_TYPE), + use_admin_context=True) self.assertRaises(exception.VolumeTypeNotFound, self.controller._delete, req, NOT_FOUND_VOLUME_TYPE) self.assertEqual(1, len(self.notifier.notifications)) @@ -175,7 +177,8 @@ class VolumeTypesManageApiTest(test.TestCase): self.mock_object(volume_types, 'destroy', return_volume_types_with_volumes_destroy) req = fakes.HTTPRequest.blank('/v3/%s/types/%s' % ( - fake.PROJECT_ID, DEFAULT_VOLUME_TYPE)) + fake.PROJECT_ID, DEFAULT_VOLUME_TYPE), + use_admin_context=True) self.assertEqual(0, len(self.notifier.notifications)) self.controller._delete(req, DEFAULT_VOLUME_TYPE) self.assertEqual(1, len(self.notifier.notifications)) @@ -217,7 +220,8 @@ class VolumeTypesManageApiTest(test.TestCase): body = {"volume_type": {"name": "vol_type_1", "os-volume-type-access:is_public": True, "extra_specs": {"key1": "value1"}}} - req = fakes.HTTPRequest.blank('/v3/%s/types' % fake.PROJECT_ID) + req = fakes.HTTPRequest.blank('/v3/%s/types' % fake.PROJECT_ID, + use_admin_context=True) self.assertEqual(0, len(self.notifier.notifications)) res_dict = self.controller._create(req, body=body) @@ -242,7 +246,8 @@ class VolumeTypesManageApiTest(test.TestCase): body = {"volume_type": {"name": "vol_type_1", "description": type_description, "extra_specs": {"key1": "value1"}}} - req = fakes.HTTPRequest.blank('/v3/%s/types' % fake.PROJECT_ID) + req = fakes.HTTPRequest.blank('/v3/%s/types' % fake.PROJECT_ID, + use_admin_context=True) res_dict = self.controller._create(req, body=body) @@ -274,7 +279,8 @@ class VolumeTypesManageApiTest(test.TestCase): body = {"volume_type": {"name": "vol_type_1", "extra_specs": {"key1": "value1"}}} - req = fakes.HTTPRequest.blank('/v3/%s/types' % fake.PROJECT_ID) + req = fakes.HTTPRequest.blank('/v3/%s/types' % fake.PROJECT_ID, + use_admin_context=True) self.assertRaises(webob.exc.HTTPConflict, self.controller._create, req, body=body) @@ -382,7 +388,8 @@ class VolumeTypesManageApiTest(test.TestCase): DEFAULT_VOLUME_TYPE, is_public=False) body = {"volume_type": {"is_public": False}} req = fakes.HTTPRequest.blank('/v3/%s/types/%s' % ( - fake.PROJECT_ID, DEFAULT_VOLUME_TYPE)) + fake.PROJECT_ID, DEFAULT_VOLUME_TYPE), + use_admin_context=True) req.method = 'PUT' self.assertEqual(0, len(self.notifier.notifications)) @@ -407,7 +414,8 @@ class VolumeTypesManageApiTest(test.TestCase): mock_get, mock_update): body = {"volume_type": {"is_public": is_public}} req = fakes.HTTPRequest.blank('/v3/%s/types/%s' % ( - fake.PROJECT_ID, DEFAULT_VOLUME_TYPE)) + fake.PROJECT_ID, DEFAULT_VOLUME_TYPE), + use_admin_context=True) req.method = 'PUT' ctxt = context.RequestContext(fake.USER_ID, fake.PROJECT_ID, True) req.environ['cinder.context'] = ctxt @@ -430,7 +438,8 @@ class VolumeTypesManageApiTest(test.TestCase): type_description = "" body = {"volume_type": {"description": type_description}} req = fakes.HTTPRequest.blank('/v3/%s/types/%s' % ( - fake.PROJECT_ID, DEFAULT_VOLUME_TYPE)) + fake.PROJECT_ID, DEFAULT_VOLUME_TYPE), + use_admin_context=True) req.method = 'PUT' resp = self.controller._update(req, DEFAULT_VOLUME_TYPE, body=body) self._check_test_results(resp, @@ -466,7 +475,8 @@ class VolumeTypesManageApiTest(test.TestCase): body = {"volume_type": {"name": "vol_type_1_1", "description": "vol_type_desc_1_1"}} req = fakes.HTTPRequest.blank('/v3/%s/types/%s' % ( - fake.PROJECT_ID, NOT_FOUND_VOLUME_TYPE)) + fake.PROJECT_ID, NOT_FOUND_VOLUME_TYPE), + use_admin_context=True) req.method = 'PUT' self.assertEqual(0, len(self.notifier.notifications)) @@ -486,7 +496,8 @@ class VolumeTypesManageApiTest(test.TestCase): body = {"volume_type": {"name": "vol_type_1_1", "description": "vol_type_desc_1_1"}} req = fakes.HTTPRequest.blank('/v3/%s/types/%s' % ( - fake.PROJECT_ID, DEFAULT_VOLUME_TYPE)) + fake.PROJECT_ID, DEFAULT_VOLUME_TYPE), + use_admin_context=True) req.method = 'PUT' self.assertEqual(0, len(self.notifier.notifications)) @@ -498,7 +509,8 @@ class VolumeTypesManageApiTest(test.TestCase): def test_update_no_name_no_description(self): body = {"volume_type": {}} req = fakes.HTTPRequest.blank('/v3/%s/types/%s' % ( - fake.PROJECT_ID, DEFAULT_VOLUME_TYPE)) + fake.PROJECT_ID, DEFAULT_VOLUME_TYPE), + use_admin_context=True) req.method = 'PUT' self.assertRaises(webob.exc.HTTPBadRequest, @@ -509,7 +521,8 @@ class VolumeTypesManageApiTest(test.TestCase): body = {"volume_type": {"name": " ", "description": "something"}} req = fakes.HTTPRequest.blank('/v3/%s/types/%s' % ( - fake.PROJECT_ID, DEFAULT_VOLUME_TYPE)) + fake.PROJECT_ID, DEFAULT_VOLUME_TYPE), + use_admin_context=True) req.method = 'PUT' self.assertRaises(webob.exc.HTTPBadRequest, @@ -554,7 +567,8 @@ class VolumeTypesManageApiTest(test.TestCase): updated_desc = "%s_%s" % (desc, UPDATE_DESC_ONLY_TYPE) body = {"volume_type": {"description": updated_desc}} req = fakes.HTTPRequest.blank('/v3/%s/types/%s' % ( - fake.PROJECT_ID, UPDATE_DESC_ONLY_TYPE)) + fake.PROJECT_ID, UPDATE_DESC_ONLY_TYPE), + use_admin_context=True) req.method = 'PUT' self.assertEqual(0, len(self.notifier.notifications)) @@ -577,7 +591,8 @@ class VolumeTypesManageApiTest(test.TestCase): updated_desc = "%s_%s" % (desc, DEFAULT_VOLUME_TYPE) body = {"volume_type": {"is_public": is_public}} req = fakes.HTTPRequest.blank('/v3/%s/types/%s' % ( - fake.PROJECT_ID, DEFAULT_VOLUME_TYPE)) + fake.PROJECT_ID, DEFAULT_VOLUME_TYPE), + use_admin_context=True) req.method = 'PUT' self.assertEqual(0, len(self.notifier.notifications)) @@ -614,7 +629,8 @@ class VolumeTypesManageApiTest(test.TestCase): # first attempt fail body = {"volume_type": {"name": name}} req = fakes.HTTPRequest.blank('/v3/%s/types/%s' % ( - fake.PROJECT_ID, UPDATE_NAME_AFTER_DELETE_TYPE)) + fake.PROJECT_ID, UPDATE_NAME_AFTER_DELETE_TYPE), + use_admin_context=True) req.method = 'PUT' self.assertEqual(0, len(self.notifier.notifications)) @@ -629,7 +645,8 @@ class VolumeTypesManageApiTest(test.TestCase): self.mock_object(volume_types, 'destroy', return_volume_types_destroy) req = fakes.HTTPRequest.blank('/v3/%s/types/%s' % ( - fake.PROJECT_ID, UPDATE_NAME_AFTER_DELETE_TYPE)) + fake.PROJECT_ID, UPDATE_NAME_AFTER_DELETE_TYPE), + use_admin_context=True) self.assertEqual(0, len(self.notifier.notifications)) self.controller._delete(req, UPDATE_NAME_AFTER_DELETE_TYPE) self.assertEqual(1, len(self.notifier.notifications)) @@ -638,7 +655,8 @@ class VolumeTypesManageApiTest(test.TestCase): mock_update.side_effect = mock.MagicMock() body = {"volume_type": {"name": updated_name}} req = fakes.HTTPRequest.blank('/v3/%s/types/%s' % ( - fake.PROJECT_ID, UPDATE_NAME_AFTER_DELETE_TYPE)) + fake.PROJECT_ID, UPDATE_NAME_AFTER_DELETE_TYPE), + use_admin_context=True) req.method = 'PUT' self.notifier.reset() @@ -702,7 +720,8 @@ class VolumeTypesManageApiTest(test.TestCase): def test_update_with_name_null(self): body = {"volume_type": {"name": None}} req = fakes.HTTPRequest.blank('/v3/%s/types/%s' % ( - fake.PROJECT_ID, DEFAULT_VOLUME_TYPE)) + fake.PROJECT_ID, DEFAULT_VOLUME_TYPE), + use_admin_context=True) req.method = 'PUT' self.assertRaises(webob.exc.HTTPBadRequest, diff --git a/cinder/tests/unit/policies/test_type_extra_specs.py b/cinder/tests/unit/policies/test_type_extra_specs.py new file mode 100644 index 00000000000..1871d8b178a --- /dev/null +++ b/cinder/tests/unit/policies/test_type_extra_specs.py @@ -0,0 +1,285 @@ +# Copyright (c) 2021 Red Hat, Inc. +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import ddt + +from cinder.api.contrib import types_extra_specs +from cinder.api import microversions as mv +from cinder.api.v3 import types +from cinder.policies import type_extra_specs as policy +from cinder.policies import volume_type as type_policy +from cinder.tests.unit.api import fakes as fake_api +from cinder.tests.unit.policies import base +from cinder.tests.unit import utils as test_utils + + +@ddt.ddt +class TypeExtraSpecsPolicyTest(base.BasePolicyTest): + """Verify extra specs policy settings for the types API""" + + # Deprecated check_str="" allows anyone to read extra specs + authorized_readers = [ + 'legacy_admin', + 'legacy_owner', + 'system_admin', + 'project_admin', + 'project_member', + 'project_reader', + 'project_foo', + 'system_member', + 'system_reader', + 'system_foo', + 'other_project_member', + 'other_project_reader', + ] + + unauthorized_readers = [ + ] + + 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', + ] + + unauthorized_exceptions = [] + + # 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 = types_extra_specs.VolumeTypeExtraSpecsController() + self.api_path = '/v3/%s/types' % (self.project_id) + self.api_version = mv.BASE_VERSION + + @ddt.data(*base.all_users) + def test_get_all_policy(self, user_id): + vol_type = test_utils.create_volume_type(self.project_admin_context, + testcase_instance=self, + name='fake_vol_type', + extra_specs={'foo': 'bar'}) + rule_name = policy.GET_ALL_POLICY + url = '%s/%s/extra_specs' % (self.api_path, vol_type.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, + type_id=vol_type.id) + + @ddt.data(*base.all_users) + def test_get_policy(self, user_id): + vol_type = test_utils.create_volume_type(self.project_admin_context, + testcase_instance=self, + name='fake_vol_type', + extra_specs={'foo': 'bar'}) + rule_name = policy.GET_POLICY + url = '%s/%s/extra_specs/foo' % (self.api_path, vol_type.id) + req = fake_api.HTTPRequest.blank(url, version=self.api_version) + + # Relax the READ_SENSITIVE_POLICY policy so that any user is able + # to "see" the spec. + self.policy.set_rules({policy.READ_SENSITIVE_POLICY: ""}, + overwrite=False) + + self.common_policy_check(user_id, + self.authorized_readers, + self.unauthorized_readers, + self.unauthorized_exceptions, + rule_name, + self.controller.show, + req, + type_id=vol_type.id, + id='foo') + + @ddt.data(*base.all_users) + def test_create_policy(self, user_id): + vol_type = test_utils.create_volume_type(self.project_admin_context, + testcase_instance=self, + name='fake_vol_type') + rule_name = policy.CREATE_POLICY + url = '%s/%s/extra_specs' % (self.api_path, vol_type.id) + req = fake_api.HTTPRequest.blank(url, version=self.api_version) + req.method = 'POST' + body = { + "extra_specs": { + "foo": "bar", + } + } + + self.common_policy_check(user_id, + self.authorized_admins, + self.unauthorized_admins, + self.unauthorized_exceptions, + rule_name, + self.controller.create, + req, + type_id=vol_type.id, + body=body) + + @ddt.data(*base.all_users) + def test_update_policy(self, user_id): + vol_type = test_utils.create_volume_type(self.project_admin_context, + testcase_instance=self, + name='fake_vol_type', + extra_specs={'foo': 'bar'}) + rule_name = policy.UPDATE_POLICY + url = '%s/%s/extra_specs/foo' % (self.api_path, vol_type.id) + req = fake_api.HTTPRequest.blank(url, version=self.api_version) + req.method = 'PUT' + body = {"foo": "zap"} + + self.common_policy_check(user_id, + self.authorized_admins, + self.unauthorized_admins, + self.unauthorized_exceptions, + rule_name, + self.controller.update, + req, + type_id=vol_type.id, + id='foo', + body=body) + + @ddt.data(*base.all_users) + def test_delete_policy(self, user_id): + vol_type = test_utils.create_volume_type(self.project_admin_context, + testcase_instance=self, + name='fake_vol_type', + extra_specs={'foo': 'bar'}) + rule_name = policy.DELETE_POLICY + url = '%s/%s/extra_specs/foo' % (self.api_path, vol_type.id) + req = fake_api.HTTPRequest.blank(url, version=self.api_version) + req.method = 'DELETE' + + self.common_policy_check(user_id, + self.authorized_admins, + self.unauthorized_admins, + self.unauthorized_exceptions, + rule_name, + self.controller.delete, + req, + type_id=vol_type.id, + id='foo') + + @ddt.data(*base.all_users) + def test_read_sensitive_policy(self, user_id): + # The 'multiattach' extra spec is user visible, and the + # 'sensitive' extra spec should not be user visible. + extra_specs = { + 'multiattach': ' True', + 'sensitive': 'secret', + } + vol_type = test_utils.create_volume_type(self.project_admin_context, + testcase_instance=self, + name='fake_vol_type', + extra_specs=extra_specs) + rule_name = policy.READ_SENSITIVE_POLICY + url = '%s/%s' % (self.api_path, vol_type.id) + req = fake_api.HTTPRequest.blank(url, version=self.api_version) + + # Relax these policies in order to get past those checks. + self.policy.set_rules({type_policy.GET_POLICY: ""}, + overwrite=False) + self.policy.set_rules({type_policy.EXTRA_SPEC_POLICY: ""}, + overwrite=False) + + # With the relaxed policies, all users are authorized because + # failing the READ_SENSITIVE_POLICY policy check is not fatal. + authorized_users = [user_id] + unauthorized_users = [] + + controller = types.VolumeTypesController() + response = self.common_policy_check(user_id, + authorized_users, + unauthorized_users, + self.unauthorized_exceptions, + rule_name, + controller.show, + req, + id=vol_type.id) + + if user_id in self.authorized_admins: + # Admins should see all extra specs + expected = extra_specs + else: + # Non-admins should only see user visible extra specs + expected = {'multiattach': ' True'} + self.assertDictEqual(expected, response['volume_type']['extra_specs']) + + +class TypeExtraSpecsPolicySecureRbacTest(TypeExtraSpecsPolicyTest): + authorized_readers = [ + 'legacy_admin', + 'system_admin', + 'project_admin', + 'project_member', + 'project_reader', + 'system_member', + 'system_reader', + 'other_project_member', + 'other_project_reader', + ] + + unauthorized_readers = [ + # These are unauthorized because they don't have the reader role + 'legacy_owner', + 'project_foo', + 'system_foo', + ] + + # NOTE(Xena): The authorized_admins and unauthorized_admins are the same + # as the TypeExtraSpecsPolicyTest. This is because in Xena the "admin only" + # rules are the legacy RULE_ADMIN_API. This will change in Yoga, when + # RULE_ADMIN_API will be deprecated in favor of the SYSTEM_ADMIN rule that + # is scope based. + 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', + ] + + 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) diff --git a/cinder/tests/unit/policies/test_volume_type.py b/cinder/tests/unit/policies/test_volume_type.py index 700bdcee0a1..cd2fc7e9f18 100644 --- a/cinder/tests/unit/policies/test_volume_type.py +++ b/cinder/tests/unit/policies/test_volume_type.py @@ -25,6 +25,7 @@ from cinder.tests.unit.api import fakes as fake_api from cinder.tests.unit import fake_constants from cinder.tests.unit.policies import base from cinder.tests.unit.policies import test_base +from cinder.tests.unit import utils as test_utils @ddt.ddt @@ -75,9 +76,11 @@ class VolumeTypePolicyTest(base.BasePolicyTest): @ddt.data(*base.all_users) def test_type_get_policy(self, user_id): + vol_type = test_utils.create_volume_type(self.project_admin_context, + testcase_instance=self, + name='fake_vol_type') rule_name = type_policy.GET_POLICY - # the default type is guaranteed to exist - url = self.api_path + '/default' + url = '%s/%s' % (self.api_path, vol_type.id) req = fake_api.HTTPRequest.blank(url, version=self.api_version) self.common_policy_check(user_id, self.authorized_readers, @@ -86,7 +89,48 @@ class VolumeTypePolicyTest(base.BasePolicyTest): rule_name, self.controller.show, req, - 'default') + id=vol_type.id) + + @ddt.data(*base.all_users) + def test_extra_spec_policy(self, user_id): + vol_type = test_utils.create_volume_type( + self.project_admin_context, + testcase_instance=self, + name='fake_vol_type', + extra_specs={'multiattach': ' True'}) + rule_name = type_policy.EXTRA_SPEC_POLICY + url = '%s/%s' % (self.api_path, vol_type.id) + req = fake_api.HTTPRequest.blank(url, version=self.api_version) + + # Relax the GET_POLICY in order to get past that check. + self.policy.set_rules({type_policy.GET_POLICY: ""}, + overwrite=False) + + # With the relaxed GET_POLICY, all users are authorized because + # failing the policy check is not fatal. + authorized_readers = [user_id] + unauthorized_readers = [] + + response = self.common_policy_check(user_id, + authorized_readers, + unauthorized_readers, + self.unauthorized_exceptions, + rule_name, + self.controller.show, + req, + id=vol_type.id) + + # Check whether the response should contain extra_specs. The logic + # is a little unusual: + # - The new rule is SYSTEM_READER_OR_PROJECT_READER (i.e. users + # with the 'reader' role) + # - The deprecated rule is RULE_ADMIN_API (i.e. users with the + # 'admin' role) + context = self.create_context(user_id) + if 'reader' in context.roles or 'admin' in context.roles: + self.assertIn('extra_specs', response['volume_type']) + else: + self.assertNotIn('extra_specs', response['volume_type']) class VolumeTypePolicySecureRbacTest(VolumeTypePolicyTest): diff --git a/cinder/tests/unit/policy.yaml b/cinder/tests/unit/policy.yaml index 5c27e7d4069..47234f669d0 100644 --- a/cinder/tests/unit/policy.yaml +++ b/cinder/tests/unit/policy.yaml @@ -16,30 +16,3 @@ # enable, set-log and get-log actions. # PUT /os-services/{action} #"volume_extension:services:update": "rule:admin_api" - -# Create, update and delete volume type. -# POST /types -# PUT /types -# DELETE /types -"volume_extension:types_manage": "" - -# List type extra specs. -# GET /types/{type_id}/extra_specs -"volume_extension:types_extra_specs:index": "" - -# Create type extra specs. -# POST /types/{type_id}/extra_specs -"volume_extension:types_extra_specs:create": "" - -# Show one specified type extra specs. -# GET /types/{type_id}/extra_specs/{extra_spec_key} -"volume_extension:types_extra_specs:show": "" - -# Update type extra specs. -# PUT /types/{type_id}/extra_specs/{extra_spec_key} -"volume_extension:types_extra_specs:update": "" - -# Delete type extra specs. -# DELETE /types/{type_id}/extra_specs/{extra_spec_key} -"volume_extension:types_extra_specs:delete": "" - diff --git a/doc/source/configuration/block-storage/policy-personas.rst b/doc/source/configuration/block-storage/policy-personas.rst index d18955ce89e..28e11a27d4d 100644 --- a/doc/source/configuration/block-storage/policy-personas.rst +++ b/doc/source/configuration/block-storage/policy-personas.rst @@ -1693,13 +1693,13 @@ matrix is validated by human beings. | ``GET /types`` | The ability to make these API calls is governed by other policies. - volume_extension:access_types_extra_specs - - rule:admin_api - - no - - no - - no - - no + - empty + - yes + - yes + - yes + - yes + - yes - yes - - no - yes * - List or show volume type with access type qos specs id attribute - | Adds ``qos_specs_id`` to the following responses: @@ -2237,13 +2237,13 @@ matrix is validated by human beings. * - List type extra specs - ``GET /types/{type_id}/extra_specs`` - volume_extension:types_extra_specs:index - - rule:admin_api - - no - - no - - no - - no + - empty + - yes + - yes + - yes + - yes + - yes - yes - - no - yes * - Create type extra specs - ``POST /types/{type_id}/extra_specs`` @@ -2259,13 +2259,13 @@ matrix is validated by human beings. * - Show one specified type extra specs - ``GET /types/{type_id}/extra_specs/{extra_spec_key}`` - volume_extension:types_extra_specs:show - - rule:admin_api - - no - - no - - no - - no + - empty + - yes + - yes + - yes + - yes + - yes - yes - - no - yes * - Update type extra specs - ``PUT /types/{type_id}/extra_specs/{extra_spec_key}`` @@ -2289,6 +2289,23 @@ matrix is validated by human beings. - yes - no - yes + * - Include extra_specs fields that may reveal sensitive information about + the deployment that should not be exposed to end users in various + volume-type responses that show extra_specs. + - | ``GET /types`` + | ``GET /types/{type_id}`` + | ``GET /types/{type_id}/extra_specs`` + | ``GET /types/{type_id}/extra_specs/{extra_spec_key}`` + | The ability to make these API calls is governed by other policies. + - volume_extension:types_extra_specs:read_sensitive + - rule:admin_api + - no + - no + - no + - no + - yes + - no + - yes .. list-table:: Volumes :header-rows: 1