diff --git a/Dockerfile.web-rust b/Dockerfile.web-rust index 76a95c124..786b0577c 100644 --- a/Dockerfile.web-rust +++ b/Dockerfile.web-rust @@ -166,6 +166,5 @@ STOPSIGNAL SIGTERM # reverse proxy for anything beyond a trusted network. The port is taken from # the build-time SERVE_PORT (baked into OPENPENCIL_SERVE_PORT); `sh -c` lets the # env var expand at container start. -CMD ["sh", "-c", "set -- --serve-web \"${OPENPENCIL_SERVE_PORT}\" --host 0.0.0.0; \ -if [ \"${OPENPENCIL_SERVE_MODE}\" = online ]; then set -- \"$@\" --online; fi; \ -exec /app/op-host-web-server \"$@\""] +CMD ["sh", "-c", "if [ \"${OPENPENCIL_SERVE_MODE}\" = online ]; then set -- --online; else set --; fi; \ +exec /app/op-host-web-server --serve-web \"${OPENPENCIL_SERVE_PORT}\" --host 0.0.0.0 \"$@\""] diff --git a/crates/op-editor-core/src/lib.rs b/crates/op-editor-core/src/lib.rs index 7bbb5d722..b0d12eed5 100644 --- a/crates/op-editor-core/src/lib.rs +++ b/crates/op-editor-core/src/lib.rs @@ -135,6 +135,7 @@ pub mod scene_template_prompt; pub mod scene_vars; pub mod selection; pub mod selection_resolve; +pub mod share_routes; pub mod state; pub mod statusbar_state; pub mod svg_import; diff --git a/crates/op-editor-core/src/share_routes.rs b/crates/op-editor-core/src/share_routes.rs new file mode 100644 index 000000000..0ec9a6fdb --- /dev/null +++ b/crates/op-editor-core/src/share_routes.rs @@ -0,0 +1,81 @@ +//! Tenant-sharing HTTP route paths, shared by the web shell (client) and the +//! serve-web daemon (server). +//! +//! Only the multi-account online deployment serves these; the local and +//! managed daemons have exactly one document and nobody to share it with. +//! Keeping the paths in one wasm-clean crate both sides already depend on is +//! the same arrangement `auth_routes` and `collab_routes` use. + +/// Prefix shared by every route below (used by the server's sensitive-POST / +/// CORS gating). +pub const API_PREFIX: &str = "/api/share/"; + +/// `POST` — add an account to the caller's own access list. +/// +/// Body: `{"userId":""}`. The grantor is the request's verified +/// identity and is never taken from the body. +pub const GRANT: &str = "/api/share/grant"; + +/// `POST` — remove an account from the caller's own access list. +pub const REVOKE: &str = "/api/share/revoke"; + +/// `GET` — who the caller shares with, and who shares with the caller. +pub const LIST: &str = "/api/share/list"; + +/// Query parameter naming the tenant a request is addressed to. +/// +/// A header would be the more usual choice, but `EventSource` cannot set +/// request headers, and `/api/mcp/events` is exactly the route a visitor +/// needs most — a shared document that does not push updates is not shared in +/// any useful sense. Rather than split the mechanism (header for XHR, query +/// for SSE) and have two places to get wrong, everything uses the query. +/// +/// The value is an account id. It is a REQUEST for access, never a grant of +/// it: the server still resolves the caller's own identity and checks it +/// against the owner's access list. +pub const TENANT_QUERY: &str = "tenant"; + +/// Read the tenant parameter out of a raw query string (no leading `?`). +/// +/// Deliberately tiny and dependency-free so both the wasm shell and the +/// daemon parse it identically. Percent-decoding is not attempted: an account +/// id is an opaque token, and a value needing escapes is not one. +pub fn tenant_from_query(query: &str) -> Option<&str> { + query + .split('&') + .filter_map(|pair| pair.split_once('=')) + .find(|(name, _)| *name == TENANT_QUERY) + .map(|(_, value)| value) + .filter(|value| !value.is_empty()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn the_tenant_parameter_is_read_out_of_a_query_string() { + assert_eq!(tenant_from_query("tenant=userA"), Some("userA")); + assert_eq!(tenant_from_query("x=1&tenant=userA&y=2"), Some("userA")); + assert_eq!(tenant_from_query("y=2&tenant=userA"), Some("userA")); + } + + #[test] + fn an_absent_or_empty_tenant_parameter_is_none() { + for query in ["", "x=1", "tenant=", "tenants=userA", "atenant=userA"] { + assert_eq!(tenant_from_query(query), None, "{query:?}"); + } + } + + #[test] + fn only_the_exact_parameter_name_matches() { + assert_eq!(tenant_from_query("Tenant=userA"), None); + } + + #[test] + fn every_route_sits_under_the_declared_prefix() { + for route in [GRANT, REVOKE, LIST] { + assert!(route.starts_with(API_PREFIX), "{route}"); + } + } +} diff --git a/crates/op-host-services/src/mcp_serve.rs b/crates/op-host-services/src/mcp_serve.rs index e958e402a..2b441eb80 100644 --- a/crates/op-host-services/src/mcp_serve.rs +++ b/crates/op-host-services/src/mcp_serve.rs @@ -375,6 +375,13 @@ pub struct HttpRequest { /// `Cookie` header value, verbatim, when present. The online daemon /// extracts its session cookie from it; every other mode ignores it. pub cookie: Option, + /// The raw query string (no leading `?`), when the target had one. + /// + /// `path` keeps its query stripped so exact-path routing is unaffected; + /// this is captured alongside so the online daemon can read the tenant + /// parameter. `EventSource` cannot set headers, which is why the tenant + /// travels in the query at all — see `share_routes::TENANT_QUERY`. + pub query: Option, } /// Parse a capped HTTP header and then read exactly its declared body length. @@ -418,13 +425,16 @@ pub fn read_http_request(stream: &mut S) -> Result ( + path.to_string(), + (!query.is_empty()).then(|| query.to_string()), + ), + None => (target.to_string(), None), + }; // Parse `Content-Length` via `split_once(':')` — byte-slicing a `&str` // (e.g. `l[..15]`) would panic if a crafted header puts a multibyte UTF-8 // boundary mid-slice; that panic would also bypass the live server's @@ -519,6 +529,7 @@ pub fn read_http_request(stream: &mut S) -> Result update_mcp_server_settings(body, state), @@ -733,8 +739,10 @@ mod online_run_loop; mod origin_guard; mod run_loop; mod serve_options; +mod share_routes; pub mod tenant; pub mod tenant_auth; +pub mod tenant_store; pub use collab_state::DaemonMutationRefusal; pub use connect_routes::*; @@ -747,10 +755,12 @@ pub use online_run_loop::*; use origin_guard::*; pub use run_loop::*; pub use serve_options::*; -pub use tenant::{TenantError, TenantLimits, TenantRegistry}; +pub use share_routes::ShareError; +pub use tenant::{TenantError, TenantLease, TenantLimits, TenantRegistry}; pub use tenant_auth::{ IdentityVerifier, OnlineAuthError, PresentedCredentials, ResolvedIdentity, StaticVerifier, }; +pub use tenant_store::{TenantStore, TenantStoreError}; #[cfg(test)] #[path = "web_canvas_server_tests.rs"] diff --git a/crates/op-host-services/src/web_canvas_server/collab_routes_tests.rs b/crates/op-host-services/src/web_canvas_server/collab_routes_tests.rs index e6b9561ed..21949d3af 100644 --- a/crates/op-host-services/src/web_canvas_server/collab_routes_tests.rs +++ b/crates/op-host-services/src/web_canvas_server/collab_routes_tests.rs @@ -301,6 +301,7 @@ fn the_collab_routes_are_gated_as_sensitive_browser_posts() { token: None, authorization: None, cookie: None, + query: None, }; assert!( super::super::is_sensitive_browser_post(&request), diff --git a/crates/op-host-services/src/web_canvas_server/online_policy.rs b/crates/op-host-services/src/web_canvas_server/online_policy.rs index 37c5f91a4..57f403a2c 100644 --- a/crates/op-host-services/src/web_canvas_server/online_policy.rs +++ b/crates/op-host-services/src/web_canvas_server/online_policy.rs @@ -28,6 +28,25 @@ pub enum ServeMode { } impl ServeMode { + /// Stable name for the wire, read by the browser shell. + pub const fn wire_name(self) -> &'static str { + match self { + Self::Local => "local", + Self::Managed => "managed", + Self::Online => "online", + } + } + + /// Whether the daemon is the SOLE sequencer for the document it serves. + /// + /// Online it is: every writer reaches the same in-memory document through + /// the same version counter, and the SSE stream publishes the result to + /// everyone. Locally it is not — the browser and the operator's file are + /// two copies and the daemon arbitrates neither. + pub const fn is_server_authoritative(self) -> bool { + self.is_online() + } + pub const fn is_online(self) -> bool { matches!(self, Self::Online) } diff --git a/crates/op-host-services/src/web_canvas_server/online_run_loop.rs b/crates/op-host-services/src/web_canvas_server/online_run_loop.rs index 5739d6266..b5c1bac53 100644 --- a/crates/op-host-services/src/web_canvas_server/online_run_loop.rs +++ b/crates/op-host-services/src/web_canvas_server/online_run_loop.rs @@ -269,7 +269,19 @@ pub(super) fn serve_one_online( )?; return Ok(false); } - let lease = match registry.lease_for(&identity) { + // Which tenant is this request addressed to? The query names an OWNER; + // whether this caller may reach it is still decided by the owner's access + // list, never by the parameter. Absent, it is the caller's own tenant. + let requested_owner = req + .query + .as_deref() + .and_then(op_editor_core::share_routes::tenant_from_query) + .map(str::to_string); + let lease = match requested_owner.as_deref() { + Some(owner) => registry.lease_for_shared(owner, &identity), + None => registry.lease_for(&identity), + }; + let lease = match lease { Ok(lease) => lease, Err(error) => { crate::mcp_serve::write_mcp_http_response( @@ -285,6 +297,43 @@ pub(super) fn serve_one_online( return Ok(false); } }; + // Sharing is administered on the CALLER's own tenant, so it is answered + // here rather than in the document tier — a visitor holding a `?tenant=` + // lease on someone else's document must not be able to re-share it. + if super::share_routes::is_share_route(&req.path) { + let own = match registry.lease_for(&identity) { + Ok(own) => own, + Err(error) => { + crate::mcp_serve::write_mcp_http_response_with_origin( + stream, + error.http_status(), + &serde_json::json!({ + "ok": false, + "error": error.code(), + "message": error.to_string(), + }) + .to_string(), + cors_origin.as_deref(), + )?; + return Ok(false); + } + }; + let reply = super::share_routes::handle( + &req.method, + &req.path, + &req.body, + &identity, + &own, + registry, + ); + crate::mcp_serve::write_mcp_http_response_with_origin( + stream, + reply.status, + &reply.body, + cors_origin.as_deref(), + )?; + return Ok(false); + } // The lease outlives the dispatch (including a minutes-long SSE stream), // so the tenant these borrows point at cannot be evicted underneath them. dispatch( diff --git a/crates/op-host-services/src/web_canvas_server/online_run_loop_tests.rs b/crates/op-host-services/src/web_canvas_server/online_run_loop_tests.rs index 862ca0f04..2758f7045 100644 --- a/crates/op-host-services/src/web_canvas_server/online_run_loop_tests.rs +++ b/crates/op-host-services/src/web_canvas_server/online_run_loop_tests.rs @@ -41,6 +41,9 @@ struct Request { content_type: Option<&'static str>, cookie: Option<&'static str>, origin: Option<&'static str>, + /// Addresses the request at another account's tenant, as the browser does + /// with `?tenant=` on the page URL. + tenant: Option<&'static str>, } impl Request { @@ -53,6 +56,7 @@ impl Request { content_type: None, cookie: None, origin: None, + tenant: None, } } @@ -97,11 +101,14 @@ impl Request { .origin .map(|o| format!("Origin: {o}\r\n")) .unwrap_or_default(); + let target = match self.tenant { + Some(tenant) => format!("{}?tenant={tenant}", self.path), + None => self.path.to_string(), + }; format!( - "{} {} HTTP/1.1\r\nHost: canvas.example\r\n{auth}{cookie}{origin}{content_type}\ + "{} {target} HTTP/1.1\r\nHost: canvas.example\r\n{auth}{cookie}{origin}{content_type}\ Content-Length: {}\r\n\r\n{}", self.method, - self.path, self.body.len(), self.body ) @@ -747,3 +754,7 @@ fn a_disallowed_origin_gets_no_cors_header_at_all() { #[cfg(test)] #[path = "online_mcp_tests.rs"] mod mcp_profile; + +#[cfg(test)] +#[path = "online_share_tests.rs"] +mod share; diff --git a/crates/op-host-services/src/web_canvas_server/online_share_tests.rs b/crates/op-host-services/src/web_canvas_server/online_share_tests.rs new file mode 100644 index 000000000..783b6a9ec --- /dev/null +++ b/crates/op-host-services/src/web_canvas_server/online_share_tests.rs @@ -0,0 +1,409 @@ +//! Sharing, the tenant query parameter, and eviction persistence, exercised +//! end to end through the online accept loop. +//! +//! Split out of `online_run_loop_tests.rs` at the 800-line cap; nested under +//! it so `use super::*` still reaches the request builder and helpers. + +use super::*; +use crate::web_canvas_server::tenant_store::TenantStore; + +/// A registry with a real on-disk store rooted in a temp directory. +struct PersistentRegistry { + root: std::path::PathBuf, + registry: TenantRegistry, +} + +impl PersistentRegistry { + fn new(label: &str) -> Self { + let root = std::env::temp_dir().join(format!( + "op-online-share-{label}-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).expect("temp root"); + Self { + registry: TenantRegistry::with_store( + 3102, + TenantLimits { + idle_evict_secs: 1, + ..TenantLimits::default() + }, + vec![PUBLIC_ORIGIN.to_string()], + TenantStore::new(Some(root.clone())), + ), + root, + } + } +} + +impl Drop for PersistentRegistry { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.root); + } +} + +/// Address a request at another account's tenant. +fn as_tenant(mut request: Request, owner: &'static str) -> Request { + request.tenant = Some(owner); + request +} + +fn share( + registry: &TenantRegistry, + token: &'static str, + route: &'static str, + target: &str, +) -> String { + serve( + registry, + &verifier(), + Request::json( + "POST", + route, + &serde_json::json!({ "userId": target }).to_string(), + ) + .with_bearer(token), + ) +} + +#[test] +fn a_visitor_reads_and_writes_the_owner_document_only_after_a_grant() { + let registry = registry(); + let verifier = verifier(); + + // Before the grant, addressing userA's tenant is refused. + let refused = serve( + ®istry, + &verifier, + as_tenant( + Request::new("GET", "/api/mcp/document").with_bearer("tokB"), + "userA", + ), + ); + assert_eq!(status_line(&refused), "HTTP/1.1 403 Forbidden", "{refused}"); + assert_eq!(body(&refused)["error"], "tenant-not-shared"); + + // userA shares. + let granted = share( + ®istry, + "tokA", + op_editor_core::share_routes::GRANT, + "userB", + ); + assert_eq!(status_line(&granted), "HTTP/1.1 200 OK", "{granted}"); + + // Now userB writes into userA's document… + let pushed = serve( + ®istry, + &verifier, + as_tenant( + Request::json("POST", "/api/mcp/document", SYNC_BODY).with_bearer("tokB"), + "userA", + ), + ); + assert_eq!(status_line(&pushed), "HTTP/1.1 200 OK", "{pushed}"); + + // …and userA sees it in their own tenant, with no parameter at all. + let owner_view = serve( + ®istry, + &verifier, + Request::new("GET", "/api/mcp/document").with_bearer("tokA"), + ); + assert!(owner_view.contains("Tenant Rect"), "{owner_view}"); + assert_eq!(body(&owner_view)["version"], 1); + + // userB's OWN document is untouched — the parameter addressed a tenant, + // it did not move the visitor into it. + let visitor_own = serve( + ®istry, + &verifier, + Request::new("GET", "/api/mcp/document").with_bearer("tokB"), + ); + assert_eq!(body(&visitor_own)["version"], 0, "{visitor_own}"); +} + +#[test] +fn a_revoke_locks_the_visitor_out_again() { + let registry = registry(); + share( + ®istry, + "tokA", + op_editor_core::share_routes::GRANT, + "userB", + ); + let allowed = serve( + ®istry, + &verifier(), + as_tenant( + Request::new("GET", "/api/mcp/version").with_bearer("tokB"), + "userA", + ), + ); + assert_eq!(status_line(&allowed), "HTTP/1.1 200 OK"); + + share( + ®istry, + "tokA", + op_editor_core::share_routes::REVOKE, + "userB", + ); + let refused = serve( + ®istry, + &verifier(), + as_tenant( + Request::new("GET", "/api/mcp/version").with_bearer("tokB"), + "userA", + ), + ); + assert_eq!(status_line(&refused), "HTTP/1.1 403 Forbidden", "{refused}"); +} + +#[test] +fn a_tenant_parameter_naming_an_unshared_account_is_refused() { + let registry = registry(); + for owner in ["userA", "nobody-at-all"] { + let response = serve( + ®istry, + &verifier(), + Request { + tenant: Some(Box::leak(owner.to_string().into_boxed_str())), + ..Request::new("GET", "/api/mcp/document") + } + .with_bearer("tokB"), + ); + assert_eq!(status_line(&response), "HTTP/1.1 403 Forbidden", "{owner}"); + } +} + +#[test] +fn an_account_may_always_address_its_own_tenant_explicitly() { + let registry = registry(); + let response = serve( + ®istry, + &verifier(), + as_tenant( + Request::new("GET", "/api/mcp/version").with_bearer("tokA"), + "userA", + ), + ); + assert_eq!(status_line(&response), "HTTP/1.1 200 OK", "{response}"); +} + +#[test] +fn the_share_list_reports_both_directions_over_the_wire() { + let registry = registry(); + share( + ®istry, + "tokA", + op_editor_core::share_routes::GRANT, + "userB", + ); + + let owner = serve( + ®istry, + &verifier(), + Request::new("GET", op_editor_core::share_routes::LIST).with_bearer("tokA"), + ); + assert_eq!(body(&owner)["sharedWith"][0], "userB", "{owner}"); + + let visitor = serve( + ®istry, + &verifier(), + Request::new("GET", op_editor_core::share_routes::LIST).with_bearer("tokB"), + ); + assert_eq!(body(&visitor)["sharedWithMe"][0], "userA", "{visitor}"); +} + +#[test] +fn a_share_route_always_administers_the_callers_own_tenant() { + // Even with a `?tenant=` parameter pointing at the owner, a visitor's + // grant lands on the visitor's own access list. + let registry = registry(); + share( + ®istry, + "tokA", + op_editor_core::share_routes::GRANT, + "userB", + ); + let response = serve( + ®istry, + &verifier(), + as_tenant( + Request::json( + "POST", + op_editor_core::share_routes::GRANT, + r#"{"userId":"userC"}"#, + ) + .with_bearer("tokB"), + "userA", + ), + ); + assert_eq!(status_line(&response), "HTTP/1.1 200 OK", "{response}"); + + // userC still cannot reach userA. + let stranger = serve( + ®istry, + &verifier(), + as_tenant( + Request::new("GET", "/api/mcp/version").with_bearer("tokC"), + "userA", + ), + ); + assert_eq!( + status_line(&stranger), + "HTTP/1.1 401 Unauthorized", + "{stranger}" + ); +} + +#[test] +fn an_evicted_tenant_is_written_and_read_back() { + let temp = PersistentRegistry::new("roundtrip"); + let verifier = verifier(); + + serve( + &temp.registry, + &verifier, + Request::json("POST", "/api/mcp/document", SYNC_BODY).with_bearer("tokA"), + ); + share( + &temp.registry, + "tokA", + op_editor_core::share_routes::GRANT, + "userB", + ); + + assert_eq!(temp.registry.evict_idle(now_unix() + 3600), 1); + assert_eq!(temp.registry.tenant_count(), 0); + assert!(temp.registry.store().has_document("userA")); + + // The document comes back… + let restored = serve( + &temp.registry, + &verifier, + Request::new("GET", "/api/mcp/document").with_bearer("tokA"), + ); + assert!(restored.contains("Tenant Rect"), "{restored}"); + + // …and so does the access list, so a share survives a reclaim. + let visitor = serve( + &temp.registry, + &verifier, + as_tenant( + Request::new("GET", "/api/mcp/version").with_bearer("tokB"), + "userA", + ), + ); + assert_eq!(status_line(&visitor), "HTTP/1.1 200 OK", "{visitor}"); +} + +#[test] +fn a_grant_is_persisted_immediately_rather_than_at_eviction() { + // A share the user was told had succeeded must survive a restart, and the + // document it applies to may not be written for another half hour. + let temp = PersistentRegistry::new("acl-now"); + share( + &temp.registry, + "tokA", + op_editor_core::share_routes::GRANT, + "userB", + ); + assert!( + temp.registry.store().load_acl("userA").contains("userB"), + "the grant must be on disk before any eviction" + ); +} + +#[test] +fn a_corrupt_stored_document_yields_a_starter_and_keeps_the_bytes() { + let temp = PersistentRegistry::new("corrupt"); + let verifier = verifier(); + serve( + &temp.registry, + &verifier, + Request::json("POST", "/api/mcp/document", SYNC_BODY).with_bearer("tokA"), + ); + assert_eq!(temp.registry.evict_idle(now_unix() + 3600), 1); + + let dir = temp.registry.store().tenant_dir("userA").expect("dir"); + std::fs::write(dir.join("current.op"), b"not a document").expect("corrupt"); + + let served = serve( + &temp.registry, + &verifier, + Request::new("GET", "/api/mcp/document").with_bearer("tokA"), + ); + assert_eq!(status_line(&served), "HTTP/1.1 200 OK", "{served}"); + assert!( + !served.contains("Tenant Rect"), + "an unreadable document must yield a starter, not a failure: {served}" + ); + let quarantined: Vec = std::fs::read_dir(&dir) + .expect("read dir") + .filter_map(|entry| entry.ok()) + .map(|entry| entry.file_name().to_string_lossy().into_owned()) + .filter(|name| name.contains("corrupt")) + .collect(); + assert_eq!( + quarantined.len(), + 1, + "the bytes must be kept, not overwritten" + ); +} + +#[test] +fn an_account_id_full_of_traversal_cannot_escape_the_data_directory() { + let temp = PersistentRegistry::new("traversal"); + let hostile = "../../../../etc/op-escape"; + let verifier = StaticVerifier::parse(&format!("tokX={hostile}")); + + serve( + &temp.registry, + &verifier, + Request::json("POST", "/api/mcp/document", SYNC_BODY).with_bearer("tokX"), + ); + assert_eq!(temp.registry.evict_idle(now_unix() + 3600), 1); + + let dir = temp.registry.store().tenant_dir(hostile).expect("dir"); + assert!( + dir.starts_with(&temp.root), + "{dir:?} escaped {:?}", + temp.root + ); + assert!( + dir.join("current.op").is_file(), + "the document still round-trips" + ); + // Nothing was created outside the store. + assert!(!std::path::Path::new("/etc/op-escape").exists()); +} + +#[test] +fn a_tenant_that_cannot_be_written_stays_resident_rather_than_losing_its_document() { + let temp = PersistentRegistry::new("unwritable"); + let verifier = verifier(); + serve( + &temp.registry, + &verifier, + Request::json("POST", "/api/mcp/document", SYNC_BODY).with_bearer("tokA"), + ); + // Make the store root a file so `create_dir_all` cannot succeed. + std::fs::remove_dir_all(&temp.root).expect("clear root"); + std::fs::write(&temp.root, b"not a directory").expect("block the root"); + + assert_eq!( + temp.registry.evict_idle(now_unix() + 3600), + 0, + "reclaiming memory must not be worth discarding a document" + ); + assert_eq!(temp.registry.tenant_count(), 1); + let still_there = serve( + &temp.registry, + &verifier, + Request::new("GET", "/api/mcp/document").with_bearer("tokA"), + ); + assert!(still_there.contains("Tenant Rect"), "{still_there}"); + + let _ = std::fs::remove_file(&temp.root); +} diff --git a/crates/op-host-services/src/web_canvas_server/share_routes.rs b/crates/op-host-services/src/web_canvas_server/share_routes.rs new file mode 100644 index 000000000..a1cddf608 --- /dev/null +++ b/crates/op-host-services/src/web_canvas_server/share_routes.rs @@ -0,0 +1,188 @@ +//! `/api/share/*` handlers — who else may open this account's document. +//! +//! Online only. The local and managed daemons have one document and one +//! operator, so there is nothing to share and these routes are not mounted. +//! +//! ## The one invariant +//! +//! The grantor is always the request's VERIFIED identity. The body names the +//! account being granted, never the account doing the granting — otherwise +//! any caller could add themselves to any document's access list, which is +//! the whole security property inverted. +//! +//! Grants run on the connection thread rather than under the document lock: +//! the access list is its own mutex on the tenant, so sharing is answerable +//! while a large document push is in flight. + +use op_editor_core::share_routes; + +use super::tenant::{TenantLease, TenantRegistry}; +use super::tenant_auth::ResolvedIdentity; +use super::WebReply; + +/// Longest body either POST accepts. Both are a single short account id. +const MAX_SHARE_BODY_BYTES: usize = 4 * 1024; + +/// Longest account id accepted in a body. +const MAX_ACCOUNT_ID_CHARS: usize = 256; + +/// Why a share request was refused. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ShareError { + BodyTooLarge, + MalformedRequest, + /// The body named the caller's own account. + SelfShare, +} + +impl ShareError { + pub const fn code(self) -> &'static str { + match self { + Self::BodyTooLarge => "payload-too-large", + Self::MalformedRequest => "malformed-share-request", + Self::SelfShare => "cannot-share-with-self", + } + } + + pub const fn http_status(self) -> &'static str { + match self { + Self::BodyTooLarge => "413 Payload Too Large", + Self::MalformedRequest | Self::SelfShare => "400 Bad Request", + } + } +} + +impl std::fmt::Display for ShareError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(match self { + Self::BodyTooLarge => "body too large", + Self::MalformedRequest => "malformed share request", + Self::SelfShare => "an account already has access to its own document", + }) + } +} + +impl std::error::Error for ShareError {} + +/// Whether `path` is one of the share routes. +pub(super) fn is_share_route(path: &str) -> bool { + matches!( + path, + share_routes::GRANT | share_routes::REVOKE | share_routes::LIST + ) +} + +/// Dispatch one `/api/share/*` request. +/// +/// `lease` is the CALLER's own tenant: grant and revoke edit the caller's +/// access list, never the tenant a `?tenant=` parameter pointed at. A visitor +/// cannot re-share a document they were merely given access to. +pub(super) fn handle( + method: &str, + path: &str, + body: &str, + identity: &ResolvedIdentity, + lease: &TenantLease, + registry: &TenantRegistry, +) -> WebReply { + match (method, path) { + ("POST", share_routes::GRANT) => mutate(body, identity, lease, registry, true), + ("POST", share_routes::REVOKE) => mutate(body, identity, lease, registry, false), + ("GET", share_routes::LIST) => list(identity, lease, registry), + _ => WebReply { + status: "405 Method Not Allowed", + body: crate::mcp_serve::rest_error_body("method not allowed for this share route"), + }, + } +} + +fn mutate( + body: &str, + identity: &ResolvedIdentity, + lease: &TenantLease, + registry: &TenantRegistry, + granting: bool, +) -> WebReply { + let account = match parse_account(body, &identity.user_id) { + Ok(account) => account, + Err(error) => return error_reply(error), + }; + let changed = if granting { + lease.tenant().grant(&account) + } else { + lease.tenant().revoke(&account) + }; + if changed { + // Persisted immediately rather than at eviction: a share the user was + // told had succeeded must survive a restart, and the document it + // applies to may not be written for another half hour. + registry.persist_acl(lease.owner_id(), lease.tenant()); + } + WebReply { + status: "200 OK", + body: serde_json::json!({ + "ok": true, + "changed": changed, + "sharedWith": lease.tenant().shared_with().into_iter().collect::>(), + }) + .to_string(), + } +} + +fn list(identity: &ResolvedIdentity, lease: &TenantLease, registry: &TenantRegistry) -> WebReply { + WebReply { + status: "200 OK", + body: serde_json::json!({ + "ok": true, + // Who may open this account's document… + "sharedWith": lease.tenant().shared_with().into_iter().collect::>(), + // …and whose documents this account may open. Resident owners + // only; see `TenantRegistry::shared_with_visitor`. + "sharedWithMe": registry.shared_with_visitor(&identity.user_id), + }) + .to_string(), + } +} + +/// Pull the target account out of a share body. +/// +/// The account id is not validated against the hub: this deployment has no +/// user-lookup endpoint yet, so an id that belongs to nobody simply grants +/// access to nobody. M5 should resolve it through the hub so a typo is +/// reported at grant time instead of silently doing nothing. +fn parse_account(body: &str, caller: &str) -> Result { + if body.len() > MAX_SHARE_BODY_BYTES { + return Err(ShareError::BodyTooLarge); + } + let parsed: serde_json::Value = + serde_json::from_str(body).map_err(|_| ShareError::MalformedRequest)?; + let account = parsed + .get("userId") + .and_then(|value| value.as_str()) + .map(str::trim) + .filter(|value| !value.is_empty()) + .ok_or(ShareError::MalformedRequest)?; + if account.chars().count() > MAX_ACCOUNT_ID_CHARS { + return Err(ShareError::MalformedRequest); + } + if account == caller { + return Err(ShareError::SelfShare); + } + Ok(account.to_string()) +} + +fn error_reply(error: ShareError) -> WebReply { + WebReply { + status: error.http_status(), + body: serde_json::json!({ + "ok": false, + "error": error.code(), + "message": error.to_string(), + }) + .to_string(), + } +} + +#[cfg(test)] +#[path = "share_routes_tests.rs"] +mod tests; diff --git a/crates/op-host-services/src/web_canvas_server/share_routes_tests.rs b/crates/op-host-services/src/web_canvas_server/share_routes_tests.rs new file mode 100644 index 000000000..840ffcf77 --- /dev/null +++ b/crates/op-host-services/src/web_canvas_server/share_routes_tests.rs @@ -0,0 +1,247 @@ +//! Tests for the share administration routes and their admission rules. + +use super::*; +use crate::mcp_serve::tool_profile::McpScopes; +use crate::web_canvas_server::tenant::{TenantError, TenantLimits}; +use crate::web_canvas_server::tenant_auth::IdentityVia; + +fn identity(user_id: &str) -> ResolvedIdentity { + ResolvedIdentity { + user_id: user_id.into(), + username: user_id.into(), + display_name: user_id.into(), + via: IdentityVia::ApiToken, + scopes: McpScopes::FULL, + } +} + +fn registry() -> TenantRegistry { + TenantRegistry::with_store( + 3102, + TenantLimits::default(), + Vec::new(), + super::super::tenant_store::TenantStore::new(None), + ) +} + +fn body_of(reply: &WebReply) -> serde_json::Value { + serde_json::from_str(&reply.body).expect("json body") +} + +fn grant(registry: &TenantRegistry, owner: &str, target: &str) -> WebReply { + let identity = identity(owner); + let lease = registry.lease_for(&identity).expect("lease"); + handle( + "POST", + share_routes::GRANT, + &serde_json::json!({ "userId": target }).to_string(), + &identity, + &lease, + registry, + ) +} + +#[test] +fn a_grant_admits_the_named_account_and_nobody_else() { + let registry = registry(); + let reply = grant(®istry, "userA", "userB"); + assert_eq!(reply.status, "200 OK"); + assert_eq!(body_of(&reply)["changed"], true); + + let visitor = identity("userB"); + assert!(registry.lease_for_shared("userA", &visitor).is_ok()); + + let stranger = identity("userC"); + assert_eq!( + registry.lease_for_shared("userA", &stranger).unwrap_err(), + TenantError::NotShared + ); +} + +#[test] +fn a_revoke_takes_effect_on_the_next_request() { + let registry = registry(); + grant(®istry, "userA", "userB"); + let visitor = identity("userB"); + assert!(registry.lease_for_shared("userA", &visitor).is_ok()); + + let owner = identity("userA"); + let lease = registry.lease_for(&owner).expect("lease"); + let reply = handle( + "POST", + share_routes::REVOKE, + r#"{"userId":"userB"}"#, + &owner, + &lease, + ®istry, + ); + assert_eq!(reply.status, "200 OK"); + assert_eq!(body_of(&reply)["changed"], true); + + // The access list is consulted per request, so this is immediate — no + // session to expire first. + assert_eq!( + registry.lease_for_shared("userA", &visitor).unwrap_err(), + TenantError::NotShared + ); +} + +#[test] +fn an_owner_always_reaches_their_own_document() { + let registry = registry(); + let owner = identity("userA"); + assert!(registry.lease_for_shared("userA", &owner).is_ok()); +} + +#[test] +fn a_repeated_grant_is_acknowledged_without_changing_anything() { + let registry = registry(); + assert_eq!( + body_of(&grant(®istry, "userA", "userB"))["changed"], + true + ); + assert_eq!( + body_of(&grant(®istry, "userA", "userB"))["changed"], + false + ); +} + +#[test] +fn revoking_an_account_that_was_never_granted_is_not_an_error() { + let registry = registry(); + let owner = identity("userA"); + let lease = registry.lease_for(&owner).expect("lease"); + let reply = handle( + "POST", + share_routes::REVOKE, + r#"{"userId":"nobody"}"#, + &owner, + &lease, + ®istry, + ); + assert_eq!(reply.status, "200 OK"); + assert_eq!(body_of(&reply)["changed"], false); +} + +#[test] +fn the_list_reports_both_directions() { + let registry = registry(); + grant(®istry, "userA", "userB"); + grant(®istry, "userC", "userB"); + + let visitor = identity("userB"); + let lease = registry.lease_for(&visitor).expect("lease"); + let reply = handle("GET", share_routes::LIST, "", &visitor, &lease, ®istry); + let body = body_of(&reply); + assert_eq!(reply.status, "200 OK"); + // userB has shared with nobody… + assert_eq!(body["sharedWith"].as_array().map(Vec::len), Some(0)); + // …and two accounts have shared with userB. + let mine: Vec<&str> = body["sharedWithMe"] + .as_array() + .expect("array") + .iter() + .filter_map(|value| value.as_str()) + .collect(); + assert_eq!(mine, vec!["userA", "userC"]); +} + +#[test] +fn a_visitor_cannot_reshare_the_document_they_were_given() { + // The route always edits the CALLER's own tenant, so a grant issued by a + // visitor lands on the visitor's own document, never the owner's. + let registry = registry(); + grant(®istry, "userA", "userB"); + grant(®istry, "userB", "userC"); + + let stranger = identity("userC"); + assert_eq!( + registry.lease_for_shared("userA", &stranger).unwrap_err(), + TenantError::NotShared, + "userB must not be able to widen userA's access list" + ); + assert!( + registry.lease_for_shared("userB", &stranger).is_ok(), + "userB may of course share their own document" + ); +} + +#[test] +fn an_account_cannot_share_with_itself() { + let registry = registry(); + let reply = grant(®istry, "userA", "userA"); + assert_eq!(reply.status, "400 Bad Request"); + assert_eq!(body_of(&reply)["error"], "cannot-share-with-self"); +} + +#[test] +fn a_malformed_share_body_is_refused() { + let registry = registry(); + let owner = identity("userA"); + let lease = registry.lease_for(&owner).expect("lease"); + for body in [ + "", + "{}", + "not json", + r#"{"userId":""}"#, + r#"{"userId":123}"#, + r#"{"user":"x"}"#, + ] { + let reply = handle("POST", share_routes::GRANT, body, &owner, &lease, ®istry); + assert_eq!(reply.status, "400 Bad Request", "{body:?}"); + } +} + +#[test] +fn an_oversized_share_body_is_refused_before_it_is_parsed() { + let registry = registry(); + let owner = identity("userA"); + let lease = registry.lease_for(&owner).expect("lease"); + let body = format!(r#"{{"userId":"{}"}}"#, "x".repeat(MAX_SHARE_BODY_BYTES)); + let reply = handle( + "POST", + share_routes::GRANT, + &body, + &owner, + &lease, + ®istry, + ); + assert_eq!(reply.status, "413 Payload Too Large"); +} + +#[test] +fn a_wrong_method_on_a_share_route_is_405() { + let registry = registry(); + let owner = identity("userA"); + let lease = registry.lease_for(&owner).expect("lease"); + let reply = handle("GET", share_routes::GRANT, "", &owner, &lease, ®istry); + assert_eq!(reply.status, "405 Method Not Allowed"); +} + +#[test] +fn the_share_routes_are_recognised_and_nothing_else_is() { + for route in [ + share_routes::GRANT, + share_routes::REVOKE, + share_routes::LIST, + ] { + assert!(is_share_route(route), "{route}"); + } + for other in ["/api/mcp/document", "/api/share", "/api/share/", "/mcp"] { + assert!(!is_share_route(other), "{other}"); + } +} + +#[test] +fn a_forbidden_share_and_an_unknown_one_answer_identically() { + // Otherwise the difference tells a caller which accounts exist. + assert_eq!(TenantError::NotShared.http_status(), "403 Forbidden"); + let registry = registry(); + let stranger = identity("userC"); + let unknown = registry + .lease_for_shared("nobody-at-all", &stranger) + .unwrap_err(); + grant(®istry, "userA", "userB"); + let forbidden = registry.lease_for_shared("userA", &stranger).unwrap_err(); + assert_eq!(unknown, forbidden); +} diff --git a/crates/op-host-services/src/web_canvas_server/tenant.rs b/crates/op-host-services/src/web_canvas_server/tenant.rs index 6471eeee7..1c89b01c3 100644 --- a/crates/op-host-services/src/web_canvas_server/tenant.rs +++ b/crates/op-host-services/src/web_canvas_server/tenant.rs @@ -27,13 +27,14 @@ //! victim, and removes it only after re-checking that the entry in the map is //! still the exact `Arc` it decided to evict. -use std::collections::HashMap; +use std::collections::{BTreeSet, HashMap}; use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; use op_editor_core::EditorState; use super::tenant_auth::ResolvedIdentity; +use super::tenant_store::TenantStore; use super::{SseHub, WebCanvasState}; /// Global connection ceiling for the online daemon. @@ -119,6 +120,9 @@ pub enum TenantError { /// This account already holds [`TenantLimits::max_conns_per_tenant`] /// connections. TooManyConnections, + /// The caller asked for another account's tenant and is not on its + /// access list. + NotShared, } impl TenantError { @@ -126,11 +130,17 @@ impl TenantError { match self { Self::TooManyTenants => "server-busy", Self::TooManyConnections => "too-many-connections", + Self::NotShared => "tenant-not-shared", } } pub const fn http_status(self) -> &'static str { - "503 Service Unavailable" + match self { + Self::TooManyTenants | Self::TooManyConnections => "503 Service Unavailable", + // A forbidden share and a non-existent one answer the same way: + // otherwise the difference is an oracle for which accounts exist. + Self::NotShared => "403 Forbidden", + } } } @@ -141,6 +151,7 @@ impl std::fmt::Display for TenantError { Self::TooManyConnections => { f.write_str("too many concurrent connections for this account") } + Self::NotShared => f.write_str("this document is not shared with you"), } } } @@ -155,6 +166,13 @@ pub struct Tenant { /// This account's SSE subscribers. Separate per tenant so a version bump /// is only ever broadcast to the account that caused it. pub(crate) hub: SseHub, + /// Accounts this tenant's owner has shared the document with. + /// + /// A `BTreeSet` rather than a `HashSet` so the persisted list has a + /// stable order and two saves of the same ACL produce the same bytes. + /// Separate from the state mutex because admission is checked on every + /// request while the document lock may be held by a long push. + shared_with: Mutex>, /// Live leases. Non-zero means "do not evict". leases: AtomicUsize, /// Unix seconds of the last lease acquire or release. @@ -162,19 +180,58 @@ pub struct Tenant { } impl Tenant { - fn new(port: u16, allow_origins: &[String], now_unix: u64) -> Self { - let mut state = WebCanvasState::new_for_tenant(EditorState::starter(), port); + fn new( + port: u16, + allow_origins: &[String], + editor: EditorState, + shared_with: BTreeSet, + now_unix: u64, + ) -> Self { + let mut state = WebCanvasState::new_for_tenant(editor, port); // Every tenant answers for the same public origin; the allowlist is a // deployment property, not an account one. state.allow_origins = allow_origins.to_vec(); Self { state: Mutex::new(state), hub: SseHub::default(), + shared_with: Mutex::new(shared_with), leases: AtomicUsize::new(0), last_active_unix: AtomicU64::new(now_unix), } } + /// Whether `visitor` may reach this tenant's document. + pub fn admits(&self, visitor: &str) -> bool { + self.shared_with + .lock() + .unwrap_or_else(|p| p.into_inner()) + .contains(visitor) + } + + /// Add an account to the access list. Returns whether it was new. + pub fn grant(&self, visitor: &str) -> bool { + self.shared_with + .lock() + .unwrap_or_else(|p| p.into_inner()) + .insert(visitor.to_string()) + } + + /// Remove an account. Returns whether it had been granted. + pub fn revoke(&self, visitor: &str) -> bool { + self.shared_with + .lock() + .unwrap_or_else(|p| p.into_inner()) + .remove(visitor) + } + + /// A snapshot of the access list. + pub fn shared_with(&self) -> BTreeSet { + self.shared_with + .lock() + .unwrap_or_else(|p| p.into_inner()) + .clone() + } + /// Live lease count. Zero is the only evictable value. pub fn lease_count(&self) -> usize { self.leases.load(Ordering::Acquire) @@ -196,6 +253,9 @@ impl Tenant { /// of it stay valid. pub struct TenantLease { tenant: Arc, + /// The account this lease resolved to — the OWNER of the document, which + /// for a shared visit is not the visitor. + user_id: String, } impl std::fmt::Debug for TenantLease { @@ -210,6 +270,12 @@ impl std::fmt::Debug for TenantLease { } impl TenantLease { + /// The owning account id. For a shared visit this is the owner, not the + /// visitor — it is the tenant's key, not the caller's identity. + pub fn owner_id(&self) -> &str { + &self.user_id + } + pub fn tenant(&self) -> &Tenant { &self.tenant } @@ -241,18 +307,34 @@ pub struct TenantRegistry { port: u16, /// Public origins this deployment answers for, stamped onto every tenant. allow_origins: Vec, + /// Where an evicted tenant is written and a returning one is read from. + store: TenantStore, } impl TenantRegistry { pub fn new(port: u16, limits: TenantLimits, allow_origins: Vec) -> Self { + Self::with_store(port, limits, allow_origins, TenantStore::from_env()) + } + + pub fn with_store( + port: u16, + limits: TenantLimits, + allow_origins: Vec, + store: TenantStore, + ) -> Self { Self { tenants: Mutex::new(HashMap::new()), limits, port, allow_origins, + store, } } + pub const fn store(&self) -> &TenantStore { + &self.store + } + /// The deployment's public origin allowlist. pub fn allow_origins(&self) -> &[String] { &self.allow_origins @@ -273,21 +355,72 @@ impl TenantRegistry { /// on [`super::tenant_auth`] for why no request-supplied value may ever /// reach this argument. /// - /// A brand new tenant starts from [`EditorState::starter`]. M1 does not - /// persist, so an evicted tenant's document is gone and a returning - /// account gets a fresh starter document; that is the documented M1 - /// semantic, and M4 replaces it with load-on-create. + /// A tenant that is not in memory is restored from disk when the store + /// holds one, and starts from [`EditorState::starter`] otherwise — so an + /// eviction is invisible to the account beyond the first request's cost. pub fn lease_for(&self, identity: &ResolvedIdentity) -> Result { + self.lease_tenant(&identity.user_id) + } + + /// Take a lease on `owner_id`'s tenant on behalf of `visitor`. + /// + /// The owner always passes. Anyone else must appear in the owner's access + /// list — and note that the list is consulted on EVERY request, so a + /// revoke takes effect on the visitor's next call rather than whenever + /// some session expires. + /// + /// A tenant that is not resident is restored first: a visitor must be + /// able to open a shared document whose owner is offline, and refusing + /// until the owner next signs in would make sharing useless. + pub fn lease_for_shared( + &self, + owner_id: &str, + visitor: &ResolvedIdentity, + ) -> Result { + if owner_id == visitor.user_id { + return self.lease_tenant(owner_id); + } + let lease = self.lease_tenant(owner_id)?; + if !lease.tenant().admits(&visitor.user_id) { + return Err(TenantError::NotShared); + } + Ok(lease) + } + + /// Who has shared with `visitor`, across every resident tenant. + /// + /// Resident only, and deliberately: a full answer would mean reading every + /// directory in the store on every call. The owners a visitor is actually + /// working with are resident by definition, and the visitor can always + /// open a share they were told about directly. + pub fn shared_with_visitor(&self, visitor: &str) -> Vec { + let tenants = self.lock(); + let mut owners: Vec = tenants + .iter() + .filter(|(owner, tenant)| owner.as_str() != visitor && tenant.admits(visitor)) + .map(|(owner, _)| owner.clone()) + .collect(); + owners.sort_unstable(); + owners + } + + fn lease_tenant(&self, user_id: &str) -> Result { let now = now_unix(); let mut tenants = self.lock(); - let tenant = match tenants.get(&identity.user_id) { + let tenant = match tenants.get(user_id) { Some(existing) => Arc::clone(existing), None => { if tenants.len() >= self.limits.max_tenants { return Err(TenantError::TooManyTenants); } - let created = Arc::new(Tenant::new(self.port, &self.allow_origins, now)); - tenants.insert(identity.user_id.clone(), Arc::clone(&created)); + let created = Arc::new(Tenant::new( + self.port, + &self.allow_origins, + self.restore_editor(user_id), + self.store.load_acl(user_id), + now, + )); + tenants.insert(user_id.to_string(), Arc::clone(&created)); created } }; @@ -299,7 +432,41 @@ impl TenantRegistry { // resolved above and being leased here. tenant.leases.fetch_add(1, Ordering::AcqRel); tenant.touch(now); - Ok(TenantLease { tenant }) + Ok(TenantLease { + tenant, + user_id: user_id.to_string(), + }) + } + + /// The document to open a tenant with. + /// + /// A stored document that will not load has already been moved aside by + /// the store, so the account gets a starter rather than a failed request — + /// losing a document is bad, but refusing to serve the account at all + /// because of it is worse. + fn restore_editor(&self, user_id: &str) -> EditorState { + match self.store.load_document(user_id) { + Ok(state) => state, + Err(super::tenant_store::TenantStoreError::Disabled) + | Err(super::tenant_store::TenantStoreError::NotStored) => EditorState::starter(), + Err(error) => { + eprintln!( + "openpencil --serve-web --online: starting a fresh document for an \ + account whose stored one could not be loaded ({error})" + ); + EditorState::starter() + } + } + } + + /// Persist a tenant's access list, if persistence is on. + pub fn persist_acl(&self, user_id: &str, tenant: &Tenant) { + if !self.store.is_enabled() { + return; + } + if let Err(error) = self.store.save_acl_for(user_id, &tenant.shared_with()) { + eprintln!("openpencil --serve-web --online: could not persist a share list ({error})"); + } } /// Reclaim tenants that hold no lease and have been idle past the limit. @@ -321,10 +488,23 @@ impl TenantRegistry { .collect(); let mut evicted = 0; for (id, victim) in victims { - // M4 persistence hook: write `victim.state` to - // `$OPENPENCIL_ONLINE_DATA_DIR//current.op` HERE, before the - // compare-and-remove below, so a returning account either finds - // the old tenant still mapped or a file it can load. + // Write BEFORE the compare-and-remove, so at no instant is the + // tenant both absent from the map and absent from disk: a request + // arriving mid-eviction either finds the resident tenant (the + // registry lock is held, so it waits) or, afterwards, the file. + if self.store.is_enabled() { + let guard = victim.state.lock().unwrap_or_else(|p| p.into_inner()); + if let Err(error) = self.store.save(&id, &guard.editor, &victim.shared_with()) { + // A tenant that cannot be written is kept resident. Evicting + // it anyway would discard the document to reclaim memory, + // which is the wrong trade for the user whose work it is. + eprintln!( + "openpencil --serve-web --online: keeping a tenant resident because \ + it could not be persisted ({error})" + ); + continue; + } + } let still_the_same = tenants .get(&id) .is_some_and(|current| Arc::ptr_eq(current, &victim)); diff --git a/crates/op-host-services/src/web_canvas_server/tenant_auth_tests.rs b/crates/op-host-services/src/web_canvas_server/tenant_auth_tests.rs index c90b22efc..636244904 100644 --- a/crates/op-host-services/src/web_canvas_server/tenant_auth_tests.rs +++ b/crates/op-host-services/src/web_canvas_server/tenant_auth_tests.rs @@ -13,6 +13,7 @@ fn request_with(authorization: Option<&str>, cookie: Option<&str>) -> HttpReques content_type: None, authorization: authorization.map(str::to_string), cookie: cookie.map(str::to_string), + query: None, } } diff --git a/crates/op-host-services/src/web_canvas_server/tenant_store.rs b/crates/op-host-services/src/web_canvas_server/tenant_store.rs new file mode 100644 index 000000000..6dea156e9 --- /dev/null +++ b/crates/op-host-services/src/web_canvas_server/tenant_store.rs @@ -0,0 +1,262 @@ +//! Where an evicted tenant's document goes, and how it comes back. +//! +//! M1 kept tenants purely in memory, so eviction lost the document and a +//! returning account got a starter. This is the other half: eviction writes, +//! and a cache miss reads. +//! +//! ## Directory naming +//! +//! The directory is `SHA-256(user_id)`, first 16 hex characters — never the +//! account id itself. An account id is a string the daemon did not choose, +//! and joining an unvetted string onto a path is how `../` gets to walk out +//! of the data directory. Hashing removes the question entirely: the output +//! alphabet is `[0-9a-f]`, so there is no traversal to defend against, no +//! case-collision on a case-insensitive filesystem, and no length limit to +//! worry about. +//! +//! ## Writing +//! +//! Temp file in the same directory, then `rename`. A crash therefore leaves +//! either the previous document or the new one, never a half-written file +//! that would fail to load on the account's next visit. +//! +//! ## Reading a file that will not load +//! +//! Renamed to `.corrupt` and the account gets a starter document. Deleting it +//! would destroy the only copy of whatever the user had; silently overwriting +//! it does the same thing one save later. Keeping it costs a few bytes and is +//! the difference between "we can look at it" and "it is gone". + +use std::collections::BTreeSet; +use std::path::{Path, PathBuf}; + +use op_editor_core::EditorState; +use sha2::{Digest, Sha256}; + +/// Root of the on-disk tenant store. Unset disables persistence entirely, +/// which is what a demo or a test wants. +pub const DATA_DIR_ENV: &str = "OPENPENCIL_ONLINE_DATA_DIR"; + +/// The document file inside a tenant directory. +const DOCUMENT_FILE: &str = "current.op"; +/// The access list inside a tenant directory. +const ACL_FILE: &str = "acl.json"; +/// Suffix a file that would not load is moved aside under. +const CORRUPT_SUFFIX: &str = "corrupt"; + +/// Longest access list that is written or read back. +/// +/// A share list is a handful of accounts; a larger one is a bug or an attempt +/// to make the daemon allocate on every eviction. +const MAX_SHARED_ACCOUNTS: usize = 256; + +/// Why a tenant could not be persisted or restored. +/// +/// Every variant is non-fatal at the call site: a failed save leaves the +/// previous file, and a failed load yields a starter document. Persistence +/// must never be able to take the service down. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum TenantStoreError { + /// No data directory is configured, so there is nothing to read or write. + Disabled, + /// The tenant has nothing stored yet. + NotStored, + /// The stored file could not be read or written. + Io(String), + /// The stored document exists but could not be loaded. It has been moved + /// aside; the caller should start fresh. + Unreadable(String), +} + +impl std::fmt::Display for TenantStoreError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Disabled => f.write_str("tenant persistence is not configured"), + Self::NotStored => f.write_str("no stored document for this account"), + Self::Io(detail) => write!(f, "tenant store IO failed: {detail}"), + Self::Unreadable(detail) => { + write!(f, "stored document could not be loaded: {detail}") + } + } + } +} + +impl std::error::Error for TenantStoreError {} + +/// The on-disk tenant store. +#[derive(Debug, Clone)] +pub struct TenantStore { + root: Option, +} + +impl TenantStore { + /// Build from the environment. No data directory means no persistence. + pub fn from_env() -> Self { + Self::new( + std::env::var(DATA_DIR_ENV) + .ok() + .map(|value| value.trim().to_string()) + .filter(|value| !value.is_empty()) + .map(PathBuf::from), + ) + } + + pub const fn new(root: Option) -> Self { + Self { root } + } + + pub const fn is_enabled(&self) -> bool { + self.root.is_some() + } + + /// The directory holding `user_id`'s tenant, if persistence is on. + /// + /// See the module docs for why this is a hash and not the id. + pub fn tenant_dir(&self, user_id: &str) -> Option { + self.root.as_ref().map(|root| root.join(dir_name(user_id))) + } + + /// Write a tenant's document and access list. + /// + /// Called during eviction, while the registry lock is held and the tenant + /// has no leases — so nothing can be writing the document underneath it. + pub fn save( + &self, + user_id: &str, + state: &EditorState, + shared_with: &BTreeSet, + ) -> Result<(), TenantStoreError> { + let Some(dir) = self.tenant_dir(user_id) else { + return Err(TenantStoreError::Disabled); + }; + std::fs::create_dir_all(&dir).map_err(|error| TenantStoreError::Io(error.to_string()))?; + // The document goes through the same streaming, atomic writer desktop + // Save uses, so a large document with embedded images is not built in + // memory twice and a crash cannot truncate the live file. + crate::doc_io::save_to_path(state, &dir.join(DOCUMENT_FILE)) + .map_err(|error| TenantStoreError::Io(error.to_string()))?; + self.save_acl(&dir, shared_with) + } + + /// Write just the access list. Used when a grant or revoke should survive + /// a restart even though the document has not changed. + pub fn save_acl_for( + &self, + user_id: &str, + shared_with: &BTreeSet, + ) -> Result<(), TenantStoreError> { + let Some(dir) = self.tenant_dir(user_id) else { + return Err(TenantStoreError::Disabled); + }; + std::fs::create_dir_all(&dir).map_err(|error| TenantStoreError::Io(error.to_string()))?; + self.save_acl(&dir, shared_with) + } + + fn save_acl(&self, dir: &Path, shared_with: &BTreeSet) -> Result<(), TenantStoreError> { + let bounded: Vec<&String> = shared_with.iter().take(MAX_SHARED_ACCOUNTS).collect(); + let body = serde_json::json!({ "sharedWith": bounded }).to_string(); + atomic_write(&dir.join(ACL_FILE), body.as_bytes()) + } + + /// Restore a tenant's document, if one was stored. + /// + /// A file that will not load is moved aside (see the module docs) and + /// reported as [`TenantStoreError::Unreadable`], so the caller starts the + /// account fresh rather than failing its request. + pub fn load_document(&self, user_id: &str) -> Result { + let Some(dir) = self.tenant_dir(user_id) else { + return Err(TenantStoreError::Disabled); + }; + let path = dir.join(DOCUMENT_FILE); + if !path.is_file() { + return Err(TenantStoreError::NotStored); + } + match crate::doc_io::load_editor_state(&path, op_editor_core::Locale::EnUs) { + Ok(state) => Ok(state), + Err(error) => { + let detail = error.to_string(); + quarantine(&path); + Err(TenantStoreError::Unreadable(detail)) + } + } + } + + /// Restore a tenant's access list. A missing or unreadable list is an + /// empty one — failing closed, since the list only ever grants access. + pub fn load_acl(&self, user_id: &str) -> BTreeSet { + let Some(dir) = self.tenant_dir(user_id) else { + return BTreeSet::new(); + }; + let path = dir.join(ACL_FILE); + let Ok(body) = std::fs::read_to_string(&path) else { + return BTreeSet::new(); + }; + let Ok(parsed) = serde_json::from_str::(&body) else { + quarantine(&path); + return BTreeSet::new(); + }; + parsed + .get("sharedWith") + .and_then(|value| value.as_array()) + .map(|entries| { + entries + .iter() + .filter_map(|entry| entry.as_str()) + .filter(|entry| !entry.trim().is_empty()) + .take(MAX_SHARED_ACCOUNTS) + .map(str::to_string) + .collect() + }) + .unwrap_or_default() + } + + /// Whether anything is stored for `user_id`. + pub fn has_document(&self, user_id: &str) -> bool { + self.tenant_dir(user_id) + .is_some_and(|dir| dir.join(DOCUMENT_FILE).is_file()) + } +} + +/// The directory name for `user_id`: the first 16 hex characters of its +/// SHA-256. See the module docs for why this is not the id itself. +pub fn dir_name(user_id: &str) -> String { + let digest = Sha256::digest(user_id.as_bytes()); + digest + .iter() + .take(8) + .map(|byte| format!("{byte:02x}")) + .collect() +} + +/// Write `bytes` to `path` via a same-directory temp file plus rename. +fn atomic_write(path: &Path, bytes: &[u8]) -> Result<(), TenantStoreError> { + let temp = path.with_extension("tmp"); + std::fs::write(&temp, bytes).map_err(|error| TenantStoreError::Io(error.to_string()))?; + std::fs::rename(&temp, path).map_err(|error| { + // Leaving the temp behind would accumulate one file per failed write. + let _ = std::fs::remove_file(&temp); + TenantStoreError::Io(error.to_string()) + }) +} + +/// Move a file that would not parse out of the way, preserving it. +/// +/// Best effort: if the rename fails there is nothing useful left to do, and +/// the caller is already returning a starter document. A pre-existing +/// `.corrupt` from an earlier failure is kept — the timestamp suffix means a +/// second bad file does not overwrite the first. +fn quarantine(path: &Path) { + let stamp = crate::web_canvas_server::tenant::now_unix(); + let target = path.with_extension(format!("{CORRUPT_SUFFIX}.{stamp}")); + if std::fs::rename(path, &target).is_err() { + return; + } + eprintln!( + "openpencil --serve-web --online: kept an unreadable tenant file at {}", + target.display() + ); +} + +#[cfg(test)] +#[path = "tenant_store_tests.rs"] +mod tests; diff --git a/crates/op-host-services/src/web_canvas_server/tenant_store_tests.rs b/crates/op-host-services/src/web_canvas_server/tenant_store_tests.rs new file mode 100644 index 000000000..0f52f6b54 --- /dev/null +++ b/crates/op-host-services/src/web_canvas_server/tenant_store_tests.rs @@ -0,0 +1,257 @@ +//! Tests for the on-disk tenant store. + +use super::*; +use op_editor_core::EditorState; + +/// A store rooted in a fresh temp directory, removed when the guard drops. +struct TempStore { + root: PathBuf, + store: TenantStore, +} + +impl TempStore { + fn new(label: &str) -> Self { + let root = std::env::temp_dir().join(format!( + "op-tenant-store-{label}-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).expect("temp root"); + Self { + store: TenantStore::new(Some(root.clone())), + root, + } + } +} + +impl Drop for TempStore { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.root); + } +} + +/// A document carrying one recognisable node, built through the same +/// canonical loader the daemon uses. +/// +/// The round trip is asserted on DOCUMENT content, not on editor UI state — +/// the `.op` format deliberately does not carry the latter, so a marker like +/// `file_name_display` would silently "fail" every time. +fn named_document(name: &str) -> EditorState { + let json = serde_json::json!({ + "version": "1.0.0", + "children": [{ + "id": "n1", "type": "rectangle", "name": name, + "x": 1, "y": 2, "width": 80, "height": 40, + }], + }) + .to_string(); + let loaded = op_pen_loader::load_canonical(&json).expect("canonical document"); + let mut state = EditorState::starter(); + state.replace_document(loaded.value); + state +} + +/// The name of the document's first node, read off the serialized form so +/// this does not depend on `PenNode`'s variant shape. +fn node_name(state: &EditorState) -> Option { + let json = serde_json::to_value(state.doc.children.first()?).ok()?; + json.get("name")?.as_str().map(str::to_string) +} + +#[test] +fn a_saved_document_comes_back() { + let temp = TempStore::new("roundtrip"); + let mut acl = BTreeSet::new(); + acl.insert("userB".to_string()); + + temp.store + .save("userA", &named_document("kept.op"), &acl) + .expect("save"); + + let restored = temp.store.load_document("userA").expect("load"); + assert_eq!(node_name(&restored).as_deref(), Some("kept.op")); + assert_eq!(temp.store.load_acl("userA"), acl); +} + +#[test] +fn an_account_with_nothing_stored_reports_so() { + let temp = TempStore::new("empty"); + assert_eq!( + temp.store.load_document("userA").unwrap_err(), + TenantStoreError::NotStored + ); + assert!(temp.store.load_acl("userA").is_empty()); + assert!(!temp.store.has_document("userA")); +} + +#[test] +fn a_disabled_store_reads_and_writes_nothing() { + let store = TenantStore::new(None); + assert!(!store.is_enabled()); + assert_eq!( + store.load_document("userA").unwrap_err(), + TenantStoreError::Disabled + ); + assert_eq!( + store + .save("userA", &EditorState::starter(), &BTreeSet::new()) + .unwrap_err(), + TenantStoreError::Disabled + ); + assert!(store.tenant_dir("userA").is_none()); +} + +#[test] +fn a_corrupt_document_is_kept_aside_and_the_account_starts_fresh() { + let temp = TempStore::new("corrupt"); + temp.store + .save("userA", &named_document("original.op"), &BTreeSet::new()) + .expect("save"); + let dir = temp.store.tenant_dir("userA").expect("dir"); + let document = dir.join("current.op"); + std::fs::write(&document, b"this is not a document").expect("corrupt it"); + + let error = temp.store.load_document("userA").unwrap_err(); + assert!( + matches!(error, TenantStoreError::Unreadable(_)), + "{error:?}" + ); + assert!( + !document.exists(), + "the unreadable file must be moved aside, not left to fail every visit" + ); + let kept: Vec<_> = std::fs::read_dir(&dir) + .expect("read dir") + .filter_map(Result::ok) + .map(|entry| entry.file_name().to_string_lossy().into_owned()) + .filter(|name| name.contains("corrupt")) + .collect(); + assert_eq!( + kept.len(), + 1, + "the bytes must be preserved, not deleted: {kept:?}" + ); +} + +#[test] +fn a_second_corrupt_file_does_not_overwrite_the_first() { + let temp = TempStore::new("corrupt-twice"); + let dir = temp.store.tenant_dir("userA").expect("dir"); + std::fs::create_dir_all(&dir).expect("dir"); + let document = dir.join("current.op"); + + // Two separate quarantines. The stamp is second-resolution, so this + // asserts only that the first is never destroyed. + std::fs::write(&document, b"garbage one").expect("write"); + let _ = temp.store.load_document("userA"); + std::fs::write(&document, b"garbage two").expect("write"); + let _ = temp.store.load_document("userA"); + + let kept: Vec<_> = std::fs::read_dir(&dir) + .expect("read dir") + .filter_map(Result::ok) + .filter(|entry| entry.file_name().to_string_lossy().contains("corrupt")) + .collect(); + assert!(!kept.is_empty(), "at least the first must survive"); +} + +#[test] +fn a_corrupt_access_list_reads_as_empty_rather_than_granting_anyone() { + let temp = TempStore::new("bad-acl"); + let dir = temp.store.tenant_dir("userA").expect("dir"); + std::fs::create_dir_all(&dir).expect("dir"); + std::fs::write(dir.join("acl.json"), b"{not json").expect("write"); + assert!( + temp.store.load_acl("userA").is_empty(), + "an unreadable access list must grant nobody" + ); +} + +#[test] +fn an_account_id_never_becomes_a_path_component() { + // The whole point of hashing: an id full of traversal cannot address a + // directory outside the store root. + let temp = TempStore::new("traversal"); + for hostile in [ + "../../../../etc/passwd", + "..", + "/etc/shadow", + "a/b/c", + "..\\..\\windows", + "userA\0evil", + ] { + let dir = temp.store.tenant_dir(hostile).expect("dir"); + assert!( + dir.starts_with(&temp.root), + "{hostile:?} escaped the data directory: {dir:?}" + ); + assert_eq!( + dir.parent(), + Some(temp.root.as_path()), + "{hostile:?} produced a nested path" + ); + let name = dir + .file_name() + .expect("name") + .to_string_lossy() + .into_owned(); + assert_eq!(name.len(), 16, "{hostile:?} -> {name}"); + assert!( + name.chars().all(|c| c.is_ascii_hexdigit()), + "{hostile:?} -> {name}" + ); + } +} + +#[test] +fn a_hostile_account_id_still_round_trips_its_own_document() { + // Hashing must not break the account, only the path injection. + let temp = TempStore::new("hostile-roundtrip"); + let hostile = "../../../../etc/passwd"; + temp.store + .save(hostile, &named_document("hostile.op"), &BTreeSet::new()) + .expect("save"); + let restored = temp.store.load_document(hostile).expect("load"); + assert_eq!(node_name(&restored).as_deref(), Some("hostile.op")); + // And it did not collide with a different account. + assert_eq!( + temp.store.load_document("userA").unwrap_err(), + TenantStoreError::NotStored + ); +} + +#[test] +fn directory_names_are_stable_and_distinct() { + assert_eq!(dir_name("userA"), dir_name("userA")); + assert_ne!(dir_name("userA"), dir_name("userB")); + assert_eq!(dir_name("userA").len(), 16); +} + +#[test] +fn a_write_leaves_no_temp_file_behind() { + let temp = TempStore::new("atomic"); + temp.store + .save("userA", &EditorState::starter(), &BTreeSet::new()) + .expect("save"); + let dir = temp.store.tenant_dir("userA").expect("dir"); + let strays: Vec<_> = std::fs::read_dir(&dir) + .expect("read dir") + .filter_map(Result::ok) + .filter(|entry| entry.file_name().to_string_lossy().ends_with(".tmp")) + .collect(); + assert!( + strays.is_empty(), + "a completed write must leave no temp file" + ); +} + +#[test] +fn saving_the_access_list_alone_does_not_require_a_document() { + let temp = TempStore::new("acl-only"); + let mut acl = BTreeSet::new(); + acl.insert("userB".to_string()); + temp.store.save_acl_for("userA", &acl).expect("save acl"); + assert_eq!(temp.store.load_acl("userA"), acl); + assert!(!temp.store.has_document("userA")); +} diff --git a/crates/op-host-services/src/web_canvas_server_tests.rs b/crates/op-host-services/src/web_canvas_server_tests.rs index 3cd305a0e..327c0cff0 100644 --- a/crates/op-host-services/src/web_canvas_server_tests.rs +++ b/crates/op-host-services/src/web_canvas_server_tests.rs @@ -435,6 +435,7 @@ fn sensitive_browser_posts_include_credentials_and_ai_routes() { content_type: None, authorization: None, cookie: None, + query: None, }; assert!(is_sensitive_browser_post(&request), "path={path}"); } diff --git a/crates/op-host-web/src/collab_sync.rs b/crates/op-host-web/src/collab_sync.rs index e335dceff..0d9bf3d5c 100644 --- a/crates/op-host-web/src/collab_sync.rs +++ b/crates/op-host-web/src/collab_sync.rs @@ -632,7 +632,9 @@ fn ensure_event_stream(base: &str) { if now_ms_f64() < SSE_RETRY_AT_MS.get() { return; } - let Ok(stream) = web_sys::EventSource::new(&format!("{base}/api/mcp/events")) else { + let Ok(stream) = web_sys::EventSource::new(&crate::daemon_base::with_tenant_param(&format!( + "{base}/api/mcp/events" + ))) else { note_stream_failure(); return; }; diff --git a/crates/op-host-web/src/daemon_base.rs b/crates/op-host-web/src/daemon_base.rs index 11d301742..c9ac8cf5b 100644 --- a/crates/op-host-web/src/daemon_base.rs +++ b/crates/op-host-web/src/daemon_base.rs @@ -107,3 +107,85 @@ mod tests { ); } } + +/// The tenant this tab is viewing, when the page URL names one. +/// +/// A visitor opens a shared document with `?tenant=` on the page +/// URL, and every daemon request this shell makes must carry the same +/// parameter — otherwise the poll, the push and the event stream would each +/// address a different document. Read once (the value cannot change without +/// a navigation, which reloads the shell) and appended by +/// [`with_tenant_param`], which every request path funnels through. +/// +/// This is a REQUEST, not a credential: the daemon still resolves the +/// caller's own identity and checks it against the owner's access list, so a +/// hand-edited parameter buys nothing. +pub fn tenant_param() -> Option { + thread_local! { + static TENANT: std::cell::OnceCell> = const { + std::cell::OnceCell::new() + }; + } + TENANT.with(|cell| { + cell.get_or_init(|| { + let search = web_sys::window()?.location().search().ok()?; + let query = search.strip_prefix('?').unwrap_or(&search); + op_editor_core::share_routes::tenant_from_query(query).map(str::to_string) + }) + .clone() + }) +} + +/// Append the tab's tenant parameter to `url`, if it has one. +/// +/// Pure so the query-joining rule is testable without a DOM; the browser +/// wrapper is [`with_tenant_param`]. +pub fn append_tenant_param(url: &str, tenant: Option<&str>) -> String { + let Some(tenant) = tenant.filter(|value| !value.is_empty()) else { + return url.to_string(); + }; + let separator = if url.contains('?') { '&' } else { '?' }; + format!( + "{url}{separator}{}={tenant}", + op_editor_core::share_routes::TENANT_QUERY + ) +} + +/// Browser wrapper over [`append_tenant_param`]. +/// +/// Applied inside the HTTP helpers rather than at each call site, so a route +/// added later cannot forget it and silently address the wrong document. +pub fn with_tenant_param(url: &str) -> String { + append_tenant_param(url, tenant_param().as_deref()) +} + +#[cfg(test)] +mod tenant_param_tests { + use super::append_tenant_param; + + #[test] + fn a_url_without_a_query_gains_one() { + assert_eq!( + append_tenant_param("http://x/api/mcp/document", Some("userA")), + "http://x/api/mcp/document?tenant=userA" + ); + } + + #[test] + fn a_url_that_already_has_a_query_is_extended() { + assert_eq!( + append_tenant_param("http://x/api/mcp/document?v=2", Some("userA")), + "http://x/api/mcp/document?v=2&tenant=userA" + ); + } + + #[test] + fn no_tenant_leaves_the_url_exactly_as_it_was() { + for tenant in [None, Some("")] { + assert_eq!( + append_tenant_param("http://x/api/mcp/document", tenant), + "http://x/api/mcp/document" + ); + } + } +} diff --git a/crates/op-host-web/src/live_sync.rs b/crates/op-host-web/src/live_sync.rs index 127cf13ee..b88b329b8 100644 --- a/crates/op-host-web/src/live_sync.rs +++ b/crates/op-host-web/src/live_sync.rs @@ -93,7 +93,10 @@ pub fn get(url: &str, on_response: Rc) -> bool { let Ok(xhr) = web_sys::XmlHttpRequest::new() else { return false; }; - if xhr.open_with_async("GET", url, true).is_err() { + if xhr + .open_with_async("GET", &crate::daemon_base::with_tenant_param(url), true) + .is_err() + { return false; } attach_daemon_headers(&xhr, url); @@ -119,7 +122,10 @@ pub fn get_with_status(url: &str, on_response: Rc) -> bool let Ok(xhr) = web_sys::XmlHttpRequest::new() else { return false; }; - if xhr.open_with_async("GET", url, true).is_err() { + if xhr + .open_with_async("GET", &crate::daemon_base::with_tenant_param(url), true) + .is_err() + { return false; } attach_daemon_headers(&xhr, url); @@ -146,7 +152,10 @@ pub fn post_json(url: &str, body: &str, on_response: Option>) let Ok(xhr) = web_sys::XmlHttpRequest::new() else { return false; }; - if xhr.open_with_async("POST", url, true).is_err() { + if xhr + .open_with_async("POST", &crate::daemon_base::with_tenant_param(url), true) + .is_err() + { return false; } attach_daemon_headers(&xhr, url); @@ -173,7 +182,10 @@ pub fn post_json_with_status(url: &str, body: &str, on_response: Rc bool { + has_conflict && (server_authoritative || matches!(phase, CollabConnectionPhase::Active)) +} + +/// Whether the daemon this shell talks to is the sole sequencer for the +/// document. +/// +/// Learned once from `GET /api/mcp/server`'s `serveMode` (see +/// [`probe_serve_mode`]) and cached, because it is a property of the +/// deployment and cannot change without a reload. Defaults to `false` until +/// the probe answers, so the conservative behaviour is what runs during +/// start-up rather than the permissive one. +pub(super) fn server_is_authoritative() -> bool { + SERVER_AUTHORITATIVE.with(|flag| flag.get()) +} + +thread_local! { + static SERVER_AUTHORITATIVE: std::cell::Cell = const { + std::cell::Cell::new(false) + }; +} + +/// Record what `GET /api/mcp/server` said about the deployment. +/// +/// Split from the fetch so the parse is testable without a DOM. +fn note_serve_mode(body: &str) { + if let Some(authoritative) = parse_server_authoritative(body) { + SERVER_AUTHORITATIVE.with(|flag| flag.set(authoritative)); + } +} + +/// Read `serveMode` out of the health response. +/// +/// `None` when the field is absent — an older daemon, which is by definition +/// not an online one, so the caller leaves the conservative default in place. +fn parse_server_authoritative(body: &str) -> Option { + let parsed: serde_json::Value = serde_json::from_str(body).ok()?; + let mode = parsed.get("serveMode")?.as_str()?; + Some(mode == "online") +} + +/// Ask the daemon once, at start-up, which deployment this is. +pub(super) fn probe_serve_mode(base: &str) { + crate::live_sync::get( + &format!("{base}/api/mcp/server"), + std::rc::Rc::new(|body: String| note_serve_mode(&body)), + ); +} + +#[cfg(test)] +mod auto_resolve_tests { + use super::*; + use op_editor_core::CollabConnectionPhase; + + #[test] + fn a_conflict_inside_an_active_session_resolves_itself() { + assert!(auto_resolve_is_safe( + true, + CollabConnectionPhase::Active, + false + )); + } + + #[test] + fn no_conflict_means_nothing_to_resolve() { + for phase in [ + CollabConnectionPhase::Idle, + CollabConnectionPhase::Active, + CollabConnectionPhase::ReadOnly, + ] { + assert!(!auto_resolve_is_safe(false, phase, false)); + } + } + + #[test] + fn every_non_active_phase_keeps_the_latch() { + // Outside an Active session there is no authoritative server document + // to accept and no `discarded_edit` projection to recover the losing + // edit from, so auto-accepting would silently destroy unpushed work. + // `Reconnecting` and `ReadOnly` look session-ish and are deliberately + // included: neither can sequence an edit. + for phase in [ + CollabConnectionPhase::Idle, + CollabConnectionPhase::Starting, + CollabConnectionPhase::Discovering, + CollabConnectionPhase::Joining, + CollabConnectionPhase::Authenticating, + CollabConnectionPhase::Reconnecting, + CollabConnectionPhase::ReadOnly, + CollabConnectionPhase::Ended, + ] { + assert!( + !auto_resolve_is_safe(true, phase, false), + "{phase:?} must keep the existing explicit-resolution semantics" + ); + } + } +} + +#[cfg(test)] +mod server_authority_tests { + use super::{auto_resolve_is_safe, parse_server_authoritative}; + use op_editor_core::CollabConnectionPhase; + + #[test] + fn an_online_daemon_is_authoritative() { + assert_eq!( + parse_server_authoritative(r#"{"running":true,"port":3100,"serveMode":"online"}"#), + Some(true) + ); + } + + #[test] + fn a_local_or_managed_daemon_is_not() { + for mode in ["local", "managed"] { + assert_eq!( + parse_server_authoritative(&format!(r#"{{"serveMode":"{mode}"}}"#)), + Some(false), + "{mode}" + ); + } + } + + #[test] + fn a_daemon_that_does_not_report_a_mode_leaves_the_default_alone() { + // An older daemon has no `serveMode`; it is by definition not an + // online one, so the conservative default must survive the probe. + for body in [r#"{"running":true}"#, "not json", "", "{}"] { + assert_eq!(parse_server_authoritative(body), None, "{body:?}"); + } + } + + #[test] + fn a_server_authoritative_deployment_auto_resolves_outside_a_session() { + // This is the M4 case: an online shared tenant has no collaboration + // session, so without this the 409 latch would never lift and the + // visitor would be frozen out of the document. + for phase in [ + CollabConnectionPhase::Idle, + CollabConnectionPhase::Starting, + CollabConnectionPhase::Reconnecting, + CollabConnectionPhase::ReadOnly, + ] { + assert!( + auto_resolve_is_safe(true, phase, true), + "{phase:?} must auto-resolve when the server is the sequencer" + ); + } + } + + #[test] + fn no_conflict_never_resolves_however_authoritative_the_server_is() { + assert!(!auto_resolve_is_safe( + false, + CollabConnectionPhase::Idle, + true + )); + assert!(!auto_resolve_is_safe( + false, + CollabConnectionPhase::Active, + true + )); + } + + #[test] + fn a_peer_daemon_outside_a_session_still_latches() { + // The local daemon holds the operator's file and arbitrates nothing; + // auto-accepting there would silently discard unpushed work. + for phase in [ + CollabConnectionPhase::Idle, + CollabConnectionPhase::Reconnecting, + CollabConnectionPhase::ReadOnly, + ] { + assert!(!auto_resolve_is_safe(true, phase, false), "{phase:?}"); + } + } +} diff --git a/crates/op-host-web/src/live_sync_glue.rs b/crates/op-host-web/src/live_sync_glue.rs index eaf2f2d0b..c11257a95 100644 --- a/crates/op-host-web/src/live_sync_glue.rs +++ b/crates/op-host-web/src/live_sync_glue.rs @@ -233,6 +233,9 @@ fn push_reasons( pub(crate) fn start(inner: &Rc>, sync: SharedSync) { ACTIVE_SYNC.with(|slot| *slot.borrow_mut() = Some(Rc::downgrade(&sync))); let base = crate::daemon_base::daemon_base(); + // One-shot: learn whether this daemon is the document's sole sequencer, + // which decides whether a sync conflict may auto-resolve. + probe_serve_mode(&base); // One document fetch / one push at a time; ticks observing an in-flight // request skip (the TS hook queues at most one — same effective shape). // `fetch_busy` stays a plain Cell (pull-side, local to this module); @@ -371,7 +374,7 @@ fn maybe_auto_resolve_conflict_in_session( .try_borrow() .map(|context| context.host().editor_state().editor_ui.collab.phase) .unwrap_or(op_editor_core::CollabConnectionPhase::Idle); - if !auto_resolve_is_safe(has_conflict, phase) { + if !auto_resolve_is_safe(has_conflict, phase, server_is_authoritative()) { return; } // Re-opens the pull for THIS pair only; the resolving pull's apply calls @@ -382,21 +385,6 @@ fn maybe_auto_resolve_conflict_in_session( } } -/// The safety decision behind [`maybe_auto_resolve_conflict_in_session`], -/// separated so it can be tested without a live shell. -/// -/// `Active` and nothing else. Every other phase — including `Reconnecting` and -/// `ReadOnly`, which look session-ish — either has no authoritative server -/// document to accept or no `discarded_edit` projection to recover the losing -/// edit from, so auto-accepting there would be the silent data loss this is -/// specifically avoiding. -const fn auto_resolve_is_safe( - has_conflict: bool, - phase: op_editor_core::CollabConnectionPhase, -) -> bool { - has_conflict && matches!(phase, op_editor_core::CollabConnectionPhase::Active) -} - /// Apply a `GET /api/mcp/document` response to the live shell. /// `WebSyncClient::sync_with_editor_meta` runs the apply closure only for a /// newer document and commits that exact version only when the closure returns @@ -782,48 +770,6 @@ mod tests { } } -#[cfg(test)] -mod auto_resolve_tests { - use super::auto_resolve_is_safe; - use op_editor_core::CollabConnectionPhase; - - #[test] - fn a_conflict_inside_an_active_session_resolves_itself() { - assert!(auto_resolve_is_safe(true, CollabConnectionPhase::Active)); - } - - #[test] - fn no_conflict_means_nothing_to_resolve() { - for phase in [ - CollabConnectionPhase::Idle, - CollabConnectionPhase::Active, - CollabConnectionPhase::ReadOnly, - ] { - assert!(!auto_resolve_is_safe(false, phase)); - } - } - - #[test] - fn every_non_active_phase_keeps_the_latch() { - // Outside an Active session there is no authoritative server document - // to accept and no `discarded_edit` projection to recover the losing - // edit from, so auto-accepting would silently destroy unpushed work. - // `Reconnecting` and `ReadOnly` look session-ish and are deliberately - // included: neither can sequence an edit. - for phase in [ - CollabConnectionPhase::Idle, - CollabConnectionPhase::Starting, - CollabConnectionPhase::Discovering, - CollabConnectionPhase::Joining, - CollabConnectionPhase::Authenticating, - CollabConnectionPhase::Reconnecting, - CollabConnectionPhase::ReadOnly, - CollabConnectionPhase::Ended, - ] { - assert!( - !auto_resolve_is_safe(true, phase), - "{phase:?} must keep the existing explicit-resolution semantics" - ); - } - } -} +#[path = "live_sync_conflict.rs"] +mod live_sync_conflict; +use live_sync_conflict::{auto_resolve_is_safe, probe_serve_mode, server_is_authoritative};