From 401294da9a7babdd6be5c7f19aef0e07811ba09e Mon Sep 17 00:00:00 2001 From: Adam Young Date: Tue, 18 Feb 2014 22:35:34 -0800 Subject: [PATCH] Reduce excess LDAP searches Many LDAP based calls are looking up user and group entries multiple times when it is not necessary. Converting from a user or group object to a DN requires a lookup which is wasteful. Instead, we add the DN to the object and filter it off before returning it to the end user. There were also search operations being performed before issuing modify operations in an attempt to check if the entry exists. The modify operations can just be attempted and we can check for an LDAP NO_SUCH_OBJECT exception instead. This reduces the number of search operations that we need to perform. The remove_user_from_group method in the SQL identity driver did not match the other drivers with regards to the exceptions it returns when the user or group does not exist. Since new tests were added to check these exceptions, the SQL driver was modified to match the behavior of the other drivers. The LDAP version of test_attribute_update is skipped as part of this patch as it was causing failures in the live_tests. It tests Blank values in a required field which is an error in LDAP. Closes-Bug: 1230260 Co-Authored By: Nathan Kinder Change-Id: I2b740412b6ca38dafceb29c6b35556b5869b1658 --- keystone/assignment/backends/ldap.py | 19 +++-- keystone/common/ldap/core.py | 17 +++- keystone/identity/backends/ldap.py | 115 ++++++++++++++++++--------- keystone/identity/backends/sql.py | 14 +++- keystone/tests/test_backend.py | 45 +++++++++-- keystone/tests/test_backend_ldap.py | 56 +++++++++++++ 6 files changed, 212 insertions(+), 54 deletions(-) diff --git a/keystone/assignment/backends/ldap.py b/keystone/assignment/backends/ldap.py index 54b2e43f9..1f61705a9 100644 --- a/keystone/assignment/backends/ldap.py +++ b/keystone/assignment/backends/ldap.py @@ -540,7 +540,10 @@ class RoleApi(common_ldap.BaseLdap): role_id) def get_role_assignments(self, tenant_dn): - roles = self._ldap_get_list(tenant_dn, ldap.SCOPE_ONELEVEL) + try: + roles = self._ldap_get_list(tenant_dn, ldap.SCOPE_ONELEVEL) + except ldap.NO_SUCH_OBJECT: + roles = [] res = [] for role_dn, attrs in roles: try: @@ -564,9 +567,12 @@ class RoleApi(common_ldap.BaseLdap): user_dn=user_dn) for role in roles] def list_project_roles_for_user(self, user_dn, project_subtree): - roles = self._ldap_get_list(project_subtree, ldap.SCOPE_SUBTREE, - query_params={ - self.member_attribute: user_dn}) + try: + roles = self._ldap_get_list(project_subtree, ldap.SCOPE_SUBTREE, + query_params={ + self.member_attribute: user_dn}) + except ldap.NO_SUCH_OBJECT: + roles = [] res = [] for role_dn, _ in roles: # ldap.dn.dn2str returns an array, where the first @@ -626,7 +632,10 @@ class RoleApi(common_ldap.BaseLdap): """Returns a list of all the role assignments linked to project_tree_dn attribute. """ - roles = self._ldap_get_list(project_tree_dn, ldap.SCOPE_SUBTREE) + try: + roles = self._ldap_get_list(project_tree_dn, ldap.SCOPE_SUBTREE) + except ldap.NO_SUCH_OBJECT: + roles = [] res = [] for role_dn, role in roles: tenant = ldap.dn.str2dn(role_dn) diff --git a/keystone/common/ldap/core.py b/keystone/common/ldap/core.py index 68ff04a78..9866c74b4 100644 --- a/keystone/common/ldap/core.py +++ b/keystone/common/ldap/core.py @@ -641,6 +641,21 @@ def _get_connection(conn_url): return PythonLDAPHandler() +def filter_entity(entity_ref): + """Filter out private items in an entity dict. + + :param entity_ref: the entity dictionary. The 'dn' field will be removed. + 'dn' is used in LDAP, but should not be returned to the user. This + value may be modified. + + :returns: entity_ref + + """ + if entity_ref: + entity_ref.pop('dn', None) + return entity_ref + + class BaseLdap(object): DEFAULT_SUFFIX = "dc=example,dc=com" DEFAULT_OU = None @@ -942,8 +957,6 @@ class BaseLdap(object): six.iteritems(query_params)]))) try: return conn.search_s(search_base, scope, query, attrlist) - except ldap.NO_SUCH_OBJECT: - return [] finally: conn.unbind_s() diff --git a/keystone/identity/backends/ldap.py b/keystone/identity/backends/ldap.py index 1cae458b7..f63a96dff 100644 --- a/keystone/identity/backends/ldap.py +++ b/keystone/identity/backends/ldap.py @@ -60,7 +60,7 @@ class Identity(identity.Driver): raise AssertionError(_('Invalid user / password')) conn = None try: - conn = self.user.get_connection(self.user._id_to_dn(user_id), + conn = self.user.get_connection(user_ref['dn'], password) if not conn: raise AssertionError(_('Invalid user / password')) @@ -69,13 +69,13 @@ class Identity(identity.Driver): finally: if conn: conn.unbind_s() - return identity.filter_user(user_ref) + return self.user.filter_attributes(user_ref) def _get_user(self, user_id): return self.user.get(user_id) def get_user(self, user_id): - return identity.filter_user(self._get_user(user_id)) + return self.user.get_filtered(user_id) def list_users(self, hints): return self.user.get_all_filtered() @@ -83,13 +83,13 @@ class Identity(identity.Driver): def get_user_by_name(self, user_name, domain_id): # domain_id will already have been handled in the Manager layer, # parameter left in so this matches the Driver specification - return identity.filter_user(self.user.get_by_name(user_name)) + return self.user.filter_attributes(self.user.get_by_name(user_name)) # CRUD def create_user(self, user_id, user): self.user.check_allow_create() user_ref = self.user.create(user) - return identity.filter_user(user_ref) + return self.user.filter_attributes(user_ref) def update_user(self, user_id, user): self.user.check_allow_update() @@ -107,57 +107,53 @@ class Identity(identity.Driver): def delete_user(self, user_id): self.user.check_allow_delete() self.assignment_api.delete_user(user_id) - user_dn = self.user._id_to_dn(user_id) + user = self.user.get(user_id) + user_dn = user['dn'] groups = self.group.list_user_groups(user_dn) for group in groups: self.group.remove_user(user_dn, group['id'], user_id) - user = self.user.get(user_id) if hasattr(user, 'tenant_id'): - self.project.remove_user(user.tenant_id, - self.user._id_to_dn(user_id)) + self.project.remove_user(user.tenant_id, user_dn) self.user.delete(user_id) def create_group(self, group_id, group): self.group.check_allow_create() group['name'] = clean.group_name(group['name']) - return self.group.create(group) + return common_ldap.filter_entity(self.group.create(group)) def get_group(self, group_id): - return self.group.get(group_id) + return self.group.get_filtered(group_id) def update_group(self, group_id, group): self.group.check_allow_update() if 'name' in group: group['name'] = clean.group_name(group['name']) - return self.group.update(group_id, group) + return common_ldap.filter_entity(self.group.update(group_id, group)) def delete_group(self, group_id): self.group.check_allow_delete() return self.group.delete(group_id) def add_user_to_group(self, user_id, group_id): - self.get_user(user_id) - self.get_group(group_id) - user_dn = self.user._id_to_dn(user_id) + user_ref = self._get_user(user_id) + user_dn = user_ref['dn'] self.group.add_user(user_dn, group_id, user_id) def remove_user_from_group(self, user_id, group_id): - self.get_user(user_id) - self.get_group(group_id) - user_dn = self.user._id_to_dn(user_id) + user_ref = self._get_user(user_id) + user_dn = user_ref['dn'] self.group.remove_user(user_dn, group_id, user_id) def list_groups_for_user(self, user_id, hints): - self.get_user(user_id) - user_dn = self.user._id_to_dn(user_id) - return self.group.list_user_groups(user_dn) + user_ref = self._get_user(user_id) + user_dn = user_ref['dn'] + return self.group.list_user_groups_filtered(user_dn) def list_groups(self, hints): - return self.group.get_all() + return self.group.get_all_filtered() def list_users_in_group(self, group_id, hints): - self.get_group(group_id) users = [] for user_dn in self.group.list_group_users(group_id): user_id = self.user._dn_to_id(user_dn) @@ -171,14 +167,18 @@ class Identity(identity.Driver): return users def check_user_in_group(self, user_id, group_id): - self.get_user(user_id) - self.get_group(group_id) user_refs = self.list_users_in_group(group_id, driver_hints.Hints()) for x in user_refs: if x['id'] == user_id: break else: - raise exception.NotFound(_('User not found in group')) + # Try to fetch the user to see if it even exists. This + # will raise a more accurate exception. + self.get_user(user_id) + raise exception.NotFound(_("User '%(user_id)s' not found in" + " group '%(group_id)s'") % + {'user_id': user_id, + 'group_id': group_id}) # TODO(termie): turn this into a data object and move logic to driver @@ -209,6 +209,8 @@ class UserApi(common_ldap.EnabledEmuMixIn, common_ldap.BaseLdap): enabled = int(obj.get('enabled', self.enabled_default)) obj['enabled'] = ((enabled & self.enabled_mask) != self.enabled_mask) + obj['dn'] = res[0] + return obj def mask_enabled_attribute(self, values): @@ -235,10 +237,13 @@ class UserApi(common_ldap.EnabledEmuMixIn, common_ldap.BaseLdap): def get_filtered(self, user_id): user = self.get(user_id) - return identity.filter_user(user) + return self.filter_attributes(user) def get_all_filtered(self): - return [identity.filter_user(user) for user in self.get_all()] + return [self.filter_attributes(user) for user in self.get_all()] + + def filter_attributes(self, user): + return identity.filter_user(common_ldap.filter_entity(user)) class GroupApi(common_ldap.BaseLdap): @@ -254,6 +259,11 @@ class GroupApi(common_ldap.BaseLdap): immutable_attrs = ['name'] model = models.Group + def _ldap_res_to_model(self, res): + model = super(GroupApi, self)._ldap_res_to_model(res) + model['dn'] = res[0] + return model + def __init__(self, conf): super(GroupApi, self).__init__(conf) self.member_attribute = (getattr(conf.ldap, 'group_member_attribute') @@ -275,11 +285,12 @@ class GroupApi(common_ldap.BaseLdap): # role support which will be added under bug 1101287 query = '(objectClass=%s)' % self.object_class - dn = self._id_to_dn(group_id) - if dn: + group_ref = self.get(group_id) + group_dn = group_ref['dn'] + if group_dn: try: conn = self.get_connection() - roles = conn.search_s(dn, ldap.SCOPE_ONELEVEL, + roles = conn.search_s(group_dn, ldap.SCOPE_ONELEVEL, query, ['%s' % '1.1']) for role_dn, _ in roles: conn.delete_s(role_dn) @@ -294,17 +305,20 @@ class GroupApi(common_ldap.BaseLdap): return super(GroupApi, self).update(group_id, values, old_obj) def add_user(self, user_dn, group_id, user_id): + group_ref = self.get(group_id) + group_dn = group_ref['dn'] try: - super(GroupApi, self).add_member(user_dn, self._id_to_dn(group_id)) + super(GroupApi, self).add_member(user_dn, group_dn) except exception.Conflict: raise exception.Conflict(_( 'User %(user_id)s is already a member of group %(group_id)s') % {'user_id': user_id, 'group_id': group_id}) def remove_user(self, user_dn, group_id, user_id): + group_ref = self.get(group_id) + group_dn = group_ref['dn'] try: - super(GroupApi, self).remove_member(user_dn, - self._id_to_dn(group_id)) + super(GroupApi, self).remove_member(user_dn, group_dn) except ldap.NO_SUCH_ATTRIBUTE: raise exception.UserNotFound(user_id=user_id) @@ -316,14 +330,29 @@ class GroupApi(common_ldap.BaseLdap): self.member_attribute, user_dn_esc, self.ldap_filter or '') - memberships = self.get_all(query) - return memberships + return self.get_all(query) + + def list_user_groups_filtered(self, user_dn): + """Return a filtered list of groups for which the user is a member.""" + + user_dn_esc = ldap.filter.escape_filter_chars(user_dn) + query = '(&(objectClass=%s)(%s=%s)%s)' % (self.object_class, + self.member_attribute, + user_dn_esc, + self.ldap_filter or '') + return self.get_all_filtered(query) def list_group_users(self, group_id): """Return a list of user dns which are members of a group.""" - group_dn = self._id_to_dn(group_id) - attrs = self._ldap_get_list(group_dn, ldap.SCOPE_BASE, - attrlist=(self.member_attribute,)) + group_ref = self.get(group_id) + group_dn = group_ref['dn'] + + try: + attrs = self._ldap_get_list(group_dn, ldap.SCOPE_BASE, + attrlist=(self.member_attribute,)) + except ldap.NO_SUCH_OBJECT: + raise self.NotFound(group_id=group_id) + users = [] for dn, member in attrs: user_dns = member.get(self.member_attribute, []) @@ -332,3 +361,11 @@ class GroupApi(common_ldap.BaseLdap): continue users.append(user_dn) return users + + def get_filtered(self, group_id): + group = self.get(group_id) + return common_ldap.filter_entity(group) + + def get_all_filtered(self, query=None): + return [common_ldap.filter_entity(group) + for group in self.get_all(query)] diff --git a/keystone/identity/backends/sql.py b/keystone/identity/backends/sql.py index 37f2df309..b17bf1066 100644 --- a/keystone/identity/backends/sql.py +++ b/keystone/identity/backends/sql.py @@ -187,7 +187,10 @@ class Identity(identity.Driver): query = query.filter_by(user_id=user_id) query = query.filter_by(group_id=group_id) if not query.first(): - raise exception.NotFound(_('User not found in group')) + raise exception.NotFound(_("User '%(user_id)s' not found in" + " group '%(group_id)s'") % + {'user_id': user_id, + 'group_id': group_id}) def remove_user_from_group(self, user_id, group_id): session = sql.get_session() @@ -198,7 +201,14 @@ class Identity(identity.Driver): query = query.filter_by(group_id=group_id) membership_ref = query.first() if membership_ref is None: - raise exception.NotFound(_('User not found in group')) + # Check if the group and user exist to return descriptive + # exceptions. + self.get_group(group_id) + self.get_user(user_id) + raise exception.NotFound(_("User '%(user_id)s' not found in" + " group '%(group_id)s'") % + {'user_id': user_id, + 'group_id': group_id}) with session.begin(): session.delete(membership_ref) diff --git a/keystone/tests/test_backend.py b/keystone/tests/test_backend.py index d5add64f9..725ef304b 100644 --- a/keystone/tests/test_backend.py +++ b/keystone/tests/test_backend.py @@ -2239,6 +2239,11 @@ class IdentityTests(object): uuid.uuid4().hex, new_group['id']) + self.assertRaises(exception.NotFound, + self.identity_api.add_user_to_group, + uuid.uuid4().hex, + uuid.uuid4().hex) + def test_check_user_in_group(self): domain = self._get_domain_fixture() new_group = {'id': uuid.uuid4().hex, 'domain_id': domain['id'], @@ -2272,10 +2277,6 @@ class IdentityTests(object): 'domain_id': DEFAULT_DOMAIN_ID, 'name': uuid.uuid4().hex} self.identity_api.create_group(new_group['id'], new_group) - self.assertRaises(exception.UserNotFound, - self.identity_api.check_user_in_group, - uuid.uuid4().hex, - new_group['id']) new_user = {'id': uuid.uuid4().hex, 'name': 'new_user', 'password': uuid.uuid4().hex, 'enabled': True, @@ -2287,6 +2288,33 @@ class IdentityTests(object): new_user['id'], new_group['id']) + def test_check_user_in_group_404(self): + new_user = {'id': uuid.uuid4().hex, 'name': 'new_user', + 'password': uuid.uuid4().hex, 'enabled': True, + 'domain_id': DEFAULT_DOMAIN_ID} + self.identity_api.create_user(new_user['id'], new_user) + + new_group = { + 'id': uuid.uuid4().hex, + 'domain_id': DEFAULT_DOMAIN_ID, + 'name': uuid.uuid4().hex} + self.identity_api.create_group(new_group['id'], new_group) + + self.assertRaises(exception.UserNotFound, + self.identity_api.check_user_in_group, + uuid.uuid4().hex, + new_group['id']) + + self.assertRaises(exception.GroupNotFound, + self.identity_api.check_user_in_group, + new_user['id'], + uuid.uuid4().hex) + + self.assertRaises(exception.NotFound, + self.identity_api.check_user_in_group, + uuid.uuid4().hex, + uuid.uuid4().hex) + def test_list_users_in_group(self): domain = self._get_domain_fixture() new_group = {'id': uuid.uuid4().hex, 'domain_id': domain['id'], @@ -2311,6 +2339,11 @@ class IdentityTests(object): self.assertNotIn('password', x) self.assertTrue(found) + def test_list_users_in_group_404(self): + self.assertRaises(exception.GroupNotFound, + self.identity_api.list_users_in_group, + uuid.uuid4().hex) + def test_list_groups_for_user(self): domain = self._get_domain_fixture() test_groups = [] @@ -2405,12 +2438,12 @@ class IdentityTests(object): new_group = {'id': uuid.uuid4().hex, 'domain_id': domain['id'], 'name': uuid.uuid4().hex} self.identity_api.create_group(new_group['id'], new_group) - self.assertRaises(exception.NotFound, + self.assertRaises(exception.GroupNotFound, self.identity_api.remove_user_from_group, new_user['id'], uuid.uuid4().hex) - self.assertRaises(exception.NotFound, + self.assertRaises(exception.UserNotFound, self.identity_api.remove_user_from_group, uuid.uuid4().hex, new_group['id']) diff --git a/keystone/tests/test_backend_ldap.py b/keystone/tests/test_backend_ldap.py index c08a22184..70627b776 100644 --- a/keystone/tests/test_backend_ldap.py +++ b/keystone/tests/test_backend_ldap.py @@ -547,6 +547,9 @@ class BaseLDAPIdentity(test_backend.IdentityTests): self.assertRaises(exception.Conflict, super(BaseLDAPIdentity, self).test_update_user_name) + def test_attribute_update(self): + self.skipTest("Blank value in a required field is an error in LDAP") + def test_arbitrary_attributes_are_returned_from_create_user(self): self.skipTest("Using arbitrary attributes doesn't work under LDAP") @@ -1275,6 +1278,59 @@ class LDAPIdentity(BaseLDAPIdentity, tests.TestCase): self.assertEqual(ldap.DEREF_SEARCHING, conn.get_option(ldap.OPT_DEREF)) + def test_list_users_no_dn(self): + users = self.identity_api.list_users() + self.assertEqual(len(default_fixtures.USERS), len(users)) + user_ids = set(user['id'] for user in users) + expected_user_ids = set(user['id'] for user in default_fixtures.USERS) + for user_ref in users: + self.assertNotIn('dn', user_ref) + self.assertEqual(expected_user_ids, user_ids) + + def test_list_groups_no_dn(self): + # Create some test groups. + domain = self._get_domain_fixture() + expected_group_ids = [] + numgroups = 3 + for _ in range(numgroups): + group = {'id': uuid.uuid4().hex, 'name': uuid.uuid4().hex, + 'domain_id': domain['id']} + self.identity_api.create_group(group['id'], group) + expected_group_ids.append(group['id']) + # Fetch the test groups and ensure that they don't contain a dn. + groups = self.identity_api.list_groups() + self.assertEqual(numgroups, len(groups)) + group_ids = set(group['id'] for group in groups) + for group_ref in groups: + self.assertNotIn('dn', group_ref) + self.assertEqual(set(expected_group_ids), group_ids) + + def test_list_groups_for_user_no_dn(self): + # Create a test user. + user = {'id': uuid.uuid4().hex, 'name': uuid.uuid4().hex, + 'domain_id': CONF.identity.default_domain_id, + 'password': uuid.uuid4().hex, + 'enabled': True} + self.identity_api.create_user(user['id'], user) + # Create some test groups and add the test user as a member. + domain = self._get_domain_fixture() + expected_group_ids = [] + numgroups = 3 + for _ in range(numgroups): + group = {'id': uuid.uuid4().hex, 'name': uuid.uuid4().hex, + 'domain_id': domain['id']} + self.identity_api.create_group(group['id'], group) + expected_group_ids.append(group['id']) + self.identity_api.add_user_to_group(user['id'], group['id']) + # Fetch the groups for the test user + # and ensure they don't contain a dn. + groups = self.identity_api.list_groups_for_user(user['id']) + self.assertEqual(numgroups, len(groups)) + group_ids = set(group['id'] for group in groups) + for group_ref in groups: + self.assertNotIn('dn', group_ref) + self.assertEqual(set(expected_group_ids), group_ids) + class LDAPIdentityEnabledEmulation(LDAPIdentity): def setUp(self):