Skip to content

Accept authn response with no NameID element #190

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

Merged
merged 3 commits into from
Sep 21, 2018

Conversation

skoranda
Copy link
Contributor

@skoranda skoranda commented Aug 17, 2018

The SAML backend SP should accept an authentication response from an IdP
that does not contain a SAML NameID element since it is not required by
the SAML 2.0 specification. This change enables the _translate_response
method for the SAMLBackend class to continue gracefully if there is no
NameID, and for the _auth_resp_callback_func method of the SATOSABase
class to not insist on grabbing a user_id generated from a NameID and
hashing it.

All Submissions:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what problem you are trying to solve with this PR?
  • Have you added information on what your changes do and why you chose this as your solution?
  • Have you written new tests for your changes?
  • Does your submission pass tests?
  • This project follows PEP8 style guide. Have you run your code against the 'flake8' linter? The methods that were edited or added pass the flake8 linter. It is beyond the scope of this PR to make all of the edited files pass the flake8 linter.

The SAML backend SP should accept an authentication response from an IdP
that does not contain a SAML NameID element since it is not required by
the SAML 2.0 specification. This change enables the _translate_response
method for the SAMLBackend class to continue gracefully if there is no
NameID, and for the _auth_resp_callback_func method of the SATOSABase
class to not insist on grabbing a user_id generated from a NameID and
hashing it.
@skoranda
Copy link
Contributor Author

Note that this PR for SATOSA requires the PR 451 for pysaml2:

IdentityPython/pysaml2#541

Copy link
Member

@c00kiemon5ter c00kiemon5ter left a comment

Choose a reason for hiding this comment

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

I think this is fine. We have to update the pysaml2 hardcoded version for SATOSA in order to use the latest available. I would also like to add more python versions to test and make the output more verbose. But this is all unrelated to this changeset. I will merge this asap.

@skoranda
Copy link
Contributor Author

Since pysaml2 PR 451 has been merged, do you have a timeline for when you can do a point release of pysaml2 and then merge this request?

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
Prior to pysaml2 v4.6.1 an exception is thrown when parsing a SAML
response with no NameID element.

  satosa.exception.SATOSAAuthenticationError: Failed to parse authn request

pysaml2 v4.6.1 onwards supports SAML responses with no NameID element.

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
@c00kiemon5ter c00kiemon5ter merged commit 912e132 into IdentityPython:master Sep 21, 2018
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.

2 participants