wgtmac commented on code in PR #867:
URL: https://github.com/apache/iceberg-cpp/pull/867#discussion_r3837612118
##########
src/iceberg/catalog/rest/auth/auth_manager.cc:
##########
@@ -121,6 +122,7 @@ class OAuth2Manager : public AuthManager {
HttpClient& client,
const std::unordered_map<std::string, std::string>& properties) override
{
ICEBERG_ASSIGN_OR_RAISE(auto config,
AuthProperties::FromProperties(properties));
+ shared_client_ = &client;
Review Comment:
shared_client_ is a borrowed `HttpClient*`, but child sessions use it after
this call. Please pass a `std::shared_ptr<HttpClient>` through the
manager/session API so the client lifetime is explicit.
##########
src/iceberg/test/auth_manager_test.cc:
##########
@@ -266,6 +576,11 @@ TEST_F(AuthManagerTest, OAuth2StaticToken) {
std::unordered_map<std::string, std::string> properties = {
{AuthProperties::kAuthType, "oauth2"},
{AuthProperties::kToken.key(), "my-static-token"},
+ {AuthProperties::kCredential.key(), "client-id:client-secret"},
Review Comment:
Please add a test with a short `expires_in` and delayed config fetch to
verify elapsed init time is deducted from the session lifetime.
##########
src/iceberg/test/rest_catalog_integration_test.cc:
##########
@@ -211,6 +215,105 @@ TEST_F(RestCatalogIntegrationTest, MakeCatalogSuccess) {
EXPECT_THAT(root->WithContext(SessionContext{}),
IsError(ErrorKind::kInvalidArgument));
}
+TEST_F(RestCatalogIntegrationTest, OAuthContextCredentialEndToEnd) {
Review Comment:
These tests call `AuthManager` directly, so they do not cover `RestCatalog`
context/table wiring. Please add one real catalog path or move them to the unit
suite.
##########
src/iceberg/catalog/rest/auth/auth_properties.cc:
##########
@@ -35,6 +36,28 @@ std::pair<std::string, std::string> ParseCredential(const
std::string& credentia
return {credential.substr(0, colon_pos), credential.substr(colon_pos + 1)};
}
+Result<std::string> ResolveOAuth2ServerUri(
+ const std::unordered_map<std::string, std::string>& properties) {
+ auto endpoint_it = properties.find(AuthProperties::kOAuth2ServerUri.key());
+ std::string endpoint = endpoint_it == properties.end() ||
endpoint_it->second.empty()
+ ? AuthProperties::kOAuth2ServerUri.value()
+ : endpoint_it->second;
+
+ if (endpoint.starts_with("http://";) || endpoint.starts_with("https://";)) {
+ return endpoint;
+ }
+ auto uri_it = properties.find(RestCatalogProperties::kUri.key());
+ if (uri_it == properties.end() || uri_it->second.empty()) {
+ return endpoint;
+ }
+
+ auto base_uri = std::string(TrimTrailingSlash(uri_it->second));
+ if (endpoint.starts_with('/')) {
+ return base_uri + endpoint;
+ }
+ return base_uri + "/" + std::string(TrimTrailingSlash(endpoint));
Review Comment:
Java preserves the relative endpoint suffix. Trimming `oauth/token/` here
changes the resolved URI; please keep the trailing slash and update the test.
##########
src/iceberg/catalog/rest/auth/auth_session.cc:
##########
@@ -141,11 +156,14 @@ class OAuth2AuthSession : public AuthSession,
OAuth2AuthSession& session_;
};
- void SetInitialToken(const OAuthTokenResponse& token_response) {
+ void UpdateTokenState(const OAuthTokenResponse& token_response) {
token_ = token_response.access_token;
- headers_ = {{std::string(kAuthorizationHeader), std::string(kBearerPrefix)
+ token_}};
+ issued_token_type_ = token_response.issued_token_type.empty()
+ ? AuthProperties::kAccessTokenType
+ : token_response.issued_token_type;
+ headers_ = AuthHeaders(token_);
- // Determine expiration time
+ expires_at_ = std::chrono::steady_clock::time_point{};
if (token_response.expires_in_secs.has_value()) {
expires_at_ = std::chrono::steady_clock::now() +
Review Comment:
`expires_in` is measured from the token request, but the session starts the
clock later. A slow config request can consume part of the token lifetime;
please carry the init fetch start time into the session.
##########
src/iceberg/catalog/rest/auth/auth_properties.cc:
##########
@@ -35,6 +36,28 @@ std::pair<std::string, std::string> ParseCredential(const
std::string& credentia
return {credential.substr(0, colon_pos), credential.substr(colon_pos + 1)};
}
+Result<std::string> ResolveOAuth2ServerUri(
+ const std::unordered_map<std::string, std::string>& properties) {
+ auto endpoint_it = properties.find(AuthProperties::kOAuth2ServerUri.key());
+ std::string endpoint = endpoint_it == properties.end() ||
endpoint_it->second.empty()
Review Comment:
Java defaults only when `oauth2-server-uri` is absent. This also defaults an
explicitly empty value; please preserve that distinction or reject empty
explicitly.
--
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]
