diff --git a/core/station/impl/src/factories/requests/add_address_book_entry.rs b/core/station/impl/src/factories/requests/add_address_book_entry.rs index a0d7d7cc8..63dd55f9c 100644 --- a/core/station/impl/src/factories/requests/add_address_book_entry.rs +++ b/core/station/impl/src/factories/requests/add_address_book_entry.rs @@ -48,7 +48,10 @@ impl<'p, 'o> AddAddressBookEntryRequestExecute<'p, 'o> { impl Execute for AddAddressBookEntryRequestExecute<'_, '_> { async fn execute(&self) -> Result { let address_book_entry = ADDRESS_BOOK_SERVICE - .create_entry(self.operation.input.to_owned()) + .create_entry( + self.operation.input.to_owned(), + Some(self.request.requested_by), + ) .await .map_err(|e| RequestExecuteError::Failed { reason: format!("Failed to create address book entry: {e}"), diff --git a/core/station/impl/src/factories/requests/edit_address_book_entry.rs b/core/station/impl/src/factories/requests/edit_address_book_entry.rs index c5f711f2c..bea07c70a 100644 --- a/core/station/impl/src/factories/requests/edit_address_book_entry.rs +++ b/core/station/impl/src/factories/requests/edit_address_book_entry.rs @@ -61,7 +61,10 @@ impl<'p, 'o> EditAddressBookEntryRequestExecute<'p, 'o> { impl Execute for EditAddressBookEntryRequestExecute<'_, '_> { async fn execute(&self) -> Result { ADDRESS_BOOK_SERVICE - .edit_entry(self.operation.input.to_owned()) + .edit_entry( + self.operation.input.to_owned(), + Some(self.request.requested_by), + ) .await .map_err(|e| RequestExecuteError::Failed { reason: format!("Failed to update address book entry: {e}"), diff --git a/core/station/impl/src/mappers/address_book.rs b/core/station/impl/src/mappers/address_book.rs index 2cf0ea57b..d428b42b7 100644 --- a/core/station/impl/src/mappers/address_book.rs +++ b/core/station/impl/src/mappers/address_book.rs @@ -39,6 +39,7 @@ impl AddressBookMapper { pub fn from_create_input( input: AddAddressBookEntryOperationInput, entry_id: UUID, + created_by: Option, ) -> Result { let new_entry = AddressBookEntry { id: entry_id, @@ -49,6 +50,7 @@ impl AddressBookMapper { labels: input.labels, metadata: input.metadata.into(), last_modification_timestamp: next_time(), + last_modified_by: created_by, }; Ok(new_entry) diff --git a/core/station/impl/src/migration_tests/mod.rs b/core/station/impl/src/migration_tests/mod.rs index 5d8132302..7f97c74ef 100644 --- a/core/station/impl/src/migration_tests/mod.rs +++ b/core/station/impl/src/migration_tests/mod.rs @@ -53,6 +53,7 @@ mod test { blockchain: crate::models::Blockchain::InternetComputer, labels: vec!["Alice".to_string(), "Bob".to_string()], last_modification_timestamp: 0, + last_modified_by: None, metadata: Metadata::default(), }, AddressBookEntry { @@ -63,6 +64,7 @@ mod test { blockchain: crate::models::Blockchain::InternetComputer, labels: vec!["Alice".to_string(), "Bob".to_string()], last_modification_timestamp: 0, + last_modified_by: None, metadata: Metadata::new( [ ("key1".to_string(), "value1".to_string()), diff --git a/core/station/impl/src/migration_tests/snapshots/address_book_repository_v3.bin b/core/station/impl/src/migration_tests/snapshots/address_book_repository_v3.bin index 9d9b778e9..9f16d3e73 100644 Binary files a/core/station/impl/src/migration_tests/snapshots/address_book_repository_v3.bin and b/core/station/impl/src/migration_tests/snapshots/address_book_repository_v3.bin differ diff --git a/core/station/impl/src/models/address_book.rs b/core/station/impl/src/models/address_book.rs index e72ce5a8d..05700079d 100644 --- a/core/station/impl/src/models/address_book.rs +++ b/core/station/impl/src/models/address_book.rs @@ -34,6 +34,13 @@ pub struct AddressBookEntry { pub labels: Vec, /// The last time the record was updated or created. pub last_modification_timestamp: Timestamp, + /// The user that last created or edited this entry. + /// + /// `AllowListed` treats presence in the address book as approval for a transfer, so this is + /// used to stop the same user both listing an address and spending to it. `None` on entries + /// written before this field existed. + #[serde(default)] + pub last_modified_by: Option, } #[storable] @@ -159,6 +166,45 @@ mod tests { use super::address_book_entry_test_utils::mock_address_book_entry; use super::*; + /// `last_modified_by` was added after entries were already in stable memory. Entries are + /// stored as CBOR, so a record written without the field must still decode, with the field + /// defaulting to `None` rather than trapping the upgrade. + #[test] + fn decodes_entries_stored_before_last_modified_by_existed() { + use ic_stable_structures::Storable; + + #[derive(serde::Serialize)] + struct LegacyAddressBookEntry { + id: AddressBookEntryId, + address_owner: String, + address: String, + blockchain: Blockchain, + address_format: AddressFormat, + metadata: Metadata, + labels: Vec, + last_modification_timestamp: Timestamp, + } + + let legacy = LegacyAddressBookEntry { + id: [7; 16], + address_owner: "Alice".to_string(), + address: "0x1234".to_string(), + blockchain: Blockchain::InternetComputer, + address_format: AddressFormat::ICPAccountIdentifier, + metadata: Metadata::default(), + labels: vec!["counterparty".to_string()], + last_modification_timestamp: 42, + }; + + let bytes = serde_cbor::to_vec(&legacy).expect("Failed to encode legacy entry"); + let decoded = AddressBookEntry::from_bytes(std::borrow::Cow::Owned(bytes)); + + assert_eq!(decoded.last_modified_by, None); + assert_eq!(decoded.address, "0x1234"); + assert_eq!(decoded.labels, vec!["counterparty".to_string()]); + assert_eq!(decoded.last_modification_timestamp, 42); + } + #[test] fn test_address_book_entry_validation() { let address_book_entry = mock_address_book_entry(); @@ -274,6 +320,7 @@ pub mod address_book_entry_test_utils { blockchain: Blockchain::InternetComputer, metadata: Metadata::mock(), last_modification_timestamp: 0, + last_modified_by: None, } } diff --git a/core/station/impl/src/models/request_policy_rule.rs b/core/station/impl/src/models/request_policy_rule.rs index ddbc925f2..09d376816 100644 --- a/core/station/impl/src/models/request_policy_rule.rs +++ b/core/station/impl/src/models/request_policy_rule.rs @@ -564,10 +564,19 @@ impl continue; }; - let is_in_address_book = ADDRESS_BOOK_REPOSITORY - .exists(asset.blockchain, transfer.input.to.clone()); + // An entry the requester listed themselves is not independent + // evidence that the destination is trusted: address book writes + // sit at a lower approval tier than transfers by default, so + // honouring them would let one user both allow-list an address + // and spend to it. Entries written before this was tracked have + // no author and are still honoured. + let listed_independently = ADDRESS_BOOK_REPOSITORY + .find_by_address(asset.blockchain, transfer.input.to.clone()) + .is_some_and(|entry| { + entry.last_modified_by != Some(request.requested_by) + }); - if is_in_address_book { + if listed_independently { return Ok(RequestPolicyRuleResult { status: EvaluationStatus::Approved, evaluated_rule: EvaluatedRequestPolicyRule::AllowListed, @@ -973,3 +982,103 @@ mod test { ); } } + +#[cfg(test)] +mod allow_listed_tests { + use super::*; + use crate::core::test_utils::init_canister_system; + use crate::models::{ + account_test_utils::mock_account, + address_book_entry_test_utils::mock_address_book_entry, + asset_test_utils::mock_asset, + request_specifier::{AddressBookMetadataMatcher, UserMatcher}, + request_test_utils::mock_request, + AccountAsset, Metadata, RequestOperation, TokenStandard, TransferOperation, + TransferOperationInput, + }; + use crate::repositories::{ACCOUNT_REPOSITORY, ADDRESS_BOOK_REPOSITORY, ASSET_REPOSITORY}; + use orbit_essentials::repository::Repository; + use orbit_essentials::types::UUID; + + const DESTINATION: &str = "0xdeadbeef"; + + fn evaluator() -> RequestPolicyRuleEvaluator { + RequestPolicyRuleEvaluator { + user_matcher: Arc::new(UserMatcher), + address_book_metadata_matcher: Arc::new(AddressBookMetadataMatcher), + } + } + + /// Lists `DESTINATION` in the address book as `listed_by`, then returns a transfer request to + /// that destination submitted by `requested_by`. + fn transfer_to_listed_address(listed_by: Option, requested_by: UUID) -> Request { + init_canister_system(); + + let asset = mock_asset(); + ASSET_REPOSITORY.insert(asset.id, asset.clone()); + + let mut account = mock_account(); + account.assets = vec![AccountAsset { + asset_id: asset.id, + balance: None, + }]; + ACCOUNT_REPOSITORY.insert(account.to_key(), account.clone()); + + let mut entry = mock_address_book_entry(); + entry.blockchain = asset.blockchain.clone(); + entry.address = DESTINATION.to_string(); + entry.last_modified_by = listed_by; + ADDRESS_BOOK_REPOSITORY.insert(entry.to_key(), entry); + + let mut request = mock_request(); + request.requested_by = requested_by; + request.operation = RequestOperation::Transfer(TransferOperation { + fee: None, + transfer_id: None, + asset: asset.clone(), + input: TransferOperationInput { + from_account_id: account.id, + from_asset_id: asset.id, + with_standard: TokenStandard::InternetComputerNative, + to: DESTINATION.to_string(), + amount: 100u64.into(), + metadata: Metadata::default(), + network: "mainnet".to_string(), + fee: None, + }, + }); + + request + } + + fn evaluate(request: Request) -> EvaluationStatus { + evaluator() + .evaluate((Arc::new(request), Arc::new(RequestPolicyRule::AllowListed))) + .expect("Failed to evaluate AllowListed") + .status + } + + #[test] + fn approves_an_address_listed_by_someone_else() { + let request = transfer_to_listed_address(Some([1; 16]), [2; 16]); + + assert_eq!(evaluate(request), EvaluationStatus::Approved); + } + + /// Address book writes sit at a lower approval tier than transfers by default, so a user must + /// not be able to both list a destination and spend to it. + #[test] + fn rejects_an_address_the_requester_listed_themselves() { + let requester = [2; 16]; + let request = transfer_to_listed_address(Some(requester), requester); + + assert_eq!(evaluate(request), EvaluationStatus::Rejected); + } + + #[test] + fn approves_entries_that_predate_authorship_tracking() { + let request = transfer_to_listed_address(None, [2; 16]); + + assert_eq!(evaluate(request), EvaluationStatus::Approved); + } +} diff --git a/core/station/impl/src/services/address_book.rs b/core/station/impl/src/services/address_book.rs index bbbda5590..e6e66bc7d 100644 --- a/core/station/impl/src/services/address_book.rs +++ b/core/station/impl/src/services/address_book.rs @@ -16,7 +16,9 @@ use crate::{ repositories::{AddressBookRepository, AddressBookWhereClause, ADDRESS_BOOK_REPOSITORY}, }; use lazy_static::lazy_static; -use orbit_essentials::{api::ServiceResult, model::ModelValidator, repository::Repository}; +use orbit_essentials::{ + api::ServiceResult, model::ModelValidator, repository::Repository, types::UUID, +}; use station_api::PaginationInput; use std::sync::Arc; use uuid::Uuid; @@ -111,11 +113,13 @@ impl AddressBookService { pub async fn create_entry( &self, input: AddAddressBookEntryOperationInput, + created_by: Option, ) -> ServiceResult { let uuid = generate_uuid_v4().await; let key = AddressBookEntry::key(*uuid.as_bytes()); - let new_entry = AddressBookMapper::from_create_input(input.to_owned(), *uuid.as_bytes())?; + let new_entry = + AddressBookMapper::from_create_input(input.to_owned(), *uuid.as_bytes(), created_by)?; new_entry.validate()?; if let Some(v) = self @@ -139,6 +143,7 @@ impl AddressBookService { pub async fn edit_entry( &self, input: EditAddressBookEntryOperationInput, + edited_by: Option, ) -> ServiceResult { let mut entry = self.get_entry_by_id(&input.address_book_entry_id)?; @@ -150,6 +155,8 @@ impl AddressBookService { entry.metadata.change(change_metadata); } + entry.last_modified_by = edited_by; + entry.validate()?; self.address_book_repository @@ -215,7 +222,10 @@ mod tests { }, }; - let result = ctx.service.create_entry(operation.input.clone()).await; + let result = ctx + .service + .create_entry(operation.input.clone(), None) + .await; let new_entry = result.unwrap(); @@ -226,7 +236,7 @@ mod tests { // adding a new entry for the same address should fail - let result = ctx.service.create_entry(operation.input).await; + let result = ctx.service.create_entry(operation.input, None).await; result.unwrap_err(); } @@ -258,7 +268,7 @@ mod tests { )), labels: None, }; - let result = ctx.service.edit_entry(operation).await; + let result = ctx.service.edit_entry(operation, None).await; assert!(result.is_ok()); let updated_entry = result.unwrap(); address_book_entry.address_owner = "test_edit".to_string(); @@ -298,7 +308,7 @@ mod tests { )), labels: None, }; - let result = ctx.service.edit_entry(operation).await; + let result = ctx.service.edit_entry(operation, None).await; assert!(result.is_ok()); let updated_entry = result.unwrap(); address_book_entry.metadata = new_metadata_dto; @@ -315,7 +325,7 @@ mod tests { change_metadata: Some(ChangeMetadata::RemoveKeys(remove_keys)), labels: None, }; - let result = ctx.service.edit_entry(operation).await; + let result = ctx.service.edit_entry(operation, None).await; assert!(result.is_ok()); let updated_entry = result.unwrap(); address_book_entry.metadata = new_metadata_dto.into();