Allow for authnRequestBinding in SAML options

[mhesler74]

Description

Allow setting the SAML authentication request binding option when using MultiSamlStrategy.

Checklist:

• Issue Addressed: [528 - https://github.com/[BUG] Unable to override default authnRequestBinding when using MultiSamlStrategy  #528]

[cjbarth]

@mhesler74 , it seems as if you are changing working tests to meet your code changes. I expected to see the existing tests continue to work and a new tests that will fail without your code change and will pass with it. The way I understand it, the current behavior is incomplete, not wrong. Is that understanding correct?

[mhesler74]

@cjbarth I don't think so since the _authnRequestBinding variable that the tests were verifying doesn't exist any longer.
I figured this was Okay since it seems to be an undocumented internal variable.

[cjbarth]

I think I'm referring more to the multiSamlStrategy removal of authnRequestBinding and the migration of that to SAMLOptions from StrategyOptions. Also, it does seem that authnRequestBinding is a documented option in the readme file and that it is supposed to be specific to a given strategy.

[mhesler74]

I don't see where it is documented that authnRequestBinding is strategy bound. The readme seems to indicate otherwise and lists it under Additional SAML behaviors which lead to me discovering the problem when I tried to enforce the different authentication request methods for different tenants. Not all of them support HTTP-POST and vice-versa so I need some mechanism to pick.

[cjbarth]

I apologize that I don't have a longer block of time right now to actually dig into the code and trace through the code changes you've made and their implications. I'm instead relying on reading the tests to make sure that existing behaviors are preserved and new behaviors have coverage.

[mhesler74]

@cjbarth I appreciate the feedback and I also like to see good test coverage but I'm a little lost on exactly where I should be adding a test to cover this. There doesn't appear to be any existing test case that covers the authnRequestBinding when it is set for HTTP-POST. The call to SAML.getAuthorizeForm is quite different from SAML.getAuthorizeUrl in that it doesn't take the options but instead relies on the properties that were set when the SAML object was initialized. That case is covered by the "uses given options to setup internal saml provider" test.

[cjbarth]

@mhesler74 I can sympathize. This is what I would suggest: 1) start with the HEAD of master and write a test, based on your existing situation, that fails because passport-saml isn't working as you need it to, then 2) make your code changes to get that test to pass. (See TDD.)

I might actually suggest that you make a draft PR with just the test that is failing as that will allow the maintainers to see more clearly what the problem is. They may be able to offer usage or resolution directions that they have gleaned from experience.

[mhesler74]

@cjbarth What I'm trying to say is I don't know how I'd even go about writing a failing test given how the existing code creates a whole new SAML object within the function itself.

      const samlService = new saml.SAML({...this._options, ...samlOptions});
      const strategy = Object.assign({}, this, {_saml: samlService});
      Object.setPrototypeOf(strategy, this);
      super.authenticate.call(strategy, req, options);

How do I gain access to that _saml object to determine if it correctly called getAuthorizeForm vs getAuthorizeUrl?

[cjbarth]

@mhesler74 I see what you are saying. I believe there is a way to figure that out, I'll see if I can have a look and find it. If not, we may need a little more test scaffolding, since it does have a real-life impact, so we are able to test it.

[mhesler74]

I committed my work in progress. Something strange is going on since I can get the tests working properly in a separate test file.. but putting them into the multiSamlStrategy.js test file doesn't work. Same exact test

…

On Wed, Feb 3, 2021 at 2:22 PM Chris Barth ***@***.***> wrote: @mhesler74 <https://github.com/mhesler74> I see what you are saying. I believe there is a way to figure that out, I'll see if I can have a look and find it. If not, we may need a little more test scaffolding, since it does have a real-life impact, so we are able to test it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#529 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJBHIVJVQNNRCSWMC7GYRWDS5GNDNANCNFSM4W5ELBAA> .

[cjbarth]

While I approve of this, I'd like a second set of eyes before this lands. Also, we definitely need a squash merge on this one.

[markstos]

I reviewed the diff and LGTM.