From cd2b95800170bb83e810814c18001f12e0960c00 Mon Sep 17 00:00:00 2001 From: mdecimus Date: Sun, 7 Sep 2025 19:20:39 +0200 Subject: [PATCH] WebDAV: Assisted discovery v2 --- crates/common/src/config/groupware.rs | 2 +- crates/dav/src/common/mod.rs | 27 ---- crates/dav/src/common/propfind.rs | 179 ++++++++++++-------------- crates/dav/src/principal/propfind.rs | 18 +-- crates/groupware/src/cache/mod.rs | 34 ++--- crates/http/src/management/queue.rs | 3 +- crates/smtp/src/queue/mod.rs | 8 +- tests/src/webdav/copy_move.rs | 6 +- tests/src/webdav/mod.rs | 17 ++- tests/src/webdav/principals.rs | 55 +++++--- tests/src/webdav/prop.rs | 8 +- 11 files changed, 170 insertions(+), 187 deletions(-) diff --git a/crates/common/src/config/groupware.rs b/crates/common/src/config/groupware.rs index d22127e2..de5a6b5d 100644 --- a/crates/common/src/config/groupware.rs +++ b/crates/common/src/config/groupware.rs @@ -84,7 +84,7 @@ impl GroupwareConfig { live_property_size: config.property("dav.property.max-size.live").unwrap_or(250), assisted_discovery: config .property("dav.collection.assisted-discovery") - .unwrap_or(false), + .unwrap_or(true), max_lock_timeout: config .property::("dav.lock.max-timeout") .map(|d| d.as_secs()) diff --git a/crates/dav/src/common/mod.rs b/crates/dav/src/common/mod.rs index a7075964..950dbe25 100644 --- a/crates/dav/src/common/mod.rs +++ b/crates/dav/src/common/mod.rs @@ -75,10 +75,6 @@ pub(crate) enum DavQueryResource<'x> { parent_collection: Collection, items: Vec, }, - Discovery { - parent_collection: Collection, - account_ids: Vec, - }, #[default] None, } @@ -303,29 +299,6 @@ impl<'x> DavQuery<'x> { } } - pub fn discovery( - propfind: PropFind, - account_ids: Vec, - collection: Collection, - headers: &RequestHeaders<'x>, - ) -> Self { - Self { - resource: DavQueryResource::Discovery { - parent_collection: collection, - account_ids, - }, - propfind, - depth: 0, - ret: headers.ret, - depth_no_root: headers.depth_no_root, - uri: headers.uri, - sync_type: Default::default(), - limit: Default::default(), - max_vcard_version: headers.max_vcard_version, - expand: false, - } - } - pub fn is_minimal(&self) -> bool { self.ret == Return::Minimal } diff --git a/crates/dav/src/common/propfind.rs b/crates/dav/src/common/propfind.rs index 1159551d..26301e24 100644 --- a/crates/dav/src/common/propfind.rs +++ b/crates/dav/src/common/propfind.rs @@ -135,8 +135,7 @@ impl PropFindRequestHandler for Server { Depth::Infinity => match resource.collection { Collection::Principal => true, Collection::Calendar | Collection::AddressBook - if self.core.groupware.assisted_discovery - || (resource.account_id.is_some() && resource.resource.is_some()) => + if resource.account_id.is_some() && resource.resource.is_some() => { true } @@ -215,34 +214,6 @@ impl PropFindRequestHandler for Server { } _ => unreachable!(), } - } else if (self.core.groupware.assisted_discovery - || matches!(headers.depth, Depth::Infinity)) - && matches!( - resource.collection, - Collection::Calendar | Collection::AddressBook - ) - { - // Assisted collection discovery - - // Validate permissions - access_token.assert_has_permission(match resource.collection { - Collection::Calendar => Permission::DavCalPropFind, - Collection::AddressBook => Permission::DavCardPropFind, - _ => unreachable!(), - })?; - - self.handle_dav_query( - access_token, - DavQuery::discovery( - request, - access_token - .all_ids_by_collection(resource.collection) - .collect(), - resource.collection, - headers, - ), - ) - .await } else { let mut response = MultiStatus::new(Vec::with_capacity(16)); @@ -380,7 +351,7 @@ impl PropFindRequestHandler for Server { items } - DavQueryResource::Discovery { + /*DavQueryResource::Discovery { parent_collection, account_ids, } => { @@ -411,7 +382,7 @@ impl PropFindRequestHandler for Server { account_ids, ) .await? - } + }*/ DavQueryResource::None => unreachable!(), }; response.set_namespace(collection_container.namespace()); @@ -1181,13 +1152,14 @@ async fn get( limit: usize, is_sync_limited: &mut bool, ) -> crate::Result> { - let account_id = resource.account_id; let container_has_children = collection_children != collection_container; + response.set_namespace(collection_container.namespace()); + + let account_id = resource.account_id; let resources = data .resources(server, access_token, account_id, sync_collection) .await .caused_by(trc::location!())?; - response.set_namespace(collection_container.namespace()); // Obtain document ids let mut display_containers = if !access_token.is_member(account_id) { @@ -1221,7 +1193,7 @@ async fn get( }); // Filter by changelog - match query.sync_type { + let is_sync = match query.sync_type { SyncType::From { id, seq } => { let changes = server .store() @@ -1356,15 +1328,19 @@ async fn get( if !*is_sync_limited { response.set_sync_token(resources.sync_token()); } + + true } SyncType::Initial => { response.set_sync_token(resources.sync_token()); + false } - SyncType::None => (), - } + SyncType::None => false, + }; - Ok(if let Some(resource) = resource.resource { - resources + let mut results = Vec::new(); + if let Some(resource) = resource.resource { + results = resources .subtree_with_depth(resource, query.depth) .filter(|item| { display_containers.as_ref().is_none_or(|containers| { @@ -1382,7 +1358,7 @@ async fn get( }) && (!query.depth_no_root || item.path() != resource) }) .map(|item| PropFindItem::new(resources.format_resource(item), account_id, item)) - .collect::>() + .collect::>(); } else { if !query.depth_no_root && query.sync_type.is_none_or_initial() { server @@ -1397,7 +1373,7 @@ async fn get( } if query.depth != 0 { - resources + results = resources .tree_with_depth(query.depth - 1) .filter(|item| { display_containers.as_ref().is_none_or(|containers| { @@ -1415,11 +1391,70 @@ async fn get( }) }) .map(|item| PropFindItem::new(resources.format_resource(item), account_id, item)) - .collect::>() - } else { - Vec::new() + .collect::>(); + + // Assisted discovery: + // If 'bob' has access to 'jane' and `bill` calendars, a query to '/dav/cal/bob' will return: + // - /dav/cal/bob/default + // - /dav/cal/jane/default + // - /dav/cal/bill/default + // This is invalid but it's the only workaround for clients which do not support multiple home-sets + if server.core.groupware.assisted_discovery + && !is_sync + && account_id == access_token.primary_id() + && matches!( + sync_collection, + SyncCollection::Calendar | SyncCollection::AddressBook + ) + { + for shared_account_id in access_token.all_ids_by_collection(collection_container) { + if shared_account_id == access_token.primary_id() { + continue; + } + let shared_resources = data + .resources(server, access_token, shared_account_id, sync_collection) + .await + .caused_by(trc::location!())?; + let shared_containers = + (!access_token.is_member(shared_account_id)).then(|| { + shared_resources.shared_containers( + access_token, + [if container_has_children { + Acl::ReadItems + } else { + Acl::Read + }], + true, + ) + }); + if shared_containers + .as_ref() + .is_none_or(|containers| !containers.is_empty()) + { + results.extend( + shared_resources + .tree_with_depth(query.depth - 1) + .filter(|item| { + item.is_container() + && shared_containers.as_ref().is_none_or(|containers| { + containers.contains(item.document_id()) + }) + }) + .map(|item| { + PropFindItem::new( + shared_resources.format_resource(item), + shared_account_id, + item, + ) + }), + ); + } + } + } } - }) + } + + Ok(results) } #[allow(clippy::too_many_arguments)] @@ -1512,58 +1547,6 @@ async fn multiget( Ok(paths) } -#[allow(clippy::too_many_arguments)] -async fn discover_root_paths( - server: &Server, - access_token: &AccessToken, - collection_container: Collection, - sync_collection: SyncCollection, - query: &DavQuery<'_>, - data: &mut PropFindData, - response: &mut MultiStatus, - account_ids: Vec, -) -> crate::Result> { - let mut paths = Vec::with_capacity(account_ids.len() * 2); - - for account_id in account_ids { - let resources = data - .resources(server, access_token, account_id, sync_collection) - .await - .caused_by(trc::location!())?; - server - .prepare_principal_propfind_response( - access_token, - collection_container, - [account_id].into_iter(), - &query.propfind, - response, - ) - .await?; - - // Obtain document ids - let display_containers = if !access_token.is_member(account_id) { - resources - .shared_containers(access_token, [Acl::ReadItems], true) - .into() - } else { - None - }; - paths.extend( - resources - .tree_with_depth(0) - .filter(|item| { - item.is_container() - && display_containers - .as_ref() - .is_none_or(|containers| containers.contains(item.document_id())) - }) - .map(|item| PropFindItem::new(resources.format_resource(item), account_id, item)), - ); - } - - Ok(paths) -} - impl PropFindItem { pub fn new(name: String, account_id: u32, resource: DavResourcePath<'_>) -> Self { Self { @@ -1745,7 +1728,6 @@ async fn add_base_collection_response( &access_token.name, access_token.primary_id, true, - false, ) .await .caused_by(trc::location!())?; @@ -1760,7 +1742,6 @@ async fn add_base_collection_response( &access_token.name, access_token.primary_id, false, - false, ) .await .caused_by(trc::location!())?; diff --git a/crates/dav/src/principal/propfind.rs b/crates/dav/src/principal/propfind.rs index a3bc1f52..35e729c1 100644 --- a/crates/dav/src/principal/propfind.rs +++ b/crates/dav/src/principal/propfind.rs @@ -268,16 +268,10 @@ impl PrincipalPropFind for Server { )); } PrincipalProperty::CalendarHomeSet => { - let hrefs = build_home_set( - self, - access_token, - name.as_ref(), - account_id, - true, - false, - ) - .await - .caused_by(trc::location!())?; + let hrefs = + build_home_set(self, access_token, name.as_ref(), account_id, true) + .await + .caused_by(trc::location!())?; fields.push(DavPropertyValue::new(property.clone(), hrefs)); response.set_namespace(Namespace::CalDav); @@ -289,7 +283,6 @@ impl PrincipalPropFind for Server { name.as_ref(), account_id, false, - false, ) .await .caused_by(trc::location!())?; @@ -419,7 +412,6 @@ pub(crate) async fn build_home_set( name: &str, account_id: u32, is_calendar: bool, - include_sharings: bool, ) -> trc::Result> { let (collection, resource_name) = if is_calendar { (Collection::Calendar, DavResourceName::Cal) @@ -434,7 +426,7 @@ pub(crate) async fn build_home_set( percent_encoding::utf8_percent_encode(name, RFC_3986), ))); - if include_sharings && account_id == access_token.primary_id() { + if !server.core.groupware.assisted_discovery && account_id == access_token.primary_id() { for account_id in access_token.all_ids_by_collection(collection) { if account_id != access_token.primary_id() { let other_name = server diff --git a/crates/groupware/src/cache/mod.rs b/crates/groupware/src/cache/mod.rs index 48b702e4..7cf0fc73 100644 --- a/crates/groupware/src/cache/mod.rs +++ b/crates/groupware/src/cache/mod.rs @@ -327,12 +327,16 @@ impl GroupwareCache for Server { .await?; AddressBook { name: name.clone(), - display_name: self - .core - .groupware - .default_addressbook_display_name - .as_ref() - .map(|display| format!("{display} ({account_name})")), + display_name: format!( + "{} ({})", + self.core + .groupware + .default_addressbook_display_name + .as_ref() + .unwrap_or(name), + account_name + ) + .into(), is_default: true, ..Default::default() } @@ -360,15 +364,15 @@ impl GroupwareCache for Server { name: name.clone(), preferences: vec![CalendarPreferences { account_id, - name: self - .core - .groupware - .default_calendar_display_name - .as_ref() - .map_or_else( - || name.clone(), - |display| format!("{display} ({account_name})",), - ), + name: format!( + "{} ({})", + self.core + .groupware + .default_calendar_display_name + .as_ref() + .unwrap_or(name), + account_name + ), ..Default::default() }], ..Default::default() diff --git a/crates/http/src/management/queue.rs b/crates/http/src/management/queue.rs index a05ea4a9..3abeac15 100644 --- a/crates/http/src/management/queue.rs +++ b/crates/http/src/management/queue.rs @@ -25,8 +25,7 @@ use serde::{Deserializer, Serializer}; use serde_json::json; use smtp::{ queue::{ - self, ArchivedMessage, ArchivedStatus, DisplayArchivedResponse, ErrorDetails, QueueId, - Status, spool::SmtpSpool, + self, ArchivedMessage, ArchivedStatus, ErrorDetails, QueueId, Status, spool::SmtpSpool, }, reporting::{dmarc::DmarcReporting, tls::TlsReporting}, }; diff --git a/crates/smtp/src/queue/mod.rs b/crates/smtp/src/queue/mod.rs index 00118ba6..903d9ca3 100644 --- a/crates/smtp/src/queue/mod.rs +++ b/crates/smtp/src/queue/mod.rs @@ -9,7 +9,7 @@ use common::{ expr::{self, functions::ResolveVariable, *}, }; use compact_str::ToCompactString; -use smtp_proto::{ArchivedResponse, Response}; +use smtp_proto::Response; use std::{ fmt::Display, net::{IpAddr, Ipv4Addr}, @@ -537,8 +537,7 @@ impl Display for ArchivedError { write!( f, "Unexpected response for {}: {}", - response.command, - response.response.to_string() + response.command, response.response ) } ArchivedError::DnsError(err) => { @@ -590,6 +589,8 @@ impl Display for ArchivedErrorDetails { } } +/* + pub trait DisplayArchivedResponse { fn to_string(&self) -> String; } @@ -602,3 +603,4 @@ impl DisplayArchivedResponse for ArchivedResponse { ) } } +*/ diff --git a/tests/src/webdav/copy_move.rs b/tests/src/webdav/copy_move.rs index 57a1d988..1d6bbe7c 100644 --- a/tests/src/webdav/copy_move.rs +++ b/tests/src/webdav/copy_move.rs @@ -11,7 +11,7 @@ use dav_proto::Depth; use groupware::DavResourceName; use hyper::StatusCode; -pub async fn test(test: &WebDavTest) { +pub async fn test(test: &WebDavTest, assisted_discovery: bool) { let client = test.client("jane"); let mike_noquota = test.client("mike"); @@ -33,12 +33,14 @@ pub async fn test(test: &WebDavTest) { let response = client .sync_collection(&user_base_path, "", Depth::Infinity, None, ["D:getetag"]) .await; + + // TODO: Fix tests for assisted discovery assert_eq!( response.hrefs().len(), if resource_type == DavResourceName::File { 1 } else { - 2 + 2 + usize::from(assisted_discovery) }, "{:?}", response.hrefs() diff --git a/tests/src/webdav/mod.rs b/tests/src/webdav/mod.rs index ee7b6680..dc1fb250 100644 --- a/tests/src/webdav/mod.rs +++ b/tests/src/webdav/mod.rs @@ -73,11 +73,13 @@ fn webdav_tests() { .unwrap() .block_on(async { // Prepare settings + let assisted_discovery = std::env::var("ASSISTED_DISCOVERY").unwrap_or_default() == "1"; let start_time = Instant::now(); let delete = true; let handle = init_webdav_tests( &std::env::var("STORE") .expect("Missing store type. Try running `STORE= cargo test`"), + assisted_discovery, delete, ) .await; @@ -85,12 +87,12 @@ fn webdav_tests() { basic::test(&handle).await; put_get::test(&handle).await; mkcol::test(&handle).await; - copy_move::test(&handle).await; - prop::test(&handle).await; + copy_move::test(&handle, assisted_discovery).await; + prop::test(&handle, assisted_discovery).await; multiget::test(&handle).await; sync::test(&handle).await; lock::test(&handle).await; - principals::test(&handle).await; + principals::test(&handle, assisted_discovery).await; acl::test(&handle).await; card_query::test(&handle).await; cal_query::test(&handle).await; @@ -121,13 +123,18 @@ pub struct WebDavTest { shutdown_tx: watch::Sender, } -async fn init_webdav_tests(store_id: &str, delete_if_exists: bool) -> WebDavTest { +async fn init_webdav_tests( + store_id: &str, + assisted_discovery: bool, + delete_if_exists: bool, +) -> WebDavTest { // Load and parse config let temp_dir = TempDir::new("webdav_tests", delete_if_exists); let mut config = Config::new( add_test_certs(SERVER) .replace("{STORE}", store_id) .replace("{TMP}", &temp_dir.path.display().to_string()) + .replace("{ASSISTED_DISCOVERY}", &assisted_discovery.to_string()) .replace( "{LEVEL}", &std::env::var("LOG").unwrap_or_else(|_| "disable".to_string()), @@ -1190,7 +1197,7 @@ minimum-interval = "1s" auto-add = true [dav.collection] -assisted-discovery = false +assisted-discovery = {ASSISTED_DISCOVERY} [store."auth"] type = "sqlite" diff --git a/tests/src/webdav/principals.rs b/tests/src/webdav/principals.rs index af50affb..d5071009 100644 --- a/tests/src/webdav/principals.rs +++ b/tests/src/webdav/principals.rs @@ -10,7 +10,7 @@ use dav_proto::schema::property::{DavProperty, PrincipalProperty, WebDavProperty use groupware::DavResourceName; use hyper::StatusCode; -pub async fn test(test: &WebDavTest) { +pub async fn test(test: &WebDavTest, assisted_discovery: bool) { println!("Running principals tests..."); let client = test.client("jane"); let principal_path = format!("D:href:{}/", DavResourceName::Principal.base_path()); @@ -55,7 +55,7 @@ pub async fn test(test: &WebDavTest) { .get(DavProperty::WebDav(WebDavProperty::Owner)) .with_values([path_pal.as_str()]) .with_status(StatusCode::OK); - if *account == "jane" { + if *account == "jane" && !assisted_discovery { props .get(DavProperty::Principal(PrincipalProperty::CalendarHomeSet)) .with_values([path_cal.as_str(), path_support_cal.as_str()]) @@ -207,22 +207,39 @@ pub async fn test(test: &WebDavTest) { .get(DavProperty::WebDav(WebDavProperty::CurrentUserPrincipal)) .with_values([jane_principal_path.as_str()]) .with_status(StatusCode::OK); - props - .get(DavProperty::Principal(PrincipalProperty::CalendarHomeSet)) - .with_values([ - format!("D:href:{}/jane/", DavResourceName::Cal.base_path()).as_str(), - format!("D:href:{}/support/", DavResourceName::Cal.base_path()).as_str(), - ]) - .with_status(StatusCode::OK); - props - .get(DavProperty::Principal( - PrincipalProperty::AddressbookHomeSet, - )) - .with_values([ - format!("D:href:{}/jane/", DavResourceName::Card.base_path()).as_str(), - format!("D:href:{}/support/", DavResourceName::Card.base_path()).as_str(), - ]) - .with_status(StatusCode::OK); + if assisted_discovery { + props + .get(DavProperty::Principal(PrincipalProperty::CalendarHomeSet)) + .with_values( + [format!("D:href:{}/jane/", DavResourceName::Cal.base_path()).as_str()], + ) + .with_status(StatusCode::OK); + props + .get(DavProperty::Principal( + PrincipalProperty::AddressbookHomeSet, + )) + .with_values([ + format!("D:href:{}/jane/", DavResourceName::Card.base_path()).as_str(), + ]) + .with_status(StatusCode::OK); + } else { + props + .get(DavProperty::Principal(PrincipalProperty::CalendarHomeSet)) + .with_values([ + format!("D:href:{}/jane/", DavResourceName::Cal.base_path()).as_str(), + format!("D:href:{}/support/", DavResourceName::Cal.base_path()).as_str(), + ]) + .with_status(StatusCode::OK); + props + .get(DavProperty::Principal( + PrincipalProperty::AddressbookHomeSet, + )) + .with_values([ + format!("D:href:{}/jane/", DavResourceName::Card.base_path()).as_str(), + format!("D:href:{}/support/", DavResourceName::Card.base_path()).as_str(), + ]) + .with_status(StatusCode::OK); + } for (account, _, name, _) in TEST_USERS .iter() @@ -269,7 +286,7 @@ pub async fn test(test: &WebDavTest) { .get(DavProperty::WebDav(WebDavProperty::Owner)) .with_values([path_pal.as_str()]) .with_status(StatusCode::OK); - if *account == "jane" { + if *account == "jane" && !assisted_discovery { props .get(DavProperty::Principal(PrincipalProperty::CalendarHomeSet)) .with_values([path_cal.as_str(), path_support_cal.as_str()]) diff --git a/tests/src/webdav/prop.rs b/tests/src/webdav/prop.rs index b9ec73c0..ec74e5b3 100644 --- a/tests/src/webdav/prop.rs +++ b/tests/src/webdav/prop.rs @@ -14,7 +14,7 @@ use dav_proto::schema::{ use groupware::DavResourceName; use hyper::StatusCode; -pub async fn test(test: &WebDavTest) { +pub async fn test(test: &WebDavTest, assisted_discovery: bool) { let client = test.client("jane"); for resource_type in [ @@ -91,12 +91,15 @@ pub async fn test(test: &WebDavTest) { .with_status(StatusCode::MULTI_STATUS) .with_hrefs( [ + format!("{group_base_path}/default/").as_str(), format!("{user_base_path}/default/").as_str(), format!("{user_base_path}/").as_str(), &test_base_path, ] .into_iter() .skip(if resource_type == DavResourceName::File { + 2 + } else if !assisted_discovery { 1 } else { 0 @@ -132,11 +135,14 @@ pub async fn test(test: &WebDavTest) { .with_status(StatusCode::MULTI_STATUS) .with_hrefs( [ + format!("{group_base_path}/default/").as_str(), format!("{user_base_path}/default/").as_str(), &test_base_path, ] .into_iter() .skip(if resource_type == DavResourceName::File { + 2 + } else if !assisted_discovery { 1 } else { 0