fix(client): remove default trust identity from PoolConfig

Trust authentication is passwordless, so defaulting PoolConfig's
identity to username "admin" let a caller who forgot to configure auth
silently connect as whatever account that default named. Drop
PoolConfig::default and AuthMethod::Trust's serde username default,
require an explicit auth identity via PoolConfig::new /
ConnectionBuilder::build (now fallible), and update NativeClient,
tests, and the cluster test harness to supply identity explicitly.

Author: farhan-syah

nodedb-client-tests/tests/native_execute_sql_params_round_trip.rs

@@ -18,18 +18,14 @@ use nodedb_test_support::pgwire_harness::TestServer;
 async fn native_execute_sql_with_bound_params_round_trips() {
     let server = TestServer::start().await;
 
-    let pool = PoolConfig {
-        addr: format!("127.0.0.1:{}", server.native_port),
-        ..Default::default()
-    };
-    // The harness provisions superuser `nodedb`; override the default
-    // PoolConfig auth username from `admin`.
-    let pool = PoolConfig {
-        auth: nodedb_types::protocol::AuthMethod::Trust {
+    // The harness provisions superuser `nodedb`; `PoolConfig` has no default
+    // identity (see `PoolConfig::new`'s doc comment), so state it explicitly.
+    let pool = PoolConfig::new(
+        format!("127.0.0.1:{}", server.native_port),
+        nodedb_types::protocol::AuthMethod::Trust {
             username: "nodedb".into(),
         },
-        ..pool
-    };
+    );
     let native = NativeClient::new(pool);
 
     // Spec: a bound parameter binds to `$1` and the server returns a

nodedb-client/src/capabilities.rs

@@ -18,7 +18,7 @@ use nodedb_types::protocol::{
 ///
 /// # Example
 /// ```no_run
-/// # use nodedb_client::native::capabilities::Capabilities;
+/// # use nodedb_client::capabilities::Capabilities;
 /// let caps = Capabilities::from_raw(0x07); // streaming + graphrag + fts
 /// assert!(caps.supports_streaming());
 /// assert!(caps.supports_graphrag());

nodedb-client/src/native/builder.rs

@@ -8,11 +8,13 @@
 //!     .password("s3cr3t")
 //!     .database("analytics")
 //!     .max_connections(20)
-//!     .build();
+//!     .build()?;
+//! # Ok::<(), nodedb_types::error::NodeDbError>(())
 //! ```
 
 use std::time::Duration;
 
+use nodedb_types::error::{NodeDbError, NodeDbResult};
 use nodedb_types::protocol::AuthMethod;
 
 use super::client::NativeClient;
@@ -99,55 +101,162 @@ impl ConnectionBuilder {
 
     /// Build the `NativeClient`.
     ///
-    /// Falls back to sensible defaults for any unset option.
-    pub fn build(self) -> NativeClient {
+    /// The connection address falls back to `127.0.0.1:6433` when unset —
+    /// that is a convenience default, not a security decision. Identity is
+    /// the one option with no default: trust auth is passwordless, so a
+    /// defaulted username would silently authenticate as whatever that
+    /// default happened to be (privilege escalation by omission). Callers
+    /// must supply either [`api_key`](Self::api_key) (which carries its own
+    /// identity via the token) or [`username`](Self::username) — do not
+    /// reintroduce a fallback for either. Never add a default for `addr`
+    /// beyond the existing one either; keep the two concerns separate.
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if neither `api_key` nor `username` was set.
+    pub fn build(self) -> NodeDbResult<NativeClient> {
         let addr = self.addr.unwrap_or_else(|| "127.0.0.1:6433".to_string());
+        let auth = resolve_auth(self.username, self.password, self.api_key)?;
 
-        let auth = if let Some(token) = self.api_key {
-            AuthMethod::ApiKey { token }
-        } else if let Some(password) = self.password {
-            let username = self.username.unwrap_or_else(|| "admin".to_string());
-            AuthMethod::Password { username, password }
-        } else {
-            let username = self.username.unwrap_or_else(|| "admin".to_string());
-            AuthMethod::Trust { username }
-        };
-
-        let default_config = PoolConfig::default();
+        // `PoolConfig::new` already carries the identity (`auth`, always
+        // built above — never omitted) plus this crate's tuning defaults;
+        // only override the tuning fields the caller actually set.
+        let default_config = PoolConfig::new(addr, auth);
 
         let config = PoolConfig {
-            addr,
-            auth,
             database: self.database,
             max_size: self.max_connections.unwrap_or(default_config.max_size),
             connect_timeout: self
                 .connect_timeout
                 .unwrap_or(default_config.connect_timeout),
             idle_timeout: self.idle_timeout.unwrap_or(default_config.idle_timeout),
             tls: self.tls.unwrap_or_default(),
+            ..default_config
         };
 
-        NativeClient::new(config)
+        Ok(NativeClient::new(config))
+    }
+}
+
+/// Resolve the builder's optional identity fields into a required
+/// [`AuthMethod`].
+///
+/// This is a disjunction, not three independent defaults: an `api_key`
+/// carries its own identity via the token, so it alone is sufficient. The
+/// trust and password branches have no such built-in identity, so they
+/// require an explicit `username` — there is no fallback for either.
+fn resolve_auth(
+    username: Option<String>,
+    password: Option<String>,
+    api_key: Option<String>,
+) -> NodeDbResult<AuthMethod> {
+    if let Some(token) = api_key {
+        return Ok(AuthMethod::ApiKey { token });
     }
+    let username = username.ok_or_else(|| {
+        NodeDbError::config(
+            "no authentication identity configured: call .username(...) or .api_key(...)",
+        )
+    })?;
+    Ok(match password {
+        Some(password) => AuthMethod::Password { username, password },
+        None => AuthMethod::Trust { username },
+    })
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
 
     #[test]
-    fn builder_defaults() {
-        let client = ConnectionBuilder::new("127.0.0.1:6433").build();
-        let _ = client; // just verify it compiles
+    fn resolve_auth_with_username_carries_it_into_trust() {
+        let auth = resolve_auth(Some("alice".to_string()), None, None).expect("username given");
+        match auth {
+            AuthMethod::Trust { username } => assert_eq!(username, "alice"),
+            other => panic!("expected AuthMethod::Trust, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn resolve_auth_with_username_and_password_carries_it_into_password() {
+        let auth = resolve_auth(Some("bob".to_string()), Some("secret".to_string()), None)
+            .expect("username given");
+        match auth {
+            AuthMethod::Password { username, password } => {
+                assert_eq!(username, "bob");
+                assert_eq!(password, "secret");
+            }
+            other => panic!("expected AuthMethod::Password, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn resolve_auth_with_api_key_succeeds_without_username() {
+        let auth = resolve_auth(None, None, Some("token-123".to_string())).expect("api_key given");
+        assert!(matches!(auth, AuthMethod::ApiKey { token } if token == "token-123"));
+    }
+
+    #[test]
+    fn resolve_auth_with_no_identity_errors() {
+        // Regression lock: no username, no api_key — must fail, never
+        // silently authenticate as some default identity.
+        let err = resolve_auth(None, None, None).expect_err("must reject a missing identity");
+        assert!(err.message().contains("authentication identity"));
+    }
+
+    #[test]
+    fn resolve_auth_with_password_but_no_username_errors() {
+        let err = resolve_auth(None, Some("secret".to_string()), None)
+            .expect_err("must reject password auth without a username");
+        assert!(err.message().contains("authentication identity"));
+    }
+
+    #[test]
+    fn builder_with_username_succeeds() {
+        let client = ConnectionBuilder::new("127.0.0.1:6433")
+            .username("alice")
+            .build();
+        assert!(client.is_ok());
+    }
+
+    #[test]
+    fn builder_with_api_key_succeeds_without_username() {
+        let client = ConnectionBuilder::new("127.0.0.1:6433")
+            .api_key("token-123")
+            .build();
+        assert!(client.is_ok());
+    }
+
+    #[test]
+    fn builder_with_no_identity_errors() {
+        // Regression lock: no username, no api_key — must fail, never
+        // silently authenticate as some default identity.
+        // `NativeClient` is not `Debug` (it owns a live connection pool), so
+        // the Ok payload is discarded before `expect_err`.
+        let err = ConnectionBuilder::new("127.0.0.1:6433")
+            .build()
+            .map(|_| ())
+            .expect_err("build() must reject a missing identity");
+        assert!(err.message().contains("authentication identity"));
+    }
+
+    #[test]
+    fn builder_password_without_username_errors() {
+        let err = ConnectionBuilder::new("127.0.0.1:6433")
+            .password("secret")
+            .build()
+            .map(|_| ())
+            .expect_err("build() must reject password auth without a username");
+        assert!(err.message().contains("authentication identity"));
     }
 
     #[test]
     fn builder_with_database() {
-        // Smoke test: verify the builder accepts a database name without panic.
         let _client = ConnectionBuilder::new("127.0.0.1:6433")
             .username("alice")
             .database("analytics")
-            .build();
+            .build()
+            .expect("username was supplied, build() must succeed");
     }
 
     #[test]
@@ -156,6 +265,7 @@ mod tests {
             .username("bob")
             .password("secret")
             .database("prod")
-            .build();
+            .build()
+            .expect("username was supplied, build() must succeed");
     }
 }

nodedb-client/src/native/client/core.rs

@@ -3,6 +3,7 @@
 //! `NativeClient` struct definition and connection/session helpers.
 
 use nodedb_types::error::{ErrorDetails, NodeDbError, NodeDbResult};
+use nodedb_types::protocol::AuthMethod;
 use nodedb_types::result::QueryResult;
 
 use super::super::pool::{Pool, PoolConfig};
@@ -23,12 +24,16 @@ impl NativeClient {
         }
     }
 
-    /// Connect to a NodeDB server with default settings.
-    pub fn connect(addr: &str) -> Self {
-        Self::new(PoolConfig {
-            addr: addr.to_string(),
-            ..Default::default()
-        })
+    /// Connect to a NodeDB server, authenticating as `auth`.
+    ///
+    /// The identity is a required argument, not a default: trust
+    /// authentication is passwordless, so a default identity would let a
+    /// caller who forgot to configure auth connect as whatever account the
+    /// default names — privilege escalation by omission. Use
+    /// [`PoolConfig::new`] plus [`NativeClient::new`] directly if other pool
+    /// tuning (max size, timeouts, TLS, database) needs to be non-default too.
+    pub fn connect_as(addr: &str, auth: AuthMethod) -> Self {
+        Self::new(PoolConfig::new(addr, auth))
     }
 
     /// Execute a SQL query and return structured results.

nodedb-client/src/native/pool.rs

@@ -18,6 +18,10 @@ use tokio::sync::{Semaphore, SemaphorePermit};
 use super::connection::NativeConnection;
 
 /// Configuration for the connection pool.
+///
+/// There is no `Default` impl — construct via [`PoolConfig::new`], which
+/// requires an explicit `auth` identity. See that constructor's doc comment
+/// for why a defaulted identity is unsafe.
 #[derive(Debug, Clone)]
 pub struct PoolConfig {
     /// Server address (host:port).
@@ -28,7 +32,8 @@ pub struct PoolConfig {
     pub connect_timeout: Duration,
     /// Idle connection timeout (connections idle longer than this are dropped).
     pub idle_timeout: Duration,
-    /// Authentication method.
+    /// Authentication method. Required — no default identity (see
+    /// [`PoolConfig::new`]).
     pub auth: AuthMethod,
     /// Target database name sent in the auth handshake frame.
     ///
@@ -38,16 +43,40 @@ pub struct PoolConfig {
     pub tls: super::connection::TlsConfig,
 }
 
-impl Default for PoolConfig {
-    fn default() -> Self {
+impl PoolConfig {
+    /// Construct a pool configuration with an explicit connection address and
+    /// authentication identity.
+    ///
+    /// There is intentionally no `Default` impl for `PoolConfig`: trust
+    /// authentication is passwordless, so a server configured for trust auth
+    /// grants full privileges to whatever username the client claims. A
+    /// default identity (e.g. a hardcoded `admin`) would let a caller connect
+    /// as the server's most privileged account simply by forgetting to
+    /// configure auth — privilege escalation by omission. Every caller must
+    /// state who they are connecting as.
+    ///
+    /// Every other tuning field keeps its previous default and can still be
+    /// overridden via struct-update syntax:
+    ///
+    /// ```rust,ignore
+    /// use nodedb_client::native::pool::PoolConfig;
+    /// use nodedb_types::protocol::AuthMethod;
+    ///
+    /// let auth = AuthMethod::Trust {
+    ///     username: "alice".into(),
+    /// };
+    /// let config = PoolConfig {
+    ///     max_size: 2,
+    ///     ..PoolConfig::new("127.0.0.1:6433", auth)
+    /// };
+    /// ```
+    pub fn new(addr: impl Into<String>, auth: AuthMethod) -> Self {
         Self {
-            addr: "127.0.0.1:6433".into(),
+            addr: addr.into(),
             max_size: 10,
             connect_timeout: Duration::from_secs(5),
             idle_timeout: Duration::from_secs(300),
-            auth: AuthMethod::Trust {
-                username: "admin".into(),
-            },
+            auth,
             database: None,
             tls: Default::default(),
         }
@@ -222,19 +251,37 @@ impl Drop for PooledConnection<'_> {
 mod tests {
     use super::*;
 
+    fn trust(username: &str) -> AuthMethod {
+        AuthMethod::Trust {
+            username: username.to_string(),
+        }
+    }
+
     #[test]
-    fn pool_config_defaults() {
-        let cfg = PoolConfig::default();
+    fn pool_config_new_sets_tuning_defaults() {
+        let cfg = PoolConfig::new("127.0.0.1:6433", trust("alice"));
         assert_eq!(cfg.addr, "127.0.0.1:6433");
         assert_eq!(cfg.max_size, 10);
         assert_eq!(cfg.connect_timeout, Duration::from_secs(5));
     }
 
+    /// `PoolConfig::new` must carry the caller's identity through to the
+    /// built config unchanged — asserted explicitly so a future refactor
+    /// cannot quietly reintroduce a hardcoded default identity.
+    #[test]
+    fn pool_config_new_carries_caller_identity() {
+        let cfg = PoolConfig::new("127.0.0.1:6433", trust("alice"));
+        match cfg.auth {
+            AuthMethod::Trust { username } => assert_eq!(username, "alice"),
+            other => panic!("expected AuthMethod::Trust, got {other:?}"),
+        }
+    }
+
     #[test]
     fn pool_creates_semaphore() {
         let pool = Pool::new(PoolConfig {
             max_size: 5,
-            ..Default::default()
+            ..PoolConfig::new("127.0.0.1:6433", trust("alice"))
         });
         assert_eq!(pool.semaphore.available_permits(), 5);
     }