refactor(jsignpdf): move the hash algorithm resolution to a dedicated class - #8275
Merged
vitormattos merged 2 commits intoSep 6, 2026
Merged
Conversation
vitormattos
requested changes
Sep 6, 2026
15 tasks
… class The hash algorithm that signs a document depends on the PDF version, so it is not a valid answer for the other hashes JSignPdf takes: reusing it would send SHA1 to a timestamp authority whenever the document is older than PDF 1.6. Issue LibreSign#8145 asks for each hash to be resolved on its own, which is hard to guarantee while the rules live inside the handler as private methods. Move getHashAlgorithm(), getHashAlgorithmForPdfVersion(), validateHashAlgorithm() and requiresPdfVersionUpgradeForSha256() to HashAlgorithmResolver, where each hash gets its own entry point and can be tested in isolation. The handler keeps reading the PDF version, the only part that is about the document and not about the policy. No behavior change: the same version thresholds, the same fallback to SHA256 and the same supported algorithms. Signed-off-by: André Maia <andrefnkmm@gmail.com> Assisted-by: Claude Code:claude-opus-5
Both the handler and the resolver carry rules that only make sense for
JSignPdf — the PDF version thresholds come from its own HashAlgorithm enum —
so they move to SignEngine/JSignPdf/ and the resolver keeps a name that does
not need to repeat the engine.
Pkcs12Handler used to build the handler class name at runtime from the
configured engine ('...\SignEngine\' . ucfirst($property)), a string no
static analysis, IDE rename or grep could follow: moving the class would
only fail when a document was signed. It now resolves through an explicit
map, so a name that is not an engine answers the same friendly error instead
of reaching the container.
Signed-off-by: André Maia <andrefnkmm@gmail.com>
Assisted-by: Claude Code:claude-opus-5
maia-andre
force-pushed
the
refactor/8145-hash-algorithm-resolver
branch
from
September 6, 2026 14:41
09233c8 to
a989900
Compare
Member
|
/backport to stable35 |
Member
|
/backport to stable34 |
Member
|
/backport to stable33 |
This was referenced Sep 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves: #
📝 Summary
Item 3 of the scope described in #8145: the hash algorithm resolution leaves
JSignPdfHandlerandbecomes a small dedicated class with isolated tests.
The algorithm that signs the document depends on the PDF version — JSignPdf still needs SHA-1 below
PDF 1.6 and refuses it from 1.7 on — so its result describes the signature only. Reusing it for the
other hashes JSignPdf takes would send SHA-1 to a timestamp authority every time the document is
older than PDF 1.6. While these rules lived in the handler as private methods there was no place
where that distinction could be stated or tested.
HashAlgorithmResolvernow owns them, one entry point per hash:forSignature(?float $pdfVersion): the version-aware algorithm that goes to--hash-algorithm.requiresPdfVersionUpgradeForSha256(float $pdfVersion): whether the document has to be rewrittenas PDF 1.6 before the configured SHA-256 is accepted.
extractPdfVersion()stays in the handler: reading the document is the handler's job, not thepolicy's.
No behavior change — same version thresholds, same fallback to SHA256, same list of supported
algorithms, same parameters reaching JSignPdf. The TSA hash algorithm as a TSA policy option (items
1 and 2 of the issue) comes in a following PR, and is the reason each hash gets its own entry point
here.
🧪 How to test
HashAlgorithmResolverTestcovers the resolution in isolation (no Nextcloud server, only a mockedPolicyService): every PDF version threshold, the unsupported/empty/unset configured algorithm, andthe PDF version upgrade rule.
JSignPdfHandlerTestkeeps proving, end to end, which--hash-algorithmreaches JSignPdf for each document.⚙️ API / Back‑end changes
HashAlgorithmResolver(new) resolves the signature hash algorithm and the PDF version upgrade ruleJSignPdfHandlerreceives it by dependency injection and no longer resolves the hash policy itselfNo API, capability or documentation change: the class is internal and autowired.
✅ Checklist
AEngineHandlerTest::testSetEngineConfiguresIdentifyMethodsForNoneEnginefailures: that class passes 26/26 when it runs alone, so they come from state left by other test
classes, not from this change (it does not touch the certificate engine)
🤖 AI (if applicable)