wgtmac commented on code in PR #985:
URL: https://github.com/apache/iceberg-cpp/pull/985#discussion_r4463383198
##########
src/iceberg/catalog/rest/auth/auth_manager.cc:
##########
@@ -263,53 +427,94 @@ class OAuth2Manager : public AuthManager {
token_request_started_at,
refresh_executor_);
}
- Result<std::shared_ptr<AuthSession>> MaybeCreateChildSession(
- const std::unordered_map<std::string, std::string>& credentials,
- bool allow_credential, std::shared_ptr<AuthSession> parent) {
- 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;
- }
-
+ Result<OAuth2SessionInfo> ParentInfo(const AuthSession& parent) const {
ICEBERG_PRECHECK(refresh_client_ != nullptr,
"OAuth2 catalog session must be initialized before child
sessions");
- auto parent_info = parent->OAuth2Info();
+ auto parent_info = parent.OAuth2Info();
ICEBERG_PRECHECK(parent_info.has_value(),
"OAuth2 child session requires OAuth2 parent metadata");
+ return std::move(*parent_info);
+ }
- if (token_it != credentials.end()) {
- ICEBERG_ASSIGN_OR_RAISE(auto config,
- ChildConfig(*parent_info,
parent_info->credential));
- return MakeSession(AccessTokenResponse(token_it->second), config,
- /*keep_refreshed=*/false);
+ Result<std::shared_ptr<AuthSession>> GetOrCreateChildSession(
+ const ChildCredential& credential, const OAuth2SessionInfo& parent_info,
+ const std::string& cache_key) {
+ // Use cache if we have a valid cache_key
+ if (!cache_key.empty() && session_cache_) {
+ return session_cache_->Get(
+ cache_key, [&]() -> Result<std::shared_ptr<AuthSession>> {
+ return CreateChildSessionUncached(credential, parent_info);
+ });
}
- if (allow_credential && credential_it != credentials.end()) {
- ICEBERG_ASSIGN_OR_RAISE(auto config,
- ChildConfig(*parent_info,
credential_it->second));
- ICEBERG_ASSIGN_OR_RAISE(auto response,
- OAuth2Util::FetchToken(*refresh_client_,
*parent, config));
- return MakeSession(response, config, /*keep_refreshed=*/false);
+ // No cache, create session directly
+ return CreateChildSessionUncached(credential, parent_info);
+ }
+
+ Result<std::shared_ptr<AuthSession>> CreateChildSessionUncached(
+ const ChildCredential& credential, const OAuth2SessionInfo& parent_info)
{
+ switch (credential.kind) {
+ case ChildCredential::Kind::kToken:
+ return CreateTokenChildSession(credential.value, parent_info);
+ case ChildCredential::Kind::kCredential:
+ return CreateCredentialChildSession(credential.value, parent_info);
+ case ChildCredential::Kind::kTokenExchange:
+ return CreateTokenExchangeChildSession(credential.value,
credential.token_type,
+ parent_info);
}
+ std::unreachable();
+ }
+ Result<std::shared_ptr<AuthSession>> CreateTokenChildSession(
+ const std::string& token, const OAuth2SessionInfo& parent_info) {
+ // Token child session: don't include parent credential in config.
+ // Token-based sessions are not refreshed; when token exchange is enabled,
+ // refresh will use the token itself rather than the parent credential.
+ auto properties = parent_info.optional_oauth_params;
+ properties[AuthProperties::kScope.key()] = parent_info.scope;
+ properties[AuthProperties::kOAuth2ServerUri.key()] =
parent_info.oauth2_server_uri;
+ ICEBERG_ASSIGN_OR_RAISE(auto config,
AuthProperties::FromProperties(properties));
+ return MakeSession(AccessTokenResponse(token), config,
/*keep_refreshed=*/false);
+ }
+
+ Result<std::shared_ptr<AuthSession>> CreateCredentialChildSession(
+ const std::string& credential, const OAuth2SessionInfo& parent_info) {
+ ICEBERG_ASSIGN_OR_RAISE(auto config, ChildConfig(parent_info, credential));
+ // Use parent session headers for FetchToken authentication
+ auto temp_parent = AuthSession::MakeDefault(parent_info.headers);
+ ICEBERG_ASSIGN_OR_RAISE(
+ auto response, OAuth2Util::FetchToken(*refresh_client_, *temp_parent,
config));
+ // Credential child sessions follow the catalog's keep_refreshed, but
refreshing
+ // needs an executor. Without one, the child keeps its initial token.
+ if (keep_refreshed_ && refresh_executor_ == nullptr) {
+ std::call_once(no_refresh_executor_warning_, [] {
+ ICEBERG_LOG_WARN(
+ "OAuth2 credential child sessions will not refresh their tokens
because "
+ "no auth executor was provided");
+ });
+ }
+ return MakeSession(response, config, keep_refreshed_ && refresh_executor_
!= nullptr);
Review Comment:
Please record the steady-clock time before FetchToken and pass it to
MakeSession; the ExchangeToken path needs the same change. Otherwise response
latency extends an opaque token's apparent lifetime, so the new cache can reuse
an expired token and credential-child refresh can run too late.
--
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]
