wgtmac commented on code in PR #867:
URL: https://github.com/apache/iceberg-cpp/pull/867#discussion_r3828949389
##########
src/iceberg/catalog/rest/auth/auth_manager.cc:
##########
@@ -148,15 +158,109 @@ class OAuth2Manager : public AuthManager {
config.optional_oauth_params(), client);
}
- return AuthSession::MakeDefault({});
+ return MakeSession(AccessTokenResponse(""), config,
/*keep_refreshed=*/false);
+ }
+
+ Result<std::shared_ptr<AuthSession>> ContextualSession(
+ const SessionContext& context, std::shared_ptr<AuthSession> parent)
override {
+ return MaybeCreateChildSession(context.credentials,
/*allow_credential=*/true,
Review Comment:
This path creates a new child session for every contextual operation.
Without a cache, repeated operations with the same context re-exchange or fetch
a token each time, which can hit OAuth rate limits. Can we cache child sessions
by context/credential before enabling this path?
##########
src/iceberg/catalog/rest/auth/auth_manager.cc:
##########
@@ -148,15 +158,109 @@ class OAuth2Manager : public AuthManager {
config.optional_oauth_params(), client);
}
- return AuthSession::MakeDefault({});
+ return MakeSession(AccessTokenResponse(""), config,
/*keep_refreshed=*/false);
+ }
+
+ Result<std::shared_ptr<AuthSession>> ContextualSession(
+ const SessionContext& context, std::shared_ptr<AuthSession> parent)
override {
+ return MaybeCreateChildSession(context.credentials,
/*allow_credential=*/true,
+ std::move(parent));
}
- // TODO(lishuxu): Override TableSession() for token exchange (RFC 8693).
- // TODO(lishuxu): Override ContextualSession() for per-context exchange.
+ Result<std::shared_ptr<AuthSession>> TableSession(
+ [[maybe_unused]] const TableIdentifier& table,
+ const std::unordered_map<std::string, std::string>& properties,
+ std::shared_ptr<AuthSession> parent) override {
+ return MaybeCreateChildSession(FilterTableSessionProperties(properties),
+ /*allow_credential=*/false,
std::move(parent));
+ }
private:
+ static OAuthTokenResponse AccessTokenResponse(std::string token) {
+ return {
+ .access_token = std::move(token),
+ .token_type = "bearer",
+ .issued_token_type = AuthProperties::kAccessTokenType,
+ };
+ }
+
+ static Result<AuthProperties> ChildConfig(const OAuth2SessionInfo&
parent_info,
+ const std::string& credential) {
+ auto properties = parent_info.optional_oauth_params;
+ properties[AuthProperties::kCredential.key()] = credential;
+ properties[AuthProperties::kScope.key()] = parent_info.scope;
+ properties[AuthProperties::kOAuth2ServerUri.key()] =
parent_info.oauth2_server_uri;
+ return AuthProperties::FromProperties(properties);
+ }
+
+ Result<std::shared_ptr<AuthSession>> MakeSession(
+ const OAuthTokenResponse& token_response, const AuthProperties& config,
+ bool keep_refreshed) const {
+ ICEBERG_PRECHECK(shared_client_ != nullptr,
+ "OAuth2 catalog session must be initialized before child
sessions");
+ return AuthSession::MakeOAuth2(token_response, config.oauth2_server_uri(),
+ config.client_id(), config.client_secret(),
+ config.scope(), keep_refreshed,
+ config.optional_oauth_params(),
*shared_client_);
+ }
+
+ Result<std::shared_ptr<AuthSession>> MaybeCreateChildSession(
+ const std::unordered_map<std::string, std::string>& credentials,
+ bool allow_credential, std::shared_ptr<AuthSession> parent) const {
+ auto token_it = credentials.find(AuthProperties::kToken.key());
+ auto credential_it = credentials.find(AuthProperties::kCredential.key());
+ auto typed_token = FindPreferredTypedToken(credentials);
+ if (token_it == credentials.end() &&
+ (!allow_credential || credential_it == credentials.end()) &&
+ !typed_token.has_value()) {
+ return parent;
+ }
+
+ ICEBERG_PRECHECK(shared_client_ != nullptr,
+ "OAuth2 catalog session must be initialized before child
sessions");
+ auto parent_info = parent->OAuth2Info();
+ ICEBERG_PRECHECK(parent_info.has_value(),
+ "OAuth2 child session requires OAuth2 parent metadata");
+
+ if (token_it != credentials.end()) {
+ ICEBERG_ASSIGN_OR_RAISE(auto config,
+ ChildConfig(*parent_info,
parent_info->credential));
+ return MakeSession(AccessTokenResponse(token_it->second), config,
Review Comment:
Child sessions are always created with keep_refreshed=false, so a
token-exchange response with expires_in or JWT exp is never refreshed. A
long-lived table/context session will keep sending an expired token. Can we
either schedule child refresh or explicitly limit expiring child tokens here?
--
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]
