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

Handling multiple configurations for SAML selected at runtime #56

Closed
pedantic-cliff opened this issue Sep 3, 2014 · 5 comments
Closed

Comments

@pedantic-cliff
Copy link

I'm working with Okta as a provider for SAML authentication and this repo works wonderfully for one "company" because it only needs one set of configuration parameters. My site however can consume several different companies' logins which we divide by "groups" of logins, meaning it requires different entryPoint, issuer and cert options per group. Ideally I'd like to provide this as part of the call to authenticate because then it can work in real time.

I made a temporary fix for my app, creating a req.saml = { issuer: ..., cert: ..., entryPoint:... } generated by another middleware which seems like the ideal way to pass it in. However I then override the options hash stored in the _saml.options which seems non-ideal because the chance to have a default is lost and opens up to a lot of bugs caused by unintentionally overriding defaults.

I was in a bit of hurry to get it so I haven't gone through the code extensively enough to make a proper pull request with a real solution. Does the above situation seem like something desirable? I can help implement it if needed (not sure when I'll start by myself though).

@ploer
Copy link
Contributor

ploer commented Sep 3, 2014

Take a look at issue #52, and the https://www.npmjs.org/package/passports library suggested there. I think that would continue to be our preferred way to handle this situation, rather than moving the extra configuration complexity into passport-saml.

I'll add a section to the docs to point in the direction of that approach. (and let me know if you can see a reason that approach wouldn't work for you).

@pedantic-cliff
Copy link
Author

That actually fits perfectly and pretty easy to plug in, thank you!

@markstos
Copy link
Contributor

This may be worth re-visiting as the author of passports no longer users or maintains it.

It seems to me to handle multiple configurations in passport-saml, the primary problem is that we hardcoded the name value and this value is set read-only. Beyond that, is there a problem with loading multiple strategies with different names using passport-saml ?

@markstos markstos reopened this Feb 14, 2018
@markstos
Copy link
Contributor

Here's the patch I'm testing out now to natively enable multiple SAML support:

RideAmigosCorp@594eed3

Maybe @ploer @richjharris or @pdspicer are interested in authenticating against multiple SAML targets are interested to review the approach. ( I still need to real-world test the approach today. )

@markstos
Copy link
Contributor

Official PR is here: #262 With no dissenting comments here, I'm going to merge it.

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

No branches or pull requests

3 participants