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

Check that the response has all of the AuthnContexts that we provided… #97

Merged
merged 1 commit into from Sep 27, 2018

Conversation

jeffFranklin
Copy link
Contributor

… in the request.

@coveralls
Copy link

coveralls commented May 26, 2018

Coverage Status

Coverage increased (+0.03%) to 96.025% when pulling 3da2298 on jeffFranklin:master into 62148cb on onelogin:master.

@pitbulk
Copy link
Contributor

pitbulk commented May 28, 2018

  • We should add a flag to enable/disable this check in order to prevent compatibility issues. ('failOnAuthnContextMistmatch'). By default disabled.
  • We should add a method on Response and Auth to return the AuthNContext of the last response processed in order to let the final app decide what to do too instead raise an exception.
  • We should on the error message, provide info about the expected possible values and the value obtained.

@jeffFranklin
Copy link
Contributor Author

Ok, I've added failOnAuthnContextMismatch, off by default. I've also added a way for the client to inspect the last authentication contexts, as well as providing a more descriptive error message on failure. I've squashed my commits down to a single commit in the pull request, but you can view the changes as separate commits at https://github.com/jeffFranklin/python3-saml/tree/authn_contexts.

@jeffFranklin
Copy link
Contributor Author

I owe a fuller explanation as to why I'm requesting the change. Our organization has a need to be able to verify that the authentication context is the one we expect. We have two contexts that we care about, one being the basic password authentication, and the other being password authentication with two-factor authentication (TimeSyncToken). There currently is no way in this library to distinguish between the two.

What my change does is two-fold - it exposes the context classes to the saml client, and, with failOnAuthnContextMismatch set, it rejects the saml response if it doesn't match what we're configured to expect.

I was unsure the way to get the issue on python3-saml's radar, and I saw the changes needed, hence the pull request. Is there something else I need to be doing? I understand that there are a multitude of libraries for the different languages that need to be kept in alignment, but is there anything that I could be doing to keep the ball rolling?

@pitbulk
Copy link
Contributor

pitbulk commented Jun 19, 2018

Sorry for the delay. I hope to provide more feedback/merge the PR this week.

@jeffFranklin
Copy link
Contributor Author

Ok. Thanks for the update! - Jeff

@jeffFranklin
Copy link
Contributor Author

Use of this toolkit is gaining interest at my organization. Is there anything I can do to assist in the evaluation of this pull request? I've rebased with your latest at https://github.com/jeffFranklin/python3-saml/tree/auth-context-2fa. I can do that on this pull request if that would help.

Thanks! -Jeff

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

3 participants