-
Notifications
You must be signed in to change notification settings - Fork 89
feat: Implement NoopAuthManager and integrate it with RestCatalog #544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
lishuxu
commented
Jan 29, 2026
- Add NoopAuthManager for "none" authentication type
- Register "none" auth type in static registry with case-insensitive matching
- Add KnownAuthTypes() to distinguish known-but-unimplemented types (NotImplemented) from unknown types (InvalidArgument)
- Integrate AuthManager into RestCatalog
| /// \brief Fetch server configuration from the REST catalog server. | ||
| Result<CatalogConfig> FetchServerConfig( | ||
| const ResourcePaths& paths, const RestCatalogProperties& current_config, | ||
| const std::shared_ptr<auth::AuthSession>& session) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const std::shared_ptr<auth::AuthSession>& session) { | |
| const auth::AuthSession& session) { |
If the session cannot be null and it is read-only, let's use its reference directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the session argument type to auth::AuthSession&. Authenticate() is non-const to support OAuth2 token refresh.
|
|
||
| std::string_view RestCatalog::name() const { return name_; } | ||
|
|
||
| Result<std::unordered_map<std::string, std::string>> RestCatalog::AuthHeaders() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better to add AuthSession* session as an optional input parameter to HttpClient::Get (and its friends)? Auth headers should be handled internally in the client instead of scattering them around here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current design keeps HttpClient focused on HTTP transport concerns, while RestCatalog handles authentication as a business-layer responsibility. So I think current design may be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is much cleaner to hide the auth api call in the http client to avoid the repeated Authenticate call. In addition, for auth type like SigV4, the authentication process requires more than headers so it requires involvement of the http client as well. This is the same pattern used by the Java RESTClient (though it uses withAuthSession as a stateful api which we don't have to mimic).
|
|
||
| std::string_view RestCatalog::name() const { return name_; } | ||
|
|
||
| Result<std::unordered_map<std::string, std::string>> RestCatalog::AuthHeaders() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is much cleaner to hide the auth api call in the http client to avoid the repeated Authenticate call. In addition, for auth type like SigV4, the authentication process requires more than headers so it requires involvement of the http client as well. This is the same pattern used by the Java RESTClient (though it uses withAuthSession as a stateful api which we don't have to mimic).
34f08f9 to
cc804e8
Compare
948d293 to
5574893
Compare
| return []([[maybe_unused]] std::string_view name, | ||
| [[maybe_unused]] const std::unordered_map<std::string, std::string>& props) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return []([[maybe_unused]] std::string_view name, | |
| [[maybe_unused]] const std::unordered_map<std::string, std::string>& props) | |
| return [](std::string_view name, | |
| const std::unordered_map<std::string, std::string>& props) |
| HttpClient& operator=(HttpClient&&) = delete; | ||
|
|
||
| /// \brief Set the authentication session | ||
| void SetAuthSession(auth::AuthSession* session); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the right approach to introduce a stateful http client. Let's remove this function and extend below functions to accept a session like:
Result<HttpResponse> Get(
const std::string& path,
const std::unordered_map<std::string, std::string>& params,
const std::unordered_map<std::string, std::string>& headers,
const ErrorHandler& error_handler,
AuthSession& session);The input session is a reference so it is required and a no-op session should be used if authentication is not enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Removed SetAuthSession and changed all HTTP methods to accept AuthSession& as a required reference parameter. When authentication is not enabled, NoopAuthManager produces a noop session via AuthSession::MakeDefault({})
| ICEBERG_ASSIGN_OR_RAISE( | ||
| const auto response, | ||
| client_->Get(path, params, /*headers=*/{}, *NamespaceErrorHandler::Instance())); | ||
| client_->Get(path, params, *NamespaceErrorHandler::Instance())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to use contextual session as the Java impl does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have a session catalog yet, so there's no session context to derive a contextual session from. I think using catalog_session_ directly is equivalent to what Java's RESTCatalog does. Will add contextual session support when session-aware catalog is supported.