openstack/deb-keystone
Code Issues Proposed changes

Reduce excess LDAP searches

Browse Source 
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 <nkinder@redhat.com>

Change-Id: I2b740412b6ca38dafceb29c6b35556b5869b1658
This commit is contained in:
Adam Young 2014-02-18 22:35:34 -08:00 committed by Nathan Kinder
parent 820e4f153a
commit 401294da9a
6 changed files with 212 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
keystone/assignment/backends/ldap.py
View File

@ -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)

17
keystone/common/ldap/core.py
View File

@ -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()

115
keystone/identity/backends/ldap.py
View File

@ -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)]

14
keystone/identity/backends/sql.py
View File

@ -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)

45
keystone/tests/test_backend.py
View File

@ -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'])

56
keystone/tests/test_backend_ldap.py
View File

@ -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):