From b556539f94d51b23e2ccd4a9c4d4ae3fce5b8bb9 Mon Sep 17 00:00:00 2001 From: Maurus Decimus <11444311+mdecimus@users.noreply.github.com> Date: Sat, 16 May 2026 11:20:43 +0200 Subject: [PATCH] JMAP for File Storage improvements --- CHANGELOG.md | 1 + .../src/config/mailstore/capabilities.rs | 8 +- crates/dav/src/common/acl.rs | 1 + crates/dav/src/file/copy_move.rs | 9 + crates/dav/src/file/proppatch.rs | 1 + crates/groupware/src/file/storage.rs | 18 +- crates/jmap-proto/src/object/file_node.rs | 17 +- crates/jmap/src/file/copy.rs | 4 + crates/jmap/src/file/get.rs | 16 +- crates/jmap/src/file/query.rs | 72 +++++- crates/jmap/src/file/set.rs | 229 ++++++++++++------ tests/src/jmap/files/node.rs | 6 +- 12 files changed, 278 insertions(+), 104 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5db30f8..735bb425 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ If you are upgrading from v0.16.x, replace the binary (or run `docker pull`). If - Dns updater: Log DNS record types and values. ## Changed +- Bump JMAP File Storage to [draft-ietf-jmap-filenode-14](https://datatracker.ietf.org/doc/html/draft-ietf-jmap-filenode-14). - Accept password hashes with `$` or `{` prefixes as secure secrets. ## Fixed diff --git a/crates/common/src/config/mailstore/capabilities.rs b/crates/common/src/config/mailstore/capabilities.rs index 98a48351..131ca9e9 100644 --- a/crates/common/src/config/mailstore/capabilities.rs +++ b/crates/common/src/config/mailstore/capabilities.rs @@ -9,7 +9,7 @@ use ahash::AHashSet; use calcard::icalendar::ICalendarDuration; use chrono::{DateTime, Utc}; use jmap_proto::{ - object::email::EmailComparator, + object::{email::EmailComparator, file_node::FileNodeComparator}, request::capability::{ BlobCapabilities, CalendarCapabilities, Capabilities, Capability, ContactsCapabilities, CoreCapabilities, EmptyCapabilities, FileNodeCapabilities, MailCapabilities, @@ -148,7 +148,11 @@ impl JmapConfig { .map(str::to_string) .collect(), ), - file_node_query_sort_options: vec![], + file_node_query_sort_options: vec![ + FileNodeComparator::Name, + FileNodeComparator::Size, + FileNodeComparator::NodeType, + ], may_create_top_level_file_node: true, case_insensitive_names: false, web_trash_url: None, diff --git a/crates/dav/src/common/acl.rs b/crates/dav/src/common/acl.rs index 37f57fbd..b3a5c796 100644 --- a/crates/dav/src/common/acl.rs +++ b/crates/dav/src/common/acl.rs @@ -185,6 +185,7 @@ impl DavAclHandler for Server { node, account_id, resource.document_id(), + true, &mut batch, ) .caused_by(trc::location!())?; diff --git a/crates/dav/src/file/copy_move.rs b/crates/dav/src/file/copy_move.rs index 0bcf267e..e894e4d0 100644 --- a/crates/dav/src/file/copy_move.rs +++ b/crates/dav/src/file/copy_move.rs @@ -396,6 +396,7 @@ async fn move_container( node, from_account_id, from_document_id, + true, &mut batch, ) .caused_by(trc::location!())? @@ -619,6 +620,7 @@ async fn overwrite_and_delete_item( dest_node, to_account_id, to_document_id, + true, &mut batch, ) .caused_by(trc::location!())? @@ -694,6 +696,7 @@ async fn overwrite_item( dest_node, to_account_id, to_document_id, + true, &mut batch, ) .caused_by(trc::location!())? @@ -748,6 +751,7 @@ async fn move_item( node, from_account_id, from_document_id, + true, &mut batch, ) .caused_by(trc::location!())? @@ -764,6 +768,8 @@ async fn move_item( access_token.account_tenant_ids(), to_account_id, to_document_id, + true, + true, &mut batch, ) .caused_by(trc::location!())? @@ -826,6 +832,8 @@ async fn copy_item( access_token.account_tenant_ids(), to_account_id, to_document_id, + true, + true, &mut batch, ) .caused_by(trc::location!())? @@ -873,6 +881,7 @@ async fn rename_item( node, from_account_id, from_document_id, + true, &mut batch, ) .caused_by(trc::location!())? diff --git a/crates/dav/src/file/proppatch.rs b/crates/dav/src/file/proppatch.rs index 9e46258f..8f76b646 100644 --- a/crates/dav/src/file/proppatch.rs +++ b/crates/dav/src/file/proppatch.rs @@ -153,6 +153,7 @@ impl FilePropPatchRequestHandler for Server { node, account_id, resource.resource, + true, &mut batch, ) .caused_by(trc::location!())? diff --git a/crates/groupware/src/file/storage.rs b/crates/groupware/src/file/storage.rs index 187e151e..a8f468d6 100644 --- a/crates/groupware/src/file/storage.rs +++ b/crates/groupware/src/file/storage.rs @@ -20,13 +20,18 @@ impl FileNode { changed_by: AccountTenantIds, account_id: u32, document_id: u32, + set_created: bool, + set_modified: bool, batch: &mut BatchBuilder, ) -> trc::Result<&mut BatchBuilder> { - // Build node let mut node = self; let now = now() as i64; - node.modified = now; - node.created = now; + if set_created { + node.created = now; + } + if set_modified { + node.modified = now; + } // Prepare write batch batch @@ -40,17 +45,20 @@ impl FileNode { ) .map(|b| b.commit_point()) } + pub fn update<'x>( self, changed_by: AccountTenantIds, node: Archive<&ArchivedFileNode>, account_id: u32, document_id: u32, + set_modified: bool, batch: &'x mut BatchBuilder, ) -> trc::Result<&'x mut BatchBuilder> { - // Build node let mut new_node = self; - new_node.modified = now() as i64; + if set_modified { + new_node.modified = now() as i64; + } batch .with_account_id(account_id) .with_collection(Collection::FileNode) diff --git a/crates/jmap-proto/src/object/file_node.rs b/crates/jmap-proto/src/object/file_node.rs index b492a34d..cd872263 100644 --- a/crates/jmap-proto/src/object/file_node.rs +++ b/crates/jmap-proto/src/object/file_node.rs @@ -323,12 +323,15 @@ impl<'de> serde::Deserialize<'de> for OnExists { D: serde::Deserializer<'de>, { let value: Option> = Option::deserialize(deserializer)?; - Ok(match value.as_deref() { - Some("replace") => OnExists::Replace, - Some("rename") => OnExists::Rename, - Some("newest") => OnExists::Newest, - _ => OnExists::Reject, - }) + match value.as_deref() { + Some("replace") => Ok(OnExists::Replace), + Some("rename") => Ok(OnExists::Rename), + Some("newest") => Ok(OnExists::Newest), + None | Some("") => Ok(OnExists::Reject), + Some(other) => Err(serde::de::Error::custom(format!( + "Invalid onExists value: {other:?}" + ))), + } } } @@ -626,7 +629,7 @@ impl<'de> DeserializeArguments<'de> for FileNodeComparator { *self = FileNodeComparator::Tree; }, _ => { - *self = FileNodeComparator::_T(key.to_string()); + *self = FileNodeComparator::_T(value.into_owned()); } ); } else { diff --git a/crates/jmap/src/file/copy.rs b/crates/jmap/src/file/copy.rs index e22d19ee..7599f496 100644 --- a/crates/jmap/src/file/copy.rs +++ b/crates/jmap/src/file/copy.rs @@ -387,11 +387,15 @@ impl FileNodeCopy for Server { None, ); let final_name = file_node.name.clone(); + let set_created = file_node.created == 0; + let set_modified = file_node.modified == 0; file_node .insert( access_token.account_tenant_ids(), account_id, document_id, + set_created, + set_modified, &mut batch, ) .caused_by(trc::location!())?; diff --git a/crates/jmap/src/file/get.rs b/crates/jmap/src/file/get.rs index bbee2511..94d64ff3 100644 --- a/crates/jmap/src/file/get.rs +++ b/crates/jmap/src/file/get.rs @@ -68,6 +68,7 @@ impl FileNodeGet for Server { SyncCollection::FileNode, ) .await?; + // TODO: draft-14 section 5 case 2 - ancestors of shared nodes should be discoverable with mayRead=false let file_node_ids = if access_token.is_member(account_id) { cache .resources @@ -217,10 +218,14 @@ impl FileNodeGet for Server { FileNodeProperty::Type => { result.insert_unchecked( FileNodeProperty::Type, - if let Some(file) = - file_node.file.as_ref().and_then(|f| f.media_type.as_ref()) - { - Value::Str(file.to_string().into()) + if let Some(file) = file_node.file.as_ref() { + Value::Str( + file.media_type + .as_ref() + .map(|t| t.to_string()) + .unwrap_or_else(|| "application/octet-stream".to_string()) + .into(), + ) } else { Value::Null }, @@ -253,6 +258,7 @@ impl FileNodeGet for Server { ); } FileNodeProperty::Accessed => { + // TODO: needs serialization change (per-user accessed timestamp); returns now() as a placeholder result.insert_unchecked( FileNodeProperty::Accessed, Value::Element(FileNodeValue::Date(UTCDate::from_timestamp( @@ -261,6 +267,7 @@ impl FileNodeGet for Server { ); } FileNodeProperty::Changed => { + // TODO: needs serialization change (dedicated server-set changed timestamp); returns modified as a placeholder result.insert_unchecked( FileNodeProperty::Changed, Value::Element(FileNodeValue::Date(UTCDate::from_timestamp( @@ -286,6 +293,7 @@ impl FileNodeGet for Server { result.insert_unchecked(FileNodeProperty::Role, Value::Null); } FileNodeProperty::IsSubscribed => { + // TODO: needs serialization change (per-user subscription state); always true for now result.insert_unchecked(FileNodeProperty::IsSubscribed, Value::Bool(true)); } property => { diff --git a/crates/jmap/src/file/query.rs b/crates/jmap/src/file/query.rs index a63d331d..cc9ee494 100644 --- a/crates/jmap/src/file/query.rs +++ b/crates/jmap/src/file/query.rs @@ -9,10 +9,11 @@ use common::{Server, auth::AccessToken}; use groupware::cache::GroupwareCache; use jmap_proto::{ method::query::{Filter, QueryRequest, QueryResponse}, - object::file_node::{FileNode, FileNodeFilter}, + object::file_node::{FileNode, FileNodeComparator, FileNodeFilter}, request::MaybeInvalid, }; use store::{ + ahash::AHashMap, roaring::RoaringBitmap, search::{SearchFilter, SearchQuery}, write::SearchIndex, @@ -190,8 +191,6 @@ impl FileNodeQuery for Server { } } - // TODO: implement FileNode/query sort (name, size, type, created, modified, nodeType, tree) - let results = SearchQuery::new(SearchIndex::InMemory) .with_filters(filters) .with_mask(if access_token.is_shared(account_id) { @@ -209,9 +208,70 @@ impl FileNodeQuery for Server { &request, ); - for document_id in results { - if !response.add(0, document_id) { - break; + // Only name, size and nodeType can be sorted from the cache. + // TODO: created/modified/type/tree sorts require archive or hierarchy traversal + let sortable = request + .sort + .as_deref() + .unwrap_or_default() + .iter() + .filter(|c| { + matches!( + c.property, + FileNodeComparator::Name + | FileNodeComparator::Size + | FileNodeComparator::NodeType + ) + }) + .collect::>(); + + if sortable.is_empty() { + for document_id in results { + if !response.add(0, document_id) { + break; + } + } + } else { + let by_id = cache + .resources + .iter() + .map(|r| (r.document_id, r)) + .collect::>(); + let mut ids = results.iter().collect::>(); + ids.sort_unstable_by(|a, b| { + for cmp in &sortable { + let ra = by_id.get(a); + let rb = by_id.get(b); + let ordering = match cmp.property { + FileNodeComparator::Name => ra + .and_then(|r| r.container_name()) + .cmp(&rb.and_then(|r| r.container_name())), + FileNodeComparator::Size => ra + .and_then(|r| r.size()) + .cmp(&rb.and_then(|r| r.size())), + FileNodeComparator::NodeType => { + // Directories sort before files + let a_dir = ra.map(|r| r.is_container()).unwrap_or(false); + let b_dir = rb.map(|r| r.is_container()).unwrap_or(false); + b_dir.cmp(&a_dir) + } + _ => std::cmp::Ordering::Equal, + }; + let ordering = if cmp.is_ascending { + ordering + } else { + ordering.reverse() + }; + if ordering != std::cmp::Ordering::Equal { + return ordering; + } + } + a.cmp(b) + }); + for document_id in ids { + if !response.add(0, document_id) { + break; + } } } diff --git a/crates/jmap/src/file/set.rs b/crates/jmap/src/file/set.rs index 34485030..ac25ff48 100644 --- a/crates/jmap/src/file/set.rs +++ b/crates/jmap/src/file/set.rs @@ -166,19 +166,14 @@ impl FileNodeSet for Server { Collision::Existing(existing) => { let effective = match on_exists { OnExists::Newest => { - let existing_modified = fetch_existing_modified( - self.store(), - account_id, - existing, - ) - .await?; + let existing_modified = + fetch_existing_modified(self.store(), account_id, existing).await?; if file_node.modified > existing_modified { OnExists::Replace } else { response.not_created.append( id, - SetError::already_exists() - .with_existing_id(Id::from(existing)), + SetError::already_exists().with_existing_id(Id::from(existing)), ); continue 'create; } @@ -293,11 +288,15 @@ impl FileNodeSet for Server { } let final_name = file_node.name.clone(); pending_names.insert(pending_key(&file_node, case_insensitive), None); + let set_created = file_node.created == 0; + let set_modified = file_node.modified == 0; file_node .insert( access_token.account_tenant_ids(), account_id, document_id, + set_created, + set_modified, &mut batch, ) .caused_by(trc::location!())?; @@ -343,45 +342,46 @@ impl FileNodeSet for Server { .caused_by(trc::location!())?; // Apply changes - let has_acl_changes = match update_file_node(object, &mut new_file_node, false, &response) - { - Ok(result) => { - if let Some(blob_id) = result.blob_id { - let file_details = new_file_node.file.get_or_insert_default(); - if !self.has_access_blob(&blob_id, access_token).await? { - response.not_updated.append( - id, - SetError::forbidden().with_description(format!( - "You do not have access to blobId {blob_id}." - )), - ); - continue; - } else if let Some(blob_contents) = self - .blob_store() - .get_blob(blob_id.hash.as_slice(), 0..usize::MAX) - .await? - { - file_details.size = blob_contents.len() as u32; - } else { - response.not_updated.append( - id, - SetError::invalid_properties() - .with_property(FileNodeProperty::BlobId) - .with_description("Blob could not be found."), - ); - continue 'update; + let (has_acl_changes, modified_set) = + match update_file_node(object, &mut new_file_node, false, &response) { + Ok(result) => { + let modified_set = result.modified_set; + if let Some(blob_id) = result.blob_id { + let file_details = new_file_node.file.get_or_insert_default(); + if !self.has_access_blob(&blob_id, access_token).await? { + response.not_updated.append( + id, + SetError::forbidden().with_description(format!( + "You do not have access to blobId {blob_id}." + )), + ); + continue; + } else if let Some(blob_contents) = self + .blob_store() + .get_blob(blob_id.hash.as_slice(), 0..usize::MAX) + .await? + { + file_details.size = blob_contents.len() as u32; + } else { + response.not_updated.append( + id, + SetError::invalid_properties() + .with_property(FileNodeProperty::BlobId) + .with_description("Blob could not be found."), + ); + continue 'update; + } + + file_details.blob_hash = blob_id.hash; } - file_details.blob_hash = blob_id.hash; + (result.has_acl_changes, modified_set) } - - result.has_acl_changes - } - Err(err) => { - response.not_updated.append(id, err); - continue 'update; - } - }; + Err(err) => { + response.not_updated.append(id, err); + continue 'update; + } + }; // Validate hierarchy if let Err(err) = validate_file_node_hierarchy( @@ -406,19 +406,14 @@ impl FileNodeSet for Server { Collision::Existing(existing) => { let effective = match on_exists { OnExists::Newest => { - let existing_modified = fetch_existing_modified( - self.store(), - account_id, - existing, - ) - .await?; + let existing_modified = + fetch_existing_modified(self.store(), account_id, existing).await?; if new_file_node.modified > existing_modified { OnExists::Replace } else { response.not_updated.append( id, - SetError::already_exists() - .with_existing_id(Id::from(existing)), + SetError::already_exists().with_existing_id(Id::from(existing)), ); continue 'update; } @@ -522,13 +517,14 @@ impl FileNodeSet for Server { pending_key(&new_file_node, case_insensitive), Some(document_id), ); - // Update record + // Update record. Bump modified to now() unless the client supplied a value. new_file_node .update( access_token.account_tenant_ids(), file_node, account_id, document_id, + !modified_set, &mut batch, ) .caused_by(trc::location!())?; @@ -643,6 +639,7 @@ impl FileNodeSet for Server { pub(super) struct UpdateResult { pub(super) has_acl_changes: bool, pub(super) blob_id: Option, + pub(super) modified_set: bool, } pub(super) struct NoResolver; @@ -661,6 +658,10 @@ pub(super) fn update_file_node Result> { let mut has_acl_changes = false; let mut blob_id = None; + let mut pending_size: Option = None; + let mut pending_type: Option> = None; + let mut pending_executable: Option = None; + let mut modified_set = false; for (property, mut value) in updates.into_expanded_object() { let Key::Property(property) = property else { @@ -672,13 +673,23 @@ pub(super) fn update_file_node - { + (FileNodeProperty::Name, Value::Str(value)) => { + if !(1..=255).contains(&value.len()) { + return Err(SetError::invalid_properties() + .with_property(FileNodeProperty::Name) + .with_description("Name must be between 1 and 255 octets.")); + } else if value.contains(|c: char| FORBIDDEN_NAME_CHARS.contains(c)) { + return Err(SetError::invalid_properties() + .with_property(FileNodeProperty::Name) + .with_description("Name contains a forbidden character.")); + } else if FORBIDDEN_NODE_NAMES + .iter() + .any(|n| n.eq_ignore_ascii_case(value.as_ref())) + { + return Err(SetError::invalid_properties() + .with_property(FileNodeProperty::Name) + .with_description("Name is reserved and cannot be used.")); + } file_node.name = value.into_owned(); } (FileNodeProperty::ParentId, Value::Element(FileNodeValue::Id(value))) => { @@ -698,32 +709,49 @@ pub(super) fn update_file_node {} (FileNodeProperty::Size, Value::Number(value)) => { - file_node.file.get_or_insert_default().size = value.cast_to_u64() as u32; + let value = value.cast_to_u64(); + if value > u32::MAX as u64 { + return Err(SetError::invalid_properties() + .with_property(FileNodeProperty::Size) + .with_description("size is too large.")); + } + pending_size = Some(value as u32); + } + (FileNodeProperty::Size, Value::Null) => { + pending_size = Some(0); } (FileNodeProperty::Type, Value::Str(value)) if (1..=256).contains(&value.len()) && value.contains('/') => { // TODO: validate full RFC 6838 Section 4.2 ABNF for media types - file_node.file.get_or_insert_default().media_type = value.into_owned().into(); + pending_type = Some(Some(value.into_owned())); } (FileNodeProperty::Type, Value::Null) => { - file_node.file.get_or_insert_default().media_type = None; + pending_type = Some(None); } (FileNodeProperty::Executable, Value::Bool(value)) => { - file_node.file.get_or_insert_default().executable = value; + pending_executable = Some(value); } (FileNodeProperty::Executable, Value::Null) => { - file_node.file.get_or_insert_default().executable = false; + pending_executable = Some(false); } - (FileNodeProperty::Created, Value::Element(FileNodeValue::Date(value))) => { + (FileNodeProperty::Created, Value::Element(FileNodeValue::Date(value))) + if is_create => + { file_node.created = value.timestamp(); } - // TODO: groupware::file::insert/update clobber modified with now(); preserve client-supplied value + (FileNodeProperty::Created, _) => { + return Err(SetError::invalid_properties() + .with_property(FileNodeProperty::Created) + .with_description("created is immutable after creation.")); + } (FileNodeProperty::Modified, Value::Element(FileNodeValue::Date(value))) => { file_node.modified = value.timestamp(); + modified_set = true; } (FileNodeProperty::Modified, Value::Null) => { file_node.modified = now() as i64; + modified_set = true; } // TODO: persist accessed per-user (draft-13 section 3.1) (FileNodeProperty::Accessed, _) => {} @@ -772,6 +800,29 @@ pub(super) fn update_file_node 0 && i < base.len() - 1 => (&base[..i], &base[i..]), _ => (base, ""), }; - let mut probe = FileNode { - parent_id, - name: String::new(), - ..FileNode::default() + let fold = |s: &str| { + if case_insensitive { + s.to_lowercase() + } else { + s.to_string() + } }; + let node_parent_id = if parent_id == 0 { + None + } else { + Some(parent_id - 1) + }; + + // Collect all sibling names once, instead of rescanning per probe. + let mut taken: AHashSet = AHashSet::new(); + for resource in &cache.resources { + if let DavResourceMetadata::File { + name, parent_id, .. + } = &resource.data + && document_id.is_none_or(|id| id != resource.document_id) + && node_parent_id == *parent_id + { + taken.insert(fold(name)); + } + } + for ((pending_parent, pending_name), _) in pending { + if *pending_parent == parent_id { + taken.insert(fold(pending_name)); + } + } + for n in 2u32.. { - probe.name = format!("{stem} ({n}){ext}"); - if matches!( - find_sibling_collision(document_id, &probe, cache, pending, case_insensitive), - Collision::None - ) { - return probe.name; + let candidate = format!("{stem} ({n}){ext}"); + if !taken.contains(&fold(&candidate)) { + return candidate; } } unreachable!() diff --git a/tests/src/jmap/files/node.rs b/tests/src/jmap/files/node.rs index c7a44822..ca00e10a 100644 --- a/tests/src/jmap/files/node.rs +++ b/tests/src/jmap/files/node.rs @@ -206,15 +206,15 @@ pub async fn test(test: &TestServer) { ); assert_eq!( response.not_created(2).description(), - "Field could not be set." + "Name contains a forbidden character." ); assert_eq!( response.not_created(3).description(), - "Field could not be set." + "Name is reserved and cannot be used." ); assert_eq!( response.not_created(4).description(), - "Field could not be set." + "Name is reserved and cannot be used." ); // Circular folder references should fail