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@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@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@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!
--
Tomas Babej
Associate Software Engeneer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
