sailfishos / sailfish-office

Sailfish Office
GNU General Public License v2.0
73 stars 26 forks source link

[sailfish-office] Allow to store PDF passwords. #193

Closed dcaliste closed 6 months ago

dcaliste commented 7 months ago

@pvuorela I tried to implement via Sailfish Secret a manager for the passwords of protected PDF documents. You can decide to store the password you type, like that it is automatically applied on next opening. In the detail page of PDF documents, you can see the password that is saved for a given document and possibly remove the stored password if you don't want to keep it anymore.

The switch to allow to store the password in the placeholder asking for the password, makes the text field loose the focus, which is sub optimal. I've no idea how to treat this properly for the moment…

pvuorela commented 7 months ago

Sorry to say, but I'm not really eager on bringing in the sailfish-secrets dependency here. It's not required so far by anything on the basic installation or the basic applications. More or less just the email crypto plugin.

dcaliste commented 7 months ago

Ah too bad :( I didn't mention the use case I encountered : I've received a document with the result of a blood test that is protected by a password. The bio lab who did the analysis, gave me the password on a paper when I went there. Obviously within one month I'll loose the paper and won't remember the password. While, I'm sure that I will be interested to read these results again in the future. Integrating password management and document viewer is the most convenient from a user point of view. Storing the password in a separate application would not be convenient at all : need to copy (by hand??) the filename into the password application, then copy back (via clipboard) the password and this each time I want to read the results...

What password backend would then be convenient for the basic installation ? Wifi is storing passwords, accounts in general (email, caldav...) are also storing passwords. So there are ways to store passwords. Which one would you suggest ?

pvuorela commented 7 months ago

Wifi and accounts are slightly different at least in the sense that they need to be accessible from multiple places. Here the passwords are application specific. Simplest could be just a local database maybe. Now with sandboxing it shouldn't be that easily readable by others either. Suppose browser is doing something like that now (didn't check what it does).

dcaliste commented 7 months ago

Oh, good good, if you think it can be achieved with a local storage, I'll update the PR with it. The PDF backend is already using it to remember page position, zoom levels... Just, I need to give a look how the browser is obfuscating the passwords. I don't think they store them in clear. But anyway, it's just for PDF documents, and I already put an option not to store the password. So it may even be stored in clear actually if it makes it easier to implement...

pvuorela commented 7 months ago

For the UI side, one case I left pondering is storing a password for a PDF and then deleting the file. After that it's not possible to clear the password and probably after downloading again the password is remembered. Is this a big enough problem to care. A separate UI for viewing and clearing all the stored passwords would feel quite much too. Some option maybe to detect deleted files, but that could trigger for unmounted SD cards. And the clearing would happen only when having the sailfish-office running.

dcaliste commented 7 months ago

Yeh, I also wondered about this problem of leftover passwords, but it may not be an issue. I don't think there will be a large number of read-protected PDFs. The database file <-> password won't grow too much. So size in my opinion will not be an issue. The second possible problem, is the fact that the user may want to actually remove the password from the device (in particular if it is stored in clear). This is not addressed by the current UI proposition. Adding a password manager is definitely overkill in my opinion. The browser comparison is quite good I think. There is a password manager, but besides using it to actually remember password (which is addressed by my UI proposition), who is using it clear out passwords that one is not interested in keeping ? If the file is deleted via the sailfish-office UI, I can still add a clean-up of the password database. Which will partially solve the issue. Detecting the deletion by checking on each startup that all stored password corresponds to existing files will have the issue of SD card, as you mentioned. All in all, my personal opinion would be that leftovers doesn't matter, like for passwords for sites we don't visit anymore, or sites that are not available or closed.

About the fact that one may delete a PDF, and then download it again, if the device still remembers the password, I would say this is a good thing. If the user download a different file, with the same name, the password won't match and will be ignored (even not displayed in the UI). Besides, I can add a clear() step in case a password doesn't succeed in unlocking a file it is supposed to.

dcaliste commented 7 months ago

I've switched the internal storage of the passwords from Sailifsh Secrets to Qt5Sql, using a QSQLITE driver. The paaswords are now saved in clear in $DataLocation/pdf.db. I've also added a clear() call if a stored password is enable to open a PDF document it is suppose to unlock, based on file path.

dcaliste commented 7 months ago

Sorry, another push… I've changed the identification of the document, from the file path on disk to the md5 sum of the file content. So instead of identifying them by their path (which is fragile, because of file move, rename…), I prefer to identify them by their content.

This solves the problem of loosing the password association when a file is renamed or moved, but it's failing if you annotate a read-protected document, because the content changed and the password is not found anymore. No ideal solution.

What do you suggest ? Is identification by path better or identification by content in your opinion ?

attah commented 7 months ago

Given that conundrum, perhaps it wasn't an improvement. I have a fair few documents on my phone, but i don't think i have ever moved or renamed any of them, let alone use apps that could.

pvuorela commented 7 months ago

If needing to choose between annotation braking the password or moving doing, maybe I'd choose just the path as that should be a bit simpler. Although guess both of those could be stored and used.

But something I just started pondering: instead of remembering the password and dealing with the corner cases, would it be possible and a good idea to support exporting a PDF which wouldn't have the password protection? That would be generally available on the device but perhaps could solve the initial use case you had. Disclaimer: didn't check poppler APIs.

dcaliste commented 7 months ago

Thank you both for the comments. Having the possibility to unlock permanently is very nice, but I don't know any function in Poppler to do this :/ For instance, I tried by unlocking a document, adding a comment, and then save it. The comment is added, but the read-protection is kept. Which is understandable, when you know that the pdfConverter that we're using to save annotations is actually rewriting the file and appending the annotations. I don't see any function to remove the read-protection (https://poppler.freedesktop.org/api/qt5/classPoppler_1_1PDFConverter.html). Too bad…

Another possibility that came this morning to my mind, would be to save the password as a metadata associated to the file with Tracker. Saying this, I've no idea if tracker allow easily to do this and how tracker is handling a file rename or a file move, hopefully not interpreting them as delete + new. But that would allow to keep the information (the password) with the data (the document). Deleting the document would also delete the password. Just, I'm wondering what happens with an sdcard. If the data for the files there are deleted and recreated at each removal and insertion, or if for removal path, the data are kept for later insertion. I'll give a look to the documentation and API of tracker in the coming days.

pvuorela commented 7 months ago

Too bad on Poppler if it's so.

For Tracker I wouldn't perhaps go that far. It should detect the file movings with inotify and I think it's just updating the file path on those cases, but putting app metadata there starts getting more complicated. Think there was support for having app specific graphs perhaps (Tracker provides graphs for Audio, Photos, Documents etc). But still if it's just about helping reopening a file with this app, the sql I'd expect simpler and more understandable.

dcaliste commented 7 months ago

Ok, so I'll go back to storing the file path then.

dcaliste commented 7 months ago

I've pushed back the code using paths to identify documents. I didn't squash with the Md5 commit. So we can continue to discuss if necessary on the matter and choose one or the other implementation.

dcaliste commented 7 months ago

Mmh, that's embarassing. They should be packaged now. Sorry.