Skip to content

Expose ValidateInResponseTo as it is required in options#220

Merged
cjbarth merged 2 commits intonode-saml:masterfrom
ghusse:expose-validate-in-response-to
Nov 17, 2022
Merged

Expose ValidateInResponseTo as it is required in options#220
cjbarth merged 2 commits intonode-saml:masterfrom
ghusse:expose-validate-in-response-to

Conversation

@ghusse
Copy link
Contributor

@ghusse ghusse commented Nov 17, 2022

Description

Options required by passport-saml when creating a new configuration require the property validateInResponseTo to use the enum ValidateInResponseTo. But this enum is not easily accessible in @node-saml/passport-saml.

In typescript, we need to import @node-saml/node-saml/lib/types to use this enum, which requires to add the dependency to @node-saml/node-saml to a project that would only require @node-saml/passport-saml.

By exposing this enum in node-saml, it'll be also exposed by passport-saml once updated to the latest version of its dependency.

Checklist:

  • Issue Addressed: [ ]
  • Link to SAML spec: [ ]
  • Tests included? [ ]
  • Documentation updated? [ ]

@codecov
Copy link

codecov bot commented Nov 17, 2022

Codecov Report

Merging #220 (ff12e7c) into master (939879d) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #220      +/-   ##
==========================================
+ Coverage   80.70%   80.75%   +0.04%     
==========================================
  Files          11       11              
  Lines         819      821       +2     
  Branches      251      251              
==========================================
+ Hits          661      663       +2     
  Misses         68       68              
  Partials       90       90              
Impacted Files Coverage Δ
src/index.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cjbarth cjbarth merged commit 51e3876 into node-saml:master Nov 17, 2022
@cjbarth cjbarth added the bug Something isn't working label Nov 17, 2022
@cjbarth
Copy link
Collaborator

cjbarth commented Nov 17, 2022

@ghusse , even though I merged this, please see some of these other linked PRs and offer your comments. There seems to be a bit of discussion around types and I want to make sure your experience it taken into account.

@ghusse
Copy link
Contributor Author

ghusse commented Nov 18, 2022

Ok, thanks. I'll take a look.

Let me know when the new version will be released, I'll probably need to make another change on passport-saml in order to reference this new version as a dependency in order to have access to it.

@cjbarth
Copy link
Collaborator

cjbarth commented Nov 18, 2022

I will land this and do releases once we figure out what the best approach forward is. It does seem though that if we stop exporting everything, then it will have to wait to be a semver-major change. If we just add to and re-work the exports, we can keep it as a semver-minor change; we can always remove the extra exports with v5 Q1 2023.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants