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.35k stars 262 forks source link

Requirements for File Manager compatibility #531

Open x0b opened 4 years ago

x0b commented 4 years ago

Thank you for adding RCX to the wiki page.

I was wondering what the requirements are for full compatibility, since it is currently just "Partially. Works from the application itself". Does KeePassDX act as SAF client or provider? If it is the second one, can it deal with pipe or socket file descriptors?

EDIT: It does seem to require a file backed descriptor, is that correct?

J-Jamet commented 4 years ago

KeePassDX uses SAF. To be fully compatible, you must manage the intent ACTION_OPEN_DOCUMENT and ACTION_CREATE_DOCUMENT to retrieve an URI with persist link.

EDIT: It does seem to require a file backed descriptor, is that correct?

No, the code you highlight is only there to retrieve data from the stream by the content resolver.

x0b commented 4 years ago

Thanks. I wasn't sure with the highlighted section, but since you are reading only 10 bytes anyway before reset it should work within any reasonable default buffer size. It actually works now, there was just a different bug.

A final question: I'm seeing three meta queries for the file whenever I open KeePassDX. Since they are so close together they will at most cause a single network call (any reasonable provider will cache this), but as you can see from the timestamps, it still incurs a ~7-9ms call time for each query. Is there a reason for this design?

// Meta Query 1
20:52:41.956 Provider: queryDocument(): rclone/remotes/Dropbox:/keepass.kdbx
20:52:41.958 Provider: retrieving node: Dropbox:/keepass.kdbx
20:52:41.959 Provider: getFileItem: using cached for Dropbox:/keepass.kdbx
20:52:41.965 Provider: queryDocument(rclone/remotes/Dropbox:/keepass.kdbx): keepass.kdbx, application/x-keepass, keepass.kdbx, 1587579790760

// Meta Query 2
20:52:42.072 Provider: queryDocument(): rclone/remotes/Dropbox:/keepass.kdbx
20:52:42.073 Provider: retrieving node: Dropbox:/keepass.kdbx
20:52:42.075 Provider: getFileItem: using cached for Dropbox:/keepass.kdbx
20:52:42.079 Provider: queryDocument(rclone/remotes/Dropbox:/keepass.kdbx): keepass.kdbx, application/x-keepass, keepass.kdbx, 1587579790760

// Meta Query 3
20:52:42.094 Provider: queryDocument(): rclone/remotes/Dropbox:/keepass.kdbx
20:52:42.096 Provider: retrieving node: Dropbox:/keepass.kdbx
20:52:42.098 Provider: getFileItem: using cached for Dropbox:/keepass.kdbx
20:52:42.102 Provider: queryDocument(rclone/remotes/Dropbox:/keepass.kdbx): keepass.kdbx, application/x-keepass, keepass.kdbx, 1587579790760

// User enters password, Open query
20:52:46.126 Provider: openDocument: rclone/remotes/Dropbox:/keepass.kdbx, mode=r
20:52:46.129 Provider: retrieving node: Dropbox:/keepass.kdbx
20:52:46.131 Provider: getFileItem: using cached for Dropbox:/keepass.kdbx
20:52:46.141 Provider: Running Pipe Transfer...
20:52:49.335 Provider: Stopping Pipe Transfer, cancelled=false

I think it is caused by the DocumentFile abstraction being called here - .exists (1), getModificationString() (2) and getSizeString() (3).

It would probably be more efficient to issue a single call and reuse the result. Since this is executed in a view method, the calls in the log might have dropped about 8-9 frames.

J-Jamet commented 4 years ago

Thanks. I wasn't sure with the highlighted section, but since you are reading only 10 bytes anyway before reset it should work within any reasonable default buffer size. It actually works now, there was just a different bug.

I may have misunderstood the question, I just do not know the internal workings of file managers, I just use the mark and the reset of the URI stream to be able to read the header of the file and choose the type of database to build.

A final question: I'm seeing three meta queries for the file whenever I open KeePassDX. Since they are so close together they will at most cause a single network call (any reasonable provider will cache this), but as you can see from the timestamps, it still incurs a ~7-9ms call time for each query. Is there a reason for this design?

I'm using the DocumentFile methods to be able to retrieve the metadata to display. What I can do is a background thread to get this metadata in the recyclerview.

It would probably be more efficient to issue a single call and reuse the result. Since this is executed in a view method, the calls in the log might have dropped about 8-9 frames.

If you can do that, it would be great indeed.

I was wondering what the requirements are for full compatibility

To answer your first question, I have not tested RCX in depth, but I noticed that requesting the URI with the "Open existing database" button from KeePassDX did not display your application to open a document. Maybe because of your DocumentsProvider.

x0b commented 4 years ago

If you can do that, it would be great indeed.

It is nothing I can do on my end - the SAF client, i.e. KeePassDX, would need to be changed. In my experience, DocumentFile is a terrible abstraction, since everyone - even you with your FileDatabaseInfo class - has to roll their own wrapper anyway. That being said, I didn't actually notice the frame drops visually, so this might not be a problem worth spending time on.

If you want though, you could introduce a method like queryMetadata() into your FileDatabaseInfo

ContentResolver contentResolver = context.getContentResolver();
try (Cursor cursor = contentResolver.query(docUri, new String[]{
       // Or whatever columns/properties are needed
        DocumentsContract.Document.COLUMN_DOCUMENT_ID,
        DocumentsContract.Document.COLUMN_DISPLAY_NAME,
        DocumentsContract.Document.COLUMN_MIME_TYPE,
        DocumentsContract.Document.COLUMN_LAST_MODIFIED,
        DocumentsContract.Document.COLUMN_SIZE
}, null, null, null)) {
   if (null == cursor) {
      // file does not exist
   }
   long modified = cursor.getLong(3);
   long size = cursor.getLong(4);
}

Obviously, a lot more low-level, and if no one (other than me) has said something about this I would not change it.

To answer your first question, I have not tested RCX in depth, but I noticed that requesting the URI with the "Open existing database" button from KeePassDX did not display your application to open a document. Maybe because of your DocumentsProvider.

Yes, exactly, there are some things in the currently released version that prevent it from working properly, but thanks to your information this will work in the future.

J-Jamet commented 4 years ago

Thank you for the metadata code, ~I will use it because, as you say, it is more optimized, even if there is no issue for the moment. I didn't have in mind that I could do it this way. I will keep the current code in the catch if there is an error.~

J-Jamet commented 8 months ago

Just checked and the DocumentFile with fromSingleUri call SingleDocumentFile class that call DocumentsContractAPI19, it's exactly the same code as https://github.com/Kunzisoft/KeePassDX/issues/531#issuecomment-618601242 so it's not more optimized.