Skip to content
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

Merged
merged 63 commits into from
Nov 15, 2018
Merged

E2e keys backup #591

merged 63 commits into from
Nov 15, 2018

Conversation

manuroe
Copy link
Contributor

@manuroe manuroe commented Oct 31, 2018

giomfo and others added 30 commits October 19, 2018 10:51
and MXEncryptedContentKey
- add optional antivirus server url
- implement Matrix Content Scanner API
To implement them, it required to add a primary key to MXOlmInboundGroupSession to MXRealmCryptoStore.
Hopefully, this will make decrypting a bit quicker too.
Copy link

@bmarty bmarty left a 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/KeyBackup/MXRecoveryKey.m Outdated Show resolved Hide resolved
MatrixSDK/Crypto/KeyBackup/MXKeyBackup.h Outdated Show resolved Hide resolved

RLMRealm *realm = self.realm;

RLMResults<MXRealmOlmInboundGroupSession *> *realmSessions = [MXRealmOlmInboundGroupSession objectsInRealm:realm where:@"backedUp = NO"];
Copy link

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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"];
Copy link

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?

Copy link
Contributor Author

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].

MatrixSDK/Crypto/KeyBackup/MXKeyBackup.m Outdated Show resolved Hide resolved
MXStrongifyAndReturnIfNil(self);

// Reset backup markers
[self->mxSession.crypto.store resetBackupMarkers];
Copy link

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
Copy link

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

Copy link
Contributor Author

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.

Copy link

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?

Copy link
Contributor Author

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.

MatrixSDK/Crypto/KeyBackup/MXKeyBackup.m Show resolved Hide resolved
@param keyBackupVersion backup information object as returned by `[MXKeyBackup version]`.
@return an error if the operation fails.
*/
- (NSError*)enableKeyBackup:(MXKeyBackupVersion*)keyBackupVersion;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link

@bmarty bmarty left a 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

MatrixSDK/Crypto/KeyBackup/MXKeyBackup.m Outdated Show resolved Hide resolved
return decryption;
}

- (MXKeyBackupData*)encryptGroupSession:(MXOlmInboundGroupSession*)session withPkEncryption:(OLMPkEncryption*)encryption
Copy link

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?

MatrixSDK/Crypto/KeyBackup/MXKeyBackup.m Outdated Show resolved Hide resolved
MatrixSDKTests/MXCryptoBackupTests.m Show resolved Hide resolved
@@ -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;
Copy link

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?

Copy link
Contributor Author

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.

MatrixSDK/Crypto/KeyBackup/MXKeyBackup.m Outdated Show resolved Hide resolved
@manuroe manuroe merged commit 04be7f2 into develop Nov 15, 2018
@manuroe manuroe deleted the e2e_keys_backup branch November 15, 2018 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants