-
Notifications
You must be signed in to change notification settings - Fork 216
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
E2e keys backup #591
E2e keys backup #591
Conversation
and MXEncryptedContentKey
- add optional antivirus server url - implement Matrix Content Scanner API
… on simulators for iOS 11 and higher
This is what could be done for #582.
To implement them, it required to add a primary key to MXOlmInboundGroupSession to MXRealmCryptoStore. Hopefully, this will make decrypting a bit quicker too.
…rivate backup key. We use https://github.com/caobo56/CBBase58, a pod that exposes only the Base58 part of https://github.com/oleganza/CoreBitcoin.
… is called with 100% before success
…nding state MXKeyBackupStateWrongBackUpVersion
…kup markers if the backup version has changed
…y fails on a bad backup state
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.
Some remarks
MatrixSDK/Crypto/Data/Store/MXRealmCryptoStore/MXRealmCryptoStore.m
Outdated
Show resolved
Hide resolved
|
||
RLMRealm *realm = self.realm; | ||
|
||
RLMResults<MXRealmOlmInboundGroupSession *> *realmSessions = [MXRealmOlmInboundGroupSession objectsInRealm:realm where:@"backedUp = NO"]; |
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.
can't you add the limit in the query with realm?
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.
With Realm lazy loading, I do not care too much but I will check.
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.
So, no. There is no pagination limit in realm-objc (realm/realm-swift#2608).
|
||
if (onlyBackedUp) | ||
{ | ||
realmSessions = [MXRealmOlmInboundGroupSession objectsInRealm:realm where:@"backedUp = YES"]; |
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.
isn't count method with Realm?
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.
Lazy loading again. There is only [RLMResults count].
MXStrongifyAndReturnIfNil(self); | ||
|
||
// Reset backup markers | ||
[self->mxSession.crypto.store resetBackupMarkers]; |
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.
Do this only on success?
return decryption; | ||
} | ||
|
||
- (MXKeyBackupData*)encryptGroupSession:(MXOlmInboundGroupSession*)session withPkEncryption:(OLMPkEncryption*)encryption |
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.
2nd param not used, you use _backupKey
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.
Fixed within the method body.
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 do not like to pass class members as parameter of method members if there is no specific reason. Is there a specific reason here?
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.
You are right. I guess it also means that these method could be moved to another util class.
Add some dev logs to check timing in this method
@param keyBackupVersion backup information object as returned by `[MXKeyBackup version]`. | ||
@return an error if the operation fails. | ||
*/ | ||
- (NSError*)enableKeyBackup:(MXKeyBackupVersion*)keyBackupVersion; |
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.
- (void)enableKeyBackup:(MXKeyBackupVersion*)keyBackupVersion error:(NSError **)error;
would be more standard and readable.
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.
Ok. This method is no more public. I left its signature as is for usability, implementability and readability.
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.
One bug and some minor remarks
return decryption; | ||
} | ||
|
||
- (MXKeyBackupData*)encryptGroupSession:(MXOlmInboundGroupSession*)session withPkEncryption:(OLMPkEncryption*)encryption |
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 do not like to pass class members as parameter of method members if there is no specific reason. Is there a specific reason here?
@@ -328,8 +338,9 @@ - (void)sendKeyBackup | |||
} | |||
else | |||
{ | |||
// Come back to the ready state so that we will retry on the next received key | |||
// Retry a bit later | |||
self.state = MXKeyBackupStateReadyToBackUp; |
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 there a risk of infinite retry in case of no network issue, or other permanent error?
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.
Yes but no because the MXHTTPClient
will wait that the network is back.
In case of other kind of error (like timeout on bad network connectivity), yes, we can loop but with a delay between 0 and 10s and it seems to be the best to do in this case.
# Conflicts: # MatrixSDK/MXRestClient.h # MatrixSDK/MXRestClient.m
# Conflicts: # MatrixSDK.xcodeproj/project.pbxproj
element-hq/element-ios#2070
This PR is inspired by matrix-org/matrix-js-sdk#736 to implement matrix-org/matrix-spec-proposals#1538