Add support for POST on a wrapped request#534
Open
sfc-gh-thardie wants to merge 6 commits intocrewjam:mainfrom
Open
Add support for POST on a wrapped request#534sfc-gh-thardie wants to merge 6 commits intocrewjam:mainfrom
sfc-gh-thardie wants to merge 6 commits intocrewjam:mainfrom
Conversation
In the case where you've used samlsp to wrap an endpoint that may get a POST request, the previous implemetation just did a HTTP Redirect after the SAML assertion was done.
Added POST support to request_tracker
patricsss
reviewed
Oct 28, 2023
Comment on lines
+238
to
+249
| text := fmt.Sprintf(`<html>`+ | ||
| `<form method="post" action="%s" id="SAMLAfterAssertionRedirectForm">`, trackedRequest.URI) | ||
| for key, values := range trackedRequest.PostData { | ||
| for _, value := range values { | ||
| text = fmt.Sprintf(`%s<input type="hidden" name="%s" value="%s" />`, text, key, value) | ||
| } | ||
| } | ||
| text += `<input id="SAMLAfterAssertionRedirectSubmitButton" type="submit" value="Continue" />` + | ||
| `</form>` + | ||
| `<script>document.getElementById('SAMLAfterAssertionRedirectSubmitButton').style.visibility='hidden';</script>` + | ||
| `<script>document.getElementById('SAMLAfterAssertionRedirectForm').submit();</script>` + | ||
| `</html>` |
There was a problem hiding this comment.
This would be great to live in a template and then be rendered with the placeholders.
https://pkg.go.dev/text/template
That would also let you abstract this away into another file either as a constant-string-literal or as an actual text file that can be embedded and read from something like an embed.FS. I would strongly recommend the string literal though if you opt for this approach.
The template package supports the nested looping you're doing here "out of the box".
This would also make it considerably easier to write tests to ensure the form generated is as expected as the templating could be more easily tested than this method which has farm more state in it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a request is made to an endpoint with the method set to POST, if the user has no session currently, we start the SAML authentication flow. When the assertion comes back, we will then redirect to the URL where the POST request was made, which will do a GET to that URL with no body. This obviously breaks the original POST.
This PR adds storage of the method and post form storage to
TrackedRequest, and once we are done with the SAML Assertion, if the original request was a POST, we use the same method thatIdpAuthnRequestuses inWriteResponseto create a form that uses a tiny bit of javascript to POST back to the original endpoint.Obviously this won't work for large forms, since the
TrackedRequestwill then generate a cookie which is too big, but it's better than making a GET to an endpoint expecting a POST.In terms of the code that generates the HTML with the script, LMK if you think the function I added as well as
WriteResponseshould be factored out into some standalone, common function that both of these methods could use, and I could make an attempt at doing that.