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

Allow for authnRequestBinding in SAML options #529

Merged
merged 8 commits into from
Feb 15, 2021

Conversation

mhesler74
Copy link
Contributor

Description

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

Checklist:

@cjbarth
Copy link
Collaborator

cjbarth commented Feb 1, 2021

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

cjbarth commented Feb 1, 2021

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.

test/multiSamlStrategy.js Outdated Show resolved Hide resolved
@mhesler74
Copy link
Contributor Author

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.

test/multiSamlStrategy.js Outdated Show resolved Hide resolved
@cjbarth
Copy link
Collaborator

cjbarth commented Feb 1, 2021

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
Copy link
Contributor Author

@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
Copy link
Collaborator

cjbarth commented Feb 3, 2021

@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
Copy link
Contributor Author

mhesler74 commented Feb 3, 2021

@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
Copy link
Collaborator

cjbarth commented Feb 3, 2021

@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
Copy link
Contributor Author

mhesler74 commented Feb 3, 2021 via email

@cjbarth cjbarth changed the title allow for authnRequestBinding in SAML options Allow for authnRequestBinding in SAML options Feb 15, 2021
Copy link
Collaborator

@cjbarth cjbarth left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@markstos markstos left a comment

Choose a reason for hiding this comment

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

I reviewed the diff and LGTM.

@cjbarth cjbarth merged commit ed4be0c into node-saml:master Feb 15, 2021
@cjbarth cjbarth added this to the 3.0.0 milestone May 10, 2021
@cjbarth cjbarth mentioned this pull request May 10, 2021
@cjbarth cjbarth added the semver-minor This PR requires a semver-minor version bump label May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor This PR requires a semver-minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Unable to override default authnRequestBinding when using MultiSamlStrategy
3 participants