Fix issue when using kerberos auth with Rails 7 #9295
Merged
Conversation
This file contains 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
kerberos auth requires a wait_for_task under the covers. When we create this task, we store a backup copy of the incoming request params within the session object, so we can restore it later after the task we are waiting for has completed, and that session object is `Marshal.dump`ed into memcached. In Rails 7, Rails changed the ActionController::Parameters object to include some more objects as context, such as the request, and the request internally has an IO object that cannot be dumped. Since we don't actually need the context and instead only need the parameters, we can use `to_unsafe_h` instead of `deep_dup`, to get a copy of those parameters in HashWithIndifferentAccess form. Later when wait_for_task reconstructs the parameters, they will be placed back into the request object, which will get them back into the ActionController::Parameters format.
|
From DMs, we found that this change is what added more context around the request parameters, and we believe the request object is what is a problem when it tries to Marshal it. See: https://www.github.com/rails/rails/pull/41809/files#diff-99dab0dfb4d0cfa044997480d5aa1b100dc60347a10cedd6d8f7a0395f6a6efdR1194 |
|
I wish I knew how to test this properly. |
|
Yeah I wanted to add specs, but I was having problems figure out how to add them. I'm hoping to do it in a followup. Note that I tested this manually using the FreeIPA demo server, and I could replicate the initial bug, and then it's fixed afterwards. |
|
I'm fine with merging this as is. We can add a test later if we figure out a good way to reliably test it. |
|
Backported to |
Fryguy
pushed a commit
that referenced
this pull request
Oct 22, 2024
Fix issue when using kerberos auth with Rails 7 (cherry picked from commit fd3d3e9)
Fryguy
added a commit
to Fryguy/manageiq-ui-classic
that referenced
this pull request
Oct 23, 2024
Follow up to ManageIQ#9295. If we revert that PR's changes, and run these tests they fail when it tries to Marshal.dump the params internal IO object. Note that the actual failure in test is slightly different from prod. In test we get `TypeError: no _dump_data is defined for class StringIO` because the TestRequest has a StringIO internally. In prod we get `can't dump IO` because the real request has a real IO object internally. Even so, this test still demonstrates the Marshal issue, while also providing an end-to-end test for external auth login.
Merged
Fryguy
added a commit
to Fryguy/manageiq-ui-classic
that referenced
this pull request
Oct 23, 2024
Follow up to ManageIQ#9295. If we revert that PR's changes, and run these tests they fail when it tries to Marshal.dump the params internal IO object. Note that the actual failure in test is slightly different from prod. In test we get `TypeError: no _dump_data is defined for class StringIO` because the TestRequest has a StringIO internally. In prod we get `can't dump IO` because the real request has a real IO object internally. Even so, this test still demonstrates the Marshal issue, while also providing an end-to-end test for external auth login.
Fryguy
added a commit
to Fryguy/manageiq-ui-classic
that referenced
this pull request
Oct 24, 2024
Follow up to ManageIQ#9295. If we revert that PR's changes, and run these tests they fail when it tries to Marshal.dump the params internal IO object. Note that the actual failure in test is slightly different from prod. In test we get `TypeError: no _dump_data is defined for class StringIO` because the TestRequest has a StringIO internally. In prod we get `can't dump IO` because the real request has a real IO object internally. Even so, this test still demonstrates the Marshal issue, while also providing an end-to-end test for external auth login.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
kerberos auth requires a wait_for_task under the covers. When we create this task, we store a backup copy of the incoming request params within the session object, so we can restore it later after the task we are waiting for has completed, and that session object is
Marshal.dumped into memcached. In Rails 7, Rails changed the ActionController::Parameters object to include some more objects as context, such as the request, and the request internally has an IO object that cannot be dumped. Since we don't actually need the context and instead only need the parameters, we can useto_unsafe_hinstead ofdeep_dup, to get a copy of those parameters in HashWithIndifferentAccess form. Later when wait_for_task reconstructs the parameters, they will be placed back into the request object, which will get them back into the ActionController::Parameters format.@jrafanie Please review.