From a06c2327fc99df0d9c7d6a8a00381fcdaf5c18c7 Mon Sep 17 00:00:00 2001 From: Thierry Bordaz Date: Oct 26 2018 12:29:37 +0000 Subject: Ticket 49985 - memberof may silently fails to update a member Bug Description: when adding 'memberof' to a member entry, the update may fail (invalid schema, db errors...). The error is reported at upper level. But in case of MODRDN the error is lost in memberof_moddn_attr_list where returned code of memberof_modop_one_replace_r is not tested Fix Description: Report a failure in memberof_moddn_attr_list as soon as memberof_modop_one_replace_r fails https://pagure.io/389-ds-base/issue/49985 Reviewed by: Simon Pichugi, William Brown Platforms tested: F27 Flag Day: no Doc impact: no --- diff --git a/dirsrvtests/tests/suites/memberof_plugin/regression_test.py b/dirsrvtests/tests/suites/memberof_plugin/regression_test.py index fb2b4da..2b0c4ae 100644 --- a/dirsrvtests/tests/suites/memberof_plugin/regression_test.py +++ b/dirsrvtests/tests/suites/memberof_plugin/regression_test.py @@ -514,7 +514,8 @@ def _config_memberof_entrycache_on_modrdn_failure(server): peoplebase = 'ou=people,%s' % SUFFIX MEMBEROF_PLUGIN_DN = ('cn=' + PLUGIN_MEMBER_OF + ',cn=plugins,cn=config') server.modify_s(MEMBEROF_PLUGIN_DN, [(ldap.MOD_REPLACE, 'memberOfAllBackends', b'on'), - (ldap.MOD_REPLACE, 'memberOfEntryScope', peoplebase.encode())]) + (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') @@ -523,7 +524,8 @@ def _disable_auto_oc_memberof(server): @pytest.mark.ds49967 def test_entrycache_on_modrdn_failure(topology_st): - """Specify a test case purpose or name here + """This test checks that when a modrdn fails, the destination entry is not returned by a search + This could happen in case the destination entry remains in the entry cache :id: a4d8ac0b-2448-406a-9dc2-5a72851e30b6 :setup: Standalone Instance @@ -669,6 +671,185 @@ 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 + + :id: 095aee01-581c-43dd-a241-71f9631a18bb + :setup: Standalone Instance + :steps: + 1. configure memberof to only scope ou=people,SUFFIX + 2. Do some cleanup and Creates 10 users + 3. Create groups0 (IN peoplebase) that contain user0 and user1 + 4. Check user0 and user1 have memberof=group0.dn + 5. Create group1 (OUT peoplebase) that contain user0 and user1 + 6. Check user0 and user1 have NOT memberof=group1.dn + 7. Move group1 IN peoplebase and check users0 and user1 HAVE memberof=group1.dn + 8. Create group2 (OUT peoplebase) that contain user2 and user3. + 9. Check user2 and user3 have NOT memberof=group2.dn + 10. configure memberof so that added objectclass does not allow 'memberof' attribute + 11. Move group2 IN peoplebase and check move failed OPERATIONS_ERROR (because memberof failed) + 12. Check user2 and user3 have NOT memberof=group2.dn + 13. ADD group3 (IN peoplebase) with user4 and user5 members and check add failed OPERATIONS_ERROR (because memberof failed) + 14. Check user4 and user5 have NOT memberof=group2.dn + :expectedresults: + 1. should succeed + 2. should succeed + 3. should succeed + 4. should succeed + 5. should succeed + 6. should succeed + 7. should succeed + 8. should succeed + 9. should succeed + 10. should succeed + 11. should fail OPERATION_ERROR because memberof plugin fails to add 'memberof' to members. + 12. should succeed + 14. should fail OPERATION_ERROR because memberof plugin fails to add 'memberof' to members + 14. should succeed + """ + # only scopes peoplebase + _config_memberof_silent_memberof_failure(topology_st.standalone) + topology_st.standalone.restart(timeout=10) + + # first do some cleanup + peoplebase = 'ou=people,%s' % SUFFIX + for i in range(10): + cn = 'user%d' % i + dn = 'cn=%s,%s' % (cn, peoplebase) + topology_st.standalone.delete_s(dn) + topology_st.standalone.delete_s('cn=group_in0,%s' % peoplebase) + topology_st.standalone.delete_s('cn=group_in1,%s' % peoplebase) + topology_st.standalone.delete_s('cn=group_out2,%s' % SUFFIX) + + # create 10 users + for i in range(10): + cn = 'user%d' % i + dn = 'cn=%s,%s' % (cn, peoplebase) + log.fatal('Adding user (%s): ' % dn) + topology_st.standalone.add_s(Entry((dn, {'objectclass': ['top', 'person'], + 'sn': 'user_%s' % cn, + 'description': 'add on standalone'}))) + + # Check that members of group0 (in the scope) have 'memberof + group0_dn = 'cn=group_in0,%s' % peoplebase + topology_st.standalone.add_s(Entry((group0_dn, {'objectclass': ['top', 'groupofnames'], + 'member': [ + 'cn=user0,%s' % peoplebase, + 'cn=user1,%s' % peoplebase, + ], + 'description': 'mygroup'}))) + + # Check the those entries have memberof with group0 + for i in range(2): + user_dn = 'cn=user%d,%s' % (i, peoplebase) + ent = topology_st.standalone.getEntry(user_dn, ldap.SCOPE_BASE, "(objectclass=*)", ['memberof']) + assert ent.hasAttr('memberof') + found = False + for val in ent.getValues('memberof'): + topology_st.standalone.log.info("!!!!!!! %s: memberof->%s (vs %s)" % (user_dn, val, group0_dn.encode().lower())) + if val.lower() == group0_dn.encode().lower(): + found = True + break + assert found + + # Create a group1 out of the scope + group1_dn = 'cn=group_out1,%s' % SUFFIX + topology_st.standalone.add_s(Entry((group1_dn, {'objectclass': ['top', 'groupofnames'], + 'member': [ + 'cn=user0,%s' % peoplebase, + 'cn=user1,%s' % peoplebase, + ], + 'description': 'mygroup'}))) + + # Check the those entries have not memberof with group1 + for i in range(2): + user_dn = 'cn=user%d,%s' % (i, peoplebase) + ent = topology_st.standalone.getEntry(user_dn, ldap.SCOPE_BASE, "(objectclass=*)", ['memberof']) + assert ent.hasAttr('memberof') + found = False + for val in ent.getValues('memberof'): + topology_st.standalone.log.info("!!!!!!! %s: memberof->%s (vs %s)" % (user_dn, val, group1_dn.encode().lower())) + if val.lower() == group1_dn.encode().lower(): + found = True + break + assert not found + + # move group1 into the scope and check user0 and user1 are memberof group1 + topology_st.standalone.rename_s(group1_dn, 'cn=group_in1', newsuperior=peoplebase, delold=0) + new_group1_dn = 'cn=group_in1,%s' % peoplebase + for i in range(2): + user_dn = 'cn=user%d,%s' % (i, peoplebase) + ent = topology_st.standalone.getEntry(user_dn, ldap.SCOPE_BASE, "(objectclass=*)", ['memberof']) + assert ent.hasAttr('memberof') + found = False + for val in ent.getValues('memberof'): + topology_st.standalone.log.info("!!!!!!! %s: memberof->%s (vs %s)" % (user_dn, val, new_group1_dn.encode().lower())) + if val.lower() == new_group1_dn.encode().lower(): + found = True + break + assert found + + # Create a group2 out of the scope + group2_dn = 'cn=group_out2,%s' % SUFFIX + topology_st.standalone.add_s(Entry((group2_dn, {'objectclass': ['top', 'groupofnames'], + 'member': [ + 'cn=user2,%s' % peoplebase, + 'cn=user3,%s' % peoplebase, + ], + 'description': 'mygroup'}))) + + # Check the those entries have not memberof with group2 + for i in (2, 3): + user_dn = 'cn=user%d,%s' % (i, peoplebase) + ent = topology_st.standalone.getEntry(user_dn, ldap.SCOPE_BASE, "(objectclass=*)", ['memberof']) + assert not ent.hasAttr('memberof') + + # memberof will not add the missing objectclass + _disable_auto_oc_memberof(topology_st.standalone) + topology_st.standalone.restart(timeout=10) + + # move group2 into the scope and check it fails + try: + 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: + pass + + # Check the those entries have not memberof + for i in (2, 3): + user_dn = 'cn=user%d,%s' % (i, peoplebase) + ent = topology_st.standalone.getEntry(user_dn, ldap.SCOPE_BASE, "(objectclass=*)", ['memberof']) + topology_st.standalone.log.info("Should assert %s has memberof is %s" % (user_dn, ent.hasAttr('memberof'))) + assert not ent.hasAttr('memberof') + + # Create a group3 in the scope + group3_dn = 'cn=group3_in,%s' % peoplebase + try: + topology_st.standalone.add_s(Entry((group3_dn, {'objectclass': ['top', 'groupofnames'], + 'member': [ + 'cn=user4,%s' % peoplebase, + 'cn=user5,%s' % peoplebase, + ], + 'description': 'mygroup'}))) + topology_st.standalone.log.info("This is unexpected, ADD should fail as the member entry have not the appropriate objectclass") + assert False + except ldap.OBJECT_CLASS_VIOLATION: + pass + except ldap.OPERATIONS_ERROR: + pass + + # Check the those entries have not 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']) + topology_st.standalone.log.info("Should assert %s has memberof is %s" % (user_dn, ent.hasAttr('memberof'))) + assert not ent.hasAttr('memberof') + if __name__ == '__main__': # Run isolated # -s for DEBUG mode diff --git a/ldap/servers/plugins/memberof/memberof.c b/ldap/servers/plugins/memberof/memberof.c index 87313ff..855a68f 100644 --- a/ldap/servers/plugins/memberof/memberof.c +++ b/ldap/servers/plugins/memberof/memberof.c @@ -1973,7 +1973,7 @@ memberof_moddn_attr_list(Slapi_PBlock *pb, MemberOfConfig *config, Slapi_DN *pre int hint = slapi_attr_first_value(attr, &val); Slapi_DN *sdn = slapi_sdn_new(); - while (val) { + while (val && (rc == 0)) { char *dn_str = 0; struct berval *bv = (struct berval *)slapi_value_get_berval(val); @@ -1996,7 +1996,7 @@ memberof_moddn_attr_list(Slapi_PBlock *pb, MemberOfConfig *config, Slapi_DN *pre strncpy(dn_str, bv->bv_val, (size_t)bv->bv_len); slapi_sdn_set_normdn_byref(sdn, dn_str); /* dn_str is normalized */ - memberof_modop_one_replace_r(pb, config, LDAP_MOD_REPLACE, + rc = memberof_modop_one_replace_r(pb, config, LDAP_MOD_REPLACE, post_sdn, pre_sdn, post_sdn, sdn, 0); hint = slapi_attr_next_value(attr, hint, &val);