Issue 5184 - memberOf does not work correctly with multiple include s…

…copes

Bug Description:

MemberOf Plugin only looks at the first include scope, and the rest are
ignored. So if multiple "memberOfEntryScope" attributes are set then the
plugin will not work as expected.

Fix Description:

The fix is to read all the memberOfEntryScope attributes and update the
group cache.

relates: #5184

Reviewed by: tbordaz(Thanks!)

dirsrvtests/tests/suites/memberof_plugin/memberof_include_scopes_test.py

@@ -0,0 +1,105 @@
+# --- BEGIN COPYRIGHT BLOCK ---
+# Copyright (C) 2022 Red Hat, Inc.
+# All rights reserved.
+#
+# License: GPL (version 3 or any later version).
+# See LICENSE for details.
+# --- END COPYRIGHT BLOCK ---
+#
+import pytest
+import os
+import time
+import ldap
+from lib389.utils import ensure_str
+from lib389.topologies import topology_st as topo
+from lib389._constants import *
+from lib389.plugins import MemberOfPlugin
+from lib389.idm.user import UserAccounts
+from lib389.idm.group import Groups
+from lib389.idm.nscontainer import nsContainers
+
+SUBTREE_1 = 'cn=sub1,%s' % SUFFIX
+SUBTREE_2 = 'cn=sub2,%s' % SUFFIX
+SUBTREE_3 = 'cn=sub3,%s' % SUFFIX
+
+def add_container(inst, dn, name):
+    """Creates container entry"""
+    conts = nsContainers(inst, dn)
+    cont = conts.create(properties={'cn': name})
+    return cont
+
+def add_member_and_group(server, cn, group_cn, subtree):
+    users = UserAccounts(server, subtree, rdn=None)
+    users.create(properties={'uid': f'test_{cn}',
+                             'cn': f'test_{cn}',
+                             'sn': f'test_{cn}',
+                             'description': 'member',
+                             'uidNumber': '1000',
+                             'gidNumber': '2000',
+                             'homeDirectory': '/home/testuser'})
+    group = Groups(server, subtree, rdn=None)
+    group.create(properties={'cn': group_cn,
+                             'member': f'uid=test_{cn},{subtree}',
+                             'description': 'group'})
+
+def check_membership(server, user_dn=None, group_dn=None, find_result=True):
+    ent = server.getEntry(user_dn, ldap.SCOPE_BASE, "(objectclass=*)", ['memberof'])
+    found = False
+    if ent.hasAttr('memberof'):
+        for val in ent.getValues('memberof'):
+            if ensure_str(val) == group_dn:
+                found = True
+                break
+
+    if find_result:
+        assert found
+    else:
+        assert (not found)
+
+def test_multiple_scopes(topo):
+    """Specify memberOf works when multiple include scopes are defined
+
+    :id: fbcd70cc-c83d-4c79-bd5b-2d8f017545ae
+    :setup: Standalone Instance
+    :steps:
+        1. Set multiple include scopes
+        2. Test members added to both scopes are correctly updated
+        3. Test user outside of scope was not updated
+    :expectedresults:
+        1. Success
+        2. Success
+        3. Success
+    """
+
+    inst = topo.standalone
+
+    # configure plugin
+    memberof = MemberOfPlugin(inst)
+    memberof.enable()
+    memberof.add('memberOfEntryScope', SUBTREE_1)
+    memberof.add('memberOfEntryScope', SUBTREE_2)
+    inst.restart()
+
+    # Add setup entries
+    add_container(inst, SUFFIX, 'sub1')
+    add_container(inst, SUFFIX, 'sub2')
+    add_container(inst, SUFFIX, 'sub3')
+    add_member_and_group(inst, 'm1', 'g1', SUBTREE_1)
+    add_member_and_group(inst, 'm2', 'g2', SUBTREE_2)
+    add_member_and_group(inst, 'm3', 'g3', SUBTREE_3)
+
+    # Check users 1 and 2 were correctly updated
+    check_membership(inst, f'uid=test_m1,{SUBTREE_1}', f'cn=g1,{SUBTREE_1}', True)
+    check_membership(inst, f'uid=test_m2,{SUBTREE_2}', f'cn=g2,{SUBTREE_2}', True)
+
+    # Check that user3, which is out of scope, was not updated
+    check_membership(inst, f'uid=test_m3,{SUBTREE_3}', f'cn=g1,{SUBTREE_1}', False)
+    check_membership(inst, f'uid=test_m3,{SUBTREE_3}', f'cn=g2,{SUBTREE_2}', False)
+    check_membership(inst, f'uid=test_m3,{SUBTREE_3}', f'cn=g3,{SUBTREE_3}', False)
+
+
+if __name__ == '__main__':
+    # Run isolated
+    # -s for DEBUG mode
+    CURRENT_FILE = os.path.realpath(__file__)
+    pytest.main(["-s", CURRENT_FILE])

ldap/servers/plugins/memberof/memberof.c

@@ -626,21 +626,6 @@ memberof_del_dn_type_callback(Slapi_Entry *e, void *callback_data)
     return rc;
 }
 
-/* Check if the the entry include scope is a child of the sdn */
-static Slapi_DN *
-memberof_scope_is_child_of_dn(MemberOfConfig *config, Slapi_DN *sdn)
-{
-    int i = 0;
-
-    while (config->entryScopes && config->entryScopes[i]) {
-        if (slapi_sdn_issuffix(config->entryScopes[i], sdn)) {
-            return config->entryScopes[i];
-        }
-        i++;
-    }
-    return NULL;
-}
-
 static void
 add_ancestors_cbdata(memberof_cached_value *ancestors, void *callback_data)
 {
@@ -791,8 +776,7 @@ memberof_call_foreach_dn(Slapi_PBlock *pb __attribute__((unused)), Slapi_DN *sdn
     search_pb = slapi_pblock_new();
     be = slapi_get_first_backend(&cookie);
     while (be) {
-        Slapi_DN *scope_sdn = NULL;
-
+        PRBool do_suffix_search = PR_TRUE;
         if (!all_backends) {
             be = slapi_be_select(sdn);
             if (be == NULL) {
@@ -813,9 +797,19 @@ memberof_call_foreach_dn(Slapi_PBlock *pb __attribute__((unused)), Slapi_DN *sdn
             if (memberof_entry_in_scope(config, base_sdn)) {
                 /* do nothing, entry scope is spanning
                  * multiple suffixes, start at suffix */
-            } else if ((scope_sdn = memberof_scope_is_child_of_dn(config, base_sdn))) {
-                /* scope is below suffix, set search base */
-                base_sdn = scope_sdn;
+            } else if (config->entryScopes) {
+                for (size_t i = 0; config->entryScopes[i]; i++) {
+                    if (slapi_sdn_issuffix(config->entryScopes[i], base_sdn)) {
+                        /* Search each include scope */
+                        slapi_search_internal_set_pb(search_pb, slapi_sdn_get_dn(config->entryScopes[i]),
+                                                     LDAP_SCOPE_SUBTREE, filter_str, 0, 0, 0, 0,
+                                                     memberof_get_plugin_id(), 0);
+                        slapi_search_internal_callback_pb(search_pb, callback_data, 0, callback, 0);
+                        /* We already did the search for this backend, don't
+                         * do it again when we fall through */
+                        do_suffix_search = PR_FALSE;
+                    }
+                }
             } else if (!all_backends) {
                 break;
             } else {
@@ -825,17 +819,20 @@ memberof_call_foreach_dn(Slapi_PBlock *pb __attribute__((unused)), Slapi_DN *sdn
             }
         }
 
-        slapi_search_internal_set_pb(search_pb, slapi_sdn_get_dn(base_sdn),
-                                     LDAP_SCOPE_SUBTREE, filter_str, 0, 0, 0, 0, memberof_get_plugin_id(), 0);
-        slapi_search_internal_callback_pb(search_pb, callback_data, 0, callback, 0);
-        slapi_pblock_get(search_pb, SLAPI_PLUGIN_INTOP_RESULT, &rc);
-        if (rc != LDAP_SUCCESS) {
-            break;
+        if (do_suffix_search) {
+            slapi_search_internal_set_pb(search_pb, slapi_sdn_get_dn(base_sdn),
+                                         LDAP_SCOPE_SUBTREE, filter_str, 0, 0, 0, 0, memberof_get_plugin_id(), 0);
+            slapi_search_internal_callback_pb(search_pb, callback_data, 0, callback, 0);
+            slapi_pblock_get(search_pb, SLAPI_PLUGIN_INTOP_RESULT, &rc);
+            if (rc != LDAP_SUCCESS) {
+                break;
+            }
         }
 
         if (!all_backends) {
             break;
         }
+        /* Reset pb for next loop */
         slapi_pblock_init(search_pb);
         be = slapi_get_next_backend(cookie);
     }