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

signature checks didn't work #30

Merged
merged 4 commits into from
Oct 18, 2016
Merged

Conversation

michael-basil
Copy link
Contributor

No description provided.

mrbasil added 4 commits October 18, 2016 10:44
signature check logic has not statements in the wrong places
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 95.253% when pulling 59b5748 on macfound:master into 898f95c on onelogin:master.

@pitbulk
Copy link
Contributor

pitbulk commented Oct 18, 2016

Nice catch

@pitbulk pitbulk merged commit 4eec1a3 into SAML-Toolkits:master Oct 18, 2016
@pitbulk
Copy link
Contributor

pitbulk commented Oct 18, 2016

Released 1.2.1

Let me know if you experience any other issue.

@jgehrcke
Copy link
Contributor

jgehrcke commented Nov 1, 2016

@pitbulk what needs to be done so that the issue that was fixed here is covered by the unit tests?

@pitbulk
Copy link
Contributor

pitbulk commented Nov 1, 2016

@jgehrcke

At response_test.py we should create the methods:

  • testSignatureWantAssertionsSigned
  • testSignatureWantMessagesSigned

Both should have as input 2 valid SAMLResponse One with only assertion signed, other with all message signed, and with strict mode enabled.

testSignatureWantAssertionsSigned:

  • With wantAssertionsSigned disabled both responses must be consider valid
  • With wantAssertionsSigned enabled only the response with signed assertion must be consider valid

testSignatureWantMessagesSigned:

  • With wantMessagesSigned disabled both responses must be consider valid
  • With wantMessagesSigned enabled only the response with signed whole messag signed must be consider valid

Do you want to contribute with a PR?

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.

4 participants