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

4 files changed, 190 insertions, 4 deletions

diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 3d013ced9..bb0de989c 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -391,6 +391,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 bde002a8d..21ee00490 100644
--- a/ipalib/plugins/group.py
+++ b/ipalib/plugins/group.py
@@ -398,7 +398,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)
@@ -425,15 +425,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 000000000..6f9949f45
--- /dev/null
+++ b/tests/test_xmlrpc/test_external_members.py
@@ -0,0 +1,160 @@
+# Authors:
+#   Ana Krivokapic <akrivoka@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 89a653088..cfd7bd339 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(