Kunzisoft / KeePassDX

Lightweight vault and password manager for Android, KeePassDX allows editing encrypted data in a single file in KeePass format and fill in the forms in a secure way.
https://www.keepassdx.com/
GNU General Public License v3.0
4.3k stars 259 forks source link

Detect File Changes and Reload Database #794

Closed H8to closed 3 years ago

H8to commented 3 years ago

Is your feature request related to a problem? Please describe. I sync my DB with Syncthing and e.g. KeePassXC on Linux does monitor the file for updates and will reload it in place. KeePassDX currently does not monitor the file for changes (modification time / inotify / FileObserver).

Describe the solution you'd like KeePassDX should reload the file when it detects changes, so that I don't have to lock and unlock it after changes on one of the devices.

Describe alternatives you've considered Instead of autoreloading you could also create a popup "file has changed by an external party - reload?"

J-Jamet commented 3 years ago

Linked to #650 #172 #100

J-Jamet commented 3 years ago

Indeed, the merge functionality still needs to be created, I have been thinking about it for a while and am doing it step by step. And your suggestion is good, before doing a full file merge we can already inform the user that some external changes have been made. For that, we have to make a monitoring system, perhaps with a new service which checks the URI metadata at regular time intervals. The technical solution is still to be determined.

Edit: I don't know FileObserver but it seems like a good approach, I will read some information on the subject.

J-Jamet commented 3 years ago

It seems that FileObserver (usin inotify) is a method related to files and not URIs, so that cannot be applied for KeePassDX. But RegisterContentObserver seems to do what we need.

H8to commented 3 years ago

Oh sure. You have to handle more than local file paths. For local files I think FileObservers will be the least energy consuming implementation as they rely on the OS event triggers. Not sure how much the two observers differ though.

J-Jamet commented 3 years ago

I tested the RegisterContentObserver method and unfortunately it does not work because it is not activated when necessary. It depends mainly on the file manager because it is up to him to indicate to his content provider that the files are modified, but several external apps do not activate this request.

But I found an alternative and rather elegant solution which uses the file descriptor of the content resolver. I check if the information of the file descriptor has been modified during the binding to the service.

Maybe it will be necessary to check if any file managers modify the files in the background (I don't think so but tell me if I'm wrong and which ones). I will see this part in a second step because I still have to display a dialog box to ask the user if the data should be overwritten or not.

The first changes are visible in the branch feature/Detect_File_Changes

J-Jamet commented 3 years ago

I've just added a dialog box that informs the user if any changes have been made. I'm going to merge these changes first.

~To automatically reload the database with the new information, it's more complicated because it requires to store the main password in clear text variable during all the time the database is open and personally I don't like to do that, I prefer that this information is only available on the fly when it is requested by a task. For data merge in the future, I may ask the user to re-input his credentials by reusing the login screen and temporarily storing the data from the old database to be merged while the stream of new content is retrieved.~

~I think this is an operation that requires you to re-enter your credentials but tell me what you think.~

mirsella commented 3 years ago

I think this is an operation that requires you to re-enter your credentials but tell me what you think.

could be a option in the settings but that's more work. a lot of users are using biometric anyway no ?

cmprmsd commented 3 years ago

I would also prefer re-entering the password or as mirsella wrote, use the biometric fingerprint to unlock the database.

Storing the password somewhere is more convenient, but I would not trade security in this case.

I wonder how KeePassCX does the reloading, as it does not ask for the password when the file has changed. 🤔

J-Jamet commented 3 years ago

It shouldn't be an option in the settings, it would be too much work, decisions have to be made.

Storing the password somewhere is more convenient, but I would not trade security in this case.

Normally this is not a security concern as it will use the same temporary memory that is emptied when the database is closed. It's simply at the architecture level that it bothers me because the opening variable of the database will be stored at the same level as its content, but thinking about it I think it's still the right method, the concepts just need to be better separated in the code.

J-Jamet commented 3 years ago

Finally there is no problem, sorry to bother you with that, I realized that the variables which allow to reconstruct the main credentials are available in an underlying way in the temporary memory, I just need to format them correctly. Sorry for the unnecessary question :rabbit2:
I'll have to refactor the code for a more scalable architecture and there won't be any need to re-request credentials to reload and merge the database. It will take a little longer, but I prefer to do it right. (This is the bad side of the fork :/)

Here is a build of the first phase of file modification recognition KeePassDX_2.9.9build202101051637.zip, I obviously leave the issue open for the following phases :

Tell me if you see any bugs.

J-Jamet commented 3 years ago

I just finished the database reload phase, it wasn't easy but it's OK. There is now a "Reload database" button that allows you to reload external data without having to re-enter your credentials. Here is an APK if you want to test, let me know if you find bugs. KeePassDX_2.9.9build202101072258.zip

I will close this issue once version 2.9.9 is released because the basic issue is solved.

I'm opening the data merge issue here : https://github.com/Kunzisoft/KeePassDX/issues/840

cloudy-dev commented 3 years ago

I just finished the database reload phase, it wasn't easy but it's OK. There is now a "Reload database" button that allows you to reload external data without having to re-enter your credentials. Here is an APK if you want to test, let me know if you find bugs. KeePassDX_2.9.9build202101072258.zip

I will close this issue once version 2.9.9 is released because the basic issue is solved.

I'm opening the data merge issue here : #840

Thanks for your work, I tried your prebuild but for now it throws a Java IO Exception: Stream Closed when trying to unlock my Database.

Edit: Creating a new Database works without a problem but my main Database that works with version 2.9.8 doesn't.

J-Jamet commented 3 years ago

~This is annoying, I did not have this problem. Can you tell me which file manager you are using?~

J-Jamet commented 3 years ago

OK, I can reproduce the problem, this is from the key file stream, I am resolving the issue and uploading a new test APK.

J-Jamet commented 3 years ago

And here is the new test build : KeePassDX_2.9.9build202101091220.zip

cloudy-dev commented 3 years ago

And here is the new test build : KeePassDX_2.9.9build202101091220.zip

Works like a charm on my end, really excited for this feature being merged :)