From ad6981e04f43b56ce0d1b41bbb6e5c20688939c0 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 21 Aug 2026 16:07:31 +0100 Subject: [PATCH 1/4] Add config loading test To ensure config files stay backwards compatible --- src/client/config.rs | 71 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/src/client/config.rs b/src/client/config.rs index da7d8ab..c1c9141 100644 --- a/src/client/config.rs +++ b/src/client/config.rs @@ -25,6 +25,8 @@ pub enum ConfigFileError { } impl ClientConfiguration { + const DEFAULT_CLIENT: &str = "numtracker"; + pub async fn from_file>(path: P) -> Result { debug!("Reading client config from {:?}", path.as_ref()); match fs::read_to_string(path.as_ref()).await { @@ -78,3 +80,72 @@ impl Display for ClientConfiguration { Ok(()) } } + +#[cfg(test)] +mod tests { + use std::fs::File; + use std::io::Write; + + use tempfile::TempDir; + + use super::*; + + const HOST: &str = "http://numtracker.example.com"; + const AUTH: &str = "https://auth.example.com"; + const CLIENT_ID: &str = "custom_client"; + + #[tokio::test] + async fn load_from_file() { + let dir = TempDir::new().unwrap(); + let cfg_file = dir.as_ref().join("config.toml"); + let mut file = File::create_new(&cfg_file).unwrap(); + write!(file, "host={HOST:?}\n").unwrap(); + + let cfg = ClientConfiguration::from_file(cfg_file).await.unwrap(); + assert_eq!(cfg.host, Some(Url::parse(HOST).unwrap())); + assert_eq!(cfg.auth, None); + assert_eq!(cfg.client_id, None); + + assert_eq!(cfg.auth_config(), None); + } + + #[tokio::test] + async fn load_from_file_with_auth() { + let dir = TempDir::new().unwrap(); + let cfg_file = dir.as_ref().join("config.toml"); + let mut file = File::create_new(&cfg_file).unwrap(); + write!(file, "host={HOST:?}\nauth={AUTH:?}\n").unwrap(); + + let cfg = ClientConfiguration::from_file(cfg_file).await.unwrap(); + assert_eq!(cfg.host, Some(Url::parse(HOST).unwrap())); + assert_eq!(cfg.auth, Some(Url::parse(AUTH).unwrap())); + assert_eq!(cfg.client_id, None); + + assert_eq!( + cfg.auth_config(), + Some((&Url::parse(AUTH).unwrap(), "numtracker")) + ); + } + + #[tokio::test] + async fn load_from_file_with_client_id() { + let dir = TempDir::new().unwrap(); + let cfg_file = dir.as_ref().join("config.toml"); + let mut file = File::create_new(&cfg_file).unwrap(); + write!( + file, + "host={HOST:?}\nauth={AUTH:?}\nclient_id={CLIENT_ID:?}\n" + ) + .unwrap(); + + let cfg = ClientConfiguration::from_file(cfg_file).await.unwrap(); + assert_eq!(cfg.host, Some(Url::parse(HOST).unwrap())); + assert_eq!(cfg.auth, Some(Url::parse(AUTH).unwrap())); + assert_eq!(cfg.client_id, Some(CLIENT_ID.into())); + + assert_eq!( + cfg.auth_config(), + Some((&Url::parse(AUTH).unwrap(), CLIENT_ID)) + ); + } +} From f64b2aaa35d5ed6320f09a69429d44bee21250d7 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 21 Aug 2026 15:24:25 +0100 Subject: [PATCH 2/4] Add configuration for client-id used for client auth --- src/cli/client.rs | 19 +++++++++++-------- src/cli/mod.rs | 2 +- src/client/cli_auth.rs | 11 +++++++---- src/client/config.rs | 16 ++++++++++++++-- src/client/mod.rs | 15 ++++++++------- src/main.rs | 2 +- 6 files changed, 42 insertions(+), 23 deletions(-) diff --git a/src/cli/client.rs b/src/cli/client.rs index 3b6e398..2e60101 100644 --- a/src/cli/client.rs +++ b/src/cli/client.rs @@ -4,14 +4,6 @@ use url::Url; #[derive(Debug, Parser)] #[clap(max_term_width = 100)] pub struct ClientOptions { - #[clap(flatten)] - pub connection: ConnectionOptions, - #[clap(subcommand)] - pub command: ClientCommand, -} - -#[derive(Debug, Parser)] -pub struct ConnectionOptions { /// The host address of the numtracker service /// /// This should be the root of the service address including the scheme and @@ -19,6 +11,14 @@ pub struct ConnectionOptions { /// eg https://numtracker.example.com #[clap(long, short = 'H', env = "NUMTRACKER_SERVICE_HOST")] pub host: Option, + #[clap(flatten)] + pub auth: AuthConfig, + #[clap(subcommand)] + pub command: ClientCommand, +} + +#[derive(Debug, Parser)] +pub struct AuthConfig { /// The host address of the authorisation provider /// /// This should be the domain that has the .well-known/openid-configuration @@ -26,6 +26,9 @@ pub struct ConnectionOptions { /// eg https://authn.example.com/realms/master #[clap(long, env = "NUMTRACKER_AUTH_HOST")] pub auth: Option, + /// The client ID to use when authenticating + #[clap(long, env = "NUMTRACKER_AUTHN_CLIENTID")] + pub client_id: Option, } #[derive(Debug, Subcommand)] diff --git a/src/cli/mod.rs b/src/cli/mod.rs index f029203..2fe6df3 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -71,7 +71,7 @@ pub enum Command { Client { _ignored: Vec }, /// View and update beamline configurations provided by an instance of the service #[cfg(feature = "client")] - Client(client::ClientOptions), + Client(Box), /// Generate the graphql schema Schema, } diff --git a/src/client/cli_auth.rs b/src/client/cli_auth.rs index 0568548..62482ba 100644 --- a/src/client/cli_auth.rs +++ b/src/client/cli_auth.rs @@ -96,7 +96,10 @@ pub enum AuthError { } impl AuthHandler { - pub async fn new(host: impl Into) -> Result { + pub async fn new( + host: impl Into, + client_id: impl Into, + ) -> Result { let http_client = reqwest::ClientBuilder::new() .redirect(Policy::none()) .build()?; @@ -109,7 +112,7 @@ impl AuthHandler { .clone(); let client = CoreClient::from_provider_metadata( meta_provider, - ClientId::new("numtracker".to_string()), + ClientId::new(client_id.into()), None, ) .set_device_authorization_url(device_authorization_url) @@ -203,9 +206,9 @@ async fn refresh_access_token(auth: &AuthHandler) -> Option { /// Get a new access token from the auth server via the device flow. /// If successful, cache the refresh token to prevent needing to log in next time -pub(crate) async fn get_access_token(h: &Url) -> Result { +pub(crate) async fn get_access_token(host: &Url, client_id: &str) -> Result { debug!("Getting new access token"); - let handler = AuthHandler::new(h.clone()).await?; + let handler = AuthHandler::new(host.clone(), client_id).await?; if let Some(token) = refresh_access_token(&handler).await { return Ok(token); } diff --git a/src/client/config.rs b/src/client/config.rs index c1c9141..e809029 100644 --- a/src/client/config.rs +++ b/src/client/config.rs @@ -8,10 +8,13 @@ use tokio::fs; use tracing::debug; use url::Url; +use crate::cli::client::AuthConfig; + #[derive(Debug, Deserialize, Default)] pub struct ClientConfiguration { pub host: Option, pub auth: Option, + pub client_id: Option, } #[derive(Debug, Display, Error, From)] @@ -53,13 +56,22 @@ impl ClientConfiguration { } } + pub(crate) fn auth_config(&self) -> Option<(&Url, &str)> { + let auth = self.auth.as_ref()?; + Some(( + auth, + self.client_id.as_deref().unwrap_or(Self::DEFAULT_CLIENT), + )) + } + pub(crate) fn with_host(mut self, host: Option) -> Self { self.host = host.or(self.host); self } - pub(crate) fn with_auth(mut self, auth: Option) -> Self { - self.auth = auth.or(self.auth); + pub(crate) fn with_auth(mut self, auth: AuthConfig) -> Self { + self.auth = auth.auth.or(self.auth); + self.client_id = auth.client_id.or(self.client_id); self } } diff --git a/src/client/mod.rs b/src/client/mod.rs index 1355d5c..a133ee0 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -21,14 +21,15 @@ pub enum ClientError { pub async fn run_client(options: ClientOptions) { let ClientOptions { - connection, + host, + auth, command, } = options; let conf = match ClientConfiguration::from_default_file().await { Ok(conf) => { info!("Configuration from file: {conf}"); - conf.with_host(connection.host).with_auth(connection.auth) + conf.with_host(host).with_auth(auth) } Err(e) => { println!("Could not read configuration: {e}"); @@ -92,15 +93,15 @@ struct ConfigureMutation; impl NumtrackerClient { async fn from_config(config: ClientConfiguration) -> Result { + let auth = match config.auth_config() { + Some((auth, client_id)) => Some(cli_auth::get_access_token(auth, client_id).await?), + None => None, + }; + let host = config.host.unwrap_or_else(|| { info!("No host specified, defaulting to localhost:8000"); Url::parse("http://localhost:8000").expect("Constant URL is valid") }); - - let auth = match config.auth { - Some(auth) => Some(cli_auth::get_access_token(&auth).await?), - None => None, - }; info!("Querying {host} with auth: {auth:?}"); Ok(NumtrackerClient { auth, host }) } diff --git a/src/main.rs b/src/main.rs index 6ae99fa..2be7815 100644 --- a/src/main.rs +++ b/src/main.rs @@ -38,7 +38,7 @@ async fn main() -> Result<(), Box> { println!("Client subcommand requires 'client' feature to be enabled when building") } #[cfg(feature = "client")] - Command::Client(opts) => client::run_client(opts).await, + Command::Client(opts) => client::run_client(*opts).await, Command::Schema => { graphql::graphql_schema(std::io::stdout()).expect("Failed to write schema") } From f5eed26898160564cd287e6c27f8ef37f091c4b3 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Fri, 21 Aug 2026 16:28:23 +0100 Subject: [PATCH 3/4] Add client command CLI tests --- src/cli/client.rs | 2 +- src/cli/mod.rs | 19 +++++++++++++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/cli/client.rs b/src/cli/client.rs index 2e60101..69e7040 100644 --- a/src/cli/client.rs +++ b/src/cli/client.rs @@ -17,7 +17,7 @@ pub struct ClientOptions { pub command: ClientCommand, } -#[derive(Debug, Parser)] +#[derive(Debug, Parser, PartialEq)] pub struct AuthConfig { /// The host address of the authorisation provider /// diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 2fe6df3..2742c39 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -202,8 +202,7 @@ mod tests { use clap::Parser; use tracing::Level; - use super::Cli; - use crate::cli::Command; + use super::*; const APP: &str = "numtracker"; #[test] @@ -413,4 +412,20 @@ mod tests { panic!("Unexpected command: {:?}", cli.command); }; } + + #[test] + fn minimal_client_command() { + let cli = Cli::try_parse_from([APP, "client", "configuration"]).unwrap(); + let Command::Client(opts) = cli.command else { + panic!("Client command returned {cli:?}"); + }; + assert_eq!(opts.host, None); + assert_eq!( + opts.auth, + client::AuthConfig { + auth: None, + client_id: None + } + ); + } } From c98d96a656a777989bf145b0365f9f0637dda8ab Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Tue, 25 Aug 2026 16:19:04 +0100 Subject: [PATCH 4/4] Use writeln instead of write --- src/client/config.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/client/config.rs b/src/client/config.rs index e809029..cf11403 100644 --- a/src/client/config.rs +++ b/src/client/config.rs @@ -111,7 +111,7 @@ mod tests { let dir = TempDir::new().unwrap(); let cfg_file = dir.as_ref().join("config.toml"); let mut file = File::create_new(&cfg_file).unwrap(); - write!(file, "host={HOST:?}\n").unwrap(); + writeln!(file, "host={HOST:?}").unwrap(); let cfg = ClientConfiguration::from_file(cfg_file).await.unwrap(); assert_eq!(cfg.host, Some(Url::parse(HOST).unwrap())); @@ -126,7 +126,7 @@ mod tests { let dir = TempDir::new().unwrap(); let cfg_file = dir.as_ref().join("config.toml"); let mut file = File::create_new(&cfg_file).unwrap(); - write!(file, "host={HOST:?}\nauth={AUTH:?}\n").unwrap(); + writeln!(file, "host={HOST:?}\nauth={AUTH:?}").unwrap(); let cfg = ClientConfiguration::from_file(cfg_file).await.unwrap(); assert_eq!(cfg.host, Some(Url::parse(HOST).unwrap())); @@ -144,9 +144,9 @@ mod tests { let dir = TempDir::new().unwrap(); let cfg_file = dir.as_ref().join("config.toml"); let mut file = File::create_new(&cfg_file).unwrap(); - write!( + writeln!( file, - "host={HOST:?}\nauth={AUTH:?}\nclient_id={CLIENT_ID:?}\n" + "host={HOST:?}\nauth={AUTH:?}\nclient_id={CLIENT_ID:?}" ) .unwrap();