From 220dbafa048269105b3f7958a5d5bfd1d988da26 Mon Sep 17 00:00:00 2001
From: Simon Pichugin <spichugi@redhat.com>
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