From: Thomas Markwalder Date: Mon, 5 May 2025 14:39:09 +0000 (-0400) Subject: [#3831] Refactored FileManager into PathChecker X-Git-Tag: Kea-2.7.9~60 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=2f304f6ca74ad7ebd80ccfe15ba2fee7dbb43776;p=thirdparty%2Fkea.git [#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 --- diff --git a/src/lib/hooks/hooks_parser.cc b/src/lib/hooks/hooks_parser.cc index 1f0c4336c1..5e5d06e90f 100644 --- a/src/lib/hooks/hooks_parser.cc +++ b/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 (default_hooks_path.empty() || reset) { - if (explicit_path.empty()) { - default_hooks_path = std::string(std::getenv("KEA_HOOKS_PATH") ? - std::getenv("KEA_HOOKS_PATH") - : DEFAULT_HOOKS_PATH); - } else { - default_hooks_path = explicit_path; + if (!hooks_path_checker_ || reset) { + hooks_path_checker_.reset(new PathChecker(DEFAULT_HOOKS_PATH, "KEA_HOOKS_PATH")); + if (!explicit_path.empty()) { + hooks_path_checker_->getPath(true, 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)); -} - } } diff --git a/src/lib/hooks/hooks_parser.h b/src/lib/hooks/hooks_parser.h index fb99add71f..3ac675bd71 100644 --- a/src/lib/hooks/hooks_parser.h +++ b/src/lib/hooks/hooks_parser.h @@ -10,6 +10,7 @@ #include #include #include +#include namespace isc { namespace hooks { diff --git a/src/lib/util/filesystem.cc b/src/lib/util/filesystem.cc index 875b477641..793f9530b1 100644 --- a/src/lib/util/filesystem.cc +++ b/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, - bool enforce_path /* = true */) { - // Remove the trailing "/" if it present so comparison to - // input's parent path functions. - auto supported_path_copy(supported_path_str); - if (supported_path_copy.back() == '/') { - supported_path_copy.pop_back(); +PathChecker::getPath(bool reset /* = false */, + const std::string explicit_path /* = "" */) { + if (reset) { + if (!explicit_path.empty()) { + path_ = explicit_path; + } else if (!env_name_.empty()) { + 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 diff --git a/src/lib/util/filesystem.h b/src/lib/util/filesystem.h index 0ab788c566..1b4099b67d 100644 --- a/src/lib/util/filesystem.h +++ b/src/lib/util/filesystem.h @@ -8,6 +8,7 @@ #define KEA_UTIL_FILESYSTEM_H #include +#include namespace isc { namespace util { @@ -146,9 +147,39 @@ private: std::string dir_name_; }; -/// @brief Class that provides basic file related tasks. -class FileManager { +/// @brief Embodies a supported path against which file paths can be validated. +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, - const std::string input_path_str, - bool enforce_path = true); + std::string validatePath(const std::string input_path_str, + bool enforce_path = true) const; + + /// @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 PathCheckerPtr; + } // namespace file } // namespace util } // namespace isc diff --git a/src/lib/util/tests/filesystem_unittests.cc b/src/lib/util/tests/filesystem_unittests.cc index 3824d3ad3c..7cf2559082 100644 --- a/src/lib/util/tests/filesystem_unittests.cc +++ b/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 #include #include +#include #include @@ -159,8 +160,79 @@ TEST(PathTest, replaceParentPath) { EXPECT_EQ("/just/some/dir/a.b", fname.str()); } -// Verifies FileManager::validatePath() when enforce_path is true. -TEST(FileManager, validatePathEnforcePath) { +/// @brief Test fixture for testing PathChecker. +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. -TEST(FileManager, validatePathEnforcePathFalse) { +// Verifies PathChecker::validatePath() when enforce_path is false. +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_); } }