wgtmac commented on code in PR #564:
URL: https://github.com/apache/iceberg-cpp/pull/564#discussion_r2853515307
##########
src/iceberg/catalog/rest/auth/auth_managers.cc:
##########
@@ -78,6 +79,34 @@ class NoopAuthManager : public AuthManager {
}
};
+/// \brief Authentication manager that performs basic authentication.
+class BasicAuthManager : public AuthManager {
+ public:
+ static Result<std::unique_ptr<AuthManager>> Make(
+ [[maybe_unused]] std::string_view name,
+ [[maybe_unused]] const std::unordered_map<std::string, std::string>&
properties) {
+ return std::make_unique<BasicAuthManager>();
+ }
+
+ Result<std::shared_ptr<AuthSession>> CatalogSession(
+ [[maybe_unused]] HttpClient& client,
+ const std::unordered_map<std::string, std::string>& properties) override
{
+ auto username_it = properties.find(AuthProperties::kBasicUsername);
+ if (username_it == properties.end()) {
+ return InvalidArgument("Invalid username: missing required property
'{}'",
+ AuthProperties::kBasicUsername);
+ }
+ auto password_it = properties.find(AuthProperties::kBasicPassword);
+ if (password_it == properties.end()) {
+ return InvalidArgument("Invalid password: missing required property
'{}'",
+ AuthProperties::kBasicPassword);
+ }
Review Comment:
```suggestion
auto username_it = properties.find(AuthProperties::kBasicUsername);
ICEBERG_PRECHECK(username_it != properties.end() &&
!username_it->second.empty(),
"Missing required property '{}'",
AuthProperties::kBasicUsername);
auto password_it = properties.find(AuthProperties::kBasicPassword);
ICEBERG_PRECHECK(password_it != properties.end() &&
!password_it->second.empty(),
"Missing required property '{}'",
AuthProperties::kBasicPassword);
```
##########
src/iceberg/catalog/rest/auth/auth_managers.cc:
##########
@@ -78,6 +79,34 @@ class NoopAuthManager : public AuthManager {
}
};
+/// \brief Authentication manager that performs basic authentication.
+class BasicAuthManager : public AuthManager {
Review Comment:
It looks a little bit weird that `BasicAuthManager` and other AuthManager
implementations are declared in `auth_managers.h/cc` not `auth_manager.h/cc`.
It would be better to move them to the latter and make `auth_managers.h/cc`
simply a factory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
