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

Support for Assertion Consumer Service HTTP-Redirect binding. #71

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

Conversation

serpulga
Copy link
Contributor

Test results:

----------------------------------------------------------------------
Ran 163 tests in 1.464s

OK

@pitbulk
Copy link
Contributor

pitbulk commented May 12, 2015

Travis failed due Pep8 and pyflakes:

The command "pep8 tests/src/OneLogin/saml2_tests/*.py demo-flask/*.py demo-django/*.py src/onelogin/saml2/*.py --config=tests/pep8.rc" exited with 1.
0.95s$ pyflakes src/onelogin/saml2 demo-django demo-flask tests/src/OneLogin/saml2_tests
tests/src/OneLogin/saml2_tests/response_test.py:12: 'OneLogin_Saml2_Response_Redirect' imported but unused

The command "pyflakes src/onelogin/saml2 demo-django demo-flask tests/src/OneLogin/saml2_tests" exited with 1.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.28%) to 97.62% when pulling 60a2e6d on serpulga:acs-redirect into b753ea7 on onelogin:master.

@pitbulk
Copy link
Contributor

pitbulk commented May 13, 2015

Now you need to refactor the valdation method of the Response class,

  • The HTTP-POST binding search the signature inside the XML (SAMLResponse and/or Assertion could be signed).
  • The HTTP-Redirect binding search the signature on the GET parameter (See an example of an HTTP-Redirect Signature validation here.

Remember that in SAML, the SP must expect and validate the Signature of the SAML Responses, in order to avoid impersonation attacks

@serpulga
Copy link
Contributor Author

Thanks @pitbulk

I'm a bit confused.
Although the code you showed me, that takes the Signature and SigAlg from

@serpulga
Copy link
Contributor Author

I added the validations, I ended up copy-pasting from the other class you pointed out.

If you want to refactor these validations I can help out, but we should do it code-wide if at all. There is definitely repetitions in all is_valid methods.

Looks like it's doing the trick, but the coverage is going to suffer.

@serpulga
Copy link
Contributor Author

@pitbulk
Any reason you like double underscoring for some attribute names? __query, __query_assertion, and __decrypt_assertion, it is making the inheritance more painful than it needs to be.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.04%) to 94.86% when pulling 0a33cd6 on serpulga:acs-redirect into b753ea7 on onelogin:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.04%) to 94.86% when pulling 1eb75ba on serpulga:acs-redirect into b753ea7 on onelogin:master.

@pitbulk
Copy link
Contributor

pitbulk commented May 14, 2015

@diwu1989
Copy link

I would benefit greatly from this, is anyone gonna work on this to get it across the line?

@serpulga
Copy link
Contributor Author

This branch actually has this functionality, but it's way too behind master.

@pitbulk
Copy link
Contributor

pitbulk commented Apr 8, 2016

@serpulga, @diwu1989 read my #78 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants