From b65d506a5f9d9b2b20777a9aceb44a8ffed6a5de Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Thu, 27 Jun 2013 21:00:05 +0000 Subject: [PATCH] Make flavors is_public option actually work When you create a flavor, you can set an is_public flag to be True or False. It is True by default. When False, the intention is that the flavor is only accessible by an admin, unless you use the flavor_access API extension to grant access to specific tenants. Unfortunately, the only place in the code where this was being enforced was when listing flavors through the API. It would filter out the non-public ones for a non-admin. Otherwise, the flavor was accessible. You could get the details, and you could boot an instance with it, if you figured out a valid flavor ID. This patch adds enforcement down in the db layer. It also fixes one place in the API where the context wasn't passed down to enable the enforcement to happen. Fix bug 1194093. Change-Id: I5b37fa0bb19683fe1642fd81222547d4a317054e --- .../compute/contrib/flavor_access.py | 2 +- .../openstack/compute/contrib/flavormanage.py | 2 +- nova/api/openstack/compute/flavors.py | 3 +- nova/compute/api.py | 2 +- nova/compute/flavors.py | 2 +- nova/db/api.py | 4 +- nova/db/sqlalchemy/api.py | 15 ++-- .../compute/contrib/test_flavor_access.py | 2 +- .../compute/contrib/test_flavor_disabled.py | 2 +- .../compute/contrib/test_flavor_manage.py | 2 +- .../compute/contrib/test_flavor_rxtx.py | 2 +- .../compute/contrib/test_flavor_swap.py | 2 +- .../compute/contrib/test_flavorextradata.py | 2 +- .../api/openstack/compute/test_flavors.py | 4 +- nova/tests/db/test_db_api.py | 70 +++++++++++++++++++ 15 files changed, 97 insertions(+), 19 deletions(-) diff --git a/nova/api/openstack/compute/contrib/flavor_access.py b/nova/api/openstack/compute/contrib/flavor_access.py index bea92d883e..82df5a3e10 100644 --- a/nova/api/openstack/compute/contrib/flavor_access.py +++ b/nova/api/openstack/compute/contrib/flavor_access.py @@ -95,7 +95,7 @@ class FlavorAccessController(object): authorize(context) try: - flavor = flavors.get_flavor_by_flavor_id(flavor_id) + flavor = flavors.get_flavor_by_flavor_id(flavor_id, ctxt=context) except exception.FlavorNotFound: explanation = _("Flavor not found.") raise webob.exc.HTTPNotFound(explanation=explanation) diff --git a/nova/api/openstack/compute/contrib/flavormanage.py b/nova/api/openstack/compute/contrib/flavormanage.py index 602e82c366..5b22539e9d 100644 --- a/nova/api/openstack/compute/contrib/flavormanage.py +++ b/nova/api/openstack/compute/contrib/flavormanage.py @@ -41,7 +41,7 @@ class FlavorManageController(wsgi.Controller): try: flavor = flavors.get_flavor_by_flavor_id( - id, read_deleted="no") + id, ctxt=context, read_deleted="no") except exception.NotFound as e: raise webob.exc.HTTPNotFound(explanation=e.format_message()) diff --git a/nova/api/openstack/compute/flavors.py b/nova/api/openstack/compute/flavors.py index c6303646a2..029e5a5bd6 100644 --- a/nova/api/openstack/compute/flavors.py +++ b/nova/api/openstack/compute/flavors.py @@ -86,7 +86,8 @@ class Controller(wsgi.Controller): def show(self, req, id): """Return data about the given flavor id.""" try: - flavor = flavors.get_flavor_by_flavor_id(id) + context = req.environ['nova.context'] + flavor = flavors.get_flavor_by_flavor_id(id, ctxt=context) req.cache_db_flavor(flavor) except exception.NotFound: raise webob.exc.HTTPNotFound() diff --git a/nova/compute/api.py b/nova/compute/api.py index 48d404148d..0b35177ee4 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -1428,7 +1428,7 @@ class API(base.Base): #NOTE(bcwaldon): this doesn't really belong in this class def get_instance_type(self, context, instance_type_id): """Get an instance type by instance type id.""" - return flavors.get_flavor(instance_type_id) + return flavors.get_flavor(instance_type_id, ctxt=context) def get(self, context, instance_id): """Get a single instance with the given instance_id.""" diff --git a/nova/compute/flavors.py b/nova/compute/flavors.py index a18b375d85..69ccfe4a04 100644 --- a/nova/compute/flavors.py +++ b/nova/compute/flavors.py @@ -206,7 +206,7 @@ def get_flavor_by_flavor_id(flavorid, ctxt=None, read_deleted="yes"): if ctxt is None: ctxt = context.get_admin_context(read_deleted=read_deleted) - return db.instance_type_get_by_flavor_id(ctxt, flavorid) + return db.instance_type_get_by_flavor_id(ctxt, flavorid, read_deleted) def get_flavor_access_by_flavor_id(flavorid, ctxt=None): diff --git a/nova/db/api.py b/nova/db/api.py index bd519110ca..126398b9e5 100644 --- a/nova/db/api.py +++ b/nova/db/api.py @@ -1375,9 +1375,9 @@ def instance_type_get_by_name(context, name): return IMPL.instance_type_get_by_name(context, name) -def instance_type_get_by_flavor_id(context, id): +def instance_type_get_by_flavor_id(context, id, read_deleted=None): """Get instance type by flavor id.""" - return IMPL.instance_type_get_by_flavor_id(context, id) + return IMPL.instance_type_get_by_flavor_id(context, id, read_deleted) def instance_type_destroy(context, name): diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 03dd439466..1a0ac2dacc 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -3811,9 +3811,16 @@ def _dict_with_extra_specs(inst_type_query): def _instance_type_get_query(context, session=None, read_deleted=None): - return model_query(context, models.InstanceTypes, session=session, + query = model_query(context, models.InstanceTypes, session=session, read_deleted=read_deleted).\ - options(joinedload('extra_specs')) + options(joinedload('extra_specs')) + if not context.is_admin: + the_filter = [models.InstanceTypes.is_public == True] + the_filter.extend([ + models.InstanceTypes.projects.any(project_id=context.project_id) + ]) + query = query.filter(or_(*the_filter)) + return query @require_context @@ -3901,9 +3908,9 @@ def instance_type_get_by_name(context, name): @require_context -def instance_type_get_by_flavor_id(context, flavor_id): +def instance_type_get_by_flavor_id(context, flavor_id, read_deleted): """Returns a dict describing specific flavor_id.""" - result = _instance_type_get_query(context).\ + result = _instance_type_get_query(context, read_deleted=read_deleted).\ filter_by(flavorid=flavor_id).\ first() if not result: diff --git a/nova/tests/api/openstack/compute/contrib/test_flavor_access.py b/nova/tests/api/openstack/compute/contrib/test_flavor_access.py index d072e0784f..c43f5f4108 100644 --- a/nova/tests/api/openstack/compute/contrib/test_flavor_access.py +++ b/nova/tests/api/openstack/compute/contrib/test_flavor_access.py @@ -68,7 +68,7 @@ def fake_get_flavor_access_by_flavor_id(flavorid): return res -def fake_get_flavor_by_flavor_id(flavorid): +def fake_get_flavor_by_flavor_id(flavorid, ctxt=None): return INSTANCE_TYPES[flavorid] diff --git a/nova/tests/api/openstack/compute/contrib/test_flavor_disabled.py b/nova/tests/api/openstack/compute/contrib/test_flavor_disabled.py index e46e02a448..01b21f3082 100644 --- a/nova/tests/api/openstack/compute/contrib/test_flavor_disabled.py +++ b/nova/tests/api/openstack/compute/contrib/test_flavor_disabled.py @@ -39,7 +39,7 @@ FAKE_FLAVORS = { } -def fake_flavor_get_by_flavor_id(flavorid): +def fake_flavor_get_by_flavor_id(flavorid, ctxt=None): return FAKE_FLAVORS['flavor %s' % flavorid] diff --git a/nova/tests/api/openstack/compute/contrib/test_flavor_manage.py b/nova/tests/api/openstack/compute/contrib/test_flavor_manage.py index 459dae932c..e5da25fad8 100644 --- a/nova/tests/api/openstack/compute/contrib/test_flavor_manage.py +++ b/nova/tests/api/openstack/compute/contrib/test_flavor_manage.py @@ -25,7 +25,7 @@ from nova import test from nova.tests.api.openstack import fakes -def fake_get_flavor_by_flavor_id(flavorid, read_deleted='yes'): +def fake_get_flavor_by_flavor_id(flavorid, ctxt=None, read_deleted='yes'): if flavorid == 'failtest': raise exception.NotFound("Not found sucka!") elif not str(flavorid) == '1234': diff --git a/nova/tests/api/openstack/compute/contrib/test_flavor_rxtx.py b/nova/tests/api/openstack/compute/contrib/test_flavor_rxtx.py index 5843a60a15..52b36d96d9 100644 --- a/nova/tests/api/openstack/compute/contrib/test_flavor_rxtx.py +++ b/nova/tests/api/openstack/compute/contrib/test_flavor_rxtx.py @@ -38,7 +38,7 @@ FAKE_FLAVORS = { } -def fake_flavor_get_by_flavor_id(flavorid): +def fake_flavor_get_by_flavor_id(flavorid, ctxt=None): return FAKE_FLAVORS['flavor %s' % flavorid] diff --git a/nova/tests/api/openstack/compute/contrib/test_flavor_swap.py b/nova/tests/api/openstack/compute/contrib/test_flavor_swap.py index fd154479d0..839519c8ce 100644 --- a/nova/tests/api/openstack/compute/contrib/test_flavor_swap.py +++ b/nova/tests/api/openstack/compute/contrib/test_flavor_swap.py @@ -39,7 +39,7 @@ FAKE_FLAVORS = { #TOD(jogo) dedup these accross nova.api.openstack.contrib.test_flavor* -def fake_flavor_get_by_flavor_id(flavorid): +def fake_flavor_get_by_flavor_id(flavorid, ctxt=None): return FAKE_FLAVORS['flavor %s' % flavorid] diff --git a/nova/tests/api/openstack/compute/contrib/test_flavorextradata.py b/nova/tests/api/openstack/compute/contrib/test_flavorextradata.py index ec0d68b041..151dab4465 100644 --- a/nova/tests/api/openstack/compute/contrib/test_flavorextradata.py +++ b/nova/tests/api/openstack/compute/contrib/test_flavorextradata.py @@ -23,7 +23,7 @@ from nova import test from nova.tests.api.openstack import fakes -def fake_get_flavor_by_flavor_id(flavorid): +def fake_get_flavor_by_flavor_id(flavorid, ctxt=None): return { 'id': flavorid, 'flavorid': str(flavorid), diff --git a/nova/tests/api/openstack/compute/test_flavors.py b/nova/tests/api/openstack/compute/test_flavors.py index 3741fcd33b..bddb33a37f 100644 --- a/nova/tests/api/openstack/compute/test_flavors.py +++ b/nova/tests/api/openstack/compute/test_flavors.py @@ -50,7 +50,7 @@ FAKE_FLAVORS = { } -def fake_flavor_get_by_flavor_id(flavorid): +def fake_flavor_get_by_flavor_id(flavorid, ctxt=None): return FAKE_FLAVORS['flavor %s' % flavorid] @@ -76,7 +76,7 @@ def empty_flavor_get_all(inactive=False, filters=None): return {} -def return_flavor_not_found(flavor_id): +def return_flavor_not_found(flavor_id, ctxt=None): raise exception.InstanceTypeNotFound(instance_type_id=flavor_id) diff --git a/nova/tests/db/test_db_api.py b/nova/tests/db/test_db_api.py index 15ef5c3c14..8fffc8e0af 100644 --- a/nova/tests/db/test_db_api.py +++ b/nova/tests/db/test_db_api.py @@ -2023,6 +2023,7 @@ class BaseInstanceTypeTestCase(test.TestCase, ModelsObjectComparatorMixin): def setUp(self): super(BaseInstanceTypeTestCase, self).setUp() self.ctxt = context.get_admin_context() + self.user_ctxt = context.RequestContext('user', 'user') def _get_base_values(self): return { @@ -2503,6 +2504,24 @@ class InstanceTypeTestCase(BaseInstanceTypeTestCase): inst_type_by_id = db.instance_type_get(self.ctxt, inst_type['id']) self._assertEqualObjects(inst_type, inst_type_by_id) + def test_instance_type_get_non_public(self): + inst_type = self._create_inst_type({'name': 'abc', 'flavorid': '123', + 'is_public': False}) + + # Admin can see it + inst_type_by_id = db.instance_type_get(self.ctxt, inst_type['id']) + self._assertEqualObjects(inst_type, inst_type_by_id) + + # Regular user can not + self.assertRaises(exception.InstanceTypeNotFound, db.instance_type_get, + self.user_ctxt, inst_type['id']) + + # Regular user can see it after being granted access + db.instance_type_access_add(self.ctxt, inst_type['flavorid'], + self.user_ctxt.project_id) + inst_type_by_id = db.instance_type_get(self.user_ctxt, inst_type['id']) + self._assertEqualObjects(inst_type, inst_type_by_id) + def test_instance_type_get_by_name(self): inst_types = [{'name': 'abc', 'flavorid': '123'}, {'name': 'def', 'flavorid': '456'}, @@ -2519,6 +2538,27 @@ class InstanceTypeTestCase(BaseInstanceTypeTestCase): self.assertRaises(exception.InstanceTypeNotFoundByName, db.instance_type_get_by_name, self.ctxt, 'nonexists') + def test_instance_type_get_by_name_non_public(self): + inst_type = self._create_inst_type({'name': 'abc', 'flavorid': '123', + 'is_public': False}) + + # Admin can see it + inst_type_by_name = db.instance_type_get_by_name(self.ctxt, + inst_type['name']) + self._assertEqualObjects(inst_type, inst_type_by_name) + + # Regular user can not + self.assertRaises(exception.InstanceTypeNotFoundByName, + db.instance_type_get_by_name, self.user_ctxt, + inst_type['name']) + + # Regular user can see it after being granted access + db.instance_type_access_add(self.ctxt, inst_type['flavorid'], + self.user_ctxt.project_id) + inst_type_by_name = db.instance_type_get_by_name(self.user_ctxt, + inst_type['name']) + self._assertEqualObjects(inst_type, inst_type_by_name) + def test_instance_type_get_by_flavor_id(self): inst_types = [{'name': 'abc', 'flavorid': '123'}, {'name': 'def', 'flavorid': '456'}, @@ -2536,6 +2576,36 @@ class InstanceTypeTestCase(BaseInstanceTypeTestCase): db.instance_type_get_by_flavor_id, self.ctxt, 'nonexists') + def test_instance_type_get_by_flavor_id_non_public(self): + inst_type = self._create_inst_type({'name': 'abc', 'flavorid': '123', + 'is_public': False}) + + # Admin can see it + inst_type_by_fid = db.instance_type_get_by_flavor_id(self.ctxt, + inst_type['flavorid']) + self._assertEqualObjects(inst_type, inst_type_by_fid) + + # Regular user can not + self.assertRaises(exception.FlavorNotFound, + db.instance_type_get_by_flavor_id, self.user_ctxt, + inst_type['flavorid']) + + # Regular user can see it after being granted access + db.instance_type_access_add(self.ctxt, inst_type['flavorid'], + self.user_ctxt.project_id) + inst_type_by_fid = db.instance_type_get_by_flavor_id(self.user_ctxt, + inst_type['flavorid']) + self._assertEqualObjects(inst_type, inst_type_by_fid) + + def test_instance_type_get_by_flavor_id_deleted(self): + inst_type = self._create_inst_type({'name': 'abc', 'flavorid': '123'}) + + db.instance_type_destroy(self.ctxt, 'abc') + + inst_type_by_fid = db.instance_type_get_by_flavor_id(self.ctxt, + inst_type['flavorid'], read_deleted='yes') + self.assertEqual(inst_type['id'], inst_type_by_fid['id']) + class InstanceTypeExtraSpecsTestCase(BaseInstanceTypeTestCase):