[Freeipa-devel] [PATCH] 0055 Fix tests which fail after ipa-adtrust-install
Petr Viktorin
pviktori at redhat.com
Wed Aug 28 14:47:13 UTC 2013
On 08/28/2013 03:03 PM, Tomas Babej wrote:
> On 08/26/2013 09:38 AM, Ana Krivokapic wrote:
>> On 08/22/2013 06:13 PM, Tomas Babej wrote:
>>> On 08/20/2013 04:14 PM, Ana Krivokapic wrote:
>>>> On 08/09/2013 05:35 PM, Tomas Babej wrote:
>>>>> On 08/09/2013 04:03 PM, Ana Krivokapic wrote:
>>>>>> On 08/09/2013 09:39 AM, Tomas Babej wrote:
>>>>>>> On 08/08/2013 04:09 PM, Ana Krivokapic wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> This patch should fix the failing unit tests.
>>>>>>>>
>>>>>>>> https://fedorahosted.org/freeipa/ticket/3852
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Freeipa-devel mailing list
>>>>>>>> Freeipa-devel at redhat.com
>>>>>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>>>>>
>>>>>>> There are two tests failing on my machine when running the tests
>>>>>>> after ipa-adtrust-install with your patch applied:
>>>>>>
>>>>>> You say there are two tests failing but I only see one below.
>>>>>>
>>>>>
>>>>> That was just debris from trying to break your patch too much, one
>>>>> of my comments rendered invalid in the end :)
>>>>>
>>>>>>>
>>>>>>> ======================================================================
>>>>>>> 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 at 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.
>>>>>>> --
>>>>>>> Tomas Babej
>>>>>>> Associate Software Engeneer | Red Hat | Identity Management
>>>>>>> RHCE | Brno Site | IRC: tbabej | freeipa.org
>>>>>>
>>>>>> 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 at 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?
>>>>>>
>>>>> 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?
>>>>>
>>>>>>
>>>>>> --
>>>>>> Regards,
>>>>>>
>>>>>> Ana Krivokapic
>>>>>> Associate Software Engineer
>>>>>> FreeIPA team
>>>>>> Red Hat Inc.
>>>>>
>>>>>
>>>>> --
>>>>> Tomas Babej
>>>>> Associate Software Engeneer | Red Hat | Identity Management
>>>>> RHCE | Brno Site | IRC: tbabej | freeipa.org
>>>>
>>>> Updated patch should detect the situation when ipa-sidgen task was
>>>> run, and add the required attribute/objectclass accordingly.
>>>>
>>>> --
>>>> Regards,
>>>>
>>>> Ana Krivokapic
>>>> Associate Software Engineer
>>>> FreeIPA team
>>>> Red Hat Inc.
>>>
>>> +
>>> +
>>> +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.
>>> --
>>> Tomas Babej
>>> Associate Software Engeneer | Red Hat | Identity Management
>>> RHCE | Brno Site | IRC: tbabej | freeipa.org
>>
>> 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.
>>
> I think this should be sufficient.
>
> Retested, ACK!
>
Pushed to
master: 196c4b5f53c5ae9d6a471ed2da1eea4d78746fcb[[BR]]
ipa-3-3: c392146101422808b8781c85f0f2720db230da28
--
Petr³
More information about the Freeipa-devel
mailing list