Bug 35904 - C4::Auth::checkauth cannot be tested easily
Summary: C4::Auth::checkauth cannot be tested easily
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 35890
  Show dependency treegraph
 
Reported: 2024-01-25 09:32 UTC by Jonathan Druart
Modified: 2024-03-11 08:49 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.03,23.05.09


Attachments
Bug 35904: Remove var loggedin (1.35 KB, patch)
2024-01-25 09:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35904: Make C4::Auth::checkauth testable easily (16.02 KB, patch)
2024-01-25 09:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35904: Remove var loggedin (1.35 KB, patch)
2024-02-14 09:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35904: Make C4::Auth::checkauth testable easily (16.52 KB, patch)
2024-02-14 09:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35904: Make C4::Auth::checkauth testable easily (16.57 KB, patch)
2024-02-14 16:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35904: Make C4::Auth::checkauth testable easily (16.63 KB, patch)
2024-02-15 03:56 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35904: (QA follow-up): tidy up code (1.22 KB, patch)
2024-02-15 03:56 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35904: Make C4::Auth::checkauth testable easily (16.69 KB, patch)
2024-02-15 09:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35904: (QA follow-up): tidy up code (1.29 KB, patch)
2024-02-15 09:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35904: (QA follow-up): rename do_not_print + add comment (5.29 KB, patch)
2024-02-15 17:51 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2024-01-25 09:32:51 UTC
We are printing headers and the tests are hard to write.
Comment 1 Jonathan Druart 2024-01-25 09:41:10 UTC
Created attachment 161377 [details] [review]
Bug 35904: Remove var loggedin

It is never used and add confusion
Comment 2 Jonathan Druart 2024-01-25 09:41:13 UTC
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!
Comment 3 Jonathan Druart 2024-02-14 09:14:38 UTC
Created attachment 162134 [details] [review]
Bug 35904: Remove var loggedin

It is never used and add confusion
Comment 4 Jonathan Druart 2024-02-14 09:14:40 UTC
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!
Comment 5 Marcel de Rooy 2024-02-14 12:24:15 UTC
(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.
Comment 6 Jonathan Druart 2024-02-14 12:53:52 UTC
(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.
Comment 7 Kyle M Hall 2024-02-14 16:08:23 UTC
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>
Comment 8 Victor Grousset/tuxayo 2024-02-15 03:56:17 UTC
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>
Comment 9 Victor Grousset/tuxayo 2024-02-15 03:56:19 UTC
Created attachment 162169 [details] [review]
Bug 35904: (QA follow-up): tidy up code
Comment 10 Victor Grousset/tuxayo 2024-02-15 04:17:55 UTC
(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
Comment 11 Martin Renvoize 2024-02-15 07:09:27 UTC
How about 'no_print_for_tests'?
Comment 12 Martin Renvoize 2024-02-15 09:27:26 UTC
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>
Comment 13 Martin Renvoize 2024-02-15 09:27:29 UTC
Created attachment 162188 [details] [review]
Bug 35904: (QA follow-up): tidy up code

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2024-02-15 09:29:18 UTC
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.
Comment 15 Martin Renvoize 2024-02-15 09:48:39 UTC
Pushed to master for 24.05.00
Comment 16 Victor Grousset/tuxayo 2024-02-15 17:51:58 UTC
Created attachment 162217 [details] [review]
Bug 35904: (QA follow-up): rename do_not_print + add comment

rename to no_print_for_tests
Comment 17 Victor Grousset/tuxayo 2024-02-15 17:55:44 UTC
(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
Comment 18 Martin Renvoize 2024-02-15 21:04:08 UTC
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.
Comment 19 Jonathan Druart 2024-02-16 07:43:25 UTC
(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.
Comment 20 Victor Grousset/tuxayo 2024-02-16 19:10:41 UTC
(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.
Comment 21 wainuiwitikapark 2024-02-21 22:42:26 UTC
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
Comment 22 Jonathan Druart 2024-02-26 10:55:40 UTC
(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
Comment 23 Fridolin Somers 2024-03-11 08:48:03 UTC
Pushed to 23.11.03
Comment 24 Fridolin Somers 2024-03-11 08:48:52 UTC
I see it in 23.05.09
Comment 25 Fridolin Somers 2024-03-11 08:49:21 UTC
Arf I dont see it in 22.11.x which contains Bug 35890