Fix for issue #252 RequestedAuthnContext: Comparison type. #270
Conversation
lib/passport-saml/config/schema.js
Outdated
| @@ -1,8 +1,8 @@ | |||
| const SCHEME = { | |||
| var SCHEME = { | |||
There was a problem hiding this comment.
It would be helpful if the commit message explained why it was necessary to replace const with var here.
There was a problem hiding this comment.
With node ver4.0, Travis fails to pass. In reality:
'const' Declarations in non-strict mode are not supported yet on Node v4.
For the sake of consistency (like the other files), I prevented the use of strict mode.
There was a problem hiding this comment.
Consistency is good, but as time marches on, I would go the other direction and enable "strict" mode and use "const". That combination is supported in Node 4.x. Node 4.x is already being EOL'ed at the end of this month and will soon no longer a target to support.
There was a problem hiding this comment.
Thanks for the great points! 👍
I'd rather create a migration branch, to have things more transparent. Can we continue with the fix as it is, and consider that migration as a separate task?
| @@ -64,6 +64,7 @@ passport.use(new SamlStrategy( | |||
| * `acceptedClockSkewMs`: Time in milliseconds of skew that is acceptable between client and server when checking `OnBefore` and `NotOnOrAfter` assertion condition validity timestamps. Setting to `-1` will disable checking these conditions entirely. Default is `0`. | |||
| * `attributeConsumingServiceIndex`: optional `AttributeConsumingServiceIndex` attribute to add to AuthnRequest to instruct the IDP which attribute set to attach to the response ([link](http://blog.aniljohn.com/2014/01/data-minimization-front-channel-saml-attribute-requests.html)) | |||
| * `disableRequestedAuthnContext`: if truthy, do not request a specific authentication context. This is [known to help when authenticating against Active Directory](https://github.com/bergie/passport-saml/issues/226) (AD FS) servers. | |||
| * `comparisonMethod`: optional comparison method used to evaluate the requested context classes or statements, one of 'exact' (default), 'minimum', 'maximum', or 'better'. | |||
There was a problem hiding this comment.
Rename this to requestedAuthContextComparison to clarify what's being compared.
|
I've reviewed this and it appears to be a spec-compliant feature addition that appears correct, documented and tested. Once the comments I've left above are addressed, it's ready to merge. |
markstos
changed the title
| @@ -8,9 +8,11 @@ var querystring = require('querystring'); | |||
| var xmlbuilder = require('xmlbuilder'); | |||
| var xmlenc = require('xml-encryption'); | |||
| var xpath = xmlCrypto.xpath; | |||
| var InMemoryCacheProvider = require('./inmemory-cache-provider.js').CacheProvider; | |||
| var Q = require('q'); | |||
There was a problem hiding this comment.
This should be rebased on master before code review continues.
|
Thanks for your review, seems the current migration has already done the job of this PR ( passport-saml/src/passport-saml/saml.ts Line 184 in 8678139 As a result, I close this PR. |
Fix for issue #252 RequestedAuthnContext: Comparison type.
The "comparisonMethod" is added as a new parameter for config. Details: