Skip to content

Bug: SP does not support both IDP Initiated SSO and SP Initiated SSO #256

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

Closed
mmailhos opened this issue Jan 31, 2020 · 2 comments
Closed

Comments

@mmailhos
Copy link
Contributor

mmailhos commented Jan 31, 2020

When having both IDP Initiated SSO and SP Initiated SSO, the if condition does not match anymore:

	if sp.AllowIDPInitiated && len(possibleRequestIDs) == 0 {
		possibleRequestIDs = append([]string{""})
	}

https://github.com/crewjam/saml/blob/master/service_provider.go#L517

After a SP Initiated SSO, len(possibleRequestIDs) is not empty anymore.
If a few minutes later, when the cache of Request IDs has not expired, a user tries to log in with IDP Initiated SSO, that condition is not going to be true anymore and results in an Authentication failed error.

So I am wondering:

  • Why (and do we really need to) check len(possibleRequestIDs) in a first place?
  • Even if I invalidate the cache on the requests IDs as soon as the authentication succeed, there are still chances that this error happens in a concurrent scenario. If a user fails to authenticate to the IDP, the IDP initiated login won't work until the request ID of the user has expired in the cache.

edit: I'll need to check from my IDP if ResponseTo is actually empty when it's IDP Initiated - got me a doubt.

@crewjam
Copy link
Owner

crewjam commented Jan 31, 2020

Good point, I think the second half of that check can be removed, for exactly the reason you specified.

@crewjam
Copy link
Owner

crewjam commented Feb 25, 2020

Closing this because it looks like it was fixed in #259. Let me know if I got that wrong. :)

@crewjam crewjam closed this as completed Feb 25, 2020
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

2 participants