We are printing headers and the tests are hard to write.
Created attachment 161377 [details] [review] Bug 35904: Remove var loggedin It is never used and add confusion
Created attachment 161378 [details] [review] Bug 35904: Make C4::Auth::checkauth testable easily This patch suggests to add a new flag do_not_print to C4::Auth::checkauth to not print the headers and allow to test this subroutine more easily. We do no longer need to mock safe_exit and redirect STDOUT to test its return values. There are still 3 left: 1. 733 # checkauth will redirect and safe_exit if not authenticated and not authorized => Better to keep this one, not trivial to replace 2. 806 # This will fail on permissions This should be replaced but testing $template->{VARS}->{nopermission} fails, I dont' think the comment is better. 3. 828 # Patron does not have the borrowers permission Same as 2. 2. and 3. should be investigated a bit more. This patch also move duplicated code to set patron's password to a subroutine set_weak_password. Test plan: Read the code and confirm that everything makes sense. QA: Do you have a better way for this? Yes it's dirty!
Created attachment 162134 [details] [review] Bug 35904: Remove var loggedin It is never used and add confusion
Created attachment 162135 [details] [review] Bug 35904: Make C4::Auth::checkauth testable easily This patch suggests to add a new flag do_not_print to C4::Auth::checkauth to not print the headers and allow to test this subroutine more easily. We do no longer need to mock safe_exit and redirect STDOUT to test its return values. There are still 3 left: 1. 733 # checkauth will redirect and safe_exit if not authenticated and not authorized => Better to keep this one, not trivial to replace 2. 806 # This will fail on permissions This should be replaced but testing $template->{VARS}->{nopermission} fails, I dont' think the comment is better. 3. 828 # Patron does not have the borrowers permission Same as 2. 2. and 3. should be investigated a bit more. This patch also move duplicated code to set patron's password to a subroutine set_weak_password. Test plan: Read the code and confirm that everything makes sense. QA: Do you have a better way for this? Yes it's dirty!
(In reply to Jonathan Druart from comment #4) > Test plan: > Read the code and confirm that everything makes sense. > QA: Do you have a better way for this? Yes it's dirty! There may be a better way, but *niet uit de losse mouw* (not off the cuff?). So I agree that it is dirty and only convenient for the unit tests. Since Auth is already a problem, why make it worse with convenience parameters for testing only? Could we address the lengthy tests in the .t itself? At first glance I am not convinced if we should proceed like this.
(In reply to Marcel de Rooy from comment #5) > (In reply to Jonathan Druart from comment #4) > > Test plan: > > Read the code and confirm that everything makes sense. > > QA: Do you have a better way for this? Yes it's dirty! > > There may be a better way, but *niet uit de losse mouw* (not off the cuff?). > So I agree that it is dirty and only convenient for the unit tests. Since > Auth is already a problem, why make it worse with convenience parameters for > testing only? Could we address the lengthy tests in the .t itself? > At first glance I am not convinced if we should proceed like this. The other way is to refactor C4::Auth. As you know I am not a fan of dirty code, and this is the only way I found to introduce as less change as possible, and (I think!) safely.
Created attachment 162151 [details] [review] Bug 35904: Make C4::Auth::checkauth testable easily This patch suggests to add a new flag do_not_print to C4::Auth::checkauth to not print the headers and allow to test this subroutine more easily. We do no longer need to mock safe_exit and redirect STDOUT to test its return values. There are still 3 left: 1. 733 # checkauth will redirect and safe_exit if not authenticated and not authorized => Better to keep this one, not trivial to replace 2. 806 # This will fail on permissions This should be replaced but testing $template->{VARS}->{nopermission} fails, I dont' think the comment is better. 3. 828 # Patron does not have the borrowers permission Same as 2. 2. and 3. should be investigated a bit more. This patch also move duplicated code to set patron's password to a subroutine set_weak_password. Test plan: Read the code and confirm that everything makes sense. QA: Do you have a better way for this? Yes it's dirty! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 162168 [details] [review] Bug 35904: Make C4::Auth::checkauth testable easily This patch suggests to add a new flag do_not_print to C4::Auth::checkauth to not print the headers and allow to test this subroutine more easily. We do no longer need to mock safe_exit and redirect STDOUT to test its return values. There are still 3 left: 1. 733 # checkauth will redirect and safe_exit if not authenticated and not authorized => Better to keep this one, not trivial to replace 2. 806 # This will fail on permissions This should be replaced but testing $template->{VARS}->{nopermission} fails, I dont' think the comment is better. 3. 828 # Patron does not have the borrowers permission Same as 2. 2. and 3. should be investigated a bit more. This patch also move duplicated code to set patron's password to a subroutine set_weak_password. Test plan: Read the code and confirm that everything makes sense. QA: Do you have a better way for this? Yes it's dirty! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 162169 [details] [review] Bug 35904: (QA follow-up): tidy up code
(In reply to Marcel de Rooy from comment #5) > Since Auth is already a problem, why make it worse with convenience parameters for > testing only? Could we address the lengthy tests in the .t itself? The two simple conditions added in Auth.pm are not really making anything worse IMHO. And the cleanup that it allows in the tests is definitely making things better there. For code readability and maintainability, the tests should be 1st class citizen. (at least just for readability and maintainability) I wonder if the new optional param do_not_print couldn't have a better name though? That either reflects better that it's for tests or that it disables the web response and just returns the template variables instead. no_http_response_and_return_template? test_disable_http_response? If that doesn't do it and no better comes, at least at the declaration of `params` there could be that do_not_print is for testing facilitation. Meanwhile here is a second signoff - basic manual testing works - Auth.t & selenium/authentication_2fa.t & selenium/authentication.t - change makes sense - code looks good - QA script happy
How about 'no_print_for_tests'?
Created attachment 162187 [details] [review] Bug 35904: Make C4::Auth::checkauth testable easily This patch suggests to add a new flag do_not_print to C4::Auth::checkauth to not print the headers and allow to test this subroutine more easily. We do no longer need to mock safe_exit and redirect STDOUT to test its return values. There are still 3 left: 1. 733 # checkauth will redirect and safe_exit if not authenticated and not authorized => Better to keep this one, not trivial to replace 2. 806 # This will fail on permissions This should be replaced but testing $template->{VARS}->{nopermission} fails, I dont' think the comment is better. 3. 828 # Patron does not have the borrowers permission Same as 2. 2. and 3. should be investigated a bit more. This patch also move duplicated code to set patron's password to a subroutine set_weak_password. Test plan: Read the code and confirm that everything makes sense. QA: Do you have a better way for this? Yes it's dirty! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 162188 [details] [review] Bug 35904: (QA follow-up): tidy up code Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I've been watching this one closely and not come up with a cleaner approach. I think we need to PQA to unblock the tree above so I'm going to do so. Scripts are happy, tests are happy.
Pushed to master for 24.05.00
Created attachment 162217 [details] [review] Bug 35904: (QA follow-up): rename do_not_print + add comment rename to no_print_for_tests
(In reply to Martin Renvoize from comment #15) > Pushed to master Is it? No sign of it on master at git.koha-community.org and github.com > How about 'no_print_for_tests'? follow-up attached for the rename and a comment at declaration
It's a dependency for a security bug, as such it's in the security repo not the public one until after release of the stables that include it.
(In reply to Victor Grousset/tuxayo from comment #16) > Created attachment 162217 [details] [review] [review] > Bug 35904: (QA follow-up): rename do_not_print + add comment > > rename to no_print_for_tests Please move this to its own bug, on top of the other things. I am not willing to rebase the whole tree only for wording.
(In reply to Martin Renvoize from comment #18) > It's a dependency for a security bug, as such it's in the security repo not > the public one until after release of the stables that include it. Ok, I get it. -- (In reply to Jonathan Druart from comment #19) > Please move this to its own bug, on top of the other things. I am not > willing to rebase the whole tree only for wording. Ok, it only made sense if all patches still needed to be pushed to the branch.
Hi there, I need to backport this to 22.05 as it is a dependency of Bug 35890. However, the patches don't apply cleanly to 22.05. I was wondering if this could please be rebased for 22.05. Thanks
(In reply to wainuiwitikapark from comment #21) > Hi there, > > I need to backport this to 22.05 as it is a dependency of Bug 35890. > > However, the patches don't apply cleanly to 22.05. I was wondering if this > could please be rebased for 22.05. > > Thanks See security/bug_35904_22.05
Pushed to 23.11.03
I see it in 23.05.09
Arf I dont see it in 22.11.x which contains Bug 35890