From 667b8be701e4d7a89eaaa103f4453b809b5a7422 Mon Sep 17 00:00:00 2001 From: Justin Hiemstra Date: Thu, 27 Apr 2023 20:57:30 +0000 Subject: [PATCH 01/14] Add support for API-configurable cache home There has been desire to create a method by which users can configure where to store the key cache without setting the env var `$XDG_CACHE_HOME` or relying on the user to have a home dir. This is especially relevant in cases where the library is embedded in a daemon (e.g. XRootD or HTCondor) that may have a preferencial key cache location. This commit adds a familiar `config_set_str` and `config_get_str` API for passing a new cache home to the library through the use of the `keycache.cache_home` key. As with `config_set_int` and `config_get_int`, these new functions can be easily updated in the future as the project develops and new configuration keys are desired. This would entail adding a new check for the desired key and the logic to handle any values associated with that key. --- src/scitokens.cpp | 67 +++++++++++++++++++++++++++++++++--- src/scitokens.h | 17 ++++++++-- src/scitokens_cache.cpp | 17 +++++++--- src/scitokens_internal.cpp | 69 ++++++++++++++++++++++++++++++++++++++ src/scitokens_internal.h | 7 ++++ test/main.cpp | 45 +++++++++++++++++++++++++ 6 files changed, 211 insertions(+), 11 deletions(-) diff --git a/src/scitokens.cpp b/src/scitokens.cpp index f28dfff..35ed7ca 100644 --- a/src/scitokens.cpp +++ b/src/scitokens.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include "scitokens.h" @@ -9,9 +10,14 @@ /** * GLOBALS */ + +// Cache timeout config std::atomic_int configurer::Configuration::m_next_update_delta{600}; std::atomic_int configurer::Configuration::m_expiry_delta{4 * 24 * 3600}; +// SciTokens cache home config +std::string configurer::Configuration::m_cache_home{""}; + SciTokenKey scitoken_key_create(const char *key_id, const char *alg, const char *public_contents, const char *private_contents, char **err_msg) { @@ -958,7 +964,6 @@ int config_set_int(const char *key, int value, char **err_msg) { } std::string _key = key; - if (_key == "keycache.update_interval_s") { if (value < 0) { if (err_msg) { @@ -970,7 +975,7 @@ int config_set_int(const char *key, int value, char **err_msg) { return 0; } - if (_key == "keycache.expiration_interval_s") { + else if (_key == "keycache.expiration_interval_s") { if (value < 0 ) { if (err_msg) { *err_msg = strdup("Expiry interval must be positive."); @@ -998,12 +1003,11 @@ int config_get_int(const char *key, char **err_msg) { } std::string _key = key; - if (_key == "keycache.update_interval_s") { return configurer::Configuration::get_next_update_delta(); } - if (_key == "keycache.expiration_interval_s") { + else if (_key == "keycache.expiration_interval_s") { return configurer::Configuration::get_expiry_delta(); } @@ -1013,4 +1017,57 @@ int config_get_int(const char *key, char **err_msg) { } return -1; } -} \ No newline at end of file +} + +int config_set_str(const char *key, const char *value, char **err_msg) { + if (!key) { + if (err_msg) { + *err_msg = strdup("A key must be provided."); + } + return -1; + } + + std::string _key = key; + if (_key == "keycache.cache_home") { + auto rp = configurer::Configuration::set_cache_home(value); + if (!rp.first) { // There was an error, pass rp.second to err_msg + if (err_msg) { + *err_msg = strdup(rp.second.c_str()); + } + return -1; + } + } + + else { + if (err_msg) { + *err_msg = strdup("Key not recognized."); + } + return -1; + } + return 0; +} + +int config_get_str(const char *key, char **output, char **err_msg) { + if (!key) { + if (err_msg) { + *err_msg = strdup("A key must be provided."); + } + return -1; + } + + std::string _key = key; + if (_key == "keycache.cache_home") { + *output = strdup(configurer::Configuration::get_cache_home().c_str()); + } + + else { + if (err_msg) { + *err_msg = strdup("Key not recognized."); + } + return -1; + } + return 0; +} + + + diff --git a/src/scitokens.h b/src/scitokens.h index 55009fb..5a02f2a 100644 --- a/src/scitokens.h +++ b/src/scitokens.h @@ -295,7 +295,7 @@ int keycache_set_jwks(const char *issuer, const char *jwks, char **err_msg); */ /** - * Update scitokens parameters. + * Update scitokens int parameters. * Takes in key/value pairs and assigns the input value to whatever * configuration variable is indicated by the key. * Returns 0 on success, and non-zero for invalid keys or values. @@ -303,13 +303,26 @@ int keycache_set_jwks(const char *issuer, const char *jwks, char **err_msg); int config_set_int(const char *key, int value, char **err_msg); /** - * Get current scitokens parameters. + * Get current scitokens int parameters. * Returns the value associated with the supplied input key on success, and -1 * on failure This assumes there are no keys for which a negative return value * is permissible. */ int config_get_int(const char *key, char **err_msg); +/** + * Set current scitokens str parameters. + * Returns 0 on success, nonzero on failure + */ +int config_set_str(const char *key, const char *value, char **err_msg); + +/** + * Get current scitokens str parameters. + * Returns 0 on success, nonzero on failure, and populates the value associated + * with the input key to output. + */ +int config_get_str(const char *key, char **output, char **err_msg); + #ifdef __cplusplus } #endif diff --git a/src/scitokens_cache.cpp b/src/scitokens_cache.cpp index c9759a6..ab6af9a 100644 --- a/src/scitokens_cache.cpp +++ b/src/scitokens_cache.cpp @@ -42,9 +42,9 @@ void initialize_cachedb(const std::string &keycache_file) { /** * Get the Cache file location - * - * 1. $XDG_CACHE_HOME - * 2. .cache subdirectory of home directory as returned by the password + * 1. User-defined through config api + * 2. $XDG_CACHE_HOME + * 3. .cache subdirectory of home directory as returned by the password * database */ std::string get_cache_file() { @@ -64,7 +64,16 @@ std::string get_cache_file() { home_dir += "/.cache"; } - std::string cache_dir(xdg_cache_home ? xdg_cache_home : home_dir.c_str()); + // Figure out where to plop the cache based on priority + std::string cache_dir; + std::string configured_cache_dir = configurer::Configuration::get_cache_home(); + if (configured_cache_dir.length() > 0) { // The variable has been configured + cache_dir = configured_cache_dir; + } + else { + cache_dir = xdg_cache_home ? xdg_cache_home : home_dir.c_str(); + } + if (cache_dir.size() == 0) { return ""; } diff --git a/src/scitokens_internal.cpp b/src/scitokens_internal.cpp index 619384e..09968cb 100644 --- a/src/scitokens_internal.cpp +++ b/src/scitokens_internal.cpp @@ -2,6 +2,7 @@ #include #include #include +#include #include #include @@ -1064,3 +1065,71 @@ bool scitokens::Enforcer::scope_validator(const jwt::claim &claim, return me->m_test_authz.empty(); } + +// Configuration class functions +std::pair configurer::Configuration::set_cache_home(const std::string dir_path) { + // If setting to "", then we should treat as though it is unsetting the config + if (dir_path.length() == 0) { // User is configuring to empty string + m_cache_home = dir_path; + return std::make_pair(true, ""); + } + + // Check that the cache_home exists, and if not try to create it + if (!check_dir(dir_path)) { + if (!mkdir_and_parents_if_needed(dir_path)) { + return std::make_pair(false, "The provided cache home path did not exist and could not be created."); + } + } + + // Now it exists and we can write to it, set the value and let scitokens_cache handle the rest + m_cache_home = dir_path; + return std::make_pair(true, ""); +} + +std::string configurer::Configuration::get_cache_home() { + return m_cache_home; +} + +bool configurer::Configuration::check_dir(const std::string dir_path) { + struct stat info; + return stat(dir_path.c_str(), &info) == 0 && (info.st_mode & S_IFDIR); +} + +bool configurer::Configuration::mkdir_and_parents_if_needed(const std::string dir_path) { + // SciTokens-cpp already makes assumptions about using Linux file paths, + // so making that assumption here as well. + + // Using these perms because that's what the actual cache file uses in scitokens_cache + mode_t mode = 0700; // Maybe these permissions should be configurable? + + int result; + std::string currentLevel; + std::vector pathComponents = path_split(dir_path); + for (const auto &component : pathComponents) { + currentLevel += "/" + component; + if (!check_dir(currentLevel)) { + result = mkdir(currentLevel.c_str(), mode); + if (!result == 0) { + return false; + } + } + } + + return result == 0; +} + +std::vector configurer::Configuration::path_split(std::string path) { + std::vector pathComponents; + std::stringstream ss(path); + std::string component; + + while (std::getline(ss, component, '/')) { + pathComponents.push_back(component); + } + + if (pathComponents[0] == "") { + pathComponents.erase(pathComponents.begin()); + } + + return pathComponents; +} diff --git a/src/scitokens_internal.h b/src/scitokens_internal.h index cca4c4b..a27d3fe 100644 --- a/src/scitokens_internal.h +++ b/src/scitokens_internal.h @@ -42,9 +42,16 @@ class Configuration { m_expiry_delta = _expiry_delta; } static int get_expiry_delta() { return m_expiry_delta; } + static std::pair set_cache_home(const std::string cache_home); + static std::string get_cache_home(); private: static std::atomic_int m_next_update_delta; static std::atomic_int m_expiry_delta; + static std::string m_cache_home; + static bool check_dir(const std::string dir_path); + static bool mkdir_and_parents_if_needed(const std::string dir_path); + static std::vector path_split(const std::string dir_path); + }; } // namespace configurer diff --git a/test/main.cpp b/test/main.cpp index d8a6a22..20091c3 100644 --- a/test/main.cpp +++ b/test/main.cpp @@ -2,6 +2,7 @@ #include #include +#include namespace { @@ -165,6 +166,41 @@ TEST_F(KeycacheTest, SetGetTest) { EXPECT_EQ(demo_scitokens2, jwks_str); } +TEST_F(KeycacheTest, SetGetConfiguredCacheHome) { + // Set cache home and jwks + char cache_path[FILENAME_MAX]; + ASSERT_TRUE(getcwd(cache_path, sizeof(cache_path)) != nullptr); // Side effect gets cwd + char *err_msg; + std::string key = "keycache.cache_home"; + + auto rv = config_set_str(key.c_str(), cache_path, &err_msg); + ASSERT_TRUE(rv == 0) << err_msg; + + rv = keycache_set_jwks(demo_scitokens_url.c_str(), + demo_scitokens2.c_str(), &err_msg); + ASSERT_TRUE(rv == 0); + + char *jwks; + rv = keycache_get_cached_jwks(demo_scitokens_url.c_str(), &jwks, &err_msg); + ASSERT_TRUE(rv == 0); + ASSERT_TRUE(jwks != nullptr); + std::string jwks_str(jwks); + free(jwks); + + EXPECT_EQ(demo_scitokens2, jwks_str); + + // Get cache home + char *output; + rv = config_get_str(key.c_str(), &output, &err_msg); + ASSERT_TRUE(rv == 0) << err_msg; + EXPECT_EQ(*output, *cache_path); + free(output); + + // Reset cache home to whatever it was before by setting empty cnfg + rv = config_set_str(key.c_str(), "", &err_msg); + ASSERT_TRUE(rv == 0) << err_msg; +} + TEST_F(KeycacheTest, InvalidConfigKeyTest) { char *err_msg; int new_update_interval = 400; @@ -236,6 +272,15 @@ TEST_F(KeycacheTest, RefreshExpiredTest) { EXPECT_EQ(jwks_str, "{\"keys\": []}"); } +// TEST_F(KeycacheTest, SetGetCacheHomeTest) { +// char *err_msg, *jwks; +// std::string key = "keycache.cache_home"; +// std::string cache_path = "/workspaces/scitokens-cpp/my_config"; +// auto rv = config_set_str(key.c_str(), cache_path.c_str(), &err_msg); + +// // Now create the cache there +// } + class SerializeTest : public ::testing::Test { protected: void SetUp() override { From d802554d197a0315ee10f8332b96e9062f9a6216 Mon Sep 17 00:00:00 2001 From: Justin Hiemstra Date: Thu, 27 Apr 2023 21:21:44 +0000 Subject: [PATCH 02/14] Cleanup a few formatting discrepancies Getting rid of a few commented blurbs, cleaning up a few extraneous spaces, etc. --- src/scitokens.cpp | 3 --- src/scitokens_internal.h | 1 - test/main.cpp | 9 --------- 3 files changed, 13 deletions(-) diff --git a/src/scitokens.cpp b/src/scitokens.cpp index 35ed7ca..fd52b75 100644 --- a/src/scitokens.cpp +++ b/src/scitokens.cpp @@ -1068,6 +1068,3 @@ int config_get_str(const char *key, char **output, char **err_msg) { } return 0; } - - - diff --git a/src/scitokens_internal.h b/src/scitokens_internal.h index a27d3fe..ecec929 100644 --- a/src/scitokens_internal.h +++ b/src/scitokens_internal.h @@ -51,7 +51,6 @@ class Configuration { static bool check_dir(const std::string dir_path); static bool mkdir_and_parents_if_needed(const std::string dir_path); static std::vector path_split(const std::string dir_path); - }; } // namespace configurer diff --git a/test/main.cpp b/test/main.cpp index 20091c3..0694891 100644 --- a/test/main.cpp +++ b/test/main.cpp @@ -272,15 +272,6 @@ TEST_F(KeycacheTest, RefreshExpiredTest) { EXPECT_EQ(jwks_str, "{\"keys\": []}"); } -// TEST_F(KeycacheTest, SetGetCacheHomeTest) { -// char *err_msg, *jwks; -// std::string key = "keycache.cache_home"; -// std::string cache_path = "/workspaces/scitokens-cpp/my_config"; -// auto rv = config_set_str(key.c_str(), cache_path.c_str(), &err_msg); - -// // Now create the cache there -// } - class SerializeTest : public ::testing::Test { protected: void SetUp() override { From 90552dada554fb472949f13d88362fdf8d6c8c4c Mon Sep 17 00:00:00 2001 From: Justin Hiemstra Date: Thu, 27 Apr 2023 21:54:14 +0000 Subject: [PATCH 03/14] Incorporate feedback from linter --- src/scitokens.cpp | 3 +-- src/scitokens_cache.cpp | 6 +++--- src/scitokens_internal.cpp | 26 +++++++++++++++----------- src/scitokens_internal.h | 1 + test/main.cpp | 4 ++-- 5 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/scitokens.cpp b/src/scitokens.cpp index fd52b75..8be1e8a 100644 --- a/src/scitokens.cpp +++ b/src/scitokens.cpp @@ -3,7 +3,6 @@ #include #include - #include "scitokens.h" #include "scitokens_internal.h" @@ -976,7 +975,7 @@ int config_set_int(const char *key, int value, char **err_msg) { } else if (_key == "keycache.expiration_interval_s") { - if (value < 0 ) { + if (value < 0) { if (err_msg) { *err_msg = strdup("Expiry interval must be positive."); } diff --git a/src/scitokens_cache.cpp b/src/scitokens_cache.cpp index ab6af9a..c61ad6b 100644 --- a/src/scitokens_cache.cpp +++ b/src/scitokens_cache.cpp @@ -66,11 +66,11 @@ std::string get_cache_file() { // Figure out where to plop the cache based on priority std::string cache_dir; - std::string configured_cache_dir = configurer::Configuration::get_cache_home(); + std::string configured_cache_dir = + configurer::Configuration::get_cache_home(); if (configured_cache_dir.length() > 0) { // The variable has been configured cache_dir = configured_cache_dir; - } - else { + } else { cache_dir = xdg_cache_home ? xdg_cache_home : home_dir.c_str(); } diff --git a/src/scitokens_internal.cpp b/src/scitokens_internal.cpp index 09968cb..70dc28b 100644 --- a/src/scitokens_internal.cpp +++ b/src/scitokens_internal.cpp @@ -1067,8 +1067,10 @@ bool scitokens::Enforcer::scope_validator(const jwt::claim &claim, } // Configuration class functions -std::pair configurer::Configuration::set_cache_home(const std::string dir_path) { - // If setting to "", then we should treat as though it is unsetting the config +std::pair +configurer::Configuration::set_cache_home(const std::string dir_path) { + // If setting to "", then we should treat as though it is unsetting the + // config if (dir_path.length() == 0) { // User is configuring to empty string m_cache_home = dir_path; return std::make_pair(true, ""); @@ -1077,29 +1079,31 @@ std::pair configurer::Configuration::set_cache_home(const std // Check that the cache_home exists, and if not try to create it if (!check_dir(dir_path)) { if (!mkdir_and_parents_if_needed(dir_path)) { - return std::make_pair(false, "The provided cache home path did not exist and could not be created."); + return std::make_pair(false, "The provided cache home path did not " + "exist and could not be created."); } } - // Now it exists and we can write to it, set the value and let scitokens_cache handle the rest + // Now it exists and we can write to it, set the value and let + // scitokens_cache handle the rest m_cache_home = dir_path; return std::make_pair(true, ""); } -std::string configurer::Configuration::get_cache_home() { - return m_cache_home; -} +std::string configurer::Configuration::get_cache_home() { return m_cache_home; } bool configurer::Configuration::check_dir(const std::string dir_path) { struct stat info; return stat(dir_path.c_str(), &info) == 0 && (info.st_mode & S_IFDIR); } -bool configurer::Configuration::mkdir_and_parents_if_needed(const std::string dir_path) { +bool configurer::Configuration::mkdir_and_parents_if_needed( + const std::string dir_path) { // SciTokens-cpp already makes assumptions about using Linux file paths, // so making that assumption here as well. - // Using these perms because that's what the actual cache file uses in scitokens_cache + // Using these perms because that's what the actual cache file uses in + // scitokens_cache mode_t mode = 0700; // Maybe these permissions should be configurable? int result; @@ -1118,7 +1122,8 @@ bool configurer::Configuration::mkdir_and_parents_if_needed(const std::string di return result == 0; } -std::vector configurer::Configuration::path_split(std::string path) { +std::vector +configurer::Configuration::path_split(std::string path) { std::vector pathComponents; std::stringstream ss(path); std::string component; @@ -1130,6 +1135,5 @@ std::vector configurer::Configuration::path_split(std::string path) if (pathComponents[0] == "") { pathComponents.erase(pathComponents.begin()); } - return pathComponents; } diff --git a/src/scitokens_internal.h b/src/scitokens_internal.h index ecec929..998f01f 100644 --- a/src/scitokens_internal.h +++ b/src/scitokens_internal.h @@ -44,6 +44,7 @@ class Configuration { static int get_expiry_delta() { return m_expiry_delta; } static std::pair set_cache_home(const std::string cache_home); static std::string get_cache_home(); + private: static std::atomic_int m_next_update_delta; static std::atomic_int m_expiry_delta; diff --git a/test/main.cpp b/test/main.cpp index 0694891..98ddf84 100644 --- a/test/main.cpp +++ b/test/main.cpp @@ -176,8 +176,8 @@ TEST_F(KeycacheTest, SetGetConfiguredCacheHome) { auto rv = config_set_str(key.c_str(), cache_path, &err_msg); ASSERT_TRUE(rv == 0) << err_msg; - rv = keycache_set_jwks(demo_scitokens_url.c_str(), - demo_scitokens2.c_str(), &err_msg); + rv = keycache_set_jwks(demo_scitokens_url.c_str(), demo_scitokens2.c_str(), + &err_msg); ASSERT_TRUE(rv == 0); char *jwks; From 2b20b41fb8abb7a83b96ce4a24dbceee5defa5ad Mon Sep 17 00:00:00 2001 From: Justin Hiemstra Date: Fri, 28 Apr 2023 15:48:44 +0000 Subject: [PATCH 04/14] Switch `m_cache_home` to use be std::shared_ptr --- src/scitokens.cpp | 2 +- src/scitokens_cache.cpp | 2 +- src/scitokens_internal.cpp | 14 +++++++------- src/scitokens_internal.h | 4 ++-- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/scitokens.cpp b/src/scitokens.cpp index 8be1e8a..49ff550 100644 --- a/src/scitokens.cpp +++ b/src/scitokens.cpp @@ -15,7 +15,7 @@ std::atomic_int configurer::Configuration::m_next_update_delta{600}; std::atomic_int configurer::Configuration::m_expiry_delta{4 * 24 * 3600}; // SciTokens cache home config -std::string configurer::Configuration::m_cache_home{""}; +std::shared_ptr configurer::Configuration::m_cache_home = std::make_shared(""); SciTokenKey scitoken_key_create(const char *key_id, const char *alg, const char *public_contents, diff --git a/src/scitokens_cache.cpp b/src/scitokens_cache.cpp index c61ad6b..944f990 100644 --- a/src/scitokens_cache.cpp +++ b/src/scitokens_cache.cpp @@ -66,7 +66,7 @@ std::string get_cache_file() { // Figure out where to plop the cache based on priority std::string cache_dir; - std::string configured_cache_dir = + std::string configured_cache_dir = configurer::Configuration::get_cache_home(); if (configured_cache_dir.length() > 0) { // The variable has been configured cache_dir = configured_cache_dir; diff --git a/src/scitokens_internal.cpp b/src/scitokens_internal.cpp index 70dc28b..22f411f 100644 --- a/src/scitokens_internal.cpp +++ b/src/scitokens_internal.cpp @@ -1067,12 +1067,12 @@ bool scitokens::Enforcer::scope_validator(const jwt::claim &claim, } // Configuration class functions -std::pair +std::pair configurer::Configuration::set_cache_home(const std::string dir_path) { // If setting to "", then we should treat as though it is unsetting the // config if (dir_path.length() == 0) { // User is configuring to empty string - m_cache_home = dir_path; + m_cache_home = std::make_shared(dir_path); return std::make_pair(true, ""); } @@ -1084,13 +1084,13 @@ configurer::Configuration::set_cache_home(const std::string dir_path) { } } - // Now it exists and we can write to it, set the value and let + // Now it exists and we can write to it, set the value and let // scitokens_cache handle the rest - m_cache_home = dir_path; + m_cache_home = std::make_shared(dir_path); return std::make_pair(true, ""); } -std::string configurer::Configuration::get_cache_home() { return m_cache_home; } +std::string configurer::Configuration::get_cache_home() { return *m_cache_home; } bool configurer::Configuration::check_dir(const std::string dir_path) { struct stat info; @@ -1102,7 +1102,7 @@ bool configurer::Configuration::mkdir_and_parents_if_needed( // SciTokens-cpp already makes assumptions about using Linux file paths, // so making that assumption here as well. - // Using these perms because that's what the actual cache file uses in + // Using these perms because that's what the actual cache file uses in // scitokens_cache mode_t mode = 0700; // Maybe these permissions should be configurable? @@ -1122,7 +1122,7 @@ bool configurer::Configuration::mkdir_and_parents_if_needed( return result == 0; } -std::vector +std::vector configurer::Configuration::path_split(std::string path) { std::vector pathComponents; std::stringstream ss(path); diff --git a/src/scitokens_internal.h b/src/scitokens_internal.h index 998f01f..36ad6fe 100644 --- a/src/scitokens_internal.h +++ b/src/scitokens_internal.h @@ -44,11 +44,11 @@ class Configuration { static int get_expiry_delta() { return m_expiry_delta; } static std::pair set_cache_home(const std::string cache_home); static std::string get_cache_home(); - + private: static std::atomic_int m_next_update_delta; static std::atomic_int m_expiry_delta; - static std::string m_cache_home; + static std::shared_ptr m_cache_home; static bool check_dir(const std::string dir_path); static bool mkdir_and_parents_if_needed(const std::string dir_path); static std::vector path_split(const std::string dir_path); From 161384584c7e072a3fb19b68ce6ae0a6c946636c Mon Sep 17 00:00:00 2001 From: Justin Hiemstra Date: Wed, 3 May 2023 14:54:39 +0000 Subject: [PATCH 05/14] Change file path handling and propagate better error messages The previous code worked when configured to use a cache home like `///foo//bar`, but it wasn't set up to handle this situation robustly -- small future changes could have easily broken the functionality, so I modified the path parsing and directory checking functions to better handle this. I also changed the way error messages are propagated by these new config options to make sure that any errors thrown when calling `mkdir()` make it to the config API's err_msg output variable. This should make debugging a bit easier in any cases where someone unwittingly tries to plop their cache someplace where they don't have read/write access. --- src/scitokens_internal.cpp | 55 +++++++++++++++++++++----------------- src/scitokens_internal.h | 4 +-- test/main.cpp | 10 ++++--- 3 files changed, 39 insertions(+), 30 deletions(-) diff --git a/src/scitokens_internal.cpp b/src/scitokens_internal.cpp index 22f411f..183ace1 100644 --- a/src/scitokens_internal.cpp +++ b/src/scitokens_internal.cpp @@ -1076,28 +1076,34 @@ configurer::Configuration::set_cache_home(const std::string dir_path) { return std::make_pair(true, ""); } + std::vector path_components = path_split(dir_path); // cleans any extraneous /'s + std::string cleaned_dir_path; + for (const auto &component : path_components) { // add the / back to the path components + cleaned_dir_path += "/" + component; + } + // Check that the cache_home exists, and if not try to create it - if (!check_dir(dir_path)) { - if (!mkdir_and_parents_if_needed(dir_path)) { - return std::make_pair(false, "The provided cache home path did not " - "exist and could not be created."); - } + auto rp = mkdir_and_parents_if_needed(cleaned_dir_path); // Structured bindings not introduced until cpp 17 + if (!rp.first) { // + std::string err_prefix{"An issue was encountered with the provided cache home path: "}; + return std::make_pair(false, err_prefix + rp.second); } + // Now it exists and we can write to it, set the value and let // scitokens_cache handle the rest - m_cache_home = std::make_shared(dir_path); + m_cache_home = std::make_shared(cleaned_dir_path); return std::make_pair(true, ""); } std::string configurer::Configuration::get_cache_home() { return *m_cache_home; } -bool configurer::Configuration::check_dir(const std::string dir_path) { - struct stat info; - return stat(dir_path.c_str(), &info) == 0 && (info.st_mode & S_IFDIR); -} +// bool configurer::Configuration::check_dir(const std::string dir_path) { +// struct stat info; +// return stat(dir_path.c_str(), &info) == 0 && (info.st_mode & S_IFDIR); +// } -bool configurer::Configuration::mkdir_and_parents_if_needed( +std::pair configurer::Configuration::mkdir_and_parents_if_needed( const std::string dir_path) { // SciTokens-cpp already makes assumptions about using Linux file paths, // so making that assumption here as well. @@ -1108,32 +1114,33 @@ bool configurer::Configuration::mkdir_and_parents_if_needed( int result; std::string currentLevel; - std::vector pathComponents = path_split(dir_path); - for (const auto &component : pathComponents) { + std::vector path_components = path_split(dir_path); + for (const auto &component : path_components) { currentLevel += "/" + component; - if (!check_dir(currentLevel)) { - result = mkdir(currentLevel.c_str(), mode); - if (!result == 0) { - return false; - } + result = mkdir(currentLevel.c_str(), mode); + if ((result < 0) && errno != EEXIST) { + std::string err_prefix{"There was an error while creating/checking the directory: mkdir error: "}; + return std::make_pair(false, err_prefix + strerror(errno)); } } - return result == 0; + return std::make_pair(true, ""); } std::vector configurer::Configuration::path_split(std::string path) { - std::vector pathComponents; + std::vector path_components; std::stringstream ss(path); std::string component; while (std::getline(ss, component, '/')) { - pathComponents.push_back(component); + if (!component.empty()) { + path_components.push_back(component); + } } - if (pathComponents[0] == "") { - pathComponents.erase(pathComponents.begin()); + if (path_components[0] == "") { + path_components.erase(path_components.begin()); } - return pathComponents; + return path_components; } diff --git a/src/scitokens_internal.h b/src/scitokens_internal.h index 36ad6fe..986a5d5 100644 --- a/src/scitokens_internal.h +++ b/src/scitokens_internal.h @@ -49,8 +49,8 @@ class Configuration { static std::atomic_int m_next_update_delta; static std::atomic_int m_expiry_delta; static std::shared_ptr m_cache_home; - static bool check_dir(const std::string dir_path); - static bool mkdir_and_parents_if_needed(const std::string dir_path); + //static bool check_dir(const std::string dir_path); + static std::pair mkdir_and_parents_if_needed(const std::string dir_path); static std::vector path_split(const std::string dir_path); }; } // namespace configurer diff --git a/test/main.cpp b/test/main.cpp index 98ddf84..fe71c28 100644 --- a/test/main.cpp +++ b/test/main.cpp @@ -167,7 +167,7 @@ TEST_F(KeycacheTest, SetGetTest) { } TEST_F(KeycacheTest, SetGetConfiguredCacheHome) { - // Set cache home and jwks + // Set cache home char cache_path[FILENAME_MAX]; ASSERT_TRUE(getcwd(cache_path, sizeof(cache_path)) != nullptr); // Side effect gets cwd char *err_msg; @@ -176,10 +176,12 @@ TEST_F(KeycacheTest, SetGetConfiguredCacheHome) { auto rv = config_set_str(key.c_str(), cache_path, &err_msg); ASSERT_TRUE(rv == 0) << err_msg; + // Set the jwks at the new cache home rv = keycache_set_jwks(demo_scitokens_url.c_str(), demo_scitokens2.c_str(), &err_msg); - ASSERT_TRUE(rv == 0); + ASSERT_TRUE(rv == 0) << err_msg; + // Fetch the cached jwks from the new cache home char *jwks; rv = keycache_get_cached_jwks(demo_scitokens_url.c_str(), &jwks, &err_msg); ASSERT_TRUE(rv == 0); @@ -189,14 +191,14 @@ TEST_F(KeycacheTest, SetGetConfiguredCacheHome) { EXPECT_EQ(demo_scitokens2, jwks_str); - // Get cache home + // Check that cache home is still what was set char *output; rv = config_get_str(key.c_str(), &output, &err_msg); ASSERT_TRUE(rv == 0) << err_msg; EXPECT_EQ(*output, *cache_path); free(output); - // Reset cache home to whatever it was before by setting empty cnfg + // Reset cache home to whatever it was before by setting empty config rv = config_set_str(key.c_str(), "", &err_msg); ASSERT_TRUE(rv == 0) << err_msg; } From d0c6e334c14ae4debf62c2d1e5b2b159488eb6f3 Mon Sep 17 00:00:00 2001 From: jhiemstrawisc <75916364+jhiemstrawisc@users.noreply.github.com> Date: Wed, 3 May 2023 10:45:02 -0500 Subject: [PATCH 06/14] Apply suggestions from linter review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- src/scitokens_internal.cpp | 6 ++++-- src/scitokens_internal.h | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/scitokens_internal.cpp b/src/scitokens_internal.cpp index 183ace1..19e2d48 100644 --- a/src/scitokens_internal.cpp +++ b/src/scitokens_internal.cpp @@ -1103,7 +1103,8 @@ std::string configurer::Configuration::get_cache_home() { return *m_cache_home; // return stat(dir_path.c_str(), &info) == 0 && (info.st_mode & S_IFDIR); // } -std::pair configurer::Configuration::mkdir_and_parents_if_needed( +std::pair +configurer::Configuration::mkdir_and_parents_if_needed( const std::string dir_path) { // SciTokens-cpp already makes assumptions about using Linux file paths, // so making that assumption here as well. @@ -1119,7 +1120,8 @@ std::pair configurer::Configuration::mkdir_and_parents_if_nee currentLevel += "/" + component; result = mkdir(currentLevel.c_str(), mode); if ((result < 0) && errno != EEXIST) { - std::string err_prefix{"There was an error while creating/checking the directory: mkdir error: "}; + std::string err_prefix{"There was an error while creating/checking " + "the directory: mkdir error: "}; return std::make_pair(false, err_prefix + strerror(errno)); } } diff --git a/src/scitokens_internal.h b/src/scitokens_internal.h index 986a5d5..c843d41 100644 --- a/src/scitokens_internal.h +++ b/src/scitokens_internal.h @@ -49,8 +49,9 @@ class Configuration { static std::atomic_int m_next_update_delta; static std::atomic_int m_expiry_delta; static std::shared_ptr m_cache_home; - //static bool check_dir(const std::string dir_path); - static std::pair mkdir_and_parents_if_needed(const std::string dir_path); + // static bool check_dir(const std::string dir_path); + static std::pair + mkdir_and_parents_if_needed(const std::string dir_path); static std::vector path_split(const std::string dir_path); }; } // namespace configurer From c12e0c2d0c119fa67ca51130e2fad02f61b4e77c Mon Sep 17 00:00:00 2001 From: jhiemstrawisc <75916364+jhiemstrawisc@users.noreply.github.com> Date: Thu, 4 May 2023 08:39:32 -0500 Subject: [PATCH 07/14] Apply suggestions from linter review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- src/scitokens.cpp | 3 ++- src/scitokens_internal.cpp | 19 ++++++++++++------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/scitokens.cpp b/src/scitokens.cpp index 49ff550..b71b5b3 100644 --- a/src/scitokens.cpp +++ b/src/scitokens.cpp @@ -15,7 +15,8 @@ std::atomic_int configurer::Configuration::m_next_update_delta{600}; std::atomic_int configurer::Configuration::m_expiry_delta{4 * 24 * 3600}; // SciTokens cache home config -std::shared_ptr configurer::Configuration::m_cache_home = std::make_shared(""); +std::shared_ptr configurer::Configuration::m_cache_home = + std::make_shared(""); SciTokenKey scitoken_key_create(const char *key_id, const char *alg, const char *public_contents, diff --git a/src/scitokens_internal.cpp b/src/scitokens_internal.cpp index 19e2d48..2347d42 100644 --- a/src/scitokens_internal.cpp +++ b/src/scitokens_internal.cpp @@ -1076,27 +1076,32 @@ configurer::Configuration::set_cache_home(const std::string dir_path) { return std::make_pair(true, ""); } - std::vector path_components = path_split(dir_path); // cleans any extraneous /'s + std::vector path_components = + path_split(dir_path); // cleans any extraneous /'s std::string cleaned_dir_path; - for (const auto &component : path_components) { // add the / back to the path components + for (const auto &component : + path_components) { // add the / back to the path components cleaned_dir_path += "/" + component; } // Check that the cache_home exists, and if not try to create it - auto rp = mkdir_and_parents_if_needed(cleaned_dir_path); // Structured bindings not introduced until cpp 17 - if (!rp.first) { // - std::string err_prefix{"An issue was encountered with the provided cache home path: "}; + auto rp = mkdir_and_parents_if_needed( + cleaned_dir_path); // Structured bindings not introduced until cpp 17 + if (!rp.first) { // + std::string err_prefix{ + "An issue was encountered with the provided cache home path: "}; return std::make_pair(false, err_prefix + rp.second); } - // Now it exists and we can write to it, set the value and let // scitokens_cache handle the rest m_cache_home = std::make_shared(cleaned_dir_path); return std::make_pair(true, ""); } -std::string configurer::Configuration::get_cache_home() { return *m_cache_home; } +std::string configurer::Configuration::get_cache_home() { + return *m_cache_home; +} // bool configurer::Configuration::check_dir(const std::string dir_path) { // struct stat info; From 3486868f51991724543f06bcb853d0fb7f67ef10 Mon Sep 17 00:00:00 2001 From: Justin Hiemstra Date: Fri, 12 May 2023 14:35:38 +0000 Subject: [PATCH 08/14] Add duplicate config APIs with prefixed names Unfortunately, the choice of names for the config_set_blah() family of APIs is quite generic and poses the potential to cause C symbol collisions when SciTokens is used by large projects that dynamically load lots of libraries (a la HTCondor). A reasonable plan to address this, since those APIs are in use and changing their names would be API-breaking, seems to be adding a second set of prefixed APIs, with the intention of deprecating the old functions at the next major release. --- src/scitokens.cpp | 114 ++++++++++++++++++++++++++++++++++++++++++++++ src/scitokens.h | 12 +++++ 2 files changed, 126 insertions(+) diff --git a/src/scitokens.cpp b/src/scitokens.cpp index b71b5b3..08633e6 100644 --- a/src/scitokens.cpp +++ b/src/scitokens.cpp @@ -994,6 +994,45 @@ int config_set_int(const char *key, int value, char **err_msg) { } } +int scitokens_config_set_int(const char *key, int value, char **err_msg) { + if (!key) { + if (err_msg) { + *err_msg = strdup("A key must be provided."); + } + return -1; + } + + std::string _key = key; + if (_key == "keycache.update_interval_s") { + if (value < 0) { + if (err_msg) { + *err_msg = strdup("Update interval must be positive."); + } + return -1; + } + configurer::Configuration::set_next_update_delta(value); + return 0; + } + + else if (_key == "keycache.expiration_interval_s") { + if (value < 0) { + if (err_msg) { + *err_msg = strdup("Expiry interval must be positive."); + } + return -1; + } + configurer::Configuration::set_expiry_delta(value); + return 0; + } + + else { + if (err_msg) { + *err_msg = strdup("Key not recognized."); + } + return -1; + } +} + int config_get_int(const char *key, char **err_msg) { if (!key) { if (err_msg) { @@ -1019,6 +1058,31 @@ int config_get_int(const char *key, char **err_msg) { } } +int scitokens_config_get_int(const char *key, char **err_msg) { + if (!key) { + if (err_msg) { + *err_msg = strdup("A key must be provided."); + } + return -1; + } + + std::string _key = key; + if (_key == "keycache.update_interval_s") { + return configurer::Configuration::get_next_update_delta(); + } + + else if (_key == "keycache.expiration_interval_s") { + return configurer::Configuration::get_expiry_delta(); + } + + else { + if (err_msg) { + *err_msg = strdup("Key not recognized."); + } + return -1; + } +} + int config_set_str(const char *key, const char *value, char **err_msg) { if (!key) { if (err_msg) { @@ -1047,6 +1111,34 @@ int config_set_str(const char *key, const char *value, char **err_msg) { return 0; } +int scitokens_config_set_str(const char *key, const char *value, char **err_msg) { + if (!key) { + if (err_msg) { + *err_msg = strdup("A key must be provided."); + } + return -1; + } + + std::string _key = key; + if (_key == "keycache.cache_home") { + auto rp = configurer::Configuration::set_cache_home(value); + if (!rp.first) { // There was an error, pass rp.second to err_msg + if (err_msg) { + *err_msg = strdup(rp.second.c_str()); + } + return -1; + } + } + + else { + if (err_msg) { + *err_msg = strdup("Key not recognized."); + } + return -1; + } + return 0; +} + int config_get_str(const char *key, char **output, char **err_msg) { if (!key) { if (err_msg) { @@ -1068,3 +1160,25 @@ int config_get_str(const char *key, char **output, char **err_msg) { } return 0; } + +int scitokens_config_get_str(const char *key, char **output, char **err_msg) { + if (!key) { + if (err_msg) { + *err_msg = strdup("A key must be provided."); + } + return -1; + } + + std::string _key = key; + if (_key == "keycache.cache_home") { + *output = strdup(configurer::Configuration::get_cache_home().c_str()); + } + + else { + if (err_msg) { + *err_msg = strdup("Key not recognized."); + } + return -1; + } + return 0; +} diff --git a/src/scitokens.h b/src/scitokens.h index 5a02f2a..85781fa 100644 --- a/src/scitokens.h +++ b/src/scitokens.h @@ -302,6 +302,9 @@ int keycache_set_jwks(const char *issuer, const char *jwks, char **err_msg); */ int config_set_int(const char *key, int value, char **err_msg); +// Prefixed version of the same API to avoid potential symbol collisions +int scitokens_config_set_int(const char *key, int value, char **err_msg); + /** * Get current scitokens int parameters. * Returns the value associated with the supplied input key on success, and -1 @@ -310,12 +313,18 @@ int config_set_int(const char *key, int value, char **err_msg); */ int config_get_int(const char *key, char **err_msg); +// Prefixed version of the same API to avoid potential symbol collisions +int scitokens_config_get_int(const char *key, char **err_msg); + /** * Set current scitokens str parameters. * Returns 0 on success, nonzero on failure */ int config_set_str(const char *key, const char *value, char **err_msg); +// Prefixed version of the same API to avoid potential symbol collisions +int scitokens_config_set_str(const char *key, const char *value, char **err_msg); + /** * Get current scitokens str parameters. * Returns 0 on success, nonzero on failure, and populates the value associated @@ -323,6 +332,9 @@ int config_set_str(const char *key, const char *value, char **err_msg); */ int config_get_str(const char *key, char **output, char **err_msg); +// Prefixed version of the same API to avoid potential symbol collisions +int scitokens_config_get_str(const char *key, char **output, char **err_msg); + #ifdef __cplusplus } #endif From 37e2445f33b24694b084bbab816a8ddf475f176a Mon Sep 17 00:00:00 2001 From: jhiemstrawisc <75916364+jhiemstrawisc@users.noreply.github.com> Date: Fri, 12 May 2023 09:39:04 -0500 Subject: [PATCH 09/14] Apply line wrap suggestions from linter Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- src/scitokens.cpp | 3 ++- src/scitokens.h | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/scitokens.cpp b/src/scitokens.cpp index 08633e6..73ea9ae 100644 --- a/src/scitokens.cpp +++ b/src/scitokens.cpp @@ -1111,7 +1111,8 @@ int config_set_str(const char *key, const char *value, char **err_msg) { return 0; } -int scitokens_config_set_str(const char *key, const char *value, char **err_msg) { +int scitokens_config_set_str(const char *key, const char *value, + char **err_msg) { if (!key) { if (err_msg) { *err_msg = strdup("A key must be provided."); diff --git a/src/scitokens.h b/src/scitokens.h index 85781fa..544aa26 100644 --- a/src/scitokens.h +++ b/src/scitokens.h @@ -323,7 +323,8 @@ int scitokens_config_get_int(const char *key, char **err_msg); int config_set_str(const char *key, const char *value, char **err_msg); // Prefixed version of the same API to avoid potential symbol collisions -int scitokens_config_set_str(const char *key, const char *value, char **err_msg); +int scitokens_config_set_str(const char *key, const char *value, + char **err_msg); /** * Get current scitokens str parameters. From fffb45a9a4ccabc93d4aaaf91d249df77486bb58 Mon Sep 17 00:00:00 2001 From: Justin Hiemstra Date: Fri, 12 May 2023 15:19:30 +0000 Subject: [PATCH 10/14] Change config_set_blah() family of functions to call scitokens_config_set_blah() To cut down on code duplication, the config_set_blah() family of APIs have been changed to immediately call the scitokens-prefixed family of APIs under the hood. --- src/scitokens.cpp | 106 ++-------------------------------------------- 1 file changed, 4 insertions(+), 102 deletions(-) diff --git a/src/scitokens.cpp b/src/scitokens.cpp index 73ea9ae..3d9c7d2 100644 --- a/src/scitokens.cpp +++ b/src/scitokens.cpp @@ -956,42 +956,7 @@ int keycache_set_jwks(const char *issuer, const char *jwks, char **err_msg) { } int config_set_int(const char *key, int value, char **err_msg) { - if (!key) { - if (err_msg) { - *err_msg = strdup("A key must be provided."); - } - return -1; - } - - std::string _key = key; - if (_key == "keycache.update_interval_s") { - if (value < 0) { - if (err_msg) { - *err_msg = strdup("Update interval must be positive."); - } - return -1; - } - configurer::Configuration::set_next_update_delta(value); - return 0; - } - - else if (_key == "keycache.expiration_interval_s") { - if (value < 0) { - if (err_msg) { - *err_msg = strdup("Expiry interval must be positive."); - } - return -1; - } - configurer::Configuration::set_expiry_delta(value); - return 0; - } - - else { - if (err_msg) { - *err_msg = strdup("Key not recognized."); - } - return -1; - } + return scitokens_config_set_int(key, value, err_msg); } int scitokens_config_set_int(const char *key, int value, char **err_msg) { @@ -1034,28 +999,7 @@ int scitokens_config_set_int(const char *key, int value, char **err_msg) { } int config_get_int(const char *key, char **err_msg) { - if (!key) { - if (err_msg) { - *err_msg = strdup("A key must be provided."); - } - return -1; - } - - std::string _key = key; - if (_key == "keycache.update_interval_s") { - return configurer::Configuration::get_next_update_delta(); - } - - else if (_key == "keycache.expiration_interval_s") { - return configurer::Configuration::get_expiry_delta(); - } - - else { - if (err_msg) { - *err_msg = strdup("Key not recognized."); - } - return -1; - } + return scitokens_config_get_int(key, err_msg); } int scitokens_config_get_int(const char *key, char **err_msg) { @@ -1084,31 +1028,7 @@ int scitokens_config_get_int(const char *key, char **err_msg) { } int config_set_str(const char *key, const char *value, char **err_msg) { - if (!key) { - if (err_msg) { - *err_msg = strdup("A key must be provided."); - } - return -1; - } - - std::string _key = key; - if (_key == "keycache.cache_home") { - auto rp = configurer::Configuration::set_cache_home(value); - if (!rp.first) { // There was an error, pass rp.second to err_msg - if (err_msg) { - *err_msg = strdup(rp.second.c_str()); - } - return -1; - } - } - - else { - if (err_msg) { - *err_msg = strdup("Key not recognized."); - } - return -1; - } - return 0; + return scitokens_config_set_str(key, value, err_msg); } int scitokens_config_set_str(const char *key, const char *value, @@ -1141,25 +1061,7 @@ int scitokens_config_set_str(const char *key, const char *value, } int config_get_str(const char *key, char **output, char **err_msg) { - if (!key) { - if (err_msg) { - *err_msg = strdup("A key must be provided."); - } - return -1; - } - - std::string _key = key; - if (_key == "keycache.cache_home") { - *output = strdup(configurer::Configuration::get_cache_home().c_str()); - } - - else { - if (err_msg) { - *err_msg = strdup("Key not recognized."); - } - return -1; - } - return 0; + return scitokens_config_get_str(key, output, err_msg); } int scitokens_config_get_str(const char *key, char **output, char **err_msg) { From 38af01c1befa7ddb2f852d9a231a32abcf7380ad Mon Sep 17 00:00:00 2001 From: Justin Hiemstra Date: Mon, 15 May 2023 14:32:47 +0000 Subject: [PATCH 11/14] Update internal documentation to emphasize future API deprecation Per Jaime's comment, since we plan to deprecate the `config_set_blah()` family of functions in favor of the prefixed versions, I've updated the internal API documentation to point people toward the preferred APIs. --- src/scitokens.h | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/scitokens.h b/src/scitokens.h index 544aa26..07f0c42 100644 --- a/src/scitokens.h +++ b/src/scitokens.h @@ -291,49 +291,49 @@ int keycache_get_cached_jwks(const char *issuer, char **jwks, char **err_msg); int keycache_set_jwks(const char *issuer, const char *jwks, char **err_msg); /** - * API for managing scitokens configuration parameters. + * APIs for managing scitokens configuration parameters. */ +// On its way to deprecation +int config_set_int(const char *key, int value, char **err_msg); + /** * Update scitokens int parameters. * Takes in key/value pairs and assigns the input value to whatever * configuration variable is indicated by the key. * Returns 0 on success, and non-zero for invalid keys or values. */ -int config_set_int(const char *key, int value, char **err_msg); - -// Prefixed version of the same API to avoid potential symbol collisions int scitokens_config_set_int(const char *key, int value, char **err_msg); +// On its way to deprecation +int config_get_int(const char *key, char **err_msg); + /** * Get current scitokens int parameters. * Returns the value associated with the supplied input key on success, and -1 - * on failure This assumes there are no keys for which a negative return value + * on failure. This assumes there are no keys for which a negative return value * is permissible. */ -int config_get_int(const char *key, char **err_msg); - -// Prefixed version of the same API to avoid potential symbol collisions int scitokens_config_get_int(const char *key, char **err_msg); +// On its way to deprecation +int config_set_str(const char *key, const char *value, char **err_msg); + /** * Set current scitokens str parameters. * Returns 0 on success, nonzero on failure */ -int config_set_str(const char *key, const char *value, char **err_msg); - -// Prefixed version of the same API to avoid potential symbol collisions int scitokens_config_set_str(const char *key, const char *value, char **err_msg); +// On its way to deprecation +int config_get_str(const char *key, char **output, char **err_msg); + /** * Get current scitokens str parameters. * Returns 0 on success, nonzero on failure, and populates the value associated * with the input key to output. */ -int config_get_str(const char *key, char **output, char **err_msg); - -// Prefixed version of the same API to avoid potential symbol collisions int scitokens_config_get_str(const char *key, char **output, char **err_msg); #ifdef __cplusplus From c06777594ac51d7fa636e71e8b463c37e3ec09fb Mon Sep 17 00:00:00 2001 From: Justin Hiemstra Date: Tue, 16 May 2023 13:39:34 +0000 Subject: [PATCH 12/14] Remove config_set/get APIs and change prefixed from scitokens to scitoken It looks like the config_set/get_blah APIs weren't actually in use and hadn't been released, so instead of keeping them around, we decided to just delete them. Also, the `scitokens`-prefixed versions of these APIs were changed to be prefixed by `scitoken` to match other functions in the library. --- src/scitokens.cpp | 24 ++++-------------------- src/scitokens.h | 20 ++++---------------- test/main.cpp | 24 ++++++++++++------------ 3 files changed, 20 insertions(+), 48 deletions(-) diff --git a/src/scitokens.cpp b/src/scitokens.cpp index 3d9c7d2..6c5f37f 100644 --- a/src/scitokens.cpp +++ b/src/scitokens.cpp @@ -955,11 +955,7 @@ int keycache_set_jwks(const char *issuer, const char *jwks, char **err_msg) { return 0; } -int config_set_int(const char *key, int value, char **err_msg) { - return scitokens_config_set_int(key, value, err_msg); -} - -int scitokens_config_set_int(const char *key, int value, char **err_msg) { +int scitoken_config_set_int(const char *key, int value, char **err_msg) { if (!key) { if (err_msg) { *err_msg = strdup("A key must be provided."); @@ -998,11 +994,7 @@ int scitokens_config_set_int(const char *key, int value, char **err_msg) { } } -int config_get_int(const char *key, char **err_msg) { - return scitokens_config_get_int(key, err_msg); -} - -int scitokens_config_get_int(const char *key, char **err_msg) { +int scitoken_config_get_int(const char *key, char **err_msg) { if (!key) { if (err_msg) { *err_msg = strdup("A key must be provided."); @@ -1027,11 +1019,7 @@ int scitokens_config_get_int(const char *key, char **err_msg) { } } -int config_set_str(const char *key, const char *value, char **err_msg) { - return scitokens_config_set_str(key, value, err_msg); -} - -int scitokens_config_set_str(const char *key, const char *value, +int scitoken_config_set_str(const char *key, const char *value, char **err_msg) { if (!key) { if (err_msg) { @@ -1060,11 +1048,7 @@ int scitokens_config_set_str(const char *key, const char *value, return 0; } -int config_get_str(const char *key, char **output, char **err_msg) { - return scitokens_config_get_str(key, output, err_msg); -} - -int scitokens_config_get_str(const char *key, char **output, char **err_msg) { +int scitoken_config_get_str(const char *key, char **output, char **err_msg) { if (!key) { if (err_msg) { *err_msg = strdup("A key must be provided."); diff --git a/src/scitokens.h b/src/scitokens.h index 07f0c42..2505163 100644 --- a/src/scitokens.h +++ b/src/scitokens.h @@ -294,19 +294,13 @@ int keycache_set_jwks(const char *issuer, const char *jwks, char **err_msg); * APIs for managing scitokens configuration parameters. */ -// On its way to deprecation -int config_set_int(const char *key, int value, char **err_msg); - /** * Update scitokens int parameters. * Takes in key/value pairs and assigns the input value to whatever * configuration variable is indicated by the key. * Returns 0 on success, and non-zero for invalid keys or values. */ -int scitokens_config_set_int(const char *key, int value, char **err_msg); - -// On its way to deprecation -int config_get_int(const char *key, char **err_msg); +int scitoken_config_set_int(const char *key, int value, char **err_msg); /** * Get current scitokens int parameters. @@ -314,27 +308,21 @@ int config_get_int(const char *key, char **err_msg); * on failure. This assumes there are no keys for which a negative return value * is permissible. */ -int scitokens_config_get_int(const char *key, char **err_msg); - -// On its way to deprecation -int config_set_str(const char *key, const char *value, char **err_msg); +int scitoken_config_get_int(const char *key, char **err_msg); /** * Set current scitokens str parameters. * Returns 0 on success, nonzero on failure */ -int scitokens_config_set_str(const char *key, const char *value, +int scitoken_config_set_str(const char *key, const char *value, char **err_msg); -// On its way to deprecation -int config_get_str(const char *key, char **output, char **err_msg); - /** * Get current scitokens str parameters. * Returns 0 on success, nonzero on failure, and populates the value associated * with the input key to output. */ -int scitokens_config_get_str(const char *key, char **output, char **err_msg); +int scitoken_config_get_str(const char *key, char **output, char **err_msg); #ifdef __cplusplus } diff --git a/test/main.cpp b/test/main.cpp index fe71c28..5530e5c 100644 --- a/test/main.cpp +++ b/test/main.cpp @@ -173,7 +173,7 @@ TEST_F(KeycacheTest, SetGetConfiguredCacheHome) { char *err_msg; std::string key = "keycache.cache_home"; - auto rv = config_set_str(key.c_str(), cache_path, &err_msg); + auto rv = scitoken_config_set_str(key.c_str(), cache_path, &err_msg); ASSERT_TRUE(rv == 0) << err_msg; // Set the jwks at the new cache home @@ -193,13 +193,13 @@ TEST_F(KeycacheTest, SetGetConfiguredCacheHome) { // Check that cache home is still what was set char *output; - rv = config_get_str(key.c_str(), &output, &err_msg); + rv = scitoken_config_get_str(key.c_str(), &output, &err_msg); ASSERT_TRUE(rv == 0) << err_msg; EXPECT_EQ(*output, *cache_path); free(output); // Reset cache home to whatever it was before by setting empty config - rv = config_set_str(key.c_str(), "", &err_msg); + rv = scitoken_config_set_str(key.c_str(), "", &err_msg); ASSERT_TRUE(rv == 0) << err_msg; } @@ -207,11 +207,11 @@ TEST_F(KeycacheTest, InvalidConfigKeyTest) { char *err_msg; int new_update_interval = 400; std::string key = "invalid key"; - auto rv = config_set_int(key.c_str(), new_update_interval, &err_msg); + auto rv = scitoken_config_set_int(key.c_str(), new_update_interval, &err_msg); ASSERT_FALSE(rv == 0); const char *key2 = nullptr; - rv = config_set_int(key2, new_update_interval, &err_msg); + rv = scitoken_config_set_int(key2, new_update_interval, &err_msg); ASSERT_FALSE(rv == 0); } @@ -219,10 +219,10 @@ TEST_F(KeycacheTest, SetGetUpdateTest) { char *err_msg; int new_update_interval = 400; std::string key = "keycache.update_interval_s"; - auto rv = config_set_int(key.c_str(), new_update_interval, &err_msg); + auto rv = scitoken_config_set_int(key.c_str(), new_update_interval, &err_msg); ASSERT_TRUE(rv == 0); - rv = config_get_int(key.c_str(), &err_msg); + rv = scitoken_config_get_int(key.c_str(), &err_msg); EXPECT_EQ(rv, new_update_interval); } @@ -230,10 +230,10 @@ TEST_F(KeycacheTest, SetGetExpirationTest) { char *err_msg; int new_expiration_interval = 2 * 24 * 3600; std::string key = "keycache.expiration_interval_s"; - auto rv = config_set_int(key.c_str(), new_expiration_interval, &err_msg); + auto rv = scitoken_config_set_int(key.c_str(), new_expiration_interval, &err_msg); ASSERT_TRUE(rv == 0); - rv = config_get_int(key.c_str(), &err_msg); + rv = scitoken_config_get_int(key.c_str(), &err_msg); EXPECT_EQ(rv, new_expiration_interval); } @@ -241,7 +241,7 @@ TEST_F(KeycacheTest, SetInvalidUpdateTest) { char *err_msg; int new_update_interval = -1; std::string key = "keycache.update_interval_s"; - auto rv = config_set_int(key.c_str(), new_update_interval, &err_msg); + auto rv = scitoken_config_set_int(key.c_str(), new_update_interval, &err_msg); ASSERT_FALSE(rv == 0); } @@ -249,7 +249,7 @@ TEST_F(KeycacheTest, SetInvalidExpirationTest) { char *err_msg; int new_expiration_interval = -2 * 24 * 3600; std::string key = "keycache.expiration_interval_s"; - auto rv = config_set_int(key.c_str(), new_expiration_interval, &err_msg); + auto rv = scitoken_config_set_int(key.c_str(), new_expiration_interval, &err_msg); ASSERT_FALSE(rv == 0); } @@ -257,7 +257,7 @@ TEST_F(KeycacheTest, RefreshExpiredTest) { char *err_msg, *jwks; int new_expiration_interval = 0; std::string key = "keycache.expiration_interval_s"; - auto rv = config_set_int(key.c_str(), new_expiration_interval, &err_msg); + auto rv = scitoken_config_set_int(key.c_str(), new_expiration_interval, &err_msg); ASSERT_TRUE(rv == 0); rv = keycache_refresh_jwks(demo_scitokens_url.c_str(), &err_msg); From 8d74f679bf76d4811de108a5c9cb25539432b729 Mon Sep 17 00:00:00 2001 From: jhiemstrawisc <75916364+jhiemstrawisc@users.noreply.github.com> Date: Tue, 16 May 2023 08:44:38 -0500 Subject: [PATCH 13/14] Apply suggestions from linter review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- src/scitokens.cpp | 2 +- src/scitokens.h | 3 +-- test/main.cpp | 18 ++++++++++++------ 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/scitokens.cpp b/src/scitokens.cpp index 6c5f37f..fe4d458 100644 --- a/src/scitokens.cpp +++ b/src/scitokens.cpp @@ -1020,7 +1020,7 @@ int scitoken_config_get_int(const char *key, char **err_msg) { } int scitoken_config_set_str(const char *key, const char *value, - char **err_msg) { + char **err_msg) { if (!key) { if (err_msg) { *err_msg = strdup("A key must be provided."); diff --git a/src/scitokens.h b/src/scitokens.h index 2505163..84e0916 100644 --- a/src/scitokens.h +++ b/src/scitokens.h @@ -314,8 +314,7 @@ int scitoken_config_get_int(const char *key, char **err_msg); * Set current scitokens str parameters. * Returns 0 on success, nonzero on failure */ -int scitoken_config_set_str(const char *key, const char *value, - char **err_msg); +int scitoken_config_set_str(const char *key, const char *value, char **err_msg); /** * Get current scitokens str parameters. diff --git a/test/main.cpp b/test/main.cpp index 5530e5c..d37a142 100644 --- a/test/main.cpp +++ b/test/main.cpp @@ -207,7 +207,8 @@ TEST_F(KeycacheTest, InvalidConfigKeyTest) { char *err_msg; int new_update_interval = 400; std::string key = "invalid key"; - auto rv = scitoken_config_set_int(key.c_str(), new_update_interval, &err_msg); + auto rv = + scitoken_config_set_int(key.c_str(), new_update_interval, &err_msg); ASSERT_FALSE(rv == 0); const char *key2 = nullptr; @@ -219,7 +220,8 @@ TEST_F(KeycacheTest, SetGetUpdateTest) { char *err_msg; int new_update_interval = 400; std::string key = "keycache.update_interval_s"; - auto rv = scitoken_config_set_int(key.c_str(), new_update_interval, &err_msg); + auto rv = + scitoken_config_set_int(key.c_str(), new_update_interval, &err_msg); ASSERT_TRUE(rv == 0); rv = scitoken_config_get_int(key.c_str(), &err_msg); @@ -230,7 +232,8 @@ TEST_F(KeycacheTest, SetGetExpirationTest) { char *err_msg; int new_expiration_interval = 2 * 24 * 3600; std::string key = "keycache.expiration_interval_s"; - auto rv = scitoken_config_set_int(key.c_str(), new_expiration_interval, &err_msg); + auto rv = + scitoken_config_set_int(key.c_str(), new_expiration_interval, &err_msg); ASSERT_TRUE(rv == 0); rv = scitoken_config_get_int(key.c_str(), &err_msg); @@ -241,7 +244,8 @@ TEST_F(KeycacheTest, SetInvalidUpdateTest) { char *err_msg; int new_update_interval = -1; std::string key = "keycache.update_interval_s"; - auto rv = scitoken_config_set_int(key.c_str(), new_update_interval, &err_msg); + auto rv = + scitoken_config_set_int(key.c_str(), new_update_interval, &err_msg); ASSERT_FALSE(rv == 0); } @@ -249,7 +253,8 @@ TEST_F(KeycacheTest, SetInvalidExpirationTest) { char *err_msg; int new_expiration_interval = -2 * 24 * 3600; std::string key = "keycache.expiration_interval_s"; - auto rv = scitoken_config_set_int(key.c_str(), new_expiration_interval, &err_msg); + auto rv = + scitoken_config_set_int(key.c_str(), new_expiration_interval, &err_msg); ASSERT_FALSE(rv == 0); } @@ -257,7 +262,8 @@ TEST_F(KeycacheTest, RefreshExpiredTest) { char *err_msg, *jwks; int new_expiration_interval = 0; std::string key = "keycache.expiration_interval_s"; - auto rv = scitoken_config_set_int(key.c_str(), new_expiration_interval, &err_msg); + auto rv = + scitoken_config_set_int(key.c_str(), new_expiration_interval, &err_msg); ASSERT_TRUE(rv == 0); rv = keycache_refresh_jwks(demo_scitokens_url.c_str(), &err_msg); From 6cb9b2c4e26eed92064fb26d6c2ed840ffabc7f1 Mon Sep 17 00:00:00 2001 From: Justin Hiemstra Date: Tue, 16 May 2023 14:46:17 +0000 Subject: [PATCH 14/14] Restore `config_set/get_int` I mistakenly removed the `config_set/get_int` APIs when removing `config_set/get_str`. --- src/scitokens.cpp | 8 ++++++++ src/scitokens.h | 6 ++++++ 2 files changed, 14 insertions(+) diff --git a/src/scitokens.cpp b/src/scitokens.cpp index fe4d458..6c79ab8 100644 --- a/src/scitokens.cpp +++ b/src/scitokens.cpp @@ -955,6 +955,10 @@ int keycache_set_jwks(const char *issuer, const char *jwks, char **err_msg) { return 0; } +int config_set_int(const char *key, int value, char **err_msg) { + return scitoken_config_set_int(key, value, err_msg); +} + int scitoken_config_set_int(const char *key, int value, char **err_msg) { if (!key) { if (err_msg) { @@ -994,6 +998,10 @@ int scitoken_config_set_int(const char *key, int value, char **err_msg) { } } +int config_get_int(const char *key, char **err_msg) { + return scitoken_config_get_int(key, err_msg); +} + int scitoken_config_get_int(const char *key, char **err_msg) { if (!key) { if (err_msg) { diff --git a/src/scitokens.h b/src/scitokens.h index 84e0916..b87003a 100644 --- a/src/scitokens.h +++ b/src/scitokens.h @@ -294,6 +294,9 @@ int keycache_set_jwks(const char *issuer, const char *jwks, char **err_msg); * APIs for managing scitokens configuration parameters. */ +// On its way to deprecation +int config_set_int(const char *key, int value, char **err_msg); + /** * Update scitokens int parameters. * Takes in key/value pairs and assigns the input value to whatever @@ -302,6 +305,9 @@ int keycache_set_jwks(const char *issuer, const char *jwks, char **err_msg); */ int scitoken_config_set_int(const char *key, int value, char **err_msg); +// on its way to deprecation +int config_get_int(const char *key, char **err_msg); + /** * Get current scitokens int parameters. * Returns the value associated with the supplied input key on success, and -1