fix(webdav): native COPY honours destination filename (M8)
Threads `new_name: Option<&str>` through FileWritePort::copy_file and FileManagementUseCase::copy_file_with_perms so a same-folder COPY /a.txt → /b.txt picks up the destination name via a single COALESCE($3::text, name) in the CTE. Without it the new row inherits the source's filename and collides on the (folder_id, name, user_id) unique index — the "Already Exists" 500 M8 was hitting. handle_copy in the native WebDAV surface now passes `(file.name != dest_name).then(|| dest_name.into())`, keeping the "same name in a different folder" case at None so existing semantics are preserved.
This commit is contained in:
@@ -255,11 +255,17 @@ pub trait FileManagementUseCase: Send + Sync + 'static {
|
|||||||
) -> Result<FileDto, DomainError>;
|
) -> Result<FileDto, DomainError>;
|
||||||
|
|
||||||
/// Copies a file, enforcing that `caller_id` is the owner.
|
/// Copies a file, enforcing that `caller_id` is the owner.
|
||||||
|
///
|
||||||
|
/// `new_name`, when `Some(_)`, becomes the copy's filename — without it
|
||||||
|
/// the copy keeps the source's name, which makes "same folder, different
|
||||||
|
/// name" copies (classic WebDAV `COPY /a.txt → /b.txt`) collide on the
|
||||||
|
/// `(folder, name, user)` unique index.
|
||||||
async fn copy_file_with_perms(
|
async fn copy_file_with_perms(
|
||||||
&self,
|
&self,
|
||||||
file_id: &str,
|
file_id: &str,
|
||||||
caller_id: Uuid,
|
caller_id: Uuid,
|
||||||
target_folder_id: Option<String>,
|
target_folder_id: Option<String>,
|
||||||
|
new_name: Option<String>,
|
||||||
) -> Result<FileDto, DomainError>;
|
) -> Result<FileDto, DomainError>;
|
||||||
|
|
||||||
/// Renames a file, enforcing that `caller_id` is the owner.
|
/// Renames a file, enforcing that `caller_id` is the owner.
|
||||||
|
|||||||
@@ -314,10 +314,16 @@ pub trait FileWritePort: Send + Sync + 'static {
|
|||||||
///
|
///
|
||||||
/// With blob-dedup, this only creates a new metadata row and increments
|
/// With blob-dedup, this only creates a new metadata row and increments
|
||||||
/// the blob reference count — zero disk I/O for the content.
|
/// the blob reference count — zero disk I/O for the content.
|
||||||
|
///
|
||||||
|
/// `new_name` is honored when `Some(_)` — without it, copying a file to
|
||||||
|
/// the same folder always collides on the source's filename. WebDAV
|
||||||
|
/// COPY uses this for the "same folder, different name" case (the
|
||||||
|
/// classic `COPY /a.txt → /b.txt` pattern).
|
||||||
async fn copy_file(
|
async fn copy_file(
|
||||||
&self,
|
&self,
|
||||||
file_id: &str,
|
file_id: &str,
|
||||||
target_folder_id: Option<String>,
|
target_folder_id: Option<String>,
|
||||||
|
new_name: Option<&str>,
|
||||||
) -> Result<File, DomainError>;
|
) -> Result<File, DomainError>;
|
||||||
|
|
||||||
/// Copies an entire folder subtree atomically using ltree.
|
/// Copies an entire folder subtree atomically using ltree.
|
||||||
|
|||||||
@@ -145,7 +145,12 @@ impl BatchOperationService {
|
|||||||
|
|
||||||
async move {
|
async move {
|
||||||
let copy_result = mgmt
|
let copy_result = mgmt
|
||||||
.copy_file_with_perms(&file_id, user_id, target_folder.map(|s| s.to_string()))
|
.copy_file_with_perms(
|
||||||
|
&file_id,
|
||||||
|
user_id,
|
||||||
|
target_folder.map(|s| s.to_string()),
|
||||||
|
None,
|
||||||
|
)
|
||||||
.await;
|
.await;
|
||||||
(file_id, copy_result)
|
(file_id, copy_result)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -127,15 +127,16 @@ impl FileManagementService {
|
|||||||
&self,
|
&self,
|
||||||
file_id: &str,
|
file_id: &str,
|
||||||
target_folder_id: Option<String>,
|
target_folder_id: Option<String>,
|
||||||
|
new_name: Option<&str>,
|
||||||
) -> Result<FileDto, DomainError> {
|
) -> Result<FileDto, DomainError> {
|
||||||
info!(
|
info!(
|
||||||
"Copying file with ID: {} to folder: {:?}",
|
"Copying file with ID: {} to folder: {:?} as {:?}",
|
||||||
file_id, target_folder_id
|
file_id, target_folder_id, new_name
|
||||||
);
|
);
|
||||||
|
|
||||||
let copied_file = self
|
let copied_file = self
|
||||||
.file_repository
|
.file_repository
|
||||||
.copy_file(file_id, target_folder_id)
|
.copy_file(file_id, target_folder_id, new_name)
|
||||||
.await
|
.await
|
||||||
.map_err(|e| {
|
.map_err(|e| {
|
||||||
error!("Error copying file (ID: {}): {}", file_id, e);
|
error!("Error copying file (ID: {}): {}", file_id, e);
|
||||||
@@ -260,13 +261,15 @@ impl FileManagementUseCase for FileManagementService {
|
|||||||
file_id: &str,
|
file_id: &str,
|
||||||
caller_id: Uuid,
|
caller_id: Uuid,
|
||||||
target_folder_id: Option<String>,
|
target_folder_id: Option<String>,
|
||||||
|
new_name: Option<String>,
|
||||||
) -> Result<FileDto, DomainError> {
|
) -> Result<FileDto, DomainError> {
|
||||||
// Copy = Read on the source file + Create on the target folder.
|
// Copy = Read on the source file + Create on the target folder.
|
||||||
self.require_file_perm(file_id, Permission::Read, caller_id)
|
self.require_file_perm(file_id, Permission::Read, caller_id)
|
||||||
.await?;
|
.await?;
|
||||||
self.require_target_folder_perm(target_folder_id.as_deref(), Permission::Create, caller_id)
|
self.require_target_folder_perm(target_folder_id.as_deref(), Permission::Create, caller_id)
|
||||||
.await?;
|
.await?;
|
||||||
self.copy_file(file_id, target_folder_id).await
|
self.copy_file(file_id, target_folder_id, new_name.as_deref())
|
||||||
|
.await
|
||||||
}
|
}
|
||||||
|
|
||||||
async fn rename_file_with_perms(
|
async fn rename_file_with_perms(
|
||||||
|
|||||||
@@ -228,6 +228,7 @@ impl FileWritePort for MockFileWritePort {
|
|||||||
&self,
|
&self,
|
||||||
_file_id: &str,
|
_file_id: &str,
|
||||||
_target_folder_id: Option<String>,
|
_target_folder_id: Option<String>,
|
||||||
|
_new_name: Option<&str>,
|
||||||
) -> Result<File, DomainError> {
|
) -> Result<File, DomainError> {
|
||||||
unimplemented!()
|
unimplemented!()
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -609,6 +609,7 @@ impl FileWritePort for MockFileRepository {
|
|||||||
&self,
|
&self,
|
||||||
_file_id: &str,
|
_file_id: &str,
|
||||||
_target_folder_id: Option<String>,
|
_target_folder_id: Option<String>,
|
||||||
|
_new_name: Option<&str>,
|
||||||
) -> std::result::Result<File, DomainError> {
|
) -> std::result::Result<File, DomainError> {
|
||||||
unimplemented!()
|
unimplemented!()
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -173,6 +173,7 @@ impl FileWritePort for StubFileWritePort {
|
|||||||
&self,
|
&self,
|
||||||
_file_id: &str,
|
_file_id: &str,
|
||||||
_target_folder_id: Option<String>,
|
_target_folder_id: Option<String>,
|
||||||
|
_new_name: Option<&str>,
|
||||||
) -> Result<File, DomainError> {
|
) -> Result<File, DomainError> {
|
||||||
Ok(File::default())
|
Ok(File::default())
|
||||||
}
|
}
|
||||||
@@ -635,6 +636,7 @@ impl FileManagementUseCase for StubFileManagementUseCase {
|
|||||||
_file_id: &str,
|
_file_id: &str,
|
||||||
_caller_id: Uuid,
|
_caller_id: Uuid,
|
||||||
_folder_id: Option<String>,
|
_folder_id: Option<String>,
|
||||||
|
_new_name: Option<String>,
|
||||||
) -> Result<FileDto, DomainError> {
|
) -> Result<FileDto, DomainError> {
|
||||||
Ok(FileDto::default())
|
Ok(FileDto::default())
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -397,10 +397,12 @@ impl FileWritePort for FileBlobWriteRepository {
|
|||||||
&self,
|
&self,
|
||||||
file_id: &str,
|
file_id: &str,
|
||||||
target_folder_id: Option<String>,
|
target_folder_id: Option<String>,
|
||||||
|
new_name: Option<&str>,
|
||||||
) -> Result<File, DomainError> {
|
) -> Result<File, DomainError> {
|
||||||
// Atomic CTE: read source file → insert new row with same blob_hash → increment ref_count.
|
// Atomic CTE: read source file → insert new row with same blob_hash → increment ref_count.
|
||||||
// Single round-trip; blob content is NOT copied (dedup makes this zero-copy).
|
// Single round-trip; blob content is NOT copied (dedup makes this zero-copy).
|
||||||
let target_fid = target_folder_id.clone();
|
let target_fid = target_folder_id.clone();
|
||||||
|
let rename_to = new_name.map(|s| s.to_string());
|
||||||
|
|
||||||
let row = retry_on_deadlock("files.copy", || {
|
let row = retry_on_deadlock("files.copy", || {
|
||||||
sqlx::query_as::<
|
sqlx::query_as::<
|
||||||
@@ -424,7 +426,7 @@ impl FileWritePort for FileBlobWriteRepository {
|
|||||||
),
|
),
|
||||||
new_file AS (
|
new_file AS (
|
||||||
INSERT INTO storage.files (name, folder_id, user_id, blob_hash, size, mime_type, category_order)
|
INSERT INTO storage.files (name, folder_id, user_id, blob_hash, size, mime_type, category_order)
|
||||||
SELECT name,
|
SELECT COALESCE($3::text, name),
|
||||||
COALESCE($2::uuid, folder_id),
|
COALESCE($2::uuid, folder_id),
|
||||||
user_id,
|
user_id,
|
||||||
blob_hash,
|
blob_hash,
|
||||||
@@ -442,6 +444,7 @@ impl FileWritePort for FileBlobWriteRepository {
|
|||||||
)
|
)
|
||||||
.bind(file_id)
|
.bind(file_id)
|
||||||
.bind(&target_fid)
|
.bind(&target_fid)
|
||||||
|
.bind(&rename_to)
|
||||||
.fetch_optional(self.pool.as_ref())
|
.fetch_optional(self.pool.as_ref())
|
||||||
})
|
})
|
||||||
.await
|
.await
|
||||||
|
|||||||
@@ -1365,6 +1365,17 @@ async fn handle_move(
|
|||||||
// SECURITY: reject path-traversal in destination
|
// SECURITY: reject path-traversal in destination
|
||||||
reject_path_traversal(&destination_path)?;
|
reject_path_traversal(&destination_path)?;
|
||||||
|
|
||||||
|
// Normalize destination through the SAME path-prefixing that
|
||||||
|
// `resolve_webdav_path` applied to `source_path` during dispatch.
|
||||||
|
// Without this, comparing source_parent_path (already prefixed with
|
||||||
|
// the user's home folder name) against dest_parent_path (raw from
|
||||||
|
// the URL, no prefix) always reports "different parent" — even for a
|
||||||
|
// pure rename at the same level — and breaks the move/rename branch
|
||||||
|
// selection below.
|
||||||
|
let destination_path = resolve_webdav_path(&state, user.id, &destination_path)
|
||||||
|
.await
|
||||||
|
.unwrap_or(destination_path);
|
||||||
|
|
||||||
// Destination lock guard: MOVE also creates/replaces a resource at
|
// Destination lock guard: MOVE also creates/replaces a resource at
|
||||||
// the destination. If that path is locked, the same If: header must
|
// the destination. If that path is locked, the same If: header must
|
||||||
// satisfy it.
|
// satisfy it.
|
||||||
@@ -1462,17 +1473,33 @@ async fn handle_move(
|
|||||||
}
|
}
|
||||||
ResolvedResource::File(file) => {
|
ResolvedResource::File(file) => {
|
||||||
if source_parent_path != dest_parent_path {
|
if source_parent_path != dest_parent_path {
|
||||||
if !dest_parent_path.is_empty()
|
// Resolve the destination's parent PATH into a folder ID
|
||||||
&& let Ok(parent) = folder_service.get_folder_by_path(dest_parent_path).await
|
// before handing it to move_file_with_perms (which takes
|
||||||
{
|
// an Option<folder_id String>, not a path). Previously
|
||||||
|
// the path was passed straight through and the move
|
||||||
|
// would silently fail because no row matches a folder
|
||||||
|
// whose id literally equals the path text.
|
||||||
|
let target_parent_id = if dest_parent_path.is_empty() {
|
||||||
|
None
|
||||||
|
} else {
|
||||||
|
let parent = folder_service
|
||||||
|
.get_folder_by_path(dest_parent_path)
|
||||||
|
.await
|
||||||
|
.map_err(|_| {
|
||||||
|
AppError::not_found(format!(
|
||||||
|
"Destination parent not found: {}",
|
||||||
|
dest_parent_path
|
||||||
|
))
|
||||||
|
})?;
|
||||||
assert_owner(
|
assert_owner(
|
||||||
parent.owner_id.as_deref(),
|
parent.owner_id.as_deref(),
|
||||||
&user.id.to_string(),
|
&user.id.to_string(),
|
||||||
dest_parent_path,
|
dest_parent_path,
|
||||||
)?;
|
)?;
|
||||||
}
|
Some(parent.id)
|
||||||
|
};
|
||||||
file_management_service
|
file_management_service
|
||||||
.move_file_with_perms(&file.id, user.id, Some(dest_parent_path.to_string()))
|
.move_file_with_perms(&file.id, user.id, target_parent_id)
|
||||||
.await
|
.await
|
||||||
.map_err(AppError::from)?;
|
.map_err(AppError::from)?;
|
||||||
}
|
}
|
||||||
@@ -1547,6 +1574,13 @@ async fn handle_copy(
|
|||||||
// SECURITY: reject path-traversal in destination
|
// SECURITY: reject path-traversal in destination
|
||||||
reject_path_traversal(&destination_path)?;
|
reject_path_traversal(&destination_path)?;
|
||||||
|
|
||||||
|
// Normalize through the same path-prefixing the dispatcher applied
|
||||||
|
// to source_path. See the long comment in handle_move for why this
|
||||||
|
// matters — same root-cause class of asymmetric-path bugs.
|
||||||
|
let destination_path = resolve_webdav_path(&state, user.id, &destination_path)
|
||||||
|
.await
|
||||||
|
.unwrap_or(destination_path);
|
||||||
|
|
||||||
// Active-lock guard on the destination (RFC 4918 §9.10.4).
|
// Active-lock guard on the destination (RFC 4918 §9.10.4).
|
||||||
if let Some(resp) = enforce_native_lock(
|
if let Some(resp) = enforce_native_lock(
|
||||||
&state.webdav_lock_store,
|
&state.webdav_lock_store,
|
||||||
@@ -1654,24 +1688,20 @@ async fn handle_copy(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
ResolvedResource::File(file) => {
|
ResolvedResource::File(file) => {
|
||||||
// M8b fix: copy_file_with_perms takes (file_id, caller, target_folder)
|
// M8b fix: copy_file_with_perms now accepts an optional new
|
||||||
// but no rename — so a copy to a different name in the SAME folder
|
// filename — without it, a copy to the same folder with a
|
||||||
// (typical for root-level "duplicate" pattern) collided with the
|
// different name collided with the source on the
|
||||||
// source filename and 500'd. Mirror MOVE: copy first, then rename
|
// (folder, name, user) unique index. Pass dest_name when it
|
||||||
// if the dest filename differs from the source's name.
|
// differs from the source so the INSERT lands with the
|
||||||
|
// intended name in a single round-trip; pass None for the
|
||||||
|
// "same name in a different folder" case to keep the existing
|
||||||
|
// semantics.
|
||||||
let file_management_service = &state.applications.file_management_service;
|
let file_management_service = &state.applications.file_management_service;
|
||||||
let copied = file_management_service
|
let copy_name = (file.name != dest_name).then(|| dest_name.to_string());
|
||||||
.copy_file_with_perms(&file.id, user.id, target_parent_id)
|
file_management_service
|
||||||
|
.copy_file_with_perms(&file.id, user.id, target_parent_id, copy_name)
|
||||||
.await
|
.await
|
||||||
.map_err(|e| AppError::internal_error(format!("Failed to copy file: {}", e)))?;
|
.map_err(|e| AppError::internal_error(format!("Failed to copy file: {}", e)))?;
|
||||||
if file.name != dest_name {
|
|
||||||
file_management_service
|
|
||||||
.rename_file_with_perms(&copied.id, user.id, dest_name)
|
|
||||||
.await
|
|
||||||
.map_err(|e| {
|
|
||||||
AppError::internal_error(format!("Failed to rename copied file: {}", e))
|
|
||||||
})?;
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user