search

ucam-department-of-psychiatry / camcops

Cambridge Cognitive and Psychiatric Test Kit (CamCOPS)
Other
12 stars 8 forks source link

Fix bug where correct password could not be entered #359

Closed martinburchell closed 2 weeks ago

martinburchell commented 4 weeks ago

Fixes #353.

The fix itself is in DatabaseManager::decrypt(). We now reconnect to the database if the database cannot be accessed with the given key or migration fails. This should allow the correct key to be used. I've removed the migrate and compatibility_sqlcipher_major_version arguments to this method as they weren't being used anywhere and I couldn't see a reason for keeping them. The latter seems to be a way of making a newer version of SQL Cipher use the default settings for an older version. @RudolfCardinal I may be missing something here.

Also fixes downloading the Qt build for the C++ tests. Draft releases aren't made public so we can't just wget the assets.

DatabaseManager::canReadDatabase() is now implied by the return status of DatabaseManager::decrypt() so there is no need to call both methods.

Removes the dependency of DatabaseManager on uifunc so we can test it without having the build the whole application.

martinburchell commented 2 weeks ago

Looks good - thank you! Yes, very sensible to shift to the SQLCipher recommended process. (I think that didn't exist at the time I wrote the original, or possibly I missed it.) The only query I have is this -- the instructions at https://www.zetetic.net/sqlcipher/sqlcipher-api/#cipher_migrate say "3. Check the result of the update by retrieving the row value result." I thought that was a reference to the result of PRAGMA cipher_migrate. Our code in DatabaseManager::decrypt() checks the result of canReadDatabase() instead, which checks the result of SELECT COUNT(*) FROM sqlite_master. Are these equivalent here, or should we be looking at the direct result of the PRAGMA command?

Yes well spotted! I doubt that documentation was there when you wrote it. I've changed pragmaCipherMigrate() to return true or false depending on success/failure of the migration as it seems it will never fail with an error. That way we can avoid the extra database query to check.