Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect File Changes and Reload Database #794

Closed
H8to opened this issue Nov 22, 2020 · 16 comments
Closed

Detect File Changes and Reload Database #794

H8to opened this issue Nov 22, 2020 · 16 comments

Comments

@H8to
Copy link

H8to commented Nov 22, 2020

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?"

@H8to H8to added the feature label Nov 22, 2020
@J-Jamet J-Jamet added this to To Do in To study via automation Nov 22, 2020
@J-Jamet
Copy link
Member

J-Jamet commented Nov 22, 2020

Linked to #650 #172 #100

@J-Jamet J-Jamet added this to the Database Sync milestone Nov 22, 2020
@J-Jamet
Copy link
Member

J-Jamet commented Nov 22, 2020

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
Copy link
Member

J-Jamet commented Nov 22, 2020

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
Copy link
Author

H8to commented Nov 25, 2020

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 J-Jamet removed this from To Do in To study Dec 18, 2020
@J-Jamet J-Jamet added this to To do in KeePassDX_2.9.9 via automation Dec 18, 2020
@J-Jamet J-Jamet moved this from To do to In progress in KeePassDX_2.9.9 Dec 24, 2020
@J-Jamet J-Jamet moved this from In progress to To do in KeePassDX_2.9.9 Jan 2, 2021
@J-Jamet J-Jamet moved this from To do to In progress in KeePassDX_2.9.9 Jan 4, 2021
J-Jamet added a commit that referenced this issue Jan 4, 2021
@J-Jamet
Copy link
Member

J-Jamet commented Jan 4, 2021

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
Copy link
Member

J-Jamet commented Jan 5, 2021

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
Copy link

mirsella commented Jan 5, 2021

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
Copy link

cmprmsd commented Jan 5, 2021

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
Copy link
Member

J-Jamet commented Jan 5, 2021

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
Copy link
Member

J-Jamet commented Jan 5, 2021

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 🐇
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 :

  • database reload
  • then database merge.

Tell me if you see any bugs.

@J-Jamet
Copy link
Member

J-Jamet commented Jan 7, 2021

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

@J-Jamet J-Jamet moved this from In progress to Done in KeePassDX_2.9.9 Jan 7, 2021
@cloudy-dev
Copy link
Contributor

cloudy-dev commented Jan 8, 2021

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
Copy link
Member

J-Jamet commented Jan 8, 2021

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

@J-Jamet J-Jamet moved this from Done to In progress in KeePassDX_2.9.9 Jan 9, 2021
@J-Jamet
Copy link
Member

J-Jamet commented Jan 9, 2021

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 added a commit that referenced this issue Jan 9, 2021
@J-Jamet
Copy link
Member

J-Jamet commented Jan 9, 2021

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

@cloudy-dev
Copy link
Contributor

cloudy-dev commented Jan 12, 2021

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

5 participants