feat(rest): introduce AuthManager/AuthSession and migrate OAuth2#2838
feat(rest): introduce AuthManager/AuthSession and migrate OAuth2#2838plusplusjiajia wants to merge 1 commit into
Conversation
4730ec1 to
6f4aad8
Compare
6f4aad8 to
e61121f
Compare
| /// The auth manager living for the lifetime of the catalog. | ||
| auth_manager: Arc<dyn AuthManager>, | ||
| /// The session authenticating requests in the current phase. | ||
| session: Arc<dyn AuthSession>, |
There was a problem hiding this comment.
Carrying my feedback from the other PR over.
I think right now having the AuthManager and AuthSession as fields of the HttpClient is perfectly fine. However, we're going to need to add additional methods to the AuthManager trait which complicate this:
fn table_session(_: TableIdent, parent: Arc<dyn AuthSession>) -> Arc<dyn AuthSession>;
fn contextual_session(_: SessionContext, parent: Arc<dyn AuthSession>) -> Arc<dyn AuthSession;table_session() and contextual_session() will help us to enable credentials vending at table-level and query-level authentication respectively.
Both take arguments that a low-level HttpClient should have no business in dealing with IMO. E.g. TableIdent is Iceberg-specific and not HTTP-specific. Same goes for the SessionContext. In that sense, I feel like the RestCatalog may be a better place to host these two fields.
| /// Drops any cached credentials so the next request re-authenticates. | ||
| async fn invalidate(&self) -> Result<()> { | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Proactively refreshes cached credentials (e.g. re-exchanges an OAuth2 | ||
| /// client credential for a new token), leaving them intact on failure. | ||
| async fn refresh(&self) -> Result<()> { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
I wonder whether we should call out in the comments that these methods are only exposed for backwards-compatibility and that they aren't the intended main interface to work with going forward.
To give implementers of custom AuthManagers some guidance.
| /// The bearer token this session would attach, if any. Test-only: lets | ||
| /// tests observe the cached token without issuing a request. | ||
| #[cfg(test)] | ||
| async fn bearer_token(&self) -> Option<String> { |
There was a problem hiding this comment.
IIUC this method is only (indirectly) used in three tests.
I'd argue that its somewhat redundant with the fn authenticate() and test helpers that simplify the ergonomics should probably live closer to the tests rather than extending the trait (which is public API).
Test helpers could rebuild this functionality in a test module. A shortened version:
async fn bearer_token_from_session(session: &dyn AuthSession) -> Result<Option<String>> {
let header = authorization_header_from_session(session).await?;
let bearer_token = header
.map(|header| header.strip_prefix("Bearer "))
Ok(bearer_token)
}
async fn authorization_header_from_session(session: &dyn AuthSession) -> Result<Option<String>> {
let req = Request::new(Method::GET, Url::parse("http://fake.com")?);
let mut req = AuthRequest::new(req);
session.authenticate(&mut req).await?;
Ok(req.headers().get(AUTHORIZATION))
}| // Release the init-phase session before deriving the catalog session, | ||
| // so a manager whose init session guards a one-shot resource (released | ||
| // on drop) can build its catalog session without deadlocking. | ||
| drop(init_session); |
There was a problem hiding this comment.
I wonder whether we can instantiate the init_session in the scope of its use (the first /v1/config request) so that we don't have to deal with explicit drops.
This could be another signal that the AuthManager should rather live in the catalog because the HttpClient is not aware of which request is being made, and so it can't tell which session is the appropriate one to use (or to build).
In that sense, it's implicitly temporally coupled to what the session field has been set to, and has to assume that the first request being made is a /v1/config request.
| self.props | ||
| .get(REST_CATALOG_PROP_AUTH_TYPE) | ||
| .cloned() | ||
| .unwrap_or_else(|| AUTH_TYPE_OAUTH2.to_string()) |
There was a problem hiding this comment.
Just flagging that this diverges from Java's default to none.
Even though the OAuth2Manager behaves similarly without a token, it doesn't match the NoopAuthManager's behavior exactly. For example:
- a configured
NoopAuthManageron client initialization will always noop - anOAuth2Managercan start authenticating if the/v1/configendpoint returns a token in the properties (this is arguably the better default behavior) - a call to
NoopSession::refresh()will always succeed but a call onOAuth2Session::refresh()will fail if no token is backing it
Modeled on Java's
AuthManagerAPI (the init/catalog session lifecycle, the Noop/OAuth2 manager set, and the SigV4-wraps-a-delegate composition coming in the follow-up), adapted to Rust idioms.What it does
AuthManager/AuthSessiontraits in a newauth/module:init_session()serves theGET /v1/confighandshake,catalog_session(merged_props)serves everything after, so a manager can rebuild its session from server-merged properties.Noop/OAuth2managers, selected via a newrest.auth.typeproperty (oauth2is the default and behaves as no auth when neithertokennorcredentialis set), injectable throughRestCatalogBuilder::with_auth_manager.HttpClientintoOAuth2Manager, with the cached token surviving the config handshake;OAuth2Manageris publicly constructible (new()+with_*).auth_manager, and the test-only fake-request token shim is gone — tests observe the session's cached bearer (#[cfg(test)] bearer_token()) and assert the header the mock server receives.No new dependencies; no public API removed (additions only,
public-api.txtregenerated).Java reference:
org.apache.iceberg.rest.auth.Deviations from Java
tableSession/contextualSessionyet — in Java they aredefaultmethods falling back to the catalog/parent session, and the Rust REST catalog has no call sites for them (contextualSessionalso needs aSessionCatalogconcept that doesn't exist here yet). Adding defaulted trait methods later is non-breaking.close()— Rust relies onDrop, and this OAuth2 implementation has no background refresh executor to shut down.AuthSessiongainsinvalidate()/refresh()(not in Java) to back the existingRestCatalog::invalidate_token/regenerate_tokenAPIs.authenticatemutates the request in place instead of returning a new one.