From 220dbafa048269105b3f7958a5d5bfd1d988da26 Mon Sep 17 00:00:00 2001 From: Simon Pichugin Date: Tue, 30 Jun 2020 15:39:30 +0200 Subject: [PATCH 8/8] Issue 49300 - entryUSN is duplicated after memberOf operation Bug Description: When we assign a member to a group we have two oprations - group modification and user modification. As a result, they both have the same entryUSN because USN Plugin assigns entryUSN value in bepreop but increments the counter in the postop and a lot of things can happen in between. Fix Description: Increment the counter in bepreop together with entryUSN assignment. Also, decrement the counter in bepostop if the failuer has happened. Add test suite to cover the change. https://pagure.io/389-ds-base/issue/49300 Reviewed by: tbordaz (Thanks!) --- .../tests/suites/plugins/entryusn_test.py | 240 ++++++++++++++++++ ldap/servers/plugins/usn/usn.c | 109 ++++---- ldap/servers/slapd/pblock.c | 14 +- ldap/servers/slapd/pblock_v3.h | 1 + ldap/servers/slapd/slapi-plugin.h | 3 + 5 files changed, 322 insertions(+), 45 deletions(-) create mode 100644 dirsrvtests/tests/suites/plugins/entryusn_test.py diff --git a/dirsrvtests/tests/suites/plugins/entryusn_test.py b/dirsrvtests/tests/suites/plugins/entryusn_test.py new file mode 100644 index 000000000..721315419 --- /dev/null +++ b/dirsrvtests/tests/suites/plugins/entryusn_test.py @@ -0,0 +1,240 @@ +# --- BEGIN COPYRIGHT BLOCK --- +# Copyright (C) 2020 Red Hat, Inc. +# All rights reserved. +# +# License: GPL (version 3 or any later version). +# See LICENSE for details. +# --- END COPYRIGHT BLOCK --- +# +import ldap +import logging +import pytest +from lib389._constants import DEFAULT_SUFFIX +from lib389.config import Config +from lib389.plugins import USNPlugin, MemberOfPlugin +from lib389.idm.group import Groups +from lib389.idm.user import UserAccounts +from lib389.idm.organizationalunit import OrganizationalUnit +from lib389.tombstone import Tombstones +from lib389.rootdse import RootDSE +from lib389.topologies import topology_st, topology_m2 + +log = logging.getLogger(__name__) + +USER_NUM = 10 +GROUP_NUM = 3 + + +def check_entryusn_no_duplicates(entryusn_list): + """Check that all values in the list are unique""" + + if len(entryusn_list) > len(set(entryusn_list)): + raise AssertionError(f"EntryUSN values have duplicates, please, check logs") + + +def check_lastusn_after_restart(inst): + """Check that last usn is the same after restart""" + + root_dse = RootDSE(inst) + last_usn_before = root_dse.get_attr_val_int("lastusn;userroot") + inst.restart() + last_usn_after = root_dse.get_attr_val_int("lastusn;userroot") + assert last_usn_after == last_usn_before + + +@pytest.fixture(scope="module") +def setup(topology_st, request): + """ + Enable USN plug-in + Enable MEMBEROF plugin + Add test entries + """ + + inst = topology_st.standalone + + log.info("Enable the USN plugin...") + plugin = USNPlugin(inst) + plugin.enable() + + log.info("Enable the MEMBEROF plugin...") + plugin = MemberOfPlugin(inst) + plugin.enable() + + inst.restart() + + users_list = [] + log.info("Adding test entries...") + users = UserAccounts(inst, DEFAULT_SUFFIX) + for id in range(USER_NUM): + user = users.create_test_user(uid=id) + users_list.append(user) + + groups_list = [] + log.info("Adding test groups...") + groups = Groups(inst, DEFAULT_SUFFIX) + for id in range(GROUP_NUM): + group = groups.create(properties={'cn': f'test_group{id}'}) + groups_list.append(group) + + def fin(): + for user in users_list: + try: + user.delete() + except ldap.NO_SUCH_OBJECT: + pass + for group in groups_list: + try: + group.delete() + except ldap.NO_SUCH_OBJECT: + pass + request.addfinalizer(fin) + + return {"users": users_list, + "groups": groups_list} + + +def test_entryusn_no_duplicates(topology_st, setup): + """Verify that entryUSN is not duplicated after memberOf operation + + :id: 1a7d382d-1214-4d56-b9c2-9c4ed57d1683 + :setup: Standalone instance, Groups and Users, USN and memberOf are enabled + :steps: + 1. Add a member to group 1 + 2. Add a member to group 1 and 2 + 3. Check that entryUSNs are different + 4. Check that lastusn before and after a restart are the same + :expectedresults: + 1. Success + 2. Success + 3. Success + 4. Success + """ + + inst = topology_st.standalone + config = Config(inst) + config.replace('nsslapd-accesslog-level', '260') # Internal op + config.replace('nsslapd-errorlog-level', '65536') + config.replace('nsslapd-plugin-logging', 'on') + entryusn_list = [] + + users = setup["users"] + groups = setup["groups"] + + groups[0].replace('member', users[0].dn) + entryusn_list.append(users[0].get_attr_val_int('entryusn')) + log.info(f"{users[0].dn}_1: {entryusn_list[-1:]}") + entryusn_list.append(groups[0].get_attr_val_int('entryusn')) + log.info(f"{groups[0].dn}_1: {entryusn_list[-1:]}") + check_entryusn_no_duplicates(entryusn_list) + + groups[1].replace('member', [users[0].dn, users[1].dn]) + entryusn_list.append(users[0].get_attr_val_int('entryusn')) + log.info(f"{users[0].dn}_2: {entryusn_list[-1:]}") + entryusn_list.append(users[1].get_attr_val_int('entryusn')) + log.info(f"{users[1].dn}_2: {entryusn_list[-1:]}") + entryusn_list.append(groups[1].get_attr_val_int('entryusn')) + log.info(f"{groups[1].dn}_2: {entryusn_list[-1:]}") + check_entryusn_no_duplicates(entryusn_list) + + check_lastusn_after_restart(inst) + + +def test_entryusn_is_same_after_failure(topology_st, setup): + """Verify that entryUSN is the same after failed operation + + :id: 1f227533-370a-48c1-b920-9b3b0bcfc32e + :setup: Standalone instance, Groups and Users, USN and memberOf are enabled + :steps: + 1. Get current group's entryUSN value + 2. Try to modify the group with an invalid syntax + 3. Get new group's entryUSN value and compare with old + 4. Check that lastusn before and after a restart are the same + :expectedresults: + 1. Success + 2. Invalid Syntax error + 3. Should be the same + 4. Success + """ + + inst = topology_st.standalone + users = setup["users"] + + # We need this update so we get the latest USN pointed to our entry + users[0].replace('description', 'update') + + entryusn_before = users[0].get_attr_val_int('entryusn') + users[0].replace('description', 'update') + try: + users[0].replace('uid', 'invalid update') + except ldap.NOT_ALLOWED_ON_RDN: + pass + users[0].replace('description', 'second update') + entryusn_after = users[0].get_attr_val_int('entryusn') + + # entryUSN should be OLD + 2 (only two user updates) + assert entryusn_after == (entryusn_before + 2) + + check_lastusn_after_restart(inst) + + +def test_entryusn_after_repl_delete(topology_m2): + """Verify that entryUSN is incremented on 1 after delete operation which creates a tombstone + + :id: 1704cf65-41bc-4347-bdaf-20fc2431b218 + :setup: An instance with replication, Users, USN enabled + :steps: + 1. Try to delete a user + 2. Check the tombstone has the incremented USN + 3. Try to delete ou=People with users + 4. Check the entry has a not incremented entryUSN + :expectedresults: + 1. Success + 2. Success + 3. Should fail with Not Allowed On Non-leaf error + 4. Success + """ + + inst = topology_m2.ms["master1"] + plugin = USNPlugin(inst) + plugin.enable() + inst.restart() + users = UserAccounts(inst, DEFAULT_SUFFIX) + + try: + user_1 = users.create_test_user() + user_rdn = user_1.rdn + tombstones = Tombstones(inst, DEFAULT_SUFFIX) + + user_1.replace('description', 'update_ts') + user_usn = user_1.get_attr_val_int('entryusn') + + user_1.delete() + + ts = tombstones.get(user_rdn) + ts_usn = ts.get_attr_val_int('entryusn') + + assert (user_usn + 1) == ts_usn + + user_1 = users.create_test_user() + org = OrganizationalUnit(inst, f"ou=People,{DEFAULT_SUFFIX}") + org.replace('description', 'update_ts') + ou_usn_before = org.get_attr_val_int('entryusn') + try: + org.delete() + except ldap.NOT_ALLOWED_ON_NONLEAF: + pass + ou_usn_after = org.get_attr_val_int('entryusn') + assert ou_usn_before == ou_usn_after + + finally: + try: + user_1.delete() + except ldap.NO_SUCH_OBJECT: + pass + + +if __name__ == '__main__': + # Run isolated + # -s for DEBUG mode + CURRENT_FILE = os.path.realpath(__file__) + pytest.main("-s %s" % CURRENT_FILE) diff --git a/ldap/servers/plugins/usn/usn.c b/ldap/servers/plugins/usn/usn.c index 12ba040c6..f2cc8a62c 100644 --- a/ldap/servers/plugins/usn/usn.c +++ b/ldap/servers/plugins/usn/usn.c @@ -333,6 +333,12 @@ _usn_add_next_usn(Slapi_Entry *e, Slapi_Backend *be) } slapi_ch_free_string(&usn_berval.bv_val); + /* + * increment the counter now and decrement in the bepostop + * if the operation will fail + */ + slapi_counter_increment(be->be_usn_counter); + slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM, "<-- _usn_add_next_usn\n"); @@ -370,6 +376,12 @@ _usn_mod_next_usn(LDAPMod ***mods, Slapi_Backend *be) *mods = slapi_mods_get_ldapmods_passout(&smods); + /* + * increment the counter now and decrement in the bepostop + * if the operation will fail + */ + slapi_counter_increment(be->be_usn_counter); + slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM, "<-- _usn_mod_next_usn\n"); return LDAP_SUCCESS; @@ -420,6 +432,7 @@ usn_betxnpreop_delete(Slapi_PBlock *pb) { Slapi_Entry *e = NULL; Slapi_Backend *be = NULL; + int32_t tombstone_incremented = 0; int rc = SLAPI_PLUGIN_SUCCESS; slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM, @@ -441,7 +454,9 @@ usn_betxnpreop_delete(Slapi_PBlock *pb) goto bail; } _usn_add_next_usn(e, be); + tombstone_incremented = 1; bail: + slapi_pblock_set(pb, SLAPI_USN_INCREMENT_FOR_TOMBSTONE, &tombstone_incremented); slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM, "<-- usn_betxnpreop_delete\n"); @@ -483,7 +498,7 @@ bail: return rc; } -/* count up the counter */ +/* count down the counter */ static int usn_bepostop(Slapi_PBlock *pb) { @@ -493,25 +508,24 @@ usn_bepostop(Slapi_PBlock *pb) slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM, "--> usn_bepostop\n"); - /* if op is not successful, don't increment the counter */ + /* if op is not successful, decrement the counter, else - do nothing */ slapi_pblock_get(pb, SLAPI_RESULT_CODE, &rc); if (LDAP_SUCCESS != rc) { - /* no plugin failure */ - rc = SLAPI_PLUGIN_SUCCESS; - goto bail; - } + slapi_pblock_get(pb, SLAPI_BACKEND, &be); + if (NULL == be) { + rc = LDAP_PARAM_ERROR; + slapi_pblock_set(pb, SLAPI_RESULT_CODE, &rc); + rc = SLAPI_PLUGIN_FAILURE; + goto bail; + } - slapi_pblock_get(pb, SLAPI_BACKEND, &be); - if (NULL == be) { - rc = LDAP_PARAM_ERROR; - slapi_pblock_set(pb, SLAPI_RESULT_CODE, &rc); - rc = SLAPI_PLUGIN_FAILURE; - goto bail; + if (be->be_usn_counter) { + slapi_counter_decrement(be->be_usn_counter); + } } - if (be->be_usn_counter) { - slapi_counter_increment(be->be_usn_counter); - } + /* no plugin failure */ + rc = SLAPI_PLUGIN_SUCCESS; bail: slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM, "<-- usn_bepostop\n"); @@ -519,13 +533,14 @@ bail: return rc; } -/* count up the counter */ +/* count down the counter on a failure and mod ignore */ static int usn_bepostop_modify(Slapi_PBlock *pb) { int rc = SLAPI_PLUGIN_FAILURE; Slapi_Backend *be = NULL; LDAPMod **mods = NULL; + int32_t do_decrement = 0; int i; slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM, @@ -534,9 +549,7 @@ usn_bepostop_modify(Slapi_PBlock *pb) /* if op is not successful, don't increment the counter */ slapi_pblock_get(pb, SLAPI_RESULT_CODE, &rc); if (LDAP_SUCCESS != rc) { - /* no plugin failure */ - rc = SLAPI_PLUGIN_SUCCESS; - goto bail; + do_decrement = 1; } slapi_pblock_get(pb, SLAPI_MODIFY_MODS, &mods); @@ -545,25 +558,29 @@ usn_bepostop_modify(Slapi_PBlock *pb) if (mods[i]->mod_op & LDAP_MOD_IGNORE) { slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM, "usn_bepostop_modify - MOD_IGNORE detected\n"); - goto bail; /* conflict occurred. - skip incrementing the counter. */ + do_decrement = 1; /* conflict occurred. + decrement he counter. */ } else { break; } } } - slapi_pblock_get(pb, SLAPI_BACKEND, &be); - if (NULL == be) { - rc = LDAP_PARAM_ERROR; - slapi_pblock_set(pb, SLAPI_RESULT_CODE, &rc); - rc = SLAPI_PLUGIN_FAILURE; - goto bail; + if (do_decrement) { + slapi_pblock_get(pb, SLAPI_BACKEND, &be); + if (NULL == be) { + rc = LDAP_PARAM_ERROR; + slapi_pblock_set(pb, SLAPI_RESULT_CODE, &rc); + rc = SLAPI_PLUGIN_FAILURE; + goto bail; + } + if (be->be_usn_counter) { + slapi_counter_decrement(be->be_usn_counter); + } } - if (be->be_usn_counter) { - slapi_counter_increment(be->be_usn_counter); - } + /* no plugin failure */ + rc = SLAPI_PLUGIN_SUCCESS; bail: slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM, "<-- usn_bepostop_modify\n"); @@ -573,34 +590,38 @@ bail: /* count up the counter */ /* if the op is delete and the op was not successful, remove preventryusn */ +/* the function is executed on TXN level */ static int usn_bepostop_delete(Slapi_PBlock *pb) { int rc = SLAPI_PLUGIN_FAILURE; Slapi_Backend *be = NULL; + int32_t tombstone_incremented = 0; slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM, "--> usn_bepostop_delete\n"); - /* if op is not successful, don't increment the counter */ + /* if op is not successful and it is a tombstone entry, decrement the counter */ slapi_pblock_get(pb, SLAPI_RESULT_CODE, &rc); if (LDAP_SUCCESS != rc) { - /* no plugin failure */ - rc = SLAPI_PLUGIN_SUCCESS; - goto bail; - } + slapi_pblock_get(pb, SLAPI_USN_INCREMENT_FOR_TOMBSTONE, &tombstone_incremented); + if (tombstone_incremented) { + slapi_pblock_get(pb, SLAPI_BACKEND, &be); + if (NULL == be) { + rc = LDAP_PARAM_ERROR; + slapi_pblock_set(pb, SLAPI_RESULT_CODE, &rc); + rc = SLAPI_PLUGIN_FAILURE; + goto bail; + } - slapi_pblock_get(pb, SLAPI_BACKEND, &be); - if (NULL == be) { - rc = LDAP_PARAM_ERROR; - slapi_pblock_set(pb, SLAPI_RESULT_CODE, &rc); - rc = SLAPI_PLUGIN_FAILURE; - goto bail; + if (be->be_usn_counter) { + slapi_counter_decrement(be->be_usn_counter); + } + } } - if (be->be_usn_counter) { - slapi_counter_increment(be->be_usn_counter); - } + /* no plugin failure */ + rc = SLAPI_PLUGIN_SUCCESS; bail: slapi_log_err(SLAPI_LOG_TRACE, USN_PLUGIN_SUBSYSTEM, "<-- usn_bepostop_delete\n"); diff --git a/ldap/servers/slapd/pblock.c b/ldap/servers/slapd/pblock.c index cb562e938..454ea9cc3 100644 --- a/ldap/servers/slapd/pblock.c +++ b/ldap/servers/slapd/pblock.c @@ -2436,7 +2436,7 @@ slapi_pblock_get(Slapi_PBlock *pblock, int arg, void *value) (*(char **)value) = NULL; } break; - + case SLAPI_SEARCH_CTRLS: if (pblock->pb_intop != NULL) { (*(LDAPControl ***)value) = pblock->pb_intop->pb_search_ctrls; @@ -2479,6 +2479,14 @@ slapi_pblock_get(Slapi_PBlock *pblock, int arg, void *value) } break; + case SLAPI_USN_INCREMENT_FOR_TOMBSTONE: + if (pblock->pb_intop != NULL) { + (*(int32_t *)value) = pblock->pb_intop->pb_usn_tombstone_incremented; + } else { + (*(int32_t *)value) = 0; + } + break; + /* ACI Target Check */ case SLAPI_ACI_TARGET_CHECK: if (pblock->pb_misc != NULL) { @@ -4156,6 +4164,10 @@ slapi_pblock_set(Slapi_PBlock *pblock, int arg, void *value) pblock->pb_intop->pb_paged_results_cookie = *(int *)value; break; + case SLAPI_USN_INCREMENT_FOR_TOMBSTONE: + pblock->pb_intop->pb_usn_tombstone_incremented = *((int32_t *)value); + break; + /* ACI Target Check */ case SLAPI_ACI_TARGET_CHECK: _pblock_assert_pb_misc(pblock); diff --git a/ldap/servers/slapd/pblock_v3.h b/ldap/servers/slapd/pblock_v3.h index 7ec2f37d6..90498c0b0 100644 --- a/ldap/servers/slapd/pblock_v3.h +++ b/ldap/servers/slapd/pblock_v3.h @@ -161,6 +161,7 @@ typedef struct _slapi_pblock_intop int pb_paged_results_index; /* stash SLAPI_PAGED_RESULTS_INDEX */ int pb_paged_results_cookie; /* stash SLAPI_PAGED_RESULTS_COOKIE */ + int32_t pb_usn_tombstone_incremented; /* stash SLAPI_PAGED_RESULTS_COOKIE */ } slapi_pblock_intop; /* Stuff that is rarely used, but still present */ diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h index 04c02cf7c..589830bb4 100644 --- a/ldap/servers/slapd/slapi-plugin.h +++ b/ldap/servers/slapd/slapi-plugin.h @@ -7483,6 +7483,9 @@ typedef enum _slapi_op_note_t { #define SLAPI_PAGED_RESULTS_INDEX 1945 #define SLAPI_PAGED_RESULTS_COOKIE 1949 +/* USN Plugin flag for tombstone entries */ +#define SLAPI_USN_INCREMENT_FOR_TOMBSTONE 1950 + /* ACI Target Check */ #define SLAPI_ACI_TARGET_CHECK 1946 -- 2.26.2