feat: Implement BasicAuthManager to support basic authentication#564
feat: Implement BasicAuthManager to support basic authentication#564wgtmac merged 2 commits intoapache:mainfrom
Conversation
wgtmac
left a comment
There was a problem hiding this comment.
Review Report: PR #564
📄 File: src/iceberg/catalog/rest/auth/auth_managers.cc
Java Counterpart: core/src/main/java/org/apache/iceberg/rest/auth/AuthManagers.java and BasicAuthManager.java
- Parity Check: ✅ The C++ logic faithfully implements the Java behavior. It correctly enforces the requirement for
basic-usernameandbasic-passwordproperties and properly concatenates them to form theBasicbase64-encoded authorization header. The case-insensitiveauth-typehandling is correctly preserved. - Style Check: ✅ Uses modern C++ paradigms, including
std::string_view, standard containers, and[[maybe_unused]]for ignored parameters. The formatting aligns well with the project's style. - Logic Check: ✅ Clean logic mapping to
Result<T>error handling. No memory leaks or unsafe behaviors detected. Returns anInvalidArgumenton missing credentials gracefully rather than throwing raw exceptions. - Design & Conciseness: ✅
BasicAuthManageris neatly encapsulated within the.ccimplementation file, avoiding unnecessary exposure in headers. Reuses existingTransformUtil::Base64Encoderather than rolling a custom base64 encoder.
📄 File: src/iceberg/test/auth_manager_test.cc
Java Counterpart: Not found (Assumed standard test parity)
- Parity Check: ✅ Comprehensive test coverage corresponding to Java's testing expectations, covering happy paths, case insensitivity, missing properties, and special character edge cases.
- Style Check: ✅ Excellent use of the GTest framework and existing custom matchers (
IsOk,IsError,HasErrorMessage). - Logic Check: ✅ Test logic and base64 assertions are mathematically sound and verifiable.
- Design & Conciseness: ✅ Clean and focused test cases.
Summary & Recommendation
- Approve. The PR is exceptionally clean, mirrors the Java implementation faithfully, respects C++ conventions, and brings good unit test coverage. No blocking issues found.
wgtmac
left a comment
There was a problem hiding this comment.
Mostly LGTM. I've left some inline comments. Previous comment was posted by my custom review agent which accidentally approved it.
| }; | ||
|
|
||
| /// \brief Authentication manager that performs basic authentication. | ||
| class BasicAuthManager : public AuthManager { |
There was a problem hiding this 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.
There was a problem hiding this comment.
Good point!
Moved NoopAuthManager and BasicAuthManager implementations to auth_manager.cc, and added auth_manager_internal.h to expose their factory functions (MakeNoopAuthManager/MakeBasicAuthManager) only to auth_managers.cc.
feat: Implement BasicAuthManager to support basic authentication