To support multiple authnContext #183
Conversation
…the new xml parser
…the new xml parser: corrected a minor syntax error
…the new xml parser: corrected a minor syntax error#2
…the new xml parser: corrected a minor syntax error#3
…le version of node3
…le version of node#1
…le version of node#2
…le version of node#3
…le version of node#4
…le version of node#5
…le version of node#6
…le version of node#7
…le version of node#8
…le version of node#9
|
I have updated the test scripts with the help of xdmnl commit |
|
I didnt know why it was failing for this Node stable version |
Dhiviya-Bharathi
changed the title
|
It would make my life much easier if this fix was merged into the master branch... |
|
I really don't like the breaking change of making this an array even for a single value. The function should detect if array or not an act accordingly, and the tests should test for both scenarios. |
|
Interestingly the way this was implemented in this PR should technically be non-breaking as the option is still specified as a string, just comma-separated.. That being said, I'd change the implementation as @cjbarth recommended as it is better practice than using comma-separated strings: options.authnContext = "some authn context"
// OR
options.authnContext = ["array", "of", "contexts"] |
|
Any ETA on when this request will make it into passport-saml? |
|
@Babsvik @tjbeers We are currently waiting for:
|
|
Is anyone working on merging this in? |
|
@illion20 I'm aware of no action since the call for refinements on Jan 3rd. Are you are interested in taking on the refinements requested then to get it merged? |
|
@markstos I was wondering since I already specify the authncontext as an array in the current version and it seems to work, at least I get authenticated through my ADFS. Or perhaps its just ignoring the it? |
|
@illion20 I'm not an expert in that part of the code. Considering reviewing the automated test coverage we have and adding an additional test if you are unsure if it's actually working. |
| self.options.authnContext.split(',').forEach(function(value, index) { | ||
| authnContextInArray.push({ | ||
| '@xmlns:saml': 'urn:oasis:names:tc:SAML:2.0:assertion', | ||
| '#text': self.options.authnContext.split(',')[index] |
There was a problem hiding this comment.
What about just using value?
There was a problem hiding this comment.
@cadesalaberry Could you say more about what you see that could be improved here?
There was a problem hiding this comment.
I believe he is wondering why not use value instead of self.options.authnContext.split(',')[index]
+ self.options.authnContext.split(',').forEach(function(value, index) {
+ authnContextInArray.push({
+ '@xmlns:saml': 'urn:oasis:names:tc:SAML:2.0:assertion',
+ '#text': valueThere was a problem hiding this comment.
I vote for making it an array even if its one value, but I have no status here:
+ self.options.authnContext.forEach( context => {
+ authnContextInArray.push({
+ '@xmlns:saml': 'urn:oasis:names:tc:SAML:2.0:assertion',
+ '#text': context
There was a problem hiding this comment.
My comment earlier favored detecting if the incoming value was an array or a single value and switching accordingly. That prevents breakage.
There was a problem hiding this comment.
@cjbarth Couldn't either be valid according to the SAML spec? Is the breakage you referring backwards compatibility in the sense that we would continue to handle single values as before?
There was a problem hiding this comment.
Yes @markstos , that is the breakage I'm referring to. We should break backwards compatibility of passing in a single value if we don't have to, and in this case I don't think we have to.
There was a problem hiding this comment.
I'm with @cjbarth on using an array instead of comma-separated.
For #252 we could extend an array implementation with detecting string vs objects (e.g.: [{ref: 'urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport', comparisonType: 'minimum'}]). That would be a pain with a comma-separated value.
|
Aside from changing this to use an array instead of CSV and making sure that backwards compatibility with |
|
@cjbarth I don't think there's anything hold this back. Your clean-up help would be welcome here. |
This fix is to solve the issue raised by me #181. To support multiple authentication method in passport saml. I have upgraded xmlbuilder module from 2.5.6 to 4.1.x from which array parsing is supported.
While configuring authnContext we have to specify as one dimensional array(even for single authntication method - may be we can make it flexible if u want).