refactor(s3_key): owner segment is a principal string; consistent encoding

.github/workspace-dep-closures.json

@@ -3804,9 +3804,13 @@
       "services/docx_unzip_handler"
     ],
     "dynamodb_client": [
+      "crates/bot_id",
+      "crates/cowlike",
       "crates/dynamodb_client",
       "crates/macro_aws_config",
       "crates/macro_env_var",
+      "crates/macro_user_id",
+      "crates/model_owner",
       "crates/models_bulk_upload",
       "crates/s3_key",
       "crates/workspace-hack"
@@ -7766,6 +7770,10 @@
       "crates/workspace-hack"
     ],
     "s3_key": [
+      "crates/bot_id",
+      "crates/cowlike",
+      "crates/macro_user_id",
+      "crates/model_owner",
       "crates/s3_key",
       "crates/workspace-hack"
     ],

crates/documents/Cargo.toml

@@ -151,7 +151,6 @@ hex = { workspace = true, optional = true }
 sqlx = { workspace = true, optional = true }
 sync_service_client = { path = "../sync_service_client" }
 task_dedup = { path = "../task_dedup", optional = true }
-urlencoding = { workspace = true }
 
 # AI Tool dependencies
 ai_toolset = { path = "../ai_toolset", optional = true }

crates/documents/src/domain/markdown_backfill.rs

@@ -4,6 +4,7 @@ use std::{future::Future, time::Duration};
 
 use anyhow::Context;
 use futures::stream::{self, StreamExt};
+use model_owner::Owner;
 use tokio::time::timeout;
 use tokio_retry::Retry;
 
@@ -36,8 +37,8 @@ pub struct MarkdownBackfillOptions {
 pub struct MarkdownBackfillCandidate {
     /// Document id.
     pub id: String,
-    /// Document owner user id.
-    pub owner: String,
+    /// Document owner.
+    pub owner: Owner,
     /// Latest document instance id containing object-storage markdown bytes.
     pub document_instance_id: Option<i64>,
     /// Legacy uploaded flag.

crates/documents/src/domain/service.rs

@@ -44,7 +44,7 @@ use model::response::PresignedUrl;
 use model_owner::Owner;
 use s3_key::{
     build_cloud_storage_bucket_document_key, build_docx_staging_bucket_document_key,
-    build_docx_to_pdf_converted_document_key,
+    build_docx_to_pdf_converted_document_key, document_key_url_path,
 };
 use tracing;
 
@@ -389,8 +389,18 @@ impl<
         }
     }
 
+    /// The CloudFront URL an object key is served from. The key is
+    /// percent-encoded per path segment here, not where it is built.
+    fn cloudfront_url_for_key(&self, key: &str) -> String {
+        format!(
+            "{}/{}",
+            self.cloudfront_config.distribution_url,
+            document_key_url_path(key)
+        )
+    }
+
     fn make_presigned_url(&self, key: &str) -> anyhow::Result<String> {
-        let constructed_url = format!("{}/{}", self.cloudfront_config.distribution_url, key);
+        let constructed_url = self.cloudfront_url_for_key(key);
         let options = self.get_signed_options();
 
         let signed_url = if !macro_aws_config::is_local_aws() {
@@ -404,12 +414,11 @@ impl<
 
     async fn get_editable_url(
         &self,
-        owner: &str,
+        owner: &Owner,
         document_id: &str,
         document_version_id: Option<i64>,
         _file_type: &str,
     ) -> anyhow::Result<LocationResponseData> {
-        let url_encoded_owner = urlencoding::encode(owner);
         let document_version_id = if let Some(id) = document_version_id {
             id
         } else {
@@ -420,47 +429,38 @@ impl<
                 .0
         };
 
-        let document_key = build_cloud_storage_bucket_document_key(
-            &url_encoded_owner,
-            document_id,
-            document_version_id,
-        );
+        let document_key =
+            build_cloud_storage_bucket_document_key(owner, document_id, document_version_id);
 
         let signed_url = self.make_presigned_url(&document_key)?;
         Ok(LocationResponseData::PresignedUrl(signed_url))
     }
 
     async fn get_static_url(
         &self,
-        owner: &str,
+        owner: &Owner,
         document_id: &str,
         _file_type: &Option<FileType>,
     ) -> anyhow::Result<LocationResponseData> {
-        let url_encoded_owner = urlencoding::encode(owner);
         let (document_version_id, _) = self
             .repo
             .get_document_version_id(document_id)
             .await
             .map_err(Into::into)?;
 
-        let document_key = build_cloud_storage_bucket_document_key(
-            &url_encoded_owner,
-            document_id,
-            document_version_id,
-        );
+        let document_key =
+            build_cloud_storage_bucket_document_key(owner, document_id, document_version_id);
 
         let signed_url = self.make_presigned_url(&document_key)?;
         Ok(LocationResponseData::PresignedUrl(signed_url))
     }
 
     async fn get_converted_docx_url(
         &self,
-        owner: &str,
+        owner: &Owner,
         document_id: &str,
     ) -> anyhow::Result<LocationResponseData> {
-        let url_encoded_owner = urlencoding::encode(owner);
-        let document_key =
-            build_docx_to_pdf_converted_document_key(&url_encoded_owner, document_id);
+        let document_key = build_docx_to_pdf_converted_document_key(owner, document_id);
 
         let signed_url = self.make_presigned_url(&document_key)?;
         Ok(LocationResponseData::PresignedUrl(signed_url))
@@ -512,7 +512,7 @@ impl<
 
     async fn get_presigned_url_by_type(
         &self,
-        owner: &str,
+        owner: &Owner,
         document_id: &str,
         file_type: Option<FileType>,
         document_version_id: Option<i64>,
@@ -730,9 +730,8 @@ impl<
                 Ok(None)
             }
             Some(FileType::Docx) => {
-                let owner = document_metadata.owner.principal_id();
                 let docx_key = build_docx_staging_bucket_document_key(
-                    &owner,
+                    &document_metadata.owner,
                     &document_id,
                     document_metadata.document_version_id,
                 );
@@ -742,9 +741,8 @@ impl<
                     .map(Some)
             }
             _ => {
-                let owner = document_metadata.owner.principal_id();
                 let key = build_cloud_storage_bucket_document_key(
-                    &owner,
+                    &document_metadata.owner,
                     &document_id,
                     document_metadata.document_version_id,
                 );
@@ -1017,11 +1015,10 @@ impl<
             return Ok(response);
         }
 
-        let owner = document_context.owner.principal_id();
         let get_converted_docx_url = params.get_converted_docx_url.unwrap_or(false);
         let response_data = self
             .get_presigned_url_by_type(
-                &owner,
+                &document_context.owner,
                 &document_id,
                 file_type,
                 params.document_version_id,
@@ -1683,19 +1680,18 @@ impl<
             .map_err(|e| DocumentError::Internal(e.into()))?;
 
         let new_document_id = new_metadata.document_id.clone();
+        let new_owner = Owner::User(user_id.clone());
 
         // File-type-specific S3 operations
         let copy_result = match file_type {
             Some(FileType::Docx) => {
                 // Copy the converted PDF version
-                let owner = original_metadata.owner.principal_id();
-                let url_encoded_owner = urlencoding::encode(&owner);
                 let source_key = build_docx_to_pdf_converted_document_key(
-                    &url_encoded_owner,
+                    &original_metadata.owner,
                     &original_metadata.document_id,
                 );
                 let dest_key =
-                    build_docx_to_pdf_converted_document_key(user_id.as_ref(), &new_document_id);
+                    build_docx_to_pdf_converted_document_key(&new_owner, &new_document_id);
                 self.upload_url_service
                     .copy_object(&source_key, &dest_key)
                     .await
@@ -1727,12 +1723,12 @@ impl<
                         .0;
 
                     let source_key = build_cloud_storage_bucket_document_key(
-                        &original_metadata.owner.principal_id(),
+                        &original_metadata.owner,
                         &original_metadata.document_id,
                         source_version_id,
                     );
                     let dest_key = build_cloud_storage_bucket_document_key(
-                        user_id.as_ref(),
+                        &new_owner,
                         &new_document_id,
                         new_metadata.document_version_id,
                     );
@@ -1776,12 +1772,12 @@ impl<
                 };
 
                 let source_key = build_cloud_storage_bucket_document_key(
-                    &original_metadata.owner.principal_id(),
+                    &original_metadata.owner,
                     &original_metadata.document_id,
                     source_version_id,
                 );
                 let dest_key = build_cloud_storage_bucket_document_key(
-                    user_id.as_ref(),
+                    &new_owner,
                     &new_document_id,
                     new_metadata.document_version_id,
                 );

crates/documents/src/domain/service/tests.rs

@@ -2892,3 +2892,45 @@ fn spreadsheet_uploads_are_rejected_instead_of_discarding_their_bytes() {
     );
     assert!(validate_spreadsheet_creation(Some(FileType::Csv), "uploaded-csv-sha").is_ok());
 }
+
+#[test]
+fn presigned_url_path_percent_encodes_the_owner_segment_of_the_object_key() {
+    let service = make_test_service(make_mock_repo());
+    let owner = Owner::from_principal_str("macro|owner@user.com").unwrap();
+    let key = build_cloud_storage_bucket_document_key(&owner, "doc-1", 7);
+
+    // The object key stays the raw principal; only the URL path is encoded.
+    assert_eq!(key, "macro|owner@user.com/doc-1/7");
+    assert_eq!(
+        service.cloudfront_url_for_key(&key),
+        "https://cdn.example.test/macro%7Cowner%40user.com/doc-1/7"
+    );
+}
+
+#[test]
+fn presigned_url_path_for_a_converted_docx_matches_the_legacy_encoding() {
+    let service = make_test_service(make_mock_repo());
+    let owner = Owner::from_principal_str("macro|owner@user.com").unwrap();
+    let key = build_docx_to_pdf_converted_document_key(&owner, "doc-1");
+
+    assert_eq!(
+        service.cloudfront_url_for_key(&key),
+        "https://cdn.example.test/macro%7Cowner%40user.com/doc-1/converted.pdf"
+    );
+}
+
+#[test]
+fn presigned_url_path_for_bot_and_team_owners_uses_their_principals() {
+    let service = make_test_service(make_mock_repo());
+    let bot = Owner::from_principal_str("bot|00000000-0000-0000-0000-00000000a1a1").unwrap();
+    let team = Owner::from_principal_str("00000000-0000-0000-0000-000000000456").unwrap();
+
+    assert_eq!(
+        service.cloudfront_url_for_key(&build_cloud_storage_bucket_document_key(&bot, "doc-1", 7)),
+        "https://cdn.example.test/bot%7C00000000-0000-0000-0000-00000000a1a1/doc-1/7"
+    );
+    assert_eq!(
+        service.cloudfront_url_for_key(&build_cloud_storage_bucket_document_key(&team, "doc-1", 7)),
+        "https://cdn.example.test/00000000-0000-0000-0000-000000000456/doc-1/7"
+    );
+}

crates/documents/src/outbound/pg_document_repo/markdown_backfill.rs

@@ -1,5 +1,8 @@
 //! PostgreSQL adapter for markdown lifecycle backfill.
 
+use anyhow::Context as _;
+use model_owner::Owner;
+
 use crate::domain::markdown_backfill::{MarkdownBackfillCandidate, MarkdownBackfillRepo};
 use crate::outbound::pg_document_repo::PgDocumentRepo;
 
@@ -44,21 +47,22 @@ impl MarkdownBackfillRepo for PgDocumentRepo {
             .fetch_all(&self.pool)
             .await?;
 
-        Ok(rows
-            .into_iter()
+        rows.into_iter()
             .map(
                 |(id, owner, document_instance_id, uploaded, content_state, content_location)| {
-                    MarkdownBackfillCandidate {
+                    let owner = Owner::from_principal_str(&owner)
+                        .with_context(|| format!("document {id} has an invalid owner"))?;
+                    Ok(MarkdownBackfillCandidate {
                         id,
                         owner,
                         document_instance_id,
                         uploaded,
                         content_state,
                         content_location,
-                    }
+                    })
                 },
             )
-            .collect())
+            .collect()
     }
 
     #[tracing::instrument(err, skip(self, candidates), fields(count = candidates.len()))]

crates/documents/src/outbound/s3_upload_url.rs

@@ -6,7 +6,7 @@ use anyhow::Context;
 use aws_sdk_s3::{presigning::PresigningConfig, primitives::ByteStream};
 use base64::Engine;
 use model::document::ContentType;
-use s3_key::SYNC_SERVICE_SNAPSHOT_PREFIX;
+use s3_key::{SYNC_SERVICE_SNAPSHOT_PREFIX, document_key_url_path};
 
 use crate::domain::ports::PresignedUploadUrlPort;
 
@@ -80,7 +80,12 @@ impl PresignedUploadUrlPort for S3UploadUrlAdapter {
         self.client
             .copy_object()
             .bucket(&self.document_storage_bucket)
-            .copy_source(format!("{}/{}", self.document_storage_bucket, source_key))
+            // S3 requires the copy source to be URL-encoded.
+            .copy_source(format!(
+                "{}/{}",
+                self.document_storage_bucket,
+                document_key_url_path(source_key)
+            ))
             .key(destination_key)
             .send()
             .await?;

crates/projects/src/domain/upload.rs

@@ -50,9 +50,8 @@ pub async fn build_destination_map<U: ProjectUploadUrlPort>(
                 continue;
             }
 
-            let owner = document.owner.principal_id();
             let key = build_docx_staging_bucket_document_key(
-                &owner,
+                &document.owner,
                 &document.document_id,
                 document.document_version_id,
             );
@@ -66,9 +65,8 @@ pub async fn build_destination_map<U: ProjectUploadUrlPort>(
             continue;
         }
 
-        let owner = document.owner.principal_id();
         let key = build_cloud_storage_bucket_document_key(
-            &owner,
+            &document.owner,
             &document.document_id,
             document.document_version_id,
         );