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

Validation and relay state customization #257

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dddoronnn
Copy link

This library is awesome in its simplicity and straightforwardness, nevertheless, I've encountered 2 things that were missing IMO.

Luckily, they were pretty easy to solve:

  1. The relayState parameter had to be set directly on the ServiceProvider entity, which required creating a new provider each time a login request had to be created with a different relay state. (That was already discussed in Why is the relayState attribute a part of EntitySettings? #163)
  2. All validations were mandatory. My specific concern was the issuer validation, since my app is multi-tenant, and I would like to be able to get SAML responses from multiple issuers into the same endpoint, then dispatching them to the right client based on the issuer.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 86.93% when pulling 73be9f2 on dddoronnn:master into 4563872 on tngan:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 86.93% when pulling 73be9f2 on dddoronnn:master into 4563872 on tngan:master.

@tngan
Copy link
Owner

tngan commented Apr 1, 2019

@dddoronnn

Thanks for your contribution.

  1. That's good, but see if you could add tests for the changes as well.
  2. Is it possible for you to create a different pair of sp and idp ? skipping the basic validation might not be a good option in terms of security.

@dddoronnn
Copy link
Author

@tngan

  1. Sure, will do.

  2. It's not about creating multiple IdPs, I'm already doing so when generating the SAML request - Using the same endpoint for accepting multiple responses is the tricky part.
    I want to be able to publish a single app onto Okta's marketplace and then I can only provide a single ACS url that accepts responses from my different clients. After I'm validating the response, I use the IssuerID to tell which client it came from - I just do it inside my app's own code.

That is a common practice, here from Okta's documentation: https://www.okta.com/integrate/documentation/saml/#single-idp-vs-multiple-idps

For a single-instance multi-tenant application where the tenancy is not defined in the URL (such as via a subdomain) ... you must then rely on additional information in the SAML response to determine which IDP is trying to authenticate (for example, using the IssuerID)

Having that said, I agree that for most use cases the validation is important and the default should be going through all possible checks.

@tngan tngan self-requested a review April 3, 2019 08:54
@tngan tngan force-pushed the master branch 3 times, most recently from 554424c to 0e61500 Compare July 7, 2019 08:45
@tjunnone
Copy link

For 2) if you could determine the issuer ID from the SAML response without parsing it fully, you could then construct the corresponding idp at runtime, and not have to skip the basic validation. A suggestion for how that could work is #357 (comment)

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

Successfully merging this pull request may close these issues.

None yet

4 participants