Move XML functions to utility module #571

forty · 2021-03-22T10:07:32Z

Description

This PR moves most of the XML but non SAML logic to a dedicated file. It tries to improve some types when it's possible.
The idea is:
1- to isolate dependencies, which I think is always a good idea. Especially when those dependencies are non in TS, this allows to group the "typing danger zone"
2- shrink saml.ts by moving as much as possible non SAML related logic out.

This PR should not break anything and is done without updating any tests as a result.

Possible follow ups:

Create a similar crypto-utils for everything crypto (PEM conversion, etc)
See if xmlbuilder dependency could be dropped in favor of xml2js builder (which is built using xmlbuilder I think). This would avoid depending on 2 different version of xmlbuilder (v15 as a direct dep, v11 via xml2js)

This PR is best reviewed commit per commit.

Let me know what you think.

src/passport-saml/xml-utils.ts

cjbarth

Note for future: I'd love to get ride of non-null assertions in favor of an actual runtime validation of the data.

src/passport-saml/xml-utils.ts

markstos · 2021-03-22T15:43:43Z

I like the idea and recommend dropping "util" from the directory name, leaving it as just "xml". The word "utility" doesn't convey much useful information. It implies minor or miscellaneous functions but I think that's implied by it being an internal library.

cjbarth · 2021-03-22T16:00:50Z

As I look more at what @markstos is saying, it seems we might also be able to consolidate some stuff from utility.ts as well since two of the functions there are actually XML functions.

forty · 2021-03-22T16:19:53Z

@cjbarth Yes, true, I did see them then forgot to mention them. I was not 100% sure I should move them, at least one of them (signXmlResponse) seems to be SAML logic to me (it's about a Response element)

I think there is actually a nice factorization opportunity with saml-post-signing, so I will move the signXml function the xml module and make it generic enough so it can be used by the port signing too.
I might move signAuthnRequestPost and signXmlResponse back to saml.ts, unless you think otherwise?

forty · 2021-03-22T16:32:56Z

(after digging a bit more signXmlResponse might actually belong to the test file where it's used)

src/passport-saml/saml-post-signing.ts

forty · 2021-03-22T17:07:23Z

@cjbarth a @markstos Added one commit to move the signXml function to xml module. The change in this commit is slightly more involved than previous commits in this PRs, so review it carefully :) In particular, I probably fixed or broke something, see my comment in the code ;)

I have not touched to the function calling signXml yet, I think their destiny could be decided later, in order not to make that PR bigger that it is already ;)

cjbarth

I really like where you are going with this, but your last commit on this branch is a step too far and breaks stuff. I'd like to get a test written that catches this. It should be an easy test. If we take "should sign a simple saml request" and "should place the Signature element after the Issuer element" and compare the signatures generated with the same settings, they shouldn't be equal.

src/passport-saml/saml.ts

src/passport-saml/xml.ts

src/passport-saml/saml-post-signing.ts

forty · 2021-03-22T21:12:42Z

I improved the existing tests, I don't see any reason to resort to regexp parsing xml if everything is deterministic and we can check the full result. I confirmed the tests failed with the previous version and fixed.

cjbarth

This looks good to me. I'll leave this open for a day or so before landing it to let others review.

cjbarth · 2021-03-23T12:53:35Z

test/saml-post-signing-tests.spec.ts

-    result.should.match(/<DigestValue>[A-Za-z0-9/+=]+<\/DigestValue>/);
-    result.should.match(/<SignatureValue>[A-Za-z0-9/+=]+<\/SignatureValue>/);
+    const doc = await parseXml2JsFromString(result);
+    doc.should.be.deepEqual({


This is certainly more robust than what we had before. I think this is why a lot of tests compare to a static file: this makes the individual test not fit on a single screen. I can't think of a great solution right now, so I'm not looking for a change, but am open to ideas, perhaps in a follow-on PR.

cjbarth · 2021-03-23T12:55:38Z

@markstos This looks good to me, but I wanted to leave it open to give you a chance to look in case you're interested.

cjbarth · 2021-03-26T21:53:16Z

@forty If you're OK with it, I'd like to hold off on landing this until #574 lands. We have been trying to get #574 done for a while and I'd like to not complicate the already herculean efforts to split out saml.ts to its own project with conflicts from this landing. That will likely mean that you're work will need to have some merge conflicts resolved.

I still want to work with you and #574 to make sure these both land before we release 3.x in a few weeks if you're up for it.

forty · 2021-03-29T19:02:45Z

Haha, well I cannot say I'm overly enthusiastic to having to tackle that conflict that promises to be huge, but I'll trust you in choosing the order of merge and will do my best to rebase quickly if needed :)

forty · 2021-03-29T19:05:53Z

(and I have to add, I'm one of those that would be more then happy to get rid of the passport dependencies, so i know it's for a good cause :) )

cjbarth · 2021-04-05T17:45:54Z

@forty We've landed that big PR to start the separation of node-saml from the passport strategy parts. If you would like to revisit your changes here, I'll try to keep up with you in reviewing them.

It adds better typings and already allowed to catch an incorrect type. It will also make the code safer since the returned type is checked at runtime.

forty · 2021-04-05T20:50:43Z

@cjbarth Done for that one too! It was easier than expected 😅

cjbarth

It looks good as it is, but I notice that there may be a duplicate helper function. See below.

src/node-saml/types.ts

forty commented Mar 22, 2021

View reviewed changes

src/passport-saml/xml-utils.ts Outdated Show resolved Hide resolved

forty commented Mar 22, 2021

View reviewed changes

src/passport-saml/xml-utils.ts Outdated Show resolved Hide resolved

cjbarth changed the title ~~Forty/xml utils~~ Move XML functions to utility module Mar 22, 2021

cjbarth requested changes Mar 22, 2021

View reviewed changes

src/passport-saml/xml-utils.ts Outdated Show resolved Hide resolved

forty force-pushed the forty/xml-utils branch from 23cbb26 to 4aa7e07 Compare March 22, 2021 16:05

forty commented Mar 22, 2021

View reviewed changes

src/passport-saml/saml-post-signing.ts Outdated Show resolved Hide resolved

forty requested a review from cjbarth March 22, 2021 17:10

cjbarth requested changes Mar 22, 2021

View reviewed changes

src/passport-saml/saml.ts Outdated Show resolved Hide resolved

src/passport-saml/xml.ts Outdated Show resolved Hide resolved

src/passport-saml/saml-post-signing.ts Outdated Show resolved Hide resolved

forty force-pushed the forty/xml-utils branch from d8dcccd to 147f99a Compare March 22, 2021 21:05

forty requested a review from cjbarth March 22, 2021 21:12

forty force-pushed the forty/xml-utils branch from 147f99a to e3445a5 Compare March 23, 2021 08:08

cjbarth approved these changes Mar 23, 2021

View reviewed changes

cjbarth added semver-major This change requires at least a semver-major version bump semver-minor This PR requires a semver-minor version bump and removed semver-major This change requires at least a semver-major version bump labels Mar 27, 2021

cjbarth added this to the 3.0.0 milestone Apr 4, 2021

forty force-pushed the forty/xml-utils branch from e3445a5 to ab4e003 Compare April 5, 2021 20:37

forty added 2 commits April 5, 2021 22:38

Move all xpath select to xml module

86650bd

It adds better typings and already allowed to catch an incorrect type. It will also make the code safer since the returned type is checked at runtime.

Move xml-crypto and xmlenc operations to xml module

833fb4a

forty and others added 6 commits April 5, 2021 22:45

Move xmldom parsing to xml module

b876772

Move xml2js to xml module

beda308

Move xmlbuilder to xml module

512fb61

Improve signature tests to better detect regressions

0816f14

Move signXML function to xml module

4a4ab15

Factorize parseDomFromString in xml module

3a6f50c

forty force-pushed the forty/xml-utils branch from ab4e003 to 3a6f50c Compare April 5, 2021 20:47

cjbarth reviewed Apr 6, 2021

View reviewed changes

src/node-saml/types.ts Show resolved Hide resolved

cjbarth approved these changes Apr 6, 2021

View reviewed changes

cjbarth merged commit 9ad5662 into node-saml:master Apr 6, 2021

cjbarth mentioned this pull request May 10, 2021

Changelog #587

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move XML functions to utility module #571

Move XML functions to utility module #571

forty commented Mar 22, 2021 •

edited

cjbarth left a comment

markstos commented Mar 22, 2021

cjbarth commented Mar 22, 2021

forty commented Mar 22, 2021

forty commented Mar 22, 2021

forty commented Mar 22, 2021 •

edited

cjbarth left a comment

forty commented Mar 22, 2021

cjbarth left a comment

cjbarth Mar 23, 2021

cjbarth commented Mar 23, 2021

cjbarth commented Mar 26, 2021

forty commented Mar 29, 2021

forty commented Mar 29, 2021

cjbarth commented Apr 5, 2021

forty commented Apr 5, 2021

cjbarth left a comment

Move XML functions to utility module #571

Move XML functions to utility module #571

Conversation

forty commented Mar 22, 2021 • edited

Description

cjbarth left a comment

Choose a reason for hiding this comment

markstos commented Mar 22, 2021

cjbarth commented Mar 22, 2021

forty commented Mar 22, 2021

forty commented Mar 22, 2021

forty commented Mar 22, 2021 • edited

cjbarth left a comment

Choose a reason for hiding this comment

forty commented Mar 22, 2021

cjbarth left a comment

Choose a reason for hiding this comment

cjbarth Mar 23, 2021

Choose a reason for hiding this comment

cjbarth commented Mar 23, 2021

cjbarth commented Mar 26, 2021

forty commented Mar 29, 2021

forty commented Mar 29, 2021

cjbarth commented Apr 5, 2021

forty commented Apr 5, 2021

cjbarth left a comment

Choose a reason for hiding this comment

forty commented Mar 22, 2021 •

edited

forty commented Mar 22, 2021 •

edited