-
Notifications
You must be signed in to change notification settings - Fork 17
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
fix: update libs #547
fix: update libs #547
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #547 +/- ##
=======================================
Coverage 78.87% 78.87%
=======================================
Files 35 35
Lines 781 781
=======================================
Hits 616 616
Misses 165 165 ☔ View full report in Codecov by Sentry. |
BREAKING-CHANGE: update saml strategy name
Kudos, SonarCloud Quality Gate passed! |
@@ -7,7 +7,7 @@ const R = require('ramda') | |||
// This is wrapped in a function so params is not evaluated upon module load, only at first usage | |||
const params = R.once(() => [ | |||
{ | |||
strategy: 'passport-saml', | |||
strategy: '@node-saml/passport-saml', | |||
passportAuthnParams: {}, | |||
options: { | |||
validateInResponseTo: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@node-saml/passport-saml
and @node-saml/node-saml
versions >= 4.x
(currently latest published vesion) has a bug which disables inResponseTo
validation if configuration value is set to true
(or anything else except one of the enum values introduced at PR node-saml/node-saml#40 ). See following bug report for further information:
Upcoming @node-saml/node-saml
(5.x
) has this fix node-saml/node-saml#314 which would throw exception if value is not one of the enum values.
At the moment this configuration and possible end user supplied configurations with value true
shall not enable inResponseTo
validation.
As a result of aforementioned inResponseTo
issue at least this code might need some changes due to this PR:
gluu-passport/server/providers.js
Line 97 in 7fc4ee2
if (providerOptions.validateInResponseTo) { |
i.e. to respect enum values instead of simple
true
/false
.
Sidenote:
You might want to inform end users about other breaking changes which include but are not limited to e.g.:
audience
is now mandatory configuration optionwantAuthnResponseSigned
andwantAssertionsSigned
are set by default totrue
. Most IdPs signs by default only assertion or response so users shall run into issues because@node-saml/passort-saml
reportsInvalid document signature
orInvalid signature
if one of those are missing. There has already been huge number of issues related to these configuration parameters. Root causes has been that people have not read@node-saml/passport-saml
and@node-saml/node-saml
change logs prior to migrate and/or just ignored change logs / documentation / not inspecting IdP configuration prior to start using SAML authentication). Here is list of example issues you can expect once this PR goes to production- NOTE:
@node-saml/passport-saml
's documentation related to core saml configuration parameters is not quite up to date.@node-saml/node-saml
(which is used internally by@node-saml/passport-saml
) contains most up to date description of core saml configuration parameters. - https://github.com/node-saml/passport-saml/blob/v4.0.4/CHANGELOG.md
- https://github.com/node-saml/node-saml/blob/v4.0.5/CHANGELOG.md
- see also
4.0.0-beta*
changes
- see also
- NOTE:
ping @kdhttps @moabu @ossdhaval @yurem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created following issue to track whats being mentioned above:
update libraries
Breaking changes
@node-saml/passport-saml