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

Adds functions for creating a PDF PAdES and adding a signature [SFEQS-966] #1

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

Conversation

grausof
Copy link
Collaborator

@grausof grausof commented Sep 6, 2022

This PR implement:

  1. generatePadesFile
  2. addSignatureToPadesFile

@grausof grausof requested a review from gunzip as a code owner September 6, 2022 12:18
Copy link

@gunzip gunzip left a comment

Choose a reason for hiding this comment

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

May you add some comments to clarify what's happening?

void generatePadesFile(
File originalPdf,
OutputStream outputStream,
String signatureFieldId
Copy link

Choose a reason for hiding this comment

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

what if we have multiple signature fields?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No problem, you have to indicate the signatureFieldId. If the signature already exists on that signatureFieldId an exception is thrown!


private static final Logger LOG = LoggerFactory.getLogger(Utility.class);

static ByteRange getByteRange(DSSDocument documentToSign, String signatureFieldId)
Copy link

Choose a reason for hiding this comment

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

can you add some comments clarifying what's happening here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

surely. I will try to clarify as best I can!

@@ -0,0 +1,36 @@
package it.pagopa.dss;
Copy link

Choose a reason for hiding this comment

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

do you think this coverage is enough?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Still lack addSignatureToPadesFile coverage!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added new test and coverage report!

Copy link

Choose a reason for hiding this comment

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

brilliant!

);

assertThat(signedByte).isEqualTo(signatureValue);
} catch (Exception e) {
Copy link

@gunzip gunzip Sep 7, 2022

Choose a reason for hiding this comment

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

I don't know the Java framework but if any exceptions occurs here it looks like it's silently swallowed and the asertion would not be called, maybe it's better to avoid to catch it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, test fails anyway. However it is best to add a throws Exception.

Copy link

Choose a reason for hiding this comment

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

Why it fails anyway? Because no assertion was called?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, i thought that without a call to assertThat the test failed instead it doesn't. I fixed by adding throws Exception on test function.

@pagopa pagopa deleted a comment from github-actions bot Sep 7, 2022
@pagopa pagopa deleted a comment from github-actions bot Sep 7, 2022
@pagopa pagopa deleted a comment from github-actions bot Sep 7, 2022
@pagopa pagopa deleted a comment from github-actions bot Sep 7, 2022
@pagopa pagopa deleted a comment from github-actions bot Sep 7, 2022
@github-actions

This comment was marked as duplicate.

@pagopa pagopa deleted a comment from github-actions bot Sep 7, 2022
@github-actions
Copy link

github-actions bot commented Sep 7, 2022

File Coverage [42.29%]
SignatureService.java 100% 🍏
SignatureServiceException.java 100% 🍏
SignatureServiceImpl.java 91.72% 🍏
Utility.java 58.39%
PdfBoxDict.java 21%
PdfBoxArray.java 17.78%
Total Project Coverage 42.29% 🍏

@github-actions
Copy link

github-actions bot commented Sep 9, 2022

File Coverage [42.29%]
SignatureService.java 100% 🍏
SignatureServiceException.java 100% 🍏
SignatureServiceImpl.java 91.72% 🍏
Utility.java 58.39%
PdfBoxDict.java 21%
PdfBoxArray.java 17.78%
Total Project Coverage 42.29% 🍏

//converting the file into a DSS document (from dss library)
DSSDocument documentToSign = new FileDocument(originalPdf);

//initialization of PAdES parameters
Copy link

Choose a reason for hiding this comment

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

this kind of comments add little value (are self-evident). instead I'd try to elaborate more ie. on what

parameters.setGenerateTBSWithoutCertificate(true);

does and why it must be set to true.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK!

src/main/java/it/pagopa/dss/SignatureServiceImpl.java Outdated Show resolved Hide resolved
src/main/java/it/pagopa/dss/SignatureServiceImpl.java Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Sep 9, 2022

File Coverage [42.29%]
SignatureService.java 100% 🍏
SignatureServiceException.java 100% 🍏
SignatureServiceImpl.java 91.72% 🍏
Utility.java 58.39%
PdfBoxDict.java 21%
PdfBoxArray.java 17.78%
Total Project Coverage 42.29% 🍏

@pagopa pagopa deleted a comment from github-actions bot Sep 12, 2022
@pagopa pagopa deleted a comment from github-actions bot Sep 12, 2022
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.

2 participants