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