Open
Conversation
Contributor
Reviewer's GuideAdds propagation of LogContext (including a correlation ID) through authentication and database lookup paths, and updates dbGetRaw to attach the correlation ID as an HTTP header, with corresponding interface, implementation, and test adjustments. Sequence diagram for authentication flow with LogContext and dbGetRawsequenceDiagram
actor Client
participant AuthenticationOperator
participant AuthenticationManager
participant AuthMap
participant DatabaseAPI
participant DatabaseService
Client->>AuthenticationOperator: execute(message)
AuthenticationOperator->>AuthenticationOperator: generate uuid
AuthenticationOperator->>AuthenticationOperator: create LogContext
AuthenticationOperator->>AuthenticationManager: hasKey(key, log_context)
AuthenticationManager->>AuthMap: hasKey(PublicKeyType_TRANSIENT, key, log_context)
alt key_not_found_transient
AuthenticationManager->>AuthMap: hasKey(PublicKeyType_SESSION, key, log_context)
alt key_not_found_session
AuthenticationManager->>AuthMap: hasKey(PublicKeyType_PERSISTENT, key, log_context)
alt key_found_persistent
AuthenticationManager-->>AuthenticationOperator: true
else key_not_found_any
AuthenticationManager-->>AuthenticationOperator: false
end
else key_found_session
AuthenticationManager-->>AuthenticationOperator: true
end
else key_found_transient
AuthenticationManager-->>AuthenticationOperator: true
end
opt key_found
AuthenticationOperator->>AuthenticationManager: incrementKeyAccessCounter(key, log_context)
AuthenticationManager->>AuthMap: incrementKeyAccessCounter(type_for_key, key, log_context)
AuthenticationOperator->>AuthenticationManager: getUID(key, log_context)
AuthenticationManager->>AuthMap: getUIDSafe(type_for_key, key, log_context)
alt uid_in_memory
AuthMap-->>AuthenticationManager: uid
else uid_not_in_memory
AuthMap->>DatabaseAPI: uidByPubKey(key, uid, log_context)
DatabaseAPI->>DatabaseAPI: buildSearchParamURL(usr_find_by_pub_key)
DatabaseAPI->>DatabaseAPI: dbGetRaw(url, uid, log_context)
DatabaseAPI->>DatabaseService: HTTP GET url
DatabaseService-->>DatabaseAPI: 200 OK body
DatabaseAPI-->>AuthMap: true uid
AuthMap-->>AuthenticationManager: uid
end
AuthenticationManager-->>AuthenticationOperator: uid
end
AuthenticationOperator-->>Client: response with uid
Sequence diagram for dbGetRaw attaching correlation ID headersequenceDiagram
participant Caller as DatabaseAPI_caller
participant DatabaseAPI
participant CURL
participant DatabaseService
Caller->>DatabaseAPI: dbGetRaw(url, result, log_context)
DatabaseAPI->>DatabaseAPI: a_result.clear()
DatabaseAPI->>CURL: curl_easy_setopt(m_curl, CURLOPT_URL, url)
DatabaseAPI->>CURL: curl_easy_setopt(m_curl, CURLOPT_WRITEFUNCTION, writeCallback)
DatabaseAPI->>CURL: curl_easy_setopt(m_curl, CURLOPT_WRITEDATA, a_result)
DatabaseAPI->>CURL: curl_easy_setopt(m_curl, CURLOPT_ERRORBUFFER, error)
DatabaseAPI->>CURL: curl_easy_setopt(m_curl, CURLOPT_HTTPGET, 1)
DatabaseAPI->>DatabaseAPI: header = x-correlation-id + log_context.correlation_id
DatabaseAPI->>CURL: headers = curl_slist_append(headers, header)
DatabaseAPI->>CURL: curl_easy_setopt(m_curl, CURLOPT_HTTPHEADER, headers)
DatabaseAPI->>CURL: curl_easy_perform(m_curl)
CURL->>DatabaseService: HTTP GET url with x-correlation-id
DatabaseService-->>CURL: HTTP response
CURL-->>DatabaseAPI: res
DatabaseAPI->>CURL: curl_slist_free_all(headers)
DatabaseAPI->>CURL: curl_easy_getinfo(m_curl, CURLINFO_RESPONSE_CODE, http_code)
alt success_2xx
DatabaseAPI-->>Caller: true
else failure
DatabaseAPI-->>Caller: false
end
Class diagram for authentication and database components with LogContextclassDiagram
class LogContext {
+string correlation_id
}
class IAuthenticationManager {
<<interface>>
+incrementKeyAccessCounter(public_key string, log_context LogContext) void
+hasKey(pub_key string, log_context LogContext) bool
+getUID(pub_key string, log_context LogContext) string
}
class AuthenticationManager {
+incrementKeyAccessCounter(public_key string, log_context LogContext) void
+purge(pub_key_type PublicKeyType) void
+hasKey(pub_key string, log_context LogContext) bool
+addKey(pub_key_type PublicKeyType, public_key string, uid string) void
+hasKey(pub_key_type PublicKeyType, public_key string, log_context LogContext) bool
+migrateKey(from_type PublicKeyType, to_type PublicKeyType, public_key string) void
+clearAllNonPersistentKeys() void
+getUID(pub_key string, log_context LogContext) string
+getUIDSafe(pub_key string, log_context LogContext) string
-m_auth_mapper AuthMap
-m_lock mutex
}
class AuthMap {
+getUID(pub_key_type PublicKeyType, public_key string, log_context LogContext) string
+getUIDSafe(pub_key_type PublicKeyType, public_key string, log_context LogContext) string
+size(pub_key_type PublicKeyType) size_t
+hasKey(pub_key_type PublicKeyType, public_key string, log_context LogContext) bool
+incrementKeyAccessCounter(pub_key_type PublicKeyType, public_key string, log_context LogContext) void
+addKey(pub_key_type PublicKeyType, public_key string, uid string) void
+getAccessCount(pub_key_type PublicKeyType, public_key string) size_t
+hasKeyType(pub_key_type PublicKeyType, public_key string) bool
-m_db_url string
-m_db_user string
-m_db_pass string
}
class DatabaseAPI {
+DatabaseAPI(db_url string, db_user string, db_pass string)
+uidByPubKey(a_pub_key string, a_uid string&, log_context LogContext) bool
+userGetKeys(a_pub_key string&, a_priv_key string&, log_context LogContext) bool
+userSetKeys(a_pub_key string, a_priv_key string, log_context LogContext) void
+userSetAccessToken(a_acc_tok string, a_refresh_tok string, a_expires_in uint32_t, other_token_data string, log_context LogContext) void
+getExpiringAccessTokens(a_expires_in uint32_t, a_expiring_tokens vector~UserTokenInfo~&, log_context LogContext) void
+purgeTransferRecords(age size_t, log_context LogContext) void
-dbGet(a_url_path const char*, a_params vector~pair~string_string~~, a_result libjson_Value&, log_context LogContext, a_log bool) long
-dbGetRaw(url string, a_result string&, log_context LogContext) bool
-dbPost(a_url_path const char*, a_params vector~pair~string_string~~, a_body string*, a_result libjson_Value&, log_context LogContext) long
}
class AuthenticationOperator {
+execute(message IMessage&) void
-m_authentication_manager IAuthenticationManager*
}
class Promote {
+enforce(auth_map AuthMap&, public_key string) void
-m_promote_from PublicKeyType
-m_promote_to PublicKeyType
-m_transient_to_session_count_threshold size_t
}
IAuthenticationManager <|.. AuthenticationManager
AuthenticationManager *-- AuthMap
AuthMap ..> DatabaseAPI
AuthenticationOperator --> IAuthenticationManager
Promote --> AuthMap
DatabaseAPI ..> LogContext
AuthenticationManager ..> LogContext
AuthMap ..> LogContext
AuthenticationOperator ..> LogContext
Promote ..> LogContext
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Most of the new
LogContextparameters are passed by value, including in interfaces; consider passing them asconst LogContext&to avoid unnecessary copies and make the intent (read-only context) clearer. - Correlation ID generation with Boost UUID is now duplicated in
AuthenticationOperator::executeandPromote::enforce; consider extracting a small helper/factory to centralize correlation ID creation and keep this logic consistent. Condition.cppusesLogContextbut only adds Boost UUID headers; make sure to include the header that definesLogContext(e.g.,common/DynaLog.hpp) here instead of relying on transitive includes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Most of the new `LogContext` parameters are passed by value, including in interfaces; consider passing them as `const LogContext&` to avoid unnecessary copies and make the intent (read-only context) clearer.
- Correlation ID generation with Boost UUID is now duplicated in `AuthenticationOperator::execute` and `Promote::enforce`; consider extracting a small helper/factory to centralize correlation ID creation and keep this logic consistent.
- `Condition.cpp` uses `LogContext` but only adds Boost UUID headers; make sure to include the header that defines `LogContext` (e.g., `common/DynaLog.hpp`) here instead of relying on transitive includes.
## Individual Comments
### Comment 1
<location path="core/server/DatabaseAPI.cpp" line_range="186-195" />
<code_context>
curl_easy_setopt(m_curl, CURLOPT_WRITEDATA, &a_result);
curl_easy_setopt(m_curl, CURLOPT_ERRORBUFFER, error);
curl_easy_setopt(m_curl, CURLOPT_HTTPGET, 1);
+ struct curl_slist* headers = nullptr;
+
+ // safe: curl_slist_append copies the string internally
+ std::string header = "x-correlation-id: " + log_context.correlation_id;
+ headers = curl_slist_append(headers, header.c_str());
+
+ // attach headers to the CURL handle
+ curl_easy_setopt(m_curl, CURLOPT_HTTPHEADER, headers);
CURLcode res = curl_easy_perform(m_curl);
+ curl_slist_free_all(headers);
long http_code = 0;
curl_easy_getinfo(m_curl, CURLINFO_RESPONSE_CODE, &http_code);
</code_context>
<issue_to_address>
**issue (bug_risk):** Reset CURLOPT_HTTPHEADER after freeing the curl_slist to avoid a dangling pointer in the CURL handle.
After `curl_slist_free_all(headers)`, `m_curl` still has `CURLOPT_HTTPHEADER` set to the freed list. If this handle is reused and a request is performed before `CURLOPT_HTTPHEADER` is updated, libcurl may dereference freed memory.
To avoid that, clear the option after freeing:
```c++
CURLcode res = curl_easy_perform(m_curl);
curl_slist_free_all(headers);
headers = nullptr;
curl_easy_setopt(m_curl, CURLOPT_HTTPHEADER, nullptr);
```
This keeps the handle from holding a pointer to freed header data between requests.
</issue_to_address>
### Comment 2
<location path="core/server/AuthenticationManager.hpp" line_range="57-60" />
<code_context>
*not be purged.
**/
- virtual void incrementKeyAccessCounter(const std::string &public_key) final;
+ virtual void incrementKeyAccessCounter(const std::string &public_key, LogContext log_context) final;
/**
</code_context>
<issue_to_address>
**suggestion (performance):** Consider passing LogContext by const reference instead of by value through the authentication APIs.
All public `AuthenticationManager` / `IAuthenticationManager` methods now take `LogContext` by value and forward it through to `AuthMap` and `DatabaseAPI`. To avoid repeated copies as `LogContext` grows, you could change these to accept `const LogContext&` instead and propagate that through the implementation and call sites, e.g.:
```c++
virtual void incrementKeyAccessCounter(const std::string &public_key, const LogContext &log_context) final;
virtual bool hasKey(const std::string &pub_key, const LogContext &log_context) const final;
virtual std::string getUID(const std::string &pub_key, const LogContext &log_context) const final;
std::string getUIDSafe(const std::string &pub_key, const LogContext &log_context) const;
```
Suggested implementation:
```
*allotted purge time frame. If the count is above one then the session key
*not be purged.
**/
virtual void incrementKeyAccessCounter(const std::string &public_key, const LogContext &log_context) final;
/**
* This will purge all keys of a particular type that have expired.
* - SESSION
* - PERSISTENT
**/
virtual bool hasKey(const std::string &pub_key, const LogContext &log_context) const final;
```
To fully implement the suggestion, you should also:
1. Update all other `AuthenticationManager` / `IAuthenticationManager` method declarations in this header that currently take `LogContext` by value to instead take `const LogContext &` (e.g. `getUID`, `getUIDSafe`, and any similar methods).
2. Update the corresponding method definitions in the `.cpp` files (e.g. `AuthenticationManager.cpp`, `AuthMap.cpp`, `DatabaseAPI` implementations) to match the new signatures (`const LogContext &`).
3. Adjust all call sites that pass a `LogContext` to these methods so they pass by reference; typically no call-site syntax change is required, but ensure there are no temporary rvalues that would now bind to a `const LogContext &`.
4. If there are overridden methods in derived classes, make sure their signatures are also updated to use `const LogContext &` so they still correctly override the interface.
</issue_to_address>
### Comment 3
<location path="common/tests/unit/test_OperatorFactory.cpp" line_range="42" />
<code_context>
* Methods only available via the interface
**/
- virtual void incrementKeyAccessCounter(const std::string &pub_key) final {
+ virtual void incrementKeyAccessCounter(const std::string &pub_key, LogContext log_context) final {
++m_counters.at(pub_key);
}
</code_context>
<issue_to_address>
**suggestion (testing):** Improve Operator/Authentication integration tests to validate LogContext usage and correlation-id generation
`DummyAuthManager` now accepts a `LogContext`, but the tests don't verify how it's used. Since `AuthenticationOperator::execute` generates a correlation ID and passes the same `LogContext` to `hasKey`, `incrementKeyAccessCounter`, and `getUID`, please enhance the tests to:
- Have `DummyAuthManager` store the last `LogContext` it receives, and
- Assert that `execute()` provides a non-empty correlation ID and reuses the same `LogContext` across all three calls.
This will validate the new wiring rather than just exercising the updated signature.
Suggested implementation:
```cpp
/**
* Methods only available via the interface
**/
virtual void incrementKeyAccessCounter(const std::string &pub_key, LogContext log_context) final {
++m_counters.at(pub_key);
m_log_contexts.push_back(log_context);
}
virtual bool hasKey(const std::string &pub_key, LogContext log_context) const {
m_log_contexts.push_back(log_context);
return m_counters.count(pub_key);
}
// Just assume all keys map to the anon_uid
virtual std::string getUID(const std::string &, LogContext log_context) const {
m_log_contexts.push_back(log_context);
return "authenticated_uid";
}
const std::vector<LogContext> &logContexts() const {
return m_log_contexts;
}
```
To fully implement the test coverage you described, you will also need to:
1. Add a member field to `DummyAuthManager` (in the same class where `m_counters` is defined):
```c++
mutable std::vector<LogContext> m_log_contexts;
```
Make sure the header `<vector>` is included if it is not already.
2. In the relevant Operator/Authentication integration test that invokes `AuthenticationOperator::execute(...)` (or uses `OperatorFactory` to create the authentication operator with `DummyAuthManager`), add assertions such as:
```c++
const auto &log_contexts = dummy_auth_manager.logContexts();
ASSERT_EQ(log_contexts.size(), 3u); // hasKey, incrementKeyAccessCounter, getUID
// All three calls must receive the same LogContext instance
EXPECT_TRUE(log_contexts[0] == log_contexts[1]);
EXPECT_TRUE(log_contexts[1] == log_contexts[2]);
// And the correlation id must be non-empty
const auto &ctx = log_contexts[0];
// Adjust the accessor below to however correlation-id is exposed by LogContext
EXPECT_FALSE(ctx.correlation_id().empty());
```
If `LogContext` does not support `operator==` or `correlation_id()`, adapt the checks accordingly (for example, compare pointer identity if `LogContext` is a smart pointer type, or use the appropriate getter for the correlation ID as defined in `common/DynaLog.hpp`).
3. If the same `DummyAuthManager` instance is reused across tests, ensure you clear `m_log_contexts` (e.g., via a `clearLogContexts()` helper) between test cases or create a fresh instance per test so that `logContexts().size()` is deterministic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+186
to
195
| struct curl_slist* headers = nullptr; | ||
|
|
||
| // safe: curl_slist_append copies the string internally | ||
| std::string header = "x-correlation-id: " + log_context.correlation_id; | ||
| headers = curl_slist_append(headers, header.c_str()); | ||
|
|
||
| // attach headers to the CURL handle | ||
| curl_easy_setopt(m_curl, CURLOPT_HTTPHEADER, headers); | ||
|
|
||
| CURLcode res = curl_easy_perform(m_curl); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Reset CURLOPT_HTTPHEADER after freeing the curl_slist to avoid a dangling pointer in the CURL handle.
After curl_slist_free_all(headers), m_curl still has CURLOPT_HTTPHEADER set to the freed list. If this handle is reused and a request is performed before CURLOPT_HTTPHEADER is updated, libcurl may dereference freed memory.
To avoid that, clear the option after freeing:
CURLcode res = curl_easy_perform(m_curl);
curl_slist_free_all(headers);
headers = nullptr;
curl_easy_setopt(m_curl, CURLOPT_HTTPHEADER, nullptr);This keeps the handle from holding a pointer to freed header data between requests.
Comment on lines
+57
to
60
| virtual void incrementKeyAccessCounter(const std::string &public_key, LogContext log_context) final; | ||
|
|
||
| /** | ||
| * This will purge all keys of a particular type that have expired. |
Contributor
There was a problem hiding this comment.
suggestion (performance): Consider passing LogContext by const reference instead of by value through the authentication APIs.
All public AuthenticationManager / IAuthenticationManager methods now take LogContext by value and forward it through to AuthMap and DatabaseAPI. To avoid repeated copies as LogContext grows, you could change these to accept const LogContext& instead and propagate that through the implementation and call sites, e.g.:
virtual void incrementKeyAccessCounter(const std::string &public_key, const LogContext &log_context) final;
virtual bool hasKey(const std::string &pub_key, const LogContext &log_context) const final;
virtual std::string getUID(const std::string &pub_key, const LogContext &log_context) const final;
std::string getUIDSafe(const std::string &pub_key, const LogContext &log_context) const;Suggested implementation:
*allotted purge time frame. If the count is above one then the session key
*not be purged.
**/
virtual void incrementKeyAccessCounter(const std::string &public_key, const LogContext &log_context) final;
/**
* This will purge all keys of a particular type that have expired.
* - SESSION
* - PERSISTENT
**/
virtual bool hasKey(const std::string &pub_key, const LogContext &log_context) const final;
To fully implement the suggestion, you should also:
- Update all other
AuthenticationManager/IAuthenticationManagermethod declarations in this header that currently takeLogContextby value to instead takeconst LogContext &(e.g.getUID,getUIDSafe, and any similar methods). - Update the corresponding method definitions in the
.cppfiles (e.g.AuthenticationManager.cpp,AuthMap.cpp,DatabaseAPIimplementations) to match the new signatures (const LogContext &). - Adjust all call sites that pass a
LogContextto these methods so they pass by reference; typically no call-site syntax change is required, but ensure there are no temporary rvalues that would now bind to aconst LogContext &. - If there are overridden methods in derived classes, make sure their signatures are also updated to use
const LogContext &so they still correctly override the interface.
| * Methods only available via the interface | ||
| **/ | ||
| virtual void incrementKeyAccessCounter(const std::string &pub_key) final { | ||
| virtual void incrementKeyAccessCounter(const std::string &pub_key, LogContext log_context) final { |
Contributor
There was a problem hiding this comment.
suggestion (testing): Improve Operator/Authentication integration tests to validate LogContext usage and correlation-id generation
DummyAuthManager now accepts a LogContext, but the tests don't verify how it's used. Since AuthenticationOperator::execute generates a correlation ID and passes the same LogContext to hasKey, incrementKeyAccessCounter, and getUID, please enhance the tests to:
- Have
DummyAuthManagerstore the lastLogContextit receives, and - Assert that
execute()provides a non-empty correlation ID and reuses the sameLogContextacross all three calls.
This will validate the new wiring rather than just exercising the updated signature.
Suggested implementation:
/**
* Methods only available via the interface
**/
virtual void incrementKeyAccessCounter(const std::string &pub_key, LogContext log_context) final {
++m_counters.at(pub_key);
m_log_contexts.push_back(log_context);
}
virtual bool hasKey(const std::string &pub_key, LogContext log_context) const {
m_log_contexts.push_back(log_context);
return m_counters.count(pub_key);
}
// Just assume all keys map to the anon_uid
virtual std::string getUID(const std::string &, LogContext log_context) const {
m_log_contexts.push_back(log_context);
return "authenticated_uid";
}
const std::vector<LogContext> &logContexts() const {
return m_log_contexts;
}
To fully implement the test coverage you described, you will also need to:
-
Add a member field to
DummyAuthManager(in the same class wherem_countersis defined):mutable std::vector<LogContext> m_log_contexts;Make sure the header
<vector>is included if it is not already. -
In the relevant Operator/Authentication integration test that invokes
AuthenticationOperator::execute(...)(or usesOperatorFactoryto create the authentication operator withDummyAuthManager), add assertions such as:const auto &log_contexts = dummy_auth_manager.logContexts(); ASSERT_EQ(log_contexts.size(), 3u); // hasKey, incrementKeyAccessCounter, getUID // All three calls must receive the same LogContext instance EXPECT_TRUE(log_contexts[0] == log_contexts[1]); EXPECT_TRUE(log_contexts[1] == log_contexts[2]); // And the correlation id must be non-empty const auto &ctx = log_contexts[0]; // Adjust the accessor below to however correlation-id is exposed by LogContext EXPECT_FALSE(ctx.correlation_id().empty());
If
LogContextdoes not supportoperator==orcorrelation_id(), adapt the checks accordingly (for example, compare pointer identity ifLogContextis a smart pointer type, or use the appropriate getter for the correlation ID as defined incommon/DynaLog.hpp). -
If the same
DummyAuthManagerinstance is reused across tests, ensure you clearm_log_contexts(e.g., via aclearLogContexts()helper) between test cases or create a fresh instance per test so thatlogContexts().size()is deterministic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket
#1663
Description
Added logcontext to dbGetRaw
Tasks
Summary by Sourcery
Propagate structured logging context with correlation IDs through authentication and database paths and attach it as an HTTP header on database requests.
New Features:
Enhancements:
Tests: