Bug 28785

Summary: Code in C4::Auth::checkauth is copy pasted
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: dcook, kyle, m.de.rooy, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Sponsored-by: Orex Digital
Version(s) released in:
21.11.00
Bug Depends on:    
Bug Blocks: 28786    
Attachments: Bug 28785: Centralize cookie auth check in check_cookie_auth
Bug 28785: Adjust check_cookie_auth calls
Bug 28785: Add skip_version_check for checkauth
Bug 28785: Don't send SessionRestrictionByIP to template
Bug 28785: Add missing POD for check_cookie_auth
Bug 28785: Centralize cookie auth check in check_cookie_auth
Bug 28785: Adjust check_cookie_auth calls
Bug 28785: Add skip_version_check for checkauth
Bug 28785: Don't send SessionRestrictionByIP to template
Bug 28785: Centralize cookie auth check in check_cookie_auth
Bug 28785: Adjust check_cookie_auth calls
Bug 28785: Add skip_version_check for checkauth
Bug 28785: Don't send SessionRestrictionByIP to template
Bug 28785: Centralize cookie auth check in check_cookie_auth
Bug 28785: Adjust check_cookie_auth calls
Bug 28785: Add skip_version_check for checkauth
Bug 28785: Don't send SessionRestrictionByIP to template

Description Jonathan Druart 2021-07-30 11:54:35 UTC
The code to check cookie authentication is duplicated in several places inside C4::Auth:
* check_cookie_auth
* checkauth
* check_api_auth

The subroutine check_cookie_auth must be called from checkauth and check_api_auth to avoid code duplication.
Comment 1 Jonathan Druart 2021-07-30 12:37:47 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2021-07-30 12:37:52 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2021-07-30 12:37:56 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2021-07-30 12:37:59 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2021-07-30 12:45:54 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2021-07-30 12:50:17 UTC
Created attachment 123320 [details] [review]
Bug 28785: Centralize cookie auth check in check_cookie_auth

This code is duplicated in 3 different places, we must call
check_cookie_auth instead.

It makes check_cookie_auth returns a 'restricted' when
SessionRestrictionByIP is set and the IP changed.
It also returns a third parameters contained the old and new IP, to fill
the "info" hash in checkauth but apparently the oldip and newip
variables are not even used from the template. We may want to remove it
completely.

No change is expected with this patch, the different authentication
methods should still work as before.

Test plan:
Log in the staff and OPAC interfaces, logout.
Log in and call script that call the 3 different subroutines modified by
this patch. For instance you can list checkouts (that is using
check_cookie_auth) and display a patron's image (using check_api_auth).

QA with good knowledge of the C4::Auth module and the different
authentication methods is required.
Comment 7 Jonathan Druart 2021-07-30 12:50:20 UTC
Created attachment 123321 [details] [review]
Bug 28785: Adjust check_cookie_auth calls

The previous patch makes check_cookie_auth return the session instead of
$sessionID, so we are adjusting the different calls to prevent
confusion.
However they are mainly used to check the authentication status and
don't care about this second variable.
Comment 8 Jonathan Druart 2021-07-30 12:50:24 UTC
Created attachment 123322 [details] [review]
Bug 28785: Add skip_version_check for checkauth

We could have this patch but we also could decide to skip it. The idea
is to avoid 2 checks of the version when we are coming from checkauth.
Comment 9 Jonathan Druart 2021-07-30 12:50:28 UTC
Created attachment 123323 [details] [review]
Bug 28785: Don't send SessionRestrictionByIP to template

It's not used.
Comment 10 Owen Leonard 2021-08-02 13:55:47 UTC
Created attachment 123392 [details] [review]
Bug 28785: Centralize cookie auth check in check_cookie_auth

This code is duplicated in 3 different places, we must call
check_cookie_auth instead.

It makes check_cookie_auth returns a 'restricted' when
SessionRestrictionByIP is set and the IP changed.
It also returns a third parameters contained the old and new IP, to fill
the "info" hash in checkauth but apparently the oldip and newip
variables are not even used from the template. We may want to remove it
completely.

No change is expected with this patch, the different authentication
methods should still work as before.

Test plan:
Log in the staff and OPAC interfaces, logout.
Log in and call script that call the 3 different subroutines modified by
this patch. For instance you can list checkouts (that is using
check_cookie_auth) and display a patron's image (using check_api_auth).

QA with good knowledge of the C4::Auth module and the different
authentication methods is required.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 11 Owen Leonard 2021-08-02 13:55:51 UTC
Created attachment 123393 [details] [review]
Bug 28785: Adjust check_cookie_auth calls

The previous patch makes check_cookie_auth return the session instead of
$sessionID, so we are adjusting the different calls to prevent
confusion.
However they are mainly used to check the authentication status and
don't care about this second variable.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 12 Owen Leonard 2021-08-02 13:55:55 UTC
Created attachment 123394 [details] [review]
Bug 28785: Add skip_version_check for checkauth

We could have this patch but we also could decide to skip it. The idea
is to avoid 2 checks of the version when we are coming from checkauth.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 13 Owen Leonard 2021-08-02 13:55:59 UTC
Created attachment 123395 [details] [review]
Bug 28785: Don't send SessionRestrictionByIP to template

It's not used.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 14 David Cook 2021-08-11 00:51:55 UTC
Comment on attachment 123393 [details] [review]
Bug 28785: Adjust check_cookie_auth calls

Review of attachment 123393 [details] [review]:
-----------------------------------------------------------------

Why do this patch instead of having check_cookie_auth return the sessionID instead of the session?
Comment 15 David Cook 2021-08-11 00:51:57 UTC Comment hidden (obsolete)
Comment 16 David Cook 2021-08-11 00:55:28 UTC
Excited to see this change overall. At some point, I would love to rewrite checkauth...
Comment 17 Martin Renvoize 2021-09-10 15:47:45 UTC
Created attachment 124776 [details] [review]
Bug 28785: Centralize cookie auth check in check_cookie_auth

This code is duplicated in 3 different places, we must call
check_cookie_auth instead.

It makes check_cookie_auth returns a 'restricted' when
SessionRestrictionByIP is set and the IP changed.
It also returns a third parameters contained the old and new IP, to fill
the "info" hash in checkauth but apparently the oldip and newip
variables are not even used from the template. We may want to remove it
completely.

No change is expected with this patch, the different authentication
methods should still work as before.

Test plan:
Log in the staff and OPAC interfaces, logout.
Log in and call script that call the 3 different subroutines modified by
this patch. For instance you can list checkouts (that is using
check_cookie_auth) and display a patron's image (using check_api_auth).

QA with good knowledge of the C4::Auth module and the different
authentication methods is required.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2021-09-10 15:47:50 UTC
Created attachment 124777 [details] [review]
Bug 28785: Adjust check_cookie_auth calls

The previous patch makes check_cookie_auth return the session instead of
$sessionID, so we are adjusting the different calls to prevent
confusion.
However they are mainly used to check the authentication status and
don't care about this second variable.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2021-09-10 15:47:53 UTC
Created attachment 124778 [details] [review]
Bug 28785: Add skip_version_check for checkauth

We could have this patch but we also could decide to skip it. The idea
is to avoid 2 checks of the version when we are coming from checkauth.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2021-09-10 15:47:57 UTC
Created attachment 124779 [details] [review]
Bug 28785: Don't send SessionRestrictionByIP to template

It's not used.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2021-09-10 15:50:03 UTC
Always takes a while to wrap my brain around Auth again.. All seems solid in testing and there's certainly a code improvement here.

Tests pass, QA script happy, QA person happy.

Passing QA
Comment 22 Jonathan Druart 2021-10-18 09:34:10 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 23 Marcel de Rooy 2022-01-28 08:43:56 UTC
Did this patch set (inadvertently) have the side-effect of no longer allowing to return to previous page when hitting an unauthorized one ?
Comment 24 Marcel de Rooy 2022-01-28 08:46:36 UTC
(In reply to Marcel de Rooy from comment #23)
> Did this patch set (inadvertently) have the side-effect of no longer
> allowing to return to previous page when hitting an unauthorized one ?

Possible in 20.11, not on master (before 29914 and friends).
It would be my first suspect.
Comment 25 Marcel de Rooy 2022-01-28 08:47:58 UTC
(In reply to Jonathan Druart from comment #6)
> QA with good knowledge of the C4::Auth module and the different
> authentication methods is required.

Certainly :)