[3186] Added doxygen commentary to user check hook lib

Added doxygen throughout, cleaned up unit tests.  Removed CLIENT_ID type
as it was unnecessary.

src/hooks/dhcp/user_chk/Makefile.am

@@ -33,7 +33,7 @@ libdhcp_user_chk_la_SOURCES += load_unload.cc
 libdhcp_user_chk_la_SOURCES += subnet_select_co.cc
 libdhcp_user_chk_la_SOURCES += user.cc user.h
 libdhcp_user_chk_la_SOURCES += user_chk_log.cc user_chk_log.h
-libdhcp_user_chk_la_SOURCES += user_data_source.cc user_data_source.h
+libdhcp_user_chk_la_SOURCES += user_data_source.h
 libdhcp_user_chk_la_SOURCES += user_file.cc user_file.h
 libdhcp_user_chk_la_SOURCES += user_registry.cc user_registry.h
 libdhcp_user_chk_la_SOURCES += version.cc

src/hooks/dhcp/user_chk/load_unload.cc

@@ -11,7 +11,8 @@
 // LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
-// load_unload.cc
+
+/// @file This file defines the load and unload hooks library functions.
 
 #include <hooks/hooks.h>
 #include <user_registry.h>
@@ -23,13 +24,28 @@
 
 using namespace isc::hooks;
 
+/// @brief Pointer to the registry instance.
 UserRegistryPtr user_registry;
+
+/// @brief Output filestream for recording user check outcomes.
 std::fstream user_chk_output;
+
+/// @brief For now, hard-code registry input file name.
 const char* registry_fname = "/tmp/user_registry.txt";
+
+/// @brief For now, hard-code user check outcome file name.
 const char* user_chk_output_fname = "/tmp/user_check_output.txt";
 
 extern "C" {
 
+/// @brief Called by the Hooks library manager when the library is loaded.
+///
+/// Instantiates the UserRegistry and opens the outcome file. Failure in
+/// either results in a failed return code.
+///
+/// @param unused library handle parameter required by Hooks API.
+///
+/// @return Returns 0 upon success, non-zero upon failure.
 int load(LibraryHandle&) {
     // non-zero indicates an error.
     int ret_val = 0;
@@ -49,14 +65,17 @@ int load(LibraryHandle&) {
         // Open up the output file for user_chk results.
         user_chk_output.open(user_chk_output_fname,
                      std::fstream::out | std::fstream::app);
-        int sav_errno = errno;
+
         if (!user_chk_output) {
+            // Grab the system error message.
+            const char* errmsg = strerror(errno);
             isc_throw(isc::Unexpected, "Cannot open output file: "
                                        << user_chk_output_fname
-                                       << " reason: " << strerror(sav_errno));
+                                       << " reason: " << errmsg);
         }
     }
     catch (const std::exception& ex) {
+        // Log the error and return failure.
         std::cout << "DHCP UserCheckHook could not be loaded: "
                   << ex.what() << std::endl;
         ret_val = 1;
@@ -65,6 +84,11 @@ int load(LibraryHandle&) {
     return (ret_val);
 }
 
+/// @brief Called by the Hooks library manager when the library is unloaded.
+///
+/// Destroys the UserRegistry and closes the outcome file.
+///
+/// @return Always returns 0.
 int unload() {
     try {
         user_registry.reset();

src/hooks/dhcp/user_chk/subnet_select_co.cc

@@ -1,3 +1,19 @@
+// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+/// @file Defines the subnet4_select and subnet6_select callout functions.
+
 #include <hooks/hooks.h>
 #include <dhcp/pkt4.h>
 #include <dhcp/dhcp6.h>
@@ -17,10 +33,50 @@ extern std::fstream user_chk_output;
 extern const char* registry_fname;
 extern const char* user_chk_output_fname;
 
-
 extern "C" {
 
-
+/// @brief Adds an entry to the end of the user check outcome file.
+///
+/// Each user entry is written in an ini-like format, with one name-value pair
+/// per line as follows:
+///
+/// id_type=<id type>
+/// client=<id str>
+/// subnet=<subnet str>
+/// registered=<is registered>"
+///
+/// where:
+/// <id type> text label of the id type: "HW_ADDR" or "DUID"
+/// <id str> user's id formatted as either isc::dhcp::Hwaddr.toText() or
+/// isc::dhcp::DUID.toText()
+/// <subnet str> selected subnet formatted as isc::dhcp::Subnet4::toText() or
+/// isc::dhcp::Subnet6::toText() as appropriate.
+/// <is registered> "yes" or "no"
+///
+/// Sample IPv4 entry would like this:
+///
+/// @code
+/// id_type=DUID
+/// client=00:01:00:01:19:ef:e6:3b:00:0c:01:02:03:04
+/// subnet=2001:db8:2::/64
+/// registered=yes
+/// id_type=duid
+/// @endcode
+///
+/// Sample IPv4 entry would like this:
+///
+/// @code
+/// id_type=DUID
+/// id_type=HW_ADDR
+/// client=hwtype=1 00:0c:01:02:03:05
+/// subnet=152.77.5.0/24
+/// registered=no
+/// @endcode
+///
+/// @param id_type_str text label identify the id type
+/// @param id_val_str text representation of the user id
+/// @param subnet_str text representation  of the selected subnet
+/// @param registered boolean indicating if the user is registered or not
 void generate_output_record(const std::string& id_type_str,
                             const std::string& id_val_str,
                             const std::string& subnet_str,
@@ -32,10 +88,25 @@ void generate_output_record(const std::string& id_type_str,
                     << "registered=" << (registered ? "yes" : "no")
                     << std::endl;
 
+    // @todo Flush is here to ensure output is immediate for demo purposes.
+    // Performance would generally dictate not using it.
     flush(user_chk_output);
 }
 
-// This callout is called at the "subnet4_select" hook.
+/// @brief  This callout is called at the "subnet4_select" hook.
+///
+/// This function searches the UserRegistry for the client indicated by the
+/// inbound IPv4 DHCP packet. If the client is found in the registry output
+/// the generate outcome record and return.
+///
+/// If the client is not found in the registry replace the selected subnet
+/// with the restricted access subnet, then generate the outcome record and
+/// return.  By convention, it is assumed that last subnet in the list of
+/// available subnets is the restricted access subnet.
+///
+/// @param handle CalloutHandle which provides access to context.
+///
+/// @return 0 upon success, non-zero otherwise.
 int subnet4_select(CalloutHandle& handle) {
     if (!user_registry) {
         std::cout << "DHCP UserCheckHook : subnet4_select UserRegistry is null"
@@ -55,22 +126,23 @@ int subnet4_select(CalloutHandle& handle) {
         // Look for the user.
         UserPtr registered_user = user_registry->findUser(*hwaddr);
         if (registered_user) {
-            // User is in the registry, so leave the pre-selected
-            // subnet alone.
+            // User is in the registry, so leave the pre-selected subnet alone.
             Subnet4Ptr subnet;
             handle.getArgument("subnet4", subnet);
-            generate_output_record("mac", hwaddr->toText(), subnet->toText(),
-                                   true);
+            // Add the outcome entry to the output file.
+            generate_output_record(UserId::HW_ADDRESS_STR, hwaddr->toText(),
+                                   subnet->toText(), true);
         } else {
-            // User is not in the registry, so assign them to
-            // the last subnet in the collection.  By convention
-            // we are assuming this is the restricted subnet.
+            // User is not in the registry, so assign them to the last subnet
+            // in the collection.  By convention we are assuming this is the
+            // restricted subnet.
             const isc::dhcp::Subnet4Collection *subnets = NULL;
             handle.getArgument("subnet4collection", subnets);
             Subnet4Ptr subnet = subnets->back();
             handle.setArgument("subnet4", subnet);
-            generate_output_record("mac", hwaddr->toText(), subnet->toText(),
-                                   false);
+            // Add the outcome entry to the output file.
+            generate_output_record(UserId::HW_ADDRESS_STR, hwaddr->toText(),
+                                   subnet->toText(), false);
         }
     } catch (const std::exception& ex) {
         std::cout << "DHCP UserCheckHook : subnet6_select unexpected error: "
@@ -80,7 +152,21 @@ int subnet4_select(CalloutHandle& handle) {
 
     return (0);
 }
-// This callout is called at the "subnet6_select" hook.
+
+/// @brief  This callout is called at the "subnet6_select" hook.
+///
+/// This function searches the UserRegistry for the client indicated by the
+/// inbound IPv6 DHCP packet. If the client is found in the registry generate
+/// the outcome record and return.
+///
+/// If the client is not found in the registry replace the selected subnet
+/// with the restricted access subnet, then generate the outcome record and
+/// return.  By convention, it is assumed that last subnet in the list of
+/// available subnets is the restricted access subnet.
+///
+/// @param handle CalloutHandle which provides access to context.
+///
+/// @return 0 upon success, non-zero otherwise.
 int subnet6_select(CalloutHandle& handle) {
     if (!user_registry) {
         std::cout << "DHCP UserCheckHook : subnet6_select UserRegistry is null"
@@ -108,22 +194,23 @@ int subnet6_select(CalloutHandle& handle) {
         // Look for the user.
         UserPtr registered_user = user_registry->findUser(*duid);
         if (registered_user) {
-            // User is in the registry, so leave the pre-selected
-            // subnet alone.
+            // User is in the registry, so leave the pre-selected subnet alone.
             Subnet6Ptr subnet;
             handle.getArgument("subnet6", subnet);
-            generate_output_record("duid", duid->toText(), subnet->toText(),
-                                   true);
+            // Add the outcome entry to the output file.
+            generate_output_record(UserId::DUID_STR, duid->toText(),
+                                   subnet->toText(), true);
         } else {
-            // User is not in the registry, so assign them to
-            // the last subnet in the collection.  By convention
-            // we are assuming this is the restricted subnet.
+            // User is not in the registry, so assign them to the last subnet
+            // in the collection.  By convention we are assuming this is the
+            // restricted subnet.
             const isc::dhcp::Subnet6Collection *subnets = NULL;
             handle.getArgument("subnet6collection", subnets);
             Subnet6Ptr subnet = subnets->back();
             handle.setArgument("subnet6", subnet);
-            generate_output_record("duid", duid->toText(), subnet->toText(),
-                                   false);
+            // Add the outcome entry to the output file.
+            generate_output_record(UserId::DUID_STR, duid->toText(),
+                                   subnet->toText(), false);
         }
     } catch (const std::exception& ex) {
         std::cout << "DHCP UserCheckHook : subnet6_select unexpected error: "

src/hooks/dhcp/user_chk/tests/Makefile.am

@@ -37,7 +37,7 @@ libdhcp_user_chk_unittests_SOURCES += ../user.cc ../user.h
 libdhcp_user_chk_unittests_SOURCES += ../user_chk_log.cc ../user_chk_log.h
 # Until logging in dynamically loaded libraries is fixed, exclude these.
 #libdhcp_user_chk_unittests_SOURCES += ../user_chk_messages.cc ../user_chk_messages.h
-libdhcp_user_chk_unittests_SOURCES += ../user_data_source.cc ../user_data_source.h
+libdhcp_user_chk_unittests_SOURCES += ../user_data_source.h
 libdhcp_user_chk_unittests_SOURCES += ../user_file.cc ../user_file.h
 libdhcp_user_chk_unittests_SOURCES += ../user_registry.cc ../user_registry.h
 libdhcp_user_chk_unittests_SOURCES += run_unittests.cc

src/hooks/dhcp/user_chk/tests/test_users_1.txt

@@ -1,3 +1,2 @@
 { "type" : "HW_ADDR", "id" : "01AC00F03344", "opt1" : "true" }
-{ "type" : "CLIENT_ID", "id" : "0899e0cc0707", "opt1" : "false" }
 { "type" : "DUID", "id" : "225060de0a0b", "opt1" : "true" }

src/hooks/dhcp/user_chk/tests/user_file_unittests.cc

@@ -25,16 +25,26 @@ using namespace std;
 
 namespace {
 
+/// @brief Convenience method for reliably building test file path names.
+///
+/// Function prefixes the given file name with a path to unit tests directory
+/// so we can reliably find test data files.
+///
+/// @param name base file name of the test file
 std::string testFilePath(const std::string& name) {
     return (std::string(USER_CHK_TEST_DIR) + "/" + name);
 }
 
+/// @brief Tests the UserFile constructor.
 TEST(UserFile, construction) {
+    // Verify that a UserFile with no file name is rejected.
     ASSERT_THROW(UserFile(""), UserFileError);
 
+    // Verify that a UserFile with a non-blank file name is accepted.
     ASSERT_NO_THROW(UserFile("someName"));
 }
 
+/// @brief Tests opening and closing UserFile
 TEST(UserFile, openFile) {
     UserFilePtr user_file;
 
@@ -47,8 +57,9 @@ TEST(UserFile, openFile) {
     EXPECT_FALSE(user_file->isOpen());
 
     // Construct a user file that should exist.
-    ASSERT_NO_THROW(user_file.reset(new
-                                    UserFile(testFilePath("test_users_1.txt"))));
+    ASSERT_NO_THROW(user_file.reset(new UserFile
+                                   (testFilePath("test_users_1.txt"))));
+    // File should not be open.
     EXPECT_FALSE(user_file->isOpen());
 
     // Verify that we can open it.
@@ -67,15 +78,57 @@ TEST(UserFile, openFile) {
     EXPECT_TRUE(user_file->isOpen());
 }
 
+
+/// @brief Tests makeUser with invalid user strings
+TEST(UserFile, makeUser) {
+    const char* invalid_strs[]= {
+        // Missinge type element.
+        "{ \"id\" : \"01AC00F03344\" }",
+        // Invalid id type string value.
+        "{ \"type\" : \"BOGUS\", \"id\" : \"01AC00F03344\"}",
+        // Non-string id type
+        "{ \"type\" : 1, \"id\" : \"01AC00F03344\"}",
+        // Missing id element.
+        "{ \"type\" : \"HW_ADDR\" }",
+        // Odd number of digits in id value.
+        "{ \"type\" : \"HW_ADDR\", \"id\" : \"1AC00F03344\"}",
+        // Invalid characters in id value.
+        "{ \"type\" : \"HW_ADDR\", \"id\" : \"THIS IS BAD!\"}",
+        // Empty id value.
+        "{ \"type\" : \"HW_ADDR\", \"id\" : \"\"}",
+        // Non-string id.
+        "{ \"type\" : \"HW_ADDR\", \"id\" : 01AC00F03344 }",
+        // Option with non-string value
+        "{ \"type\" : \"HW_ADDR\", \"id\" : \"01AC00F03344\", \"opt\" : 4 }",
+        NULL
+        };
+
+    // Create a UseFile to work with.
+    UserFilePtr user_file;
+    ASSERT_NO_THROW(user_file.reset(new UserFile("noFile")));
+
+    // Iterate over the list of invalid user strings and verify
+    // each one fails.
+    const char** tmp = invalid_strs;;
+    while (*tmp) {
+        EXPECT_THROW(user_file->makeUser(*tmp), UserFileError)
+                     << "Invalid str not caught: ["
+                     <<  *tmp << "]" << std::endl;
+        ++tmp;
+    }
+}
+
+/// @brief Test reading from UserFile
 TEST(UserFile, readFile) {
     UserFilePtr user_file;
 
-    // Construct an open a known file.
-    ASSERT_NO_THROW(user_file.reset(new
-                                    UserFile(testFilePath("test_users_1.txt"))));
+    // Construct and then open a known file.
+    ASSERT_NO_THROW(user_file.reset(new UserFile
+                                    (testFilePath("test_users_1.txt"))));
     ASSERT_NO_THROW(user_file->open());
     EXPECT_TRUE(user_file->isOpen());
 
+    // File should contain two valid users. Read and verify each.
     UserPtr user;
     int i = 0;
     do {
@@ -87,17 +140,13 @@ TEST(UserFile, readFile) {
                 EXPECT_EQ("true", user->getProperty("opt1"));
                 break;
             case 1:
-                EXPECT_EQ(UserId::CLIENT_ID, user->getUserId().getType());
-                EXPECT_EQ("0899e0cc0707", user->getUserId().toText());
-                EXPECT_EQ("false", user->getProperty("opt1"));
-                break;
-            case 2:
                 EXPECT_EQ(UserId::DUID, user->getUserId().getType());
                 EXPECT_EQ("225060de0a0b", user->getUserId().toText());
                 EXPECT_EQ("true", user->getProperty("opt1"));
                 break;
             default:
-                // this is an error, TBD
+                // Third time around, we are at EOF User should be null.
+                ASSERT_FALSE(user);
                 break;
         }
     } while (user);
@@ -106,6 +155,4 @@ TEST(UserFile, readFile) {
     ASSERT_NO_THROW(user_file->close());
 }
 
-
-
 } // end of anonymous namespace

src/hooks/dhcp/user_chk/tests/user_registry_unittests.cc

@@ -27,65 +27,72 @@ using namespace std;
 
 namespace {
 
+/// @brief Convenience method for reliably building test file path names.
+///
+/// Function prefixes the given file name with a path to unit tests directory
+/// so we can reliably find test data files.
+///
+/// @param name base file name of the test file
 std::string testFilePath(const std::string& name) {
     return (std::string(USER_CHK_TEST_DIR) + "/" + name);
 }
 
+/// @brief Tests UserRegistry construction.
 TEST(UserRegistry, constructor) {
+    // Currently there is only the default constructor which does not throw.
     UserRegistryPtr reg;
     ASSERT_NO_THROW(reg.reset(new UserRegistry()));
 }
 
+/// @brief Tests mechanics of adding, finding, removing Users.
 TEST(UserRegistry, userBasics) {
+    // Create an empty registry.
     UserRegistryPtr reg;
     ASSERT_NO_THROW(reg.reset(new UserRegistry()));
 
-    UserIdPtr id, id2;
-    // Make user ids.
-    ASSERT_NO_THROW(id.reset(new UserId(UserId::HW_ADDRESS, "01010101")));
-    ASSERT_NO_THROW(id2.reset(new UserId(UserId::HW_ADDRESS, "02020202")));
-
-    UserPtr user, user2;
     // Verify that a blank user cannot be added.
+    UserPtr user;
     ASSERT_THROW(reg->addUser(user), UserRegistryError);
 
-    // Make new users.
+    // Make a new id and user.
+    UserIdPtr id;
+    ASSERT_NO_THROW(id.reset(new UserId(UserId::HW_ADDRESS, "01010101")));
     ASSERT_NO_THROW(user.reset(new User(*id)));
-    ASSERT_NO_THROW(user2.reset(new User(*id)));
 
-    // Add first user.
+    // Verify that we can add a user.
     ASSERT_NO_THROW(reg->addUser(user));
 
-    // Verify user added can be found.
+    // Verify that the user can be found.
     UserPtr found_user;
     ASSERT_NO_THROW(found_user = reg->findUser(*id));
     EXPECT_TRUE(found_user);
     EXPECT_EQ(found_user->getUserId(), *id);
 
-    // Verify user not added cannot be found.
+    // Verify that searching for a non-existant user returns empty user pointer.
+    UserIdPtr id2;
+    ASSERT_NO_THROW(id2.reset(new UserId(UserId::HW_ADDRESS, "02020202")));
     ASSERT_NO_THROW(found_user = reg->findUser(*id2));
     EXPECT_FALSE(found_user);
 
-    // Verfiy user can no longer be found.
+    // Verify that the user can be deleted.
     ASSERT_NO_THROW(reg->removeUser(*id));
     ASSERT_NO_THROW(found_user = reg->findUser(*id));
     EXPECT_FALSE(found_user);
 }
 
+/// @brief Tests finding users by isc::dhcp::HWaddr instance.
 TEST(UserRegistry, findByHWAddr) {
+    // Create the registry.
     UserRegistryPtr reg;
     ASSERT_NO_THROW(reg.reset(new UserRegistry()));
 
-    // Make a user.
+    // Make a new user and add it.
     UserPtr user;
     ASSERT_NO_THROW(user.reset(new User(UserId::HW_ADDRESS, "01010101")));
-
-    // Verify user can be added.
     ASSERT_NO_THROW(reg->addUser(user));
 
     // Make a HWAddr instance using the same id value.
-    isc::dhcp::HWAddr hwaddr(user->getUserId().getId(),
-                             isc::dhcp::HTYPE_ETHER);
+    isc::dhcp::HWAddr hwaddr(user->getUserId().getId(), isc::dhcp::HTYPE_ETHER);
 
     // Verify user can be found by HWAddr.
     UserPtr found_user;
@@ -94,100 +101,116 @@ TEST(UserRegistry, findByHWAddr) {
     EXPECT_EQ(found_user->getUserId(), user->getUserId());
 }
 
+/// @brief Tests finding users by isc::dhcp::DUID instance.
 TEST(UserRegistry, findByDUID) {
+    // Create the registry.
     UserRegistryPtr reg;
     ASSERT_NO_THROW(reg.reset(new UserRegistry()));
 
-    // Make a user.
+    // Make a new user and add it.
     UserPtr user;
     ASSERT_NO_THROW(user.reset(new User(UserId::DUID, "01010101")));
-
-    // Verify user can be added.
     ASSERT_NO_THROW(reg->addUser(user));
 
-    // Make a HWAddr instance using the same id value.
+    // Make a DUID instance using the same id value.
     isc::dhcp::DUID duid(user->getUserId().getId());
 
-    // Verify user can be found by HWAddr.
+    // Verify user can be found by DUID.
     UserPtr found_user;
     ASSERT_NO_THROW(found_user = reg->findUser(duid));
     EXPECT_TRUE(found_user);
     EXPECT_EQ(found_user->getUserId(), user->getUserId());
 }
 
-TEST(UserRegistry, findByClientId) {
-    UserRegistryPtr reg;
-    ASSERT_NO_THROW(reg.reset(new UserRegistry()));
-
-    // Make a user.
-    UserPtr user;
-    ASSERT_NO_THROW(user.reset(new User(UserId::CLIENT_ID, "01010101")));
-
-    // Verify user can be added.
-    ASSERT_NO_THROW(reg->addUser(user));
-
-    // Make a HWAddr instance using the same id value.
-    isc::dhcp::ClientId client_id(user->getUserId().getId());
-
-    // Verify user can be found by HWAddr.
-    UserPtr found_user;
-    ASSERT_NO_THROW(found_user = reg->findUser(client_id));
-    EXPECT_TRUE(found_user);
-    EXPECT_EQ(found_user->getUserId(), user->getUserId());
-}
-
+/// @brief Tests mixing users of different types.
 TEST(UserRegistry, oneOfEach) {
+    // Create the registry.
     UserRegistryPtr reg;
     ASSERT_NO_THROW(reg.reset(new UserRegistry()));
 
     // Make user ids.
-    UserIdPtr idh, idc, idd;
+    UserIdPtr idh, idd;
     ASSERT_NO_THROW(idh.reset(new UserId(UserId::HW_ADDRESS, "01010101")));
-    ASSERT_NO_THROW(idc.reset(new UserId(UserId::CLIENT_ID, "02020202")));
     ASSERT_NO_THROW(idd.reset(new UserId(UserId::DUID, "03030303")));
 
+    // Make and add HW_ADDRESS user.
     UserPtr user;
     user.reset(new User(*idh));
     ASSERT_NO_THROW(reg->addUser(user));
 
-    user.reset(new User(*idc));
-    ASSERT_NO_THROW(reg->addUser(user));
+    // Make and add DUID user.
     user.reset(new User(*idd));
     ASSERT_NO_THROW(reg->addUser(user));
 
+    // Verify we can find both.
     UserPtr found_user;
     ASSERT_NO_THROW(found_user = reg->findUser(*idh));
-    ASSERT_NO_THROW(found_user = reg->findUser(*idc));
     ASSERT_NO_THROW(found_user = reg->findUser(*idd));
 }
 
-TEST(UserRegistry, userFileTest) {
+/// @brief Tests loading the registry from a file.
+TEST(UserRegistry, refreshFromFile) {
+    // Create the registry.
     UserRegistryPtr reg;
     ASSERT_NO_THROW(reg.reset(new UserRegistry()));
 
-    // Create the data source.
     UserDataSourcePtr user_file;
-    ASSERT_NO_THROW(user_file.reset(new
-                                    UserFile(testFilePath("test_users_1.txt"))));
+
+    // Verify that data source cannot be set to null source.
+    ASSERT_THROW(reg->setSource(user_file), UserRegistryError);
+
+    // Create the data source.
+    ASSERT_NO_THROW(user_file.reset(new UserFile
+                                    (testFilePath("test_users_1.txt"))));
 
     // Set the registry's data source and refresh the registry.
     ASSERT_NO_THROW(reg->setSource(user_file));
     ASSERT_NO_THROW(reg->refresh());
 
     // Verify we can find all the expected users.
-    UserPtr found_user;
     UserIdPtr id;
     ASSERT_NO_THROW(id.reset(new UserId(UserId::HW_ADDRESS, "01ac00f03344")));
-    ASSERT_NO_THROW(found_user = reg->findUser(*id));
-    EXPECT_TRUE(found_user);
-
-    ASSERT_NO_THROW(id.reset(new UserId(UserId::CLIENT_ID, "0899e0cc0707")));
-    ASSERT_NO_THROW(found_user = reg->findUser(*id));
-    EXPECT_TRUE(found_user);
+    EXPECT_TRUE(reg->findUser(*id));
 
     ASSERT_NO_THROW(id.reset(new UserId(UserId::DUID, "225060de0a0b")));
-    ASSERT_NO_THROW(found_user = reg->findUser(*id));
-    EXPECT_TRUE(found_user);
+    EXPECT_TRUE(reg->findUser(*id));
+}
+
+/// @brief Tests preservation of registry upon refresh failure.
+TEST(UserRegistry, refreshFail) {
+    // Create the registry.
+    UserRegistryPtr reg;
+    ASSERT_NO_THROW(reg.reset(new UserRegistry()));
+
+    // Create the data source.
+    UserDataSourcePtr user_file;
+    ASSERT_NO_THROW(user_file.reset(new UserFile
+                                    (testFilePath("test_users_1.txt"))));
+
+    // Set the registry's data source and refresh the registry.
+    ASSERT_NO_THROW(reg->setSource(user_file));
+    ASSERT_NO_THROW(reg->refresh());
+
+    // Make user ids of expected users.
+    UserIdPtr id1, id2;
+    ASSERT_NO_THROW(id1.reset(new UserId(UserId::HW_ADDRESS, "01ac00f03344")));
+    ASSERT_NO_THROW(id2.reset(new UserId(UserId::DUID, "225060de0a0b")));
+
+    // Verify we can find all the expected users.
+    EXPECT_TRUE(reg->findUser(*id1));
+    EXPECT_TRUE(reg->findUser(*id2));
+
+    // Replace original data source with a new one containing an invalid entry.
+    ASSERT_NO_THROW(user_file.reset(new UserFile
+                                    (testFilePath("test_users_err.txt"))));
+    ASSERT_NO_THROW(reg->setSource(user_file));
+
+    // Refresh should throw due to invalid data.
+    EXPECT_THROW(reg->refresh(), UserRegistryError);
+
+    // Verify we can still find all the original users.
+    EXPECT_TRUE(reg->findUser(*id1));
+    EXPECT_TRUE(reg->findUser(*id2));
 }
 
 } // end of anonymous namespace

src/hooks/dhcp/user_chk/tests/user_unittests.cc

@@ -24,49 +24,73 @@ using namespace std;
 
 namespace {
 
+/// @brief Tests User construction variants.
+/// Note, since all constructors accept or rely on UserId, invalid id
+/// variants are tested under UserId not here.
 TEST(UserTest, construction) {
     std::string test_address("01FF02AC030B0709");
 
+    // Create a user id.
     UserIdPtr id;
     ASSERT_NO_THROW(id.reset(new UserId(UserId::HW_ADDRESS, test_address)));
 
-    UserPtr user;
     // Verify construction from a UserId.
+    UserPtr user;
     ASSERT_NO_THROW(user.reset(new User(*id)));
 
-    // Verify construction from a type and an address vector.
+    // Verify construction from an id type and an address vector.
     ASSERT_NO_THROW(user.reset(new User(UserId::HW_ADDRESS, id->getId())));
     ASSERT_NO_THROW(user.reset(new User(UserId::DUID, id->getId())));
-    ASSERT_NO_THROW(user.reset(new User(UserId::CLIENT_ID, id->getId())));
 
-    // Verify construction from a type and an address string.
+    // Verify construction from an id type and an address string.
     ASSERT_NO_THROW(user.reset(new User(UserId::HW_ADDRESS, test_address)));
     ASSERT_NO_THROW(user.reset(new User(UserId::DUID, test_address)));
-    ASSERT_NO_THROW(user.reset(new User(UserId::CLIENT_ID, test_address)));
 }
 
+/// @brief Tests property map fundamentals.
 TEST(UserTest, properties) {
+    // Create a user.
     UserPtr user;
     ASSERT_NO_THROW(user.reset(new User(UserId::DUID, "01020304050607")));
 
+    // Verify that we can add and retrieve a property.
     std::string value = "";
     EXPECT_NO_THROW(user->setProperty("one","1"));
     EXPECT_NO_THROW(value = user->getProperty("one"));
     EXPECT_EQ("1", value);
 
+    // Verify that we can update and then retrieve the property.
     EXPECT_NO_THROW(user->setProperty("one","1.0"));
     EXPECT_NO_THROW(value = user->getProperty("one"));
     EXPECT_EQ("1.0", value);
 
+    // Verify that we can remove and then NOT find the property.
     EXPECT_NO_THROW(user->delProperty("one"));
     EXPECT_NO_THROW(value = user->getProperty("one"));
     EXPECT_TRUE(value.empty());
 
+    // Verify that a blank property name is NOT permitted.
     EXPECT_THROW(user->setProperty("", "blah"), isc::BadValue);
 
+    // Verify that a blank property value IS permitted.
     EXPECT_NO_THROW(user->setProperty("one", ""));
     EXPECT_NO_THROW(value = user->getProperty("one"));
     EXPECT_TRUE(value.empty());
+
+    PropertyMap map;
+    map["one"]="1.0";
+    map["two"]="2.0";
+
+    ASSERT_NO_THROW(user->setProperties(map));
+
+    EXPECT_NO_THROW(value = user->getProperty("one"));
+    EXPECT_EQ("1.0", value);
+
+    EXPECT_NO_THROW(value = user->getProperty("two"));
+    EXPECT_EQ("2.0", value);
+
+    const PropertyMap& map2 = user->getProperties();
+    EXPECT_EQ(map2, map);
 }
 
 } // end of anonymous namespace

src/hooks/dhcp/user_chk/tests/userid_unittests.cc

@@ -24,50 +24,54 @@ using namespace std;
 
 namespace {
 
+/// @brief Test invalid constructors.
 TEST(UserIdTest, invalidConstructors) {
     // Verify that constructor does not allow empty id vector.
     std::vector<uint8_t> empty_bytes;
     ASSERT_THROW(UserId(UserId::HW_ADDRESS, empty_bytes), isc::BadValue);
     ASSERT_THROW(UserId(UserId::DUID, empty_bytes), isc::BadValue);
-    ASSERT_THROW(UserId(UserId::CLIENT_ID, empty_bytes), isc::BadValue);
 
     // Verify that constructor does not allow empty id string.
     ASSERT_THROW(UserId(UserId::HW_ADDRESS, ""), isc::BadValue);
     ASSERT_THROW(UserId(UserId::DUID, ""), isc::BadValue);
-    ASSERT_THROW(UserId(UserId::CLIENT_ID, ""), isc::BadValue);
 }
 
+/// @brief Test making and using HW_ADDRESS type UserIds
 TEST(UserIdTest, hwAddress_type) {
-
+    // Verify text label look up for HW_ADDRESS enum.
     EXPECT_EQ(std::string(UserId::HW_ADDRESS_STR),
               UserId::lookupTypeStr(UserId::HW_ADDRESS));
 
+    // Verify enum look up for HW_ADDRESS text label.
     EXPECT_EQ(UserId::HW_ADDRESS,
               UserId::lookupType(UserId::HW_ADDRESS_STR));
 
+    // Build a test address vector.
     uint8_t tmp[] = { 0x01, 0xFF, 0x02, 0xAC, 0x03, 0x0B, 0x07, 0x08 };
     std::vector<uint8_t> bytes(tmp, tmp + (sizeof(tmp)/sizeof(uint8_t)));
 
+    // Verify construction from an HW_ADDRESS id type and address vector.
     UserIdPtr id;
     ASSERT_NO_THROW(id.reset(new UserId(UserId::HW_ADDRESS, bytes)));
+    // Verify that the id can be fetched.
     EXPECT_EQ(id->getType(), UserId::HW_ADDRESS);
     EXPECT_EQ(bytes, id->getId());
 
-    // Verify a == b;
+    // Check relational oeprators when a == b.
     UserIdPtr id2;
     ASSERT_NO_THROW(id2.reset(new UserId(UserId::HW_ADDRESS, id->toText())));
     EXPECT_TRUE(*id == *id2);
     EXPECT_FALSE(*id != *id2);
     EXPECT_FALSE(*id < *id2);
 
-    // Verify a < b;
+    // Check relational oeprators when a < b.
     ASSERT_NO_THROW(id2.reset(new UserId(UserId::HW_ADDRESS,
                                          "01FF02AC030B0709")));
     EXPECT_FALSE(*id == *id2);
     EXPECT_TRUE(*id != *id2);
     EXPECT_TRUE(*id < *id2);
 
-    // Verify a > b;
+    // Check relational oeprators when a > b.
     ASSERT_NO_THROW(id2.reset(new UserId(UserId::HW_ADDRESS,
                                          "01FF02AC030B0707")));
     EXPECT_FALSE(*id == *id2);
@@ -75,108 +79,60 @@ TEST(UserIdTest, hwAddress_type) {
     EXPECT_FALSE(*id < *id2);
 }
 
+/// @brief Test making and using DUID type UserIds
 TEST(UserIdTest, duid_type) {
+    // Verify text label look up for DUID enum.
     EXPECT_EQ(std::string(UserId::DUID_STR),
               UserId::lookupTypeStr(UserId::DUID));
 
+    // Verify enum look up for DUID text label.
     EXPECT_EQ(UserId::DUID,
               UserId::lookupType(UserId::DUID_STR));
 
+    // Build a test DUID vector.
     uint8_t tmp[] = { 0x01, 0xFF, 0x02, 0xAC, 0x03, 0x0B, 0x07, 0x08 };
     std::vector<uint8_t> bytes(tmp, tmp + (sizeof(tmp)/sizeof(uint8_t)));
 
+    // Verify construction from an DUID id type and address vector.
     UserIdPtr id;
     ASSERT_NO_THROW(id.reset(new UserId(UserId::DUID, bytes)));
+    // Verify that the id can be fetched.
     EXPECT_EQ(id->getType(), UserId::DUID);
     EXPECT_EQ(bytes, id->getId());
 
-    // Verify a == b;
+    // Check relational oeprators when a == b.
     UserIdPtr id2;
     ASSERT_NO_THROW(id2.reset(new UserId(UserId::DUID, id->toText())));
     EXPECT_TRUE(*id == *id2);
     EXPECT_FALSE(*id != *id2);
     EXPECT_FALSE(*id < *id2);
 
-    // Verify a < b;
+    // Check relational oeprators when a < b.
     ASSERT_NO_THROW(id2.reset(new UserId(UserId::DUID, "01FF02AC030B0709")));
     EXPECT_FALSE(*id == *id2);
     EXPECT_TRUE(*id != *id2);
     EXPECT_TRUE(*id < *id2);
 
-    // Verify a > b;
+    // Check relational oeprators when a > b.
     ASSERT_NO_THROW(id2.reset(new UserId(UserId::DUID, "01FF02AC030B0707")));
     EXPECT_FALSE(*id == *id2);
     EXPECT_TRUE(*id != *id2);
     EXPECT_FALSE(*id < *id2);
 }
 
-TEST(UserIdTest, client_id_type) {
-    EXPECT_EQ(std::string(UserId::CLIENT_ID_STR),
-              UserId::lookupTypeStr(UserId::CLIENT_ID));
-
-    EXPECT_EQ(UserId::CLIENT_ID,
-              UserId::lookupType(UserId::CLIENT_ID_STR));
-
-    uint8_t tmp[] = { 0x01, 0xFF, 0x02, 0xAC, 0x03, 0x0B, 0x07, 0x08 };
-    std::vector<uint8_t> bytes(tmp, tmp + (sizeof(tmp)/sizeof(uint8_t)));
-
-    UserIdPtr id;
-    ASSERT_NO_THROW(id.reset(new UserId(UserId::CLIENT_ID, bytes)));
-    EXPECT_EQ(id->getType(), UserId::CLIENT_ID);
-    EXPECT_EQ(bytes, id->getId());
-
-    // Verify a == b;
-    UserIdPtr id2;
-    ASSERT_NO_THROW(id2.reset(new UserId(UserId::CLIENT_ID, id->toText())));
-    EXPECT_TRUE(*id == *id2);
-    EXPECT_FALSE(*id != *id2);
-    EXPECT_FALSE(*id < *id2);
-
-    // Verify a < b;
-    ASSERT_NO_THROW(id2.reset(new UserId(UserId::CLIENT_ID,
-                                         "01FF02AC030B0709")));
-    EXPECT_FALSE(*id == *id2);
-    EXPECT_TRUE(*id != *id2);
-    EXPECT_TRUE(*id < *id2);
-
-    // Verify a > b;
-    ASSERT_NO_THROW(id2.reset(new UserId(UserId::CLIENT_ID,
-                                         "01FF02AC030B0707")));
-    EXPECT_FALSE(*id == *id2);
-    EXPECT_TRUE(*id != *id2);
-    EXPECT_FALSE(*id < *id2);
-}
-
+/// @brief Tests that UserIds of different types compare correctly.
 TEST(UserIdTest, mixed_type_compare) {
-    UserIdPtr hw, duid, client;
-    // Address are the same
+    UserIdPtr hw, duid;
+    // Create UserIds with different types, but same id data.
     ASSERT_NO_THROW(hw.reset(new UserId(UserId::HW_ADDRESS,
                                         "01FF02AC030B0709")));
     ASSERT_NO_THROW(duid.reset(new UserId(UserId::DUID,
                                           "01FF02AC030B0709")));
-    ASSERT_NO_THROW(client.reset(new UserId(UserId::CLIENT_ID,
-                                            "01FF02AC030B0709")));
 
     // Verify that UserIdType influences logical comparators.
-    EXPECT_TRUE(*hw < *duid);
-    EXPECT_TRUE(*duid < *client);
-
-
-    // Now use different addresses.
-    ASSERT_NO_THROW(hw.reset(new UserId(UserId::HW_ADDRESS,
-                                        "01010101")));
-    ASSERT_NO_THROW(duid.reset(new UserId(UserId::DUID,
-                                         "02020202")));
-    ASSERT_NO_THROW(client.reset(new UserId(UserId::CLIENT_ID,
-                                            "03030303")));
-
     EXPECT_FALSE(*hw == *duid);
     EXPECT_TRUE(*hw != *duid);
     EXPECT_TRUE(*hw < *duid);
-
-    EXPECT_FALSE(*duid == *client);
-    EXPECT_TRUE(*duid != *client);
-    EXPECT_TRUE(*duid < *client);
 }
 
 

src/hooks/dhcp/user_chk/user.cc

@@ -22,9 +22,9 @@
 #include <sstream>
 
 //********************************* UserId ******************************
+
 const char* UserId::HW_ADDRESS_STR = "HW_ADDR";
 const char* UserId::DUID_STR = "DUID";
-const char* UserId::CLIENT_ID_STR = "CLIENT_ID";
 
 UserId::UserId(UserIdType id_type, const std::vector<uint8_t>& id)
     : id_type_(id_type), id_(id) {
@@ -35,13 +35,12 @@ UserId::UserId(UserIdType id_type, const std::vector<uint8_t>& id)
 
 UserId::UserId(UserIdType id_type, const std::string & id_str) :
     id_type_(id_type) {
-
     if (id_str.empty()) {
         isc_throw(isc::BadValue, "UserId id string may not be blank");
     }
 
-    // logic to convert id_str etc...
-    // input str is expected to be 2-digits per bytes, no delims
+    // Convert the id string to vector.
+    // Input is expected to be 2-digits per bytes, no delimiters.
     std::vector<uint8_t> addr_bytes;
     decodeHex(id_str, addr_bytes);
 
@@ -51,10 +50,6 @@ UserId::UserId(UserIdType id_type, const std::string & id_str) :
             isc::dhcp::HWAddr hwaddr(addr_bytes, isc::dhcp::HTYPE_ETHER);
             break;
             }
-        case CLIENT_ID: {
-            isc::dhcp::ClientId client_id(addr_bytes);
-            break;
-            }
         case DUID: {
             isc::dhcp::DUID duid(addr_bytes);
             break;
@@ -148,9 +143,6 @@ UserId::lookupTypeStr(UserIdType type) {
         case DUID:
             tmp = DUID_STR;
             break;
-        case CLIENT_ID:
-            tmp = CLIENT_ID_STR;
-            break;
         default:
             isc_throw(isc::BadValue, "Invalid UserIdType:" << type);
             break;
@@ -165,8 +157,6 @@ UserId::lookupType(const std::string& type_str) {
         return (HW_ADDRESS);
     } else if (type_str.compare(DUID_STR) == 0) {
         return (DUID);
-    } else if (type_str.compare(CLIENT_ID_STR) == 0) {
-        return (CLIENT_ID);
     }
 
     isc_throw(isc::BadValue, "Invalid UserIdType string:" << type_str);
@@ -210,6 +200,7 @@ void User::setProperty(const std::string& name, const std::string& value) {
         isc_throw (isc::BadValue, "User property name cannot be blank");
     }
 
+    // Note that if the property exists its value will be updated.
     properties_[name]=value;
 }
 
@@ -220,6 +211,9 @@ User::getProperty(const std::string& name) const {
         return ((*it).second);
     }
 
+    // By returning an empty string rather than throwing, we allow the
+    // flexibility of defaulting to blank if not specified.  Let the caller
+    // decide if that is valid or not.
     return ("");
 }
 

src/hooks/dhcp/user_chk/user.h

@@ -21,33 +21,79 @@
 #include <stdint.h>
 #include <vector>
 
+/// @file user.h This file defines classes: UserId and User.
+/// @brief These classes are used to describe and recognize DHCP lease
+/// requestors (i.e. clients).
+
+/// @brief Encapsulates a unique identifier for a DHCP client.
+/// This class provides a generic wrapper around the information used to
+/// uniquely identify the requester in a DHCP request packet.  It provides
+/// the necessary operators such that it can be used as a key within STL
+/// containers such as maps.  It supports both IPv4 and IPv6 clients.
 class UserId {
 public:
-    // Use explicit value to ensure consistent numeric ordering for key
-    // comparisions.
+    /// @brief Defines the supported types of user ids.
+    // Use explicit values to ensure consistent numeric ordering for key
+    // comparisons.
     enum UserIdType {
+        /// @brief Hardware addresses (MAC) are used for IPv4 clients.
         HW_ADDRESS = 0,
-        DUID = 1,
-        CLIENT_ID = 2
+        /// @brief DUIDs are used for IPv6 clients.
+        DUID = 1
     };
 
+    /// @brief Defines the text label hardware address id type.
     static const char* HW_ADDRESS_STR;
+    /// @brief Define the text label DUID id type.
     static const char* DUID_STR;
-    static const char* CLIENT_ID_STR;
 
+    /// @brief Constructor
+    ///
+    /// Constructs a UserId from an id type and id vector.
+    ///
+    /// @param id_type The type of user id contained in vector
+    /// @param id a vector of unsigned bytes containing the id
+    ///
+    /// @throw isc::BadValue if the vector is empty.
     UserId(UserIdType id_type, const std::vector<uint8_t>& id);
 
+    /// @brief Constructor
+    ///
+    /// Constructs a UserId from an id type and id string.
+    ///
+    /// @param id_type The type of user id contained in string.
+    /// The string is expected to contain an even number of hex digits
+    /// without delimiters.
+    ///
+    /// @param id a vector of unsigned bytes containing the id
+    ///
+    /// @throw isc::BadValue if the string is empty, contains non
+    /// valid hex digits, or an odd number of hex digits.
     UserId(UserIdType id_type, const std::string& id_str);
 
+    /// @brief Destructor.
     ~UserId();
 
     /// @brief Returns a const reference to the actual id value
     const std::vector<uint8_t>& getId() const;
 
-    /// @brief Returns the UserIdType
+    /// @brief Returns the user id type
     UserIdType getType() const;
 
-    /// @brief Returns textual representation of a id (e.g. 00:01:02:03:ff)
+    /// @brief Returns textual representation of the id
+    ///
+    /// Outputs a string of hex digits representing the id with an
+    /// optional delimiter between digit pairs (i.e. bytes).
+    ///
+    /// Without a delimiter:
+    ///   "0c220F"
+    ///
+    /// with colon as a delimiter:
+    ///   "0c:22:0F"
+    ///
+    /// @param delim_char The delimiter to place in between
+    /// "bytes". It defaults to none.
+    ///  (e.g. 00010203ff)
     std::string toText(char delim_char=0x0) const;
 
     /// @brief Compares two UserIds for equality
@@ -56,14 +102,31 @@ public:
     /// @brief Compares two UserIds for inequality
     bool operator !=(const UserId & other) const;
 
-    /// @brief Performs less than comparision of two UserIds
+    /// @brief Performs less than comparison of two UserIds
     bool operator <(const UserId & other) const;
 
+    /// @brief Returns the text label for a given id type
+    ///
+    /// @param type The id type value for which the label is desired
+    ///
+    /// @throw isc::BadValue if type is not valid.
     static std::string lookupTypeStr(UserIdType type);
 
+    /// @brief Returns the id type for a given text label
+    ///
+    /// @param type_str The text label for which the id value is desired
+    ///
+    /// @throw isc::BadValue if type_str is not a valid text label.
     static UserIdType lookupType(const std::string& type_str);
 
 private:
+    /// @brief Converts a string of hex digits to vector of bytes
+    ///
+    /// @param input string of hex digits to convert
+    /// @param bytes vector in which to place the result
+    ///
+    /// @throw isc::BadValue if input string contains invalid hex digits
+    /// or has an odd number of digits.
     void decodeHex(const std::string& input, std::vector<uint8_t>& bytes) const;
 
     /// @brief The type of id value
@@ -74,43 +137,122 @@ private:
 
 };
 
+/// @brief Outputs the UserId contents in a string to the given stream.
+///
+/// The output string has the form "<type>=<id>" where:
+///
+/// <type> is the text label returned by UserId::lookupTypeStr()
+/// <id> is the output of UserId::toText() without a delimiter.
+///
+/// Examples:
+///       HW_ADDR=0c0e0a01ff06
+///       DUID=0001000119efe63b000c01020306
+///
+/// @param os output stream to which to write
+/// @param user_id source object to output
 std::ostream&
 operator<<(std::ostream& os, const UserId& user_id);
 
+/// @brief Defines a smart pointer to UserId
 typedef boost::shared_ptr<UserId> UserIdPtr;
 
+/// @brief Defines a map of string values keyed by string labels.
 typedef std::map<std::string, std::string> PropertyMap;
 
+/// @brief Represents a unique DHCP user
+/// This class is used to represent a specific DHCP user who is identified by a
+/// unique id and who possesses a set of properties.
 class User {
 public:
-
+    /// @brief Constructor
+    ///
+    /// Constructs a new User from a given id with an empty set of properties.
+    ///
+    /// @param user_id Id to assign to the user
+    ///
+    /// @throw isc::BadValue if user id is blank.
     User(const UserId & user_id);
 
+    /// @brief Constructor
+    ///
+    /// Constructs a new User from a given id type and vector containing the
+    /// id data with an empty set of properties.
+    ///
+    /// @param user_id Type of id contained in the id vector
+    /// @param id Vector of data representing the user's id
+    ///
+    /// @throw isc::BadValue if user id vector is empty.
     User(UserId::UserIdType id_type, const std::vector<uint8_t>& id);
 
+    /// @brief Constructor
+    ///
+    /// Constructs a new User from a given id type and string containing the
+    /// id data with an empty set of properties.
+    ///
+    /// @param user_id Type of id contained in the id vector
+    /// @param id string of hex digits representing the user's id
+    ///
+    /// @throw isc::BadValue if user id string is empty or invalid
     User(UserId::UserIdType id_type, const std::string& id_str);
 
+    /// @brief Destructor
     ~User();
 
+    /// @brief Returns a reference to the map of properties.
+    ///
+    /// Note that this reference can go out of scope and should not be
+    /// relied upon other than for momentary use.
     const PropertyMap& getProperties() const;
 
+    /// @brief Sets the user's properties from a given property map
+    ///
+    /// Replaces the contents of the user's property map with the given
+    /// property map.
+    ///
+    /// @param properties property map to assign to the user
     void setProperties(const PropertyMap& properties);
 
+    /// @brief Sets a given property to the given value
+    ///
+    /// Adds or updates the given property to the given value.
+    ///
+    /// @param name string by which the property is identified (keyed)
+    /// @param value string data to associate with the property
+    ///
+    /// @throw isc::BadValue if name is blank.
     void setProperty(const std::string& name, const std::string& value);
 
+    /// @brief Fetches the string value for a given property name.
+    ///
+    /// @param name property name to fetch
+    ///
+    /// @return Returns the string value for the given name or an empty string
+    /// if the property is not found in the property map.
     std::string getProperty(const std::string& name) const;
 
+    /// @brief Removes the given property from the property map.
+    ///
+    /// Removes the given property from the map if found. If not, no harm no
+    /// foul.
+    ///
+    /// @param name property name to remove
     void delProperty(const std::string& name);
 
+    /// @brief Returns the user's id.
+    ///
+    /// Note that this reference can go out of scope and should not be
+    /// relied upon other than for momentary use.
     const UserId& getUserId() const;
 
 private:
-
+    /// @brief The user's id.
     UserId user_id_;
 
+    /// @brief The user's property map.
     PropertyMap properties_;
 };
 
+/// @brief Defines a smart pointer to a User.
 typedef boost::shared_ptr<User> UserPtr;
 
 #endif

src/hooks/dhcp/user_chk/user_data_source.cc

@@ -1,46 +0,0 @@
-// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
-//
-// Permission to use, copy, modify, and/or distribute this software for any
-// purpose with or without fee is hereby granted, provided that the above
-// copyright notice and this permission notice appear in all copies.
-//
-// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
-// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
-// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
-// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
-// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
-// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
-// PERFORMANCE OF THIS SOFTWARE.
-
-#include <user_data_source.h>
-
-UserDataSource::UserDataSource() : open_flag_(false) {
-}
-
-UserDataSource::~UserDataSource() {
-}
-
-void
-UserDataSource::open() {
-    open_flag_ = false;
-}
-
-UserPtr
-UserDataSource::readNextUser() {
-    return (UserPtr());
-}
-
-void
-UserDataSource::close() {
-    open_flag_ = false;
-}
-
-bool
-UserDataSource::isOpen() const {
-    return open_flag_;
-}
-
-void
-UserDataSource::setOpenFlag(bool value) {
-  open_flag_ = value;
-}

src/hooks/dhcp/user_chk/user_data_source.h

@@ -14,29 +14,62 @@
 #ifndef _USER_DATA_SOURCE_H
 #define _USER_DATA_SOURCE_H
 
+/// @file user_data_source.h Defines the base class, UserDataSource.
+#include <exceptions/exceptions.h>
 #include <user.h>
 
-class UserDataSource {
-
+/// @brief Thrown if UserDataSource encounters an error
+class UserDataSourceError : public isc::Exception {
 public:
-    UserDataSource();
-
-    virtual ~UserDataSource();
+    UserDataSourceError(const char* file, size_t line,
+                               const char* what) :
+        isc::Exception(file, line, what) { };
+};
 
-    virtual void open();
+/// @brief Defines an interface for reading user data into a registry.
+/// This is an abstract class which defines the interface for reading Users
+/// from an IO source such as a file.
+class UserDataSource {
+public:
+    /// @brief Constructor.
+    UserDataSource() {};
 
-    virtual UserPtr readNextUser();
+    /// @brief Virtual Destructor.
+    virtual ~UserDataSource() {};
 
-    virtual void close();
+    /// @brief Opens the data source.
+    ///
+    /// Prepares the data source for reading.  Upon successful completion the
+    /// data source is ready to read from the beginning of its content.
+    ///
+    /// @throw UserDataSourceError if the source fails to open.
+    virtual void open() = 0;
 
-    bool isOpen() const;
+    /// @brief Fetches the next user from the data source.
+    ///
+    /// Reads the next User from the data source and returns it.  If no more
+    /// data is available it should return an empty (null) user.
+    ///
+    /// @throw UserDataSourceError if an error occurs.
+    virtual UserPtr readNextUser() = 0;
 
-    void setOpenFlag(bool value);
+    /// @brief Closes that data source.
+    ///
+    /// Closes the data source.
+    ///
+    /// This method must not throw exceptions.
+    virtual void close() = 0;
 
-private:
-    bool open_flag_;
+    /// @brief Returns true if the data source is open.
+    ///
+    /// This method should return true once the data source has been
+    /// successfully opened and until it has been closed.
+    ///
+    /// It is assumed to be exception safe.
+    virtual bool isOpen() const = 0;
 };
 
+/// @brief Defines a smart pointer to a UserDataSource.
 typedef boost::shared_ptr<UserDataSource> UserDataSourcePtr;
 
 #endif

src/hooks/dhcp/user_chk/user_file.cc

@@ -19,7 +19,7 @@
 #include <boost/foreach.hpp>
 #include <errno.h>
 
-UserFile::UserFile(const std::string& fname) : fname_(fname) {
+UserFile::UserFile(const std::string& fname) : fname_(fname), ifs_() {
     if (fname_.empty()) {
         isc_throw(UserFileError, "file name cannot be blank");
     }
@@ -41,8 +41,6 @@ UserFile::open() {
         isc_throw(UserFileError, "cannot open file:" << fname_
                                  << " reason: " << strerror(sav_error));
     }
-
-    setOpenFlag(true);
 }
 
 UserPtr
@@ -54,17 +52,17 @@ UserFile::readNextUser() {
     if (ifs_.good()) {
         char buf[4096];
 
-        // get the next line
+        // Get the next line.
         ifs_.getline(buf, sizeof(buf));
 
-        // we got something, try to make a user out of it.
+        // We got something, try to make a user out of it.
         if (ifs_.gcount() > 0) {
             return(makeUser(buf));
         }
     }
 
-    // returns an empty user
-    return (UserDataSource::readNextUser());
+    // Returns an empty user on EOF.
+    return (UserPtr());
 }
 
 UserPtr
@@ -88,26 +86,37 @@ UserFile::makeUser(const std::string& user_string) {
     std::string id_type_str;
     std::string id_str;
 
+    // Iterate over the elements, saving of "type" and "id" to their
+    // respective locals.  Anything else is assumed to be an option so
+    // add it to the local property map.
     std::pair<std::string, isc::data::ConstElementPtr> element_pair;
     BOOST_FOREACH (element_pair, elements->mapValue()) {
+        // Get the element's label.
         std::string label = element_pair.first;
-        std::string value = "";
-        element_pair.second->getValue(value);
+
+        // Currently everything must be a string.
+        if (element_pair.second->getType() != isc::data::Element::string) {
+            isc_throw (UserFileError, "UserFile entry: " << user_string
+                       << "has non-string value for : " << label);
+        }
+
+        std::string value = element_pair.second->stringValue();
 
         if (label == "type") {
             id_type_str = value;
         } else if (label == "id") {
             id_str = value;
         } else {
-            if (properties.find(label) != properties.end()) {
-                isc_throw (UserFileError,
-                           "UserFile entry contains duplicate values: "
-                           << user_string);
-            }
+            // JSON parsing reduces any duplicates to the last value parsed,
+            // so we will never see duplicates here.
+            std::cout << "adding propetry: " << label << ":[" << value << "]"
+                      << std::endl;
+
             properties[label]=value;
         }
     }
 
+    // First we attempt to translate the id type.
     UserId::UserIdType id_type;
     try {
         id_type = UserId::lookupType(id_type_str);
@@ -116,25 +125,37 @@ UserFile::makeUser(const std::string& user_string) {
                                   << user_string << " " << ex.what());
     }
 
+    // Id type is valid, so attempt to make the user based on that and
+    // the value we have for "id".
     UserPtr user;
     try {
         user.reset(new User(id_type, id_str));
     } catch (const std::exception& ex) {
         isc_throw (UserFileError, "UserFile cannot create user form entry: "
                                   << user_string << " " << ex.what());
-   }
-
+    }
 
+    // We have a new User, so add in the properties and return it.
     user->setProperties(properties);
     return (user);
 }
 
+bool
+UserFile::isOpen() const {
+    return (ifs_.is_open());
+}
+
 void
 UserFile::close() {
-    if (ifs_.is_open()) {
-        ifs_.close();
+    try {
+        if (ifs_.is_open()) {
+            ifs_.close();
+        }
+    } catch (const std::exception& ex) {
+        // Highly unlikely to occur but let's at least spit out an error.
+        // Beyond that we swallow it for tidiness.
+        std::cout << "UserFile unexpected error closing the file: "
+                  << fname_ << " : " << ex.what() << std::endl;
     }
-
-    setOpenFlag(false);
 }
 

src/hooks/dhcp/user_chk/user_file.h

@@ -11,11 +11,10 @@
 // LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
-
 #ifndef _USER_FILE_H
 #define _USER_FILE_H
 
-#include <exceptions/exceptions.h>
+/// @file user_file.h Defines the class, UserFile, which implements the UserDataSource interface for text files.
 
 #include <user_data_source.h>
 #include <user.h>
@@ -26,36 +25,106 @@
 
 using namespace std;
 
-/// @brief Thrown UserFile encounters an error
-class UserFileError : public isc::Exception {
+/// @brief Thrown a UserFile encounters an error.
+/// Note that it derives from UserDataSourceError to comply with the interface.
+class UserFileError : public UserDataSourceError {
 public:
     UserFileError(const char* file, size_t line,
                                const char* what) :
-        isc::Exception(file, line, what) { };
+        UserDataSourceError(file, line, what) { };
 };
 
+/// @brief Provides a UserDataSource implementation for JSON text files.
+/// This class allows a text file of JSON entries to be treated as a source of
+/// User entries.  The format of the file is one user entry per line, where
+/// each line contains a JSON string as follows:
+///
+/// { "type" : "<user type>", "id" : "<user_id>" (options)  }
+///
+/// where:
+///
+/// <user_type>  text label of the id type: "HW_ADDR" or "DUID"
+/// <user_id>  the user's id as a string of hex digits without delimiters
+/// (options) zero or more string elements as name-value pairs, separated by
+/// commas: "opt1" : "val1",  "other_opt", "77" ...
+///
+/// Each entry must have a valid entry for "type" and a valid entry or "id".
+///
+/// If an entry contains duplicate option names, that option will be assigend
+/// the last value found. This is typical JSON behavior.
+/// Currently, only string option values (i.e. enclosed in quotes) are
+/// supported.
+///
+/// Example file entries might look like this:
+/// @code
+///
+/// { "type" : "HW_ADDR", "id" : "01AC00F03344", "opt1" : "true" }
+/// { "type" : "DUID", "id" : "225060de0a0b", "opt1" : "false" }
+///
+/// @endcode
 class UserFile : public UserDataSource {
 public:
+    /// @brief Constructor
+    ///
+    /// Create a UserFile for the given file name without opening the file.
+    /// @param fname pathname to the input file.
+    ///
+    /// @throw UserFileError if given file name is empty.
     UserFile(const std::string& fname);
 
+    /// @brief Destructor.
+    ////
+    /// The destructor does call the close method.
     virtual ~UserFile();
 
-    // must throw if open fails.
-    void open();
-
-    UserPtr readNextUser();
-
-    void close();
-
+    /// @brief Opens the input file for reading.
+    ///
+    /// Upon successful completion, the file is opened and positioned to start
+    /// reading from the beginning of the file.
+    ///
+    /// @throw UserFileError if the file cannot be opened.
+    virtual void open();
+
+    /// @brief Fetches the next user from the file.
+    ///
+    /// Reads the next user entry from the file and attempts to create a
+    /// new User from the text therein.  If there is no more data to be read
+    /// it returns an empty UserPtr.
+    ///
+    /// @return A UserPtr pointing to the new User or an empty pointer on EOF.
+    ///
+    /// @throw UserFileError if an error occurs while reading.
+    virtual UserPtr readNextUser();
+
+    /// @brief Closes the underlying file.
+    ///
+    /// Method is exception safe.
+    virtual void close();
+
+    /// @brief Returns true if the file is open.
+    ///
+    /// @return True if the underlying file is open, false otherwise.
+    virtual bool isOpen() const;
+
+    /// @brief Creates a new User instance from JSON text.
+    ///
+    /// @param user_string string the JSON text for a user entry.
+    ///
+    /// @return A pointer to the newly created User instance.
+    ///
+    /// @throw UserFileError if the entry is invalid.
     UserPtr makeUser(const std::string& user_string);
 
 private:
+    /// @brief Pathname of the input text file.
     string fname_;
 
+    /// @brief Input file stream.
     std::ifstream ifs_;
 
 };
 
+/// @brief Defines a smart pointer to a UserFile.
 typedef boost::shared_ptr<UserFile> UserFilePtr;
 
 #endif

src/hooks/dhcp/user_chk/user_registry.cc

@@ -64,12 +64,6 @@ UserRegistry::findUser(const isc::dhcp::HWAddr& hwaddr) const {
 }
 
 const UserPtr&
-UserRegistry::findUser(const isc::dhcp::ClientId& client_id) const {
-    UserId id(UserId::CLIENT_ID, client_id.getClientId());
-    return (findUser(id));
-}
-
-const UserPtr&
 UserRegistry::findUser(const isc::dhcp::DUID& duid) const {
     UserId id(UserId::DUID, duid.getDuid());
     return (findUser(id));
@@ -81,10 +75,15 @@ void UserRegistry::refresh() {
                   "UserRegistry: cannot refresh, no data source");
     }
 
+    // If the source isn't open, open it.
     if (!source_->isOpen()) {
         source_->open();
     }
 
+    // Make a copy in case something goes wrong midstream.
+    UserMap backup(users_);
+
+    // Empty the registry then read users from source until source is empty.
     clearall();
     try {
         UserPtr user;
@@ -92,11 +91,15 @@ void UserRegistry::refresh() {
             addUser(user);
         }
     } catch (const std::exception& ex) {
+        // Source was compromsised so restore registry from backup.
+        users_ = backup;
+        // Close the source.
         source_->close();
         isc_throw (UserRegistryError, "UserRegistry: refresh failed during read"
                    << ex.what());
     }
 
+    // Close the source.
     source_->close();
 }
 
@@ -105,6 +108,11 @@ void UserRegistry::clearall() {
 }
 
 void UserRegistry::setSource(UserDataSourcePtr& source) {
+    if (!source) {
+        isc_throw (UserRegistryError,
+                   "UserRegistry: data source cannot be set to null");
+    }
+
     source_ = source;
 }
 

src/hooks/dhcp/user_chk/user_registry.h

@@ -14,6 +14,8 @@
 #ifndef _USER_REGISTRY_H
 #define _USER_REGISTRY_H
 
+/// @file user_registry.h Defines the class, UserRegistry.
+
 #include <dhcp/hwaddr.h>
 #include <dhcp/duid.h>
 #include <exceptions/exceptions.h>
@@ -32,40 +34,95 @@ public:
         isc::Exception(file, line, what) { };
 };
 
+/// @brief Defines a map of unique Users keyed by UserId.
 typedef std::map<UserId,UserPtr> UserMap;
 
+/// @brief Embodies an update-able, searchable list of unique users
+/// This class provides the means to create and maintain a searchable list
+/// of unique users. List entries are pointers to instances of User, keyed
+/// by their UserIds.
+/// Users may be added and removed from the list individually or the list
+/// may be updated by loading it from a data source, such as a file.
 class UserRegistry {
 public:
+    /// @brief Constructor
+    ///
+    /// Creates a new registry with an empty list of users and no data source.
     UserRegistry();
 
+    /// @brief Destructor
     ~UserRegistry();
 
+    /// @brief Adds a given user to the registry.
+    ///
+    /// @param user A pointer to the user to add
+    ///
+    /// @throw UserRegistryError if the user is null or if the user already
+    /// exists in the registry.
     void addUser(UserPtr& user);
 
+    /// @brief Finds a user in the registry by user id
+    ///
+    /// @param id The user id for which to search
+    ///
+    /// @return A pointer to the user if found or an null pointer if not.
     const UserPtr& findUser(const UserId& id) const;
 
+    /// @brief Removes a user from the registry by user id
+    ///
+    /// Removes the user entry if found, if not simply return.
+    ///
+    /// @param id The user id of the user to remove
     void removeUser(const UserId&  id);
 
+    /// @brief Finds a user in the registry by hardware address
+    ///
+    /// @param hwaddr The hardware address for which to search
+    ///
+    /// @return A pointer to the user if found or an null pointer if not.
     const UserPtr& findUser(const isc::dhcp::HWAddr& hwaddr) const;
 
-    const UserPtr& findUser(const isc::dhcp::ClientId& client_id) const;
-
+    /// @brief Finds a user in the registry by DUID
+    ///
+    /// @param duid The DUID for which to search
+    ///
+    /// @return A pointer to the user if found or an null pointer if not.
     const UserPtr& findUser(const isc::dhcp::DUID& duid) const;
 
+    /// @brief Updates the registry from its data source.
+    ///
+    /// This method will replace the contents of the registry with new content
+    /// read from its data source.  It will attempt to open the source and
+    /// then add users from the source to the registry until the source is
+    /// exhausted.  If an error occurs accessing the source the registry
+    /// contents will be restored to that of before the call to refresh.
+    ///
+    /// @throw UserRegistryError if the data source has not been set (is null)
+    /// or if an error occurs accessing the data source.
     void refresh();
 
+    /// @brief Removes all entries from the registry.
     void clearall();
 
+    /// @brief Returns a reference to the data source.
     const UserDataSourcePtr& getSource();
 
+    /// @brief Sets the data source to the given value.
+    ///
+    /// @param source reference to the data source to use.
+    ///
+    /// @throw UserRegistryError if new source value is null.
     void setSource(UserDataSourcePtr& source);
 
 private:
+    /// @brief The registry of users.
     UserMap users_;
 
+    /// @brief The current data source of users.
     UserDataSourcePtr source_;
 };
 
+/// @brief Define a smart pointer to a UserRegistry.
 typedef boost::shared_ptr<UserRegistry> UserRegistryPtr;
 
 #endif

src/hooks/dhcp/user_chk/version.cc

@@ -17,6 +17,7 @@
 
 extern "C" {
 
+/// @brief Version function required by Hooks API for compatibility checks.
 int version() {
     return (BIND10_HOOKS_VERSION);
 }