This seems to be a recent change in behavior. To reproduce: 1. Find/create a report, run it and copy the URL. Something like "http://localhost:8081/cgi-bin/koha/reports/guided_reports.pl?id=1&op=run" 2. Logout, or use a private browsing tab, and use the URL from above. 3. You'll be promoted to login fist, do that. 4. Once you are logged in you'll be taken to the reports home page instead of your specific report.
I'll just add that I do see the report id in the login form in a hidden field, so it does carry to the login form, but gets lost after login.
I found that when using a local login, after authentication the op param is updated to cud-login. After authentication the id param is not in the url, but it is included in the payload. The report doesn't load because the op param is not longer 'run' Interestingly after an SSO authentication the page loads with the complete original url and the report loads
Confirming that we see that it works via SSO here as well. Seems to be limited to local login.
Owen reported the same problem on suggestion/suggestion.pl I am stuck here, what I am trying to do won't work as we don't sent back the modified CGI object to the controller. Any ideas, David?
Created attachment 191568 [details] [review] Bug 39055: wip Patch from commit 1c381a0
(In reply to Jonathan Druart from comment #4) > Owen reported the same problem on suggestion/suggestion.pl > > I am stuck here, what I am trying to do won't work as we don't sent back the > modified CGI object to the controller. > > Any ideas, David? Yeah, that's an interesting one. A lot of other apps have /login* endpoints which may or may not redirect back to a sanitized referrer. That would be easy to handle. But Koha has always allowed login on any page, which has its pros and cons... The simplest option would probably be change the login not to use the "op" parameter. We could change it to "login_op". We would need to update Koha/Middleware/CSRF.pm to handle this special case because a login POST could contain an "op" without a "cud-" prefix. Alternatively, we could try to hack the inputs to stuff the query string params back in via a Plack::Middleware or through C4/Auth.pm but I don't like that idea as it's dirty/hacky.
Created attachment 191617 [details] [review] Bug 39055: Change "op" to "login_op" when logging in to allow original op to flow through This patch changes "op" to "login_op" when logging in, so that the original op can flow through to whichever controller is processing the login and subsequent op. Test plan: 0. apply the patch and koha-plack --restart kohadev 1. Without logging in, go to the following: http://localhost:8081/cgi-bin/koha/reports/guided_reports.pl?id=1&op=run 2. Log in and note that you can see the report 3. Try logging into the OPAC 4. Try logging into the self-checkout 5. Create a new self-reg account and try logging in with the auto-filled details 6. Test anything else you can think to test here
I've done a patch for my "login_op" idea and it seems to work pretty well. It's the only practical idea I've got at the moment I think.
Yes I agree with the solution, ready for signoff.
Created attachment 192121 [details] [review] Bug 39055: Change "op" to "login_op" when logging in to allow original op to flow through This patch changes "op" to "login_op" when logging in, so that the original op can flow through to whichever controller is processing the login and subsequent op. Test plan: 0. apply the patch and koha-plack --restart kohadev 1. Without logging in, go to the following: http://localhost:8081/cgi-bin/koha/reports/guided_reports.pl?id=1&op=run 2. Log in and note that you can see the report 3. Try logging into the OPAC 4. Try logging into the self-checkout 5. Create a new self-reg account and try logging in with the auto-filled details 6. Test anything else you can think to test here Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Created attachment 192308 [details] [review] Bug 39055: Fix mojo/acquisition/vendors.t Patch from commit 2c5d7b3
t/db_dependent/mojo/csrf.t is failing Shouldn't we add a case in the middleware to force login_op to be POSTed?
(In reply to Jonathan Druart from comment #12) > Shouldn't we add a case in the middleware to force login_op to be POSTed? Yeah, that's probably true. I think there are fail safes in C4/Auth.pm but yeah good point.
Created attachment 192350 [details] [review] Bug 39055: Change "op" to "login_op" when logging in to allow original op to flow through This patch changes "op" to "login_op" when logging in, so that the original op can flow through to whichever controller is processing the login and subsequent op. Test plan: 0. apply the patch and koha-plack --restart kohadev 1. Without logging in, go to the following: http://localhost:8081/cgi-bin/koha/reports/guided_reports.pl?id=1&op=run 2. Log in and note that you can see the report 3. Try logging into the OPAC 4. Try logging into the self-checkout 5. Create a new self-reg account and try logging in with the auto-filled details 6. Test anything else you can think to test here Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 192351 [details] [review] Bug 39055: Fix mojo/acquisition/vendors.t Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 192352 [details] [review] Bug 39055: Add more login_op checks to middleware Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 192353 [details] [review] Bug 39055: Add change to Koha::App::Plugin::CSRF and fix unit test Signed-off-by: David Cook <dcook@prosentient.com.au>
I forgot to test it properly and it broke... and I'm now in the middle of other things. I'll come back to this...
Created attachment 192354 [details] [review] Bug 39055: Fix login condition when using op and login_op