-
Notifications
You must be signed in to change notification settings - Fork 46
ENH: meta command - Add Permissions + fix version read on encrypted PDFs #189
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
base: main
Are you sure you want to change the base?
Conversation
Lucas-C
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really great!
There are those items in the PR checklist that you need to add though:
- Unit tests for all 2 possible cases:
unencrypted&encrypted - A mention of the new feature added to
CHANGELOG.md
|
alr, removing the diff table and adding unittest and changelogs |
|
Quick sanity check on the test fixture paths I’m planning to use the following in the tests: from pathlib import Path rel = Path("sample-files/002-trivial-libre-office-writer/002-trivial-libre-office-writer.pdf") Assuming RESOURCES_ROOT points to the repo root (as in tests/conftest.py), this resolves to: /sample-files/002-trivial-libre-office-writer/002-trivial-libre-office-writer.pdf …which is the expected location in the sample-files submodule. If RESOURCES_ROOT already points inside sample-files/, I’ll drop the leading sample-files/ from rel. The test also pytest.skip(...)s if the file isn’t present, and the submodule can be initialized with: git submodule update --init --recursive Shout if you prefer me to reference it as: rel = Path("002-trivial-libre-office-writer/002-trivial-libre-office-writer.pdf") Either style works; just want to match the project’s convention. |
Let's got for this: SAMPLE_FILES = RESOURCES_ROOT / "sample-files"
input_pdf = SAMPLE_FILES / rel |
tests/test_metadata.py
Outdated
| assert "permissions" in metadata | ||
| perms = metadata["permissions"] | ||
| assert perms not in {"n/a (unencrypted)", "unknown"} | ||
| # If not "all denied", check formatting invariants |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be known, given that we test a specific PDF file, right?
Maybe it would be better to have 2 distinct test cases/methods:
- one that checks that
none (all denied)is displayed - one that checks that the expected permissions are displayed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so instead of running that test on only those two file i can run that on all the files in the sample-files/, would that be cool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be cool, yes.
However I tested your code, and the assert act_set == exp_set assertion is never evaluated.
Which means that we don't have any test covering this case...
With which PDF test file have you been able to test pdfly meta behaviour, where it displays a list of permissions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it seems like the test case isn't fully covered, and I don't have a PDF with the permissions metadata behavior to fully test it yet. I tried generating a sample PDF with permissions using PyPDF2, but I wasn't able to achieve the desired result. Honestly, I don't have a lot of experience with unittest and pytest yet, so I'm still getting familiar with some of the testing patterns. If you have any advice or pointers on how to create or detect PDFs with specific permissions metadata, it would be really helpful!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sample-files/005-libreoffice-writer-password/libreoffice-writer-password.pdf has some permissions set:
$ qpdf --show-encryption sample-files/005-libreoffice-writer-password/libreoffice-writer-password.pdf --password=openpassword
R = 3
P = -1028
User password = openpassword
Supplied password is user password
extract for accessibility: allowed
extract for any purpose: allowed
print low resolution: allowed
print high resolution: allowed
modify document assembly: not allowed
modify forms: allowed
modify annotations: allowed
modify other: allowed
modify anything: not allowed
So there is probably an issue somewhere in the code...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the password need to be provided to read the permissions, as with qpdf?
|
I made a final feedback comment, but appart from that everything looks really good! 👍 I'll merge your PR once you have addressed that last point and the CI pipeline is passing. |
|
Hey lucas, can u check now? |
|
The unit tests are failing: |
Co-authored-by: Lucas Cimon <[email protected]>
|
Hi @iamrishu11 🙂 👋 |

Fixes #19
Description
What’s new
If none are allowed, it shows none (all denied).
Why
Fixes Feature request: display pdf permissions in
pdfly metaoutput #19.How
Tests
For an encrypted pdf
For a normal file
Notes
Checklist:
A unit test is covering the code added / modified by this PR
In case of a new feature, docstrings have been added, with also some documentation in the
docs/folderA mention of the change is present in
CHANGELOG.mdThis PR is ready to be merged
By submitting this pull request, I confirm that my contribution is made under the terms of the BSD 3-Clause license.