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

print: Use Poppler to render pages to print #366

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

ThierryHFR
Copy link

When you print (event "handle_print") with the new mozilla printing interface (thunderbird and firefox), you systematically open the portal printing box.
The printing is supposed to start immediately.
The eventemnent "handle-prepare-print" is the only one to open the portal print box.

@TingPing TingPing changed the title Fix the mozilla print event. print: Use Poppler to render pages to print Dec 21, 2021
@TingPing
Copy link
Member

Your description of this change isn't very helpful. Could you go into more detail why its necessary to depend on Poppler?

@TingPing

This comment has been minimized.

@ThierryHFR
Copy link
Author

Firefox sends a pdf file that contains all the information, paper size, margins...
You have to read the pdf to be able to print it page by page.

@ThierryHFR
Copy link
Author

I don't agree:
the event "handle-prepare-print" opens the dialog box
the event "handle-print" starts the printing

This is the behavior of xdg-desktop-porta-kdel

@TingPing
Copy link
Member

TingPing commented Dec 21, 2021

One thing I am certain of: Poppler is not a safe library to use as you use it here.

The data that applications send is considered untrusted and the xdg-desktop-portal-gtk process is on the host.

If poppler really is something we should be using it needs to be executed in a sandboxed subprocess.

@ThierryHFR
Copy link
Author

I have no idea how to sandboxed the use of poppler, I am taking advice

@TingPing
Copy link
Member

TingPing commented Dec 21, 2021

We use the bubblewrap tool for sandboxing.

Broadly speaking you use the GSubprocess APIs to launch something similiar to bwrap --die-with-parent --unshare-all --ro-bind / / --dev /dev --proc /proc --tmpfs /tmp --file $your_fd /tmp/your_file your-subprocess

This will execute a process that has no network access, can't write to disk, etc. You could transfer return data over stdout or another fd.

I'm not sure the exact format you would return necessarily, its just I don't believe we should be doing any PDF processing in the main process.

@ThierryHFR
Copy link
Author

Thanks, I'll take a look.

@TingPing
Copy link
Member

the event "handle-prepare-print" opens the dialog box
the event "handle-print" starts the printing

Also to answer my own question. You are right.

PreparePrint() returns a token and Print() directly prints if that token is present, otherwise presents a dialog. I'm not sure this PR respects that entirely but this might be handled at the xdg-desktop-portal layer.

@ThierryHFR
Copy link
Author

I based myself on the implementation of kde, if there is a token we use it. otherwise we launch directly the printing, we never launch the dialog box.
This is a good error handling mechanism. Should it be present? It is the choice of the GtK implementation.

@TingPing
Copy link
Member

It sounds like KDE is not following the documentation: https://flatpak.github.io/xdg-desktop-portal/#gdbus-org.freedesktop.portal.Print

@ThierryHFR
Copy link
Author

It sounds like KDE is not following the documentation: https://flatpak.github.io/xdg-desktop-portal/#gdbus-org.freedesktop.portal.Print

Thanks for the documentation, and you are right.
Indeed Kde does not respect the documentation, but Mozilla does the same.
the new Mozilla print box builds a PDF according to the user's choices, size defined by the browser, color, pages. The Print action and the file descriptor of the generated PDF are transmitted.
Without Portal the mechanism works perfectly, with Portal the absence of the token opens the portal print box.

@TingPing
Copy link
Member

Without Portal the mechanism works perfectly, with Portal the absence of the token opens the portal print box.

Yes that is on purpose. Again the point of portals is for users to always be in control, not applications. If an application can print anything it is a bypass.

@ThierryHFR
Copy link
Author

This can also be seen as an evolution. A PDF contains all the information necessary to be processed by a printer. Why transmit more information than necessary. For a long time the standard printer was postscript, the pdf is an evolution.

@TingPing
Copy link
Member

This is a security boundary it has nothing to do with the format.

Applications in Flatpak cannot print without explicit user approval.

@ThierryHFR
Copy link
Author

ThierryHFR commented Dec 21, 2021

That I understand!
It's the application that generates the token, and that transmits it to Flatpak, it's just an understood format, to discuss.
When I am in Firefox or thunderbird (new printing interface) I have the feeling (as a user) that it is me who decides. The new mozilla print interface is the result of increased security! !

@ThierryHFR
Copy link
Author

@TingPing
I'm thinking of sandboxing the pdftocairo usage which will generate a JPEG file.
Is this ok?

@TingPing
Copy link
Member

TingPing commented Dec 28, 2021

The safest format might be a raw GdkPixbuf, since it doesn't involve parsing anything. Or a similar pixel format in cairo.

@ThierryHFR
Copy link
Author

Can we add a binary to the project that does the work?
No binary, extracts a PDF page, and returns raw data.

@TingPing
Copy link
Member

Yes absolutely you can build the binary here, install it to $libexec/xdg-desktop-portal-gtk/

@ThierryHFR
Copy link
Author

I'm almost done! This gives you an idea of things.
I'm moving slowly, it's the holiday season.
I haven't figured out how to get through this MR while working.

Fix build dependency.

Refactoring of the MR to sandbox dangerous code

Fix printing

Update test.yml

Force install deps

Fix build

Fix the print quality

Test if the file is a pdf and finish the print job properly
@ThierryHFR
Copy link
Author

@TingPing,
I just finished!
On the "handle-print" event:
1 - If a token exists, use the token
2 - If the file-descriptor contains a pdf file, use the file-descriptor (as a token). I added the pdftoraw binary in the "/usr/libexec" folder. It allows to obtain the following information:

a) --test => is this a pdf?
b) --pages => The number of pages
c) --width => The width in pixels
d) --height => The height in pixels
e) -raw => The pixels of a GdkPixbuf

3 - Otherwise open the print dialog box

src/pdftoraw.c Outdated Show resolved Hide resolved
src/pdftoraw.c Outdated Show resolved Hide resolved
@@ -42,6 +43,15 @@

#include "gtkbackports.h"

#define DPI 150.0
Copy link
Member

Choose a reason for hiding this comment

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

Document origin of this magic number?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed by 7ec9062 and 3a6cd1e

Copy link
Member

Choose a reason for hiding this comment

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

I'm still not sure I understand the reasoning behind "my choice".

Copy link
Author

Choose a reason for hiding this comment

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

I chose 150 as the print resolution, I hesitated with 300 but that corresponds more to a print resolution for graphic designer.

Copy link
Member

Choose a reason for hiding this comment

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

Please edit this comment to say that. Something along the lines of "150 is a reasonable resolution for consumer printing".

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

src/print.c Outdated Show resolved Hide resolved
src/print.c Outdated Show resolved Hide resolved
src/pdftoraw.c Outdated Show resolved Hide resolved
src/print.c Outdated Show resolved Hide resolved
src/print.c Outdated Show resolved Hide resolved
src/print.c Outdated Show resolved Hide resolved
src/print.c Outdated Show resolved Hide resolved
src/print.c Outdated Show resolved Hide resolved
src/print.c Show resolved Hide resolved
@ThierryHFR
Copy link
Author

ThierryHFR commented Jan 3, 2022

It would be nice to integrate a git script analyzing the style as it is done on the gitlab project of sane-backend :
style-check.sh
Make a check:

git ls-files | xargs ./tools/style-check.sh

Correct:

git ls-files | xargs ./tools/style-check.sh --fix

@ThierryHFR
Copy link
Author

I broke something, I'm looking for ...

@ThierryHFR
Copy link
Author

I think I'm done!

src/print.c Outdated Show resolved Hide resolved
g_ptr_array_add (args, g_strdup_printf ("--file=%s", filename));
g_ptr_array_add (args, g_strdup_printf ("--raw=%d", page));
g_ptr_array_add (args, NULL);
process = g_subprocess_newv ((const gchar * const *)args->pdata, G_SUBPROCESS_FLAGS_STDOUT_PIPE, NULL);
Copy link
Member

Choose a reason for hiding this comment

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

This still needs to run under bwrap to be sandboxed.

Copy link
Author

@ThierryHFR ThierryHFR Jan 3, 2022

Choose a reason for hiding this comment

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

I don't understand, can you give me more details? Thank you

Copy link
Member

Choose a reason for hiding this comment

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

This discussion: #366 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

I think that bwrap is completely respected!
The executed process does not access the network, does not write to the disk and the return is done on STDOUT
If we talk about this line :

413  if ((fd2 = g_file_open_tmp (PACKAGE_NAME "XXXXXX", &filename, &err)) == -1)
414    return FALSE;
415
416  ostream = g_unix_output_stream_new (fd2, TRUE);

This is a copy of the code in the same file:

503  if ((fd2 = g_file_open_tmp (PACKAGE_NAME "XXXXXX", &filename, error)) == -1)
504    return FALSE;
505
506  ostream = (GUnixOutputStream *)g_unix_output_stream_new (fd2, TRUE);1

I am open to your suggestions.

@@ -42,6 +43,15 @@

#include "gtkbackports.h"

#define DPI 150.0
Copy link
Member

Choose a reason for hiding this comment

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

Please edit this comment to say that. Something along the lines of "150 is a reasonable resolution for consumer printing".

src/pdftoraw.c Outdated Show resolved Hide resolved
src/pdftoraw.c Outdated Show resolved Hide resolved
src/pdftoraw.c Outdated Show resolved Hide resolved
src/pdftoraw.c Outdated Show resolved Hide resolved
src/print.c Outdated Show resolved Hide resolved
src/print.c Show resolved Hide resolved
Ordissimo and others added 3 commits January 3, 2022 22:19
@tinywrkb
Copy link

Can this be behind a make configure option?
This adds a libpoppler (&-glib) requirement to a minimal base system.
CUPS is removing printing filters support, so no libpoppler there anymore.

@ThierryHFR
Copy link
Author

Can this be behind a make configure option?

Fixed by ee2cc51

@tinywrkb
Copy link

Fixed by ee2cc51

Thanks!

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.

3 participants