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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add encryption support and access privileges #2696
base: master
Are you sure you want to change the base?
Conversation
|
Based on foliojs/pdfkit#820 upstream Fixes diegomura#672
Syncs up with the upstream version at: https://github.com/foliojs/pdfkit/blob/5635f8a02135acae1a6c1b904af55afe05d0ab7e/lib/security.js
494459f
to
5d9a8e9
Compare
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.
Woah, nice one! Looks good to me.
I wonder if we could have some tests for that. As long as #2633 repairs them 馃ゲ
Also, changeset is missing. Can you please run yarn changeset
as explained in CONTRIBUTORS.md?
} | ||
|
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.
Nit: we probably want to keep this space
|
||
return Buffer.from(byteArray); | ||
}; | ||
import CryptoJS from 'crypto-js'; |
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.
I wonder how this change affects bundle size and what was the original reason for selective import done like that.
Hi, @diegomura! Love your library - I've been using it extensively at https://github.com/klimeryk/recalendar.js/ (https://recalendar.me/). I stumbled upon #672 and figured it was a great chance to contribute back :)
I've tried my best to bring over the changes from the upstream pdfkit version. Encryption was originally added in foliojs/pdfkit#820 there. It was mostly a straightforward update... except changes in
reference.js
. Looks like that class has been fundamentally changed - upstream it uses internallyBuffer
, while in react-pdf, the class extendsWriteable
instead.I've tried first keeping the changes to
reference.js
minimal (see 4376bcb). Most of the changes were easy to port - basically copy and paste. The only part I did not know was the changes infinalize
inreference.js
here. As a result, this was a partial success - non-encrypted PDFs look as before (good), you can generate a PDF that requires a password, the password is properly validated... but then the opened PDF is missing content. See this file: recalendar-password.pdf (password:secret
)I've then tried just switching to upstream's
reference.js
, especially seeing your intention of doing that from #2613. See 5d9a8e9. That looks more promising - the decrypted PDF has the text and almost looks perfect... except, for some reason, the font seems to be missing, so it looks off. See this file: recalendar-password2.pdf (password:secret
)And this applies to both encrypted and non-encrypted generated PDFs. So I'm guessing there's some modification/customization, maybe in some other file that I missed that was done in this version of pdfkit that is not compatible with the upstream version of
reference.js
? Unfortunately, I could not find it. I've tried also going back in history ofreference.js
, but unfortunately these changes seems to be from before it was moved to monorepo and I cannot find the previous repository 馃槥I figured I'd create this PR for you to have a look, maybe something will spark a quick solution or hint from you. If not, I can continue digging 馃檱
Testing
I'm guessing you have your preferred ways of testing different use cases :) Enabling password protection simply boils down to passing
userPassword
when creating a new instance ofPDFDocument
like so:To be on the safe side, I've also tried enabling all optional permissions, but that did not make a difference: