From d9784b09531b19f6541602a31cfd49c9878ef2ca Mon Sep 17 00:00:00 2001
From: Simon Pichugin <spichugi@redhat.com>
Date: Thu, 31 Aug 2023 11:19:05 -0700
Subject: [PATCH] Issue 5848 - Fix condition and add a CI test (#5916)

Description: Add a "positive" test for the issue and fix the condition
to make sure that 65535 and no --replica-id are correctly accepted.

Related: https://github.com/389ds/389-ds-base/issues/5848

Reviewed by: @mreynolds389 @tbordaz (Thanks!)
---
 dirsrvtests/tests/suites/clu/dsconf_test.py | 34 ++++++++++++++++++++-
 src/lib389/lib389/cli_conf/replication.py   | 23 ++++++++------
 2 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/dirsrvtests/tests/suites/clu/dsconf_test.py b/dirsrvtests/tests/suites/clu/dsconf_test.py
index eb3c426c7..4f7da0b58 100644
--- a/dirsrvtests/tests/suites/clu/dsconf_test.py
+++ b/dirsrvtests/tests/suites/clu/dsconf_test.py
@@ -99,7 +99,7 @@ def test_dsconf_with_ldaps(topology_st, enable_config, config_type):
 
 
 @pytest.mark.parametrize('instance_role', ('consumer', 'hub'))
-def test_check_replica_id_rejected (instance_role):
+def test_check_replica_id_rejected_hub_consumer(instance_role):
     """Test dsconf CLI does not accept replica-id parameter for comsumer and hubs
 
     :id: 274b47f8-111a-11ee-8321-98fa9ba19b65
@@ -129,3 +129,35 @@ def test_check_replica_id_rejected (instance_role):
     log.info(f'output message : {msg}')
     assert "Replication successfully enabled for" not in msg, f"Test Failed: --replica-id option is accepted....It shouldn't for {instance_role}"
     log.info(f"Test PASSED: --replica-id option is NOT accepted for {instance_role}.")
+
+
+@pytest.mark.parametrize('instance_role, replica_id',
+                         [('consumer', None), ('hub', None), ('consumer', "65535"), ('hub', "65535")])
+def test_check_replica_id_accepted_hub_consumer(topology_st, instance_role, replica_id):
+    """Test dsconf CLI accepts 65535 replica-id parameter for comsumer and hubs
+
+    :id: e0a1a1e6-11c1-40e6-92fe-cb550fb2170d
+    :parametrized: yes
+    :customerscenario: True
+    :setup: Create DS instance
+    :steps:
+        1. Create ldap instance
+        2. Use dsconf cli to create replica and don't specify replica id for a consumer or hub
+        3. Use dsconf cli to create replica and specify replica id for a consumer or hub
+    :expectedresults:
+        1. Success
+        2. Success
+        3. Success
+    """
+    print("DN_DM {}".format(DN_DM))
+    cmdline = ['/usr/sbin/dsconf', 'standalone1', '-D', DN_DM, '-w', 'password', 'replication', 'enable', '--suffix', DEFAULT_SUFFIX, '--role', instance_role]
+    if replica_id is not None:
+        cmdline.append(f'--replica-id={replica_id}')
+    log.info(f'Command used : {cmdline}')
+    proc = subprocess.Popen(cmdline, stdout=subprocess.PIPE)
+
+    msg = proc.communicate()
+    msg = msg[0].decode('utf-8')
+    log.info(f'output message : {msg}')
+    assert "Replication successfully enabled for" in msg
+    log.info(f"Test PASSED: --replica-id option is accepted for {instance_role}.")
diff --git a/src/lib389/lib389/cli_conf/replication.py b/src/lib389/lib389/cli_conf/replication.py
index a75774ca0..2e2803ced 100644
--- a/src/lib389/lib389/cli_conf/replication.py
+++ b/src/lib389/lib389/cli_conf/replication.py
@@ -154,6 +154,17 @@ def enable_replication(inst, basedn, log, args):
         # error - unknown type
         raise ValueError(f"Unknown replication role ({role}), you must use \"supplier\", \"hub\", or \"consumer\"")
 
+    if args.replica_id is not None:
+        # is it a number?
+        try:
+            rid_num = int(rid)
+        except ValueError:
+            raise ValueError("--replica-id expects a number between 1 and 65535")
+
+        # Is it in range?
+        if rid_num < 1 or rid_num > 65535:
+            raise ValueError("--replica-id expects a number between 1 and 65535")
+
     # Start the propeties and update them as needed
     repl_properties = {
         'cn': 'replica',
@@ -170,15 +181,9 @@ def enable_replication(inst, basedn, log, args):
             # Error, supplier needs a rid TODO
             raise ValueError('You must specify the replica ID (--replica-id) when enabling a \"supplier\" replica')
 
-        # is it a number?
-        try:
-            rid_num = int(rid)
-        except ValueError:
-            raise ValueError("--replica-id expects a number between 1 and 65534")
-
         # Is it in range?
         if rid_num < 1 or rid_num > 65534:
-            raise ValueError("--replica-id expects a number between 1 and 65534")
+            raise ValueError("--replica-id expects a number between 1 and 65534 for supplier role")
 
         # rid is good add it to the props
         repl_properties['nsDS5ReplicaId'] = args.replica_id
@@ -186,9 +191,9 @@ def enable_replication(inst, basedn, log, args):
     # Validate consumer and hub settings
     elif role == "consumer" or role == "hub":
         # Check Replica ID
-        if args.replica_id is not None or args.replica_id != 65535:
+        if args.replica_id is not None and rid_num != 65535:
             # Error, Replica ID cannot be specified for consumer and hub roles
-            raise ValueError('Replica ID cannot be specified for consumer and hub roles')
+            raise ValueError('Replica ID other than 65535 cannot be specified for consumer and hub roles')
 
     # Bind DN or Bind DN Group?
     if args.bind_group_dn:
-- 
2.41.0