Adding logic for client-side authentication#204
Conversation
avalerio-tkd
commented
Dec 20, 2025
avalerio-tkd
commented
- Updated HttpClientInterface to support common authentication logic.
- Updated Http implemenations for authentication flow.
- Adding extracting credentials from connection config file. -
- Updated Http implemenations for authentication flow. - Adding extracting credentials from connection config file. -
- Added validation for Json parsing of connection config file. - Cleaned up error messages and exceptions.
- Added validation for Json parsing of connection config file. - Cleaned up error messages and exceptions.
argmarco-tkd
left a comment
argmarco-tkd
left a comment
There was a problem hiding this comment.
Thanks for this change, and the thorough addition of tests. Overall LGTM, left a few comments. The main comment/request that I have - let's double check the multithreading logic around HTTPClientBase and token acquisition - left a few comments in there.
| client_id_.clear(); | ||
| api_key_.clear(); | ||
| // Allow strings or nested structures in the JSON payload for authentication credentials. | ||
| // TODO: Unify usage of nlohmann or crow json for parsing and dumping. |
There was a problem hiding this comment.
let's make sure we have a ticket for this.
|
|
||
| // Token authentication endpoint - POST /token | ||
| CROW_ROUTE(app, "/token").methods("POST"_method)([&credential_store](const crow::request& req) { | ||
| // +++++ Remove this after testing +++++ |
There was a problem hiding this comment.
let's create a ticket for this cleanup
There was a problem hiding this comment.
Removed printouts and temp printout functions.
| #include <condition_variable> | ||
| #include <cstdint> | ||
| #include <map> | ||
| #include <mutex> | ||
| #include <optional> |
There was a problem hiding this comment.
nit: mutex, condition_variable - are we forcing implementation choices from the interface? (I know that below we say that impls must be thread-safe, etc - but do we need to add members of these types here, in the interface?)
There was a problem hiding this comment.
This is a great comment and thanks for the pickiness! We centralized the token/auth handling logic on the parent class and making it no longer an interface, so having these sync-required as class members should be fine. It's not forcing subclasses to use these, it’s just how the base implementation protects its own internal state.
Happy to discuss further if you see need.
| token_fetch_in_progress_ = true; | ||
| } | ||
|
|
||
| // Fetch token without holding token_mutex_. |
There was a problem hiding this comment.
why? (let's document)
There was a problem hiding this comment.
Updated the comment, with a little help from our Cursor assistant friends :)
| #include "http_client_base.h" | ||
|
|
||
| // Implemenetation of the HttpClientInterface which uses a pool of connections for a given base_url. | ||
| // Implemenetation of the HttpClientBase which uses a pool of connections for a given base_url. |
There was a problem hiding this comment.
typo: "Implementation"
| std::shared_ptr<HttpClientInterface> http_client = HttplibPooledClient::Acquire(server_url_, num_worker_threads); | ||
|
|
||
| // TODO: Split credentials config file key and credentials file from connection config. | ||
| HttpClientBase::ClientCredentials credentials = ExtractClientCredentials(*config_json_opt); |
|
|
||
| std::string client_id_; | ||
| std::string api_key_; | ||
| // Stores top-level key/value pairs from the token request JSON payload containing the authentication credentials. |
There was a problem hiding this comment.
nice. let's clarify that this is not the actual token.
There was a problem hiding this comment.
Comment added.
| #include "encryption_sequencer.h" | ||
| #include "auth_utils.h" | ||
|
|
||
| // +++++ Remove this after testing +++++ |
There was a problem hiding this comment.
let's make sure we have a ticket for this cleanup.
There was a problem hiding this comment.
Removed already.
- Cleaned up temporary printouts. - Added cmdline option to API server to allow/disallow credential checking explicitly.
|
Thanks @argmarco-tkd for the review! Addressed the comments. Could you PTAL? |
- Added testing_credentials.json to .gitignore.
…docs are updated with instructions)
| if (is_token_valid_at(now)) { | ||
| return cached_token_; | ||
| } | ||
| while (token_fetch_in_progress_) { |
There was a problem hiding this comment.
got it. gave it another pass at the function, it makes sense. thanks.
2 participants
