From 933b46f1f434ac7c11e155611c91f21e31c4d6f7 Mon Sep 17 00:00:00 2001 From: Mark Reynolds 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