Rookiestyle / KeePassOTP

Add OTP support (two factor authentication) to KeePass
GNU General Public License v3.0
405 stars 29 forks source link

Accidental removal of OTP database possible #127

Closed caspadan closed 1 year ago

caspadan commented 1 year ago

I don't want to call this a bug, but seems the best category for it.

Overview

It seems you may be able to (unintentionally) delete your separate OTP database - possibly without knowing.

Steps to Reproduce

  1. Open a database that has KeePassOTP set up where you have your OTPs stored in a separate database (ensure this OTP database is not set to auto-open).
  2. The OTP database will not be open. Now go to KeePassOTP settings > OTP settings (db specific) and uncheck "Save OTP secrets in a separate database" and viola.
  3. If you save your database now, your OTP secrets are gone.
  4. Checking "Save OTP secrets in a separate database" at this point no longer seems to restore the OTP database.

NOTES: You'll see that as soon as you uncheck "Save OTP secrets in a separate database", the drop down menu to the right no longer has the "Open Database" option. However, if the OTP database is opened when you uncheck "Save OTP secrets in a separate database", then there is a message telling you that you can delete the OTP database or choose to deactivate it; the latter case sees the OTP database restored upon enabling the checkbox again.

Expected Behavior

The OTP database should perhaps only be disabled when unchecking "Save OTP secrets in a separate database" and the OTP database is not open. Or a similar message should pop up (with the same functionality) to the message displayed when the database is open. Otherwise, in the case where a separate OTP database exists, the option should only be uncheckable after that database is opened. Or that option should not have the ability to delete the OTP database at all and the database can only be deleted via the dropdown menu.

Actual Behavior

See the steps to reproduce above. The OTP database seems to be deleted if "Save OTP secrets in a separate database" is unchecked while the OTP database is not open.

Currently, as a safeguard or a workaround to avoid potential loss, this option should only be unchecked when the OTP database is open.

Context

OS: Win 10 x64 KeePass Version: 2.53.1 Plugin Version: 1.6.3

caspadan commented 1 year ago

This had me thinking on something else, somewhat related to deleting the separate OTP database: Let's say you export your OTP database and then delete the OTP database from the main one. There seems to be no way to re-import it as a separate database again should you later want to have it back within the main one. The only option is to create a new database. Obviously this might be a bit tricky as the user needs to be sure with such a functionality that what they are importing as the OTP database was originally an export from the main database they are currently working on. Just a thought I had.

Rookiestyle commented 1 year ago

Open a database that has KeePassOTP set up where you have your OTPs stored in a separate database (ensure this OTP database is not set to auto-open). The OTP database will not be open. Now go to KeePassOTP settings > OTP settings (db specific) and uncheck "Save OTP secrets in a separate database" and viola. If you save your database now, your OTP secrets are gone. Checking "Save OTP secrets in a separate database" at this point no longer seems to restore the OTP database.

I consider this a bug and will fix it :)

Your 2nd comment about deliberately deleting the OTP db and saving the main db afterwards indeed results in the OTP db to be gone. I do not plan to implement an import feature. They user of course can open the exported OTP db and add OTP manually or restore a backup of the main db including the OTP db

caspadan commented 1 year ago

Thanks @Rookiestyle

The only concern (and the whole point of bringing this issue up) of course is with your OTP database not open, you can accidentally delete it by unchecking the option to save OTP secrets in a second database.

The import idea was just a passing thought or consideration, but I agree with leaving it as it is.

Take care.