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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return response' "InResponseTo" field from validation #33

Merged
merged 8 commits into from
Feb 6, 2023

Conversation

Philonous
Copy link
Contributor

This PR adds support for InResponseTo in Response elements

  • Adds field inResponseTo to Result type
  • Changes the type of the validateResponse function to return it together with the validated assertion
  • Should (or rather, MUST) be matched to request IDs, also depending on if unsolicited assertions are allowed.

According to saml-core [1]:

InResponseTo [Optional]
A reference to the identifier of the request to which the response corresponds, if any. If the response
is not generated in response to a request, or if the ID attribute value of a request cannot be
determined (for example, the request is malformed), then this attribute MUST NOT be present.
Otherwise, it MUST be present and its value MUST match the value of the corresponding request's
ID attribute.

Also compare this stack exchange post [2] which argues that this value should be validated

I don't think this validation has to happen within this library, but it should be returned so that callers of the library can implement it themselves, similar to how checks for duplicate assertionId are left as an exercise to the reader 馃槄

Checklist

  • All definitions are documented with Haddock-style comments.
  • All exported definitions have @since annotations.
  • The changelog has been updated.

@Philonous
Copy link
Contributor Author

Fixed the failing tests

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.

Thank you for adding support for this! I agree that it is probably sensible to support this and validate the tag whenever possible.

I have commented on a few bits with some suggestions, which I hope make sense. Let me know what you think.

src/Network/Wai/SAML2/Validation.hs Outdated Show resolved Hide resolved
src/Network/Wai/SAML2.hs Outdated Show resolved Hide resolved
src/Network/Wai/SAML2.hs Show resolved Hide resolved
src/Network/Wai/SAML2.hs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@Philonous
Copy link
Contributor Author

I think I've addressed all the comments in new requests and I've re-based the PR onto master.

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.

Looks good, thank you! Just a few minor points to change before this can be merged.

wai-saml2.cabal Outdated Show resolved Hide resolved
src/Network/Wai/SAML2/Validation.hs Outdated Show resolved Hide resolved
src/Network/Wai/SAML2.hs Outdated Show resolved Hide resolved
src/Network/Wai/SAML2.hs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@mbg mbg merged commit 51206fd into mbg:master Feb 6, 2023
@mbg
Copy link
Owner

mbg commented Feb 6, 2023

@Philonous I made the few last changes, rebased this, and merged it. Thank you for again your work on this! 馃槃

@mbg mbg mentioned this pull request Feb 6, 2023
4 tasks
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