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

Move XML functions to utility module #571

Merged
merged 8 commits into from Apr 6, 2021

Conversation

forty
Copy link
Contributor

@forty forty commented Mar 22, 2021

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 Outdated Show resolved Hide resolved
src/passport-saml/xml-utils.ts Outdated Show resolved Hide resolved
@cjbarth cjbarth changed the title Forty/xml utils Move XML functions to utility module Mar 22, 2021
Copy link
Collaborator

@cjbarth cjbarth left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@markstos
Copy link
Contributor

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
Copy link
Collaborator

cjbarth commented Mar 22, 2021

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
Copy link
Contributor Author

forty commented Mar 22, 2021

@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
Copy link
Contributor Author

forty commented Mar 22, 2021

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

src/passport-saml/saml-post-signing.ts Outdated Show resolved Hide resolved
@forty
Copy link
Contributor Author

forty commented Mar 22, 2021

@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 ;)

@forty forty requested a review from cjbarth March 22, 2021 17:10
Copy link
Collaborator

@cjbarth cjbarth left a comment

Choose a reason for hiding this comment

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

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 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
Copy link
Contributor Author

forty commented Mar 22, 2021

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.

Copy link
Collaborator

@cjbarth cjbarth left a comment

Choose a reason for hiding this comment

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

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

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({
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

cjbarth commented Mar 23, 2021

@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
Copy link
Collaborator

cjbarth commented Mar 26, 2021

@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.

@cjbarth 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
@forty
Copy link
Contributor Author

forty commented Mar 29, 2021

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
Copy link
Contributor Author

forty commented Mar 29, 2021

(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 cjbarth added this to the 3.0.0 milestone Apr 4, 2021
@cjbarth
Copy link
Collaborator

cjbarth commented Apr 5, 2021

@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
Copy link
Contributor Author

forty commented Apr 5, 2021

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

Copy link
Collaborator

@cjbarth cjbarth left a comment

Choose a reason for hiding this comment

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

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

src/node-saml/types.ts Show resolved Hide resolved
cjbarth
cjbarth approved these changes Apr 6, 2021
@cjbarth cjbarth merged commit 9ad5662 into node-saml:master Apr 6, 2021
3 checks passed
@cjbarth cjbarth mentioned this pull request May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor This PR requires a semver-minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants