From 62af3beb9b2a3137a76456f534db7be1b172210c Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Mat=C3=BA=C5=A1=20Hon=C4=9Bk?= <mhonek@redhat.com>

Date: Wed, 16 Jan 2019 09:49:28 +0100

Subject: [PATCH] Issue 50474 - Unify result codes for add and modify of repl5

 config

Bug Description:

Same constraints resulting in error are reported as different LDAP

result codes when using different operation for adjusting these.

Fix Description:

A part of the code had not conveyed the error reason down the stack,

therefore adding this information and returning the proper code.

Fixes: https://pagure.io/389-ds-base/issue/50474

Author: Matus Honek <mhonek@redhat.com>

Review by: mreynolds, spichugi (thanks!)

---

 .../suites/replication/replica_config_test.py | 25 +++++--

 ldap/servers/plugins/replication/repl5.h      |  2 +-

 .../plugins/replication/repl5_replica.c       | 71 +++++++++++--------

 .../replication/repl5_replica_config.c        |  3 +-

 4 files changed, 63 insertions(+), 38 deletions(-)

diff --git a/dirsrvtests/tests/suites/replication/replica_config_test.py b/dirsrvtests/tests/suites/replication/replica_config_test.py

index 9a0e1b41f..3dc03713a 100644

--- a/dirsrvtests/tests/suites/replication/replica_config_test.py

+++ b/dirsrvtests/tests/suites/replication/replica_config_test.py

@@ -4,7 +4,6 @@ import copy

 import os

 import ldap

 from lib389._constants import *

-from lib389 import Entry

 from lib389.topologies import topology_st as topo

 from lib389.replica import Replicas

@@ -104,12 +103,14 @@ def agmt_setup(topo):

 def perform_invalid_create(many, properties, attr, value):

     my_properties = copy.deepcopy(properties)

     my_properties[attr] = value

-    with pytest.raises(ldap.LDAPError):

+    with pytest.raises(ldap.LDAPError) as ei:

         many.create(properties=my_properties)

+    return ei.value

 def perform_invalid_modify(o, attr, value):

-    with pytest.raises(ldap.LDAPError):

+    with pytest.raises(ldap.LDAPError) as ei:

         o.replace(attr, value)

+    return ei.value

 @pytest.mark.parametrize("attr, too_small, too_big, overflow, notnum, valid", repl_add_attrs)

 def test_replica_num_add(topo, attr, too_small, too_big, overflow, notnum, valid):

@@ -254,9 +255,25 @@ def test_agmt_num_modify(topo, attr, too_small, too_big, overflow, notnum, valid

     # Value is valid

     agmt.replace(attr, valid)

+

+@pytest.mark.bz1546739

+def test_same_attr_yields_same_return_code(topo):

+    """Test that various operations with same incorrect attribute value yield same return code

+    """

+    attr = 'nsDS5ReplicaId'

+

+    replica_reset(topo)

+    replicas = Replicas(topo.standalone)

+    e = perform_invalid_create(replicas, replica_dict, attr, too_big)

+    assert type(e) is ldap.UNWILLING_TO_PERFORM

+

+    replica = replica_setup(topo)

+    e = perform_invalid_modify(replica, attr, too_big)

+    assert type(e) is ldap.UNWILLING_TO_PERFORM

+

+

 if __name__ == '__main__':

     # Run isolated

     # -s for DEBUG mode

     CURRENT_FILE = os.path.realpath(__file__)

     pytest.main(["-s", CURRENT_FILE])

-

diff --git a/ldap/servers/plugins/replication/repl5.h b/ldap/servers/plugins/replication/repl5.h

index 138578d5f..1801a333e 100644

--- a/ldap/servers/plugins/replication/repl5.h

+++ b/ldap/servers/plugins/replication/repl5.h

@@ -662,7 +662,7 @@ Replica *replica_new(const Slapi_DN *root);

 Replica *windows_replica_new(const Slapi_DN *root);

 /* this function should be called to construct the replica object

    during addition of the replica over LDAP */

-Replica *replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation);

+int replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation, Replica **r);

 void replica_destroy(void **arg);

 int replica_subentry_update(Slapi_DN *repl_root, ReplicaId rid);

 int replica_subentry_check(Slapi_DN *repl_root, ReplicaId rid);

diff --git a/ldap/servers/plugins/replication/repl5_replica.c b/ldap/servers/plugins/replication/repl5_replica.c

index 6a5363e43..b3f03d5c0 100644

--- a/ldap/servers/plugins/replication/repl5_replica.c

+++ b/ldap/servers/plugins/replication/repl5_replica.c

@@ -128,8 +128,9 @@ replica_new(const Slapi_DN *root)

     e = _replica_get_config_entry(root, NULL);

     if (e) {

         errorbuf[0] = '\0';

-        r = replica_new_from_entry(e, errorbuf,

-                                   PR_FALSE /* not a newly added entry */);

+        replica_new_from_entry(e, errorbuf,

+                               PR_FALSE, /* not a newly added entry */

+                               &r);

         if (NULL == r) {

             slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "replica_new - "

@@ -143,17 +144,17 @@ replica_new(const Slapi_DN *root)

 }

 /* constructs the replica object from the newly added entry */

-Replica *

-replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation)

+int

+replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation, Replica **rp)

 {

-    int rc = 0;

     Replica *r;

+    int rc = LDAP_SUCCESS;

     if (e == NULL) {

         if (NULL != errortext) {

             PR_snprintf(errortext, SLAPI_DSE_RETURNTEXT_SIZE, "NULL entry");

         }

-        return NULL;

+        return LDAP_OTHER;

     }

     r = (Replica *)slapi_ch_calloc(1, sizeof(Replica));

@@ -162,7 +163,7 @@ replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation)

         if (NULL != errortext) {

             PR_snprintf(errortext, SLAPI_DSE_RETURNTEXT_SIZE, "Out of memory");

         }

-        rc = -1;

+        rc = LDAP_OTHER;

         goto done;

     }

@@ -170,7 +171,7 @@ replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation)

         if (NULL != errortext) {

             PR_snprintf(errortext, SLAPI_DSE_RETURNTEXT_SIZE, "failed to create replica lock");

         }

-        rc = -1;

+        rc = LDAP_OTHER;

         goto done;

     }

@@ -178,7 +179,7 @@ replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation)

         if (NULL != errortext) {

             PR_snprintf(errortext, SLAPI_DSE_RETURNTEXT_SIZE, "failed to create replica lock");

         }

-        rc = -1;

+        rc = LDAP_OTHER;

         goto done;

     }

@@ -191,14 +192,17 @@ replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation)

     /* read parameters from the replica config entry */

     rc = _replica_init_from_config(r, e, errortext);

-    if (rc != 0) {

+    if (rc != LDAP_SUCCESS) {

         goto done;

     }

     /* configure ruv */

     rc = _replica_configure_ruv(r, PR_FALSE);

     if (rc != 0) {

+        rc = LDAP_OTHER;

         goto done;

+    } else {

+        rc = LDAP_SUCCESS;

     }

     /* If smallest csn exists in RUV for our local replica, it's ok to begin iteration */

@@ -217,8 +221,12 @@ replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation)

          * (done by the update state event scheduled below)

          */

     }

-    if (rc != 0)

+    if (rc != 0) {

+        rc = LDAP_OTHER;

         goto done;

+    } else {

+        rc = LDAP_SUCCESS;

+    }

     /* ONREPL - the state update can occur before the entry is added to the DIT.

        In that case the updated would fail but nothing bad would happen. The next

@@ -237,11 +245,12 @@ replica_new_from_entry(Slapi_Entry *e, char *errortext, PRBool is_add_operation)

     }

 done:

-    if (rc != 0 && r) {

+    if (rc != LDAP_SUCCESS && r) {

         replica_destroy((void **)&r);

     }

-    return r;

+    *rp = r;

+    return rc;

 }

@@ -1789,9 +1798,9 @@ _replica_check_validity(const Replica *r)

     if (r->repl_root == NULL || r->repl_type == 0 || r->repl_rid == 0 ||

         r->repl_csngen == NULL || r->repl_name == NULL) {

-        return -1;

+        return LDAP_OTHER;

     } else {

-        return 0;

+        return LDAP_SUCCESS;

     }

 }

@@ -1841,7 +1850,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext)

                     (char *)slapi_entry_get_dn((Slapi_Entry *)e));

         slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "_replica_init_from_config - %s\n",

                       errormsg);

-        return -1;

+        return LDAP_OTHER;

     }

     r->repl_root = slapi_sdn_new_dn_passin(val);

@@ -1851,7 +1860,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext)

         if ((val = slapi_entry_attr_get_charptr(e, attr_replicaType))) {

             if (repl_config_valid_num(attr_replicaType, val, 0, REPLICA_TYPE_UPDATABLE, &rc, errormsg, &rtype) != 0) {

                 slapi_ch_free_string(&val;;

-                return -1;

+                return LDAP_UNWILLING_TO_PERFORM;

             }

             r->repl_type = rtype;

             slapi_ch_free_string(&val;;

@@ -1867,7 +1876,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext)

         if ((val = slapi_entry_attr_get_charptr(e, type_replicaBackoffMin))) {

             if (repl_config_valid_num(type_replicaBackoffMin, val, 1, INT_MAX, &rc, errormsg, &backoff_min) != 0) {

                 slapi_ch_free_string(&val;;

-                return -1;

+                return LDAP_UNWILLING_TO_PERFORM;

             }

             slapi_ch_free_string(&val;;

         } else {

@@ -1882,7 +1891,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext)

         if ((val = slapi_entry_attr_get_charptr(e, type_replicaBackoffMax))) {

             if (repl_config_valid_num(type_replicaBackoffMax, val, 1, INT_MAX, &rc, errormsg, &backoff_max) != 0) {

                 slapi_ch_free_string(&val;;

-                return -1;

+                return LDAP_UNWILLING_TO_PERFORM;

             }

             slapi_ch_free_string(&val;;

         } else {

@@ -1899,7 +1908,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext)

                     backoff_min, backoff_max);

         slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "_replica_init_from_config - "

                       "%s\n", errormsg);

-        return -1;

+        return LDAP_UNWILLING_TO_PERFORM;

     } else {

         slapi_counter_set_value(r->backoff_min, backoff_min);

         slapi_counter_set_value(r->backoff_max, backoff_max);

@@ -1910,7 +1919,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext)

         if ((val = slapi_entry_attr_get_charptr(e, type_replicaProtocolTimeout))) {

             if (repl_config_valid_num(type_replicaProtocolTimeout, val, 0, INT_MAX, &rc, errormsg, &ptimeout) != 0) {

                 slapi_ch_free_string(&val;;

-                return -1;

+                return LDAP_UNWILLING_TO_PERFORM;

             }

             slapi_ch_free_string(&val;;

             slapi_counter_set_value(r->protocol_timeout, ptimeout);

@@ -1926,7 +1935,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext)

         if ((val = slapi_entry_attr_get_charptr(e, type_replicaReleaseTimeout))) {

             if (repl_config_valid_num(type_replicaReleaseTimeout, val, 0, INT_MAX, &rc, errortext, &release_timeout) != 0) {

                 slapi_ch_free_string(&val;;

-                return -1;

+                return LDAP_UNWILLING_TO_PERFORM;

             }

             slapi_counter_set_value(r->release_timeout, release_timeout);

             slapi_ch_free_string(&val;;

@@ -1950,7 +1959,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext)

                         type_replicaPrecisePurge, precise_purging);

             slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "_replica_init_from_config - "

                           "%s\n", errormsg);

-            return -1;

+            return LDAP_UNWILLING_TO_PERFORM;

         }

         slapi_ch_free_string(&precise_purging);

     } else {

@@ -1963,7 +1972,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext)

         if((val = slapi_entry_attr_get_charptr(e, attr_flags))) {

             if (repl_config_valid_num(attr_flags, val, 0, 1, &rc, errortext, &rflags) != 0) {

                 slapi_ch_free_string(&val;;

-                return -1;

+                return LDAP_UNWILLING_TO_PERFORM;

             }

             r->repl_flags = (uint32_t)rflags;

             slapi_ch_free_string(&val;;

@@ -1990,7 +1999,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext)

             int64_t rid;

             if (repl_config_valid_num(attr_replicaId, val, 1, 65534, &rc, errormsg, &rid) != 0) {

                 slapi_ch_free_string(&val;;

-                return -1;

+                return LDAP_UNWILLING_TO_PERFORM;

             }

             r->repl_rid = (ReplicaId)rid;

             slapi_ch_free_string(&val;;

@@ -2000,7 +2009,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext)

                         attr_replicaId, (char *)slapi_entry_get_dn((Slapi_Entry *)e));

             slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name,

                           "_replica_init_from_config - %s\n", errormsg);

-            return -1;

+            return LDAP_OTHER;

         }

     }

@@ -2013,7 +2022,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext)

                     (char *)slapi_entry_get_dn((Slapi_Entry *)e));

         slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name,

                       "_replica_init_from_config - %s\n", errormsg);

-        return -1;

+        return LDAP_OTHER;

     }

     r->repl_csngen = object_new((void *)gen, (FNFree)csngen_free);

@@ -2031,7 +2040,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext)

     if ((val = slapi_entry_attr_get_charptr(e, attr_replicaBindDnGroupCheckInterval))) {

         if (repl_config_valid_num(attr_replicaBindDnGroupCheckInterval, val, -1, INT_MAX, &rc, errormsg, &interval) != 0) {

             slapi_ch_free_string(&val;;

-            return -1;

+            return LDAP_UNWILLING_TO_PERFORM;

         }

         r->updatedn_group_check_interval = interval;

         slapi_ch_free_string(&val;;

@@ -2051,7 +2060,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext)

                         (char *)slapi_entry_get_dn((Slapi_Entry *)e), rc);

             slapi_log_err(SLAPI_LOG_ERR, repl_plugin_name, "_replica_init_from_config - %s\n",

                           errormsg);

-            return -1;

+            return LDAP_OTHER;

         } else

             r->new_name = PR_TRUE;

     }

@@ -2072,7 +2081,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext)

     if ((val = slapi_entry_attr_get_charptr(e, type_replicaPurgeDelay))) {

         if (repl_config_valid_num(type_replicaPurgeDelay, val, -1, INT_MAX, &rc, errormsg, &interval) != 0) {

             slapi_ch_free_string(&val;;

-            return -1;

+            return LDAP_UNWILLING_TO_PERFORM;

         }

         r->repl_purge_delay = interval;

         slapi_ch_free_string(&val;;

@@ -2083,7 +2092,7 @@ _replica_init_from_config(Replica *r, Slapi_Entry *e, char *errortext)

     if ((val = slapi_entry_attr_get_charptr(e, type_replicaTombstonePurgeInterval))) {

         if (repl_config_valid_num(type_replicaTombstonePurgeInterval, val, -1, INT_MAX, &rc, errormsg, &interval) != 0) {

             slapi_ch_free_string(&val;;

-            return -1;

+            return LDAP_UNWILLING_TO_PERFORM;

         }

         r->tombstone_reap_interval = interval;

         slapi_ch_free_string(&val;;

diff --git a/ldap/servers/plugins/replication/repl5_replica_config.c b/ldap/servers/plugins/replication/repl5_replica_config.c

index 7649aa14e..749e90936 100644

--- a/ldap/servers/plugins/replication/repl5_replica_config.c

+++ b/ldap/servers/plugins/replication/repl5_replica_config.c

@@ -267,9 +267,8 @@ replica_config_add(Slapi_PBlock *pb __attribute__((unused)),

     }

     /* create replica object */

-    r = replica_new_from_entry(e, errortext, PR_TRUE /* is a newly added entry */);

+    *returncode = replica_new_from_entry(e, errortext, PR_TRUE /* is a newly added entry */, &r);

     if (r == NULL) {

-        *returncode = LDAP_OPERATIONS_ERROR;

         goto done;

     }

-- 

2.21.0