[#3831] Refactored FileManager into PathChecker

Refactored to internally support env variable and explicit paths
modified:
    src/lib/hooks/hooks_parser.cc
    src/lib/hooks/hooks_parser.h
    src/lib/util/filesystem.cc
    src/lib/util/filesystem.h
    src/lib/util/tests/filesystem_unittests.cc

src/lib/hooks/hooks_parser.cc

@@ -24,20 +24,31 @@ using namespace isc::util::file;
 namespace isc {
 namespace hooks {
 
+namespace {
+    // Singleton PathChecker to set and hold valid hooks library path.
+    PathCheckerPtr hooks_path_checker_;
+};
+
 std::string
 HooksLibrariesParser::getHooksPath(bool reset /* = false */, const std::string explicit_path /* = "" */) {
-    static std::string default_hooks_path = ""; 
+    if (!hooks_path_checker_ || reset) {
-    if (default_hooks_path.empty() || reset) {
+        hooks_path_checker_.reset(new PathChecker(DEFAULT_HOOKS_PATH, "KEA_HOOKS_PATH"));
-        if (explicit_path.empty()) {
+        if (!explicit_path.empty()) {
-            default_hooks_path = std::string(std::getenv("KEA_HOOKS_PATH") ?
+            hooks_path_checker_->getPath(true, explicit_path);
-                                             std::getenv("KEA_HOOKS_PATH")
-                                             : DEFAULT_HOOKS_PATH);
-        } else {
-            default_hooks_path = explicit_path;
         }
     }
 
-    return (default_hooks_path);
+    return (hooks_path_checker_->getPath());
+}
+
+std::string
+HooksLibrariesParser::validatePath(const std::string libpath,
+                                   bool enforce_path /* = true */) {
+    if (!hooks_path_checker_) {
+        getHooksPath();
+    }
+
+    return (hooks_path_checker_->validatePath(libpath, enforce_path));
 }
 
 // @todo use the flat style, split into list and item
@@ -119,12 +130,5 @@ HooksLibrariesParser::parse(HooksConfig& libraries, ConstElementPtr value) {
     }
 }
 
-std::string
-HooksLibrariesParser::validatePath(const std::string libpath,
-                                   bool enforce_path /* = true */) {
-    return (FileManager::validatePath(HooksLibrariesParser::getHooksPath(),
-                                      libpath, enforce_path));
-}
-
 }
 }

src/lib/hooks/hooks_parser.h

@@ -10,6 +10,7 @@
 #include <cc/data.h>
 #include <cc/simple_parser.h>
 #include <hooks/hooks_config.h>
+#include <util/filesystem.h>
 
 namespace isc {
 namespace hooks {

src/lib/util/filesystem.cc

@@ -221,16 +221,38 @@ string TemporaryDirectory::dirName() {
     return dir_name_;
 }
 
+PathChecker::PathChecker(const std::string default_path,
+                             const std::string env_name /* = "" */)
+    : default_path_(default_path), env_name_(env_name) {
+    getPath(true);
+}
+
 std::string
-FileManager::validatePath(const std::string supported_path_str, const std::string input_path_str,
+PathChecker::getPath(bool reset /* = false */,
-                          bool enforce_path /* = true */) {
+                     const std::string explicit_path /* = "" */) {
-    // Remove the trailing "/" if it present so comparison to
+    if (reset) {
-    // input's parent path functions.
+        if (!explicit_path.empty()) {
-    auto supported_path_copy(supported_path_str);
+            path_ = explicit_path;
-    if (supported_path_copy.back() == '/') {
+        } else if (!env_name_.empty()) {
-        supported_path_copy.pop_back();
+            path_ = std::string(std::getenv(env_name_.c_str()) ?
+                                std::getenv(env_name_.c_str()) : default_path_);
+        } else {
+            path_ = default_path_;
+        }
+
+        // Remove the trailing "/" if it present so comparison to
+        // other Path::parentPath() works.
+        if (path_.back() == '/') {
+            path_.pop_back();
+        }
     }
 
+    return (path_);
+}
+
+std::string
+PathChecker::validatePath(const std::string input_path_str,
+                          bool enforce_path /* = true */) const {
     Path input_path(trim(input_path_str));
     auto filename = input_path.filename();
     if (filename.empty()) {
@@ -246,17 +268,18 @@ FileManager::validatePath(const std::string supported_path_str, const std::strin
 
 
         // We only allow absolute path equal to default. Catch an invalid path.
-        if (parent_path != supported_path_copy) {
+        if (parent_path != path_) {
             isc_throw(BadValue, "invalid path specified: '"
                       << parent_path << "', supported path is '"
-                      << supported_path_copy << "'");
+                      << path_ << "'");
         }
     }
 
-    std::string valid_path(supported_path_copy + "/" +  filename);
+    std::string valid_path(path_ + "/" +  filename);
     return (valid_path);
 }
 
+
 }  // namespace file
 }  // namespace util
 }  // namespace isc

src/lib/util/filesystem.h

@@ -8,6 +8,7 @@
 #define KEA_UTIL_FILESYSTEM_H
 
 #include <string>
+#include <boost/shared_ptr.hpp>
 
 namespace isc {
 namespace util {
@@ -146,9 +147,39 @@ private:
     std::string dir_name_;
 };
 
-/// @brief Class that provides basic file related tasks.
+/// @brief Embodies a supported path against which file paths can be validated.
-class FileManager {
+class PathChecker {
 public:
+    /// @brief Constructor.
+    ///
+    /// Makes a call to getPath(true) to initialize the supported path.
+    ///
+    /// @param default_path path to use unless overidden by explicitly or via
+    /// environment variable.
+    /// @param env_name name of environment variable (if one), that can override
+    /// the default path.
+    PathChecker(const std::string default_path, const std::string env_name = "");
+
+    /// @brief Destructor.
+    virtual ~PathChecker() {};
+
+    /// @brief Fetches the supported path.
+    ///
+    /// When called with reset=true it will calculate the supported path as
+    /// follows:
+    ///
+    /// 1. Use the value of explicit_path parameter if not blank
+    /// 2. Use the value of the environment variable, if one is provided and it
+    /// is defined in the environment
+    /// 3. Use the value of default path.
+    ///
+    /// @param reset recalculate when true, defaults to false.
+    /// @param explicit_path set default hooks path to this value. This is
+    /// for testing purposes only.
+    ///
+    /// @return String containing the default hooks path.
+    std::string getPath(bool reset = false, const std::string explicit_path = "");
+
     /// @brief Validates a file path against a supported path.
     ///
     /// If the input path specifies a parent path and file name, the parent path
@@ -156,8 +187,6 @@ public:
     /// the validated path. If the input path contains only a file name the function
     /// returns valid path using the supported path and the input path name.
     ///
-    /// @param supported_path_str absolute path specifying the  supported path
-    /// of the file against which the input path is validated.
     /// @param input_path_str file path to validate.
     /// @param enforce_path enables validation against the supported path.  If false
     /// verifies only that the path contains a file name.
@@ -166,11 +195,33 @@ public:
     ///
     /// @throw BadValue if the input path does not include a file name or if the
     /// it the parent path does not path the supported path.
-    static std::string validatePath(const std::string supported_path_str,
+    std::string validatePath(const std::string input_path_str,
-                                    const std::string input_path_str,
+                             bool enforce_path = true) const;
-                                    bool enforce_path = true);
+
+    /// @brief Fetches the default path.
+    std::string getDefaultPath() const {
+        return (default_path_);
+    }
+
+    /// @brief Fetches the environment variable name.
+    std::string getEnvName() const {
+        return (env_name_);
+    }
+
+private:
+    /// @brief Default supported path.
+    std::string default_path_;
+
+    /// @brief Name of environment variable (if one) that can override the default.
+    std::string env_name_;
+
+    /// @brief The supported path currently in effect.
+    std::string path_;
 };
 
+/// @brief Defines a pointer to a PathChecker.
+typedef boost::shared_ptr<PathChecker> PathCheckerPtr;
+
 }  // namespace file
 }  // namespace util
 }  // namespace isc

src/lib/util/tests/filesystem_unittests.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2015-2024 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2015-2025 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -14,6 +14,7 @@
 #include <fstream>
 #include <list>
 #include <string>
+#include <cstdlib>
 
 #include <gtest/gtest.h>
 
@@ -159,8 +160,79 @@ TEST(PathTest, replaceParentPath) {
     EXPECT_EQ("/just/some/dir/a.b", fname.str());
 }
 
-// Verifies FileManager::validatePath() when enforce_path is true.
+/// @brief Test fixture for testing PathChecker.
-TEST(FileManager, validatePathEnforcePath) {
+class PathCheckerTest : public ::testing::Test {
+public:
+    /// @brief Constructor
+    PathCheckerTest() {
+        env_name_ = "KEA_PATCHECKER_TEST_PATH";
+        // Save current value of the environment path.
+        char* env_path = std::getenv(env_name_.c_str());
+        if (env_path) {
+            original_path_ = std::string(env_name_.c_str());
+        }
+
+        // Clear the environment path.
+        unsetenv(env_name_.c_str());
+    }
+
+    /// @brief Destructor
+    ~PathCheckerTest() {
+        // Restore the original environment path.
+        if (!original_path_.empty()) {
+            setenv(env_name_.c_str(), original_path_.c_str(), 1);
+        } else {
+            unsetenv(env_name_.c_str());
+        }
+    }
+
+    /// @brief Name of env variable for overriding default path.
+    std::string env_name_;
+
+    /// @brief Retains the environment variable's original value.
+    std::string original_path_;
+
+};
+
+TEST_F(PathCheckerTest, getPathDefault) {
+    // No environment variable or explicit path should
+    // return the default path after construction.
+    ASSERT_FALSE(std::getenv(env_name_.c_str()));
+    PathChecker checker("/tmp/def_path", env_name_.c_str());
+    ASSERT_EQ(checker.getPath(), checker.getDefaultPath());
+
+    // A subsequent call with reset=true should do the same.
+    EXPECT_EQ(checker.getPath(true), checker.getDefaultPath());
+}
+
+TEST_F(PathCheckerTest, getPathEnvVariable) {
+    // Override default with an env variable upon construction.
+    setenv(env_name_.c_str(), "/tmp/override", 1);
+    ASSERT_TRUE(std::getenv(env_name_.c_str()));
+    PathChecker checker("/tmp/def_path", env_name_.c_str());
+    ASSERT_EQ(checker.getPath(), "/tmp/override");
+
+    // A subsequent call with reset=true should do the same.
+    EXPECT_EQ(checker.getPath(true), "/tmp/override");
+}
+
+TEST_F(PathCheckerTest, getPathExplicit) {
+    // Default construction with no env variable.
+    ASSERT_FALSE(std::getenv(env_name_.c_str()));
+    PathChecker checker("/tmp/def_path", env_name_.c_str());
+    ASSERT_EQ(checker.getPath(), checker.getDefaultPath());
+
+    // A subsequent call wiht reset=true and an explicit path
+    // should return the explicit path.
+    ASSERT_EQ(checker.getPath(true, "/tmp/explicit"),
+              "/tmp/explicit");
+
+    // A subsequent call with no arguments should do the same.
+    EXPECT_EQ(checker.getPath(), "/tmp/explicit");
+}
+
+// Verifies PathChecker::validatePath() when enforce_path is true.
+TEST(PathChecker, validatePathEnforcePath) {
     std::string def_path(TEST_DATA_BUILDDIR);
     struct Scenario {
         int line_;
@@ -207,6 +279,8 @@ TEST(FileManager, validatePathEnforcePath) {
     }
     };
 
+    // Create a PathChecker with a supported path of def_path.
+    PathChecker checker(def_path);
     for (auto scenario : scenarios) {
         std::ostringstream oss;
         oss << " Scenario at line: " << scenario.line_;
@@ -214,18 +288,18 @@ TEST(FileManager, validatePathEnforcePath) {
         std::string validated_path;
         if (scenario.exp_error_.empty()) {
             ASSERT_NO_THROW_LOG(validated_path =
-                                FileManager::validatePath(def_path, scenario.lib_path_));
+                                checker.validatePath(scenario.lib_path_));
             EXPECT_EQ(validated_path, scenario.exp_path_);
         } else {
             ASSERT_THROW_MSG(validated_path =
-                             FileManager::validatePath(def_path, scenario.lib_path_),
+                             checker.validatePath(scenario.lib_path_),
                              BadValue, scenario.exp_error_);
         }
     }
 }
 
-// Verifies FileManager::validatePath() when enforce_path is false.
+// Verifies PathChecker::validatePath() when enforce_path is false.
-TEST(FileManager, validatePathEnforcePathFalse) {
+TEST(PathChecker, validatePathEnforcePathFalse) {
     std::string def_path(TEST_DATA_BUILDDIR);
     struct Scenario {
         int line_;
@@ -272,6 +346,8 @@ TEST(FileManager, validatePathEnforcePathFalse) {
     }
     };
 
+    // Create a PathChecker with a supported path of def_path.
+    PathChecker checker(def_path);
     for (auto scenario : scenarios) {
         std::ostringstream oss;
         oss << " Scenario at line: " << scenario.line_;
@@ -279,11 +355,11 @@ TEST(FileManager, validatePathEnforcePathFalse) {
         std::string validated_path;
         if (scenario.exp_error_.empty()) {
             ASSERT_NO_THROW_LOG(validated_path =
-                                FileManager::validatePath(def_path, scenario.lib_path_, false));
+                                checker.validatePath(scenario.lib_path_, false));
             EXPECT_EQ(validated_path, scenario.exp_path_);
         } else {
             ASSERT_THROW_MSG(validated_path =
-                             FileManager::validatePath(def_path, scenario.lib_path_, false),
+                             checker.validatePath(scenario.lib_path_, false),
                              BadValue, scenario.exp_error_);
         }
     }