-
Notifications
You must be signed in to change notification settings - Fork 57
Conversation
streams/src/message/keyload.rs
Outdated
fork.mask(&mut psk_id)?; | ||
|
||
let mut masked_key = [0u8; KEY_SIZE]; | ||
if let Some(psk) = keyload.psk_store.get(&psk_id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could check here if the key has not yet been decrypted (key.is_none()
), in order to avoid decrypting it multiple times unnecessarily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great. Just one old comment pending to address, but everything else is ready to merge in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am i correct in thinking we dont really need PskId anymore? We should remove it in that case
#[cfg(feature = "did")] | ||
Identifier::DID(did) => { | ||
let oneof = Uint8::new(2); | ||
let oneof = Uint8::new(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is fine now since were doing a large rewrite, but we should keep in mind in the future, that we should not change these numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
self.send_keyload( | ||
link_to, | ||
// Alas, must collect to release the &self immutable borrow | ||
self.subscribers().map(Permissioned::Read).collect::<Vec<_>>(), | ||
subscribers, | ||
psks, | ||
) | ||
.await | ||
} | ||
|
||
pub async fn send_keyload_for_all_rw(&mut self, link_to: MsgId) -> Result<SendResponse<TSR>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm im not sure how this method passed review earlier. Did we get requested to add this method, or are we sure this will be needed in many cases? If thats the case, should we change the default? Alternatively, i think a way to specify the "Permisison type" per identifier is better. (not in this PR, but should be given thought. (I am referring to timed permissions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth addressing when we get to the high level API rewrite, because defining the Permissions
more directly will make more sense semantically once send_keyload
assumes the new change_permissions()
definition
@kwek20 #241 Arnau made this issue after this conversation: #237 (comment). What's your opinion? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave a comment on the issue regarding PskId, as this PR is just moving it, it can be done in a new one. (if we decide to remove it)
Description of change
Moves the
psks
HashMap out ofKeyStore
and intoUser
directly.Permissioned::Read
to each of them when masking keyType of change
How the change has been tested