Catch metadata realm decryption. #4285
Conversation
|
This line is necessary in the test to produce a RealmFileException. Without it, the test raises a MismatchedConfiguration which doesn't reflect the decryption failure expected in production. 
|
|
That indicates that there's something still holding onto the RealmCoordinator after failing to open the Realm. Clearing the cache works around one specific symptom of that without fixing the problem, and will break things if there are any other Realms open. |
| realm = get_realm(); | ||
| } | ||
| catch (const RealmFileException&) { | ||
| if (!should_encrypt) |
There was a problem hiding this comment.
This should check the error code from the exception rather than should_encrypt.
There was a problem hiding this comment.
Opening with the wrong key and opening when should_encrypt configuration doesn't match the original configuration are both RealmFileException::Kind::AccessError and have the same error message. What other ways are there to differentiate?
There was a problem hiding this comment.
In both of those cases we want to delete the Realm and recreate it. Currently this code will delete the Realm when any error occurs if we want to encrypt the Realm even if it's an unrelated error, and will fail to delete the Realm in a case where we want to (disabling encryption when the existing file is encrypted).
There was a problem hiding this comment.
I think I understand. I previously was trying to retain the response of throwing an error when should_encrypt == false was applied to a path that was encrypted.
The usage of this realm_coordinate->clear_cache() is only used in the test. I couldn't find another way to close the metadata realm in order to attempt opening with the wrong key. I tried with other methods on Do you also mean that using clear cache is dangerous in a testing context? |
test/object-store/sync/metadata.cpp
Outdated
| CHECK(user_metadata->is_valid()); | ||
|
|
||
| // Close realm | ||
| _impl::RealmCoordinator::get_coordinator(metadata_path)->clear_cache(); |
There was a problem hiding this comment.
@tgoyne Is clearing the cache here bad? I couldn't figure any other way to successfully close the realm. I understand there are dangers for doing this normally, but would it be suitable for a test context?
There was a problem hiding this comment.
The above code needs to be wrapped in another scope. The metadata realm will be closed when everything using it goes out of scope (in this case, just user_metadata as the manager itself doesn't keep the Realm open).
There was a problem hiding this comment.
Let me know if 8317f78 is what you mean.
|
I'm going to retry build because the CI error looks like a server connectivity thing. Then I'll merge. |
Related to RCOCOA-1035
What, How & Why?
What: Opening a metadata realm with the wrong encryption key or
should_encryptproperty will now remove the metadata realm and create a new realm using the new key or configuration option.How: Catch opening the metadata realm when the SyncMetadataManager is being constructed. If caught exception, remove metadata realm, open a new one in its place.
Why: Realm-core creates an encryption key for devices if one does not exist. It's possible for a device to lose its encryption key, but retain the sync metadata realm (for example, via a backup). Previously, the newly created key couldn't open the realm and an exception could be thrown each time the app is started.
☑️ ToDos