Blob Blame Raw
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