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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: sign protected pdfs #498

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

xhyrom
Copy link
Contributor

@xhyrom xhyrom commented Aug 19, 2024

Fixes #495
Resolves #395

PDF/A detekcia vypnutá pri zaheslovanom dokumente source

taktiež to riešenie s vytvorením našeho "protected" dokumentu, avšak neviem, či je tu úprimne iné riešenie keďže to heslo musíme mať uložené pri dokumente

output.mp4

new SaveFileResponder(file, autogram, userSettings.shouldSignPDFAsPades()),
userSettings.isPdfaCompliance(), userSettings.getSignatureLevel(), userSettings.isEn319132(), tspSource, userSettings.isPlainXmlEnabled());
autogram.sign(job);
var document = SigningJob.createDSSFileDocumentFromFile(file);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nebolo by lepšie to volať v tejto metóde?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a prečo to nedáme do SigningJob.build? To sa volá aj pre buildFromRequest aj pre buildFromFile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keď potrebuješ tie getParametersForFile, tak by som to normláne osobitne zavolal v buildFromFile a buildFromRequest - nevadí, že to bude 2x, stále to bude lepšie ako teraz volané z GUI.

A neviem úplne, čo sa stane, keď pošleš taký súbor cez API - nebude už server chcieť niečo čítať z toho súboru?

Copy link
Contributor Author

@xhyrom xhyrom Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Viem, že som to včera nespravil tak lebo SigningJob#build vracia SigningJob a ja Autogram#handleProtectedPdfDocument musím obaliť do work threadu (inak by to celé zamrzlo), teda tak či tak by sme to museli ešte vyššie obaliť v tomto jednom prípade ked signujeme jeden súbor, keďže pri hromadnom to obalené už je a pri CLI to netreba. Avšak bolo by to asi lepšie, zmazal by sa duplikačný kód 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keď potrebuješ tie getParametersForFile, tak by som to normláne osobitne zavolal v buildFromFile a buildFromRequest - nevadí, že to bude 2x, stále to bude lepšie ako teraz volané z GUI.

A neviem úplne, čo sa stane, keď pošleš taký súbor cez API - nebude už server chcieť niečo čítať z toho súboru?

Ešte stále je to draft, API budem riešiť dnes 😅

Copy link
Contributor Author

@xhyrom xhyrom Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nemáš nejaký lepší nápad ako sa zbaviť tohto?
https://github.com/slovensko-digital/autogram/pull/498/files#diff-daf36d389667b41baf5336c039d7fc72f9132298c577ef4ec11e45dd5fe54071R114
potrebujeme proste to obaliť v threade, keďže inak celá app zamrzne

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moc pekné riešenie nemám. Ale teda dá sa poslať GUI inštancia do MainMenuControllera a potom volať rovno gui.onWorkThreadDo a nemusíš tam mať tú obaľovaciu metódu cez Autogram.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ale teda tento handleProtectedPdfDocument bude musieť ostať takto ešte pred vzniknutím SigningJobu, lebo obsah pdf potrebujeme čítať ešte pri vyrábaní parametrov. Otázka je, či to vyhodíme z tohto buildu a spravíme to ešte niekde predtým.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xhyrom tuto ešte asi posledná vec. Pre SigningJob máme 2 buildre buildFromRequest a buildFromFile. Do buildFromRequest už ide AutogramDocument s nastaveným heslom ak treba. Avšak do buildFromFile ešte nejde, AutogramDocument sa vyrába v ňom a musí sa v ňom ešte volať autogram.handleProtectedPdfDocument(document).

V prvom rade, ten buildFromRequest už nemá veľmi zmysel, lebo len zavolá private build. Tým pádom tu máme jediný špeciálny buidler tento druhý, ktorý do seba ťahá Autogram. Navrhujem build spraviť public, vyhodiť buildFromRequest a premiestniť buildFromFile do Autogramu. Tým pádom bude závislosť jendosmerná - Autogram bude používať SigningJob - teraz je to obojsmerné, lebo Autogram aj tak používa SigningJob, ale v tomto builderi musí aj SignignJob používať Autogram.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xhyrom toto si nepozeral? Presunutie toho "buildFromFile" do Autogramu.

@celuchmarek
Copy link
Member

celuchmarek commented Aug 20, 2024

@xhyrom ale mám pocit, že zabúdaš, že PDF môže byť "zaheslované" dvoma spôsobmi - encrypted vs password protected. Encrypted ani neprečítaš. Password protected sa zobrazí, ale nedá sa upravovať (napr. nejaký formulár v ňom) iba s heslom.

@xhyrom
Copy link
Contributor Author

xhyrom commented Aug 20, 2024

@xhyrom ale mám pocit, že zabúdaš, že PDF môže byť "zaheslované" dvoma spôsobmi - encrypted vs password protected. Encrypted ani neprečítaš. Password protected sa zobrazí, ale nedá sa upravovať (napr. nejaký formulár v ňom) iba s heslom.

Áno, máš pravdu. Zatiaľ neriešim správne problém ktorý sa vyskytol presne v #495

Vďaka pdftk sa mi ale podarilo reprodukovať problém - pdftk unprotected.pdf output perms-for-editing.pdf owner_pw heslo123

@xhyrom
Copy link
Contributor Author

xhyrom commented Aug 20, 2024

@celuchmarek
Copy link
Member

Okay teda. A keď hodíš encrypted alebo password protected PDF do Autogramu, tak ťa to v oboch prípadoch vyzve na zadanie hesla a potom to funguje ďalej? Hoď aj nejaké obrázky alebo video, pls.

@xhyrom
Copy link
Contributor Author

xhyrom commented Aug 22, 2024

Okay teda. A keď hodíš encrypted alebo password protected PDF do Autogramu, tak ťa to v oboch prípadoch vyzve na zadanie hesla a potom to funguje ďalej? Hoď aj nejaké obrázky alebo video, pls.

Áno, presne tak. Hodím keď budem na kompe 👍

@xhyrom
Copy link
Contributor Author

xhyrom commented Aug 22, 2024

output.mp4

@celuchmarek
Copy link
Member

Ok, super. Ešte si to vyskúšam lokálne. Vieš sem niekde hodiť tie sample PDF súbory s heslami, aby som už nemusel vyrábať ďalšie?

Navyše, pri PDF uzamknutých proti zmenám, je potrebné zadávať heslo aj pre zobrazenie dokumentu? Rozmýšľam, či v tom prípade nepýtať heslo až pri podpisovaní.

@xhyrom
Copy link
Contributor Author

xhyrom commented Aug 26, 2024

Ok, super. Ešte si to vyskúšam lokálne. Vieš sem niekde hodiť tie sample PDF súbory s heslami, aby som už nemusel vyrábať ďalšie?

Navyše, pri PDF uzamknutých proti zmenám, je potrebné zadávať heslo aj pre zobrazenie dokumentu? Rozmýšľam, či v tom prípade nepýtať heslo až pri podpisovaní.

Hneď ako budem môcť tak ich sem poposielam, čo sa týka toho zobrazenia, môžeme to spraviť, ale v podstate je to podľa mňa zbytočné.

@xhyrom
Copy link
Contributor Author

xhyrom commented Aug 27, 2024

unprotected.pdf
perms-for-editing.pdf (heslo123)
test-protected.pdf (heslo123, toto je enkryptované)

@xhyrom xhyrom marked this pull request as ready for review August 27, 2024 13:15
@xhyrom
Copy link
Contributor Author

xhyrom commented Oct 9, 2024

@xhyrom už to vyzerá dosť dobre. Pozri tých pár komentov. A potom ešte z testovania som narazil na error s batchom. Ak som zadal zlé heslo na niektorom dokumente, vrátilo to nepekný error a celý batch sa zastavil a nejako divne zasekol. Vieš to pozrieť? Neviem, či to je iba týmito heslami alebo tam je iná chyba.

Snímka obrazovky 2024-10-09 o 14 17 39 ``` digital.slovensko.autogram.core.errors.AutogramException: eu.europa.esig.dss.pades.exception.InvalidPasswordException: Encrypted document : Cannot decrypt PDF, the password is incorrect at digital.slovensko.autogram.core.AutogramBatchStartCallback.handleException(AutogramBatchStartCallback.java:35) at digital.slovensko.autogram.core.AutogramBatchStartCallback.accept(AutogramBatchStartCallback.java:26) at digital.slovensko.autogram.core.AutogramBatchStartCallback.accept(AutogramBatchStartCallback.java:9) at digital.slovensko.autogram.ui.gui.BatchDialogController.lambda$onMainButtonPressed$3(BatchDialogController.java:92) at java.base/java.lang.Thread.run(Thread.java:833) Caused by: eu.europa.esig.dss.pades.exception.InvalidPasswordException: Encrypted document : Cannot decrypt PDF, the password is incorrect at eu.europa.esig.dss.pdf.pdfbox.PdfBoxDocumentReader.(PdfBoxDocumentReader.java:115) ... at eu.europa.esig.dss.validation.SignedDocumentValidator.validateDocument(SignedDocumentValidator.java:356) at digital.slovensko.autogram.core.SignatureValidator.getSignedDocumentSimpleReport(SignatureValidator.java:175) at digital.slovensko.autogram.core.SigningJob.getParametersForFile(SigningJob.java:200) at digital.slovensko.autogram.core.SigningJob.buildFromFile(SigningJob.java:195) at digital.slovensko.autogram.ui.BatchGuiFileResponder.onBatchStartSuccess(BatchGuiFileResponder.java:67) at digital.slovensko.autogram.core.AutogramBatchStartCallback.handleSuccess(AutogramBatchStartCallback.java:42) at digital.slovensko.autogram.core.AutogramBatchStartCallback.accept(AutogramBatchStartCallback.java:24) ... 3 more ```

malo by byť opravené

var result = new PDFAStructureValidator().validate(job.getDocument());
// PDF/A doesn't support encryption
if (job.getDocument().hasOpenDocumentPassword()) {
ui.onUIThreadDo(() -> ui.onPDFAComplianceCheckFailed(job));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ešte uvažujem, či sa neplatí vyrobiť pre tento prípad inú hlášku. Prípadne vedieť do toho onPDFAComplianceCheckFailed poslať ešte AutogramException, na základe ktorej sa zobrazí taký alebo onaký text. Totiž, pri tomto hesle má zmysel povedať userovi konkrétne, že nie len že dokument nie je v súlade s PDF/A, ale je to práva preto, že je zaheslovaný.

Copy link
Member

@celuchmarek celuchmarek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ešte zostal z minula ten SigningJob.buildFromFile a pozri aj ten warning pri PDFA

@xhyrom
Copy link
Contributor Author

xhyrom commented Oct 26, 2024

ešte zostal z minula ten SigningJob.buildFromFile a pozri aj ten warning pri PDFA

Čo myslíš s tým SigningJob#buildFromFile? Viem, že si mal review pri buildFromRequest, čo som aj upravil.

@celuchmarek
Copy link
Member

Ten static SigningJob.buildFromFile chcem presunúť do instance Autogram.buildSigningJobFromFile. Mám na to commit, potrebujem iba permission do tvojho forku.

@celuchmarek celuchmarek requested a review from a team as a code owner November 16, 2024 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants