On 02/19/2013 09:46 PM, Rob Crittenden wrote: > Ana Krivokapic wrote: >> When adding a duplicate member to a group, an error message is issued, >> informing the user that the entry is already a member of the group. This >> message was missing in case of an external member. >> >> Ticket: https://fedorahosted.org/freeipa/ticket/3254 > > This works ok but the sister command, group-remove-member, has the > same problem. Can you add a fix there as well? > > I don't know if there is a way to add a unit test for this since the > external member is validated meaning we'd need to set up trusts as > well. It might be nice to have an optional test that can be run when a > trust is configured to avoid regressions. > > rob > I fixed the group-remove-member command and added unit tests which can be run when the trust is established (they will be skipped when the trust is not established).
I also noticed that, in contrast to group-add-member, group-remove-member did not allow the format 'AD\name' or 'n...@ad.domain.com' for the --external option. I included this fix in the patch, so the two user friendly formats are now supported. Updated patch is attached. -- Regards, Ana Krivokapic Associate Software Engineer FreeIPA team Red Hat Inc.
From 3dd6762f8cd1abc9dae85731edbf2cd84f5f2055 Mon Sep 17 00:00:00 2001 From: Ana Krivokapic <akriv...@redhat.com> Date: Thu, 21 Feb 2013 10:56:03 -0500 Subject: [PATCH] Improve error messages for external group members When adding a duplicate member to a group, an error message is issued, informing the user that the entry is already a member of the group. Similarly, when trying to delete an entry which is not a member, an error message is issued, informing the user that the entry is not a member of the group. These error messages were missing in case of external members. This patch also adds support for using the AD\name or n...@ad.domain.com format in ipa group-remove-member command. This format was supported in group-add-member, but not in group-remove-member. Unit test file covering these cases was also added. https://fedorahosted.org/freeipa/ticket/3254 --- ipalib/plugins/baseldap.py | 4 + ipalib/plugins/group.py | 27 ++++- tests/test_xmlrpc/test_external_members.py | 160 +++++++++++++++++++++++++++++ tests/test_xmlrpc/xmlrpc_test.py | 3 + 4 files changed, 190 insertions(+), 4 deletions(-) create mode 100644 tests/test_xmlrpc/test_external_members.py diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py index 85e2bec36b011fd4539dfadc8d97535bb157bca7..e5b54e3d1c48919c21c3b5893a198de2075bfcac 100644 --- a/ipalib/plugins/baseldap.py +++ b/ipalib/plugins/baseldap.py @@ -382,6 +382,10 @@ def remove_external_post_callback(memberattr, membertype, externalattr, ldap, co external_entries.remove(entry[0]) completed_external += 1 else: + msg = unicode(errors.NotGroupMember().message) + newerror = (entry[0], msg) + ind = failed[memberattr][membertype].index(entry) + failed[memberattr][membertype][ind] = newerror failed_entries.append(membername) if completed_external: diff --git a/ipalib/plugins/group.py b/ipalib/plugins/group.py index 06e80931a0d77beb93b08cdf2637e3c750c1bafa..3c8926f9305326af605464eafe6bdc439419dce3 100644 --- a/ipalib/plugins/group.py +++ b/ipalib/plugins/group.py @@ -397,7 +397,7 @@ class group_add_member(LDAPAddMember): result = add_external_post_callback('member', 'group', 'ipaexternalmember', ldap, completed, failed, dn, entry_attrs, keys, options, external_callback_normalize=False) - failed['member']['group'] = restore + failed_sids + failed['member']['group'] += restore + failed_sids return result api.register(group_add_member) @@ -424,15 +424,34 @@ class group_remove_member(LDAPRemoveMember): assert isinstance(dn, DN) result = (completed, dn) if 'ipaexternalmember' in options: - sids = options['ipaexternalmember'] - restore = list() + if not _dcerpc_bindings_installed: + raise errors.NotFound(reason=_('Cannot perform external member validation without ' + 'Samba 4 support installed. Make sure you have installed ' + 'server-trust-ad sub-package of IPA on the server')) + domain_validator = ipaserver.dcerpc.DomainValidator(self.api) + if not domain_validator.is_configured(): + raise errors.NotFound(reason=_('Cannot perform join operation without own domain configured. ' + 'Make sure you have run ipa-adtrust-install on the IPA server first')) + sids = [] + failed_sids = [] + for sid in options['ipaexternalmember']: + if domain_validator.is_trusted_sid_valid(sid): + sids.append(sid) + else: + try: + actual_sid = domain_validator.get_trusted_domain_object_sid(sid) + except errors.PublicError, e: + failed_sids.append((sid, unicode(e))) + else: + sids.append(actual_sid) + restore = [] if 'member' in failed and 'group' in failed['member']: restore = failed['member']['group'] failed['member']['group'] = list((id,id) for id in sids) result = remove_external_post_callback('member', 'group', 'ipaexternalmember', ldap, completed, failed, dn, entry_attrs, keys, options) - failed['member']['group'] = restore + failed['member']['group'] += restore + failed_sids return result api.register(group_remove_member) diff --git a/tests/test_xmlrpc/test_external_members.py b/tests/test_xmlrpc/test_external_members.py new file mode 100644 index 0000000000000000000000000000000000000000..6f9949f4555c66bf5033f72a531108d44ca46e42 --- /dev/null +++ b/tests/test_xmlrpc/test_external_members.py @@ -0,0 +1,160 @@ +# Authors: +# Ana Krivokapic <akriv...@redhat.com> +# +# Copyright (C) 2013 Red Hat +# see file 'COPYING' for use and warranty information +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +""" +Test adding/removing external members (trusted domain objects) to IPA groups. +These tests are skipped if trust is not established. +""" + +import nose +from ipalib import api +from ipapython.dn import DN +from tests.test_xmlrpc import objectclasses +from xmlrpc_test import Declarative, fuzzy_uuid, fuzzy_user_or_group_sid + +group_name = u'external_group' +group_desc = u'Test external group' +group_dn = DN(('cn', group_name), api.env.container_group, api.env.basedn) + + +def get_trusted_group_name(): + trusts = api.Command['trust_find']() + if trusts['count'] == 0: + return None + + ad_netbios = trusts['result'][0]['ipantflatname'] + return u'%s\Domain Admins' % ad_netbios + + +class test_external_members(Declarative): + @classmethod + def setUpClass(cls): + super(test_external_members, cls).setUpClass() + if not api.Backend.xmlclient.isconnected(): + api.Backend.xmlclient.connect(fallback=False) + + trusts = api.Command['trust_find']() + if trusts['count'] == 0: + raise nose.SkipTest('Trust is not established') + + cleanup_commands = [ + ('group_del', [group_name], {}), + ] + + tests = [ + dict( + desc='Create external group "%s"' % group_name, + command=( + 'group_add', [group_name], dict(description=group_desc, external=True) + ), + expected=dict( + value=group_name, + summary=u'Added group "%s"' % group_name, + result=dict( + cn=[group_name], + description=[group_desc], + objectclass=objectclasses.externalgroup, + ipauniqueid=[fuzzy_uuid], + dn=group_dn, + ), + ), + ), + dict( + desc='Add external member "%s" to group "%s"' % (get_trusted_group_name(), group_name), + command=( + 'group_add_member', [group_name], dict(ipaexternalmember=get_trusted_group_name()) + ), + expected=dict( + completed=1, + failed=dict( + member=dict( + group=tuple(), + user=tuple(), + ), + ), + result=dict( + dn=group_dn, + ipaexternalmember=[fuzzy_user_or_group_sid], + cn=[group_name], + description=[group_desc], + ), + ), + ), + dict( + desc='Try to add duplicate external member "%s" to group "%s"' % (get_trusted_group_name(), group_name), + command=( + 'group_add_member', [group_name], dict(ipaexternalmember=get_trusted_group_name()) + ), + expected=dict( + completed=0, + failed=dict( + member=dict( + group=[(fuzzy_user_or_group_sid, u'This entry is already a member')], + user=tuple(), + ), + ), + result=dict( + dn=group_dn, + ipaexternalmember=[fuzzy_user_or_group_sid], + cn=[group_name], + description=[group_desc], + ), + ), + ), + dict( + desc='Remove external member "%s" from group "%s"' % (get_trusted_group_name(), group_name), + command=( + 'group_remove_member', [group_name], dict(ipaexternalmember=get_trusted_group_name()) + ), + expected=dict( + completed=1, + failed=dict( + member=dict( + group=tuple(), + user=tuple(), + ), + ), + result=dict( + dn=group_dn, + cn=[group_name], + ipaexternalmember=[], + description=[group_desc], + ), + ), + ), + dict( + desc='Try to remove external entry "%s" which is not a member of group "%s" from group "%s"' % (get_trusted_group_name(), group_name, group_name), + command=( + 'group_remove_member', [group_name], dict(ipaexternalmember=get_trusted_group_name()) + ), + expected=dict( + completed=0, + failed=dict( + member=dict( + group=[(fuzzy_user_or_group_sid, u'This entry is not a member')], + user=tuple(), + ), + ), + result=dict( + dn=group_dn, + cn=[group_name], + description=[group_desc], + ), + ), + ), + ] diff --git a/tests/test_xmlrpc/xmlrpc_test.py b/tests/test_xmlrpc/xmlrpc_test.py index 0a046b454d200064df938ad295252f6d7fb5b866..2567863b90fc9f04d80281edf64e8342adb71cf0 100644 --- a/tests/test_xmlrpc/xmlrpc_test.py +++ b/tests/test_xmlrpc/xmlrpc_test.py @@ -49,6 +49,9 @@ _sid_identifier_authority = '(0x[0-9a-f]{1,12}|[0-9]{1,10})' fuzzy_domain_sid = Fuzzy( '^S-1-5-21-%(idauth)s-%(idauth)s-%(idauth)s$' % dict(idauth=_sid_identifier_authority) ) +fuzzy_user_or_group_sid = Fuzzy( + '^S-1-5-21-%(idauth)s-%(idauth)s-%(idauth)s-%(idauth)s$' % dict(idauth=_sid_identifier_authority) +) # Matches netgroup dn. Note (?i) at the beginning of the regexp is the ingnore case flag fuzzy_netgroupdn = Fuzzy( -- 1.8.1.2
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
