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

enable encryption of contacts, accounts, and crypto DB #3878

Merged
merged 15 commits into from
Feb 23, 2021
Merged

Conversation

gileluard
Copy link
Contributor

fix #3867

private let keychainStore: KeychainStore = KeychainStore(withKeychain: Keychain(service: keychainService, accessGroup: BuildSettings.keychainAccessGroup))

private override init() {
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should call super.init() here but better to remove this init as there is nothing to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer let the init private as the EncryptionKeyManager is only meant to be instantiated through the singleton pattern

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I missed the private

Comment on lines 27 to 31
private static let contactsIv: KeyValueStoreKey = "contactsIv"
private static let contactsAesKey: KeyValueStoreKey = "contactsKey"
private static let accountIv: KeyValueStoreKey = "acountIv"
private static let accountAesKey: KeyValueStoreKey = "acountKey"
private static let realmCryptoKey: KeyValueStoreKey = "realmCryptoKey"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can let these variable as is. Just to mention that you can also group some variable sharing the same purpose like this:

private enum StoreKeys {
static let contactsIv: KeyValueStoreKey
static let contactsAes: KeyValueStoreKey
static let realmCrypto: KeyValueStoreKey
...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea but I'm not sure we need enum in this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be a struct with just constants. Using an enum in this sample is just to indicate that we cannot instantiate it.

}

private func generateKeyIfNotExists(forKey key: String, size: Int) {
if !keychainStore.containsObject(forKey: key) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer using:

guard keychainStore.containsObject(forKey: key) else {
  return
}

in this context

// MARK: - Private methods

private func generateIvIfNotExists(forKey key: String) {
if !keychainStore.containsObject(forKey: key) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer using:

guard keychainStore.containsObject(forKey: key) else {
  return
}

in this context

@manuroe
Copy link
Member

manuroe commented Feb 19, 2021

Last changes requires this PR: matrix-org/matrix-ios-sdk#1027

Copy link
Contributor Author

@gileluard gileluard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@manuroe your changes LGTM

@manuroe manuroe merged commit 00d71fb into develop Feb 23, 2021
@manuroe manuroe deleted the element_3867 branch February 23, 2021 11:54
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.

Enable encryption in MXRealmCryptoStore
4 participants