diff --git a/system/jlib/jptree.hpp b/system/jlib/jptree.hpp index 80888c8218d..e52db5eec1a 100644 --- a/system/jlib/jptree.hpp +++ b/system/jlib/jptree.hpp @@ -438,11 +438,14 @@ extern jlib_decl unsigned getPropertyTreeHash(const IPropertyTree & source, unsi //to not be modified and to remain valid and consistent until it is released. interface ISyncedPropertyTree : extends IInterface { +//The following functions check whether something is up to date before returning their values. + //Return a version-hash which changes whenever the property tree changes - so that a caller can determine whether it needs to update + virtual unsigned getVersion() const = 0; virtual const IPropertyTree * getTree() const = 0; virtual bool getProp(MemoryBuffer & result, const char * xpath) const = 0; virtual bool getProp(StringBuffer & result, const char * xpath) const = 0; - //Return a version-hash which changes whenever the property tree changes - so that a caller can determine whether it needs to update - virtual unsigned getVersion() const = 0; + +// The following functions return the current cached state - they do not force a check to see if the value is up to date virtual bool isStale() const = 0; // An indication that the property tree may be out of date because it couldn't be resynchronized. virtual bool isValid() const = 0; // Is the property tree non-null? Typically called at startup to check configuration is provided. }; diff --git a/system/jlib/jsecrets.cpp b/system/jlib/jsecrets.cpp index 47c58c41b53..c85516075b5 100644 --- a/system/jlib/jsecrets.cpp +++ b/system/jlib/jsecrets.cpp @@ -375,10 +375,11 @@ class SecretCacheEntry : public CInterface friend class SecretCache; public: - SecretCacheEntry(IPropertyTree * _contents, cache_timestamp _now) - : contents(_contents), contentTimestamp(_now), accessedTimestamp(_now), checkedTimestamp(_now) + //A cache entry is initally created that has a create an access time of now, but the checkTimestamp + //is set so that needsRefresh() will return true. + SecretCacheEntry(cache_timestamp _now) + : contentTimestamp(_now), accessedTimestamp(_now), checkedTimestamp(_now - 2 * secretTimeoutMs) { - updateHash(); } unsigned getHash() const @@ -474,7 +475,7 @@ class SecretCache else { //Insert an entry with a null value that is marked as out of date - result = new SecretCacheEntry(nullptr, now - 2 * secretTimeoutMs); + result = new SecretCacheEntry(now); secrets.emplace(secretKey, result); } return result; @@ -1155,7 +1156,6 @@ class CSecret final : public CInterfaceOf CSecret(const char *_category, const char * _name, const char * _vaultId, const char * _version, SecretCacheEntry * _secret) : category(_category), name(_name), vaultId(_vaultId), version(_version), secret(_secret) { - updateHash(); } virtual const IPropertyTree * getTree() const override; @@ -1182,7 +1182,7 @@ class CSecret final : public CInterfaceOf { CriticalBlock block(secretCs); checkUptoDate(); - return secretHash; + return secret->getHash(); } virtual bool isValid() const override { @@ -1191,7 +1191,6 @@ class CSecret final : public CInterfaceOf protected: void checkUptoDate() const; - void updateHash() const; protected: StringAttr category; @@ -1200,7 +1199,6 @@ class CSecret final : public CInterfaceOf StringAttr version; mutable CriticalSection secretCs; mutable SecretCacheEntry * secret; - mutable unsigned secretHash = 0; }; @@ -1225,7 +1223,6 @@ void CSecret::checkUptoDate() const //Check the secret is always returned consistently. It would be possible to call a slightly //more optimal function to refresh a secret, but this is simplest. assertex(secret == newSecret); - updateHash(); } catch (IException * e) { @@ -1236,11 +1233,6 @@ void CSecret::checkUptoDate() const } } -void CSecret::updateHash() const -{ - secretHash = secret->getHash(); -} - ISyncedPropertyTree * resolveSecret(const char *category, const char * name, const char * optVaultId, const char * optVersion) { validateCategoryName(category); diff --git a/testing/unittests/jlibtests.cpp b/testing/unittests/jlibtests.cpp index 6a074989d43..f66c2ed2e78 100644 --- a/testing/unittests/jlibtests.cpp +++ b/testing/unittests/jlibtests.cpp @@ -32,11 +32,14 @@ #include "jlzw.hpp" #include "jqueue.hpp" #include "jregexp.hpp" +#include "jsecrets.hpp" #include "jutil.hpp" #include "junicode.hpp" #include "unittests.hpp" +#define CPPUNIT_ASSERT_EQUAL_STR(x, y) CPPUNIT_ASSERT_EQUAL(std::string(x),std::string(y)) + static const unsigned oneMinute = 60000; // msec class JlibTraceTest : public CppUnit::TestFixture @@ -3611,4 +3614,175 @@ CPPUNIT_TEST_SUITE_REGISTRATION( JLibUnicodeTest ); CPPUNIT_TEST_SUITE_NAMED_REGISTRATION( JLibUnicodeTest, "JLibUnicodeTest" ); +class JLibSecretsTest : public CppUnit::TestFixture +{ +public: + CPPUNIT_TEST_SUITE(JLibSecretsTest); + CPPUNIT_TEST(setup); + CPPUNIT_TEST(testUpdate); + CPPUNIT_TEST_SUITE_END(); + + //Each test creates a different instance of the class(!) so member values cannot be used to pass items + //from one test to another + StringBuffer secretRoot; + +protected: + void checkSecret(const char * secret, const char * key, const char * expectedValue) + { + Owned match = getSecret("testing", secret); + CPPUNIT_ASSERT(match); + const char * secretValue = match->queryProp(key); + CPPUNIT_ASSERT(secretValue); + CPPUNIT_ASSERT_EQUAL_STR(secretValue, expectedValue); + } + + bool hasSecret(const char * name) + { + Owned match = getSecret("testing", name); + return match != nullptr; + } + + void initPath() + { + char cwd[1024]; + CPPUNIT_ASSERT(GetCurrentDirectory(1024, cwd)); + secretRoot.set(cwd).append(PATHSEPCHAR).append("unittest-secrets"); + secretRoot.append(PATHSEPCHAR).append("testing"); // catgegory + } + + void setup() + { + char cwd[1024]; + CPPUNIT_ASSERT(GetCurrentDirectory(1024, cwd)); + secretRoot.append(cwd).append(PATHSEPCHAR).append("unittest-secrets"); + + recursiveRemoveDirectory(secretRoot); + CPPUNIT_ASSERT(recursiveCreateDirectory(secretRoot.str())); + setSecretMount(secretRoot); + setSecretTimeout(100); // Set the timeout so we can check it is working. + + secretRoot.append(PATHSEPCHAR).append("testing"); // catgegory + CPPUNIT_ASSERT(recursiveCreateDirectory(secretRoot.str())); + } + + void testUpdate() + { + initPath(); // secretRoot needs to be called for each test + + CPPUNIT_ASSERT(!hasSecret("secret1")); + writeTestingSecret("secret1", "value", "secret1Value"); + //Secret should not appear yet - null should be cached. + CPPUNIT_ASSERT(!hasSecret("secret1")); + + Owned secret2 = resolveSecret("testing", "secret2", nullptr, nullptr); + CPPUNIT_ASSERT(!secret2->isValid()); + CPPUNIT_ASSERT(!secret2->isStale()); + + MilliSleep(50); + //Secret should not appear yet - null should be cached. + CPPUNIT_ASSERT(!hasSecret("secret1")); + CPPUNIT_ASSERT(!secret2->isValid()); + CPPUNIT_ASSERT(!secret2->isStale()); + + MilliSleep(100); + //Secret should not be updated - enough time has passed + checkSecret("secret1", "value", "secret1Value"); + CPPUNIT_ASSERT(!secret2->isValid()); + CPPUNIT_ASSERT(secret2->isStale()); + + Owned secret3 = resolveSecret("testing", "secret3", nullptr, nullptr); + unsigned version = secret3->getVersion(); + CPPUNIT_ASSERT(!secret3->isValid()); + CPPUNIT_ASSERT(!secret3->isStale()); + writeTestingSecret("secret3", "value", "secret3Value"); + CPPUNIT_ASSERT(!secret3->isValid()); + CPPUNIT_ASSERT_EQUAL(version, secret3->getVersion()); + + //After sleep new value should not have been picked up + MilliSleep(50); + CPPUNIT_ASSERT(!secret3->isValid()); + CPPUNIT_ASSERT_EQUAL(version, secret3->getVersion()); + + //After sleep new value should now have been picked up + MilliSleep(100); + checkSecret("secret3", "value", "secret3Value"); + unsigned version2 = secret3->getVersion(); + CPPUNIT_ASSERT(!secret3->isStale()); + CPPUNIT_ASSERT(secret3->isValid()); + CPPUNIT_ASSERT(version != version2); + + //Sleep and check that the hash value has not changed + MilliSleep(200); + checkSecret("secret3", "value", "secret3Value"); + CPPUNIT_ASSERT(!secret3->isStale()); + CPPUNIT_ASSERT(secret3->isValid()); + CPPUNIT_ASSERT_EQUAL(version2, secret3->getVersion()); + + //Remove the secret - should have no immediate effect + writeTestingSecret("secret3", "value", nullptr); + CPPUNIT_ASSERT(!secret3->isStale()); + CPPUNIT_ASSERT(secret3->isValid()); + CPPUNIT_ASSERT_EQUAL(version2, secret3->getVersion()); + + MilliSleep(50); + CPPUNIT_ASSERT(!secret3->isStale()); + CPPUNIT_ASSERT(secret3->isValid()); + CPPUNIT_ASSERT_EQUAL(version2, secret3->getVersion()); + checkSecret("secret3", "value", "secret3Value"); + + MilliSleep(100); + CPPUNIT_ASSERT(secret3->isStale()); // Value has gone, but the old value is still returned + CPPUNIT_ASSERT(secret3->isValid()); + CPPUNIT_ASSERT_EQUAL(version2, secret3->getVersion()); + checkSecret("secret3", "value", "secret3Value"); + + //Update the value = the change should not be seen until the cache entry expires + writeTestingSecret("secret3", "value", "secret3NewValue"); + CPPUNIT_ASSERT(secret3->isStale()); + CPPUNIT_ASSERT(secret3->isValid()); + CPPUNIT_ASSERT_EQUAL(version2, secret3->getVersion()); + checkSecret("secret3", "value", "secret3Value"); + + MilliSleep(50); + CPPUNIT_ASSERT(secret3->isStale()); + CPPUNIT_ASSERT(secret3->isValid()); + CPPUNIT_ASSERT_EQUAL(version2, secret3->getVersion()); + checkSecret("secret3", "value", "secret3Value"); + + MilliSleep(100); + //These functions do not check for up to date values, so they return the same as before + CPPUNIT_ASSERT(secret3->isStale()); // Value still appears to be out of date + CPPUNIT_ASSERT(secret3->isValid()); + + //This call should force the value to be updated + CPPUNIT_ASSERT(version2 != secret3->getVersion()); + CPPUNIT_ASSERT(!secret3->isStale()); // New value has now been picked up + CPPUNIT_ASSERT(secret3->isValid()); + checkSecret("secret3", "value", "secret3NewValue"); + } + + void writeTestingSecret(const char * secret, const char * key, const char * value) + { + StringBuffer filename; + filename.append(secretRoot).append(PATHSEPCHAR).append(secret); + CPPUNIT_ASSERT(recursiveCreateDirectory(filename.str())); + + filename.append(PATHSEPCHAR).append(key); + + Owned file = createIFile(filename.str()); + if (value) + { + Owned io = file->open(IFOcreate); + io->write(0, strlen(value), value); + } + else + file->remove(); + } +}; + +CPPUNIT_TEST_SUITE_REGISTRATION( JLibSecretsTest ); +CPPUNIT_TEST_SUITE_NAMED_REGISTRATION( JLibSecretsTest, "JLibSecretsTest" ); + + + #endif // _USE_CPPUNIT