feat: Implement OAuth2 authentication for REST catalog#577
feat: Implement OAuth2 authentication for REST catalog#577lishuxu wants to merge 3 commits intoapache:mainfrom
Conversation
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
This is a comprehensive code review report generated by gemini-cli against PR #577. The review rigorously compares the C++ implementation with the RFC 6749 (OAuth2) spec and the current Iceberg Java behavior.
Overall, the structure and C++ style are excellent, but there are a few parity and robustness issues that need addressing.
| namespace iceberg::rest::auth { | ||
|
|
||
| /// \brief Response from an OAuth2 token endpoint. | ||
| struct ICEBERG_REST_EXPORT OAuthTokenResponse { |
There was a problem hiding this comment.
Why this is defined here but not in the src/iceberg/catalog/rest/types.h?
| /// | ||
| /// \param json_str The JSON string to parse. | ||
| /// \return The parsed token response or an error. | ||
| ICEBERG_REST_EXPORT Result<OAuthTokenResponse> OAuthTokenResponseFromJsonString( |
There was a problem hiding this comment.
ditto, should this be moved to src/iceberg/catalog/rest/json_serde_internal.h and also add its ToJson?
| struct ICEBERG_REST_EXPORT OAuthTokenResponse { | ||
| std::string access_token; // required | ||
| std::string token_type; // required, typically "bearer" | ||
| int64_t expires_in = 0; // optional, seconds until expiration |
There was a problem hiding this comment.
| int64_t expires_in = 0; // optional, seconds until expiration | |
| std::optional<int64_t> expires_in; // optional, seconds until expiration |
Let's use optional because the default 0 will cause trouble.
| /// \param scope OAuth2 scope to request. | ||
| /// \param session Auth session for the request (typically a no-op session). | ||
| /// \return The token response or an error. | ||
| ICEBERG_REST_EXPORT Result<OAuthTokenResponse> FetchToken( |
There was a problem hiding this comment.
It looks strange that AuthSession& session is required here so we have to create a AuthSession anyway before calling this. Should we use the same input parameter list as the Java api?
| const std::string& /*client_id*/, const std::string& /*client_secret*/, | ||
| const std::string& /*scope*/, HttpClient& /*client*/) { | ||
| // TODO(lishuxu): Create OAuth2AuthSession with auto-refresh support. | ||
| return MakeDefault({{"Authorization", "Bearer " + initial_token.access_token}}); |
There was a problem hiding this comment.
Can we avoid magic string like "Authorization" and "Bearer " by defining them as constants in src/iceberg/catalog/rest/oauth2_util.h?
| FetchToken(init_client, ctx.token_endpoint, ctx.client_id, | ||
| ctx.client_secret, ctx.scope, *noop_session)); | ||
| return AuthSession::MakeDefault( | ||
| {{"Authorization", "Bearer " + init_token_response_->access_token}}); |
There was a problem hiding this comment.
ditto, let's avoid magic strings scattered around.
| // TODO(lishuxu): Override ContextualSession() to support per-context token exchange. | ||
|
|
||
| private: | ||
| struct OAuth2Context { |
There was a problem hiding this comment.
Is this trying to mirror the AuthConfig in Java? Perhaps we should extend it from ConfigBase to accept a map for better extensibility.
2 participants
Add OAuth2 authentication support for the REST catalog, including:
Also includes:
headers properly override defaults
TODO:
RefreshToken and ExchangeToken will be supported later