This revision implements a method in SQLiteQueryExecutor to restore database from backup file using SQLite backup API. Additionally it modifies is_database_queryable function so that it can be used for database at any path with any encryption key. It is necessary since we need to check if backup file is actually encrypted with the encryption key provided from backup service and to handle and edge case that we successfully re-encrypted backup database with our encryption key but restoration was interrupted before we replaced database content with backup. Finally this differential implements a function to change encryption key of an encrypted database according to SQLCipher docs: https://www.zetetic.net/sqlcipher/sqlcipher-api/#Changing_Key.
Details
Details
- Build app.
- Write some drafts in some threads.
- Rebuild the app, but modify updateDraft function in SQLiteQueryExecutor to create main compaction instead of updating draft. Add some logging to print encryptionKey to the console.
- Update draft.
- Ensure that backup file is created by downloading container from Xcode or by examining device file explorer in Android studio. Ensure file is encrypted.
- Log-out.
- Re-build the app, but modify updateDraft function in SQLiteQueryExecutor to restore database from main compaction. Hardcode the same backup id that you used to create compaction and encryptionKey that was logged to the console.
- Log-in.
- Update draft.
- Ensure that drafts saved before log-out are on the device.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-5263
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
- Make restoreFromMainCompaction virtual method of DatabaseQueryExecutor to match convention.
- Update web SQLiteQueryExecutorBindings and wasm file.
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 1533 ↗ | (On Diff #35138) | Can you explain why we need to rekey the database? |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 1533 ↗ | (On Diff #35138) | I guess the idea is that we want to be using a different encryption key, since this will not be part of the same compaction. Each compaction has a different key, so when we restore a database we start a new compaction, and rekey it to use a new key. Does this sound accurate? |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 1533 ↗ | (On Diff #35138) | Initially I wanted to restore original database from backup by simply replacing database file with backup file using rename function. In order for this to work I needed to rekey backup database from its original encryption key to the current device encryption key (the device that is restoring). Therefore I implemented rekeying. However later I decided to use backup API instead of simple rename (it is much safer option) but didn't realize rekeying is not necessary anymore. Instead of rekeying backup we can simply set encryption key on the connection to backup database to its original encryption key and everything will work the same. |
Comment Actions
Remove line in which we wrote encryptionKey to attachments file. It was a artifact from debugging I forgot to remove.
Comment Actions
Looks good but I think it's better to first test it also on web
| native/cpp/CommonCpp/DatabaseManagers/DatabaseQueryExecutor.h | ||
|---|---|---|
| 90–92 ↗ | (On Diff #35161) | let's put this method above first #ifdef - this looks confusing |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
| 887 ↗ | (On Diff #35161) | it that case, I would probably rename on_database_open function to something more meaningfull |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.h | ||
| 96–98 ↗ | (On Diff #35161) | also here, put this above #ifdef |
Comment Actions
- Change the order of method definition.
- Rename on_database_open to default_on_db_open_callback
Comment Actions
Change restore logc on web. First migrate backup to plaintext SQLite file and then restore main database from backup. This way we avoid changing main db structure to
SQLCipher file and hence avoid performance issues.
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 1524–1563 ↗ | (On Diff #35472) | This web-specific restore logic needs review. |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 1535–1542 ↗ | (On Diff #35486) | wondering if we could somehow improve formatting to make it more readable |
| 1555 ↗ | (On Diff #35486) | shouldn't we free the error here? |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 1535–1542 ↗ | (On Diff #35486) | I am afraid that clang-formatter keeps our hands tied. Additionally the exact same formatting is used in validate_encryption in a query that encrypts unencrypted database. |
| 1555 ↗ | (On Diff #35486) | We should. I will have to update previous diff as well. |