Resolve "Support PDF generation via a headless Chromium running in the background using celery" #2109
No reviewers
Labels
No labels
Security
TeX
auto-update
board
done
board
ready
board
todo
check
delete-eslint-rc-js
check
update-builddeps-package-json
check
update-eslint-rc-js
check
update-gitignore
check
update-merge-request-template
check
update-prettier-ignore
check
update-pyproject-toml
check
update-renovate-json
check
update-tox-ini
part
backend
part
ci
part
docs
part
frontend
part
i18n
part
non-technical
part
packaging
prio
1
prio
2
prio
3
release-mr-5.x
size
large
size
medium
size
small
source
customer
source
customer::fsmw
source
customer::fss
source
customer::teckids
source
downstream
type
breaking
type
bug
type
feature
type
refactoring
workflow
blocked
workflow
confirmed
workflow
current-todo
workflow
discussing
workflow
new-app
workflow
wontfix
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
aleksis/AlekSIS-Core!2109
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "357-support-pdf-generation-via-electron-pdf-running-in-the-background-using-celery"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #357
added 2 commits
Compare with previous version
assigned to @nik and unassigned @hansegucker
requested review from @lloydmeins
marked this merge request as ready
Please use
location.replacecf. https://stackoverflow.com/a/506004
s/at/on/ (like you did in JS)
Ouch…
/static? It's as configurable asSTATIC_ROOT, so obviously using a hard-coded string is wrong.We have (or should have) a tempfile mechanism that depends on site configuration, as to not just dump tempfiles in
/tmp. Also, security measure should be taken so as to not leak personal data to system locations readable by other processes that way."Unlike TemporaryFile(), the user of mkstemp() is responsible for deleting the temporary file when done with it."
https://docs.python.org/3/library/tempfile.html#tempfile.mkstemp
Actually, the way you call electron-pdf, we could potentially also call it directly in the request context…
Are you sure this does not need setting up an X.org environment before calling electron-pdf?
Also, reading https://superuser.com/a/737587, I think you might want to try whether https://wkhtmltopdf.org/ and https://github.com/incuna/django-wkhtmltopdf, both of which are in Debian, are an option instead of electron-pdf.
wkhtmltopdf was tested, but it wasn't able to understand modern CSS.
OK.
You could try using
subprocess.Popen'spass_fdmechanism to pass open file descriptors to electron-pdf. I tested using/dev/stdinand/dev/stdout, and the latte rworks (the PDF is output to stdout), but it seems stdin is consumed before being read by the DOM loader. Using a separate file descriptor could work, probably usingos.pipe.I'm unsure how to do/test this.
Me neither… but I am experimenting with it.
The first two points of your annotation are easy to resolve, but for the last one I have no idea how to solve it. The URLs to CSS files, images etc. are typically built up like "/static/...". These URLs doesn't exist for electron-pdf, so we replace the
/static/stuff by the actual directory of the static files.changed this line in version 2 of the diff
changed this line in version 2 of the diff
changed this line in version 2 of the diff
added 3 commits
Compare with previous version
I will skip this as we are going to make celery non-optional.
added 2 commits
Compare with previous version
Well, why is the URL in the template, then?
All that temp file handling is quite messy. You should really use a context manager here (context managers are always the bette rchoice for resources that are to be cleand up afterwards). In that regard, you are looking for the
TemporaryDirectoryclass instead of callingmkstempdirectly: https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryDirectory (that way, you get a directory, in which you can use your own file names even). You can trust the OS to do the right thing concerning privileges and such that way and do not need an extra setting.Because the URL is resolved correctly by web browsers if the page is opened from a web server.
marked this merge request as draft
added 97 commits
masterCompare with previous version
changed this line in version 5 of the diff
added 1 commit
Compare with previous version
changed this line in version 6 of the diff
added 1 commit
Compare with previous version
Done.
Any idea?
In my understanding, the whole concept of this MR is that the page is not opened via a regular web server anymore.
Yes, that's the problem.
added 185 commits
masterCompare with previous version
changed this line in version 9 of the diff
added 1 commit
Compare with previous version
marked this merge request as ready
I added a new solve option for this in e7fe54d1695f336475565404ac335c00b91f7bec. Could you please check if this could be a good one?
This also needs some little changes in Django Any-JS, so don't just merge.
resolved all threads
approved this merge request
Unrelated whitespace change
Then what is the meaning of this comment?
How do you know this display is free? Or why do you even care?
Just let Xvfb choose itself.
(You are probably looking for the
xvfb-runcommand which you can pass the electron-pdf command directly and it does everything necessary)This looks like a view. Why is it not in
views.py(and class-based)?I'm not yet convinced by that.
It does unnecessary requests to the static server, which might not even be reachable from within the AlekSIS server/container. We cannot require the AleKSIS ap perver to have HTTP access to the static files server.
This makes #376 worse. We cannot store such sensitive data in
MEDIA_ROOT, without thought about this. Either put it somewhere else and add a view with proper permission checking to retrieve it, or at least make a model forthe result that has the file linked that can leverage the permission checking mechanism we implement globally to solve #376.This clobbers any previous ocntext, including the
static_prefixyou added further up.Do not forget to test inside Docker, and update the Docker image to contain the necessary dependencies.
In general, I'm still reluctant to force users to run Chrome on their servers.
added 12 commits
mastere3f97741- Add support for an additional button and a redirect on success in progress template34cffc2e- Add tools for rendering PDFs with electron-pdf0f1a77d3- Use location.replace instead of location.href in JavaScriptdaea4090- Improve handling of temporary files in generate_pdf()d8ddbca7- Let generate_pdf() start a virtual X framebuffer for electron-pdf920f0509- Use non-optional Celery for PDF generation6e316fb2- Use TemporaryDirectory for handling of temporary files in PDF generationCompare with previous version
No, the previous context is not relevant for the progress template, just for the
render_stringmethod.First of all, it looks like the
rendermethod. This was the intention because there are views for example in Chronos (timetable) which return a browser version in one case and a print version in an other case. But what I can imagine is to add a class-based view that makes use of thisrender_pdfmethod.marked this merge request as draft
Do not re-use variable names. if a variable has two meanings in one scope, use two differently-named varibales.
(I have the impression that we discussed this already on another MR less tha na wekk ago ☺)
changed this line in version 11 of the diff
added 1 commit
367a14b4- Add absolute URL to all static URLs for printing via electron-pdfCompare with previous version
changed this line in version 12 of the diff
added 1 commit
adb077c4- Run test with xfvb-run in Docker imageCompare with previous version
added 11 commits
masterCompare with previous version
added 1 commit
Compare with previous version
added 12 commits
master3b7fa96a- Run test with xfvb-run in Docker imagefa9747a6- Merge branch 'master' into...Compare with previous version
added 1 commit
Compare with previous version
added 1 commit
d8762574- Install xorg in Docker for use with electron-pdfCompare with previous version
added 1 commit
Compare with previous version
added 1 commit
2ec48e88- Install xorg in Docker for use with electron-pdfCompare with previous version
added 1 commit
df6b2672- Install xorg in Docker for use with electron-pdfCompare with previous version
changed this line in version 21 of the diff
changed this line in version 21 of the diff
added 1 commit
c0995406- Use an headless Chromium instead of electron-pdfCompare with previous version
changed title from Draft: Resolve "Support PDF generation via {-electron-pdf-} running in the background using celery" to Draft: Resolve "Support PDF generation via {+an headless Chromium+} running in the background using celery"
added 1 commit
40f11494- Use a headless Chromium instead of electron-pdfCompare with previous version
changed title from Draft: Resolve "Support PDF generation via a{-n-} headless Chromium running in the background using celery" to Draft: Resolve "Support PDF generation via a headless Chromium running in the background using celery"
added 1 commit
524b91d7- Use a headless Chromium instead of electron-pdfCompare with previous version
added 12 commits
186c111c- Add support for an additional button and a redirect on success in progress template875906c2- Add tools for rendering PDFs with electron-pdf00e9882a- Use location.replace instead of location.href in JavaScript6743559d- Improve handling of temporary files in generate_pdf()707715bb- Let generate_pdf() start a virtual X framebuffer for electron-pdfd397a439- Use non-optional Celery for PDF generationda0e44bf- Use TemporaryDirectory for handling of temporary files in PDF generatione299f045- Add absolute URL to all static URLs for printing via electron-pdf8e0e5b6f- Test electron-pdf with xfvb-run in Docker image4a76e450- Install xorg in Docker for use with electron-pdf205dc4a2- Use a headless Chromium instead of electron-pdf1091cda7- Add a view to test the PDF generation (based on a generic PDF generation view)Compare with previous version
changed this line in version 25 of the diff
added 5 commits
masterb881c8c0- Merge branch 'master' into...Compare with previous version
added 1 commit
b03040bd- Store references to generated PDF files in DB and generate PDF files by calling an actual URLCompare with previous version
added 3 commits
827b235c- Add missing import of render_pdf968caaac- Add selenium test for PDF generation0ed66417- Let PDF generation task fail if Chromium doesn't return a success return codeCompare with previous version
added 14 commits
master9066d31a- Reformatacb973e1- Rewrite clean up task for PDF files to use PDFFile objects0913d30c- Add further tests for PDFFile model and clean-up task08ad3ce8- Merge branch 'master' into...b80b5d2d- Merge migrations and fix migration order after mergeCompare with previous version
resolved all threads
changed this line in version 29 of the diff
added 1 commit
1177b1f9- Rename context variable in render_pdf to avoid confusionCompare with previous version
@nik Everything works fine now. The only thing that's missing is the authentication.
added 11 commits
master1510cc4a- Merge branch 'master' into...275e6da3- Introduce secret for PDF files to protect HTML page against unauthorized accessCompare with previous version
mentioned in merge request AlekSIS-App-Alsijil!1824
Blocked by #376
marked this merge request as ready
Unnecessary left-over.
Please move into a function/method used as
defaulton the field.The timeout should be in minutes, and default to something very small (like 3 minutes, or so).
The HMAC can be re-used for the filename.
https://www.youtube.com/watch?v=Hgx0RTx0aFg
These people sorting things alphabetically ...
changed this line in version 31 of the diff
changed this line in version 31 of the diff
changed this line in version 31 of the diff
added 1 commit
27f94b46- Apply 3 suggestion(s) to 2 file(s)Compare with previous version
changed this line in version 32 of the diff
changed this line in version 32 of the diff
added 1 commit
0246d430- [PDF] Set default expiration time much shorter and use default attributeCompare with previous version
added 1 commit
1868fd7f- Update poetry.lock to get current dev dependenciesCompare with previous version
changed this line in version 34 of the diff
added 7 commits
master29dbcb7b- Merge branch 'master' into...Compare with previous version
added 1 commit
8bf6e49d- Use HMAC secret as filename for generated PDF filesCompare with previous version
changed this line in version 36 of the diff
added 2 commits
7b228c51- [PDF] Remove code to replace static and media URLs with absolute URLs8bbde224- Use TransactionTestCase for PDFFile testsCompare with previous version
resolved all threads
changed this line in version 37 of the diff
added 1 commit
29fbbeb0- Update docstrings for PDF generation utilsCompare with previous version
resolved all threads
changed this line in version 38 of the diff
added 1 commit
5168ee93- Fix escape in DockerfileCompare with previous version
mentioned in commit
183a0dbfb0