From dcf66a96a43dd1383be1e8134ef4941e5bf43c9e Mon Sep 17 00:00:00 2001 From: Rajat Dhasmana Date: Thu, 17 Sep 2020 10:16:50 +0000 Subject: [PATCH] Modify/Move project validation methods to api_utils We currently only validate the existence of a project on the default types code so that we cannot set it for non existing projects, but there are other places where we would benefit from having this validation, such as volume type access, setting quotas. To achieve this we are moving methods from quota_utils and default_types to api_utils. This patch also modifies the method get_project_hierarchy to get_project and removes related hierarchy tests since with the removal of nested quota driver we don't require the hierarchy properties anymore. Related-Bug: #1638804 Change-Id: I20f8e500f583eb85f24a4c36f344c11c7face06c --- cinder/api/api_utils.py | 91 ++++++++++++++++ cinder/api/v3/default_types.py | 25 +---- cinder/quota_utils.py | 96 ----------------- cinder/tests/functional/test_default_types.py | 13 +-- cinder/tests/unit/api/contrib/test_quotas.py | 2 +- .../tests/unit/api/v3/test_default_types.py | 2 +- .../policies/test_default_volume_types.py | 16 ++- cinder/tests/unit/test_quota_utils.py | 100 +----------------- cinder/tests/unit/test_utils.py | 38 +++++++ 9 files changed, 152 insertions(+), 231 deletions(-) diff --git a/cinder/api/api_utils.py b/cinder/api/api_utils.py index 0fcba435aa4..05a1c7a07c2 100644 --- a/cinder/api/api_utils.py +++ b/cinder/api/api_utils.py @@ -10,13 +10,23 @@ # License for the specific language governing permissions and limitations # under the License. +from keystoneauth1 import exceptions as ks_exc +from keystoneauth1 import identity +from keystoneauth1 import loading as ka_loading +from keystoneclient import client +from oslo_config import cfg from oslo_log import log as logging from oslo_utils import strutils import webob from webob import exc +from cinder import exception from cinder.i18n import _ +CONF = cfg.CONF +CONF.import_group('keystone_authtoken', + 'keystonemiddleware.auth_token.__init__') + LOG = logging.getLogger(__name__) @@ -142,3 +152,84 @@ def walk_class_hierarchy(clazz, encountered=None): for subsubclass in walk_class_hierarchy(subclass, encountered): yield subsubclass yield subclass + + +def _keystone_client(context, version=(3, 0)): + """Creates and returns an instance of a generic keystone client. + + :param context: The request context + :param version: version of Keystone to request + :return: keystoneclient.client.Client object + """ + if context.system_scope is not None: + auth_plugin = identity.Token( + auth_url=CONF.keystone_authtoken.auth_url, + token=context.auth_token, + system_scope=context.system_scope + ) + elif context.domain_id is not None: + auth_plugin = identity.Token( + auth_url=CONF.keystone_authtoken.auth_url, + token=context.auth_token, + domain_id=context.domain_id + ) + elif context.project_id is not None: + auth_plugin = identity.Token( + auth_url=CONF.keystone_authtoken.auth_url, + token=context.auth_token, + project_id=context.project_id + ) + else: + # We're dealing with an unscoped token from keystone that doesn't + # carry any authoritative power outside of the user simplify proving + # they know their own password. This token isn't associated with any + # authorization target (e.g., system, domain, or project). + auth_plugin = context.get_auth_plugin() + + client_session = ka_loading.session.Session().load_from_options( + auth=auth_plugin, + insecure=CONF.keystone_authtoken.insecure, + cacert=CONF.keystone_authtoken.cafile, + key=CONF.keystone_authtoken.keyfile, + cert=CONF.keystone_authtoken.certfile, + split_loggers=CONF.service_user.split_loggers) + return client.Client(auth_url=CONF.keystone_authtoken.auth_url, + session=client_session, version=version) + + +class GenericProjectInfo(object): + """Abstraction layer for Keystone V2 and V3 project objects""" + def __init__(self, project_id, project_keystone_api_version, + domain_id=None, name=None, description=None): + self.id = project_id + self.keystone_api_version = project_keystone_api_version + self.domain_id = domain_id + self.name = name + self.description = description + + +def get_project(context, project_id): + """Method to verify project exists in keystone""" + keystone = _keystone_client(context) + generic_project = GenericProjectInfo(project_id, keystone.version) + project = keystone.projects.get(project_id) + generic_project.domain_id = project.domain_id + generic_project.name = project.name + generic_project.description = project.description + return generic_project + + +def validate_project_and_authorize(context, project_id, policy_check=None, + validate_only=False): + try: + target_project = get_project(context, project_id) + if not validate_only: + target_project = {'project_id': target_project.id} + context.authorize(policy_check, target=target_project) + except ks_exc.http.NotFound: + explanation = _("Project with id %s not found." % project_id) + raise exc.HTTPNotFound(explanation=explanation) + except exception.NotAuthorized: + explanation = _("You are not authorized to perform this " + "operation.") + raise exc.HTTPForbidden(explanation=explanation) diff --git a/cinder/api/v3/default_types.py b/cinder/api/v3/default_types.py index ffdf607fc03..536f52e983b 100644 --- a/cinder/api/v3/default_types.py +++ b/cinder/api/v3/default_types.py @@ -16,9 +16,9 @@ """The resource filters api.""" from http import HTTPStatus -from keystoneauth1 import exceptions as ks_exc from webob import exc +from cinder.api import api_utils as utils from cinder.api import microversions as mv from cinder.api.openstack import wsgi from cinder.api.schemas import default_types @@ -29,7 +29,6 @@ from cinder import exception from cinder.i18n import _ from cinder import objects from cinder.policies import default_types as policy -from cinder import quota_utils class DefaultTypesController(wsgi.Controller): @@ -37,22 +36,6 @@ class DefaultTypesController(wsgi.Controller): _view_builder_class = default_types_view.ViewBuilder - def _validate_project_and_authorize(self, context, project_id, - policy_check): - try: - target_project = quota_utils.get_project_hierarchy(context, - project_id) - target_project = {'project_id': target_project.id, - 'domain_id': target_project.domain_id} - context.authorize(policy_check, target=target_project) - except ks_exc.http.NotFound: - explanation = _("Project with id %s not found." % project_id) - raise exc.HTTPNotFound(explanation=explanation) - except exception.NotAuthorized: - explanation = _("You are not authorized to perform this " - "operation.") - raise exc.HTTPForbidden(explanation=explanation) - @wsgi.response(HTTPStatus.OK) @wsgi.Controller.api_version(mv.DEFAULT_TYPE_OVERRIDES) @validation.schema(default_types.create_or_update) @@ -63,7 +46,7 @@ class DefaultTypesController(wsgi.Controller): project_id = id volume_type_id = body['default_type']['volume_type'] - self._validate_project_and_authorize(context, project_id, + utils.validate_project_and_authorize(context, project_id, policy.CREATE_UPDATE_POLICY) try: volume_type_id = objects.VolumeType.get_by_name_or_id( @@ -85,7 +68,7 @@ class DefaultTypesController(wsgi.Controller): context = req.environ['cinder.context'] project_id = id - self._validate_project_and_authorize(context, project_id, + utils.validate_project_and_authorize(context, project_id, policy.GET_POLICY) default_type = db.project_default_volume_type_get(context, project_id) if not default_type: @@ -117,7 +100,7 @@ class DefaultTypesController(wsgi.Controller): context = req.environ['cinder.context'] project_id = id - self._validate_project_and_authorize(context, project_id, + utils.validate_project_and_authorize(context, project_id, policy.DELETE_POLICY) db.project_default_volume_type_unset(context, id) diff --git a/cinder/quota_utils.py b/cinder/quota_utils.py index 4480496071c..d13f97ee3ce 100644 --- a/cinder/quota_utils.py +++ b/cinder/quota_utils.py @@ -12,9 +12,6 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. -from keystoneauth1 import identity -from keystoneauth1 import loading as ka_loading -from keystoneclient import client from oslo_config import cfg from oslo_log import log as logging @@ -27,23 +24,6 @@ CONF.import_group('keystone_authtoken', LOG = logging.getLogger(__name__) -class GenericProjectInfo(object): - """Abstraction layer for Keystone V2 and V3 project objects""" - def __init__(self, project_id, project_keystone_api_version, - project_parent_id=None, - project_subtree=None, - project_parent_tree=None, - is_admin_project=False, - domain_id=None): - self.id = project_id - self.domain_id = domain_id - self.keystone_api_version = project_keystone_api_version - self.parent_id = project_parent_id - self.subtree = project_subtree - self.parents = project_parent_tree - self.is_admin_project = is_admin_project - - def get_volume_type_reservation(ctxt, volume, type_id, reserve_vol_type_only=False, negative=False): @@ -101,82 +81,6 @@ def _filter_domain_id_from_parents(domain_id, tree): return new_tree -def get_project_hierarchy(context, project_id, subtree_as_ids=False, - parents_as_ids=False, is_admin_project=False): - """A Helper method to get the project hierarchy. - - Along with hierarchical multitenancy in keystone API v3, projects can be - hierarchically organized. Therefore, we need to know the project - hierarchy, if any, in order to do default volume type operations properly. - """ - keystone = _keystone_client(context) - generic_project = GenericProjectInfo(project_id, keystone.version) - if keystone.version == 'v3': - project = keystone.projects.get(project_id, - subtree_as_ids=subtree_as_ids, - parents_as_ids=parents_as_ids) - - generic_project.parent_id = None - generic_project.domain_id = project.domain_id - if project.parent_id != project.domain_id: - generic_project.parent_id = project.parent_id - - generic_project.subtree = ( - project.subtree if subtree_as_ids else None) - - generic_project.parents = None - if parents_as_ids: - generic_project.parents = _filter_domain_id_from_parents( - project.domain_id, project.parents) - - generic_project.is_admin_project = is_admin_project - - return generic_project - - -def _keystone_client(context, version=(3, 0)): - """Creates and returns an instance of a generic keystone client. - - :param context: The request context - :param version: version of Keystone to request - :return: keystoneclient.client.Client object - """ - if context.system_scope is not None: - auth_plugin = identity.Token( - auth_url=CONF.keystone_authtoken.auth_url, - token=context.auth_token, - system_scope=context.system_scope - ) - elif context.domain_id is not None: - auth_plugin = identity.Token( - auth_url=CONF.keystone_authtoken.auth_url, - token=context.auth_token, - domain_id=context.domain_id - ) - elif context.project_id is not None: - auth_plugin = identity.Token( - auth_url=CONF.keystone_authtoken.auth_url, - token=context.auth_token, - project_id=context.project_id - ) - else: - # We're dealing with an unscoped token from keystone that doesn't - # carry any authoritative power outside of the user simplify proving - # they know their own password. This token isn't associated with any - # authorization target (e.g., system, domain, or project). - auth_plugin = context.get_auth_plugin() - - client_session = ka_loading.session.Session().load_from_options( - auth=auth_plugin, - insecure=CONF.keystone_authtoken.insecure, - cacert=CONF.keystone_authtoken.cafile, - key=CONF.keystone_authtoken.keyfile, - cert=CONF.keystone_authtoken.certfile, - split_loggers=CONF.service_user.split_loggers) - return client.Client(auth_url=CONF.keystone_authtoken.auth_url, - session=client_session, version=version) - - OVER_QUOTA_RESOURCE_EXCEPTIONS = {'snapshots': exception.SnapshotLimitExceeded, 'backups': exception.BackupLimitExceeded, 'volumes': exception.VolumeLimitExceeded, diff --git a/cinder/tests/functional/test_default_types.py b/cinder/tests/functional/test_default_types.py index 1277f95752c..d3b1535b471 100644 --- a/cinder/tests/functional/test_default_types.py +++ b/cinder/tests/functional/test_default_types.py @@ -35,7 +35,7 @@ class DefaultVolumeTypesTest(functional_helpers._FunctionalTestBase): _keystone_client.version = 'v3' _keystone_client.projects.get.side_effect = self._get_project _keystone_client_get = mock.patch( - 'cinder.quota_utils._keystone_client', + 'cinder.api.api_utils._keystone_client', lambda *args, **kwargs: _keystone_client) _keystone_client_get.start() self.addCleanup(_keystone_client_get.stop) @@ -44,14 +44,11 @@ class DefaultVolumeTypesTest(functional_helpers._FunctionalTestBase): return self.project class FakeProject(object): - _dom_id = uuid.uuid4().hex - - def __init__(self, parent_id=None): + def __init__(self, name=None): self.id = uuid.uuid4().hex - self.parent_id = parent_id - self.domain_id = self._dom_id - self.subtree = None - self.parents = None + self.name = name + self.description = 'fake project description' + self.domain_id = 'default' @mock.patch.object(context.RequestContext, 'authorize') def test_default_type_set(self, mock_authorize): diff --git a/cinder/tests/unit/api/contrib/test_quotas.py b/cinder/tests/unit/api/contrib/test_quotas.py index e628378f515..f4fa7d4375a 100644 --- a/cinder/tests/unit/api/contrib/test_quotas.py +++ b/cinder/tests/unit/api/contrib/test_quotas.py @@ -99,7 +99,7 @@ class QuotaSetsControllerTestBase(test.TestCase): self.req.environ['cinder.context'].project_id = uuid.uuid4().hex - get_patcher = mock.patch('cinder.quota_utils.get_project_hierarchy', + get_patcher = mock.patch('cinder.api.api_utils.get_project', self._get_project) get_patcher.start() self.addCleanup(get_patcher.stop) diff --git a/cinder/tests/unit/api/v3/test_default_types.py b/cinder/tests/unit/api/v3/test_default_types.py index 4a287055fbf..bcefed5e486 100644 --- a/cinder/tests/unit/api/v3/test_default_types.py +++ b/cinder/tests/unit/api/v3/test_default_types.py @@ -79,7 +79,7 @@ class DefaultVolumeTypesApiTest(test.TestCase): self.type2 = self._create_volume_type( self.ctxt, 'volume_type2') - get_patcher = mock.patch('cinder.quota_utils.get_project_hierarchy', + get_patcher = mock.patch('cinder.api.api_utils.get_project', self._get_project) get_patcher.start() self.addCleanup(get_patcher.stop) diff --git a/cinder/tests/unit/policies/test_default_volume_types.py b/cinder/tests/unit/policies/test_default_volume_types.py index 1b97cfbbdcd..381bfba53dc 100644 --- a/cinder/tests/unit/policies/test_default_volume_types.py +++ b/cinder/tests/unit/policies/test_default_volume_types.py @@ -15,6 +15,7 @@ from http import HTTPStatus from unittest import mock +import uuid from cinder.api import microversions as mv from cinder import db @@ -38,7 +39,7 @@ class DefaultVolumeTypesPolicyTests(test_base.CinderPolicyTests): _keystone_client.version = 'v3' _keystone_client.projects.get.side_effect = self._get_project _keystone_client_get = mock.patch( - 'cinder.quota_utils._keystone_client', + 'cinder.api.api_utils._keystone_client', lambda *args, **kwargs: _keystone_client) _keystone_client_get.start() self.addCleanup(_keystone_client_get.stop) @@ -47,14 +48,11 @@ class DefaultVolumeTypesPolicyTests(test_base.CinderPolicyTests): return self.project class FakeProject(object): - _dom_id = fake_constants.DOMAIN_ID - - def __init__(self, parent_id=None): - self.id = fake_constants.PROJECT_ID - self.parent_id = parent_id - self.domain_id = self._dom_id - self.subtree = None - self.parents = None + def __init__(self, name=None): + self.id = uuid.uuid4().hex + self.name = name + self.description = 'fake project description' + self.domain_id = 'default' def test_system_admin_can_set_default(self): system_admin_context = self.system_admin_context diff --git a/cinder/tests/unit/test_quota_utils.py b/cinder/tests/unit/test_quota_utils.py index ec4f6c99c8f..35aae9fdb01 100644 --- a/cinder/tests/unit/test_quota_utils.py +++ b/cinder/tests/unit/test_quota_utils.py @@ -18,6 +18,7 @@ from unittest import mock from oslo_config import cfg from oslo_config import fixture as config_fixture +from cinder.api import api_utils from cinder import context from cinder import exception from cinder import quota_utils @@ -29,13 +30,6 @@ CONF = cfg.CONF class QuotaUtilsTest(test.TestCase): - class FakeProject(object): - def __init__(self, id='foo', parent_id=None): - self.id = id - self.parent_id = parent_id - self.subtree = None - self.parents = None - self.domain_id = 'default' def setUp(self): super(QuotaUtilsTest, self).setUp() @@ -49,7 +43,7 @@ class QuotaUtilsTest(test.TestCase): @mock.patch('keystoneauth1.session.Session') def test_keystone_client_instantiation(self, ksclient_session, ksclient_class): - quota_utils._keystone_client(self.context) + api_utils._keystone_client(self.context) ksclient_class.assert_called_once_with(auth_url=self.auth_url, session=ksclient_session(), version=(3, 0)) @@ -61,7 +55,7 @@ class QuotaUtilsTest(test.TestCase): self, ks_token, ksclient_session, ksclient_class): system_context = context.RequestContext( 'fake_user', 'fake_proj_id', system_scope='all') - quota_utils._keystone_client(system_context) + api_utils._keystone_client(system_context) ks_token.assert_called_once_with( auth_url=self.auth_url, token=system_context.auth_token, system_scope=system_context.system_scope) @@ -73,7 +67,7 @@ class QuotaUtilsTest(test.TestCase): self, ks_token, ksclient_session, ksclient_class): domain_context = context.RequestContext( 'fake_user', 'fake_proj_id', domain_id='default') - quota_utils._keystone_client(domain_context) + api_utils._keystone_client(domain_context) ks_token.assert_called_once_with( auth_url=self.auth_url, token=domain_context.auth_token, domain_id=domain_context.domain_id) @@ -85,51 +79,11 @@ class QuotaUtilsTest(test.TestCase): self, ks_token, ksclient_session, ksclient_class): project_context = context.RequestContext( 'fake_user', project_id=fake.PROJECT_ID) - quota_utils._keystone_client(project_context) + api_utils._keystone_client(project_context) ks_token.assert_called_once_with( auth_url=self.auth_url, token=project_context.auth_token, project_id=project_context.project_id) - @mock.patch('keystoneclient.client.Client') - def test_get_project_keystoneclient_v2(self, ksclient_class): - keystoneclient = ksclient_class.return_value - keystoneclient.version = 'v2.0' - expected_project = quota_utils.GenericProjectInfo( - self.context.project_id, 'v2.0') - project = quota_utils.get_project_hierarchy( - self.context, self.context.project_id) - self.assertEqual(expected_project.__dict__, project.__dict__) - - @mock.patch('keystoneclient.client.Client') - def test_get_project_keystoneclient_v3(self, ksclient_class): - keystoneclient = ksclient_class.return_value - keystoneclient.version = 'v3' - returned_project = self.FakeProject(self.context.project_id, 'bar') - del returned_project.subtree - keystoneclient.projects.get.return_value = returned_project - expected_project = quota_utils.GenericProjectInfo( - self.context.project_id, 'v3', 'bar', domain_id='default') - project = quota_utils.get_project_hierarchy( - self.context, self.context.project_id) - self.assertEqual(expected_project.__dict__, project.__dict__) - - @mock.patch('keystoneclient.client.Client') - def test_get_project_keystoneclient_v3_with_subtree(self, ksclient_class): - keystoneclient = ksclient_class.return_value - keystoneclient.version = 'v3' - returned_project = self.FakeProject(self.context.project_id, 'bar') - subtree_dict = {'baz': {'quux': None}} - returned_project.subtree = subtree_dict - keystoneclient.projects.get.return_value = returned_project - expected_project = quota_utils.GenericProjectInfo( - self.context.project_id, 'v3', 'bar', subtree_dict, - domain_id='default') - project = quota_utils.get_project_hierarchy( - self.context, self.context.project_id, subtree_as_ids=True) - keystoneclient.projects.get.assert_called_once_with( - self.context.project_id, parents_as_ids=False, subtree_as_ids=True) - self.assertEqual(expected_project.__dict__, project.__dict__) - def _setup_mock_ksclient(self, mock_client, version='v3', subtree=None, parents=None): keystoneclient = mock_client.return_value @@ -141,50 +95,6 @@ class QuotaUtilsTest(test.TestCase): proj.parent_id = next(iter(parents.keys())) keystoneclient.projects.get.return_value = proj - @mock.patch('keystoneclient.client.Client') - def test__filter_domain_id_from_parents_domain_as_parent( - self, mock_client): - # Test with a top level project (domain is direct parent) - self._setup_mock_ksclient(mock_client, parents={'default': None}) - project = quota_utils.get_project_hierarchy( - self.context, self.context.project_id, parents_as_ids=True) - self.assertIsNone(project.parent_id) - self.assertIsNone(project.parents) - - @mock.patch('keystoneclient.client.Client') - def test__filter_domain_id_from_parents_domain_as_grandparent( - self, mock_client): - # Test with a child project (domain is more than a parent) - self._setup_mock_ksclient(mock_client, - parents={'bar': {'default': None}}) - project = quota_utils.get_project_hierarchy( - self.context, self.context.project_id, parents_as_ids=True) - self.assertEqual('bar', project.parent_id) - self.assertEqual({'bar': None}, project.parents) - - @mock.patch('keystoneclient.client.Client') - def test__filter_domain_id_from_parents_no_domain_in_parents( - self, mock_client): - # Test that if top most parent is not a domain (to simulate an older - # keystone version) nothing gets removed from the tree - parents = {'bar': {'foo': None}} - self._setup_mock_ksclient(mock_client, parents=parents) - project = quota_utils.get_project_hierarchy( - self.context, self.context.project_id, parents_as_ids=True) - self.assertEqual('bar', project.parent_id) - self.assertEqual(parents, project.parents) - - @mock.patch('keystoneclient.client.Client') - def test__filter_domain_id_from_parents_no_parents( - self, mock_client): - # Test that if top no parents are present (to simulate an older - # keystone version) things don't blow up - self._setup_mock_ksclient(mock_client) - project = quota_utils.get_project_hierarchy( - self.context, self.context.project_id, parents_as_ids=True) - self.assertIsNone(project.parent_id) - self.assertIsNone(project.parents) - def _process_reserve_over_quota(self, overs, usages, quotas, expected_ex, resource='volumes'): diff --git a/cinder/tests/unit/test_utils.py b/cinder/tests/unit/test_utils.py index dec973e1299..5c264023a7c 100644 --- a/cinder/tests/unit/test_utils.py +++ b/cinder/tests/unit/test_utils.py @@ -24,6 +24,7 @@ import webob.exc import cinder from cinder.api import api_utils +from cinder import context from cinder import exception from cinder.tests.unit import fake_constants as fake from cinder.tests.unit import test @@ -1415,3 +1416,40 @@ class LimitOperationsTestCase(test.TestCase): mock_exec.assert_called_once_with(mocked_semaphore.__enter__) mocked_semaphore.__exit__.assert_not_called() mocked_semaphore.__exit__.assert_called_once_with(None, None, None) + + +class TestKeystoneProjectGet(test.TestCase): + class FakeProject(object): + def __init__(self, id='foo', name=None): + self.id = id + self.name = name + self.description = 'fake project description' + self.domain_id = 'default' + + @mock.patch('keystoneclient.client.Client') + def test_get_project_keystoneclient_v2(self, ksclient_class): + self.context = context.RequestContext('fake_user', 'fake_proj_id') + keystoneclient = ksclient_class.return_value + keystoneclient.version = 'v2.0' + returned_project = self.FakeProject(self.context.project_id, 'bar') + keystoneclient.projects.get.return_value = returned_project + expected_project = api_utils.GenericProjectInfo( + self.context.project_id, 'v2.0', domain_id='default', name='bar', + description='fake project description') + project = api_utils.get_project( + self.context, self.context.project_id) + self.assertEqual(expected_project.__dict__, project.__dict__) + + @mock.patch('keystoneclient.client.Client') + def test_get_project_keystoneclient_v3(self, ksclient_class): + self.context = context.RequestContext('fake_user', 'fake_proj_id') + keystoneclient = ksclient_class.return_value + keystoneclient.version = 'v3' + returned_project = self.FakeProject(self.context.project_id, 'bar') + keystoneclient.projects.get.return_value = returned_project + expected_project = api_utils.GenericProjectInfo( + self.context.project_id, 'v3', domain_id='default', name='bar', + description='fake project description') + project = api_utils.get_project( + self.context, self.context.project_id) + self.assertEqual(expected_project.__dict__, project.__dict__)