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

Fix assertion fallback #870

Merged
merged 3 commits into from
Mar 24, 2017
Merged

Conversation

Calpicow
Copy link
Contributor

This fixes the case where all xmlns are contained within the top level element.

@ericchiang
Copy link
Contributor

@Calpicow do you have a test case which demonstrates this? I'd like to avoid regressions.

@Calpicow
Copy link
Contributor Author

Yes, I'll add some tests.

@Calpicow
Copy link
Contributor Author

Tests added

@Calpicow
Copy link
Contributor Author

Calpicow commented Mar 23, 2017

Edit: Appears to be regression in goxmldsig

=== RUN   TestVerifySignedMessageAndUnsignedAssertion
--- FAIL: TestVerifySignedMessageAndUnsignedAssertion (0.00s)
	saml_test.go:56: crypto/rsa: verification error
=== RUN   TestVerifyUnsignedMessageAndSignedAssertion
--- FAIL: TestVerifyUnsignedMessageAndSignedAssertion (0.00s)
	saml_test.go:56: crypto/rsa: verification error
=== RUN   TestVerifySignedMessageAndSignedAssertion
--- FAIL: TestVerifySignedMessageAndSignedAssertion (0.00s)
	saml_test.go:56: crypto/rsa: verification error

@ericchiang
Copy link
Contributor

@Calpicow looking into this now

@Calpicow
Copy link
Contributor Author

Tests are passing

@@ -500,8 +501,8 @@ func verify(validator *dsig.ValidationContext, data []byte) (signed []byte, err
verified = true
doc.SetRoot(transformedResponse)
}
assertion := response.SelectElement("Assertion")
if assertion == nil {
assertion, err := etreeutils.NSSelectOne(response, "urn:oasis:names:tc:SAML:2.0:assertion", "Assertion")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you comment a little more on what this change is doing and why it's required?

@Calpicow
Copy link
Contributor Author

Updated. How's that?

@ericchiang
Copy link
Contributor

perfect, thanks.

Copy link
Contributor

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

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

lgtm, merging on green.

@ericchiang ericchiang merged commit 2a6ae0a into dexidp:master Mar 24, 2017
@Calpicow Calpicow deleted the fix_assertion_fallback branch October 5, 2017 17:43
mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019
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

2 participants