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

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)