From 30d4f0be141e1b4de849e68676ecda5b337164ec Mon Sep 17 00:00:00 2001 From: Wu Wenxiang Date: Sun, 16 Jun 2013 10:47:01 +0800 Subject: Move user fileds type check to identity.Manager The fileds type's checking logic during creating and updating users apply to all driver calls. It should be centralized in the identity.Manager rather that continuing the trend of spreading them out between controllers, managers and drivers. This patch move the enable type checking logic to identity.Manager and modify the related test cases in test_backend.py. Change-Id: I37df56a61cd5ab332dcc9d74a7e99ee9041aa32e --- keystone/identity/backends/kvs.py | 5 -- keystone/identity/backends/ldap/core.py | 6 --- keystone/identity/backends/sql.py | 6 --- keystone/identity/core.py | 15 ++++-- tests/test_backend.py | 95 ++++++++++++++++++++------------- 5 files changed, 70 insertions(+), 57 deletions(-) diff --git a/keystone/identity/backends/kvs.py b/keystone/identity/backends/kvs.py index 339d2e75..2eea08cf 100644 --- a/keystone/identity/backends/kvs.py +++ b/keystone/identity/backends/kvs.py @@ -182,8 +182,6 @@ class Identity(kvs.Base, identity.Driver): # CRUD def create_user(self, user_id, user): - user['name'] = clean.user_name(user['name']) - user['enabled'] = clean.user_enabled(user.get('enabled', True)) try: self.get_user(user_id) except exception.UserNotFound: @@ -214,13 +212,10 @@ class Identity(kvs.Base, identity.Driver): def update_user(self, user_id, user): if 'name' in user: - user['name'] = clean.user_name(user['name']) existing = self.db.get('user_name-%s' % user['name']) if existing and user_id != existing['id']: msg = 'Duplicate name, %s.' % user['name'] raise exception.Conflict(type='user', details=msg) - if 'enabled' in user: - user['enabled'] = clean.user_enabled(user['enabled']) # get the old name and delete it too try: old_user = self.db.get('user-%s' % user_id) diff --git a/keystone/identity/backends/ldap/core.py b/keystone/identity/backends/ldap/core.py index 89ecd06e..d959acee 100644 --- a/keystone/identity/backends/ldap/core.py +++ b/keystone/identity/backends/ldap/core.py @@ -188,17 +188,11 @@ class Identity(identity.Driver): # CRUD def create_user(self, user_id, user): user = self._validate_domain(user) - user['name'] = clean.user_name(user['name']) - user['enabled'] = clean.user_enabled(user.get('enabled', True)) user_ref = self.user.create(user) return self._set_default_domain(identity.filter_user(user_ref)) def update_user(self, user_id, user): user = self._validate_domain(user) - if 'name' in user: - user['name'] = clean.user_name(user['name']) - if 'enabled' in user: - user['enabled'] = clean.user_enabled(user['enabled']) return self._set_default_domain(self.user.update(user_id, user)) def create_project(self, tenant_id, tenant): diff --git a/keystone/identity/backends/sql.py b/keystone/identity/backends/sql.py index b2be8774..f81feb1d 100644 --- a/keystone/identity/backends/sql.py +++ b/keystone/identity/backends/sql.py @@ -626,8 +626,6 @@ class Identity(sql.Base, identity.Driver): @sql.handle_conflicts(type='user') def create_user(self, user_id, user): - user['name'] = clean.user_name(user['name']) - user['enabled'] = clean.user_enabled(user.get('enabled', True)) user = utils.hash_user_password(user) session = self.get_session() with session.begin(): @@ -664,10 +662,6 @@ class Identity(sql.Base, identity.Driver): @sql.handle_conflicts(type='user') def update_user(self, user_id, user): - if 'name' in user: - user['name'] = clean.user_name(user['name']) - if 'enabled' in user: - user['enabled'] = clean.user_enabled(user['enabled']) session = self.get_session() if 'id' in user and user_id != user['id']: raise exception.ValidationError('Cannot change user ID') diff --git a/keystone/identity/core.py b/keystone/identity/core.py index 205c8849..b6f595d1 100644 --- a/keystone/identity/core.py +++ b/keystone/identity/core.py @@ -75,22 +75,29 @@ class Manager(manager.Manager): def create_user(self, context, user_id, user_ref): user = user_ref.copy() + user['name'] = clean.user_name(user['name']) user.setdefault('enabled', True) user['enabled'] = clean.user_enabled(user['enabled']) return self.driver.create_user(user_id, user) + def update_user(self, context, user_id, user_ref): + user = user_ref.copy() + if 'name' in user: + user['name'] = clean.user_name(user['name']) + if 'enabled' in user: + user['enabled'] = clean.user_enabled(user['enabled']) + return self.driver.update_user(user_id, user) + def create_group(self, context, group_id, group_ref): group = group_ref.copy() - if 'description' not in group: - group['description'] = '' + group.setdefault('description', '') return self.driver.create_group(group_id, group) def create_project(self, context, tenant_id, tenant_ref): tenant = tenant_ref.copy() tenant.setdefault('enabled', True) tenant['enabled'] = clean.project_enabled(tenant['enabled']) - if 'description' not in tenant: - tenant['description'] = '' + tenant.setdefault('description', '') return self.driver.create_project(tenant_id, tenant) diff --git a/tests/test_backend.py b/tests/test_backend.py index 9e506928..52af2046 100644 --- a/tests/test_backend.py +++ b/tests/test_backend.py @@ -128,7 +128,7 @@ class IdentityTests(object): 'domain_id': DEFAULT_DOMAIN_ID, 'password': 'no_meta2', } - self.identity_api.create_user(user['id'], user) + self.identity_man.create_user(EMPTY_CONTEXT, user['id'], user) self.identity_api.add_user_to_project(self.tenant_baz['id'], user['id']) user_ref, tenant_ref, metadata_ref = self.identity_man.authenticate( @@ -155,7 +155,7 @@ class IdentityTests(object): 'name': unicode_name, 'domain_id': DEFAULT_DOMAIN_ID, 'password': uuid.uuid4().hex} - ref = self.identity_api.create_user(user['id'], user) + ref = self.identity_man.create_user(EMPTY_CONTEXT, user['id'], user) self.assertEqual(unicode_name, ref['name']) def test_get_project(self): @@ -290,7 +290,8 @@ class IdentityTests(object): self.identity_man.create_user(EMPTY_CONTEXT, 'fake1', user) user['name'] = 'fake2' self.assertRaises(exception.Conflict, - self.identity_man.create_user, EMPTY_CONTEXT, + self.identity_man.create_user, + EMPTY_CONTEXT, 'fake1', user) @@ -303,7 +304,8 @@ class IdentityTests(object): self.identity_man.create_user(EMPTY_CONTEXT, 'fake1', user) user['id'] = 'fake2' self.assertRaises(exception.Conflict, - self.identity_man.create_user, EMPTY_CONTEXT, + self.identity_man.create_user, + EMPTY_CONTEXT, 'fake2', user) @@ -332,7 +334,7 @@ class IdentityTests(object): 'password': uuid.uuid4().hex} self.identity_man.create_user(EMPTY_CONTEXT, user['id'], user) user['domain_id'] = domain2['id'] - self.identity_api.update_user(user['id'], user) + self.identity_man.update_user(EMPTY_CONTEXT, user['id'], user) def test_move_user_between_domains_with_clashing_names_fails(self): domain1 = {'id': uuid.uuid4().hex, 'name': uuid.uuid4().hex} @@ -356,7 +358,8 @@ class IdentityTests(object): # fail since the names clash user1['domain_id'] = domain2['id'] self.assertRaises(exception.Conflict, - self.identity_api.update_user, + self.identity_man.update_user, + EMPTY_CONTEXT, user1['id'], user1) @@ -371,11 +374,12 @@ class IdentityTests(object): 'domain_id': DEFAULT_DOMAIN_ID, 'password': 'fakepass', 'tenants': ['bar']} - self.identity_api.create_user('fake1', user1) - self.identity_api.create_user('fake2', user2) + self.identity_man.create_user(EMPTY_CONTEXT, 'fake1', user1) + self.identity_man.create_user(EMPTY_CONTEXT, 'fake2', user2) user2['name'] = 'fake1' self.assertRaises(exception.Conflict, - self.identity_api.update_user, + self.identity_man.update_user, + EMPTY_CONTEXT, 'fake2', user2) @@ -385,10 +389,11 @@ class IdentityTests(object): 'domain_id': DEFAULT_DOMAIN_ID, 'password': 'fakepass', 'tenants': ['bar']} - self.identity_api.create_user('fake1', user) + self.identity_man.create_user(EMPTY_CONTEXT, 'fake1', user) user['id'] = 'fake2' self.assertRaises(exception.ValidationError, - self.identity_api.update_user, + self.identity_man.update_user, + EMPTY_CONTEXT, 'fake1', user) user_ref = self.identity_api.get_user('fake1') @@ -538,11 +543,15 @@ class IdentityTests(object): new_user1 = {'id': uuid.uuid4().hex, 'name': uuid.uuid4().hex, 'password': uuid.uuid4().hex, 'enabled': True, 'domain_id': new_domain['id']} - self.identity_api.create_user(new_user1['id'], new_user1) + self.identity_man.create_user(EMPTY_CONTEXT, + new_user1['id'], + new_user1) new_user2 = {'id': uuid.uuid4().hex, 'name': uuid.uuid4().hex, 'password': uuid.uuid4().hex, 'enabled': True, 'domain_id': new_domain['id']} - self.identity_api.create_user(new_user2['id'], new_user2) + self.identity_man.create_user(EMPTY_CONTEXT, + new_user2['id'], + new_user2) roles_ref = self.identity_api.list_grants( user_id=new_user1['id'], domain_id=new_domain['id']) @@ -588,7 +597,9 @@ class IdentityTests(object): new_user1 = {'id': uuid.uuid4().hex, 'name': uuid.uuid4().hex, 'password': uuid.uuid4().hex, 'enabled': True, 'domain_id': new_domain['id']} - self.identity_api.create_user(new_user1['id'], new_user1) + self.identity_man.create_user(EMPTY_CONTEXT, + new_user1['id'], + new_user1) self.assertRaises(exception.UserNotFound, self.identity_api.get_roles_for_user_and_domain, @@ -1414,7 +1425,8 @@ class IdentityTests(object): def test_update_user_404(self): user_id = uuid.uuid4().hex self.assertRaises(exception.UserNotFound, - self.identity_api.update_user, + self.identity_man.update_user, + EMPTY_CONTEXT, user_id, {'id': user_id}) @@ -1423,7 +1435,7 @@ class IdentityTests(object): 'name': uuid.uuid4().hex, 'domain_id': DEFAULT_DOMAIN_ID, 'password': uuid.uuid4().hex} - self.identity_api.create_user(user['id'], user) + self.identity_man.create_user(EMPTY_CONTEXT, user['id'], user) self.identity_api.add_user_to_project(self.tenant_bar['id'], user['id']) self.identity_api.delete_user(user['id']) @@ -1436,7 +1448,7 @@ class IdentityTests(object): 'name': uuid.uuid4().hex, 'domain_id': DEFAULT_DOMAIN_ID, 'password': uuid.uuid4().hex} - self.identity_api.create_user(user['id'], user) + self.identity_man.create_user(EMPTY_CONTEXT, user['id'], user) self.identity_api.add_role_to_user_and_project( user['id'], self.tenant_bar['id'], @@ -1526,7 +1538,8 @@ class IdentityTests(object): user = {'id': 'fake1', 'name': 'a' * 65, 'domain_id': DEFAULT_DOMAIN_ID} self.assertRaises(exception.ValidationError, - self.identity_man.create_user, EMPTY_CONTEXT, + self.identity_man.create_user, + EMPTY_CONTEXT, 'fake1', user) @@ -1534,7 +1547,8 @@ class IdentityTests(object): user = {'id': 'fake1', 'name': '', 'domain_id': DEFAULT_DOMAIN_ID} self.assertRaises(exception.ValidationError, - self.identity_man.create_user, EMPTY_CONTEXT, + self.identity_man.create_user, + EMPTY_CONTEXT, 'fake1', user) @@ -1542,14 +1556,16 @@ class IdentityTests(object): user = {'id': 'fake1', 'name': None, 'domain_id': DEFAULT_DOMAIN_ID} self.assertRaises(exception.ValidationError, - self.identity_man.create_user, EMPTY_CONTEXT, + self.identity_man.create_user, + EMPTY_CONTEXT, 'fake1', user) user = {'id': 'fake1', 'name': 123, 'domain_id': DEFAULT_DOMAIN_ID} self.assertRaises(exception.ValidationError, - self.identity_man.create_user, EMPTY_CONTEXT, + self.identity_man.create_user, + EMPTY_CONTEXT, 'fake1', user) @@ -1584,7 +1600,8 @@ class IdentityTests(object): self.identity_man.create_user(EMPTY_CONTEXT, 'fake1', user) user['name'] = 'a' * 65 self.assertRaises(exception.ValidationError, - self.identity_api.update_user, + self.identity_man.update_user, + EMPTY_CONTEXT, 'fake1', user) @@ -1594,7 +1611,8 @@ class IdentityTests(object): self.identity_man.create_user(EMPTY_CONTEXT, 'fake1', user) user['name'] = '' self.assertRaises(exception.ValidationError, - self.identity_api.update_user, + self.identity_man.update_user, + EMPTY_CONTEXT, 'fake1', user) @@ -1605,13 +1623,15 @@ class IdentityTests(object): user['name'] = None self.assertRaises(exception.ValidationError, - self.identity_api.update_user, + self.identity_man.update_user, + EMPTY_CONTEXT, 'fake1', user) user['name'] = 123 self.assertRaises(exception.ValidationError, - self.identity_api.update_user, + self.identity_man.update_user, + EMPTY_CONTEXT, 'fake1', user) @@ -1712,54 +1732,57 @@ class IdentityTests(object): def test_update_user_enable(self): user = {'id': 'fake1', 'name': 'fake1', 'enabled': True, 'domain_id': DEFAULT_DOMAIN_ID} - self.identity_api.create_user('fake1', user) + self.identity_man.create_user(EMPTY_CONTEXT, 'fake1', user) user_ref = self.identity_api.get_user('fake1') self.assertEqual(user_ref['enabled'], True) user['enabled'] = False - self.identity_api.update_user('fake1', user) + self.identity_man.update_user(EMPTY_CONTEXT, 'fake1', user) user_ref = self.identity_api.get_user('fake1') self.assertEqual(user_ref['enabled'], user['enabled']) # If not present, enabled field should not be updated del user['enabled'] - self.identity_api.update_user('fake1', user) + self.identity_man.update_user(EMPTY_CONTEXT, 'fake1', user) user_ref = self.identity_api.get_user('fake1') self.assertEqual(user_ref['enabled'], False) user['enabled'] = True - self.identity_api.update_user('fake1', user) + self.identity_man.update_user(EMPTY_CONTEXT, 'fake1', user) user_ref = self.identity_api.get_user('fake1') self.assertEqual(user_ref['enabled'], user['enabled']) del user['enabled'] - self.identity_api.update_user('fake1', user) + self.identity_man.update_user(EMPTY_CONTEXT, 'fake1', user) user_ref = self.identity_api.get_user('fake1') self.assertEqual(user_ref['enabled'], True) # Integers are valid Python's booleans. Explicitly test it. user['enabled'] = 0 - self.identity_api.update_user('fake1', user) + self.identity_man.update_user(EMPTY_CONTEXT, 'fake1', user) user_ref = self.identity_api.get_user('fake1') self.assertEqual(user_ref['enabled'], False) # Any integers other than 0 are interpreted as True user['enabled'] = -42 - self.identity_api.update_user('fake1', user) + self.identity_man.update_user(EMPTY_CONTEXT, 'fake1', user) user_ref = self.identity_api.get_user('fake1') self.assertEqual(user_ref['enabled'], True) def test_update_user_enable_fails(self): user = {'id': 'fake1', 'name': 'fake1', 'enabled': True, 'domain_id': DEFAULT_DOMAIN_ID} - self.identity_api.create_user('fake1', user) + self.identity_man.create_user(EMPTY_CONTEXT, 'fake1', user) user_ref = self.identity_api.get_user('fake1') self.assertEqual(user_ref['enabled'], True) # Strings are not valid boolean values user['enabled'] = "false" self.assertRaises(exception.ValidationError, - self.identity_api.update_user, 'fake1', user) + self.identity_man.update_user, + EMPTY_CONTEXT, + 'fake1', + user) def test_update_project_enable(self): tenant = {'id': 'fake1', 'name': 'fake1', 'enabled': True, @@ -2025,14 +2048,14 @@ class IdentityTests(object): user = {'domain_id': CONF.identity.default_domain_id, 'id': uuid.uuid4().hex, 'name': uuid.uuid4().hex, 'password': 'passw0rd'} - self.identity_api.create_user(user['id'], user) + self.identity_man.create_user(EMPTY_CONTEXT, user['id'], user) user_ref = self.identity_api.get_user(user['id']) del user['password'] user_ref_dict = dict((x, user_ref[x]) for x in user_ref) self.assertDictContainsSubset(user, user_ref_dict) user['password'] = uuid.uuid4().hex - self.identity_api.update_user(user['id'], user) + self.identity_man.update_user(EMPTY_CONTEXT, user['id'], user) user_ref = self.identity_api.get_user(user['id']) del user['password'] user_ref_dict = dict((x, user_ref[x]) for x in user_ref) -- cgit