[389-ds-base] branch 389-ds-base-1.4.2 updated: Issue 49300 - entryUSN is duplicated after memberOf operation

This is an automated email from the git hooks/post-receive script.

spichugi pushed a commit to branch 389-ds-base-1.4.2
in repository 389-ds-base.

The following commit(s) were added to refs/heads/389-ds-base-1.4.2 by this push:
     new 14b0dcb  Issue 49300 - entryUSN is duplicated after memberOf operation
14b0dcb is described below

commit 14b0dcbaf86b99031b368893566e60ab19e156c7
Author: Simon Pichugin <spichugi(a)redhat.com&gt;
AuthorDate: Tue Jun 30 15:39:30 2020 +0200

    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!)
---
 dirsrvtests/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(-)

diff --git a/dirsrvtests/tests/suites/plugins/entryusn_test.py
b/dirsrvtests/tests/suites/plugins/entryusn_test.py
new file mode 100644
index 0000000..7213154
--- /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
+
+
+(a)pytest.fixture(scope=&quot;module&quot;)
+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 12ba040..f2cc8a6 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 cb562e9..454ea9c 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 7ec2f37..90498c0 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 b78395d..a71050f 100644
--- a/ldap/servers/slapd/slapi-plugin.h
+++ b/ldap/servers/slapd/slapi-plugin.h
@@ -7471,6 +7471,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
 

-- 
To stop receiving notification emails like this one, please contact
the administrator of this repository.