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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add encryption support and access privileges #2696

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

klimeryk
Copy link

@klimeryk klimeryk commented Mar 29, 2024

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 internally Buffer, while in react-pdf, the class extends Writeable 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 in finalize in reference.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 of reference.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 of PDFDocument like so:

const ctx = new PDFDocument( {
    // ...
    userPassword: 'secret',
} );

To be on the safe side, I've also tried enabling all optional permissions, but that did not make a difference:

    permissions: {                                                                                                                                        
        printing: 'highResolution',
        modifying: true,
        copying: true,
        annotating: true,
        fillingForms: true,
        contentAccessibility: true,
        documentAssembly: true,
    },

Copy link

changeset-bot bot commented Mar 29, 2024

鈿狅笍 No Changeset found

Latest commit: 5d9a8e9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@wojtekmaj wojtekmaj left a 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?

}

Copy link
Contributor

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';
Copy link
Contributor

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.

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.

None yet

2 participants