[5280] Changes after review:

 - added missing comments for couple handlers
 - duplicated leaseX-update check removed, unit-test added
 - added missing parameters in lease_mgr_unitest.cc
 - added sanity check for state when adding and updating leases
 - not possibly anymore to sneak v4 addresses when v6 is expected
   (and vice versa)

src/hooks/dhcp/lease_cmds/lease_cmds.cc

@@ -303,16 +303,88 @@ private:
     static ConstElementPtr
     lease6DelHandler(const string& command, ConstElementPtr args);
 
+    /// @brief lease4-update handler
+    ///
+    /// This command attempts to update existing IPv4 lease. The parameters
+    /// specified will replace existing lease. The only condition is that
+    /// the IP address must not change. If you want to change the IP address,
+    /// please use lease4-del and lease4-add instead.
+    ///
+    /// Example command:
+    /// {
+    ///     "command": "lease4-update",
+    ///     "arguments": {
+    ///         "subnet-id": 44,
+    ///         "ip-address": "192.0.2.1",
+    ///         "hw-address": "1a:1b:1c:1d:1e:1f",
+    ///         "hostname": "newhostname.example.org"
+    ///     }
+    /// };
+    ///
+    /// @param command - should be "lease4-update", but it is ignored
+    /// @param args arguments that describe the lease being updated.
     static ConstElementPtr
     lease4UpdateHandler(const string& command, ConstElementPtr args);
 
-    /// @brief Not implemented yet.
+    /// @brief lease6-update handler
+    ///
+    /// This command attempts to update existing IPv6 lease. The parameters
+    /// specified will replace existing lease. The only condition is that
+    /// the IP address must not change. If you want to change the IP address,
+    /// please use lease6-del and lease6-add instead.
+    ///
+    /// Example command:
+    /// {
+    ///     "command": "lease6-update",
+    ///     "arguments": {
+    ///         "subnet-id": 66,
+    ///         "ip-address": "2001:db8::1",
+    ///         "iaid": 7654321,
+    ///         "duid": "88:88:88:88:88:88:88:88",
+    ///         "hostname": "newhostname.example.org"
+    ///     }
+    /// }";
+    ///
+    /// @param command - should be "lease6-update" (but it is ignored)
+    /// @param args arguments that describe the lease being updated.
     static ConstElementPtr
     lease6UpdateHandler(const string& command, ConstElementPtr args);
 
+    /// @brief lease4-wipe handler
+    ///
+    /// This commands attempts to remove all IPv4 leases from a specific
+    /// subnet. Currently the leases are removed from the database,
+    /// without any processing (like calling hooks or doing DDNS
+    /// cleanups).
+    ///
+    /// Example command:
+    /// {\n"
+    ///     "command": "lease4-wipe",\n"
+    ///     "arguments": {"
+    ///         "subnet-id": 44
+    ///     }\n"
+    /// }";
+    /// @param command - should be "lease4-wipe" (but is ignored)
+    /// @param args arguments that describe the lease being updated.
     static ConstElementPtr
     lease4WipeHandler(const string& command, ConstElementPtr args);
 
+    /// @brief lease6-wipe handler
+    ///
+    /// This commands attempts to remove all IPv4 leases from a specific
+    /// subnet. Currently the leases are removed from the database,
+    /// without any processing (like calling hooks or doing DDNS
+    /// cleanups).
+    ///
+    /// Example command:
+    /// {\n"
+    ///     "command": "lease4-wipe",\n"
+    ///     "arguments": {"
+    ///         "subnet-id": 44
+    ///     }\n"
+    /// }";
+    /// @param command - should be "lease4-wipe" (but is ignored)
+    /// @param args arguments that describe the lease being updated.
     static ConstElementPtr
     lease6WipeHandler(const string& command, ConstElementPtr args);
 
@@ -321,10 +393,11 @@ private:
     /// See @ref Parameters class for detailed description of what is expected
     /// in the args structure.
     ///
-    /// @param args - arguments passed to command
+    /// @param v6 whether addresses allowed are v4 (false) or v6 (true)
+    /// @param args arguments passed to command
     /// @return parsed parameters
     /// @throw BadValue if input arguments don't make sense.
-    static Parameters getParameters(const ConstElementPtr& args);
+    static Parameters getParameters(bool v6, const ConstElementPtr& args);
 };
 
 LeaseCmdsImpl::LeaseCmdsImpl() {
@@ -435,7 +508,7 @@ LeaseCmdsImpl::leaseAddHandler(const std::string& name,
 }
 
 LeaseCmdsImpl::Parameters
-LeaseCmdsImpl::getParameters(const ConstElementPtr& params) {
+LeaseCmdsImpl::getParameters(bool v6, const ConstElementPtr& params) {
     Parameters x;
 
     if (!params || params->getType() != Element::map) {
@@ -470,6 +543,14 @@ LeaseCmdsImpl::getParameters(const ConstElementPtr& params) {
         }
 
         x.addr = IOAddress(tmp->stringValue());
+
+        if ((v6 && !x.addr.isV6()) || (!v6 && !x.addr.isV4())) {
+            stringstream txt;
+            txt << "Invalid " << (v6 ? "IPv6" : "IPv4")
+                << " address specified: " << tmp->stringValue();
+            isc_throw(BadValue, txt.str());
+        }
+
         x.query_type = Parameters::TYPE_ADDR;
         return (x);
     }
@@ -537,7 +618,7 @@ LeaseCmdsImpl::leaseGetHandler(const std::string& name, ConstElementPtr params)
     Lease6Ptr lease6;
     bool v4 = (name == "lease4-get");
     try {
-        p = getParameters(params);
+        p = getParameters(!v4, params);
 
         switch (p.query_type) {
         case Parameters::TYPE_ADDR: {
@@ -607,7 +688,7 @@ LeaseCmdsImpl::lease4DelHandler(const std::string& , ConstElementPtr params) {
     Lease4Ptr lease4;
     IOAddress addr(IOAddress::IPV4_ZERO_ADDRESS());
     try {
-        p = getParameters(params);
+        p = getParameters(false, params);
 
         switch (p.query_type) {
         case Parameters::TYPE_ADDR: {
@@ -661,7 +742,7 @@ LeaseCmdsImpl::lease6DelHandler(const std::string& , ConstElementPtr params) {
     Lease6Ptr lease6;
     IOAddress addr(IOAddress::IPV6_ZERO_ADDRESS());
     try {
-        p = getParameters(params);
+        p = getParameters(true, params);
 
         switch (p.query_type) {
         case Parameters::TYPE_ADDR: {
@@ -726,14 +807,6 @@ LeaseCmdsImpl::lease4UpdateHandler(const string& , ConstElementPtr params) {
         // subnet-id is valid, etc)
         lease4 = parser.parse(config, params);
 
-        // Ok, now check if there is a lease to be updated.
-        Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(lease4->addr_);
-        if (!existing) {
-            stringstream tmp;
-            tmp << "There is no lease for address " << lease4->addr_ << ", can't update.";
-            return (createAnswer(CONTROL_RESULT_EMPTY, tmp.str()));
-        }
-
         LeaseMgrFactory::instance().updateLease4(lease4);
         return (createAnswer(CONTROL_RESULT_SUCCESS, "IPv4 lease updated."));
 
@@ -758,15 +831,6 @@ LeaseCmdsImpl::lease6UpdateHandler(const string& , ConstElementPtr params) {
         // subnet-id is valid, etc)
         lease6 = parser.parse(config, params);
 
-        // Ok, now check if there is a lease to be updated.
-        Lease6Ptr existing = LeaseMgrFactory::instance().getLease6(lease6->type_,
-                                                                   lease6->addr_);
-        if (!existing) {
-            stringstream tmp;
-            tmp << "There is no lease for address " << lease6->addr_ << ", can't update.";
-            return (createAnswer(CONTROL_RESULT_EMPTY, tmp.str()));
-        }
-
         LeaseMgrFactory::instance().updateLease6(lease6);
         return (createAnswer(CONTROL_RESULT_SUCCESS, "IPv6 lease updated."));
 
@@ -779,11 +843,10 @@ ConstElementPtr
 LeaseCmdsImpl::lease4WipeHandler(const string& /*cmd*/, ConstElementPtr params) {
     try {
 
-        // We need the lease to be specified.
+        // The subnet-id is a mandatory parameter.
         if (!params) {
             isc_throw(isc::BadValue, "no parameters specified for lease4-wipe command");
         }
-
         SimpleParser parser;
         SubnetID id = parser.getUint32(params, "subnet-id");
 
@@ -802,11 +865,10 @@ ConstElementPtr
 LeaseCmdsImpl::lease6WipeHandler(const string& /*cmd*/, ConstElementPtr params) {
     try {
 
-        // We need the lease to be specified.
+        // The subnet-id is a mandatory parameter.
         if (!params) {
             isc_throw(isc::BadValue, "no parameters specified for lease6-wipe command");
         }
-
         SimpleParser parser;
         SubnetID id = parser.getUint32(params, "subnet-id");
 

src/hooks/dhcp/lease_cmds/lease_parser.cc

@@ -103,6 +103,12 @@ Lease4Parser::parse(ConstSrvConfigPtr& cfg,
         state = getUint8(lease_info, "state");
     }
 
+    // Check if the state value is sane.
+    if (state > Lease::STATE_EXPIRED_RECLAIMED) {
+        isc_throw(BadValue, "Invalid state value: " << state << ", supported "
+                  "values are: 0 (default), 1 (declined) and 2 (expired-reclaimed)");
+    }
+
     // Let's fabricate some data and we're ready to go.
     uint32_t t1 = subnet->getT1();
     uint32_t t2 = subnet->getT2();
@@ -229,6 +235,12 @@ Lease6Parser::parse(ConstSrvConfigPtr& cfg,
         state = getUint8(lease_info, "state");
     }
 
+    // Check if the state value is sane.
+    if (state > Lease::STATE_EXPIRED_RECLAIMED) {
+        isc_throw(BadValue, "Invalid state value: " << state << ", supported "
+                  "values are: 0 (default), 1 (declined) and 2 (expired-reclaimed)");
+    }
+
     // Let's fabricate some data and we're ready to go.
     uint32_t t1 = subnet->getT1();
     uint32_t t2 = subnet->getT2();

src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc

@@ -543,6 +543,21 @@ TEST_F(LeaseCmdsTest, Lease4AddBadParams) {
         "}";
     exp_rsp = "Non-IPv4 address specified: 2001:db8::1";
     testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp);
+
+    // currently defined states are 0,1 and 2. 123 is junk.
+    txt =
+        "{\n"
+        "    \"command\": \"lease4-add\",\n"
+        "    \"arguments\": {"
+        "        \"subnet-id\": 44,\n"
+        "        \"ip-address\": \"192.0.2.1\",\n"
+        "        \"hw-address\": \"1a:1b:1c:1d:1e:1f\",\n"
+        "        \"state\": 123\n"
+        "    }\n"
+        "}";
+    exp_rsp = "Invalid state value: 123, supported values are: 0 (default), 1 "
+        "(declined) and 2 (expired-reclaimed)";
+    testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp);
 }
 
 // Check that a simple, well formed lease4 can be added.
@@ -761,6 +776,22 @@ TEST_F(LeaseCmdsTest, Lease6AddBadParams) {
         "}";
     exp_rsp = "Non-IPv6 address specified: 192.0.2.1";
     testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp);
+
+    // Invalid state: the only supported values are 0,1,2.
+    txt =
+        "{\n"
+        "    \"command\": \"lease6-add\",\n"
+        "    \"arguments\": {"
+        "        \"subnet-id\": 66,\n"
+        "        \"ip-address\": \"2001:db8::1\",\n"
+        "        \"duid\": \"1a:1b:1c:1d:1e:1f\",\n"
+        "        \"iaid\": 1234\n,"
+        "        \"state\": 123\n"
+        "    }\n"
+        "}";
+    exp_rsp = "Invalid state value: 123, supported values are: 0 (default), 1 "
+        "(declined) and 2 (expired-reclaimed)";
+    testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp);
 }
 
 // Check that a simple, well formed lease6 can be added.
@@ -944,6 +975,35 @@ TEST_F(LeaseCmdsTest, Lease4GetMissingParams) {
     testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp);
 }
 
+// Checks that lease4-get sanitizes its input.
+TEST_F(LeaseCmdsTest, Lease4GetByAddrBadParam) {
+
+    // Initialize lease manager (false = v4, true = add a lease)
+    initLeaseMgr(false, true);
+
+    // Invalid family
+    string cmd =
+        "{\n"
+        "    \"command\": \"lease4-get\",\n"
+        "    \"arguments\": {"
+        "        \"ip-address\": \"2001:db8::1\""
+        "    }\n"
+        "}";
+    string exp_rsp = "Invalid IPv4 address specified: 2001:db8::1";
+    testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp);
+
+    // This is way off
+    cmd =
+        "{\n"
+        "    \"command\": \"lease4-get\",\n"
+        "    \"arguments\": {"
+        "        \"ip-address\": \"221B Baker St.\""
+        "    }\n"
+        "}";
+    exp_rsp = "Failed to convert string to address '221B Baker St.': Invalid argument";
+    testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp);
+}
+
 // Checks that lease4-get can handle a situation when the query is
 // valid, but the lease is not there.
 TEST_F(LeaseCmdsTest, Lease4GetByAddrNotFound) {
@@ -1112,6 +1172,35 @@ TEST_F(LeaseCmdsTest, Lease6GetByAddr) {
     checkLease6(lease, "2001:db8::1", 0, 66, "77:77:77:77:77:77:77:77", false);
 }
 
+// Checks that lease6-get sanitizes its input.
+TEST_F(LeaseCmdsTest, Lease6GetByAddrBadParam) {
+
+    // Initialize lease manager (true = v6, true = add a lease)
+    initLeaseMgr(true, true);
+
+    // Invalid family
+    string cmd =
+        "{\n"
+        "    \"command\": \"lease6-get\",\n"
+        "    \"arguments\": {"
+        "        \"ip-address\": \"192.0.2.1\""
+        "    }\n"
+        "}";
+    string exp_rsp = "Invalid IPv6 address specified: 192.0.2.1";
+    testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp);
+
+    // This is way off
+    cmd =
+        "{\n"
+        "    \"command\": \"lease6-get\",\n"
+        "    \"arguments\": {"
+        "        \"ip-address\": \"221B Baker St.\""
+        "    }\n"
+        "}";
+    exp_rsp = "Failed to convert string to address '221B Baker St.': Invalid argument";
+    testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp);
+}
+
 // Checks that lease6-get(subnet-id, type, addr6) can handle a situation when
 // the query is correctly formed and the lease is returned.
 TEST_F(LeaseCmdsTest, Lease6GetByAddrPrefix) {
@@ -1283,6 +1372,31 @@ TEST_F(LeaseCmdsTest, Lease4UpdateBadParams) {
     testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp);
 }
 
+// Check that lease4-update correctly handles case when there is
+// no lease to be updated.
+TEST_F(LeaseCmdsTest, Lease4UpdateNoLease) {
+
+    // Initialize lease manager (false = v4, false = don't add any lease)
+    initLeaseMgr(false, false);
+
+    // Check that the lease manager pointer is there.
+    ASSERT_TRUE(lmptr_);
+
+    // Now send the command.
+    string txt =
+        "{\n"
+        "    \"command\": \"lease4-update\",\n"
+        "    \"arguments\": {"
+        "        \"subnet-id\": 44,\n"
+        "        \"ip-address\": \"192.0.2.1\",\n"
+        "        \"hw-address\": \"1a:1b:1c:1d:1e:1f\",\n"
+        "        \"hostname\": \"newhostname.example.org\""
+        "    }\n"
+        "}";
+    string exp_rsp = "failed to update the lease with address 192.0.2.1 - no such lease";
+    testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp);
+}
+
 // Check that a lease4 can be updated. We're changing hw-address
 // and a hostname.
 TEST_F(LeaseCmdsTest, Lease4Update) {
@@ -1458,6 +1572,33 @@ TEST_F(LeaseCmdsTest, Lease6Update) {
     EXPECT_EQ(7654321, l->iaid_);
 }
 
+
+// Check that lease6-update correctly handles case when there is
+// no lease to be updated.
+TEST_F(LeaseCmdsTest, Lease6UpdateNoLease) {
+
+    // Initialize lease manager (true = v6, false = don't add any lease)
+    initLeaseMgr(true, false);
+
+    // Check that the lease manager pointer is there.
+    ASSERT_TRUE(lmptr_);
+
+    // Now send the command.
+    string txt =
+        "{\n"
+        "    \"command\": \"lease6-update\",\n"
+        "    \"arguments\": {"
+        "        \"subnet-id\": 66,\n"
+        "        \"ip-address\": \"2001:db8::1\",\n"
+        "        \"iaid\": 7654321,\n"
+        "        \"duid\": \"88:88:88:88:88:88:88:88\",\n"
+        "        \"hostname\": \"newhostname.example.org\""
+        "    }\n"
+        "}";
+    string exp_rsp = "failed to update the lease with address 2001:db8::1 - no such lease";
+    testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp);
+}
+
 // Checks that lease6-del can handle a situation when the query is
 // broken (some required parameters are missing).
 TEST_F(LeaseCmdsTest, Lease4DelMissingParams) {
@@ -1576,6 +1717,36 @@ TEST_F(LeaseCmdsTest, Lease4DelByAddr) {
     EXPECT_FALSE(lmptr_->getLease4(IOAddress("192.0.2.1")));
 }
 
+
+// Checks that lease4-del sanitizes its input.
+TEST_F(LeaseCmdsTest, Lease4DelByAddrBadParam) {
+
+    // Initialize lease manager (false = v4, true = add a lease)
+    initLeaseMgr(false, true);
+
+    // Invalid family
+    string cmd =
+        "{\n"
+        "    \"command\": \"lease4-del\",\n"
+        "    \"arguments\": {"
+        "        \"ip-address\": \"2001:db8::1\""
+        "    }\n"
+        "}";
+    string exp_rsp = "Invalid IPv4 address specified: 2001:db8::1";
+    testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp);
+
+    // This is way off
+    cmd =
+        "{\n"
+        "    \"command\": \"lease6-del\",\n"
+        "    \"arguments\": {"
+        "        \"ip-address\": \"221B Baker St.\""
+        "    }\n"
+        "}";
+    exp_rsp = "Failed to convert string to address '221B Baker St.': Invalid argument";
+    testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp);
+}
+
 // Checks that lease4-del can handle a situation when the query is
 // well formed, but the lease is not there.
 TEST_F(LeaseCmdsTest, Lease4DelByHWAddrNotFound) {
@@ -1697,6 +1868,35 @@ TEST_F(LeaseCmdsTest, Lease6DelByAddr) {
     EXPECT_FALSE(lmptr_->getLease6(Lease::TYPE_NA, IOAddress("2001:db8::1")));
 }
 
+// Checks that lease6-del sanitizes its input.
+TEST_F(LeaseCmdsTest, Lease6DelByAddrBadParam) {
+
+    // Initialize lease manager (true = v6, true = add a lease)
+    initLeaseMgr(true, true);
+
+    // Invalid family
+    string cmd =
+        "{\n"
+        "    \"command\": \"lease6-del\",\n"
+        "    \"arguments\": {"
+        "        \"ip-address\": \"192.0.2.1\""
+        "    }\n"
+        "}";
+    string exp_rsp = "Invalid IPv6 address specified: 192.0.2.1";
+    testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp);
+
+    // This is way off
+    cmd =
+        "{\n"
+        "    \"command\": \"lease6-del\",\n"
+        "    \"arguments\": {"
+        "        \"ip-address\": \"221B Baker St.\""
+        "    }\n"
+        "}";
+    exp_rsp = "Failed to convert string to address '221B Baker St.': Invalid argument";
+    testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp);
+}
+
 // Checks that lease6-del(subnet-id, type, addr6) can handle a situation when
 // the query is correctly formed and the lease is deleted.
 TEST_F(LeaseCmdsTest, Lease6DelByAddrPrefix) {

src/lib/dhcpsrv/tests/lease_mgr_unittest.cc

@@ -223,12 +223,14 @@ public:
                   " is not implemented");
     }
 
-    /// @brief Pretends to will all IPv4 leases from a subnet
+    /// @brief Pretends to wipe all IPv4 leases from a subnet
+    /// @param subnet_id (ignored, but one day may specify the subnet)
     virtual size_t wipeLeases4(const SubnetID&) {
         isc_throw(NotImplemented, "ConreteLeaseMgr::wipeLeases4 not implemented");
     }
 
-    /// @brief Pretends to will all IPv4 leases from a subnet
+    /// @brief Pretends to wipe all IPv4 leases from a subnet
+    /// @param subnet_id (ignored, but one day may specify the subnet)
     virtual size_t wipeLeases6(const SubnetID&) {
         isc_throw(NotImplemented, "ConreteLeaseMgr::wipeLeases4 not implemented");
     }