feat: add restcatalog authentication api #479

lishuxu · 2026-01-02T10:24:36Z

feat: add restcatalog authentication api

src/iceberg/catalog/rest/auth/auth_manager.h

src/iceberg/catalog/rest/auth/CMakeLists.txt

src/iceberg/catalog/rest/CMakeLists.txt

src/iceberg/catalog/rest/type_fwd.h

WZhuo · 2026-01-15T02:06:06Z

src/iceberg/catalog/rest/auth/auth_managers.cc

+/// \brief Convert a string to lowercase for case-insensitive comparison.
+std::string ToLower(std::string_view str) {
+  std::string result(str);
+  std::ranges::transform(result, result.begin(),
+                         [](unsigned char c) { return std::tolower(c); });
+  return result;
+}


There is a StringUtils::ToLower func in util/string_util.h

Good catch, fixed.

src/iceberg/catalog/rest/auth/auth_session.cc

src/iceberg/catalog/rest/auth/auth_manager.h

wgtmac · 2026-01-15T06:57:22Z

src/iceberg/catalog/rest/auth/auth_session.h

+  /// (i.e., request headers take precedence).
+  ///
+  /// \param headers The headers map to add authentication information to.
+  void Authenticate(std::unordered_map<std::string, std::string>& headers) override;


Do you think it is worth adding a class HttpRequest? We don't have it yet.

I don't think we need an HttpRequest class at this stage. The current design is sufficient for most auth schemes (Basic, Bearer, OAuth2, API keys), but we can add can add it later if needed.

wgtmac · 2026-01-15T06:58:28Z

src/iceberg/catalog/rest/auth/auth_managers.cc

+/// OAuth2 authentication. Otherwise, defaults to no authentication.
+///
+/// This behavior is consistent with Java Iceberg's AuthManagers.
+std::string InferAuthType(


Should we add a enum class for the auth type?

Auth types come from external config files/properties as strings, so the string-based approach allows users to register custom auth types without modifying core code. It also matches the Iceberg REST spec.

src/iceberg/catalog/rest/auth/auth_managers.cc

wgtmac · 2026-01-15T07:00:35Z

src/iceberg/catalog/rest/auth/auth_managers.h

+
+ private:
+  /// \brief Get the global registry of auth manager factories.
+  static std::unordered_map<std::string, AuthManagerFactory>& GetRegistry();


We don't need this function in the header file, right?

src/iceberg/catalog/rest/auth/auth_managers.h

HuaHuaY · 2026-01-16T08:13:39Z

src/iceberg/catalog/rest/auth/auth_session.cc

+  for (const auto& [key, value] : headers_) {
+    headers.try_emplace(key, value);
+  }


Suggested change

for (const auto& [key, value] : headers_) {

headers.try_emplace(key, value);

}

headers.merge(headers_);

merge() is destructive - it moves nodes from headers_ to headers, leaving headers_ empty or partially empty.
so, I think the current implementation can be retained.

wgtmac · 2026-01-16T12:35:47Z

src/iceberg/catalog/rest/CMakeLists.txt

    rest_util.cc
-    types.cc)
+    types.cc
+    auth/auth_manager.cc


Sort them alphabetically

wgtmac · 2026-01-16T12:37:10Z

src/iceberg/catalog/rest/auth/auth_manager.h

+#include "iceberg/catalog/rest/iceberg_rest_export.h"
+#include "iceberg/catalog/rest/type_fwd.h"
+#include "iceberg/result.h"
+#include "iceberg/table_identifier.h"


Suggested change

#include "iceberg/table_identifier.h"

We can remove this

wgtmac · 2026-01-16T12:57:34Z

src/iceberg/catalog/rest/auth/auth_manager.h

+///
+/// AuthManager is responsible for creating authentication sessions at different scopes:
+/// - InitSession: Short-lived session for catalog initialization (optional)
+/// - CatalogSession: Long-lived session for catalog-level operations (required)
+/// - TableSession: Optional table-specific session or reuse of catalog session
+///
+/// Implementations are registered via AuthManagers::Register() and loaded by auth type.


Suggested change

///

/// AuthManager is responsible for creating authentication sessions at different scopes:

/// - InitSession: Short-lived session for catalog initialization (optional)

/// - CatalogSession: Long-lived session for catalog-level operations (required)

/// - TableSession: Optional table-specific session or reuse of catalog session

///

/// Implementations are registered via AuthManagers::Register() and loaded by auth type.

We don't need such verbose comments.

wgtmac · 2026-01-16T15:49:31Z

src/iceberg/catalog/rest/type_fwd.h

+
+class AuthManager;
+class AuthSession;
+class DefaultAuthSession;


Why do we need to forward declare DefaultAuthSession? Isn't it used only by the implementation?

wgtmac · 2026-01-16T15:51:33Z

src/iceberg/catalog/rest/auth/auth_session.h

+  /// \return Status indicating success or failure of authentication.
+  ///         - Success: Returns Status::OK
+  ///         - Failure: Returns one of the following errors:


Suggested change

/// \return Status indicating success or failure of authentication.

/// - Success: Returns Status::OK

/// - Failure: Returns one of the following errors:

/// \return Status indicating success or one of the following errors:

wgtmac · 2026-01-16T16:06:12Z

src/iceberg/catalog/rest/auth/auth_managers.cc

+
+  // Infer from OAuth2 properties (credential or token)
+  bool has_credential =
+      properties.contains(std::string(AuthProperties::kOAuth2Credential));


Do we need to define them as const std::string? It creates a string every time it is used.

wgtmac · 2026-01-16T16:07:47Z

src/iceberg/catalog/rest/auth/auth_managers.cc

+  auto& registry = GetRegistry();
+  auto it = registry.find(auth_type);
+  if (it == registry.end()) {
+    return NotImplemented("Authentication type '{}' is not supported", auth_type);


Please add a TODO to say we need to fallback to our default auth manager implementations.

wgtmac · 2026-01-16T16:08:26Z

src/iceberg/catalog/rest/auth/auth_managers.cc

+}  // namespace
+
+void AuthManagers::Register(std::string_view auth_type, AuthManagerFactory factory) {
+  GetRegistry()[StringUtils::ToLower(std::string(auth_type))] = std::move(factory);


It seems that we have unnecessary string allocations here.

wgtmac · 2026-01-16T16:10:11Z

src/iceberg/catalog/rest/auth/auth_manager.h

+  /// \param table Target table identifier.
+  /// \param properties Table-specific auth properties returned by the server.
+  /// \param parent Catalog session to inherit from or extract information from.
+  /// \return A new session for the table, nullptr to reuse parent session, or an error.


It looks weird to return nullptr to reuse parent session. I'm not sure if it is a good idea to use std::unique_ptr<AuthSession>&& parent to make the ownership explicit: either directly return parent or the created session owns its parent. I didn't any case to share the session, am I missing something?

wgtmac · 2026-01-16T16:11:04Z

src/iceberg/catalog/rest/auth/auth_manager.h

+  /// \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::unique_ptr<AuthSession>> CatalogSession(


Why we don't have contextual session as the Java impl? I think this is used by each rest endpoint before sending the http request.

shuxu.li added 3 commits January 2, 2026 18:23

feat: add restcatalog authentication api

572d02a

feat: add restcatalog authentication api

904fdf0

feat: add restcatalog authentication api

83084ad

HeartLinked reviewed Jan 13, 2026

View reviewed changes

src/iceberg/catalog/rest/auth/auth_manager.h Outdated Show resolved Hide resolved

src/iceberg/catalog/rest/auth/auth_manager.h Outdated Show resolved Hide resolved

feat: add restcatalog authentication api

72df76d

wgtmac reviewed Jan 14, 2026

View reviewed changes

WZhuo reviewed Jan 15, 2026

View reviewed changes

wgtmac requested changes Jan 15, 2026

View reviewed changes

HuaHuaY reviewed Jan 16, 2026

View reviewed changes

shuxu.li added 2 commits January 16, 2026 17:44

feat: add restcatalog authentication api

54b4868

feat: add restcatalog authentication api

28e195a

wgtmac reviewed Jan 16, 2026

View reviewed changes

feat: add restcatalog authentication api #479

Are you sure you want to change the base?

feat: add restcatalog authentication api #479

Uh oh!

Conversation

lishuxu commented Jan 2, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants