From 933b46f1f434ac7c11e155611c91f21e31c4d6f7 Mon Sep 17 00:00:00 2001
From: Mark Reynolds <mreynolds@redhat.com>
Date: Thu, 21 Feb 2019 16:58:05 -0500
Subject: [PATCH] Ticket 50236 - memberOf should be more robust
Bug Description: When doing a modrdn, or any memberOf update, if the entry
already has the memberOf attribute with the same value
the operation is incorrectly rejected.
Fix Description: If we get an error 20 (type or value exists) return success.
Also fixed a coding mistake that causes the wrong error
code to be returned. This also required fixing the CI
test to check for the new correct errro code.
https://pagure.io/389-ds-base/issue/50236
Reviewed by: firstyear, spichugi, and tbordaz (Thanks!!!)
---
.../suites/memberof_plugin/regression_test.py | 48 ++++++++++---------
ldap/servers/plugins/memberof/memberof.c | 35 ++++++++++----
2 files changed, 53 insertions(+), 30 deletions(-)
diff --git a/dirsrvtests/tests/suites/memberof_plugin/regression_test.py b/dirsrvtests/tests/suites/memberof_plugin/regression_test.py
index 2b0c4aec2..9d0ce35ed 100644
--- a/dirsrvtests/tests/suites/memberof_plugin/regression_test.py
+++ b/dirsrvtests/tests/suites/memberof_plugin/regression_test.py
@@ -11,13 +11,12 @@ import pytest
import os
import time
import ldap
-import subprocess
from random import sample
from lib389.utils import ds_is_older, ensure_list_bytes, ensure_bytes, ensure_str
from lib389.topologies import topology_m1h1c1 as topo, topology_st, topology_m2 as topo_m2
from lib389._constants import *
from lib389.plugins import MemberOfPlugin
-from lib389 import agreement, Entry
+from lib389 import Entry
from lib389.idm.user import UserAccount, UserAccounts, TEST_USER_PROPERTIES
from lib389.idm.group import Groups, Group
from lib389.replica import ReplicationManager
@@ -49,7 +48,7 @@ def add_users(topo_m2, users_num, suffix):
users_list = []
users = UserAccounts(topo_m2.ms["master1"], suffix, rdn=None)
log.info('Adding %d users' % users_num)
- for num in sample(range(1000), users_num):
+ for num in sample(list(range(1000)), users_num):
num_ran = int(round(num))
USER_NAME = 'test%05d' % num_ran
user = users.create(properties={
@@ -76,8 +75,8 @@ def config_memberof(server):
for ent in ents:
log.info('update %s to add nsDS5ReplicatedAttributeListTotal' % ent.dn)
server.agreement.setProperties(agmnt_dn=ents[0].dn,
- properties={RA_FRAC_EXCLUDE:'(objectclass=*) $ EXCLUDE memberOf',
- RA_FRAC_EXCLUDE_TOTAL_UPDATE:'(objectclass=*) $ EXCLUDE '})
+ properties={RA_FRAC_EXCLUDE: '(objectclass=*) $ EXCLUDE memberOf',
+ RA_FRAC_EXCLUDE_TOTAL_UPDATE: '(objectclass=*) $ EXCLUDE '})
def send_updates_now(server):
@@ -184,14 +183,14 @@ def test_memberof_with_repl(topo):
for i in range(3):
CN = '%s%d' % (GROUP_CN, i)
groups = Groups(M1, SUFFIX)
- testgroup = groups.create(properties={'cn' : CN})
+ testgroup = groups.create(properties={'cn': CN})
time.sleep(2)
test_groups.append(testgroup)
# Step 4
#Now start testing by adding differnt user to differn group
if not ds_is_older('1.3.7'):
- test_groups[0].remove('objectClass', 'nsMemberOf')
+ test_groups[0].remove('objectClass', 'nsMemberOf')
member_dn = test_users[0].dn
grp0_dn = test_groups[0].dn
@@ -211,7 +210,7 @@ def test_memberof_with_repl(topo):
# Step 7
for i in [grp0_dn, grp1_dn]:
for inst in [M1, H1, C1]:
- _find_memberof(inst, member_dn, i)
+ _find_memberof(inst, member_dn, i)
# Step 8
for i in [M1, H1, C1]:
@@ -225,7 +224,7 @@ def test_memberof_with_repl(topo):
# For negative testcase, we are using assertionerror
for inst in [M1, H1, C1]:
for i in [grp0_dn, member_dn]:
- with pytest.raises(AssertionError):
+ with pytest.raises(AssertionError):
_find_memberof(inst, i, grp1_dn)
# Step 11
@@ -369,7 +368,7 @@ def test_memberof_with_changelog_reset(topo_m2):
'objectclass': ensure_list_bytes(['top', 'groupOfNames'])}
for user in users_list:
- dic_of_attributes.setdefault('member',[])
+ dic_of_attributes.setdefault('member', [])
dic_of_attributes['member'].append(user.dn)
log.info('Adding the test group using async function')
@@ -427,7 +426,7 @@ def rename_entry(server, cn, from_subtree, to_subtree):
server.rename_s(dn, nrdn, newsuperior=to_subtree, delold=0)
-def _find_memberof(server, user_dn=None, group_dn=None, find_result=True):
+def _find_memberof_ext(server, user_dn=None, group_dn=None, find_result=True):
assert (server)
assert (user_dn)
assert (group_dn)
@@ -495,18 +494,19 @@ def test_memberof_group(topology_st):
dn2 = '%s,%s' % ('uid=test_m2', SUBTREE_1)
g1 = '%s,%s' % ('cn=g1', SUBTREE_1)
g2 = '%s,%s' % ('cn=g2', SUBTREE_2)
- _find_memberof(inst, dn1, g1, True)
- _find_memberof(inst, dn2, g1, True)
- _find_memberof(inst, dn1, g2, False)
- _find_memberof(inst, dn2, g2, False)
+ _find_memberof_ext(inst, dn1, g1, True)
+ _find_memberof_ext(inst, dn2, g1, True)
+ _find_memberof_ext(inst, dn1, g2, False)
+ _find_memberof_ext(inst, dn2, g2, False)
rename_entry(inst, 'cn=g2', SUBTREE_2, SUBTREE_1)
g2n = '%s,%s' % ('cn=g2-new', SUBTREE_1)
- _find_memberof(inst, dn1, g1, True)
- _find_memberof(inst, dn2, g1, True)
- _find_memberof(inst, dn1, g2n, True)
- _find_memberof(inst, dn2, g2n, True)
+ _find_memberof_ext(inst, dn1, g1, True)
+ _find_memberof_ext(inst, dn2, g1, True)
+ _find_memberof_ext(inst, dn1, g2n, True)
+ _find_memberof_ext(inst, dn2, g2n, True)
+
def _config_memberof_entrycache_on_modrdn_failure(server):
@@ -517,11 +517,13 @@ def _config_memberof_entrycache_on_modrdn_failure(server):
(ldap.MOD_REPLACE, 'memberOfEntryScope', peoplebase.encode()),
(ldap.MOD_REPLACE, 'memberOfAutoAddOC', b'nsMemberOf')])
+
def _disable_auto_oc_memberof(server):
MEMBEROF_PLUGIN_DN = ('cn=' + PLUGIN_MEMBER_OF + ',cn=plugins,cn=config')
server.modify_s(MEMBEROF_PLUGIN_DN,
[(ldap.MOD_REPLACE, 'memberOfAutoAddOC', b'nsContainer')])
+
@pytest.mark.ds49967
def test_entrycache_on_modrdn_failure(topology_st):
"""This test checks that when a modrdn fails, the destination entry is not returned by a search
@@ -657,7 +659,7 @@ def test_entrycache_on_modrdn_failure(topology_st):
topology_st.standalone.rename_s(group2_dn, 'cn=group_in2', newsuperior=peoplebase, delold=0)
topology_st.standalone.log.info("This is unexpected, modrdn should fail as the member entry have not the appropriate objectclass")
assert False
- except ldap.OPERATIONS_ERROR:
+ except ldap.OBJECT_CLASS_VIOLATION:
pass
# retrieve the entry having the specific description value
@@ -671,9 +673,11 @@ def test_entrycache_on_modrdn_failure(topology_st):
assert ent.dn == group2_dn
assert found
+
def _config_memberof_silent_memberof_failure(server):
_config_memberof_entrycache_on_modrdn_failure(server)
+
def test_silent_memberof_failure(topology_st):
"""This test checks that if during a MODRDN, the memberof plugin fails
then MODRDN also fails
@@ -817,7 +821,7 @@ def test_silent_memberof_failure(topology_st):
topology_st.standalone.rename_s(group2_dn, 'cn=group_in2', newsuperior=peoplebase, delold=0)
topology_st.standalone.log.info("This is unexpected, modrdn should fail as the member entry have not the appropriate objectclass")
assert False
- except ldap.OPERATIONS_ERROR:
+ except ldap.OBJECT_CLASS_VIOLATION:
pass
# Check the those entries have not memberof
@@ -843,7 +847,7 @@ def test_silent_memberof_failure(topology_st):
except ldap.OPERATIONS_ERROR:
pass
- # Check the those entries have not memberof
+ # Check the those entries do not have memberof
for i in (4, 5):
user_dn = 'cn=user%d,%s' % (i, peoplebase)
ent = topology_st.standalone.getEntry(user_dn, ldap.SCOPE_BASE, "(objectclass=*)", ['memberof'])
diff --git a/ldap/servers/plugins/memberof/memberof.c b/ldap/servers/plugins/memberof/memberof.c
index 329a845a7..b54eb3977 100644
--- a/ldap/servers/plugins/memberof/memberof.c
+++ b/ldap/servers/plugins/memberof/memberof.c
@@ -919,8 +919,7 @@ memberof_postop_modrdn(Slapi_PBlock *pb)
* entry that is being renamed. */
for (i = 0; configCopy.groupattrs && configCopy.groupattrs[i]; i++) {
if (0 == slapi_entry_attr_find(post_e, configCopy.groupattrs[i], &attr)) {
- if ((ret = memberof_moddn_attr_list(pb, &configCopy, pre_sdn,
- post_sdn, attr) != 0)) {
+ if ((ret = memberof_moddn_attr_list(pb, &configCopy, pre_sdn, post_sdn, attr)) != 0) {
slapi_log_err(SLAPI_LOG_ERR, MEMBEROF_PLUGIN_SUBSYSTEM,
"memberof_postop_modrdn - Update failed for (%s), error (%d)\n",
slapi_sdn_get_dn(pre_sdn), ret);
@@ -1720,12 +1719,32 @@ memberof_modop_one_replace_r(Slapi_PBlock *pb, MemberOfConfig *config, int mod_o
replace_mod.mod_values = replace_val;
}
rc = memberof_add_memberof_attr(mods, op_to, config->auto_add_oc);
- if (rc == LDAP_NO_SUCH_ATTRIBUTE) {
- /* the memberof values to be replaced do not exist
- * just add the new values */
- mods[0] = mods[1];
- mods[1] = NULL;
- rc = memberof_add_memberof_attr(mods, op_to, config->auto_add_oc);
+ if (rc == LDAP_NO_SUCH_ATTRIBUTE || rc == LDAP_TYPE_OR_VALUE_EXISTS) {
+ if (rc == LDAP_TYPE_OR_VALUE_EXISTS) {
+ /*
+ * For some reason the new modrdn value is present, so retry
+ * the delete by itself and ignore the add op by tweaking
+ * the mod array.
+ */
+ mods[1] = NULL;
+ rc = memberof_add_memberof_attr(mods, op_to, config->auto_add_oc);
+ } else {
+ /*
+ * The memberof value to be replaced does not exist so just
+ * add the new value. Shuffle the mod array to apply only
+ * the add operation.
+ */
+ mods[0] = mods[1];
+ mods[1] = NULL;
+ rc = memberof_add_memberof_attr(mods, op_to, config->auto_add_oc);
+ if (rc == LDAP_TYPE_OR_VALUE_EXISTS) {
+ /*
+ * The entry already has the expected memberOf value, no
+ * problem just return success.
+ */
+ rc = LDAP_SUCCESS;
+ }
+ }
}
}
}
--
2.17.2