<html> <head> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> <div class="moz-cite-prefix">On 08/22/2013 06:13 PM, Tomas Babej wrote: </div> <blockquote cite="mid:5216389F.1070907@redhat.com" type="cite"> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 08/20/2013 04:14 PM, Ana Krivokapic wrote: </div> <blockquote cite="mid:521379DD.5050607@redhat.com" type="cite"> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 08/09/2013 05:35 PM, Tomas Babej wrote: </div> <blockquote cite="mid:52050C3E.4020101@redhat.com" type="cite"> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 08/09/2013 04:03 PM, Ana Krivokapic wrote: </div> <blockquote cite="mid:5204F6CC.2090503@redhat.com" type="cite"> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 08/09/2013 09:39 AM, Tomas Babej wrote: </div> <blockquote cite="mid:52049C9F.6080706@redhat.com" type="cite"> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 08/08/2013 04:09 PM, Ana Krivokapic wrote: </div> <blockquote cite="mid:5203A683.4040907@redhat.com" type="cite"> <pre wrap="">Hello, This patch should fix the failing unit tests. <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/3852">https://fedorahosted.org/freeipa/ticket/3852</a> </pre> <fieldset class="mimeAttachmentHeader"></fieldset> <pre wrap="">_______________________________________________ Freeipa-devel mailing list <a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:Freeipa-devel@redhat.com">Freeipa-devel@redhat.com</a> <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/freeipa-devel">https://www.redhat.com/mailman/listinfo/freeipa-devel</a></pre> </blockquote> There are two tests failing on my machine when running the tests after ipa-adtrust-install with your patch applied: </blockquote> You say there are two tests failing but I only see one below. </blockquote> That was just debris from trying to break your patch too much, one of my comments rendered invalid in the end :) <blockquote cite="mid:5204F6CC.2090503@redhat.com" type="cite"> <blockquote cite="mid:52049C9F.6080706@redhat.com" type="cite"> ====================================================================== FAIL: test_group[24]: group_find: Search for POSIX groups ---------------------------------------------------------------------- Traceback (most recent call last): [...] AssertionError: assert_deepequal: dict keys mismatch. test_group[24]: group_find: Search for POSIX groups missing keys = [] extra keys = ['ipantsecurityidentifier'] expected = {'dn': ipapython.dn.DN('cn=editors,cn=groups,cn=accounts,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com'), 'cn': [u'editors'], 'objectclass': Fuzzy(None, None, <function <lambda> at 0x3768c08>), 'gidnumber': [Fuzzy('^\\d+$', <type 'basestring'>, None)], 'ipauniqueid': [Fuzzy('^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$', <type 'unicode'>, None)], 'description': [u'Limited admins who can edit other users']} got = {'dn': u'cn=editors,cn=groups,cn=accounts,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com', 'cn': (u'editors',), 'objectclass': (u'top', u'groupofnames', u'posixgroup', u'ipausergroup', u'ipaobject', u'nestedGroup', u'ipantgroupattrs'), 'ipantsecurityidentifier': (u'S-1-5-21-1457515837-642396627-3509099663-1002',), 'gidnumber': (u'1804600002',), 'ipauniqueid': (u'7c6e1672-0039-11e3-9567-001a4a2221fb',), 'description': (u'Limited admins who can edit other users',)} path = ('result', 1) I think you need the wrap the dictionary discribing the editor's group entry with the add_sid wrapper, and its objectclasses using the add_oc wrapper. [tbabej@vm-139 freeipa]$ git diff diff --git a/ipatests/test_xmlrpc/test_group_plugin.py b/ipatests/test_xmlrpc/test_group_plugin.py index d380fe5..14c70cd 100644 --- a/ipatests/test_xmlrpc/test_group_plugin.py +++ b/ipatests/test_xmlrpc/test_group_plugin.py @@ -447,14 +447,15 @@ class test_group(Declarative): objectclasses.posixgroup, u'ipantgroupattrs')), 'ipauniqueid': [fuzzy_uuid], }), - { + add_sid({ 'dn': get_group_dn('editors'), 'gidnumber': [fuzzy_digits], 'cn': [u'editors'], 'description': [u'Limited admins who can edit other users'], - 'objectclass': fuzzy_set_ci(objectclasses.posixgroup), + 'objectclass': fuzzy_set_ci(add_oc( + objectclasses.posixgroup, u'ipantgroupattrs')), 'ipauniqueid': [fuzzy_uuid], - }, + }), dict( dn=get_group_dn(group1), cn=[group1], These changes were sufficient for me to have the unit test suite run without errors. <pre class="moz-signature" cols="72">-- Tomas Babej Associate Software Engeneer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org</pre> </blockquote> I retested the patch and the tests are passing in my setup. The editors group definitely does not have the ipantsecurityidentifier attribute nor the ipantgroupattrs objectclass: [akrivoka@vm-181 freeipa]$ ipa group-show editors --all dn: cn=editors,cn=groups,cn=accounts,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com Group name: editors Description: Limited admins who can edit other users GID: 1977000002 ipauniqueid: 91b3597e-00f3-11e3-92ae-001a4a22217b objectclass: top, groupofnames, posixgroup, ipausergroup, ipaobject, nestedGroup What I noticed though, is that if I delete and re-create the editors group (after ipa-adtrust-install has been run), it then gets the above mentioned attribute and objectclass. Maybe you did some similar manipulation in your setup, resulting in the test failing? </blockquote> I think it does depend on whether you have ran the ipa-sidgen task when running the ipa-adtrust-install. Do you think we can cover both cases here? <blockquote cite="mid:5204F6CC.2090503@redhat.com" type="cite"> <pre class="moz-signature" cols="80">-- Regards, Ana Krivokapic Associate Software Engineer FreeIPA team Red Hat Inc.</pre> </blockquote> <pre class="moz-signature" cols="72">-- Tomas Babej Associate Software Engeneer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org</pre> </blockquote> Updated patch should detect the situation when ipa-sidgen task was run, and add the required attribute/objectclass accordingly. <pre class="moz-signature" cols="80">-- Regards, Ana Krivokapic Associate Software Engineer FreeIPA team Red Hat Inc.</pre> </blockquote> + + +class sidgen_was_run(Command): + NO_CLI = True + + __doc__ = _('Determine whether ipa-adtrust-install has been run with ' + 'sidgen task') + + def execute(self, *keys, **options): + ldap = self.api.Backend.ldap2 + editors_dn = DN( + ('cn', 'editors'), + ('cn', 'groups'), + ('cn', 'accounts'), + api.env.basedn + ) + + try: + editors_entry = ldap.get_entry(editors_dn) + except errors.NotFound: + return dict(result=False) + + attr = editors_entry.get('ipaNTSecurityIdentifier') + if not attr: + return dict(result=False) + + return dict(result=True) + +api.register(sidgen_was_run) The problem with this detection is that it uses the editors group, which might not exist, and in such case, it might return improper result (false negative for IPA server without editors group). It works well for our use case in the unit tests, since other unit tests expect that this group exists as well. However, if we're adding a new command to the API, I'd prefer something more reliable, so that it can be reused it the future. Unfortunately, I was unable to find any way how to detect this. Running ipa-sidgen-task on the server leaves no permanent trace in the LDAP, and we setup the task configuration in either case. So without setting a flag somewhere (which would be IMHO overkill for this use case) I guess we're left with the approach implemented in the patch. I'm personally OK with that, but we should include docstrings that point out that this command might provide false negatives in case the editors group does not exist. If nobody has objections, with the documentation improved this patch has an ACK from me. <pre class="moz-signature" cols="72">-- Tomas Babej Associate Software Engeneer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org</pre> </blockquote> I added a docstring explaining that the command depends on the presence of "editors" group. I also changed the behavior so the command now raises an exception if the "editors" group does not exist, instead of returning false negative. Also, please note that this command is hidden from the CLI (it can only be called using the API). Updated patch is attached. <pre class="moz-signature" cols="80">-- Regards, Ana Krivokapic Associate Software Engineer FreeIPA team Red Hat Inc.</pre> </body> </html>