On 08/19/2016 05:18 PM, Lukas Slebodnik wrote:
On (19/08/16 13:35), Petr Cech wrote:
On 08/19/2016 01:00 PM, Petr Cech wrote:
On 08/18/2016 12:39 PM, Lukas Slebodnik wrote:
On (17/08/16 14:32), Petr Cech wrote:
Hello list,

there is attached patch set for intg. testing of ldap nested netgroups.

I used last version of Lukas patch 'sssd_netgroup.py: Resolve nested
netgroups'. I don't know if it is on list.

It is still WIP. It is in state that it is possible to run it.
But there are comments in code what is needed to fix.


If I remove some netgroups (in test), it is updated on LDAP and in
cache, but
sssd_netgroup.get_sssd_netgroups() returns nothing.

Yes, I was too strict regarding to failures in resolving nested
netgroups.
glibc(getent netgroup) does not fail if there is non-existing nested
netgroup.

There is bunch of pep8 warnings. Please fix them.
sh$ pep8 src/tests/intg/test_netgroup.py | wc -l
29

Date: Wed, 17 Aug 2016 13:58:30 +0200
Subject: [PATCH 2/2] WIP: INTG: Tests for ldap nested netgroups

This patch adds tests on reproducer of t2841.

Resolves:
https://fedorahosted.org/sssd/ticket/2841
---
src/tests/intg/Makefile.am      |   1 +
src/tests/intg/test_netgroup.py | 487
++++++++++++++++++++++++++++++++++++++++
2 files changed, 488 insertions(+)
create mode 100644 src/tests/intg/test_netgroup.py

diff --git a/src/tests/intg/Makefile.am b/src/tests/intg/Makefile.am
index
b8cc5c006845f911d8518df815925455482e9f6d..b3c553539d9e74dae986fa6551041544dd687c11
100644
--- a/src/tests/intg/Makefile.am
+++ b/src/tests/intg/Makefile.am
@@ -14,6 +14,7 @@ dist_noinst_DATA = \
    util.py \
    test_memory_cache.py \
    test_ts_cache.py \
+    test_netgroup.py \
    $(NULL)

config.py: config.py.m4
diff --git a/src/tests/intg/test_netgroup.py
b/src/tests/intg/test_netgroup.py
new file mode 100644
index
0000000000000000000000000000000000000000..4be2b1c7048f3d8dc4797557d6decf1367eea36d

--- /dev/null
+++ b/src/tests/intg/test_netgroup.py
@@ -0,0 +1,487 @@
+#
+# Netgroup integration test
+#
+# Copyright (c) 2016 Red Hat, Inc.
+# Author: Petr Cech <[email protected]>
+#
+# This 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; version 2 only
+#
+# 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/>.
+#
+
//snip

[email protected]
+def reproducer_t2841(request, ldap_conn):
+    ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+
+    ent_list.add_netgroup("t2841_netgroup1", ["(host1,user1,domain1)"])
+    ent_list.add_netgroup("t2841_netgroup2", ["(host2,user2,domain2)"])
+    ent_list.add_netgroup("t2841_netgroup3", [],
+                         ["t2841_netgroup1", "t2841_netgroup2"])
+
+    create_ldap_fixture(request, ldap_conn, ent_list)
+    conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=True)
+    create_conf_fixture(request, conf)
+    create_sssd_fixture(request)
+    return (ldap_conn, ent_list)
+
+
+def test_reproducer_t2841(reproducer_t2841):
+    """
+    Adding two nested netgroup.
+    """
+
+    ldap_conn = reproducer_t2841[0]
+    ent_list = reproducer_t2841[1]
+
+    res, errno, netgroups =
sssd_netgroup.get_sssd_netgroups("t2841_netgroup1")
+    assert res == sssd_netgroup.NssReturnCode.SUCCESS
+    assert netgroups ==  [('host1', 'user1', 'domain1')]
+
+    res, errno, netgroups =
sssd_netgroup.get_sssd_netgroups("t2841_netgroup2")
+    assert res == sssd_netgroup.NssReturnCode.SUCCESS
+    assert netgroups ==  [('host2', 'user2', 'domain2')]
+
+    res, errno, netgroups =
sssd_netgroup.get_sssd_netgroups("t2841_netgroup3")
+    assert res == sssd_netgroup.NssReturnCode.SUCCESS
+    assert netgroups ==  [('host2', 'user2', 'domain2'),
+                          ('host1', 'user1', 'domain1')]
+
+    # removing of t2841_netgroup1
+    ldap_conn.delete_s(ent_list[0][0])
+    ent_list.remove(ent_list[0])
+    if subprocess.call(["sss_cache", "-N"]) != 0:
+        raise Exception("sssd_cache failed")
+
+    res, errno, netgroups =
sssd_netgroup.get_sssd_netgroups("t2841_netgroup1")
+    assert res == sssd_netgroup.NssReturnCode.NOTFOUND
+    assert netgroups ==  []
+
+    res, errno, netgroups =
sssd_netgroup.get_sssd_netgroups("t2841_netgroup2")
+    assert res == sssd_netgroup.NssReturnCode.SUCCESS
+    assert netgroups ==  [('host2', 'user2', 'domain2')]
+
+    # FIX: This should be SUCCES
+    res, errno, netgroups =
sssd_netgroup.get_sssd_netgroups("t2841_netgroup3")
+    assert res == sssd_netgroup.NssReturnCode.NOTFOUND
+    assert netgroups ==  []
Please never write test which always passes.
Test must cover the test case and not be green in 100% cases.
Otherwise we cousl have "assert True" everywhere :-)

+
+    # point A
+    # run_shell()
+
+    # removing of t2841_netgroup2
+    ldap_conn.delete_s(ent_list[0][0])
+    ent_list.remove(ent_list[0])
+    if subprocess.call(["sss_cache", "-N"]) != 0:
+        raise Exception("sssd_cache failed")
+
+    res, errno, netgroups =
sssd_netgroup.get_sssd_netgroups("t2841_netgroup1")
+    assert res == sssd_netgroup.NssReturnCode.NOTFOUND
+    assert netgroups ==  []
+
+    res, errno, netgroups =
sssd_netgroup.get_sssd_netgroups("t2841_netgroup2")
+    assert res == sssd_netgroup.NssReturnCode.NOTFOUND
+    assert netgroups ==  []
+
+    # FIX: This should be SUCCES
+    res, errno, netgroups =
sssd_netgroup.get_sssd_netgroups("t2841_netgroup3")
+    assert res == sssd_netgroup.NssReturnCode.NOTFOUND
+    assert netgroups ==  []
+
+    # point B
+    # run_shell()

I fixed the expected results(@see patch) and the test passes with latest
version of sssd_netgroup.py and git master. There are two possible
explanations
a) we need to close ticket 2841 as works for me
b) the test test_reproducer_t2841 is testing something different.

BTW I tested the same test-case outside of cwrap with simple shell
script.

sh# ldapadd -x -w Secret123 -D 'cn=admin,dc=example,dc=com' -h
localhost:10389 \
            -f netgroups.ldif
adding new entry "ou=Netgroups,dc=example,dc=com"

adding new entry "cn=t2841_netgroup1,ou=Netgroups,dc=example,dc=com"

adding new entry "cn=t2841_netgroup2,ou=Netgroups,dc=example,dc=com"

adding new entry "cn=t2841_netgroup3,ou=Netgroups,dc=example,dc=com"

sh# systemctl restart sssd

sh# systemctl restart sssd
sh# getent netgroup t2841_netgroup1 || echo FAILED
t2841_netgroup1       (host1,user1,domain1)
sh# getent netgroup t2841_netgroup2 || echo FAILED
t2841_netgroup2       (host2,user2,domain2)
sh# getent netgroup t2841_netgroup3 || echo FAILED
t2841_netgroup3       (host2,user2,domain2) (host1,user1,domain1)


sh# ldapdelete -x -w Secret123 -D 'cn=admin,dc=example,dc=com' -h
localhost:10389 cn=t2841_netgroup1,ou=Netgroups,dc=example,dc=com
sh# sss_cache -N

sh# getent netgroup t2841_netgroup1 || echo FAILED
FAILED
sh# getent netgroup t2841_netgroup2 || echo FAILED
t2841_netgroup2       (host2,user2,domain2)
sh# getent netgroup t2841_netgroup3 || echo FAILED
t2841_netgroup3       (host2,user2,domain2)

sh# ldapdelete -x -w Secret123 -D 'cn=admin,dc=example,dc=com' -h
localhost:10389 cn=t2841_netgroup2,ou=Netgroups,dc=example,dc=com
sh# sss_cache -N

sh# getent netgroup t2841_netgroup1 || echo FAILED
FAILED
sh# getent netgroup t2841_netgroup2 || echo FAILED
FAILED
sh# getent netgroup t2841_netgroup3 || echo FAILED
t2841_netgroup3

This is only the attempt to change Subject.

Important part from previous mail:

Could you explain it?

The situation is:
netgroups: A, B, C
B and C in A

I remove B and C, but the right way is
remove B, C from A,
not B, C themself.

LS

Hello,

there is whole patch set for
https://fedorahosted.org/sssd/ticket/2841
attached.

The first patch fixes the issue,
the second patch adds support for netgroups to INTG tests,
the third adds tests.

This patch set is applicable after Lukas's patch
sssd_netgroup.py: Resolve nested netgroups


--
Petr^4 Čech

From 94d790db56243419fc0629291bc2b19a773aa851 Mon Sep 17 00:00:00 2001
From: Petr Cech <[email protected]>
Date: Fri, 22 Jul 2016 14:28:54 +0200
Subject: [PATCH 1/3] LDAP: Fixing of removing netgroup from cache

There were problem with local key which wasn't properly removed.
This patch fixes it.

Resolves:
https://fedorahosted.org/sssd/ticket/2841
---
src/providers/ldap/sdap_async_netgroups.c | 40 +++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)

diff --git a/src/providers/ldap/sdap_async_netgroups.c 
b/src/providers/ldap/sdap_async_netgroups.c
index 
df233d956df70cfcb5f68bd2afc9e2a23c50c3bb..cf7d7b12361f8cc578b891961c0c5566442f1b4e
 100644
--- a/src/providers/ldap/sdap_async_netgroups.c
+++ b/src/providers/ldap/sdap_async_netgroups.c
@@ -38,6 +38,35 @@ bool is_dn(const char *str)
    return (ret == LDAP_SUCCESS ? true : false);
}

+static errno_t add_to_missing_attrs(TALLOC_CTX * mem_ctx,
+                                    struct sysdb_attrs *attrs,
+                                    const char *ext_key,
+                                    char ***_missing)
+{
+    bool is_present = false;
+    size_t size = 0;
+    size_t ret;
+
+    for (int i = 0; i < attrs->num; i++) {
+        if (strcmp(ext_key, attrs->a[i].name) == 0) {
+            is_present = true;
+        }
+        size++;
+    }
+
+    if (is_present == false) {
+        ret = add_string_to_list(attrs, ext_key, _missing);
+        if (ret != EOK) {
+            goto done;
+        }
+    }
+
+    ret = EOK;
+
+done:
+    return ret;
+}
+
static errno_t sdap_save_netgroup(TALLOC_CTX *memctx,
                                  struct sss_domain_info *dom,
                                  struct sdap_options *opts,
@@ -138,6 +167,17 @@ static errno_t sdap_save_netgroup(TALLOC_CTX *memctx,
        goto fail;
    }

+    /* Prepare SYSDB_NETGROUP_MEMBER removing
+     * if not present in netgroup_attrs
+     */
+    ret = add_to_missing_attrs(attrs, netgroup_attrs, SYSDB_NETGROUP_MEMBER,
+                               &missing);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "Failed to add [%s] to missing 
attributes\n",
+              SYSDB_NETGROUP_MEMBER);
+        goto fail;
+    }
+
Thank you for test but I would appraciate a little bit simpler solution.
"memberNisNetgroup"(SYSDB_NETGROUP_MEMBER)
Your patch will append "memberNisNetgroup"(SYSDB_NETGROUP_MEMBER) to missing
attributes if it is not in netgroup_attrs.

However "memberNisNetgroup" and "originalMemberNisNetgroup" are
tightly coupled attributes in sysdb.
* the sysdb attributes "originalMemberNisNetgroup" contain the original
  value from LDAP which can be name of netgroup of dn.
* the sysdb attributes "memberNisNetgroup" always contain shortname
  used by sssd.
They are even the same if memberNisNetgroup in LDAP does not contain dn.
One is generated from other.


And function list_missing_attrs already find that "originalMemberNisNetgroup"
is a missing attribute. Therefore if "originalMemberNisNetgroup"
is in missing attibutes list then we have to add there also
 "memberNisNetgroup".

e.g.
diff --git a/src/providers/ldap/sdap_async_netgroups.c 
b/src/providers/ldap/sdap_async_netgroups.c
index df233d9..1fe40f5 100644
--- a/src/providers/ldap/sdap_async_netgroups.c
+++ b/src/providers/ldap/sdap_async_netgroups.c
@@ -138,6 +138,14 @@ static errno_t sdap_save_netgroup(TALLOC_CTX *memctx,
         goto fail;
     }

+    if (string_in_list(SYSDB_ORIG_NETGROUP_MEMBER, missing, false)) {
+        ret = add_string_to_list(attrs, SYSDB_NETGROUP_MEMBER, &missing);
+        if (ret != EOK) {
+            DEBUG(SSSDBG_CRIT_FAILURE, "Failed to add string into list\n");
+            goto fail;
+        }
+    }
+
     ret = sysdb_add_netgroup(dom, name, NULL, netgroup_attrs, missing,
                              dom->netgroup_timeout, now);
     if (ret) goto fail;

+ some optional nice explanation/comment why :-)

Hi Lukas,

thanks. I tested your solution, it is valid. I have to agree it is simpler. I fixed my patch in this way.


    ret = sysdb_add_netgroup(dom, name, NULL, netgroup_attrs, missing,
                             dom->netgroup_timeout, now);
    if (ret) goto fail;
--
2.7.4


BTW the same bug is also ipa_save_netgroup in 
"src/providers/ipa/ipa_netgroups.c"
But sysdb_add_netgroup is called wit NULL for missing attrinbutes :-)

Anyway sdap_save_netgroup and ipa_save_netgroup do almost the same
They just use different maps. You touched the netgroup related code.
So it would be good to do small refactoring and reuse ldap
sdap_save_netgroup in ipa_save_netgroup. So it would be good
If you could fix ticket #3117 in recent future. Because you still remember
the netgroup related code in LDAP. (It's not a blocker for #2841)
It is just a recomendation :-)

Right, I will take a look.


From b284c65bb6651cb32d09b3b35599127ce995cc4f Mon Sep 17 00:00:00 2001
From: Petr Cech <[email protected]>
Date: Wed, 17 Aug 2016 14:01:09 +0200
Subject: [PATCH 2/3] INTG: Adding support for netgroups to ldap_ent

Resolves:
https://fedorahosted.org/sssd/ticket/2841
---
src/tests/intg/ldap_ent.py | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)

diff --git a/src/tests/intg/ldap_ent.py b/src/tests/intg/ldap_ent.py
index 
f8f2f7fe6977aec6fd704ad1c78a476a163a16f1..f7835a5a9c0a0f50a91c0085f02964cfafcfeebf
 100644
--- a/src/tests/intg/ldap_ent.py
+++ b/src/tests/intg/ldap_ent.py
@@ -87,6 +87,21 @@ def group_bis(base_dn, cn, gidNumber, member_uids=[], 
member_gids=[]):
    return ("cn=" + cn + ",ou=Groups," + base_dn, attr_list)


+def netgroup(base_dn, cn, triples=[], members=[]):
IIRC [] is a aangerous default value as argument
and we should use (). You might ask freeIPA/python developers
why

I asked. Reason is that () is immutable
and it is best practise.
Good to know. :-)

Addressed.

+    """
+    Generate an RFC2307bis netgroup add-modlist for passing to ldap.add*.
+    """
+    attr_list = [
+        ('objectClass', ['top', 'nisNetgroup'])
+    ]
+    member_list = []
      this variable is unused
+    if len(triples) > 0:
+        attr_list.append(('nisNetgroupTriple', triples))
+    if len(members) > 0:
+        attr_list.append(('memberNisNetgroup', members))
+    return ("cn=" + cn + ",ou=Netgroups," + base_dn, attr_list)
+
+
class List(list):
    """LDAP add-modlist list"""

@@ -124,3 +139,8 @@ class List(list):
        self.append(group_bis(base_dn or self.base_dn,
                              cn, gidNumber,
                              member_uids, member_gids))
+
+    def add_netgroup(self, cn, triples=[], members=[], base_dn=None):
                                         ^^
                           the same case as in previous function

Addressed.

+        """Add an RFC2307bis netgroup add-modlist."""
+        self.append(netgroup(base_dn or self.base_dn,
+                             cn, triples, members))
--
2.7.4


From 483b2bf279f47772fddaf4aa7087f6336ddf78b9 Mon Sep 17 00:00:00 2001
From: Petr Cech <[email protected]>
Date: Wed, 17 Aug 2016 13:58:30 +0200
Subject: [PATCH 3/3] INTG: Tests for ldap nested netgroups

This patch adds tests on reproducer of t2841.

Resolves:
https://fedorahosted.org/sssd/ticket/2841
---
src/tests/intg/Makefile.am      |   1 +
src/tests/intg/test_netgroup.py | 490 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 491 insertions(+)
create mode 100644 src/tests/intg/test_netgroup.py

diff --git a/src/tests/intg/Makefile.am b/src/tests/intg/Makefile.am
index 
d73e4216310ccd1c90e6b7eb0a0e60068fc45bd5..75422a4417046116bec11a8a680fe2248e3afb69
 100644
--- a/src/tests/intg/Makefile.am
+++ b/src/tests/intg/Makefile.am
@@ -15,6 +15,7 @@ dist_noinst_DATA = \
    test_ldap.py \
    test_memory_cache.py \
    test_ts_cache.py \
+    test_netgroup.py \
    $(NULL)

config.py: config.py.m4
diff --git a/src/tests/intg/test_netgroup.py b/src/tests/intg/test_netgroup.py
new file mode 100644
index 
0000000000000000000000000000000000000000..d004faf2c2f3ee01c1fb41e36820709427c5cbd9
--- /dev/null
+++ b/src/tests/intg/test_netgroup.py
@@ -0,0 +1,490 @@
+#
+# Netgroup integration test
+#
+# Copyright (c) 2016 Red Hat, Inc.
+# Author: Petr Cech <[email protected]>
+#
+# This 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; version 2 only
+#
+# 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/>.
+#
+
+import os
+import stat
+import sys
+import pwd
+import grp
+import ent
Previous 4 imports are unused

Thanks. Addressed.

Lukas, do you have any method how to
find such situations?
I would like improve my developing process.

+import config
+import signal
+import subprocess
+import time
+import ldap
+import ldap.modlist
+import pytest
+import ds_openldap
+import ldap_ent
+import sssd_netgroup as sssd_ntg
+from util import *
It would be good to aoin wildcard import and import just
required names (in our case just unindent)
It might be a leftover after debugginf with run_shell

It would be also good to separate system modules with internam modules
empty line might be enough
e.g.
import os
import stat
import signal
import subprocess
import time
import ldap
import ldap.modlist
import pytest

import config
import ds_openldap
import ldap_ent
...

+
+
+LDAP_BASE_DN = "dc=example,dc=com"
+INTERACTIVE_TIMEOUT = 4
Unused

+
+
[email protected](scope="module")
+def ds_inst(request):
+    """LDAP server instance fixture"""
+    ds_inst = ds_openldap.DSOpenLDAP(
+        config.PREFIX, 10389, LDAP_BASE_DN,
+        "cn=admin", "Secret123"
+    )
+
+    try:
+        ds_inst.setup()
+    except:
+        ds_inst.teardown()
+        raise
+    request.addfinalizer(lambda: ds_inst.teardown())
+    return ds_inst
+
+
[email protected](scope="module")
+def ldap_conn(request, ds_inst):
+    """LDAP server connection fixture"""
+    ldap_conn = ds_inst.bind()
+    ldap_conn.ds_inst = ds_inst
+    request.addfinalizer(lambda: ldap_conn.unbind_s())
+    return ldap_conn
+
+
+def create_ldap_entries(ldap_conn, ent_list=None):
+    """Add LDAP entries from ent_list"""
+    if ent_list is not None:
+        for entry in ent_list:
+            ldap_conn.add_s(entry[0], entry[1])
+
+
+def cleanup_ldap_entries(ldap_conn, ent_list=None):
+    """Remove LDAP entries added by create_ldap_entries"""
+    if ent_list is None:
+        for ou in ("Users", "Groups", "Netgroups", "Services", "Policies"):
+            for entry in ldap_conn.search_s("ou=" + ou + "," +
+                                            ldap_conn.ds_inst.base_dn,
+                                            ldap.SCOPE_ONELEVEL,
+                                            attrlist=[]):
+                ldap_conn.delete_s(entry[0])
+    else:
+        for entry in ent_list:
+            ldap_conn.delete_s(entry[0])
+
+
+def create_ldap_cleanup(request, ldap_conn, ent_list=None):
+    """Add teardown for removing all user/group LDAP entries"""
+    request.addfinalizer(lambda: cleanup_ldap_entries(ldap_conn, ent_list))
+
+
+def create_ldap_fixture(request, ldap_conn, ent_list=None):
+    """Add LDAP entries and add teardown for removing them"""
+    create_ldap_entries(ldap_conn, ent_list)
+    create_ldap_cleanup(request, ldap_conn, ent_list)
+
+
+SCHEMA_RFC2307_BIS = "rfc2307bis"
+
+
+def format_basic_conf(ldap_conn, schema, enum):
+    """Format a basic SSSD configuration"""
+    schema_conf = "ldap_schema         = " + schema + "\n"
+    schema_conf += "ldap_group_object_class = groupOfNames\n"
+    return unindent("""\
+        [sssd]
+        debug_level         = 0xffff
+        domains             = LDAP
+        services            = nss, pam
pam responder is not used; it needn't run

Addressed.

+
+        [nss]
+        debug_level         = 0xffff
+        memcache_timeout    = 0
netgroup is not cached in memory cache so you can remove
the line

Addressed.

+
+        [pam]
+        debug_level         = 0xffff
+
+        [domain/LDAP]
+        ldap_auth_disable_tls_never_use_in_production = true
PLease remove this line because you are not testing authentication

Addressed.

+        debug_level         = 0xffff
+        enumerate           = {enum}
It would be good to do not test with enumeration
and remove argument enum from the function format_basic_conf
If you want to test netgroups with enumeration then please
add separate tests into test_enumeration

Addressed.

+        {schema_conf}
+        id_provider         = ldap
+        auth_provider       = ldap
+        ldap_uri            = {ldap_conn.ds_inst.ldap_url}
+        ldap_search_base    = {ldap_conn.ds_inst.base_dn}
+        ldap_netgroup_search_base = ou=Netgroups,{ldap_conn.ds_inst.base_dn}
Do we need enbale debug_level?
IMHO it would be needed just for froubleshooting

No, we needn't. Addressed.

+    """).format(**locals())
+
+
+def create_conf_file(contents):
+    """Create sssd.conf with specified contents"""
+    conf = open(config.CONF_PATH, "w")
+    conf.write(contents)
+    conf.close()
+    os.chmod(config.CONF_PATH, stat.S_IRUSR | stat.S_IWUSR)
+
+
+def cleanup_conf_file():
+    """Remove sssd.conf, if it exists"""
+    if os.path.lexists(config.CONF_PATH):
+        os.unlink(config.CONF_PATH)
+
+
+def create_conf_cleanup(request):
+    """Add teardown for removing sssd.conf"""
+    request.addfinalizer(cleanup_conf_file)
+
+
+def create_conf_fixture(request, contents):
+    """
+    Create sssd.conf with specified contents and add teardown for removing it
+    """
+    create_conf_file(contents)
+    create_conf_cleanup(request)
+
+
+def create_sssd_process():
+    """Start the SSSD process"""
+    if subprocess.call(["sssd", "-D", "-f"]) != 0:
+        raise Exception("sssd start failed")
+
+
+def cleanup_sssd_process():
+    """Stop the SSSD process and remove its state"""
+    try:
+        pid_file = open(config.PIDFILE_PATH, "r")
+        pid = int(pid_file.read())
+        os.kill(pid, signal.SIGTERM)
+        while True:
+            try:
+                os.kill(pid, signal.SIGCONT)
+            except:
+                break
+            time.sleep(1)
+    except:
+        pass
+    for path in os.listdir(config.DB_PATH):
+        os.unlink(config.DB_PATH + "/" + path)
+    for path in os.listdir(config.MCACHE_PATH):
+        os.unlink(config.MCACHE_PATH + "/" + path)
+
+
+def create_sssd_cleanup(request):
+    """Add teardown for stopping SSSD and removing its state"""
+    request.addfinalizer(cleanup_sssd_process)
+
+
+def create_sssd_fixture(request):
+    """Start SSSD and add teardown for stopping it and removing its state"""
+    create_sssd_process()
+    create_sssd_cleanup(request)
+
+
[email protected]
+def add_nested_netgroup(request, ldap_conn):
+    ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+
+    ent_list.add_netgroup("nested_netgroup")
                             Is it an appropriate name?
                             I would say it is a empty netgroup
                             The same applies to the name of
                             fixture and name of following test.

Addressed.

+
+    create_ldap_fixture(request, ldap_conn, ent_list)
+    conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=False)
+    create_conf_fixture(request, conf)
+    create_sssd_fixture(request)
+    return None
+
+
+def test_add_nested_netgroup(add_nested_netgroup):
+    """
+    Adding nested netgroup.
+    """
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("nested_netgroup")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == []
+
+
[email protected]
+def add_two_nested_netgroup(request, ldap_conn):
+    ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+
+    ent_list.add_netgroup("nested_netgroup1")
+    ent_list.add_netgroup("nested_netgroup2")
      The same here name of netgroups are confusing
      Or did you forgot to use nested netgroups here?

I really meant empty groups. Addressed.

+
+    create_ldap_fixture(request, ldap_conn, ent_list)
+    conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=False)
+    create_conf_fixture(request, conf)
+    create_sssd_fixture(request)
+    return None
+
+
+def test_add_two_nested_netgroup(add_two_nested_netgroup):
+    """
+    Adding two nested netgroup.
+    """
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("nested_netgroup1")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == []
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("nested_netgroup2")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == []
+
+
+# TODO: Test for removing one by one
Is there a reason for todo? or is this line just a leftover?

It is not conneted to the ticket. But remove tests are missing.
I added test_remove_step_by_step().

+
[email protected]
+def add_tripled_netgroup(request, ldap_conn):
+    ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+
+    ent_list.add_netgroup("tripled_netgroup", ["(host,user,domain)"])
+
+    create_ldap_fixture(request, ldap_conn, ent_list)
+    conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=False)
+    create_conf_fixture(request, conf)
+    create_sssd_fixture(request)
+    return None
+
+
+def test_add_tripled_netgroup(add_tripled_netgroup):
+    """
+    Adding netgroup with trinity.
                           ^^^^^^^^
                           I think that netgroups terminology
                           would use triplet.

Triplet sounds better in this context. Addressed.

+    """
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("tripled_netgroup")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [("host", "user", "domain")]
+
+
[email protected]
+def add_advanced_tripled_netgroup(request, ldap_conn):
+    ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+
+    ent_list.add_netgroup("adv_tripled_netgroup", ["(host1,user1,domain1)",
+                                                   "(host2,user2,domain2)"])
+
      I would recomment to merge this fixture ( + related test)
      with previous one. It woul a little bit reduce cuplicated code.

Addressed.

+    create_ldap_fixture(request, ldap_conn, ent_list)
+    conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=False)
+    create_conf_fixture(request, conf)
+    create_sssd_fixture(request)
+    return None
+
+
+def test_add_advanced_tripled_netgroup(add_advanced_tripled_netgroup):
+    """
+    Adding netgroup with two trinities.
+    """
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("adv_tripled_netgroup")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert sorted(netgroups) == sorted([("host1", "user1", "domain1"),
+                                        ("host2", "user2", "domain2")])
+
+
[email protected]
+def add_two_tripled_netgroup(request, ldap_conn):
+    ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+
+    ent_list.add_netgroup("tripled_netgroup1", ["(host1,user1,domain1)"])
+    ent_list.add_netgroup("tripled_netgroup2", ["(host2,user2,domain2)"])
+
      the same as in previous case. The name of netgroups are not conflicting
      therefore it could be tested in common test (test_simple_netgroups)
      becasue you are testing just simple netgroups just with triplets
      and not nested netgroups.

I think this test is redudant after merging previous two.


+    create_ldap_fixture(request, ldap_conn, ent_list)
+    conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=False)
+    create_conf_fixture(request, conf)
+    create_sssd_fixture(request)
+    return None
+
+
+def test_add_two_tripled_netgroup(add_two_tripled_netgroup):
+    """
+    Adding two netgroups, each with trinity.
+    """
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("tripled_netgroup1")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [("host1", "user1", "domain1")]
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("tripled_netgroup2")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [("host2", "user2", "domain2")]
+
+
+# TODO: Test for removing one by one
Is there a reason for todo? or is it an leftover?

The same as above.

+
[email protected]
+def add_mixed_netgroup(request, ldap_conn):
+    ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+
+    ent_list.add_netgroup("mixed_netgroup1", [], [])
+    ent_list.add_netgroup("mixed_netgroup2", [], ["mixed_netgroup1"])
+
+    ent_list.add_netgroup("mixed_netgroup3", ["(host1,user1,domain1)"], [])
+    ent_list.add_netgroup("mixed_netgroup4",
+                          ["(host2,user2,domain2)", "(host3,user3,domain3)"],
+                          [])
+
+    ent_list.add_netgroup("mixed_netgroup5",
+                          ["(host4,user4,domain4)"],
+                          ["mixed_netgroup1"])
+    ent_list.add_netgroup("mixed_netgroup6",
+                          ["(host5,user5,domain5)"],
+                          ["mixed_netgroup2"])
+
+    ent_list.add_netgroup("mixed_netgroup7", [], ["mixed_netgroup3"])
+    ent_list.add_netgroup("mixed_netgroup8", [],
+                          ["mixed_netgroup3", "mixed_netgroup4"])
+
+    ent_list.add_netgroup("mixed_netgroup9",
+                          ["(host6,user6,domain6)"],
+                          ["mixed_netgroup3", "mixed_netgroup4"])
+
+    create_ldap_fixture(request, ldap_conn, ent_list)
+    conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=False)
+    create_conf_fixture(request, conf)
+    create_sssd_fixture(request)
+    return None
+
+
+def test_add_mixed_netgroup(add_mixed_netgroup):
+    """
+    Adding many netgroups of different type.
+    """
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup1")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == []
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup2")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == []
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup3")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [("host1", "user1", "domain1")]
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup4")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert sorted(netgroups) == sorted([("host2", "user2", "domain2"),
+                                        ("host3", "user3", "domain3")])
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup5")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [("host4", "user4", "domain4")]
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup6")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [("host5", "user5", "domain5")]
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup7")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [("host1", "user1", "domain1")]
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup8")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert sorted(netgroups) == sorted([("host1", "user1", "domain1"),
+                                        ("host2", "user2", "domain2"),
+                                        ("host3", "user3", "domain3")])
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup9")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert sorted(netgroups) == sorted([("host1", "user1", "domain1"),
+                                        ("host2", "user2", "domain2"),
+                                        ("host3", "user3", "domain3"),
+                                        ("host6", "user6", "domain6")])
+
+
[email protected]
+def reproducer_t2841(request, ldap_conn):
+    ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+
+    ent_list.add_netgroup("t2841_netgroup1", ["(host1,user1,domain1)"])
+    ent_list.add_netgroup("t2841_netgroup2", ["(host2,user2,domain2)"])
+    ent_list.add_netgroup("t2841_netgroup3", [],
+                          ["t2841_netgroup1", "t2841_netgroup2"])
+
+    create_ldap_fixture(request, ldap_conn, ent_list)
+    conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=False)
+    create_conf_fixture(request, conf)
+    create_sssd_fixture(request)
+    return None
+
+
+def test_reproducer_t2841(reproducer_t2841, ldap_conn):
It would be good to use more desciptive name.
So it is immediately visible in pytest output what you want to test.
The pytest output is very reduced for green test.
and reproducer_t2841 is not very descriptive.

What about test_removing_nested_netgroups?

Righ, it sounds better. Addressed.


+    """
+    Regression test for ticket 2841.
+    https://fedorahosted.org/sssd/ticket/2841
+    """
+
+    t2841_netgroup3_dn = 'cn=t2841_netgroup3,ou=Netgroups,dc=example,dc=com'
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup1")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [('host1', 'user1', 'domain1')]
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup2")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [('host2', 'user2', 'domain2')]
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup3")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert sorted(netgroups) == sorted([('host1', 'user1', 'domain1'),
+                                        ('host2', 'user2', 'domain2')])
+
+    # removing of t2841_netgroup1 from t2841_netgroup3
+    old = {'memberNisNetgroup': ["t2841_netgroup1", "t2841_netgroup2"]}
+    new = {'memberNisNetgroup': ["t2841_netgroup2"]}
+
+    ldif = ldap.modlist.modifyModlist(old, new)
+    ldap_conn.modify_s(t2841_netgroup3_dn, ldif)
+
+    if subprocess.call(["sss_cache", "-N"]) != 0:
+        raise Exception("sssd_cache failed")
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup1")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [('host1', 'user1', 'domain1')]
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup2")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [('host2', 'user2', 'domain2')]
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup3")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [('host2', 'user2', 'domain2')]
+
+    # removing of t2841_netgroup2 from t2841_netgroup3
+    old = {'memberNisNetgroup': ["t2841_netgroup2"]}
+    new = {'memberNisNetgroup': []}
+
+    ldif = ldap.modlist.modifyModlist(old, new)
+    ldap_conn.modify_s(t2841_netgroup3_dn, ldif)
+
+    if subprocess.call(["sss_cache", "-N"]) != 0:
+        raise Exception("sssd_cache failed")
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup1")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [('host1', 'user1', 'domain1')]
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup2")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [('host2', 'user2', 'domain2')]
+
+    res, errno, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup3")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == []

BTW in all cases you are tesing positive results
So the errno variabel is never used.
It make sense to check errno just for failures.
So you can replace errno with _ which means taht you are
intentionaly ingoring the value.

Addressed.

Than you very much for test. I really appreciate it.
Now wi will be sure that there woudl not be a regression for
the bug and we can refactor code without any worries :-)
I had a many comments but they were mostly nitpicking.

One more time thank you very much and good job.

LS

Thanks Lukas for code review.
New patch set is attached.

Regards

--
Petr^4 Čech
>From 25be35537e0d91af6939c3400340fe01cfb32ea7 Mon Sep 17 00:00:00 2001
From: Petr Cech <[email protected]>
Date: Fri, 22 Jul 2016 14:28:54 +0200
Subject: [PATCH 1/3] LDAP: Fixing of removing netgroup from cache

There were problem with local key which wasn't properly removed.
This patch fixes it.

Resolves:
https://fedorahosted.org/sssd/ticket/2841
---
 src/providers/ldap/sdap_async_netgroups.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/providers/ldap/sdap_async_netgroups.c b/src/providers/ldap/sdap_async_netgroups.c
index df233d956df70cfcb5f68bd2afc9e2a23c50c3bb..f36ea030414638ae2c778bc532fa09a0b69ed7de 100644
--- a/src/providers/ldap/sdap_async_netgroups.c
+++ b/src/providers/ldap/sdap_async_netgroups.c
@@ -138,6 +138,18 @@ static errno_t sdap_save_netgroup(TALLOC_CTX *memctx,
         goto fail;
     }
 
+    /* We can get SYSDB_ORIG_NETGROUP_MEMBER, but not SYSDB_NETGROUP_MEMBER
+     * from LDAP. Thus we add SYSDB_NETGROUP_MEMBER to missing
+     * if SYSDB_ORIG_NETGROUP_MEMBER is in.
+     */
+    if (string_in_list(SYSDB_ORIG_NETGROUP_MEMBER, missing, false)) {
+        ret = add_string_to_list(attrs, SYSDB_NETGROUP_MEMBER, &missing);
+        if (ret != EOK) {
+            DEBUG(SSSDBG_CRIT_FAILURE, "Failed to add string into list\n");
+            goto fail;
+        }
+    }
+
     ret = sysdb_add_netgroup(dom, name, NULL, netgroup_attrs, missing,
                              dom->netgroup_timeout, now);
     if (ret) goto fail;
-- 
2.7.4

>From 4ff31b600f01c810367756165500cfdb95f2be2b Mon Sep 17 00:00:00 2001
From: Petr Cech <[email protected]>
Date: Wed, 17 Aug 2016 14:01:09 +0200
Subject: [PATCH 2/3] INTG: Adding support for netgroups to ldap_ent

Resolves:
https://fedorahosted.org/sssd/ticket/2841
---
 src/tests/intg/ldap_ent.py | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/src/tests/intg/ldap_ent.py b/src/tests/intg/ldap_ent.py
index f8f2f7fe6977aec6fd704ad1c78a476a163a16f1..2bb93f8bf605876acfb273088944dcb121e67241 100644
--- a/src/tests/intg/ldap_ent.py
+++ b/src/tests/intg/ldap_ent.py
@@ -87,6 +87,21 @@ def group_bis(base_dn, cn, gidNumber, member_uids=[], member_gids=[]):
     return ("cn=" + cn + ",ou=Groups," + base_dn, attr_list)
 
 
+def netgroup(base_dn, cn, triples=(), members=()):
+    """
+    Generate an RFC2307bis netgroup add-modlist for passing to ldap.add*.
+    """
+    attr_list = [
+        ('objectClass', ['top', 'nisNetgroup'])
+    ]
+    member_list = []
+    if len(triples) > 0:
+        attr_list.append(('nisNetgroupTriple', triples))
+    if len(members) > 0:
+        attr_list.append(('memberNisNetgroup', members))
+    return ("cn=" + cn + ",ou=Netgroups," + base_dn, attr_list)
+
+
 class List(list):
     """LDAP add-modlist list"""
 
@@ -124,3 +139,8 @@ class List(list):
         self.append(group_bis(base_dn or self.base_dn,
                               cn, gidNumber,
                               member_uids, member_gids))
+
+    def add_netgroup(self, cn, triples=(), members=(), base_dn=None):
+        """Add an RFC2307bis netgroup add-modlist."""
+        self.append(netgroup(base_dn or self.base_dn,
+                             cn, triples, members))
-- 
2.7.4

>From ccb7aa90a1a63db0b13b60b92afeffc2325af50c Mon Sep 17 00:00:00 2001
From: Petr Cech <[email protected]>
Date: Wed, 17 Aug 2016 13:58:30 +0200
Subject: [PATCH 3/3] INTG: Tests for ldap nested netgroups

This patch adds tests on reproducer of t2841.

Resolves:
https://fedorahosted.org/sssd/ticket/2841
---
 src/tests/intg/Makefile.am      |   1 +
 src/tests/intg/test_netgroup.py | 488 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 489 insertions(+)
 create mode 100644 src/tests/intg/test_netgroup.py

diff --git a/src/tests/intg/Makefile.am b/src/tests/intg/Makefile.am
index d73e4216310ccd1c90e6b7eb0a0e60068fc45bd5..75422a4417046116bec11a8a680fe2248e3afb69 100644
--- a/src/tests/intg/Makefile.am
+++ b/src/tests/intg/Makefile.am
@@ -15,6 +15,7 @@ dist_noinst_DATA = \
     test_ldap.py \
     test_memory_cache.py \
     test_ts_cache.py \
+    test_netgroup.py \
     $(NULL)
 
 config.py: config.py.m4
diff --git a/src/tests/intg/test_netgroup.py b/src/tests/intg/test_netgroup.py
new file mode 100644
index 0000000000000000000000000000000000000000..3c5dc6f3634ece0669f1748ccbd49b514eeb874f
--- /dev/null
+++ b/src/tests/intg/test_netgroup.py
@@ -0,0 +1,488 @@
+#
+# Netgroup integration test
+#
+# Copyright (c) 2016 Red Hat, Inc.
+# Author: Petr Cech <[email protected]>
+#
+# This 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; version 2 only
+#
+# 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/>.
+#
+
+import os
+import stat
+import signal
+import subprocess
+import time
+import ldap
+import ldap.modlist
+import pytest
+
+import config
+import ds_openldap
+import ldap_ent
+from util import unindent
+import sssd_netgroup as sssd_ntg
+
+LDAP_BASE_DN = "dc=example,dc=com"
+
+
[email protected](scope="module")
+def ds_inst(request):
+    """LDAP server instance fixture"""
+    ds_inst = ds_openldap.DSOpenLDAP(
+        config.PREFIX, 10389, LDAP_BASE_DN,
+        "cn=admin", "Secret123"
+    )
+
+    try:
+        ds_inst.setup()
+    except:
+        ds_inst.teardown()
+        raise
+    request.addfinalizer(lambda: ds_inst.teardown())
+    return ds_inst
+
+
[email protected](scope="module")
+def ldap_conn(request, ds_inst):
+    """LDAP server connection fixture"""
+    ldap_conn = ds_inst.bind()
+    ldap_conn.ds_inst = ds_inst
+    request.addfinalizer(lambda: ldap_conn.unbind_s())
+    return ldap_conn
+
+
+def create_ldap_entries(ldap_conn, ent_list=None):
+    """Add LDAP entries from ent_list"""
+    if ent_list is not None:
+        for entry in ent_list:
+            ldap_conn.add_s(entry[0], entry[1])
+
+
+def cleanup_ldap_entries(ldap_conn, ent_list=None):
+    """Remove LDAP entries added by create_ldap_entries"""
+    if ent_list is None:
+        for ou in ("Users", "Groups", "Netgroups", "Services", "Policies"):
+            for entry in ldap_conn.search_s("ou=" + ou + "," +
+                                            ldap_conn.ds_inst.base_dn,
+                                            ldap.SCOPE_ONELEVEL,
+                                            attrlist=[]):
+                ldap_conn.delete_s(entry[0])
+    else:
+        for entry in ent_list:
+            ldap_conn.delete_s(entry[0])
+
+
+def create_ldap_cleanup(request, ldap_conn, ent_list=None):
+    """Add teardown for removing all user/group LDAP entries"""
+    request.addfinalizer(lambda: cleanup_ldap_entries(ldap_conn, ent_list))
+
+
+def create_ldap_fixture(request, ldap_conn, ent_list=None):
+    """Add LDAP entries and add teardown for removing them"""
+    create_ldap_entries(ldap_conn, ent_list)
+    create_ldap_cleanup(request, ldap_conn, ent_list)
+
+
+SCHEMA_RFC2307_BIS = "rfc2307bis"
+
+
+def format_basic_conf(ldap_conn, schema):
+    """Format a basic SSSD configuration"""
+    schema_conf = "ldap_schema         = " + schema + "\n"
+    schema_conf += "ldap_group_object_class = groupOfNames\n"
+    return unindent("""\
+        [sssd]
+        domains             = LDAP
+        services            = nss
+
+        [domain/LDAP]
+        {schema_conf}
+        id_provider         = ldap
+        auth_provider       = ldap
+        ldap_uri            = {ldap_conn.ds_inst.ldap_url}
+        ldap_search_base    = {ldap_conn.ds_inst.base_dn}
+        ldap_netgroup_search_base = ou=Netgroups,{ldap_conn.ds_inst.base_dn}
+    """).format(**locals())
+
+
+def create_conf_file(contents):
+    """Create sssd.conf with specified contents"""
+    conf = open(config.CONF_PATH, "w")
+    conf.write(contents)
+    conf.close()
+    os.chmod(config.CONF_PATH, stat.S_IRUSR | stat.S_IWUSR)
+
+
+def cleanup_conf_file():
+    """Remove sssd.conf, if it exists"""
+    if os.path.lexists(config.CONF_PATH):
+        os.unlink(config.CONF_PATH)
+
+
+def create_conf_cleanup(request):
+    """Add teardown for removing sssd.conf"""
+    request.addfinalizer(cleanup_conf_file)
+
+
+def create_conf_fixture(request, contents):
+    """
+    Create sssd.conf with specified contents and add teardown for removing it
+    """
+    create_conf_file(contents)
+    create_conf_cleanup(request)
+
+
+def create_sssd_process():
+    """Start the SSSD process"""
+    if subprocess.call(["sssd", "-D", "-f"]) != 0:
+        raise Exception("sssd start failed")
+
+
+def cleanup_sssd_process():
+    """Stop the SSSD process and remove its state"""
+    try:
+        pid_file = open(config.PIDFILE_PATH, "r")
+        pid = int(pid_file.read())
+        os.kill(pid, signal.SIGTERM)
+        while True:
+            try:
+                os.kill(pid, signal.SIGCONT)
+            except:
+                break
+            time.sleep(1)
+    except:
+        pass
+    for path in os.listdir(config.DB_PATH):
+        os.unlink(config.DB_PATH + "/" + path)
+    for path in os.listdir(config.MCACHE_PATH):
+        os.unlink(config.MCACHE_PATH + "/" + path)
+
+
+def create_sssd_cleanup(request):
+    """Add teardown for stopping SSSD and removing its state"""
+    request.addfinalizer(cleanup_sssd_process)
+
+
+def create_sssd_fixture(request):
+    """Start SSSD and add teardown for stopping it and removing its state"""
+    create_sssd_process()
+    create_sssd_cleanup(request)
+
+
[email protected]
+def add_empty_netgroup(request, ldap_conn):
+    ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+
+    ent_list.add_netgroup("empty_netgroup")
+
+    create_ldap_fixture(request, ldap_conn, ent_list)
+    conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS)
+    create_conf_fixture(request, conf)
+    create_sssd_fixture(request)
+    return None
+
+
+def test_add_empty_netgroup(add_empty_netgroup):
+    """
+    Adding empty netgroup.
+    """
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("empty_netgroup")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == []
+
+
[email protected]
+def add_two_empty_netgroup(request, ldap_conn):
+    ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+
+    ent_list.add_netgroup("empty_netgroup1")
+    ent_list.add_netgroup("empty_netgroup2")
+
+    create_ldap_fixture(request, ldap_conn, ent_list)
+    conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS)
+    create_conf_fixture(request, conf)
+    create_sssd_fixture(request)
+    return None
+
+
+def test_add_two_empty_netgroup(add_two_empty_netgroup):
+    """
+    Adding two empty netgroups.
+    """
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("empty_netgroup1")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == []
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("empty_netgroup2")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == []
+
+
[email protected]
+def add_tripled_netgroup(request, ldap_conn):
+    ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+
+    ent_list.add_netgroup("tripled_netgroup", ["(host,user,domain)"])
+
+    ent_list.add_netgroup("adv_tripled_netgroup", ["(host1,user1,domain1)",
+                                                   "(host2,user2,domain2)"])
+
+    create_ldap_fixture(request, ldap_conn, ent_list)
+    conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS)
+    create_conf_fixture(request, conf)
+    create_sssd_fixture(request)
+    return None
+
+
+def test_add_tripled_netgroup(add_tripled_netgroup):
+    """
+    Adding netgroup with triplet.
+    """
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("tripled_netgroup")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [("host", "user", "domain")]
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("adv_tripled_netgroup")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert sorted(netgroups) == sorted([("host1", "user1", "domain1"),
+                                        ("host2", "user2", "domain2")])
+
+
[email protected]
+def add_mixed_netgroup(request, ldap_conn):
+    ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+
+    ent_list.add_netgroup("mixed_netgroup1", [], [])
+    ent_list.add_netgroup("mixed_netgroup2", [], ["mixed_netgroup1"])
+
+    ent_list.add_netgroup("mixed_netgroup3", ["(host1,user1,domain1)"], [])
+    ent_list.add_netgroup("mixed_netgroup4",
+                          ["(host2,user2,domain2)", "(host3,user3,domain3)"],
+                          [])
+
+    ent_list.add_netgroup("mixed_netgroup5",
+                          ["(host4,user4,domain4)"],
+                          ["mixed_netgroup1"])
+    ent_list.add_netgroup("mixed_netgroup6",
+                          ["(host5,user5,domain5)"],
+                          ["mixed_netgroup2"])
+
+    ent_list.add_netgroup("mixed_netgroup7", [], ["mixed_netgroup3"])
+    ent_list.add_netgroup("mixed_netgroup8", [],
+                          ["mixed_netgroup3", "mixed_netgroup4"])
+
+    ent_list.add_netgroup("mixed_netgroup9",
+                          ["(host6,user6,domain6)"],
+                          ["mixed_netgroup3", "mixed_netgroup4"])
+
+    create_ldap_fixture(request, ldap_conn, ent_list)
+    conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS)
+    create_conf_fixture(request, conf)
+    create_sssd_fixture(request)
+    return None
+
+
+def test_add_mixed_netgroup(add_mixed_netgroup):
+    """
+    Adding many netgroups of different type.
+    """
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup1")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == []
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup2")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == []
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup3")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [("host1", "user1", "domain1")]
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup4")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert sorted(netgroups) == sorted([("host2", "user2", "domain2"),
+                                        ("host3", "user3", "domain3")])
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup5")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [("host4", "user4", "domain4")]
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup6")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [("host5", "user5", "domain5")]
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup7")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [("host1", "user1", "domain1")]
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup8")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert sorted(netgroups) == sorted([("host1", "user1", "domain1"),
+                                        ("host2", "user2", "domain2"),
+                                        ("host3", "user3", "domain3")])
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("mixed_netgroup9")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert sorted(netgroups) == sorted([("host1", "user1", "domain1"),
+                                        ("host2", "user2", "domain2"),
+                                        ("host3", "user3", "domain3"),
+                                        ("host6", "user6", "domain6")])
+
+
[email protected]
+def remove_step_by_step(request, ldap_conn):
+    ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+
+    ent_list.add_netgroup("rm_empty_netgroup1", ["(host1,user1,domain1)"])
+    ent_list.add_netgroup("rm_empty_netgroup2",
+                          ["(host2,user2,domain2)"],
+                          ["rm_empty_netgroup1"])
+
+    create_ldap_fixture(request, ldap_conn, ent_list)
+    conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS)
+    create_conf_fixture(request, conf)
+    create_sssd_fixture(request)
+    return ent_list
+
+
+def test_remove_step_by_step(remove_step_by_step, ldap_conn):
+    """
+    Removing netgroups step by step.
+    """
+
+    ent_list = remove_step_by_step
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("rm_empty_netgroup1")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [('host1', 'user1', 'domain1')]
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("rm_empty_netgroup2")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert sorted(netgroups) == sorted([('host1', 'user1', 'domain1'),
+                                        ('host2', 'user2', 'domain2')])
+
+    # removing of rm_empty_netgroup1
+    ldap_conn.delete_s(ent_list[0][0])
+    ent_list.remove(ent_list[0])
+
+    if subprocess.call(["sss_cache", "-N"]) != 0:
+        raise Exception("sssd_cache failed")
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("rm_empty_netgroup1")
+    assert res == sssd_ntg.NssReturnCode.NOTFOUND
+    assert netgroups == []
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("rm_empty_netgroup2")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [('host2', 'user2', 'domain2')]
+
+    # removing of rm_empty_netgroup2
+    ldap_conn.delete_s(ent_list[0][0])
+    ent_list.remove(ent_list[0])
+
+    if subprocess.call(["sss_cache", "-N"]) != 0:
+        raise Exception("sssd_cache failed")
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("rm_empty_netgroup1")
+    assert res == sssd_ntg.NssReturnCode.NOTFOUND
+    assert netgroups == []
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("rm_empty_netgroup2")
+    assert res == sssd_ntg.NssReturnCode.NOTFOUND
+    assert netgroups == []
+
+
[email protected]
+def removing_nested_netgroups(request, ldap_conn):
+    ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+
+    ent_list.add_netgroup("t2841_netgroup1", ["(host1,user1,domain1)"])
+    ent_list.add_netgroup("t2841_netgroup2", ["(host2,user2,domain2)"])
+    ent_list.add_netgroup("t2841_netgroup3", [],
+                          ["t2841_netgroup1", "t2841_netgroup2"])
+
+    create_ldap_fixture(request, ldap_conn, ent_list)
+    conf = format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS)
+    create_conf_fixture(request, conf)
+    create_sssd_fixture(request)
+    return None
+
+
+def test_removing_nested_netgroups(removing_nested_netgroups, ldap_conn):
+    """
+    Regression test for ticket 2841.
+    https://fedorahosted.org/sssd/ticket/2841
+    """
+
+    netgroup_dn = 'cn=t2841_netgroup3,ou=Netgroups,dc=example,dc=com'
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup1")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [('host1', 'user1', 'domain1')]
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup2")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [('host2', 'user2', 'domain2')]
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup3")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert sorted(netgroups) == sorted([('host1', 'user1', 'domain1'),
+                                        ('host2', 'user2', 'domain2')])
+
+    # removing of t2841_netgroup1 from t2841_netgroup3
+    old = {'memberNisNetgroup': ["t2841_netgroup1", "t2841_netgroup2"]}
+    new = {'memberNisNetgroup': ["t2841_netgroup2"]}
+
+    ldif = ldap.modlist.modifyModlist(old, new)
+    ldap_conn.modify_s(netgroup_dn, ldif)
+
+    if subprocess.call(["sss_cache", "-N"]) != 0:
+        raise Exception("sssd_cache failed")
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup1")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [('host1', 'user1', 'domain1')]
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup2")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [('host2', 'user2', 'domain2')]
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup3")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [('host2', 'user2', 'domain2')]
+
+    # removing of t2841_netgroup2 from t2841_netgroup3
+    old = {'memberNisNetgroup': ["t2841_netgroup2"]}
+    new = {'memberNisNetgroup': []}
+
+    ldif = ldap.modlist.modifyModlist(old, new)
+    ldap_conn.modify_s(netgroup_dn, ldif)
+
+    if subprocess.call(["sss_cache", "-N"]) != 0:
+        raise Exception("sssd_cache failed")
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup1")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [('host1', 'user1', 'domain1')]
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup2")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == [('host2', 'user2', 'domain2')]
+
+    res, _, netgroups = sssd_ntg.get_sssd_netgroups("t2841_netgroup3")
+    assert res == sssd_ntg.NssReturnCode.SUCCESS
+    assert netgroups == []
-- 
2.7.4

_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/admin/lists/[email protected]

Reply via email to