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