Page MenuHomePhabricator

Implement SQLiteQueryExecutor method to restore database from main compaction
ClosedPublic

Authored by marcin on Jan 2 2024, 4:55 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Oct 15, 2:02 PM
Unknown Object (File)
Fri, Oct 11, 9:18 PM
Unknown Object (File)
Fri, Oct 11, 9:18 PM
Unknown Object (File)
Fri, Oct 11, 9:18 PM
Unknown Object (File)
Fri, Oct 11, 9:15 PM
Unknown Object (File)
Mon, Oct 7, 5:31 PM
Unknown Object (File)
Oct 4 2024, 3:21 PM
Unknown Object (File)
Sep 5 2024, 3:06 PM
Subscribers

Details

Summary

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.

Test Plan
  1. Build app.
  2. Write some drafts in some threads.
  3. 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.
  4. Update draft.
  5. Ensure that backup file is created by downloading container from Xcode or by examining device file explorer in Android studio. Ensure file is encrypted.
  6. Log-out.
  7. 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.
  8. Log-in.
  9. Update draft.
  10. Ensure that drafts saved before log-out are on the device.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin edited the test plan for this revision. (Show Details)
marcin requested review of this revision.Jan 2 2024, 6:01 AM
marcin edited the test plan for this revision. (Show Details)
  1. Make restoreFromMainCompaction virtual method of DatabaseQueryExecutor to match convention.
  2. Update web SQLiteQueryExecutorBindings and wasm file.
ashoat added inline comments.
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.
Rekeying backup database file is not necessary. Thanks for catching this!

Plan changes to remove rekeying

Remove unecessary backup database rekeying

Remove line in which we wrote encryptionKey to attachments file. It was a artifact from debugging I forgot to remove.

kamil requested changes to this revision.Jan 5 2024, 4:53 AM

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

This revision now requires changes to proceed.Jan 5 2024, 4:53 AM
  1. Change the order of method definition.
  2. Rename on_database_open to default_on_db_open_callback

Looks good but I think it's better to first test it also on web

Test executed: https://linear.app/comm/issue/ENG-6146/final-testing-task

This revision is now accepted and ready to land.Jan 9 2024, 6:20 AM

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.

marcin added inline comments.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
1524–1563 ↗(On Diff #35472)

This web-specific restore logic needs review.

kamil added inline comments.
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?

This revision is now accepted and ready to land.Jan 16 2024, 3:41 AM
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.