diff --git a/rust/cloud-storage/Cargo.lock b/rust/cloud-storage/Cargo.lock index e17aa176249..01394d81b8d 100644 --- a/rust/cloud-storage/Cargo.lock +++ b/rust/cloud-storage/Cargo.lock @@ -6569,6 +6569,7 @@ dependencies = [ "serde_json", "serde_with", "sqlx", + "system_properties", "thiserror 2.0.17", "tokio", "tower 0.5.2", diff --git a/rust/cloud-storage/models_properties/src/lib.rs b/rust/cloud-storage/models_properties/src/lib.rs index c294c64240b..312ed744b2c 100644 --- a/rust/cloud-storage/models_properties/src/lib.rs +++ b/rust/cloud-storage/models_properties/src/lib.rs @@ -13,4 +13,4 @@ pub mod service; pub mod shared; // Re-export commonly used shared types for convenience -pub use shared::{DataType, EntityReference, EntityType, PropertyOwner}; +pub use shared::{DataType, EntityPropertyReference, EntityReference, EntityType, PropertyOwner}; diff --git a/rust/cloud-storage/models_properties/src/shared/entity_reference.rs b/rust/cloud-storage/models_properties/src/shared/entity_reference.rs index 96f7966385c..7192d6e3bb3 100644 --- a/rust/cloud-storage/models_properties/src/shared/entity_reference.rs +++ b/rust/cloud-storage/models_properties/src/shared/entity_reference.rs @@ -2,12 +2,22 @@ use serde::{Deserialize, Serialize}; use utoipa::ToSchema; +use uuid::Uuid; use super::EntityType; /// Entity reference for entity-type property values. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, ToSchema)] pub struct EntityReference { + pub entity_id: String, pub entity_type: EntityType, +} + +/// Reference to an entity property, including the property definition ID. +/// Used for property lookups and deletion validation. +#[derive(Debug, Clone)] +pub struct EntityPropertyReference { pub entity_id: String, + pub entity_type: EntityType, + pub property_definition_id: Uuid, } diff --git a/rust/cloud-storage/models_properties/src/shared/mod.rs b/rust/cloud-storage/models_properties/src/shared/mod.rs index 410ed1f1f79..b3a50e00b06 100644 --- a/rust/cloud-storage/models_properties/src/shared/mod.rs +++ b/rust/cloud-storage/models_properties/src/shared/mod.rs @@ -4,6 +4,6 @@ pub mod entity_type; pub mod property_owner; pub use data_type::DataType; -pub use entity_reference::EntityReference; +pub use entity_reference::{EntityPropertyReference, EntityReference}; pub use entity_type::EntityType; pub use property_owner::PropertyOwner; diff --git a/rust/cloud-storage/properties_db_client/.sqlx/query-17bcb8a4765cf0bfc406a98a2a8829342f248afbdfddef55ef1f44aa4c9b034e.json b/rust/cloud-storage/properties_db_client/.sqlx/query-d7b0cf2d0add26c4c959e055b70f8ee8faf694757d7187135602b7508f0b0eca.json similarity index 69% rename from rust/cloud-storage/properties_db_client/.sqlx/query-17bcb8a4765cf0bfc406a98a2a8829342f248afbdfddef55ef1f44aa4c9b034e.json rename to rust/cloud-storage/properties_db_client/.sqlx/query-d7b0cf2d0add26c4c959e055b70f8ee8faf694757d7187135602b7508f0b0eca.json index 72c36bfbe7e..ce40e97428b 100644 --- a/rust/cloud-storage/properties_db_client/.sqlx/query-17bcb8a4765cf0bfc406a98a2a8829342f248afbdfddef55ef1f44aa4c9b034e.json +++ b/rust/cloud-storage/properties_db_client/.sqlx/query-d7b0cf2d0add26c4c959e055b70f8ee8faf694757d7187135602b7508f0b0eca.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n SELECT \n ep.entity_id,\n ep.entity_type as \"entity_type: EntityType\"\n FROM entity_properties ep\n JOIN property_definitions pd ON ep.property_definition_id = pd.id\n WHERE ep.id = $1\n AND pd.is_system = FALSE\n ", + "query": "\n SELECT \n ep.entity_id,\n ep.entity_type as \"entity_type: EntityType\",\n ep.property_definition_id\n FROM entity_properties ep\n WHERE ep.id = $1\n ", "describe": { "columns": [ { @@ -28,6 +28,11 @@ } } } + }, + { + "ordinal": 2, + "name": "property_definition_id", + "type_info": "Uuid" } ], "parameters": { @@ -36,9 +41,10 @@ ] }, "nullable": [ + false, false, false ] }, - "hash": "17bcb8a4765cf0bfc406a98a2a8829342f248afbdfddef55ef1f44aa4c9b034e" + "hash": "d7b0cf2d0add26c4c959e055b70f8ee8faf694757d7187135602b7508f0b0eca" } diff --git a/rust/cloud-storage/properties_db_client/src/entity_properties/get.rs b/rust/cloud-storage/properties_db_client/src/entity_properties/get.rs index 4d219648c42..a67acf07c61 100644 --- a/rust/cloud-storage/properties_db_client/src/entity_properties/get.rs +++ b/rust/cloud-storage/properties_db_client/src/entity_properties/get.rs @@ -9,7 +9,7 @@ use models_properties::service::entity_property::EntityProperty; use models_properties::service::entity_property_with_definition::EntityPropertyWithDefinition; use models_properties::service::property_definition::PropertyDefinition; use models_properties::service::property_value::PropertyValue; -use models_properties::{DataType, EntityReference, EntityType}; +use models_properties::{DataType, EntityPropertyReference, EntityReference, EntityType}; type Result = std::result::Result; @@ -228,34 +228,31 @@ async fn attach_property_options( Ok(()) } -/// Gets the entity_id and entity_type for a given entity_property_id. -/// Used for permission checking before deletion. -/// -/// Note: This excludes system properties (is_system = TRUE). System properties -/// cannot be deleted, so this function will return None for them. +/// Looks up an entity property by its ID. +/// Returns entity reference (for permissions) and definition ID (for required property check). #[tracing::instrument(skip(db))] -pub async fn get_entity_type_from_entity_property( +pub async fn lookup_entity_property( db: &Pool, entity_property_id: Uuid, -) -> Result> { +) -> Result> { let row = sqlx::query!( r#" SELECT ep.entity_id, - ep.entity_type as "entity_type: EntityType" + ep.entity_type as "entity_type: EntityType", + ep.property_definition_id FROM entity_properties ep - JOIN property_definitions pd ON ep.property_definition_id = pd.id WHERE ep.id = $1 - AND pd.is_system = FALSE "#, entity_property_id ) .fetch_optional(db) .await?; - Ok(row.map(|r| EntityReference { + Ok(row.map(|r| EntityPropertyReference { entity_id: r.entity_id, entity_type: r.entity_type, + property_definition_id: r.property_definition_id, })) } @@ -658,18 +655,24 @@ mod tests { migrator = "MACRO_DB_MIGRATIONS", fixtures(path = "../../fixtures", scripts("properties")) )] - async fn test_get_entity_type_from_entity_property(pool: Pool) -> anyhow::Result<()> { + async fn test_lookup_entity_property(pool: Pool) -> anyhow::Result<()> { const _: &sqlx::migrate::Migrator = &MACRO_DB_MIGRATIONS; let entity_property_id = "e0111111-1111-1111-1111-111111111111" .parse::() .unwrap(); - let entity_ref = get_entity_type_from_entity_property(&pool, entity_property_id).await?; + let info = lookup_entity_property(&pool, entity_property_id).await?; - assert!(entity_ref.is_some()); - let entity_ref = entity_ref.unwrap(); - assert_eq!(entity_ref.entity_id, "doc1"); - assert_eq!(entity_ref.entity_type, EntityType::Document); + assert!(info.is_some()); + let info = info.unwrap(); + assert_eq!(info.entity_id, "doc1"); + assert_eq!(info.entity_type, EntityType::Document); + assert_eq!( + info.property_definition_id, + "11111111-1111-1111-1111-111111111111" + .parse::() + .unwrap() + ); Ok(()) } @@ -678,17 +681,15 @@ mod tests { migrator = "MACRO_DB_MIGRATIONS", fixtures(path = "../../fixtures", scripts("properties")) )] - async fn test_get_entity_type_from_entity_property_not_found( - pool: Pool, - ) -> anyhow::Result<()> { + async fn test_lookup_entity_property_not_found(pool: Pool) -> anyhow::Result<()> { const _: &sqlx::migrate::Migrator = &MACRO_DB_MIGRATIONS; let entity_property_id = "00000000-0000-0000-0000-000000000000" .parse::() .unwrap(); - let entity_ref = get_entity_type_from_entity_property(&pool, entity_property_id).await?; + let info = lookup_entity_property(&pool, entity_property_id).await?; - assert!(entity_ref.is_none()); + assert!(info.is_none()); Ok(()) } diff --git a/rust/cloud-storage/properties_service/Cargo.toml b/rust/cloud-storage/properties_service/Cargo.toml index 3ecff76d223..103afae96a5 100644 --- a/rust/cloud-storage/properties_service/Cargo.toml +++ b/rust/cloud-storage/properties_service/Cargo.toml @@ -38,6 +38,7 @@ models_permissions = { path = "../models_permissions" } models_properties = { path = "../models_properties" } properties_db_client = { path = "../properties_db_client" } secretsmanager_client = { path = "../secretsmanager_client" } +system_properties = { path = "../system_properties" } serde = { workspace = true } serde_json = { workspace = true } serde_with = { workspace = true } diff --git a/rust/cloud-storage/properties_service/src/api/permissions.rs b/rust/cloud-storage/properties_service/src/api/permissions.rs index 6ececad7ea9..394bb817baa 100644 --- a/rust/cloud-storage/properties_service/src/api/permissions.rs +++ b/rust/cloud-storage/properties_service/src/api/permissions.rs @@ -76,7 +76,8 @@ async fn get_access_level( EntityType::Project => "project", EntityType::Thread => "thread", EntityType::Channel => "channel", - EntityType::Company | EntityType::Task | EntityType::User => { + EntityType::Task => "document", + EntityType::Company | EntityType::User => { tracing::warn!("property operations not supported for this entity type"); return Err(PermissionError::UnsupportedEntityType); } diff --git a/rust/cloud-storage/properties_service/src/api/properties/entities/delete_property.rs b/rust/cloud-storage/properties_service/src/api/properties/entities/delete_property.rs index 735ca1a51ad..c3ad50b63ed 100644 --- a/rust/cloud-storage/properties_service/src/api/properties/entities/delete_property.rs +++ b/rust/cloud-storage/properties_service/src/api/properties/entities/delete_property.rs @@ -3,15 +3,15 @@ use axum::{ http::StatusCode, response::{IntoResponse, Response}, }; +use models_properties::EntityReference; +use system_properties::SystemPropertyKey; use thiserror::Error; use uuid::Uuid; use crate::api::context::ApiContext; use model::user::UserContext; use properties_db_client::{ - entity_properties::{ - delete as entity_properties_delete, get::get_entity_type_from_entity_property, - }, + entity_properties::{delete as entity_properties_delete, get::lookup_entity_property}, error::PropertiesDatabaseError, }; @@ -25,6 +25,8 @@ pub enum DeleteEntityPropertyErr { Permission(#[from] crate::api::permissions::PermissionError), #[error("Entity property not found")] NotFound, + #[error("This property is required and cannot be removed from this entity")] + RequiredProperty, } impl IntoResponse for DeleteEntityPropertyErr { @@ -34,6 +36,7 @@ impl IntoResponse for DeleteEntityPropertyErr { | DeleteEntityPropertyErr::DatabaseError(_) => StatusCode::INTERNAL_SERVER_ERROR, DeleteEntityPropertyErr::Permission(e) => e.status_code(), DeleteEntityPropertyErr::NotFound => StatusCode::NOT_FOUND, + DeleteEntityPropertyErr::RequiredProperty => StatusCode::FORBIDDEN, }; if status_code.is_server_error() { @@ -57,6 +60,7 @@ impl IntoResponse for DeleteEntityPropertyErr { ), responses( (status = 204, description = "Entity property removed successfully"), + (status = 403, description = "Property is required and cannot be removed"), (status = 404, description = "Entity property not found"), (status = 500, description = "Internal server error") ), @@ -70,10 +74,8 @@ pub async fn delete_entity_property( ) -> Result { tracing::info!("removing entity property"); - // Get entity property metadata to check permissions - // Note: get_entity_type_from_entity_property excludes system properties, - // so system properties will return NotFound here. - let entity_ref = get_entity_type_from_entity_property(&context.db, entity_property_uuid) + // Lookup entity property + let property_info = lookup_entity_property(&context.db, entity_property_uuid) .await .inspect_err(|e| { tracing::error!( @@ -84,11 +86,30 @@ pub async fn delete_entity_property( .ok_or(DeleteEntityPropertyErr::NotFound)?; tracing::debug!( - entity_id = %entity_ref.entity_id, - entity_type = ?entity_ref.entity_type, - "fetched entity property metadata" + entity_id = %property_info.entity_id, + entity_type = ?property_info.entity_type, + property_definition_id = %property_info.property_definition_id, + "fetched entity property info" ); + // Check if this property is required for the entity type (e.g., Task properties) + if SystemPropertyKey::is_required_for_entity( + property_info.property_definition_id, + property_info.entity_type, + ) { + tracing::warn!( + entity_type = ?property_info.entity_type, + property_definition_id = %property_info.property_definition_id, + "attempted to remove required property" + ); + return Err(DeleteEntityPropertyErr::RequiredProperty); + } + + let entity_ref = EntityReference { + entity_id: property_info.entity_id, + entity_type: property_info.entity_type, + }; + crate::api::permissions::check_entity_edit_permission( &context, &user_context.user_id, diff --git a/rust/cloud-storage/properties_service/src/api/properties/entities/get.rs b/rust/cloud-storage/properties_service/src/api/properties/entities/get.rs index e7b53e5f7ea..ba83bab85fa 100644 --- a/rust/cloud-storage/properties_service/src/api/properties/entities/get.rs +++ b/rust/cloud-storage/properties_service/src/api/properties/entities/get.rs @@ -102,10 +102,11 @@ pub async fn get_entity_properties( ), async { match entity_type { - EntityType::Document => { + EntityType::Document | EntityType::Task => { super::super::metadata::get_document_metadata_properties( &context.db, &entity_id, + entity_type, ) .await } diff --git a/rust/cloud-storage/properties_service/src/api/properties/metadata.rs b/rust/cloud-storage/properties_service/src/api/properties/metadata.rs index a5ba3a67c62..ecde6e08de3 100644 --- a/rust/cloud-storage/properties_service/src/api/properties/metadata.rs +++ b/rust/cloud-storage/properties_service/src/api/properties/metadata.rs @@ -22,6 +22,7 @@ pub enum MetadataError { pub async fn get_document_metadata_properties( db: &Pool, document_id: &str, + entity_type: EntityType, ) -> Result, MetadataError> { tracing::info!("getting document metadata properties from macrodb"); @@ -54,7 +55,7 @@ pub async fn get_document_metadata_properties( metadata::DOCUMENT_NAME, models_properties::DataType::String, document_metadata.name, - EntityType::Document, + entity_type, )); } @@ -68,7 +69,8 @@ pub async fn get_document_metadata_properties( metadata::OWNER, models_properties::DataType::Entity, owner_entity_ref, - EntityType::Document, + entity_type, + Some(EntityType::User), )); } @@ -77,7 +79,7 @@ pub async fn get_document_metadata_properties( metadata::CREATED_AT, models_properties::DataType::Date, document_metadata.created_at, - EntityType::Document, + entity_type, )); // 4. Last updated time property @@ -85,7 +87,7 @@ pub async fn get_document_metadata_properties( metadata::LAST_UPDATED, models_properties::DataType::Date, document_metadata.updated_at, - EntityType::Document, + entity_type, )); // 5. Project property @@ -98,14 +100,16 @@ pub async fn get_document_metadata_properties( metadata::PROJECT, models_properties::DataType::Entity, project_entity_ref, - EntityType::Document, + entity_type, + Some(EntityType::Project), )); } else { // Add project property with null value metadata_properties.push(create_metadata_property_null( metadata::PROJECT, models_properties::DataType::Entity, - EntityType::Document, + entity_type, + Some(EntityType::Project), )); } @@ -126,7 +130,13 @@ pub fn create_metadata_property_str( entity_type: EntityType, ) -> EntityPropertyWithDefinition { let property_value = PropertyValue::Str(value); - create_metadata_property_inner(display_name, data_type, Some(property_value), entity_type) + create_metadata_property_inner( + display_name, + data_type, + Some(property_value), + entity_type, + None, + ) } pub fn create_metadata_property_date( @@ -136,7 +146,13 @@ pub fn create_metadata_property_date( entity_type: EntityType, ) -> EntityPropertyWithDefinition { let property_value = PropertyValue::Date(value); - create_metadata_property_inner(display_name, data_type, Some(property_value), entity_type) + create_metadata_property_inner( + display_name, + data_type, + Some(property_value), + entity_type, + None, + ) } pub fn create_metadata_property_entity_ref( @@ -144,9 +160,16 @@ pub fn create_metadata_property_entity_ref( data_type: models_properties::DataType, value: EntityReference, entity_type: EntityType, + specific_entity_type: Option, ) -> EntityPropertyWithDefinition { let property_value = PropertyValue::EntityRef(vec![value]); - create_metadata_property_inner(display_name, data_type, Some(property_value), entity_type) + create_metadata_property_inner( + display_name, + data_type, + Some(property_value), + entity_type, + specific_entity_type, + ) } fn create_metadata_property_inner( @@ -154,6 +177,7 @@ fn create_metadata_property_inner( data_type: models_properties::DataType, value: Option, entity_type: EntityType, + specific_entity_type: Option, ) -> EntityPropertyWithDefinition { // Metadata properties are computed on-the-fly and never persisted // Use System owner since they don't belong to any user or org @@ -165,7 +189,7 @@ fn create_metadata_property_inner( display_name: display_name.to_string(), data_type, is_multi_select: false, - specific_entity_type: None, + specific_entity_type, created_at: chrono::Utc::now(), updated_at: chrono::Utc::now(), is_system: false, // Metadata properties are not DB-stored system properties @@ -194,6 +218,13 @@ pub fn create_metadata_property_null( property_name: &str, data_type: models_properties::DataType, entity_type: EntityType, + specific_entity_type: Option, ) -> EntityPropertyWithDefinition { - create_metadata_property_inner(property_name, data_type, None, entity_type) + create_metadata_property_inner( + property_name, + data_type, + None, + entity_type, + specific_entity_type, + ) } diff --git a/rust/cloud-storage/system_properties/src/domain/model/constants/system_property_key.rs b/rust/cloud-storage/system_properties/src/domain/model/constants/system_property_key.rs index 478de788ddb..6cc0a768f08 100644 --- a/rust/cloud-storage/system_properties/src/domain/model/constants/system_property_key.rs +++ b/rust/cloud-storage/system_properties/src/domain/model/constants/system_property_key.rs @@ -1,5 +1,6 @@ //! System property key enum. +use models_properties::EntityType; use serde::{Deserialize, Serialize}; use uuid::Uuid; @@ -116,4 +117,34 @@ impl SystemPropertyKey { pub fn is_system_uuid(uuid: Uuid) -> bool { Self::from_uuid(uuid).is_some() } + + /// Returns the property definition IDs that are required (cannot be removed) for a given entity type. + /// These are fundamental built-in properties that define the entity's core behavior. + /// + /// Extensible: Add new entity types and their required properties here as needed. + pub fn required_property_ids_for_entity(entity_type: EntityType) -> &'static [Uuid] { + match entity_type { + EntityType::Task => &[ + Self::ASSIGNEES_UUID, + Self::STATUS_UUID, + Self::PRIORITY_UUID, + Self::DUE_DATE_UUID, + Self::PARENT_TASK_UUID, + Self::SUBTASKS_UUID, + Self::DEPENDS_ON_UUID, + Self::EFFORT_UUID, + Self::STORY_POINTS_UUID, + Self::RELEVANT_DOCUMENTS_UUID, + ], + // Other entity types don't have required properties yet + // Add new cases here as needed: + // EntityType::Email => &[...], + _ => &[], + } + } + + /// Check if a property definition cannot be removed from the given entity type. + pub fn is_required_for_entity(property_definition_id: Uuid, entity_type: EntityType) -> bool { + Self::required_property_ids_for_entity(entity_type).contains(&property_definition_id) + } }