-
Notifications
You must be signed in to change notification settings - Fork 190
Description
This is a proposal to remove the originalXmlWithIds
property and the getOriginalXmlWithIds()
function from the library.
In the documentation it says it is for "validation" but that seems to be a misinterpretation of the original intent.
It seems to be a relic from the first commit, when there was no way to specify where you wanted to insert the signature.
But since we now have the ComputeSignatureOptionsLocation
where the location of the signature can be specified, there seems to no longer be a need for this function.
It also creates a copy of the entire XML string inside the library, which is an unnecessary memory cost, especially when working with large XML files.
Within the library this is only used for some tests regarding Id generation.
These tests could be rewritten to be DOM aware and work on the signed document.
For example this:
const originalXmlWithIds = sig.getOriginalXmlWithIds();
const expectedOriginalXmlWithIds =
'<root><x xmlns="ns" Id="_0"/><y attr="value" Id="_1"/><z><w Id="_2"/></z></root>';
expect(originalXmlWithIds, "wrong OriginalXmlWithIds").to.equal(expectedOriginalXmlWithIds);
could be refactored to this:
const signedDoc = new xmldom.DOMParser().parseFromString(signedXml);
const xId = xpath.select1("//*[local-name(.)='x']/@*[local-name(.)='Id']", signedDoc);
isDomNode.assertIsAttributeNode(xId);
expect(xId.textContent).to.equal("_0");
const yId = xpath.select1("//*[local-name(.)='y']/@*[local-name(.)='Id']", signedDoc);
isDomNode.assertIsAttributeNode(yId);
expect(yId.textContent).to.equal("_1");
const wId = xpath.select1("//*[local-name(.)='w']/@*[local-name(.)='Id']", signedDoc);
isDomNode.assertIsAttributeNode(wId);
expect(wId.textContent).to.equal("_2");
I am willing to do this as part of my PR #506 if this is acceptable.