Update outdated dependencies #227

mcab · 2021-02-04T00:17:42Z

Picking off from #225 and 226 (#198), we're looking to address #212 in this one.

We're addressing xml-crypto in #215 because it's possibly breaking.

To merge this PR, we're looking to:

move xmlbuilder to xmlbuilder2.
bump minimum node version to 10.

underscore and xml2js (which Update dependencies #212 references) should already be updated, and don't require a version bump:

❯ npm i
npm WARN deprecated lodash-node@2.4.1: This package is discontinued. Use lodash@^4.0.0.
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN notsup Unsupported engine for xmlbuilder@2.2.1: wanted: {"node":"0.8.x || 0.10.x"} (current: {"node":"12.14.1","npm":"6.14.8"})
npm WARN notsup Not compatible with your version of node/npm: xmlbuilder@2.2.1
npm WARN testests@1.0.0 No description
npm WARN testests@1.0.0 No repository field.

added 19 packages from 76 contributors and audited 19 packages in 6.725s
found 1 high severity vulnerability
  run `npm audit fix` to fix them, or `npm audit` for details

❯ npm ls --depth=1
test@1.0.0 /[...]/test
└─┬ saml2-js@2.0.9
  ├── async@2.6.3
  ├── debug@2.6.9
  ├── underscore@1.12.0
  ├── xml-crypto@0.10.1
  ├── xml-encryption@1.2.1
  ├── xml2js@0.4.23
  ├── xmlbuilder@2.2.1
  └── xmldom@0.1.31

This addresses async being an outdated dependency. https://github.com/caolan/async/blob/b7361922b1fd231cefa13ff80bfd359f482b5e6e/CHANGELOG.md#v300 notes the major difference between v2 and v3: "Most Async methods return a Promise when the final callback is omitted, making them await-able!" In our usage of async in the library, we explicitly have callback functions specified. In our tests, we seem to do so as well. Tests (and coverage) runs fine, so I'm fine with this dependency bump.

There's some usage of debug within the library. Looking at the release logs for v3 and v4 (https://github.com/visionmedia/debug/releases/tag/3.0.0, https://github.com/visionmedia/debug/releases/tag/4.0.0), the main highlights are the expected version bumps for where the package is used. Running debug with: DEBUG=* npm run test 2> debug-output.md And comparing across v2 versus v4 didn't result in anything majorly different, besides the timestamps generated for the run. Since there's nothing that broke with this, bumping up the version.

This one is tricky. We primarily use it for DOMParser(), but we also use it for XMLSerializer(). Any major changes to these functions should give us pause. xmldom/xmldom@v0.1.31...0.2.1 The main changes here stem from HTML entity parsing and some CI changes. Should be fine to bump up. 0.3.0 added a new function (getElementsByClassName()) as well as some restructuring of the library, but that's also fine. xmldom/xmldom@0.3.0...0.4.0 0.4.0 strictly checks that the type for parseFromString() is a string. In all cases in the library where we use this, we make sure the input to the function is a string.

mcab · 2021-02-04T00:43:46Z

Updating xmlbuilder did not work smoothly. As mentioned in #110, attribute errors pop up with the test suite.

Diving deeper into what happens, on xmlbuilder with ~2.2.0:

<?xml version="1.0"?>
<AuthnRequest xmlns="urn:oasis:names:tc:SAML:2.0:protocol"
    xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" Version="2.0" ID="_5efd661713df639c7680934733fccd4fdcd3500df9" IssueInstant="2021-02-04T00:37:35.006Z" Destination="c" AssertionConsumerServiceURL="b" ProtocolBinding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" ForceAuthn="true">
    <saml:Issuer>a</saml:Issuer>
    <NameIDPolicy Format="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified" AllowCreate="true"/>
    <RequestedAuthnContext Comparison="exact">
        <saml:AuthnContextClassRef>context:class</saml:AuthnContextClassRef>
    </RequestedAuthnContext>
</AuthnRequest>

With xmlbuilder2 on ^2.4.0:

<?xml version="1.0"?>
<AuthnRequest xmlns="urn:oasis:names:tc:SAML:2.0:protocol"
    xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" Version="2.0" ID="_7b765210810fcbd46336408a9b4015f8c467fa50fe" IssueInstant="2021-02-04T00:38:02.190Z" Destination="c" AssertionConsumerServiceURL="b" ProtocolBinding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" ForceAuthn="true">
    <saml:Issuer>a</saml:Issuer>
    <NameIDPolicy Format="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified" AllowCreate="true"/>
    <RequestedAuthnContext>
        <saml:AuthnContextClassRef>context:class</saml:AuthnContextClassRef>
    </RequestedAuthnContext>
    <RequestedAuthnContext Comparison="exact"/>
</AuthnRequest>

Instead of being combined on the same node, it's treated as two separate nodes now. We need to figure out why this is the case in order to bump this library, as well as a test case to catch this behavior.

mcab · 2021-02-04T01:14:00Z

For the test case 'contains an AuthnContext if requested', we're failing on proper XML generation.

The internal representation for this happens at https://github.com/Clever/saml2/blob/master/lib/saml2.coffee#L30-L32.

We generate an array of objects, which leads to the proper generation in versions pre-2.3.0 in xmlbuilder, but fails post 2.3.0 in xmlbuilder, and xmlbuilder2:

[
  { 'saml:AuthnContextClassRef': 'context:class' },
  { '@Comparison': 'exact' }
]

Manually creating an object which has multiple keys works:

[
  {
    'saml:AuthnContextClassRef': 'context:class',
    '@Comparison': 'exact'
  }
]

The immediate concern is how we define this as class_refs, as if we can pass an array of these saml:AuthnContextClassRef. Everything online indicates that this should be a singular object, but I believe that a fix that allows for the immediate above, or:

[
  {
    'saml:AuthnContextClassRef': ['context:class', 'context:two'],
    '@Comparison': 'exact'
  }
]

would fulfill the issue. Whether or not this is valid SAML behavior is another question.

@Comparison

This requires a small code change for tests to still pass. On xmlbuilder ~2.2.0, XML creation worked with an array of objects. What this means in practice is that we were able to use a block like: ```javascript [ { 'saml:AuthnContextClassRef': 'context:class' }, { '@Comparison': 'exact' } ] ``` to make an XML block that looked like: ```xml <RequestedAuthnContext Comparison="exact"> <saml:AuthnContextClassRef>context:class</saml:AuthnContextClassRef> </RequestedAuthnContext> ``` Any xmlbuilder version past that would generate XML that looked like: ```xml <RequestedAuthnContext> <saml:AuthnContextClassRef>context:class</saml:AuthnContextClassRef> </RequestedAuthnContext> <RequestedAuthnContext Comparison="exact"/> ``` However, if we change the context node to be a flat object: ```javascript { 'saml:AuthnContextClassRef': ['context:class', 'context:two'], '@Comparison': 'exact' } ``` we pass the tests.

This provides more context for failing the case where Comparison=exact is not found as an attribute on the target node.

mcab · 2021-02-04T01:38:57Z

I ended up resolving this by creating the object, and passing that through to xmlbuilder.

This leaves xml-crypto as the last one to update, which is being done in #215.

❯ npm outdated
Package     Current  Wanted  Latest  Location
xml-crypto   0.10.1  0.10.1   2.0.0  saml2-js

mcab · 2021-02-04T01:43:28Z

This seems to be functionally the same as current master, but I'll bump the minor version in case that this does turn out to be breaking. It shouldn't, but 🤷‍♂️ .

@Val

The upgrade to `xmlbuilder2` broke some generated XMLs, the metadata is part of them as of [AuthNRequest](#227 (comment)), there is probably more if the XML is build with arrays. Explanation, given this code: ``` root.ele({ number: [ "one", "two", { three: { "@Val": 3 } } ] }); ``` `xmlbuilder` used to [render array like that](https://github.com/oozcitak/xmlbuilder-js/wiki/Conversion-From-Object#arrays): ``` <number> <one/> <two/> <three val="3"/> </number> ``` And `xmlbuilder2` now [renders them like that](https://oozcitak.github.io/xmlbuilder2/object-conversion.html): ``` <number>one</number> <number>two</number> <number> <three val="3"/> </number> ``` There was the flag `separateArrayItems` in `xmlbuilder` to change the behavior but I do not see it in `xmlbuilder2`.

mcab added 3 commits February 3, 2021 14:29

mcab self-assigned this Feb 4, 2021

mcab added 2 commits February 3, 2021 17:31

test: Update 'contains an AuthnContext if requested'

8afe878

This provides more context for failing the case where Comparison=exact is not found as an attribute on the target node.

chore: Bump minimum node version to >=10.x

548d87f

mcab merged commit 6f6d903 into master Feb 4, 2021

mcab deleted the mcab/update-outdated-dependencies branch February 4, 2021 01:43

mcab mentioned this pull request Feb 4, 2021

Update dependencies #212

Closed

artonge mentioned this pull request Feb 8, 2021

Fix create_metadata output #229

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update outdated dependencies #227

Update outdated dependencies #227

Uh oh!

mcab commented Feb 4, 2021

Uh oh!

mcab commented Feb 4, 2021 •

edited

Loading

Uh oh!

mcab commented Feb 4, 2021 •

edited

Loading

Uh oh!

mcab commented Feb 4, 2021

Uh oh!

mcab commented Feb 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Update outdated dependencies #227

Update outdated dependencies #227

Uh oh!

Conversation

mcab commented Feb 4, 2021

Uh oh!

mcab commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcab commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcab commented Feb 4, 2021

Uh oh!

mcab commented Feb 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mcab commented Feb 4, 2021 •

edited

Loading

mcab commented Feb 4, 2021 •

edited

Loading