wgtmac commented on code in PR #479: URL: https://github.com/apache/iceberg-cpp/pull/479#discussion_r2700649571
########## src/iceberg/catalog/rest/auth/auth_session.h: ########## @@ -0,0 +1,75 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include <memory> +#include <string> +#include <unordered_map> + +#include "iceberg/catalog/rest/iceberg_rest_export.h" +#include "iceberg/result.h" + +/// \file iceberg/catalog/rest/auth/auth_session.h +/// \brief Authentication session interface for REST catalog. + +namespace iceberg::rest::auth { + +/// \brief An authentication session that can authenticate outgoing HTTP requests. +class ICEBERG_REST_EXPORT AuthSession { + public: + virtual ~AuthSession() = default; + + /// \brief Authenticate the given request headers. + /// + /// This method adds authentication information (e.g., Authorization header) + /// to the provided headers map. The implementation should be idempotent. + /// + /// \param[in,out] headers The headers map to add authentication information to. + /// \return Status indicating success or one of the following errors: + /// - AuthenticationFailed: General authentication failure (invalid credentials, + /// etc.) + /// - TokenExpired: Authentication token has expired and needs refresh + /// - NotAuthorized: Not authenticated (401) + /// - IOError: Network or connection errors when reaching auth server + /// - RestError: HTTP errors from authentication service + virtual Status Authenticate(std::unordered_map<std::string, std::string>& headers) = 0; + + /// \brief Close the session and release any resources. + /// + /// This method is called when the session is no longer needed. For stateful + /// sessions (e.g., OAuth2 with token refresh), this should stop any background + /// threads and release resources. + /// + /// \return Status indicating success or failure of closing the session. + virtual Status Close() { return {}; } + + /// \brief Create a session with static headers. + /// + /// This factory method creates a session that adds a fixed set of headers to each + /// request. It is suitable for authentication methods that use static credentials, + /// such as Basic auth or static bearer tokens. + /// + /// \param headers The headers to add to each request for authentication. + /// \return A new session that adds the given headers to requests. + static std::shared_ptr<AuthSession> Make( Review Comment: ```suggestion static std::shared_ptr<AuthSession> MakeDefault( ``` I'm not sure if we need to extend it to accept more types. Let's make it explicit for now that it creates a default session. ########## src/iceberg/catalog/rest/auth/auth_managers.cc: ########## @@ -0,0 +1,88 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/catalog/rest/auth/auth_managers.h" + +#include <algorithm> +#include <cctype> + +#include "iceberg/catalog/rest/auth/auth_properties.h" +#include "iceberg/util/string_util.h" + +namespace iceberg::rest::auth { + +namespace { + +/// \brief Registry type for AuthManager factories with heterogeneous lookup support. +using AuthManagerRegistry = + std::unordered_map<std::string, AuthManagerFactory, StringHash, StringEqual>; + +/// \brief Infer the authentication type from properties. +/// +/// If no explicit auth type is set, this function tries to infer it from +/// other properties. If "credential" or "token" is present, it implies +/// OAuth2 authentication. Otherwise, defaults to no authentication. +/// +/// This behavior is consistent with Java Iceberg's AuthManagers. Review Comment: Let's avoid verbose comments. ########## src/iceberg/catalog/rest/auth/auth_manager.h: ########## @@ -0,0 +1,102 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include <memory> +#include <string> +#include <unordered_map> + +#include "iceberg/catalog/rest/iceberg_rest_export.h" +#include "iceberg/catalog/rest/type_fwd.h" +#include "iceberg/result.h" +#include "iceberg/type_fwd.h" + +/// \file iceberg/catalog/rest/auth/auth_manager.h +/// \brief Authentication manager interface for REST catalog. + +namespace iceberg::rest::auth { + +/// \brief Produces authentication sessions for catalog and table requests. +class ICEBERG_REST_EXPORT AuthManager { + public: + virtual ~AuthManager() = default; + + /// \brief Create a short-lived session used to contact the configuration endpoint. + /// + /// This session is used only during catalog initialization to fetch server + /// configuration and perform initial authentication. It is typically discarded after + /// initialization. + /// + /// \param init_client HTTP client used for initialization requests. + /// \param properties Client configuration supplied by the catalog. + /// \return Session for initialization or an error if credentials cannot be acquired. + virtual Result<std::shared_ptr<AuthSession>> InitSession( + HttpClient& init_client, + const std::unordered_map<std::string, std::string>& properties); + + /// \brief Create the long-lived catalog session that acts as the parent session. + /// + /// This session is used for all catalog-level operations (list namespaces, list tables, + /// etc.) and serves as the parent session for contextual and table-specific sessions. + /// It is owned by the catalog and reused throughout the catalog's lifetime. + /// + /// \param shared_client HTTP client owned by the catalog and reused for auth calls. + /// \param properties Catalog properties (client config + server defaults). + /// \return Session for catalog operations or an error if authentication cannot be set + /// up. + virtual Result<std::shared_ptr<AuthSession>> CatalogSession( + HttpClient& shared_client, + const std::unordered_map<std::string, std::string>& properties) = 0; + + /// \brief Create or reuse a session for a specific context. + /// + /// This method is used by SessionCatalog to create sessions for different contexts + /// (e.g., different users or tenants). + /// + /// \param context Context properties (e.g., user credentials, tenant info). + /// \param parent Catalog session to inherit from or return as-is. + /// \return A context-specific session, or the parent session if no context-specific + /// session is needed, or an error if session creation fails. + virtual Result<std::shared_ptr<AuthSession>> ContextualSession( + const std::unordered_map<std::string, std::string>& context, + const std::shared_ptr<AuthSession>& parent); Review Comment: ```suggestion std::shared_ptr<AuthSession> parent); ``` ########## src/iceberg/catalog/rest/auth/auth_managers.h: ########## @@ -0,0 +1,65 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include <functional> +#include <memory> +#include <string> +#include <string_view> +#include <unordered_map> + +#include "iceberg/catalog/rest/auth/auth_manager.h" +#include "iceberg/catalog/rest/iceberg_rest_export.h" +#include "iceberg/result.h" + +/// \file iceberg/catalog/rest/auth/auth_managers.h +/// \brief Factory for creating authentication managers. + +namespace iceberg::rest::auth { + +/// \brief Function that creates an AuthManager from its name. +/// +/// \param name Name of the auth manager. +/// \param properties Properties required by the auth manager. +/// \return Newly created manager instance or an error if creation fails. +using AuthManagerFactory = std::function<Result<std::unique_ptr<AuthManager>>( + std::string_view name, + const std::unordered_map<std::string, std::string>& properties)>; + +/// \brief Registry-backed factory for AuthManager implementations. +class ICEBERG_REST_EXPORT AuthManagers { + public: + /// \brief Load a manager by consulting the "rest.auth.type" configuration. + /// + /// \param name Name of the auth manager. + /// \param properties Catalog properties used to determine auth type. + /// \return Manager instance or an error if no factory matches. + static Result<std::unique_ptr<AuthManager>> Load( + std::string_view name, + const std::unordered_map<std::string, std::string>& properties); + + /// \brief Register or override the factory for a given auth type. + /// + /// \param auth_type Case-insensitive type identifier (e.g., "basic"). + /// \param factory Factory function that produces the manager. + static void Register(std::string_view auth_type, AuthManagerFactory factory); Review Comment: nit: add some comment to say the thread safety implication. ########## src/iceberg/catalog/rest/auth/auth_managers.cc: ########## @@ -0,0 +1,88 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/catalog/rest/auth/auth_managers.h" + +#include <algorithm> +#include <cctype> + +#include "iceberg/catalog/rest/auth/auth_properties.h" +#include "iceberg/util/string_util.h" + +namespace iceberg::rest::auth { + +namespace { + +/// \brief Registry type for AuthManager factories with heterogeneous lookup support. +using AuthManagerRegistry = + std::unordered_map<std::string, AuthManagerFactory, StringHash, StringEqual>; + +/// \brief Infer the authentication type from properties. +/// +/// If no explicit auth type is set, this function tries to infer it from +/// other properties. If "credential" or "token" is present, it implies +/// OAuth2 authentication. Otherwise, defaults to no authentication. +/// +/// This behavior is consistent with Java Iceberg's AuthManagers. Review Comment: ```suggestion // Infer the authentication type from properties. ``` ########## src/iceberg/catalog/rest/auth/auth_managers.cc: ########## @@ -0,0 +1,88 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/catalog/rest/auth/auth_managers.h" + +#include <algorithm> +#include <cctype> + +#include "iceberg/catalog/rest/auth/auth_properties.h" +#include "iceberg/util/string_util.h" + +namespace iceberg::rest::auth { + +namespace { + +/// \brief Registry type for AuthManager factories with heterogeneous lookup support. +using AuthManagerRegistry = + std::unordered_map<std::string, AuthManagerFactory, StringHash, StringEqual>; + +/// \brief Infer the authentication type from properties. +/// +/// If no explicit auth type is set, this function tries to infer it from +/// other properties. If "credential" or "token" is present, it implies +/// OAuth2 authentication. Otherwise, defaults to no authentication. +/// +/// This behavior is consistent with Java Iceberg's AuthManagers. +std::string InferAuthType( + const std::unordered_map<std::string, std::string>& properties) { + auto it = properties.find(AuthProperties::kAuthType); + if (it != properties.end() && !it->second.empty()) { + return StringUtils::ToLower(it->second); + } + + // Infer from OAuth2 properties (credential or token) + bool has_credential = properties.contains(AuthProperties::kOAuth2Credential); + bool has_token = properties.contains(AuthProperties::kOAuth2Token); + if (has_credential || has_token) { + return AuthProperties::kAuthTypeOAuth2; + } + + // Default to no authentication + return AuthProperties::kAuthTypeNone; +} + +/// \brief Get the global registry of auth manager factories. Review Comment: ```suggestion // Get the global registry of auth manager factories. ``` For comments that are not in the header, we don't need to use the docxygen style `///`. ########## src/iceberg/catalog/rest/auth/auth_manager.h: ########## @@ -0,0 +1,102 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include <memory> +#include <string> +#include <unordered_map> + +#include "iceberg/catalog/rest/iceberg_rest_export.h" +#include "iceberg/catalog/rest/type_fwd.h" +#include "iceberg/result.h" +#include "iceberg/type_fwd.h" + +/// \file iceberg/catalog/rest/auth/auth_manager.h +/// \brief Authentication manager interface for REST catalog. + +namespace iceberg::rest::auth { + +/// \brief Produces authentication sessions for catalog and table requests. +class ICEBERG_REST_EXPORT AuthManager { + public: + virtual ~AuthManager() = default; + + /// \brief Create a short-lived session used to contact the configuration endpoint. + /// + /// This session is used only during catalog initialization to fetch server + /// configuration and perform initial authentication. It is typically discarded after + /// initialization. + /// + /// \param init_client HTTP client used for initialization requests. + /// \param properties Client configuration supplied by the catalog. + /// \return Session for initialization or an error if credentials cannot be acquired. + virtual Result<std::shared_ptr<AuthSession>> InitSession( + HttpClient& init_client, + const std::unordered_map<std::string, std::string>& properties); + + /// \brief Create the long-lived catalog session that acts as the parent session. + /// + /// This session is used for all catalog-level operations (list namespaces, list tables, + /// etc.) and serves as the parent session for contextual and table-specific sessions. + /// It is owned by the catalog and reused throughout the catalog's lifetime. + /// + /// \param shared_client HTTP client owned by the catalog and reused for auth calls. + /// \param properties Catalog properties (client config + server defaults). + /// \return Session for catalog operations or an error if authentication cannot be set + /// up. + virtual Result<std::shared_ptr<AuthSession>> CatalogSession( + HttpClient& shared_client, + const std::unordered_map<std::string, std::string>& properties) = 0; + + /// \brief Create or reuse a session for a specific context. + /// + /// This method is used by SessionCatalog to create sessions for different contexts + /// (e.g., different users or tenants). + /// + /// \param context Context properties (e.g., user credentials, tenant info). + /// \param parent Catalog session to inherit from or return as-is. + /// \return A context-specific session, or the parent session if no context-specific + /// session is needed, or an error if session creation fails. + virtual Result<std::shared_ptr<AuthSession>> ContextualSession( + const std::unordered_map<std::string, std::string>& context, + const std::shared_ptr<AuthSession>& parent); + + /// \brief Create or reuse a session scoped to a single table/view. + /// + /// This method is called when loading a table that may have table-specific auth + /// properties returned by the server. + /// + /// \param table Target table identifier. + /// \param properties Table-specific auth properties returned by the server. + /// \param parent Catalog or contextual session to inherit from or return as-is. + /// \return A table-specific session, or the parent session if no table-specific + /// session is needed, or an error if session creation fails. + virtual Result<std::shared_ptr<AuthSession>> TableSession( + const TableIdentifier& table, + const std::unordered_map<std::string, std::string>& properties, + const std::shared_ptr<AuthSession>& parent); Review Comment: ```suggestion std::shared_ptr<AuthSession> parent); ``` ########## src/iceberg/catalog/rest/auth/auth_managers.cc: ########## @@ -0,0 +1,88 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/catalog/rest/auth/auth_managers.h" + +#include <algorithm> +#include <cctype> + +#include "iceberg/catalog/rest/auth/auth_properties.h" +#include "iceberg/util/string_util.h" + +namespace iceberg::rest::auth { + +namespace { + +/// \brief Registry type for AuthManager factories with heterogeneous lookup support. +using AuthManagerRegistry = + std::unordered_map<std::string, AuthManagerFactory, StringHash, StringEqual>; + +/// \brief Infer the authentication type from properties. +/// +/// If no explicit auth type is set, this function tries to infer it from +/// other properties. If "credential" or "token" is present, it implies +/// OAuth2 authentication. Otherwise, defaults to no authentication. +/// +/// This behavior is consistent with Java Iceberg's AuthManagers. +std::string InferAuthType( + const std::unordered_map<std::string, std::string>& properties) { + auto it = properties.find(AuthProperties::kAuthType); + if (it != properties.end() && !it->second.empty()) { + return StringUtils::ToLower(it->second); + } + + // Infer from OAuth2 properties (credential or token) + bool has_credential = properties.contains(AuthProperties::kOAuth2Credential); + bool has_token = properties.contains(AuthProperties::kOAuth2Token); + if (has_credential || has_token) { + return AuthProperties::kAuthTypeOAuth2; + } + + // Default to no authentication Review Comment: ```suggestion ``` -- 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]
