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

fix: update libs #547

Merged
merged 14 commits into from
Nov 24, 2023
Merged

fix: update libs #547

merged 14 commits into from
Nov 24, 2023

Conversation

kdhttps
Copy link
Contributor

@kdhttps kdhttps commented Nov 22, 2023

update libraries

Breaking changes

  • need to update passport-saml script name in provider to @node-saml/passport-saml
    image

@kdhttps kdhttps self-assigned this Nov 22, 2023
Copy link

codecov bot commented Nov 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dbe3511) 78.87% compared to head (7fc4ee2) 78.87%.
Report is 21 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

@kdhttps kdhttps marked this pull request as ready for review November 22, 2023 16:50
BREAKING-CHANGE: update saml strategy name
Copy link

sonarcloud bot commented Nov 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@yurem yurem merged commit 45630fc into master Nov 24, 2023
11 of 12 checks 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,
Copy link

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:

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 option
  • wantAuthnResponseSigned and wantAssertionsSigned are set by default to true. Most IdPs signs by default only assertion or response so users shall run into issues because @node-saml/passort-saml reports Invalid document signature or Invalid 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

ping @kdhttps @moabu @ossdhaval @yurem

Copy link

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants