diff --git a/sn_client/src/api.rs b/sn_client/src/api.rs index ab485d020e..2a1e2292a6 100644 --- a/sn_client/src/api.rs +++ b/sn_client/src/api.rs @@ -1150,7 +1150,7 @@ fn merge_split_register_records( // merge it with the others if they are valid let register: SignedRegister = all_registers.into_iter().fold(one_valid_reg, |mut acc, r| { - if acc.verified_merge(r).is_err() { + if acc.verified_merge(&r).is_err() { warn!("Skipping register that failed to merge. Entry found for {key:?}"); } acc @@ -1225,7 +1225,7 @@ mod tests { // test with 2 valid records: should return the two merged let mut expected_merge = signed_register1.clone(); - expected_merge.merge(signed_register2)?; + expected_merge.merge(&signed_register2)?; let map = HashMap::from_iter(vec![ (xorname1, (record1.clone(), peers1.clone())), (xorname2, (record2, peers2.clone())), diff --git a/sn_client/src/register.rs b/sn_client/src/register.rs index 36223ce8ee..3758d9e3ff 100644 --- a/sn_client/src/register.rs +++ b/sn_client/src/register.rs @@ -541,7 +541,7 @@ impl ClientRegister { self.register.clone() } }; - self.register.merge(remote_replica); + self.register.merge(&remote_replica)?; self.push(verify_store).await?; Ok((storage_cost, royalties_fees)) diff --git a/sn_node/src/put_validation.rs b/sn_node/src/put_validation.rs index 769bfbb223..6bf33de5ad 100644 --- a/sn_node/src/put_validation.rs +++ b/sn_node/src/put_validation.rs @@ -600,7 +600,7 @@ impl Node { // merge the two registers let mut merged_register = local_register.clone(); - merged_register.verified_merge(register.to_owned())?; + merged_register.verified_merge(register)?; if merged_register == local_register { trace!("Register with addr {reg_addr:?} is the same as the local version"); Ok(None) diff --git a/sn_registers/src/register.rs b/sn_registers/src/register.rs index 5d0e46bdac..a3f319e87c 100644 --- a/sn_registers/src/register.rs +++ b/sn_registers/src/register.rs @@ -94,22 +94,20 @@ impl SignedRegister { } /// Merge two SignedRegisters - pub fn merge(&mut self, other: SignedRegister) -> Result<()> { - if self.base_register != other.base_register { - return Err(Error::DifferentBaseRegister); - } - self.ops.extend(other.ops); + pub fn merge(&mut self, other: &Self) -> Result<()> { + self.base_register + .verify_is_mergeable(&other.base_register)?; + self.ops.extend(other.ops.clone()); Ok(()) } /// Merge two SignedRegisters but verify the incoming content /// Significantly slower than merge, use when you want to trust but verify the `other` - pub fn verified_merge(&mut self, other: SignedRegister) -> Result<()> { - if self.base_register != other.base_register { - return Err(Error::DifferentBaseRegister); - } + pub fn verified_merge(&mut self, other: &Self) -> Result<()> { + self.base_register + .verify_is_mergeable(&other.base_register)?; other.verify()?; - self.ops.extend(other.ops); + self.ops.extend(other.ops.clone()); Ok(()) } @@ -190,11 +188,6 @@ impl Register { self.crdt.get(hash).ok_or(Error::NoSuchEntry(hash)) } - /// Return a value corresponding to the provided 'hash', if present. - pub fn get_cloned(&self, hash: EntryHash) -> Result { - self.crdt.get(hash).cloned().ok_or(Error::NoSuchEntry(hash)) - } - /// Read the last entry, or entries when there are branches, if the register is not empty. pub fn read(&self) -> BTreeSet<(EntryHash, Entry)> { self.crdt.read() @@ -215,6 +208,8 @@ impl Register { signer: &SecretKey, ) -> Result<(EntryHash, RegisterOp)> { self.check_entry_and_reg_sizes(&entry)?; + // check permissions before writing on the underlying CRDT + self.check_user_permissions(signer.public_key())?; let (hash, address, crdt_op) = self.crdt.write(entry, children)?; let op = RegisterOp::new(address, crdt_op, signer); Ok((hash, op)) @@ -228,8 +223,10 @@ impl Register { } /// Merge another Register into this one. - pub fn merge(&mut self, other: Self) { - self.crdt.merge(other.crdt); + pub fn merge(&mut self, other: &Self) -> Result<()> { + self.verify_is_mergeable(other)?; + self.crdt.merge(other.crdt.clone()); + Ok(()) } /// Check if a register op is valid for our current register @@ -275,10 +272,20 @@ impl Register { Ok(()) } + + // Private helper to check if this Register is mergeable with another + fn verify_is_mergeable(&self, other: &Self) -> Result<()> { + if self.address() != other.address() || self.permissions != other.permissions { + return Err(Error::DifferentBaseRegister); + } + Ok(()) + } } #[cfg(test)] mod tests { + use crate::RegisterOp; + use super::{ EntryHash, Error, Permissions, Register, RegisterAddress, Result, MAX_REG_NUM_ENTRIES, }; @@ -346,38 +353,73 @@ mod tests { let meta: XorName = xor_name::rand::random(); let item = random_register_entry(); - // create a Register with the owner as the only allowed to write - let mut replica = Register::new(owner, meta, Permissions::default()); + // Create replicas where anyone can write to them, including the owner ofc + let mut replica1 = Register::new(owner, meta, Permissions::new_anyone_can_write()); + let mut replica2 = replica1.clone(); + let mut signed_replica3 = replica1.clone().into_signed(&owner_sk)?; + // ...owner and the other user can both write to them + let (_, op1) = replica1.write(item.clone(), &BTreeSet::new(), &owner_sk)?; + let (_, op2) = replica1.write(item.clone(), &BTreeSet::new(), &other_user_sk)?; + replica2.apply_op(op1)?; + replica2.apply_op(op2)?; + signed_replica3.verified_merge(&replica2.into_signed(&owner_sk)?)?; - // owner can write to it - let (_, op) = replica.write(item.clone(), &BTreeSet::new(), &owner_sk)?; - replica.apply_op(op)?; + // Create replicas allowing both the owner and other user to write to them + let mut replica1 = Register::new(owner, meta, Permissions::new_with([other_user])); + let mut replica2 = replica1.clone(); + let mut signed_replica3 = replica1.clone().into_signed(&owner_sk)?; + // ...owner and the other user can both write to them + let (_, op1) = replica1.write(item.clone(), &BTreeSet::new(), &owner_sk)?; + let (_, op2) = replica1.write(item.clone(), &BTreeSet::new(), &other_user_sk)?; + replica2.apply_op(op1)?; + replica2.apply_op(op2)?; + signed_replica3.verified_merge(&replica2.into_signed(&owner_sk)?)?; - // other user cannot write to it - let (_, op) = replica.write(item.clone(), &BTreeSet::new(), &other_user_sk)?; - let res = replica.apply_op(op); + // Create replicas with the owner as the only allowed to write + let mut replica1 = Register::new(owner, meta, Permissions::default()); + let mut replica2 = replica1.clone(); + // ...owner can write to them + let (_, op) = replica1.write(item.clone(), &BTreeSet::new(), &owner_sk)?; + replica2.apply_op(op.clone())?; + // ...whilst other user cannot write to them + let res = replica1.write(item.clone(), &BTreeSet::new(), &other_user_sk); + assert!( + matches!(&res, Err(err) if err == &Error::AccessDenied(other_user)), + "Unexpected result: {res:?}" + ); + let (_, address, crdt_op) = replica1.crdt.write(item.clone(), &BTreeSet::new())?; + let op_signed_by_other_user = RegisterOp::new(address, crdt_op, &other_user_sk); + let res = replica2.apply_op(op_signed_by_other_user); assert!( matches!(&res, Err(err) if err == &Error::AccessDenied(other_user)), "Unexpected result: {res:?}" ); - // create a Register allowing both the owner and other user to write to it - let mut replica2 = Register::new(owner, meta, Permissions::new_with([other_user])); - - // owner and the other user can both write to it - let (_, op1) = replica2.write(item.clone(), &BTreeSet::new(), &owner_sk)?; - let (_, op2) = replica2.write(item.clone(), &BTreeSet::new(), &other_user_sk)?; - replica2.apply_op(op1)?; - replica2.apply_op(op2)?; - - // create a Register where anyone can write to it, including the owner ofc - let mut replica3 = Register::new(owner, meta, Permissions::new_anyone_can_write()); + // Create Registers with different permissions to write + let mut reg1 = Register::new(owner, meta, Permissions::default()); + let mut reg2 = Register::new(owner, meta, Permissions::new_with([other_user])); + // ...owner can write to both of them, the other user only to one of them + reg1.write(item.clone(), &BTreeSet::new(), &owner_sk)?; + reg2.write(item.clone(), &BTreeSet::new(), &owner_sk)?; + reg2.write(item.clone(), &BTreeSet::new(), &other_user_sk)?; + // ...but they cannot be merged due to different permissions sets + let res1 = reg1.merge(®2); + let res2 = reg2.merge(®1); + assert!( + matches!(&res1, Err(err) if err == &Error::DifferentBaseRegister), + "Unexpected result: {res1:?}" + ); + assert_eq!(res1, res2); - // owner and the other user can both write to it - let (_, op1) = replica3.write(item.clone(), &BTreeSet::new(), &owner_sk)?; - let (_, op2) = replica3.write(item, &BTreeSet::new(), &other_user_sk)?; - replica3.apply_op(op1)?; - replica3.apply_op(op2)?; + let mut signed_reg1 = reg1.into_signed(&owner_sk)?; + let mut signed_reg2 = reg2.into_signed(&owner_sk)?; + let res1 = signed_reg1.verified_merge(&signed_reg2); + let res2 = signed_reg2.verified_merge(&signed_reg1); + assert!( + matches!(&res1, Err(err) if err == &Error::DifferentBaseRegister), + "Unexpected result: {res1:?}" + ); + assert_eq!(res1, res2); Ok(()) }