diff --git a/.github/workspace-dep-closures.json b/.github/workspace-dep-closures.json index 1e7a33773ac..f8b473f982d 100644 --- a/.github/workspace-dep-closures.json +++ b/.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" ], diff --git a/Cargo.lock b/Cargo.lock index 5161bd3b27d..87d56725c8f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5789,7 +5789,6 @@ dependencies = [ "tracing", "tracing-subscriber", "unicode-segmentation", - "urlencoding", "utoipa", "uuid", "workspace-hack", @@ -5818,6 +5817,7 @@ dependencies = [ "macro_sha_count_client", "macro_uuid", "model", + "model_owner", "redis", "s3_client", "s3_key", @@ -14692,6 +14692,7 @@ name = "s3_key" version = "0.1.0" dependencies = [ "anyhow", + "model_owner", "urlencoding", "workspace-hack", ] diff --git a/crates/documents/Cargo.toml b/crates/documents/Cargo.toml index 3452eb215e0..fc4831dde01 100644 --- a/crates/documents/Cargo.toml +++ b/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 } diff --git a/crates/documents/src/domain/markdown_backfill.rs b/crates/documents/src/domain/markdown_backfill.rs index 1abb06e5495..ff2c4e9aeba 100644 --- a/crates/documents/src/domain/markdown_backfill.rs +++ b/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, /// Legacy uploaded flag. diff --git a/crates/documents/src/domain/service.rs b/crates/documents/src/domain/service.rs index d096d9769d8..9b8c48e255f 100644 --- a/crates/documents/src/domain/service.rs +++ b/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 { - 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, _file_type: &str, ) -> anyhow::Result { - let url_encoded_owner = urlencoding::encode(owner); let document_version_id = if let Some(id) = document_version_id { id } else { @@ -420,11 +429,8 @@ 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)) @@ -432,22 +438,18 @@ impl< async fn get_static_url( &self, - owner: &str, + owner: &Owner, document_id: &str, _file_type: &Option, ) -> anyhow::Result { - 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)) @@ -455,12 +457,10 @@ impl< async fn get_converted_docx_url( &self, - owner: &str, + owner: &Owner, document_id: &str, ) -> anyhow::Result { - 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, document_version_id: Option, @@ -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, ); diff --git a/crates/documents/src/domain/service/tests.rs b/crates/documents/src/domain/service/tests.rs index 2c0bae2ff68..2b4dbf36dd1 100644 --- a/crates/documents/src/domain/service/tests.rs +++ b/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" + ); +} diff --git a/crates/documents/src/outbound/pg_document_repo/markdown_backfill.rs b/crates/documents/src/outbound/pg_document_repo/markdown_backfill.rs index f19846f3494..aeba23243b8 100644 --- a/crates/documents/src/outbound/pg_document_repo/markdown_backfill.rs +++ b/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()))] diff --git a/crates/documents/src/outbound/s3_upload_url.rs b/crates/documents/src/outbound/s3_upload_url.rs index de603f12c56..9d4c0d4468f 100644 --- a/crates/documents/src/outbound/s3_upload_url.rs +++ b/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?; diff --git a/crates/projects/src/domain/upload.rs b/crates/projects/src/domain/upload.rs index a870778aaf8..be83b70d2f1 100644 --- a/crates/projects/src/domain/upload.rs +++ b/crates/projects/src/domain/upload.rs @@ -50,9 +50,8 @@ pub async fn build_destination_map( 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( 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, ); diff --git a/crates/s3_key/Cargo.toml b/crates/s3_key/Cargo.toml index 3253f45cbaa..5fbc33279e9 100644 --- a/crates/s3_key/Cargo.toml +++ b/crates/s3_key/Cargo.toml @@ -6,5 +6,6 @@ publish = false [dependencies] anyhow = { workspace = true } +model_owner = { path = "../model_owner" } urlencoding = { workspace = true } workspace-hack = { version = "0.1", path = "../workspace-hack" } diff --git a/crates/s3_key/src/document_key.rs b/crates/s3_key/src/document_key.rs index 0efc2e42d2b..a9737464530 100644 --- a/crates/s3_key/src/document_key.rs +++ b/crates/s3_key/src/document_key.rs @@ -1,4 +1,5 @@ use anyhow::Context; +use model_owner::Owner; /// The file name used for converted DOCX-to-PDF documents. pub const CONVERTED_DOCUMENT_FILE_NAME: &str = "converted"; @@ -19,26 +20,29 @@ pub const DOCX_EXTENSION: &str = "docx"; /// Represents an S3 key in the document storage bucket. /// /// Covers all known key shapes: -/// - `Versioned`: `{user_id}/{document_id}/{version_id}` — a specific document version -/// - `ConvertedPdf`: `{user_id}/{document_id}/converted.pdf` — a DOCX converted to PDF +/// - `Versioned`: `{owner}/{document_id}/{version_id}` — a specific document version +/// - `ConvertedPdf`: `{owner}/{document_id}/converted.pdf` — a DOCX converted to PDF /// - `TempDocx`: `temp_files/{document_id}.docx` — a temporary DOCX export /// - `SyncServiceSnapshot`: `sync_service_snapshot/{document_id}` — a cached CRDT snapshot /// - `BomPart`: `{sha}` — a content-addressable BOM part from DOCX uploads +/// +/// `{owner}` is the owner segment: the owning principal's string as +/// [`owner_segment`] builds it. #[derive(Eq, PartialEq, Debug, Clone)] pub enum DocumentKey { - /// A versioned document: `{user_id}/{document_id}/{version_id}` + /// A versioned document: `{owner}/{document_id}/{version_id}` Versioned { - /// The owner's user ID. - user_id: String, + /// The owner segment, an owner principal string. + owner_segment: String, /// The document ID. document_id: String, /// The document version ID (document_instance_id or document_bom_id). version_id: i64, }, - /// A DOCX file converted to PDF: `{user_id}/{document_id}/converted.pdf` + /// A DOCX file converted to PDF: `{owner}/{document_id}/converted.pdf` ConvertedPdf { - /// The owner's user ID. - user_id: String, + /// The owner segment, an owner principal string. + owner_segment: String, /// The document ID. document_id: String, }, @@ -65,8 +69,39 @@ fn is_sha256_hex(s: &str) -> bool { s.len() == SHA256_HEX_LEN && s.bytes().all(|b| b.is_ascii_hexdigit()) } +/// Builds the owner segment of a document key. +/// +/// This is the one place that decides how an owner is spelled inside an object +/// key. The segment is the owner's principal string verbatim: `macro|` +/// for a user, `bot|` for a bot, and a hyphenated UUID for a team. +/// Nothing is percent-encoded: every object written so far sits under the raw +/// user principal and must stay addressable, and bot and team principals use a +/// subset of the characters a user principal already uses. +/// +/// A key is not a URL. Code that places a key in a URL path encodes the whole +/// key with [`document_key_url_path`] at that point rather than encoding the +/// owner here. +fn owner_segment(owner: &Owner) -> String { + owner.principal_id() +} + +/// Reads the owner segment of a key back into an owner principal string. +/// +/// Exact inverse of [`owner_segment`]: the segment is taken verbatim, so +/// [`DocumentKey::to_key`] reproduces the parsed key byte for byte. Nothing is +/// percent-decoded here; `%` is a legal character in a user principal's email, +/// so decoding would turn one owner into another. A listener that receives +/// form-encoded keys (classic S3 event notifications, unlike EventBridge) +/// decodes the whole key at its inbound boundary before parsing. +fn parse_owner_segment(segment: &str) -> String { + segment.to_string() +} + impl DocumentKey { /// Parses an S3 key from the document storage bucket into a `DocumentKey`. + /// + /// The key must be the object key as stored, not a form-encoded copy from + /// an event notification; see [`parse_owner_segment`]. pub fn from_s3_key(key: &str) -> Result { let split: Vec<&str> = key.split('/').collect(); @@ -85,7 +120,7 @@ impl DocumentKey { document_id: split[1].to_string(), }), 3 => { - let user_id = urlencoding::decode(split[0]).context("UTF-8")?.into_owned(); + let owner_segment = parse_owner_segment(split[0]); let document_id = split[1].to_string(); let tail = split[2]; @@ -93,7 +128,7 @@ impl DocumentKey { format!("{CONVERTED_DOCUMENT_FILE_NAME}.{PDF_EXTENSION}"); if tail == converted_pdf_suffix { Ok(Self::ConvertedPdf { - user_id, + owner_segment, document_id, }) } else { @@ -101,7 +136,7 @@ impl DocumentKey { "invalid version id: expected integer, got '{tail}'" ))?; Ok(Self::Versioned { - user_id, + owner_segment, document_id, version_id, }) @@ -127,6 +162,16 @@ impl DocumentKey { } } + /// Returns the owner segment for the key shapes that carry one. + pub fn owner_segment(&self) -> Option<&str> { + match self { + Self::Versioned { owner_segment, .. } | Self::ConvertedPdf { owner_segment, .. } => { + Some(owner_segment) + } + Self::TempDocx { .. } | Self::SyncServiceSnapshot { .. } | Self::BomPart { .. } => None, + } + } + /// Returns `true` if this is a versioned document key. This is the default. pub fn is_versioned(&self) -> bool { matches!(self, Self::Versioned { .. }) @@ -169,14 +214,19 @@ impl DocumentKey { pub fn to_key(&self) -> String { match self { Self::Versioned { - user_id, + owner_segment, document_id, version_id, - } => build_cloud_storage_bucket_document_key(user_id, document_id, version_id), + } => build_document_key_from_segment(owner_segment, document_id, version_id, None), Self::ConvertedPdf { - user_id, + owner_segment, + document_id, + } => build_document_key_from_segment( + owner_segment, document_id, - } => build_docx_to_pdf_converted_document_key(user_id, document_id), + CONVERTED_DOCUMENT_FILE_NAME, + Some(PDF_EXTENSION), + ), Self::TempDocx { document_id } => build_temp_docx_key(document_id), Self::SyncServiceSnapshot { document_id } => { format!("{SYNC_SERVICE_SNAPSHOT_PREFIX}/{document_id}") @@ -186,48 +236,57 @@ impl DocumentKey { } } -fn build_cloud_storage_bucket_document_key_helper( - user_id: &str, +/// Joins an already-built owner segment with the rest of a document key. +/// +/// Every `{owner}/{document_id}/...` key goes through here so the layout lives +/// in one place; only [`owner_segment`] and [`DocumentKey::to_key`] hand it a +/// segment. +fn build_document_key_from_segment( + owner_segment: &str, document_id: &str, document_version_id: T, file_type: Option<&str>, ) -> String { + let prefix = build_document_prefix_from_segment(owner_segment, document_id); match file_type { Some(file_type) => { - format!( - "{}/{}/{}.{}", - user_id, - document_id, - document_version_id.to_string(), - file_type - ) - } - None => { - format!( - "{}/{}/{}", - user_id, - document_id, - document_version_id.to_string() - ) + format!("{prefix}/{}.{file_type}", document_version_id.to_string()) } + None => format!("{prefix}/{}", document_version_id.to_string()), } } +fn build_document_prefix_from_segment(owner_segment: &str, document_id: &str) -> String { + format!("{owner_segment}/{document_id}") +} + /// Builds a document key for a document in the cloud storage bucket. -/// The format is `{user_id}/{document_id}/{document_version_id}`. +/// The format is `{owner}/{document_id}/{document_version_id}`. pub fn build_cloud_storage_bucket_document_key( - user_id: &str, + owner: &Owner, document_id: &str, document_version_id: T, ) -> String { - build_cloud_storage_bucket_document_key_helper(user_id, document_id, document_version_id, None) + build_document_key_from_segment( + &owner_segment(owner), + document_id, + document_version_id, + None, + ) +} + +/// Builds the key prefix shared by every object of one document in the cloud +/// storage bucket: `{owner}/{document_id}`. Use it to list or delete a +/// document's objects. +pub fn build_cloud_storage_bucket_document_prefix(owner: &Owner, document_id: &str) -> String { + build_document_prefix_from_segment(&owner_segment(owner), document_id) } /// Builds the S3 key for a converted DOCX document's PDF output. -/// Format: `{user_id}/{document_id}/converted.pdf` -pub fn build_docx_to_pdf_converted_document_key(user_id: &str, document_id: &str) -> String { - build_cloud_storage_bucket_document_key_helper( - user_id, +/// Format: `{owner}/{document_id}/converted.pdf` +pub fn build_docx_to_pdf_converted_document_key(owner: &Owner, document_id: &str) -> String { + build_document_key_from_segment( + &owner_segment(owner), document_id, CONVERTED_DOCUMENT_FILE_NAME, Some(PDF_EXTENSION), @@ -235,14 +294,14 @@ pub fn build_docx_to_pdf_converted_document_key(user_id: &str, document_id: &str } /// Builds the S3 key for a DOCX document's staging bucket. -/// Format: `{user_id}/{document_id}/{document_version_id}.docx` +/// Format: `{owner}/{document_id}/{document_version_id}.docx` pub fn build_docx_staging_bucket_document_key( - user_id: &str, + owner: &Owner, document_id: &str, document_version_id: i64, ) -> String { - build_cloud_storage_bucket_document_key_helper( - user_id, + build_document_key_from_segment( + &owner_segment(owner), document_id, document_version_id, Some(DOCX_EXTENSION), @@ -255,5 +314,20 @@ pub fn build_temp_docx_key(document_id: &str) -> String { format!("{}/{}.{}", TEMP_FILE_PREFIX, document_id, DOCX_EXTENSION) } +/// Percent-encodes a document key for use as a URL path. +/// +/// Each `/`-separated segment is encoded on its own so the separators survive. +/// Object keys are never stored encoded (see [`owner_segment`]); this is only +/// for building a URL by hand, such as a CloudFront signed URL or an S3 copy +/// source, where `|` and `@` in a user principal are not path-safe. Document +/// IDs and version segments contain only unreserved characters, so for a +/// user-owned key this changes exactly the owner segment. +pub fn document_key_url_path(key: &str) -> String { + key.split('/') + .map(urlencoding::encode) + .collect::>() + .join("/") +} + #[cfg(test)] mod test; diff --git a/crates/s3_key/src/document_key/test.rs b/crates/s3_key/src/document_key/test.rs index 3f87feba351..4bc075214e0 100644 --- a/crates/s3_key/src/document_key/test.rs +++ b/crates/s3_key/src/document_key/test.rs @@ -1,12 +1,215 @@ use super::*; +const USER_PRINCIPAL: &str = "macro|hutch@macro.com"; +const BOT_PRINCIPAL: &str = "bot|00000000-0000-0000-0000-00000000a1a1"; +const TEAM_PRINCIPAL: &str = "019ce2bb-e97e-7fdb-b6b4-d6b36fbe9da0"; +const DOCUMENT_ID: &str = "01a0914f-1fde-7873-80ba-6eef315d3b50"; + +fn owner(principal: &str) -> Owner { + Owner::from_principal_str(principal).expect("test principal is valid") +} + +/// One owner of each kind, paired with the principal string its segment must equal. +fn owners() -> [(&'static str, Owner); 3] { + [ + (USER_PRINCIPAL, owner(USER_PRINCIPAL)), + (BOT_PRINCIPAL, owner(BOT_PRINCIPAL)), + (TEAM_PRINCIPAL, owner(TEAM_PRINCIPAL)), + ] +} + +/// Golden test: a user-owned document must produce exactly the keys production +/// wrote before the owner segment became a principal string. Any drift here +/// orphans every stored object. +#[test] +fn user_owned_keys_are_byte_identical_to_legacy_keys() { + let owner = owner(USER_PRINCIPAL); + + assert_eq!( + build_cloud_storage_bucket_document_key(&owner, DOCUMENT_ID, 789), + "macro|hutch@macro.com/01a0914f-1fde-7873-80ba-6eef315d3b50/789" + ); + assert_eq!( + build_docx_to_pdf_converted_document_key(&owner, DOCUMENT_ID), + "macro|hutch@macro.com/01a0914f-1fde-7873-80ba-6eef315d3b50/converted.pdf" + ); + assert_eq!( + build_docx_staging_bucket_document_key(&owner, DOCUMENT_ID, 789), + "macro|hutch@macro.com/01a0914f-1fde-7873-80ba-6eef315d3b50/789.docx" + ); + assert_eq!( + build_cloud_storage_bucket_document_prefix(&owner, DOCUMENT_ID), + "macro|hutch@macro.com/01a0914f-1fde-7873-80ba-6eef315d3b50" + ); +} + +/// Golden test for the read side: the CloudFront URL path for a user-owned key +/// must match what the callers produced when they percent-encoded the owner +/// themselves. +#[test] +fn user_owned_url_path_is_byte_identical_to_legacy_url_path() { + let owner = owner(USER_PRINCIPAL); + + let versioned = build_cloud_storage_bucket_document_key(&owner, DOCUMENT_ID, 789); + assert_eq!( + document_key_url_path(&versioned), + "macro%7Chutch%40macro.com/01a0914f-1fde-7873-80ba-6eef315d3b50/789" + ); + + let converted = build_docx_to_pdf_converted_document_key(&owner, DOCUMENT_ID); + assert_eq!( + document_key_url_path(&converted), + "macro%7Chutch%40macro.com/01a0914f-1fde-7873-80ba-6eef315d3b50/converted.pdf" + ); +} + +#[test] +fn owner_segment_is_the_principal_for_every_owner_kind() { + for (principal, owner) in owners() { + assert_eq!( + build_cloud_storage_bucket_document_key(&owner, DOCUMENT_ID, 1), + format!("{principal}/{DOCUMENT_ID}/1"), + ); + assert_eq!( + build_docx_to_pdf_converted_document_key(&owner, DOCUMENT_ID), + format!("{principal}/{DOCUMENT_ID}/converted.pdf"), + ); + assert_eq!( + build_docx_staging_bucket_document_key(&owner, DOCUMENT_ID, 1), + format!("{principal}/{DOCUMENT_ID}/1.docx"), + ); + assert_eq!( + build_cloud_storage_bucket_document_prefix(&owner, DOCUMENT_ID), + format!("{principal}/{DOCUMENT_ID}"), + ); + } +} + +#[test] +fn versioned_key_round_trips_for_every_owner_kind() { + for (principal, owner) in owners() { + let key = build_cloud_storage_bucket_document_key(&owner, DOCUMENT_ID, 789); + let parsed = DocumentKey::from_s3_key(&key).unwrap(); + + assert_eq!( + parsed, + DocumentKey::Versioned { + owner_segment: principal.to_string(), + document_id: DOCUMENT_ID.to_string(), + version_id: 789, + } + ); + assert_eq!(parsed.owner_segment(), Some(principal)); + assert_eq!(parsed.to_key(), key); + assert!(parsed.is_versioned()); + } +} + +#[test] +fn converted_pdf_key_round_trips_for_every_owner_kind() { + for (principal, owner) in owners() { + let key = build_docx_to_pdf_converted_document_key(&owner, DOCUMENT_ID); + let parsed = DocumentKey::from_s3_key(&key).unwrap(); + + assert_eq!( + parsed, + DocumentKey::ConvertedPdf { + owner_segment: principal.to_string(), + document_id: DOCUMENT_ID.to_string(), + } + ); + assert_eq!(parsed.owner_segment(), Some(principal)); + assert_eq!(parsed.to_key(), key); + assert!(parsed.is_converted_pdf()); + } +} + +#[test] +fn parsed_owner_segment_is_a_valid_principal_for_every_owner_kind() { + for (_, owner) in owners() { + let key = build_cloud_storage_bucket_document_key(&owner, DOCUMENT_ID, 1); + let parsed = DocumentKey::from_s3_key(&key).unwrap(); + let segment = parsed.owner_segment().unwrap(); + + assert_eq!(Owner::from_principal_str(segment).unwrap(), owner); + } +} + +#[test] +fn url_path_encodes_each_segment_and_keeps_separators() { + for (principal, owner) in owners() { + let key = build_cloud_storage_bucket_document_key(&owner, DOCUMENT_ID, 789); + let path = document_key_url_path(&key); + + assert_eq!( + path, + format!("{}/{DOCUMENT_ID}/789", urlencoding::encode(principal)) + ); + assert_eq!(path.matches('/').count(), 2); + assert!(!path.contains('|')); + assert!(!path.contains('@')); + } +} + +#[test] +fn url_path_leaves_a_team_owned_key_unchanged() { + let key = build_cloud_storage_bucket_document_key(&owner(TEAM_PRINCIPAL), DOCUMENT_ID, 789); + assert_eq!(document_key_url_path(&key), key); +} + +#[test] +fn url_path_leaves_owner_less_keys_unchanged() { + for key in [ + build_temp_docx_key(DOCUMENT_ID), + format!("{SYNC_SERVICE_SNAPSHOT_PREFIX}/{DOCUMENT_ID}"), + "7b5ce90c96ec3c24d8764ba75076bc0c2c5256b2d44e71cf9a8f001ea21ed678".to_string(), + ] { + assert_eq!(document_key_url_path(&key), key); + } +} + +/// `%` is legal in an email local part, so an owner can contain a literal +/// percent sequence. Parsing must not decode it into a different owner. +#[test] +fn owner_with_a_literal_percent_sequence_round_trips_unchanged() { + let owner = owner("macro|a%2bb@macro.com"); + let key = build_cloud_storage_bucket_document_key(&owner, DOCUMENT_ID, 1); + assert_eq!(key, format!("macro|a%2bb@macro.com/{DOCUMENT_ID}/1")); + + let parsed = DocumentKey::from_s3_key(&key).unwrap(); + assert_eq!(parsed.owner_segment(), Some("macro|a%2bb@macro.com")); + assert_eq!(parsed.to_key(), key); + assert_eq!( + Owner::from_principal_str(parsed.owner_segment().unwrap()).unwrap(), + owner + ); +} + +/// A form-encoded key is a different key. Callers decode at their inbound +/// boundary; the parser never guesses. +#[test] +fn from_s3_key_keeps_a_percent_encoded_owner_segment_verbatim() { + let encoded = "macro%7Chutch%40macro.com/01a0914f-1fde-7873-80ba-6eef315d3b50/789"; + let key = DocumentKey::from_s3_key(encoded).unwrap(); + + assert_eq!( + key, + DocumentKey::Versioned { + owner_segment: "macro%7Chutch%40macro.com".to_string(), + document_id: DOCUMENT_ID.to_string(), + version_id: 789, + } + ); + assert_eq!(key.to_key(), encoded); +} + #[test] fn test_versioned_key_from_s3_key() { let key = DocumentKey::from_s3_key("user123/doc456/789").unwrap(); assert_eq!( key, DocumentKey::Versioned { - user_id: "user123".to_string(), + owner_segment: "user123".to_string(), document_id: "doc456".to_string(), version_id: 789, } @@ -20,7 +223,7 @@ fn test_converted_key_from_s3_key() { assert_eq!( key, DocumentKey::ConvertedPdf { - user_id: "user123".to_string(), + owner_segment: "user123".to_string(), document_id: "doc456".to_string(), } ); @@ -39,6 +242,7 @@ fn test_temp_docx_key_from_s3_key() { } ); assert_eq!(key.to_key(), "temp_files/doc456.docx"); + assert_eq!(key.owner_segment(), None); assert!(key.is_temp()); assert!(!key.is_converted_pdf()); } @@ -54,6 +258,7 @@ fn test_sync_service_snapshot_key_from_s3_key() { ); assert_eq!(key.to_key(), "sync_service_snapshot/doc456"); assert_eq!(key.document_id(), Some("doc456")); + assert_eq!(key.owner_segment(), None); assert_eq!(key.version_id_string(), None); assert!(key.is_sync_service_snapshot()); assert!(!key.is_temp()); @@ -61,19 +266,6 @@ fn test_sync_service_snapshot_key_from_s3_key() { assert!(!key.is_converted_pdf()); } -#[test] -fn test_url_encoded_user_id() { - let key = DocumentKey::from_s3_key("user%20123/doc456/789").unwrap(); - assert_eq!( - key, - DocumentKey::Versioned { - user_id: "user 123".to_string(), - document_id: "doc456".to_string(), - version_id: 789, - } - ); -} - #[test] fn test_invalid_key_format() { assert!(DocumentKey::from_s3_key("only-one").is_err()); @@ -94,6 +286,7 @@ fn test_bom_part_key() { ); assert!(key.is_bom_part()); assert_eq!(key.document_id(), None); + assert_eq!(key.owner_segment(), None); assert_eq!(key.version_id_string(), None); assert_eq!( key.to_key(), @@ -142,24 +335,6 @@ fn test_version_id_string() { assert_eq!(snapshot.version_id_string(), None); } -#[test] -fn test_build_cloud_storage_bucket_document_key() { - let key = build_cloud_storage_bucket_document_key("owner", "document-id", 1); - assert_eq!(key, "owner/document-id/1"); -} - -#[test] -fn test_build_docx_to_pdf_converted_document_key() { - let key = build_docx_to_pdf_converted_document_key("owner", "document-id"); - assert_eq!(key, "owner/document-id/converted.pdf"); -} - -#[test] -fn test_build_docx_staging_bucket_document_key() { - let key = build_docx_staging_bucket_document_key("owner", "document-id", 1); - assert_eq!(key, "owner/document-id/1.docx"); -} - #[test] fn test_build_temp_docx_key() { let key = build_temp_docx_key("document-id"); diff --git a/crates/s3_key/src/lib.rs b/crates/s3_key/src/lib.rs index 3b0d3eab340..1613f3b0a1c 100644 --- a/crates/s3_key/src/lib.rs +++ b/crates/s3_key/src/lib.rs @@ -6,8 +6,8 @@ mod document_key; pub use document_key::{ CONVERTED_DOCUMENT_FILE_NAME, DOCX_EXTENSION, DocumentKey, PDF_EXTENSION, SYNC_SERVICE_SNAPSHOT_PREFIX, TEMP_FILE_PREFIX, build_cloud_storage_bucket_document_key, - build_docx_staging_bucket_document_key, build_docx_to_pdf_converted_document_key, - build_temp_docx_key, + build_cloud_storage_bucket_document_prefix, build_docx_staging_bucket_document_key, + build_docx_to_pdf_converted_document_key, build_temp_docx_key, document_key_url_path, }; mod bulk_upload_key; pub use bulk_upload_key::BulkUploadStagingKey; diff --git a/crates/sqs_client/src/search/document.rs b/crates/sqs_client/src/search/document.rs index 045c71b7726..d5d30edec6b 100644 --- a/crates/sqs_client/src/search/document.rs +++ b/crates/sqs_client/src/search/document.rs @@ -5,7 +5,8 @@ use s3_key::CONVERTED_DOCUMENT_FILE_NAME; /// Search text extractor message #[derive(serde::Serialize, serde::Deserialize, PartialEq, Eq, Debug)] pub struct SearchExtractorMessage { - /// The user id who owns the document + /// The owner principal of the document: `macro|`, `bot|`, or a team UUID. + /// The field keeps its historical name on the wire. pub user_id: String, /// The document id pub document_id: String, diff --git a/services/convert_service/src/api/backfill/backfill_docx.rs b/services/convert_service/src/api/backfill/backfill_docx.rs index 6ee5d208f99..16d33108b4d 100644 --- a/services/convert_service/src/api/backfill/backfill_docx.rs +++ b/services/convert_service/src/api/backfill/backfill_docx.rs @@ -117,8 +117,8 @@ async fn process_docx( serde_json::from_value(bom_parts).context("unable to serialize bom parts")?; // check if the converted item exists in s3 - let owner = document.owner.principal_id(); - let key = build_docx_to_pdf_converted_document_key(&owner, &document.document_id); + let key = + build_docx_to_pdf_converted_document_key(&document.owner, &document.document_id); let exists = s3_client .exists(document_storage_bucket, &key) @@ -168,8 +168,8 @@ async fn process_docx( let zipped_docx = zip_bom_parts(bom_parts_with_content).await?; let from_key = build_temp_docx_key(&document.document_id); - let owner = document.owner.principal_id(); - let to_key = build_docx_to_pdf_converted_document_key(&owner, &document.document_id); + let to_key = + build_docx_to_pdf_converted_document_key(&document.owner, &document.document_id); s3_client .put(document_storage_bucket, &from_key, &zipped_docx) diff --git a/services/document_storage_service/src/api/documents/export_document.rs b/services/document_storage_service/src/api/documents/export_document.rs index 8c41aa91a6b..c08c4535831 100644 --- a/services/document_storage_service/src/api/documents/export_document.rs +++ b/services/document_storage_service/src/api/documents/export_document.rs @@ -18,6 +18,7 @@ use model::{ document::{DocumentBasic, FileType, response::LocationResponseData}, response::{ErrorResponse, GenericErrorResponse}, }; +use model_owner::Owner; use s3_key::build_temp_docx_key; use models_permissions::share_permission::access_level::ViewAccessLevel; @@ -85,9 +86,13 @@ pub async fn handler( let presigned_url = match file_type { FileType::Docx => export_docx_document(&state, &document_context.document_id).await, _ => { - let owner = document_context.owner.principal_id(); - export_basic_document(&state, &owner, &document_context.document_id, file_type) - .await + export_basic_document( + &state, + &document_context.owner, + &document_context.document_id, + file_type, + ) + .await } } .map_err(|e| { @@ -122,7 +127,7 @@ pub async fn handler( /// Wrapper for get_presigned_url_by_type to export basic documents async fn export_basic_document( ctx: &ApiContext, - owner: &str, + owner: &Owner, document_id: &str, file_type: FileType, ) -> anyhow::Result { diff --git a/services/document_storage_service/src/api/documents/get_document_key.rs b/services/document_storage_service/src/api/documents/get_document_key.rs index ae486c70fe2..c63e29182ba 100644 --- a/services/document_storage_service/src/api/documents/get_document_key.rs +++ b/services/document_storage_service/src/api/documents/get_document_key.rs @@ -63,7 +63,7 @@ pub async fn get_document_key_handler( .send(StatusCode::BAD_REQUEST); }; - let owner = document_context.owner.principal_id(); + let owner = &document_context.owner; let key = match file_type { FileType::Pdf => { let document_version_id = @@ -80,9 +80,9 @@ pub async fn get_document_key_handler( } }; - build_cloud_storage_bucket_document_key(&owner, &document_id, document_version_id) + build_cloud_storage_bucket_document_key(owner, &document_id, document_version_id) } - FileType::Docx => build_docx_to_pdf_converted_document_key(&owner, &document_id), + FileType::Docx => build_docx_to_pdf_converted_document_key(owner, &document_id), _ => { tracing::error!("invalid file type"); return GenericResponse::builder() diff --git a/services/document_storage_service/src/api/documents/initialize_user_documents.rs b/services/document_storage_service/src/api/documents/initialize_user_documents.rs index 6e1114f1c3e..e0135c207f4 100644 --- a/services/document_storage_service/src/api/documents/initialize_user_documents.rs +++ b/services/document_storage_service/src/api/documents/initialize_user_documents.rs @@ -164,7 +164,7 @@ pub async fn handler( let s3_client = shared_s3_client.clone(); // Clone the client for parallel usage let markdown_template = shared_markdown_template.clone(); let canvas_template = shared_canvas_template.clone(); - let user_id = user_context.authorization.user.macro_user_id.clone(); + let owner = Owner::User(user_context.authorization.user.macro_user_id.clone()); async move { let uri_document_name = urlencoding::encode(document.document_name.as_str()); let deref_file_type = document.file_type.as_deref(); @@ -175,7 +175,7 @@ pub async fn handler( }; let target_key = build_cloud_storage_bucket_document_key( - user_id.as_ref(), + &owner, &document.document_id, document.document_version_id, ); diff --git a/services/document_storage_service/src/api/documents/location.rs b/services/document_storage_service/src/api/documents/location.rs index 65265c6cc1c..47a6a3619ab 100644 --- a/services/document_storage_service/src/api/documents/location.rs +++ b/services/document_storage_service/src/api/documents/location.rs @@ -22,7 +22,11 @@ use model::{ document::{DocumentBasic, FileType, FileTypeExt, response::LocationResponseData}, response::{GenericErrorResponse, GenericResponse, PresignedUrl}, }; -use s3_key::{build_cloud_storage_bucket_document_key, build_docx_to_pdf_converted_document_key}; +use model_owner::Owner; +use s3_key::{ + build_cloud_storage_bucket_document_key, build_docx_to_pdf_converted_document_key, + document_key_url_path, +}; #[derive(serde::Deserialize)] pub struct Params { @@ -67,10 +71,9 @@ pub async fn get_location_handler( .as_deref() .and_then(|f| FileType::from_str(f).ok()); - let owner = document_context.owner.principal_id(); let response_data = get_presigned_url_by_type( &state, - &owner, + &document_context.owner, &document_id, file_type, params.document_version_id, @@ -112,18 +115,17 @@ pub async fn get_location_handler( } /// Signs a document key and returns a CloudFront presigned URL. -/// `check_key` is the non-URL-encoded key for S3 existence verification. -/// `signed_key` is the URL-encoded key for the CloudFront signed URL. +/// The object key is used as-is for S3 existence verification and +/// percent-encoded only where it becomes the URL path. #[tracing::instrument(skip(state), err)] async fn sign_document_key( state: &ApiContext, - #[allow(unused_variables)] check_key: &str, - signed_key: &str, + #[allow(unused_variables)] key: &str, ) -> anyhow::Result { #[cfg(feature = "location_check")] { - tracing::trace!("checking if file exists in s3, key: {}", check_key); - let exists = &state.s3_client.exists(check_key).await?; + tracing::trace!("checking if file exists in s3, key: {}", key); + let exists = &state.s3_client.exists(key).await?; if !exists { anyhow::bail!(DOCUMENT_DOES_NOT_EXIST); } @@ -146,7 +148,7 @@ async fn sign_document_key( &state .config .document_storage_service_cloudfront_distribution_url, - signed_key, + &document_key_url_path(key), &signed_options, )?; @@ -161,7 +163,7 @@ async fn sign_document_key( #[tracing::instrument(skip(state))] pub(in crate::api::documents) async fn get_versioned_url( state: &ApiContext, - owner: &str, + owner: &Owner, document_id: &str, document_version_id: Option, is_static: bool, @@ -180,30 +182,21 @@ pub(in crate::api::documents) async fn get_versioned_url( } }; - let check_key = - build_cloud_storage_bucket_document_key(owner, document_id, document_version_id); - let url_encoded_owner = urlencoding::encode(owner); - let signed_key = build_cloud_storage_bucket_document_key( - &url_encoded_owner, - document_id, - document_version_id, - ); + let key = build_cloud_storage_bucket_document_key(owner, document_id, document_version_id); - sign_document_key(state, &check_key, &signed_key).await + sign_document_key(state, &key).await } /// Gets the presigned url for the converted docx file #[tracing::instrument(skip(state))] async fn get_converted_docx_url( state: &ApiContext, - owner: &str, + owner: &Owner, document_id: &str, ) -> anyhow::Result { - let check_key = build_docx_to_pdf_converted_document_key(owner, document_id); - let url_encoded_owner = urlencoding::encode(owner); - let signed_key = build_docx_to_pdf_converted_document_key(&url_encoded_owner, document_id); + let key = build_docx_to_pdf_converted_document_key(owner, document_id); - sign_document_key(state, &check_key, &signed_key).await + sign_document_key(state, &key).await } #[tracing::instrument(skip(state))] @@ -297,7 +290,7 @@ pub(in crate::api::documents) fn get_cloudfront_signed_options( #[tracing::instrument(skip(state))] pub(in crate::api::documents) async fn get_presigned_url_by_type( state: &ApiContext, - owner: &str, + owner: &Owner, document_id: &str, file_type: Option, document_version_id: Option, diff --git a/services/document_storage_service/src/api/documents/simple_save.rs b/services/document_storage_service/src/api/documents/simple_save.rs index 9078d6443d3..252df437cd0 100644 --- a/services/document_storage_service/src/api/documents/simple_save.rs +++ b/services/document_storage_service/src/api/documents/simple_save.rs @@ -116,9 +116,8 @@ pub async fn handler( } }; - let owner = document.owner.principal_id(); let key = build_cloud_storage_bucket_document_key( - &owner, + &document.owner, &document.document_id, document.document_version_id, ); diff --git a/services/document_storage_service/src/service/delete_document_worker/handle.rs b/services/document_storage_service/src/service/delete_document_worker/handle.rs index 64a27cee4c2..4fb11b06b79 100644 --- a/services/document_storage_service/src/service/delete_document_worker/handle.rs +++ b/services/document_storage_service/src/service/delete_document_worker/handle.rs @@ -1,6 +1,7 @@ use anyhow::Context; use documents_hex::domain::ports::editing::EditingWorkerService; use entity_access::domain::models::EntityType; +use model_owner::Owner; use properties::{EditReceipt, PropertiesService as _}; use super::DeleteDocumentWorkerContext; @@ -12,7 +13,7 @@ pub async fn handle( ) -> anyhow::Result<()> { tracing::debug!("processing delete document message"); - let (document_id, mut user_id) = if let Some(attributes) = message.message_attributes.as_ref() { + let (document_id, mut owner) = if let Some(attributes) = message.message_attributes.as_ref() { let document_id = attributes .get("document_id") .map(|document_id| { @@ -21,19 +22,25 @@ pub async fn handle( }) .context("document_id should be a message attribute")?; - let user_id = attributes.get("user_id").map(|user_id| { - tracing::trace!(user_id=?user_id, "found user_id in message attributes"); - user_id.string_value().unwrap_or_default().to_string() - }); + // The `user_id` attribute carries the owner principal (`macro|`, + // `bot|`, or a team UUID); it keeps its historical name on the wire. + let owner = attributes + .get("user_id") + .map(|user_id| { + tracing::trace!(user_id=?user_id, "found user_id in message attributes"); + Owner::from_principal_str(user_id.string_value().unwrap_or_default()) + }) + .transpose() + .context("user_id message attribute should be an owner principal")?; - (document_id, user_id) + (document_id, owner) } else { ctx.worker.cleanup_message(message).await?; anyhow::bail!("message attributes not found") }; - // Only need to get and delete document from macrodb if the user_id is not present in the message attributes - if user_id.is_none() { + // Only need to get and delete document from macrodb if the owner is not present in the message attributes + if owner.is_none() { tracing::info!(document_id=%document_id, "starting delete process for document"); let document = macro_db_client::document::get_deleted_document_info(&ctx.db, document_id) @@ -42,10 +49,9 @@ pub async fn handle( |e| tracing::error!(error=?e, document_id=%document_id, "unable to get document"), )?; - let shared_document = document.clone(); - user_id = Some(shared_document.owner.to_string()); + owner = Some(document.owner.clone()); - tracing::trace!(document_id=%document_id, user_id=?user_id, file_type=?document.file_type, "retrieved document"); + tracing::trace!(document_id=%document_id, owner=?owner, file_type=?document.file_type, "retrieved document"); if let Some(file_type) = document.file_type && file_type.as_str() == "docx" @@ -81,12 +87,12 @@ pub async fn handle( tracing::warn!(error=?e, "could not delete entity mentions for document"); }); - let user_id = user_id.context("user_id should be some")?; + let owner = owner.context("owner should be some")?; // Delete files from s3 - tracing::trace!(user_id=%user_id, document_id=%document_id, "deleting files from s3"); + tracing::trace!(owner=%owner, document_id=%document_id, "deleting files from s3"); ctx.s3_client - .delete_document(&user_id, document_id) + .delete_document(&owner, document_id) .await .context("failed to delete files from s3")?; tracing::trace!(document_id=%document_id, "deleted files from s3"); diff --git a/services/document_storage_service/src/service/s3.rs b/services/document_storage_service/src/service/s3.rs index 6aedc3f57a9..a8ae037c698 100644 --- a/services/document_storage_service/src/service/s3.rs +++ b/services/document_storage_service/src/service/s3.rs @@ -24,6 +24,7 @@ pub use S3Client as S3; use tokio::sync::Semaphore; use model::document::{ContentType, FileType}; +use model_owner::Owner; #[derive(Clone, Debug)] pub struct S3Client { @@ -149,12 +150,12 @@ impl S3Client { .await } - /// Deletes all document instances stored under a user's document - pub async fn delete_document(&self, user_id: &str, document_id: &str) -> anyhow::Result<()> { + /// Deletes all document instances stored under an owner's document + pub async fn delete_document(&self, owner: &Owner, document_id: &str) -> anyhow::Result<()> { delete::delete_document( &self.inner, &self.document_storage_bucket, - user_id, + owner, document_id, ) .await diff --git a/services/document_storage_service/src/service/s3/delete.rs b/services/document_storage_service/src/service/s3/delete.rs index 9e00b7845ad..f7b934f08bd 100644 --- a/services/document_storage_service/src/service/s3/delete.rs +++ b/services/document_storage_service/src/service/s3/delete.rs @@ -1,5 +1,7 @@ use aws_sdk_s3 as s3; +use model_owner::Owner; use s3::types::{Delete, ObjectIdentifier}; +use s3_key::build_cloud_storage_bucket_document_prefix; use tracing::instrument; #[instrument(skip(client))] @@ -37,12 +39,12 @@ pub(in crate::service::s3) async fn delete_objects( Ok(()) } -/// Deletes all document instances stored under a user's document +/// Deletes all document instances stored under an owner's document #[instrument(skip(client))] pub(in crate::service::s3) async fn delete_document( client: &s3::Client, bucket: &str, - user_id: &str, + owner: &Owner, document_id: &str, ) -> anyhow::Result<()> { if cfg!(feature = "local") { @@ -51,7 +53,7 @@ pub(in crate::service::s3) async fn delete_document( let mut to_delete: Vec = Vec::new(); - let prefix = format!("{}/{}", user_id, document_id); + let prefix = build_cloud_storage_bucket_document_prefix(owner, document_id); let resp = client .list_objects_v2() .bucket(bucket) @@ -74,17 +76,19 @@ pub(in crate::service::s3) async fn delete_document( #[cfg(test)] mod tests { use mockall::predicate::eq; + use model_owner::Owner; use crate::service::s3::S3; #[tokio::test] async fn test_delete_document() { + let owner = Owner::from_principal_str("macro|user@example.com").unwrap(); let mut mock = S3::default(); mock.expect_delete_document() - .with(eq("user_id"), eq("document_id")) + .with(eq(owner.clone()), eq("document_id")) .return_once(|_, _| Ok(())); - let result = mock.delete_document("user_id", "document_id").await; + let result = mock.delete_document(&owner, "document_id").await; assert!(result.is_ok()); } diff --git a/services/docx_unzip_handler/Cargo.toml b/services/docx_unzip_handler/Cargo.toml index 5cc5c527685..fed25d38585 100644 --- a/services/docx_unzip_handler/Cargo.toml +++ b/services/docx_unzip_handler/Cargo.toml @@ -25,6 +25,7 @@ macro_env = { path = "../../crates/macro_env" } macro_sha_count_client = { path = "../../crates/macro_sha_count_client" } macro_uuid = { path = "../../crates/macro_uuid" } model = { path = "../../crates/model" } +model_owner = { path = "../../crates/model_owner" } s3_key = { path = "../../crates/s3_key" } redis = { workspace = true, features = [ "aio", diff --git a/services/docx_unzip_handler/src/models.rs b/services/docx_unzip_handler/src/models.rs index 349c3f57fc9..d77f63db923 100644 --- a/services/docx_unzip_handler/src/models.rs +++ b/services/docx_unzip_handler/src/models.rs @@ -3,10 +3,13 @@ use std::borrow::Cow; use anyhow::Context; use lambda_runtime::tracing; use model::document::SaveBomPart; +use model_owner::Owner; +use s3_key::build_docx_staging_bucket_document_key; +/// The parts of a DOCX staging bucket key: `{owner}/{document_id}/{document_bom_id}.docx`. #[derive(serde::Serialize, serde::Deserialize, Eq, PartialEq, Debug)] pub struct DocumentKeyParts { - pub user_id: String, + pub owner: Owner, pub document_id: String, pub document_bom_id: i64, } @@ -14,29 +17,28 @@ pub struct DocumentKeyParts { impl DocumentKeyParts { #[tracing::instrument] pub fn from_s3_key(key: &str) -> Result { - // user_id/document_id/document_bom_id.docx + // owner/document_id/document_bom_id.docx let split = key.split("/").collect::>(); if split.len() != 3 { anyhow::bail!("invalid key format"); } - let encoded_user_id = split[0].to_string(); - let user_id = urlencoding::decode(&encoded_user_id).context("UTF-8")?; + // S3 event notifications deliver keys form-encoded. + let owner_segment = urlencoding::decode(split[0]).context("UTF-8")?; + let owner = Owner::from_principal_str(&owner_segment) + .context("owner segment is not an owner principal")?; let document_bom_id = split[2].split(".").collect::>()[0] .parse::() .context("expect a number for verison id")?; Ok(Self { - user_id: user_id.to_string(), + owner, document_id: split[1].to_string(), document_bom_id, }) } pub fn to_key(&self) -> String { - format!( - "{}/{}/{}.docx", - self.user_id, self.document_id, self.document_bom_id - ) + build_docx_staging_bucket_document_key(&self.owner, &self.document_id, self.document_bom_id) } } diff --git a/services/docx_unzip_handler/src/service/document/process_document.rs b/services/docx_unzip_handler/src/service/document/process_document.rs index ec292ce6816..09b1769dacd 100644 --- a/services/docx_unzip_handler/src/service/document/process_document.rs +++ b/services/docx_unzip_handler/src/service/document/process_document.rs @@ -53,9 +53,9 @@ pub async fn process( from_bucket: bucket.clone(), from_key: document_key_parts.to_key(), // The key for storing a converted version of a file is - // "{user_id}/{document_id}/converted.{file_extension}" + // "{owner}/{document_id}/converted.{file_extension}" to_key: build_docx_to_pdf_converted_document_key( - &document_key_parts.user_id, + &document_key_parts.owner, &document_key_parts.document_id, ), to_bucket: ctx.config.document_storage_bucket.clone(), diff --git a/services/search_processing_service/src/process/document/raw_document.rs b/services/search_processing_service/src/process/document/raw_document.rs index 0052c7011e6..8a167b8cb9a 100644 --- a/services/search_processing_service/src/process/document/raw_document.rs +++ b/services/search_processing_service/src/process/document/raw_document.rs @@ -5,6 +5,7 @@ use chrono::Utc; use document_sub_type::DocumentSubType; use lexical_client::types::MarkdownParseResult; use model::document::{DocumentMetadata, FileType}; +use model_owner::Owner; use models_properties::EntityType; use models_search::unified::is_searchable_association; use opensearch_client::{ @@ -262,11 +263,10 @@ pub async fn update_search_with_raw_document( .as_str(), )?; + let key_owner = Owner::from_principal_str(&search_extractor_message.user_id) + .context("search extractor message user_id is not an owner principal")?; let key = if document_version_id == CONVERTED_DOCUMENT_FILE_NAME { - build_docx_to_pdf_converted_document_key( - &search_extractor_message.user_id, - &search_extractor_message.document_id, - ) + build_docx_to_pdf_converted_document_key(&key_owner, &search_extractor_message.document_id) } else if let Some(msg_version_id) = search_extractor_message.document_version_id.as_ref() && msg_version_id != &document_version_id { @@ -278,7 +278,7 @@ pub async fn update_search_with_raw_document( return Ok(()); } else { build_cloud_storage_bucket_document_key( - &search_extractor_message.user_id, + &key_owner, &search_extractor_message.document_id, &document_version_id, )