-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(2631): Add e2e encryption/decryption to messages #2733
base: develop
Are you sure you want to change the base?
Conversation
…e stored in orbitdb
* Add slack notifications on release outcome * Update CHANGELOG.md
…e the native decoder breaks on mobile (#2728)
* handle permissions errors during invite creation and display appropriate screens when non-admins try to admit users * handle slow generating links and admin only features in mobile * update changelog * make import relative
* User correct output variable for version on linux build * Update language on non-admin invite pages * Fix version outputs on everything else
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.
Looks good overall. Approving, but there's some comments that would be nice to have clarified before you merge.
this.logger.info(`${this.channelData.id} database updated`, entry.hash, entry.payload.value?.channelId) | ||
|
||
const message = await this.messagesService.onConsume(entry.payload.value!) | ||
if (message == null) { | ||
this.logger.error(`Couldn't consume message ${entry.payload.value!.id}`) | ||
return |
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 this could lead to messages being dropped from the local redux store permanently if the peer receives a message encrypted with a version of the team key that they haven't yet added to their sigchain. Do we need some method of caching messages that we fail to decrypt, and then retrying their decryption when a new team key is added to the sigchain maybe?
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.
Unless there's some other place in the code I haven't looked at yet where we pull all the messages in the store on some regular basis that is.
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.
This is handled already by the state manager. When we get a new message (and possibly elsewhere) it sends all message IDs up to the state manager and if anything is missing from redux it fetches it.
|
||
for await (const x of this.getStore().iterator()) { | ||
if (ids == null || ids?.includes(x.value.id)) { | ||
// NOTE: we skipped the verification process when reading many messages in the previous version | ||
// so I'm skipping it here - is that really the correct behavior? | ||
const processedMessage = await this.messagesService.onConsume(x.value, false) | ||
messages.push(processedMessage) | ||
const decryptedMessage = await this.messagesService.onConsume(x.value) |
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.
Is the comment above still relevant? It seems like we're doing some verification by proving we can decrypt the message, or is the verification you're talking about something else?
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.
Also, is there maybe some utility in adding a parameter to the getEntries method to skip decryption and get the raw encrypted entries? Not sure if we'll need that, but could be a nice to have.
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.
Yea that's not relevant anymore.
Not currently but it would be possible to add.
Pull Request Checklist
(Optional) Mobile checklist
Please ensure you completed the following checks if you did any changes to the mobile package: