From 573fb0cc1fb4fce990a40d4debdf7489648c5a6c Mon Sep 17 00:00:00 2001 From: Yuriy Taraday Date: Tue, 11 Feb 2014 23:12:11 +0400 Subject: [PATCH] Remove explicit access to is_admin in context is_admin should not be exlicitely accessed from code. Policies should be used to check if it's set and context.elevated() to run something in admin context. Change-Id: I27754a378073844c3e24d94038a5d24f9a3c6dc3 --- climate/api/context.py | 7 +---- climate/api/service.py | 8 +++++- climate/context.py | 11 -------- climate/db/api.py | 4 +-- climate/db/sqlalchemy/api.py | 28 +++++-------------- climate/manager/rpcapi.py | 4 +-- climate/manager/service.py | 4 +-- climate/tests/api/test_context.py | 8 ------ .../db/sqlalchemy/test_sqlalchemy_api.py | 24 ++-------------- climate/tests/manager/test_rpcapi.py | 4 +-- climate/tests/test_context.py | 11 -------- 11 files changed, 26 insertions(+), 87 deletions(-) diff --git a/climate/api/context.py b/climate/api/context.py index e9da9ac2..ac6652ce 100644 --- a/climate/api/context.py +++ b/climate/api/context.py @@ -17,7 +17,6 @@ import json from climate import context from climate import exceptions -from climate import policy def ctx_from_headers(headers): @@ -28,7 +27,7 @@ def ctx_from_headers(headers): except TypeError: raise exceptions.WrongFormat() - ctx = context.ClimateContext( + return context.ClimateContext( user_id=headers['X-User-Id'], tenant_id=headers['X-Tenant-Id'], auth_token=headers['X-Auth-Token'], @@ -37,7 +36,3 @@ def ctx_from_headers(headers): tenant_name=headers['X-Tenant-Name'], roles=map(unicode.strip, headers['X-Roles'].split(',')), ) - target = {'tenant_id': ctx.tenant_id, 'user_id': ctx.user_id} - if policy.enforce(ctx, "admin", target, do_raise=False): - ctx.is_admin = True - return ctx diff --git a/climate/api/service.py b/climate/api/service.py index e7e774bb..7e87435d 100644 --- a/climate/api/service.py +++ b/climate/api/service.py @@ -13,6 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +from climate import context from climate import exceptions from climate.manager import rpcapi as manager_rpcapi from climate.openstack.common import log as logging @@ -32,7 +33,12 @@ class API(object): @policy.authorize('leases', 'get') def get_leases(self): """List all existing leases.""" - return self.manager_rpcapi.list_leases() + ctx = context.current() + if policy.enforce(ctx, 'admin', {}, do_raise=False): + tenant_id = None + else: + tenant_id = ctx.tenant_id + return self.manager_rpcapi.list_leases(tenant_id=tenant_id) @policy.authorize('leases', 'create') def create_lease(self, data): diff --git a/climate/context.py b/climate/context.py index 6618d547..e0c56064 100644 --- a/climate/context.py +++ b/climate/context.py @@ -96,14 +96,3 @@ def current(): def elevated(): return ClimateContext.elevated() - - -def is_user_context(context): - """Indicates if the request context is a normal user""" - if not context: - return False - if context.is_admin: - return False - if not context.user_id or not context.tenant_id: - return False - return True diff --git a/climate/db/api.py b/climate/db/api.py index 41a4fde0..de41f56e 100644 --- a/climate/db/api.py +++ b/climate/db/api.py @@ -171,9 +171,9 @@ def lease_get(lease_id): @to_dict -def lease_list(): +def lease_list(tenant_id=None): """Return a list of all existing leases.""" - return IMPL.lease_list() + return IMPL.lease_list(tenant_id) def lease_destroy(lease_id): diff --git a/climate/db/sqlalchemy/api.py b/climate/db/sqlalchemy/api.py index bf785cf1..7b07638f 100644 --- a/climate/db/sqlalchemy/api.py +++ b/climate/db/sqlalchemy/api.py @@ -21,7 +21,6 @@ import sqlalchemy as sa from sqlalchemy.sql.expression import asc from sqlalchemy.sql.expression import desc -from climate import context from climate.db.sqlalchemy import models from climate.openstack.common.db import exception as db_exc from climate.openstack.common.db.sqlalchemy import session as db_session @@ -40,7 +39,7 @@ def get_backend(): return sys.modules[__name__] -def model_query(model, session=None, project_only=False): +def model_query(model, session=None): """Query helper. :param model: base model to query @@ -49,23 +48,7 @@ def model_query(model, session=None, project_only=False): """ session = session or get_session() - query = session.query(model) - - if project_only and 'tenant_id' not in model.__table__.columns: - # model not having tenant_id as attribute - LOG.debug("Query not filtered per tenant") - return query - - if project_only: - try: - ctx = context.current() - except RuntimeError: - # Context is not available - return query - if context.is_user_context(ctx): - query = query.filter_by(tenant_id=ctx.tenant_id) - - return query + return session.query(model) def setup_db(): @@ -224,8 +207,11 @@ def lease_get_all_by_user(user_id): raise NotImplementedError -def lease_list(): - return model_query(models.Lease, get_session(), project_only=True).all() +def lease_list(tenant_id=None): + query = model_query(models.Lease, get_session()) + if tenant_id is not None: + query = query.filter_by(tenant_id=tenant_id) + return query.all() def lease_create(values): diff --git a/climate/manager/rpcapi.py b/climate/manager/rpcapi.py index 95396579..f812b5ca 100644 --- a/climate/manager/rpcapi.py +++ b/climate/manager/rpcapi.py @@ -30,9 +30,9 @@ class ManagerRPCAPI(service.RPCClient): """Get detailed info about some lease.""" return self.call('get_lease', lease_id=lease_id) - def list_leases(self): + def list_leases(self, tenant_id=None): """List all leases.""" - return self.call('list_leases') + return self.call('list_leases', tenant_id=tenant_id) def create_lease(self, lease_values): """Create lease with specified parameters.""" diff --git a/climate/manager/service.py b/climate/manager/service.py index ee401ecd..5f08aa1a 100644 --- a/climate/manager/service.py +++ b/climate/manager/service.py @@ -153,8 +153,8 @@ class ManagerService(service_utils.RPCServer, service.Service): def get_lease(self, lease_id): return db_api.lease_get(lease_id) - def list_leases(self): - return db_api.lease_list() + def list_leases(self, tenant_id=None): + return db_api.lease_list(tenant_id) def create_lease(self, lease_values): """Create a lease with reservations. diff --git a/climate/tests/api/test_context.py b/climate/tests/api/test_context.py index 125aff29..e0ce7df1 100644 --- a/climate/tests/api/test_context.py +++ b/climate/tests/api/test_context.py @@ -18,7 +18,6 @@ import json from climate.api import context as api_context from climate import context from climate import exceptions -from climate import policy from climate import tests @@ -48,13 +47,6 @@ class ContextTestCase(tests.TestCase): tenant_id=u'1', user_name=u'user_name') - def test_ctx_from_headers_with_admin(self): - catalog = json.dumps({'nova': 'catalog'}) - self.fake_headers[u'X-Service-Catalog'] = catalog - self.patch(policy, 'enforce').return_value = True - ctx = api_context.ctx_from_headers(self.fake_headers) - self.assertEqual(True, ctx.is_admin) - def test_ctx_from_headers_no_catalog(self): self.assertRaises( exceptions.ServiceCatalogNotFound, diff --git a/climate/tests/db/sqlalchemy/test_sqlalchemy_api.py b/climate/tests/db/sqlalchemy/test_sqlalchemy_api.py index 0ad34385..409d2594 100644 --- a/climate/tests/db/sqlalchemy/test_sqlalchemy_api.py +++ b/climate/tests/db/sqlalchemy/test_sqlalchemy_api.py @@ -15,7 +15,6 @@ import datetime -from climate import context from climate.db.sqlalchemy import api as db_api from climate.db.sqlalchemy import models from climate.openstack.common import uuidutils @@ -160,27 +159,10 @@ class SQLAlchemyDBApiTestCase(tests.DBTestCase): def setUp(self): super(SQLAlchemyDBApiTestCase, self).setUp() - self.set_context(context.ClimateContext(user_id='fake', - tenant_id='fake')) - def test_model_query(self): - db_api.lease_create(_get_fake_virt_lease_values()) - query = db_api.model_query(models.Lease, project_only=None) - self.assertEqual(1, len(query.all())) - query = db_api.model_query(models.Lease, project_only=True) - self.assertEqual(1, len(query.all())) - self.set_context(context.ClimateContext(user_id='fake', - tenant_id='wrong')) - query = db_api.model_query(models.Lease, project_only=True) - self.assertEqual(0, len(query.all())) - - def test_model_query_as_admin(self): - db_api.lease_create(_get_fake_virt_lease_values()) - self.set_context(context.ClimateContext(user_id='fake', - tenant_id='wrong', - is_admin=True)) - query = db_api.model_query(models.Lease, project_only=True) - self.assertEqual(1, len(query.all())) + lease = db_api.lease_create(_get_fake_virt_lease_values()) + query = db_api.model_query(models.Lease) + self.assertEqual([lease.to_dict()], [l.to_dict() for l in query.all()]) def test_create_virt_lease(self): """Create a virtual lease and verify that all tables have been diff --git a/climate/tests/manager/test_rpcapi.py b/climate/tests/manager/test_rpcapi.py index 4fb9b70a..05f844a9 100644 --- a/climate/tests/manager/test_rpcapi.py +++ b/climate/tests/manager/test_rpcapi.py @@ -35,8 +35,8 @@ class RPCAPITestCase(tests.TestCase): self.call.assert_called_once_with('get_lease', lease_id=1) def test_list_leases(self): - self.manager.list_leases() - self.call.assert_called_once_with('list_leases') + self.manager.list_leases('fake') + self.call.assert_called_once_with('list_leases', tenant_id='fake') def test_create_lease(self): self.manager.create_lease(self.fake_values) diff --git a/climate/tests/test_context.py b/climate/tests/test_context.py index 9bc4036b..d35b99e1 100644 --- a/climate/tests/test_context.py +++ b/climate/tests/test_context.py @@ -102,14 +102,3 @@ class TestClimateContext(tests.TestCase): self.assertEqual(ctx.user_id, "user") self.assertEqual(ctx.tenant_id, "tenant") self.assertEqual(ctx.is_admin, True) - - def test_is_user_context(self): - ctx = context.ClimateContext(user_id="user", tenant_id="tenant") - self.assertEqual(True, context.is_user_context(ctx)) - ctx = ctx.elevated() - self.assertEqual(False, context.is_user_context(ctx)) - - def test_is_user_context_with_empty_context(self): - ctx = context.ClimateContext() - self.assertEqual(False, context.is_user_context(ctx)) - self.assertEqual(False, context.is_user_context(None))