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

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

Merged
merged 2 commits into from Feb 27, 2024

Conversation

dcaliste
Copy link
Contributor

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

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

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

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

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

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

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 dcaliste force-pushed the password branch 2 times, most recently from 79f60cd to cf0a29e Compare January 31, 2024 16:06
@dcaliste
Copy link
Contributor Author

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

dcaliste commented Feb 1, 2024

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

attah commented Feb 1, 2024

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

pvuorela commented Feb 1, 2024

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

dcaliste commented Feb 2, 2024

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

pvuorela commented Feb 2, 2024

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

dcaliste commented Feb 2, 2024

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

@dcaliste
Copy link
Contributor Author

dcaliste commented Feb 2, 2024

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.

Copy link
Contributor

@pvuorela pvuorela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new details pages aren't packaged now so doesn't work properly. Seems missing installation on cmake side

@dcaliste
Copy link
Contributor Author

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

plugin/PDFDocumentPage.qml Outdated Show resolved Hide resolved
plugin/PDFDetailsPage.qml Outdated Show resolved Hide resolved
plugin/PDFDetailsPage.qml Outdated Show resolved Hide resolved
Copy link
Contributor

@pvuorela pvuorela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple minor comments, generally seems good.

pdf/pdfjob.cpp Outdated Show resolved Hide resolved
plugin/PDFDetailsPage.qml Outdated Show resolved Hide resolved
Copy link
Contributor

@pvuorela pvuorela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

@pvuorela pvuorela merged commit 8472ec8 into sailfishos:master Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants