fix: Rerun the full securejoin protocol if the address was outdated#8358
fix: Rerun the full securejoin protocol if the address was outdated#8358Hocuri wants to merge 7 commits into
Conversation
…heir singular relay and added a new one and then perform a securejoin
…a message actually works
…ropped their singular relay and added a new one and then perform a securejoin" This reverts commit a9d19bc.
Hocuri
changed the title
| Contact { | ||
| contact_id: ContactId, | ||
| fingerprint: Fingerprint, | ||
| addr: String, |
There was a problem hiding this comment.
QrInvite is serialized into the database, so this addr should likely be passed next to the QrInvite as a separate argument.
There was a problem hiding this comment.
Or you need to put #[serde(default)] on it for when it is deserialized from the bob state table.
| let has_up_to_date_key = if let Some(public_key_bytes) = public_key_bytes { | ||
| let public_key = SignedPublicKey::from_slice(&public_key_bytes)?; | ||
| let addrs_in_key = addresses_from_public_key(&public_key); | ||
| // The key is up to date if it contains all the addresses from the QR code: | ||
| addrs_in_key | ||
| .is_some_and(|addrs_in_key| invite.addrs().iter().all(|a| addrs_in_key.contains(a))) | ||
| } else { | ||
| false | ||
| }; |
There was a problem hiding this comment.
I'm a bit unsure whether I prefer this PR here or #8355. For example, this logic here is somewhat hard to read. OTOH, the fix here is more "self-contained" in the securejoin logic, rather than needing logic changes elsewhere.
There was a problem hiding this comment.
i think it's readable enough.
maybe rename the var to key_contains_all_invite_addrs and remove the comment? When i read "has_up_to_date_key" in other places i immediately wonder what that means concretely. It might also be that someone is scanning an old invite link, and the relay addresses in the Alice key that Bob has are actually the correct ones. #8355 would use both the key and the invite addresses but i think it's fine to only use invite addresses. This handshake would fail anyway if Alice has meanwhile changed all her relays.
iequidoo
left a comment
iequidoo
left a comment
There was a problem hiding this comment.
Overall looks easy to read, but link2xt's comment looked unobvious for me
hpk42
left a comment
hpk42
left a comment
There was a problem hiding this comment.
lgtm, also maybe make sense to to generate multi-address invite links soon.
| // The key is up to date iff it contains all the addresses from the QR code: | ||
| let has_up_to_date_key = if let Some(public_key_bytes) = public_key_bytes { | ||
| let public_key = SignedPublicKey::from_slice(&public_key_bytes)?; | ||
| let addrs_in_key = addresses_from_public_key(&public_key).unwrap_or_default(); |
There was a problem hiding this comment.
This unwrap_or_default() hides a corner case when we have a key for a key-contact, but the key has no addresses (because it does not have the notation subpacket). In this case if the address of the key-contact (the one in the contacts table which is usually the last seen From address and the address we are going to send messages to) is the same the address in the invite, there is no need to request a new key.
Can be fixed like this:
let inviter_addrs = if let Some(addrs_in_key) = addresses_from_public_key(&public_key) {
addrs_in_key
} else if let Some(contact_addr) = context.query_get_value("SELECT addr FROM contacts WHERE fingerprint=?", (invite.fingerprint().hex(),)).await? {
vec![contact_addr]
} else {
Vec::new()
}4 participants
Alternative to #8355, fixes #8329