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.

Sign up for GitHub

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

Jump to bottom

Add validation test, including a failing example #52

Merged
merged 2 commits into from
Jul 31, 2024
Merged

Add validation test, including a failing example #52

merged 2 commits into from
Jul 31, 2024

Conversation

fumieval
Copy link
Contributor

@fumieval fumieval commented Jun 22, 2023 •
edited
Loading

Summary

This PR adds a new test, validation, that tries to validate concrete examples of SAMLResponses. It contains a couple of samples that wai-saml2 can't handle at the moment.

Most code is extracted from #45

Checklist

  • All definitions are documented with Haddock-style comments.
  • All exported definitions have @since annotations.
  • Code is formatted in line with the existing code.
  • The changelog has been updated.

@github-actions github-actions bot requested a review from mbg June 22, 2023 08:18
@fumieval fumieval force-pushed the validation-test branch 2 times, most recently from fd5db35 to 71fcd42 Compare June 22, 2023 09:48
@fumieval fumieval mentioned this pull request Jun 22, 2023
Open
4 tasks
mbg
mbg reviewed Jun 24, 2023
View reviewed changes
Copy link
Owner

@mbg mbg left a comment

Choose a reason for hiding this comment

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

Thanks for moving this out! That does make it easier to review, although ironically I did already have pending review comments for this part of #45 🙈 so I have copied my relevant comments from there over to the review here. Sorry that other review has been taking much longer than I anticipated.

package.yaml Outdated
@@ -50,7 +50,7 @@ library:

tests:
parser:
main: Parser.hs
main: parser.hs
Copy link
Owner

Choose a reason for hiding this comment

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

Why the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because GHC complains that the module name should be Parser, while I want this to be Main. That's probably because the tests directory now contains more than one file.

package.yaml Outdated Show resolved Hide resolved
tests/validation.hs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -1,5 +1,7 @@
# Changelog for `wai-saml2`

- Added `validation` test ([#52](https://github.com/mbg/wai-saml2/pull/52) by [@fumieval](https://github.com/fumieval))
Copy link
Owner

Choose a reason for hiding this comment

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

This change probably doesn't need to be in the changelog, but I don't feel strongly either way. So happy for you to leave it in if you want.

@fumieval fumieval force-pushed the validation-test branch 2 times, most recently from 5903c00 to 69f937a Compare June 26, 2023 02:13
@fumieval fumieval changed the title Add validation test, including a couple of failing examples Add validation test, including a failing example Jun 26, 2023
@fumieval
Copy link
Contributor Author

I wonder why the tests fail on stack-lts-19 or older...

mbg
mbg reviewed Jul 1, 2023
View reviewed changes
Copy link
Owner

@mbg mbg left a comment

Choose a reason for hiding this comment

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

It seems like you omitted an expectFail in your most recent commit, which I think leads to the test failures now. The real question is why the tests for LTS 20 don't fail 😕

package.yaml Outdated Show resolved Hide resolved
tests/Validation.hs Show resolved Hide resolved
@fumieval fumieval force-pushed the validation-test branch 2 times, most recently from c8ea0ac to ded1e75 Compare July 6, 2023 09:09
fumieval added a commit to fumieval/c14n that referenced this pull request Jul 7, 2023
@fumieval
Fix non-determistic incorrect output with respect to nsPrefixes
8c9dc38 
This change fixes the test failures on mbg/wai-saml2#52 . It turns out that the use of `unsafeAsCString` causes `nsPrefixes` to be effectively empty most of the time (yes, it is __non-deterministic__!)

I confirmed that the tests above with this patch pass on lts-19.33 or older.

I suggest to release the fix in an urgent manner.
@fumieval fumieval mentioned this pull request Jul 7, 2023
Merged
@fumieval
Copy link
Contributor Author

fumieval commented Jul 7, 2023

I figured out the cause of the test failures, and fixed it in mbg/c14n#3

mbg
mbg reviewed Aug 6, 2023
View reviewed changes
Copy link
Owner

@mbg mbg left a comment

Choose a reason for hiding this comment

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

Thanks again for figuring out and fixing the issue over in the c14n library that fixed the tests here. This mostly looks good now, just two minor niggles that would be good to sort out before merging this.

stack-lts-16.1.yaml.lock Outdated Show resolved Hide resolved
stack-lts-16.1.yaml Outdated Show resolved Hide resolved
@fumieval
Copy link
Contributor Author

@mbg Friendly reminder that this is ready for review

mbg
mbg approved these changes Jul 31, 2024
View reviewed changes
Copy link
Owner

@mbg mbg left a comment

Choose a reason for hiding this comment

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

Sorry about that! I must have missed that this was ready for another review; you can always hit the icon next to my username in the list of reviewers as well to re-request a review if you need to.

I think this looks good to merge as is now. Thanks for your work on finding and fixing the bug in c14n, and teasing these tests out into their own PR.

@mbg mbg merged commit 12003d4 into mbg:master Jul 31, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbg mbg mbg approved these changes

Assignees

@fumieval fumieval

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fumieval @mbg