-
Notifications
You must be signed in to change notification settings - Fork 358
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 issue when using kerberos auth with Rails 7 #9295
Conversation
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
|
Fix issue when using kerberos auth with Rails 7 (cherry picked from commit fd3d3e9)
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.
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.
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.
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 useto_unsafe_h
instead 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.