Description
Jonathan Druart
2021-07-30 11:54:35 UTC
Created attachment 123314 [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. Created attachment 123315 [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. Created attachment 123316 [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. Created attachment 123317 [details] [review] Bug 28785: Don't send SessionRestrictionByIP to template It's not used. Created attachment 123319 [details] [review] Bug 28785: Add missing POD for check_cookie_auth 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. 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. 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. Created attachment 123323 [details] [review] Bug 28785: Don't send SessionRestrictionByIP to template It's not used. 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> 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> 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> 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 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 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? Excited to see this change overall. At some point, I would love to rewrite checkauth... 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> 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> 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> 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> 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 Pushed to master for 21.11, thanks to everybody involved! Did this patch set (inadvertently) have the side-effect of no longer allowing to return to previous page when hitting an unauthorized one ? (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. (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 :) |