fix(auth): keep the refresh token the provider hands back

The reply to a refresh carries a new refresh token and the provider
stops honouring the one that was sent. Only the access half was read,
so the cache kept re-sending a token the server had already retired.

- deserialize the refresh half and write it back to the cache, as an
  Option so a provider that does not rotate leaves the stored one be
- bound the refresh request on its own: the caller holds a lock that
  spans processes while it runs, so a provider that goes quiet would
  stall every other request on the machine
- give config create/update one rule for what a remote URL is, and
  store it without the trailing slash, matching the Python client so
  the two can share config.ini; this drops the validators crate and
  77 transitive dependencies with it
- join the base URL and the API's relative download path with the
  separator neither of them carries, as download.rs already does

Cargo.toml

@@ -20,7 +20,6 @@ futures = "0.3"
 dirs = "6.0.0"
 anyhow = "1.0.102"
 configparser = "3.2.0"
-validators = "0.25.3"
 sha2 = "0.11.0"
 rpassword = "7.5.4"
 base64 = "0.22"

src/api/users.rs

@@ -2,6 +2,12 @@ use crate::connection::MDRSConnection;
 use crate::models::user::User as ModelUser;
 use anyhow::bail;
 use serde::Deserialize;
+use std::time::Duration;
+
+/// Bound the refresh on its own: the caller holds a lock that spans processes while this
+/// runs, so a provider that accepts the connection and then goes quiet would stall every
+/// other request on this machine rather than just this one.
+const TOKEN_REFRESH_TIMEOUT: Duration = Duration::from_secs(30);
 
 /// Full API response shape from GET v3/users/current/
 #[derive(Debug, Deserialize)]
@@ -20,6 +26,16 @@ struct UsersCurrentResponseLaboratory {
 #[derive(Deserialize)]
 struct TokenRefreshResponse {
     access: String,
+    /// Present when the provider rotates refresh tokens, absent when it does not,
+    /// so the caller keeps the token it already holds if nothing new arrives.
+    #[serde(default)]
+    refresh: Option<String>,
+}
+
+/// The token pair a refresh yields.
+pub struct RefreshedToken {
+    pub access: String,
+    pub refresh: Option<String>,
 }
 
 impl MDRSConnection {
@@ -40,19 +56,83 @@ impl MDRSConnection {
     }
 
     /// Refresh the access token using the refresh token.
-    /// POST v3/users/token/refresh/ {refresh: ...} -> {access: new_access}
-    pub async fn token_refresh(&self, refresh_token: &str) -> Result<String, anyhow::Error> {
+    /// POST v3/users/token/refresh/ {refresh: ...} -> {access, refresh?}
+    ///
+    /// A rotating provider answers with a new refresh token and stops honouring the one
+    /// that was sent, so both halves of the reply have to be kept.
+    pub async fn token_refresh(
+        &self,
+        refresh_token: &str,
+    ) -> Result<RefreshedToken, anyhow::Error> {
         let body = serde_json::json!({ "refresh": refresh_token });
         let resp = self
             .client
             .post(self.build_url("v3/users/token/refresh/"))
             .json(&body)
+            .timeout(TOKEN_REFRESH_TIMEOUT)
             .send()
             .await?;
         if !resp.status().is_success() {
             bail!("Token refresh failed: {}", resp.status());
         }
         let r: TokenRefreshResponse = resp.json().await?;
-        Ok(r.access)
+        Ok(RefreshedToken {
+            access: r.access,
+            refresh: r.refresh,
+        })
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use tokio::io::{AsyncReadExt, AsyncWriteExt};
+    use tokio::net::TcpListener;
+
+    async fn refresh_against_stub(body: &'static str) -> RefreshedToken {
+        let listener = TcpListener::bind("127.0.0.1:0").await.unwrap();
+        let addr = listener.local_addr().unwrap();
+
+        let server = tokio::spawn(async move {
+            let (mut stream, _) = listener.accept().await.unwrap();
+            let mut buf = [0u8; 4096];
+            let n = stream.read(&mut buf).await.unwrap();
+            let req = String::from_utf8_lossy(&buf[..n]);
+            assert!(req.starts_with("POST /v3/users/token/refresh/ HTTP/1.1"));
+            assert!(req.contains("\"refresh\":\"old-refresh\""));
+
+            let response = format!(
+                "HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: {}\r\nconnection: close\r\n\r\n{}",
+                body.len(),
+                body
+            );
+            stream.write_all(response.as_bytes()).await.unwrap();
+        });
+
+        let conn = MDRSConnection::new(&format!("http://{addr}"));
+        let refreshed = conn.token_refresh("old-refresh").await.unwrap();
+        server.await.unwrap();
+        refreshed
+    }
+
+    /// A rotating provider stops honouring the token that was sent, so the reply's
+    /// refresh token has to reach the caller rather than being dropped.
+    #[tokio::test]
+    async fn token_refresh_returns_the_rotated_refresh_token() {
+        let refreshed =
+            refresh_against_stub(r#"{"access":"new-access","refresh":"new-refresh"}"#).await;
+
+        assert_eq!(refreshed.access, "new-access");
+        assert_eq!(refreshed.refresh.as_deref(), Some("new-refresh"));
+    }
+
+    /// A provider that does not rotate answers with the access token alone, and the
+    /// caller keeps the refresh token it already holds.
+    #[tokio::test]
+    async fn token_refresh_reports_no_rotation_when_the_reply_omits_it() {
+        let refreshed = refresh_against_stub(r#"{"access":"new-access"}"#).await;
+
+        assert_eq!(refreshed.access, "new-access");
+        assert_eq!(refreshed.refresh, None);
     }
 }

src/cache/mod.rs

@@ -387,10 +387,13 @@ async fn refresh_and_persist_in_dir(
         .ok_or_else(|| anyhow!("Remote `{}` is not configured.", remote))?;
     let conn = MDRSConnection::new(&url);
 
-    let new_access = conn.token_refresh(&cache.token.refresh).await?;
+    let refreshed = conn.token_refresh(&cache.token.refresh).await?;
 
     let mut updated_cache = cache.clone();
-    updated_cache.token.access = new_access;
+    updated_cache.token.access = refreshed.access;
+    if let Some(refresh) = refreshed.refresh {
+        updated_cache.token.refresh = refresh;
+    }
     updated_cache.digest = compute_digest(
         updated_cache.user.as_ref(),
         &updated_cache.token.access,

src/commands/config.rs

@@ -49,9 +49,10 @@ pub fn get_remote_url(remote: &str) -> Result<Option<String>, anyhow::Error> {
 }
 
 pub fn config_create(remote: &str, url: &str) -> Result<(), anyhow::Error> {
-    if !validate_url(url) {
+    let Some(url) = normalize_url(url) else {
         bail!("Malformed URL");
-    }
+    };
+    let url = url.as_str();
     let path = config_path();
     sanitize_config_file(&path)?;
     let path_str = path.to_string_lossy().to_string();
@@ -78,9 +79,10 @@ pub fn config_create(remote: &str, url: &str) -> Result<(), anyhow::Error> {
 }
 
 pub fn config_update(remote: &str, url: &str) -> Result<(), anyhow::Error> {
-    if !validate_url(url) {
+    let Some(url) = normalize_url(url) else {
         bail!("Malformed URL");
-    }
+    };
+    let url = url.as_str();
     let path = config_path();
     sanitize_config_file(&path)?;
     let path_str = path.to_string_lossy().to_string();
@@ -145,6 +147,54 @@ pub fn config_delete(remote: &str) -> Result<(), anyhow::Error> {
     Ok(())
 }
 
-fn validate_url(url: &str) -> bool {
-    validators::url::Url::parse(url).is_ok()
+/// Check a remote URL and put it in the one form every client agrees on.
+///
+/// A bare hostname is accepted, so a development server on `localhost` is as acceptable
+/// as a deployment behind a domain name. The trailing slash goes because the URL is
+/// joined with a path that brings its own.
+fn normalize_url(url: &str) -> Option<String> {
+    let parsed = reqwest::Url::parse(url).ok()?;
+    if !matches!(parsed.scheme(), "http" | "https") {
+        return None;
+    }
+    parsed.host_str()?;
+    Some(url.trim_end_matches('/').to_string())
+}
+
+#[cfg(test)]
+mod url_tests {
+    use super::normalize_url;
+
+    /// Both clients share config.ini, so they have to agree on what a remote URL is.
+    #[test]
+    fn trailing_slash_is_dropped() {
+        assert_eq!(
+            normalize_url("http://127.0.0.1:8000/api/").as_deref(),
+            Some("http://127.0.0.1:8000/api")
+        );
+        assert_eq!(
+            normalize_url("https://neurodata.riken.jp/api/").as_deref(),
+            Some("https://neurodata.riken.jp/api")
+        );
+    }
+
+    #[test]
+    fn bare_hostname_is_accepted() {
+        assert_eq!(
+            normalize_url("http://localhost:8000/api").as_deref(),
+            Some("http://localhost:8000/api")
+        );
+    }
+
+    #[test]
+    fn only_http_schemes_are_accepted() {
+        for url in [
+            "ftp://x.example.com/",
+            "file:///etc/passwd",
+            "not-a-url",
+            "http://",
+        ] {
+            assert_eq!(normalize_url(url), None, "{url} should be rejected");
+        }
+    }
 }

src/commands/ls.rs

@@ -214,7 +214,13 @@ fn file_to_json(f: &File, base_url: &str) -> Value {
     let download_url = if f.download_url.starts_with("http") {
         f.download_url.clone()
     } else {
-        format!("{}{}", base_url.trim_end_matches('/'), f.download_url)
+        // The API answers with a relative path and no leading separator, so supply one
+        // rather than running the two together.
+        format!(
+            "{}/{}",
+            base_url.trim_end_matches('/'),
+            f.download_url.trim_start_matches('/')
+        )
     };
     json!({
         "id": f.id,