Support a client-id allowlist (#62)

This will support setting up publicly-accessible personal servers, without also allowing anyone to create a new client.
GothenburgBitFactory · Nov 22, 2024 · 50d028f · 50d028f
1 parent 5ad3b8e
commit 50d028f
Show file tree

Hide file tree

Showing 8 changed files with 188 additions and 53 deletions.
diff --git a/README.md b/README.md
@@ -19,6 +19,19 @@ It is comprised of three crates:
  - `taskchmpaion-sync-server-sqlite` implements an SQLite backend for the core
  - `taskchampion-sync-server` implements a simple HTTP server for the protocol
 
+## Running the Server
+
+The server is configured with command-line options. See
+`taskchampion-sync-server --help` for full details.
+
+The `--data-dir` option specifies where the server should store its data, and
+`--port` gives the port on which the HTTP server runs. The server does not
+implement TLS; for public deployments, the recommendation is to use a reverse
+proxy such as Nginx, haproxy, or Apache httpd.
+
+By default, the server allows all client IDs. To limit the accepted client IDs,
+such as when running a personal server, use `--allow-client-id <client-id>`.
+
 ## Installation
 
 ### As container

diff --git a/server/src/api/add_snapshot.rs b/server/src/api/add_snapshot.rs
@@ -1,4 +1,4 @@
-use crate::api::{client_id_header, server_error_to_actix, ServerState, SNAPSHOT_CONTENT_TYPE};
+use crate::api::{server_error_to_actix, ServerState, SNAPSHOT_CONTENT_TYPE};
 use actix_web::{error, post, web, HttpMessage, HttpRequest, HttpResponse, Result};
 use futures::StreamExt;
 use std::sync::Arc;
@@ -29,7 +29,7 @@ pub(crate) async fn service(
         return Err(error::ErrorBadRequest("Bad content-type"));
     }
 
-    let client_id = client_id_header(&req)?;
+    let client_id = server_state.client_id_header(&req)?;
 
     // read the body in its entirety
     let mut body = web::BytesMut::new();
@@ -75,7 +75,7 @@ mod test {
             txn.add_version(client_id, version_id, NIL_VERSION_ID, vec![])?;
         }
 
-        let server = WebServer::new(Default::default(), storage);
+        let server = WebServer::new(Default::default(), None, storage);
         let app = App::new().configure(|sc| server.config(sc));
         let app = test::init_service(app).await;
 
@@ -117,7 +117,7 @@ mod test {
             txn.new_client(client_id, NIL_VERSION_ID).unwrap();
         }
 
-        let server = WebServer::new(Default::default(), storage);
+        let server = WebServer::new(Default::default(), None, storage);
         let app = App::new().configure(|sc| server.config(sc));
         let app = test::init_service(app).await;
 
@@ -147,7 +147,7 @@ mod test {
         let client_id = Uuid::new_v4();
         let version_id = Uuid::new_v4();
         let storage = InMemoryStorage::new();
-        let server = WebServer::new(Default::default(), storage);
+        let server = WebServer::new(Default::default(), None, storage);
         let app = App::new().configure(|sc| server.config(sc));
         let app = test::init_service(app).await;
 
@@ -167,7 +167,7 @@ mod test {
         let client_id = Uuid::new_v4();
         let version_id = Uuid::new_v4();
         let storage = InMemoryStorage::new();
-        let server = WebServer::new(Default::default(), storage);
+        let server = WebServer::new(Default::default(), None, storage);
         let app = App::new().configure(|sc| server.config(sc));
         let app = test::init_service(app).await;
 

diff --git a/server/src/api/add_version.rs b/server/src/api/add_version.rs
@@ -1,7 +1,6 @@
 use crate::api::{
-    client_id_header, failure_to_ise, server_error_to_actix, ServerState,
-    HISTORY_SEGMENT_CONTENT_TYPE, PARENT_VERSION_ID_HEADER, SNAPSHOT_REQUEST_HEADER,
-    VERSION_ID_HEADER,
+    failure_to_ise, server_error_to_actix, ServerState, HISTORY_SEGMENT_CONTENT_TYPE,
+    PARENT_VERSION_ID_HEADER, SNAPSHOT_REQUEST_HEADER, VERSION_ID_HEADER,
 };
 use actix_web::{error, post, web, HttpMessage, HttpRequest, HttpResponse, Result};
 use futures::StreamExt;
@@ -40,7 +39,7 @@ pub(crate) async fn service(
         return Err(error::ErrorBadRequest("Bad content-type"));
     }
 
-    let client_id = client_id_header(&req)?;
+    let client_id = server_state.client_id_header(&req)?;
 
     // read the body in its entirety
     let mut body = web::BytesMut::new();
@@ -116,7 +115,7 @@ mod test {
             txn.new_client(client_id, Uuid::nil()).unwrap();
         }
 
-        let server = WebServer::new(Default::default(), storage);
+        let server = WebServer::new(Default::default(), None, storage);
         let app = App::new().configure(|sc| server.config(sc));
         let app = test::init_service(app).await;
 
@@ -150,7 +149,7 @@ mod test {
         let client_id = Uuid::new_v4();
         let version_id = Uuid::new_v4();
         let parent_version_id = Uuid::new_v4();
-        let server = WebServer::new(Default::default(), InMemoryStorage::new());
+        let server = WebServer::new(Default::default(), None, InMemoryStorage::new());
         let app = App::new().configure(|sc| server.config(sc));
         let app = test::init_service(app).await;
 
@@ -201,7 +200,7 @@ mod test {
             txn.new_client(client_id, version_id).unwrap();
         }
 
-        let server = WebServer::new(Default::default(), storage);
+        let server = WebServer::new(Default::default(), None, storage);
         let app = App::new().configure(|sc| server.config(sc));
         let app = test::init_service(app).await;
 
@@ -229,7 +228,7 @@ mod test {
         let client_id = Uuid::new_v4();
         let parent_version_id = Uuid::new_v4();
         let storage = InMemoryStorage::new();
-        let server = WebServer::new(Default::default(), storage);
+        let server = WebServer::new(Default::default(), None, storage);
         let app = App::new().configure(|sc| server.config(sc));
         let app = test::init_service(app).await;
 
@@ -249,7 +248,7 @@ mod test {
         let client_id = Uuid::new_v4();
         let parent_version_id = Uuid::new_v4();
         let storage = InMemoryStorage::new();
-        let server = WebServer::new(Default::default(), storage);
+        let server = WebServer::new(Default::default(), None, storage);
         let app = App::new().configure(|sc| server.config(sc));
         let app = test::init_service(app).await;
 

diff --git a/server/src/api/get_child_version.rs b/server/src/api/get_child_version.rs
@@ -1,6 +1,6 @@
 use crate::api::{
-    client_id_header, server_error_to_actix, ServerState, HISTORY_SEGMENT_CONTENT_TYPE,
-    PARENT_VERSION_ID_HEADER, VERSION_ID_HEADER,
+    server_error_to_actix, ServerState, HISTORY_SEGMENT_CONTENT_TYPE, PARENT_VERSION_ID_HEADER,
+    VERSION_ID_HEADER,
 };
 use actix_web::{error, get, web, HttpRequest, HttpResponse, Result};
 use std::sync::Arc;
@@ -21,7 +21,7 @@ pub(crate) async fn service(
     path: web::Path<VersionId>,
 ) -> Result<HttpResponse> {
     let parent_version_id = path.into_inner();
-    let client_id = client_id_header(&req)?;
+    let client_id = server_state.client_id_header(&req)?;
 
     return match server_state
         .server
@@ -70,7 +70,7 @@ mod test {
                 .unwrap();
         }
 
-        let server = WebServer::new(Default::default(), storage);
+        let server = WebServer::new(Default::default(), None, storage);
         let app = App::new().configure(|sc| server.config(sc));
         let app = test::init_service(app).await;
 
@@ -104,7 +104,7 @@ mod test {
         let client_id = Uuid::new_v4();
         let parent_version_id = Uuid::new_v4();
         let storage = InMemoryStorage::new();
-        let server = WebServer::new(Default::default(), storage);
+        let server = WebServer::new(Default::default(), None, storage);
         let app = App::new().configure(|sc| server.config(sc));
         let app = test::init_service(app).await;
 
@@ -132,7 +132,7 @@ mod test {
             txn.add_version(client_id, test_version_id, NIL_VERSION_ID, b"vers".to_vec())
                 .unwrap();
         }
-        let server = WebServer::new(Default::default(), storage);
+        let server = WebServer::new(Default::default(), None, storage);
         let app = App::new().configure(|sc| server.config(sc));
         let app = test::init_service(app).await;
 

diff --git a/server/src/api/get_snapshot.rs b/server/src/api/get_snapshot.rs
@@ -1,6 +1,4 @@
-use crate::api::{
-    client_id_header, server_error_to_actix, ServerState, SNAPSHOT_CONTENT_TYPE, VERSION_ID_HEADER,
-};
+use crate::api::{server_error_to_actix, ServerState, SNAPSHOT_CONTENT_TYPE, VERSION_ID_HEADER};
 use actix_web::{error, get, web, HttpRequest, HttpResponse, Result};
 use std::sync::Arc;
 
@@ -17,7 +15,7 @@ pub(crate) async fn service(
     req: HttpRequest,
     server_state: web::Data<Arc<ServerState>>,
 ) -> Result<HttpResponse> {
-    let client_id = client_id_header(&req)?;
+    let client_id = server_state.client_id_header(&req)?;
 
     if let Some((version_id, data)) = server_state
         .server
@@ -54,7 +52,7 @@ mod test {
             txn.new_client(client_id, Uuid::new_v4()).unwrap();
         }
 
-        let server = WebServer::new(Default::default(), storage);
+        let server = WebServer::new(Default::default(), None, storage);
         let app = App::new().configure(|sc| server.config(sc));
         let app = test::init_service(app).await;
 
@@ -90,7 +88,7 @@ mod test {
             .unwrap();
         }
 
-        let server = WebServer::new(Default::default(), storage);
+        let server = WebServer::new(Default::default(), None, storage);
         let app = App::new().configure(|sc| server.config(sc));
         let app = test::init_service(app).await;
 

diff --git a/server/src/api/mod.rs b/server/src/api/mod.rs
@@ -1,5 +1,8 @@
+use std::collections::HashSet;
+
 use actix_web::{error, web, HttpRequest, Result, Scope};
 use taskchampion_sync_server_core::{ClientId, Server, ServerError};
+use uuid::Uuid;
 
 mod add_snapshot;
 mod add_version;
@@ -28,6 +31,28 @@ pub(crate) const SNAPSHOT_REQUEST_HEADER: &str = "X-Snapshot-Request";
 /// The type containing a reference to the persistent state for the server
 pub(crate) struct ServerState {
     pub(crate) server: Server,
+    pub(crate) client_id_allowlist: Option<HashSet<Uuid>>,
+}
+
+impl ServerState {
+    /// Get the client id
+    fn client_id_header(&self, req: &HttpRequest) -> Result<ClientId> {
+        fn badrequest() -> error::Error {
+            error::ErrorBadRequest("bad x-client-id")
+        }
+        if let Some(client_id_hdr) = req.headers().get(CLIENT_ID_HEADER) {
+            let client_id = client_id_hdr.to_str().map_err(|_| badrequest())?;
+            let client_id = ClientId::parse_str(client_id).map_err(|_| badrequest())?;
+            if let Some(allow_list) = &self.client_id_allowlist {
+                if !allow_list.contains(&client_id) {
+                    return Err(error::ErrorForbidden("unknown x-client-id"));
+                }
+            }
+            Ok(client_id)
+        } else {
+            Err(badrequest())
+        }
+    }
 }
 
 pub(crate) fn api_scope() -> Scope {
@@ -51,16 +76,46 @@ fn server_error_to_actix(err: ServerError) -> actix_web::Error {
     }
 }
 
-/// Get the client id
-fn client_id_header(req: &HttpRequest) -> Result<ClientId> {
-    fn badrequest() -> error::Error {
-        error::ErrorBadRequest("bad x-client-id")
+#[cfg(test)]
+mod test {
+    use super::*;
+    use taskchampion_sync_server_core::InMemoryStorage;
+
+    #[test]
+    fn client_id_header_allow_all() {
+        let client_id = Uuid::new_v4();
+        let state = ServerState {
+            server: Server::new(Default::default(), InMemoryStorage::new()),
+            client_id_allowlist: None,
+        };
+        let req = actix_web::test::TestRequest::default()
+            .insert_header((CLIENT_ID_HEADER, client_id.to_string()))
+            .to_http_request();
+        assert_eq!(state.client_id_header(&req).unwrap(), client_id);
     }
-    if let Some(client_id_hdr) = req.headers().get(CLIENT_ID_HEADER) {
-        let client_id = client_id_hdr.to_str().map_err(|_| badrequest())?;
-        let client_id = ClientId::parse_str(client_id).map_err(|_| badrequest())?;
-        Ok(client_id)
-    } else {
-        Err(badrequest())
+
+    #[test]
+    fn client_id_header_allow_list() {
+        let client_id_ok = Uuid::new_v4();
+        let client_id_disallowed = Uuid::new_v4();
+        let state = ServerState {
+            server: Server::new(Default::default(), InMemoryStorage::new()),
+            client_id_allowlist: Some([client_id_ok].into()),
+        };
+        let req = actix_web::test::TestRequest::default()
+            .insert_header((CLIENT_ID_HEADER, client_id_ok.to_string()))
+            .to_http_request();
+        assert_eq!(state.client_id_header(&req).unwrap(), client_id_ok);
+        let req = actix_web::test::TestRequest::default()
+            .insert_header((CLIENT_ID_HEADER, client_id_disallowed.to_string()))
+            .to_http_request();
+        assert_eq!(
+            state
+                .client_id_header(&req)
+                .unwrap_err()
+                .as_response_error()
+                .status_code(),
+            403
+        );
     }
 }