vbuch / node-signpdf

Simple signing of PDFs in node.
MIT License
688 stars 175 forks source link

PDFKit 0.11 support #115

Closed dhensby closed 2 years ago

dhensby commented 3 years ago

Added support for v0.11 of PDFKit (which has added AcroForm support). Currently this library is incompatible with v0.11 as the relevant bootstrapping is not applied to the AcroForm.

Changes:

Todo:

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 353


Totals Coverage Status
Change from base Build 344: 0.0%
Covered Lines: 326
Relevant Lines: 326

💛 - Coveralls
dhensby commented 3 years ago

I'm marking this as draft as this isn't creating the signatures quite right - from what I can see the AcroForm isn't being added to the PDF with the /Type /AcroForm attribute

Good PDF (before this work):

21 0 obj
<<
/Type /AcroForm
/SigFlags 3
/Fields [20 0 R]
>>
endobj

Bad PDF (with this PR):

11 0 obj
<<
/Fields [12 0 R]
/NeedAppearances true
/DA (/F1 0 Tf 0 g)
/DR <<
/Font <<
/F1 9 0 R
>>
>>
/SigFlags 3
>>
endobj

Other issues:

dhensby commented 3 years ago

@vbuch this is working now and green. It's not making quite the same use of the form APIs provided and has to make some modifications to the AcroForm that is generated, but it does mean that PDFs are successfully signed by the library when using pdfkit 0.11

vbuch commented 3 years ago

@dhensby I haven't had a good look at the code. But I really, really like it that you did it! I'll have a look ASAP.

I don't like that you needed to change plainAddPlaceholder. It's so fragile that I'm affraid changes to it will easily break it. Also, would really love to have support for both pdfkit10 and pdfkit11. Can you check if it is possible to have them both? pdfkitAddPlaceholder and pdfkit11AddPlaceholder. Don't try to reuse code but rather copy and paste into a separate helper. At some point lerna will come in this lib and then such a change will make a lot of sense. Also it won't be a breaking change but a feature one if you did it this way.

dhensby commented 3 years ago

I don't like that you needed to change plainAddPlaceholder.

The changes to plainAddPlaceholder don't really touch on the functionality very much. Mostly it decouples the buffer parsing that was happening in PdfSigner.sign() (which was only relevant when plainAddPlaceholder was being used) and moves it to the getAcroForm helper. Then I've updated the pdfkitReferenceMock to mock the new methods that pdfkitAddPlaceholder relies on. I think that these changes are relitively low risk as they are predominantly re-factoring, however they probably do require some real-world testing.

Also, would really love to have support for both pdfkit10 and pdfkit11

I don't see why we couldn't do that and I'll find some time to look into that.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had activity in the past 90 days. It will be closed if no further activity occurs. Thank you for your contributions.

dhensby commented 3 years ago

I'm planning on getting back to this soon

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had activity in the past 90 days. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had activity in the past 90 days. It will be closed if no further activity occurs. Thank you for your contributions.

djoli commented 2 years ago

@vbuch any possibility of looking back into this? I was dead keen on using your library to add signature support to some PDFs I am generating via pdfkit (newer version as I require Acroform support), but discovered there's currently no support beyond version 0.10.0 (after a long day of digging through source code and trial and error).

vbuch commented 2 years ago

I've started splitting the packages with lerna so, once I'm done, this will be back in scope. Need to find the time.

vbuch commented 10 months ago

Hey @dhensby I'll have a look at your code here tomorrow and try to apply it on top of https://github.com/vbuch/node-signpdf/tree/feature/split

vbuch commented 10 months ago

@dhensby would you want to create @signpdf/placeholder-pdfkit011 based on this PR and the new structure in https://github.com/vbuch/node-signpdf/pull/194 ?