BUG: Fail on wrong state of reserved permission bits during encryption

[stefan6419846]

Fixes #2401.

This change is a bit tricky, as it can be interpreted as a breaking one. We previously did not ensure that the user would use this correctly and this will throw a hard error now. On the other hand, users which would get an error now have been using this the wrong way the whole time anyway.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.15%. Comparing base (4a41c53) to head (5f7d018).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2421   +/-   ##
=======================================
  Coverage   95.14%   95.15%           
=======================================
  Files          51       51           
  Lines        8551     8565   +14     
  Branches     1706     1711    +5     
=======================================
+ Hits         8136     8150   +14     
  Misses        261      261           
  Partials      154      154           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pubpub-zz]

I dislike this change. as @stefan6419846 has stated it this introduced some possible errors that did not used to be. overloading __init__ or __new__ isn't there a way to force the default flags?

[stefan6419846]

overloading init or new isn't there a way to force the default flags?

I do not think that this suits the IntFlag usage, where you tend to just concatenate existing values like UAP.PRINT | UAP.ABC. With the proposed code, you can already start with UAP.minimal() and just add the desired bits/flags to it.

The general alternative would be to silently and auto-magically fix the corresponding bits in the background, but personally I prefer clean user input which gets validated and fixed by the user.

[MartinThoma]

The general alternative would be to silently and auto-magically fix the corresponding bits in the background

Adding a warning as a first step would be a no-brainer. I'm undecided if R13-R32 should then (a) be auto-fixed (b) just get the warning (c) get an exception.

[pubpub-zz]

Adding a warning as a first step would be a no-brainer. I'm undecided if R13-R32 should then (a) be auto-fixed (b) just get the warning (c) get an exception.

First your reading of R13-R32 is correct and should be set to 1
Second, not everybody knows the PDF specification sof all defaults (bits 1-2 to 0, bits 7,8,13-32 set to 1) should be set to default values without warning nor exception.

[stefan6419846]

Second, not everybody knows the PDF specification sof all defaults (bits 1-2 to 0, bits 7,8,13-32 set to 1) should be set to default values without warning nor exception.

While this might be a breaking change, I tend to still disagree about this: With UAP.minimal() and adding the desired permissions or UAP.all() and removing the not desired permissions everyone is able to generate the correct permission flags - we should just document this properly. Whoever mangles with the reserved bits (R* values) should be aware of what this means, id est that it contradicts the official specs.

[stefan6419846]

Given the last feedback, I would probably go with the following approach:

• Validate the permission bits.
• In case of an error: Report a warning with a dedicated class and automatically fix it.

This way everyone can decide whether to propagate the warning as an error through a warning filter or whether to silence it. I have seen no indication that a general deprecation of wrong permission bits is desired for now to avoid requiring too deep knowledge of PDF internals.

[MartinThoma]

Given the last feedback, I would probably go with the following approach:

• Validate the permission bits.
• In case of an error: Report a warning with a dedicated class and automatically fix it.

That doesn't fit our definition of what warnings are used for: https://pypdf.readthedocs.io/en/stable/user/suppress-warnings.html (but besides that, it sounds reasonable to me).

Maybe we should just check if self.strict is True?

That would also give the user control + we have a default that works for most people / cases.

[stefan6419846]

This would mostly apply to the writer, which AFAIK does not have a strict mode at the moment. We would have to add this if required.