[Bug 35904] New: C4::Auth::checkauth cannot be tested easily
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Bug ID: 35904 Summary: C4::Auth::checkauth cannot be tested easily Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: ASSIGNED Severity: normal Priority: P5 - low Component: Test Suite Assignee: jonathan.druart+koha@gmail.com Reporter: jonathan.druart+koha@gmail.com QA Contact: testopia@bugs.koha-community.org We are printing headers and the tests are hard to write. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |35890 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #1 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 161377 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161377&action=edit Bug 35904: Remove var loggedin It is never used and add confusion -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #2 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 161378 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161378&action=edit 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au, | |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #161377|0 |1 is obsolete| | Attachment #161378|0 |1 is obsolete| | --- Comment #3 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 162134 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162134&action=edit Bug 35904: Remove var loggedin It is never used and add confusion -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #4 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 162135 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162135&action=edit 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162134|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #6 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162135|0 |1 is obsolete| | --- Comment #7 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 162151 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162151&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162151|0 |1 is obsolete| | --- Comment #8 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 162168 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162168&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #9 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 162169 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162169&action=edit Bug 35904: (QA follow-up): tidy up code -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net --- Comment #10 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- How about 'no_print_for_tests'? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162168|0 |1 is obsolete| | --- Comment #12 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 162187 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162187&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162169|0 |1 is obsolete| | --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 162188 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162188&action=edit Bug 35904: (QA follow-up): tidy up code Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA QA Contact|testopia@bugs.koha-communit |martin.renvoize@ptfs-europe |y.org |.com --- Comment #14 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |24.05.00 released in| | Status|Passed QA |Pushed to master --- Comment #15 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Pushed to master for 24.05.00 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #16 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 162217 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162217&action=edit Bug 35904: (QA follow-up): rename do_not_print + add comment rename to no_print_for_tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #17 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #18 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #19 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Victor Grousset/tuxayo from comment #16)
Created attachment 162217 [details] [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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162217|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #20 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 wainuiwitikapark@catalyst.net.nz changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wainuiwitikapark@catalyst.n | |et.nz --- Comment #21 from wainuiwitikapark@catalyst.net.nz --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #22 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|24.05.00 |24.05.00,23.11.03 released in| | CC| |fridolin.somers@biblibre.co | |m Status|Pushed to master |Pushed to stable --- Comment #23 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.11.03 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|24.05.00,23.11.03 |24.05.00,23.11.03,23.05.09 released in| | Status|Pushed to stable |Pushed to oldstable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #24 from Fridolin Somers <fridolin.somers@biblibre.com> --- I see it in 23.05.09 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #25 from Fridolin Somers <fridolin.somers@biblibre.com> --- Arf I dont see it in 22.11.x which contains Bug 35890 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |frederic@tamil.fr -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC|martin.renvoize@ptfs-europe | |.com | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #26 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- (In reply to Fridolin Somers from comment #25)
Arf I dont see it in 22.11.x which contains Bug 35890
Patch doesn't apply cleanly for 22.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #27 from Fridolin Somers <fridolin.somers@biblibre.com> --- (In reply to Wainui Witika-Park from comment #26)
(In reply to Fridolin Somers from comment #25)
Arf I dont see it in 22.11.x which contains Bug 35890
Patch doesn't apply cleanly for 22.11
Ah but it is a dependency of Bug 35890. I see this patches are in 22.05.x : efcc2b3c32 Bug 35904: (QA follow-up): tidy up code 7d0e211255 Bug 35904: Make C4::Auth::checkauth testable easily @Joubu could you try to create patches for 22.11 ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #28 from Jonathan Druart <jonathan.druart@gmail.com> --- This is an old one and bug 35890 has been pushed to 22.11.x 18 months ago. Why do we need it now? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 --- Comment #29 from Fridolin Somers <fridolin.somers@biblibre.com> --- (In reply to Jonathan Druart from comment #28)
This is an old one and bug 35890 has been pushed to 22.11.x 18 months ago. Why do we need it now?
I asked because it is LTS, but not for long now. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to oldstable |Needs documenting --- Comment #30 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- Not backporting to 22.11.x unless requested -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35904 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Needs documenting |RESOLVED CC| |david@davidnind.com --- Comment #31 from David Nind <david@davidnind.com> --- Test suite, no changes to the manual required. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org