-
Notifications
You must be signed in to change notification settings - Fork 62
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
Add support for encrypting user metadata used by Sync. #1100
Conversation
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.
Really nice. Only thing missing seem to be a test verifying that we actually encrypt the metadata realm.
...c/src/androidAndroidTest/kotlin/io/realm/kotlin/test/mongodb/shared/AppConfigurationTests.kt
Outdated
Show resolved
Hide resolved
...c/src/androidAndroidTest/kotlin/io/realm/kotlin/test/mongodb/shared/AppConfigurationTests.kt
Outdated
Show resolved
Hide resolved
.../library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/AppConfigurationImpl.kt
Show resolved
Hide resolved
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/AppConfiguration.kt
Show resolved
Hide resolved
...ages/test-sync/src/androidAndroidTest/kotlin/io/realm/kotlin/test/mongodb/shared/AppTests.kt
Outdated
Show resolved
Hide resolved
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. I only have one smaller comment around the test and it needs to parse CI, but otherwise this looks ready to merge 👍
packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/AppConfiguration.kt
Show resolved
Hide resolved
...ages/test-sync/src/androidAndroidTest/kotlin/io/realm/kotlin/test/mongodb/shared/AppTests.kt
Outdated
Show resolved
Hide resolved
...ages/test-sync/src/androidAndroidTest/kotlin/io/realm/kotlin/test/mongodb/shared/AppTests.kt
Show resolved
Hide resolved
...ages/test-sync/src/androidAndroidTest/kotlin/io/realm/kotlin/test/mongodb/shared/AppTests.kt
Outdated
Show resolved
Hide resolved
...ages/test-sync/src/androidAndroidTest/kotlin/io/realm/kotlin/test/mongodb/shared/AppTests.kt
Outdated
Show resolved
Hide resolved
...ages/test-sync/src/androidAndroidTest/kotlin/io/realm/kotlin/test/mongodb/shared/AppTests.kt
Outdated
Show resolved
Hide resolved
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 great 🚀
public class AppConfigurationImpl constructor( | ||
override val appId: String, | ||
override val baseUrl: String = DEFAULT_BASE_URL, | ||
override val encryptionKey: ByteArray?, |
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.
Should we keep a copy of the encryption key at SDK level? I think that we should not keep any copy, as they should be safely stored by the user or core.
RealmConfiguration
also stores a copy. For that case we have a core function to retrieve the key, that would help testing, but we lack of such function for the metadata encryption key.
We could address this in another PR.
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 agree, this is a bit of a smell, but I wouldn't consider it a big deal, since if you have access to read the memory where this key resides, you also have access to the open Realm and can just ask Core for the data.
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.
Interesting points. Currently, however, the exposed core function realm_config_get_encryption_key
(not for metadata) is not used in any configuration class. The actual
functions are implemented but yes, a copy is still being stored.
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.
Nice 🚀
Description
Closes #413.
Users can encrypt user metadata via
AppConfiguration.Builder.encryptionKey()
and access their encryption key viaAppConfiguration.encryptionKey
.TODO