We call get_template_and_user with authnotrequired => 1, it should never be 1 but depends on OPACPublic instead git grep -P 'authnotrequired\s*=>\s*1' opac
Created attachment 99084 [details] [review] Bug 24663: Test OpacPublic for all OPAC scripts Prior to this patchset there were 3 different calls to get_template_and_user (or checkauth) with the authnotrequired param: * authnotrequired => 0 * authnotrequired => 1 * authnotrequired => ( C4::Context->preference("OpacPublic") ? 1 : 0 ) The first one says that an unauthenticated user can access the page, the second that the user has to be authenticated, and the last one that it depends on the OpacPublic syspref. Actually we must replace the first one with the third one, if the OPAC is not public, the authentication must be forced. To do so we are going to remove the "authnotrequired => 0" occurrences, and check the OpacPublic syspref's value in C4::Auth
Created attachment 99085 [details] [review] Bug 24663: Remove authnotrequired if set to 0 It defaults to 0 in get_template_and_user
Created attachment 99086 [details] [review] Bug 24663: Force auth in adveditorshortcuts.pl This was wrong!
Created attachment 99087 [details] [review] Bug 24663: Force authentication in svc/records/preview (?) Was this wrong?
Created attachment 99112 [details] [review] Bug 24663: Handle special cases for recovery password and selfreg The password recovery and self-registration features need to be accessible at the OPAC even if not public. Test plan: Self register a new account, then ask for a new password with OpacPublic turned off
(In reply to Jonathan Druart from comment #5) > Created attachment 99112 [details] [review] [review] > Bug 24663: Handle special cases for recovery password and selfreg > > The password recovery and self-registration features need to be > accessible at the OPAC even if not public. > > Test plan: > Self register a new account, then ask for a new password with OpacPublic > turned off That where it gets dirty. Suggestions welcome!
Created attachment 99258 [details] [review] Bug 24663: Test OpacPublic for all OPAC scripts Prior to this patchset there were 3 different calls to get_template_and_user (or checkauth) with the authnotrequired param: * authnotrequired => 0 * authnotrequired => 1 * authnotrequired => ( C4::Context->preference("OpacPublic") ? 1 : 0 ) The first one says that an unauthenticated user can access the page, the second that the user has to be authenticated, and the last one that it depends on the OpacPublic syspref. Actually we must replace the first one with the third one, if the OPAC is not public, the authentication must be forced. To do so we are going to remove the "authnotrequired => 0" occurrences, and check the OpacPublic syspref's value in C4::Auth Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99259 [details] [review] Bug 24663: Remove authnotrequired if set to 0 It defaults to 0 in get_template_and_user Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99260 [details] [review] Bug 24663: Force auth in adveditorshortcuts.pl This was wrong! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99261 [details] [review] Bug 24663: Force authentication in svc/records/preview (?) Was this wrong? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 99262 [details] [review] Bug 24663: Handle special cases for recovery password and selfreg The password recovery and self-registration features need to be accessible at the OPAC even if not public. Test plan: Self register a new account, then ask for a new password with OpacPublic turned off Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Works as expected.. it basically moves us to a small whitelist of always public pages in Auth.pm as opposed to leaving it up to each controller in the opac to specify it.
After running the QA tools almost seemed to kill my computer... I am going to share the findings: FAIL svc/members/search FAIL pod *** ERROR: =back without previous =over in file svc/members/search FAIL reports/reserves_stats.pl FAIL pod *** WARNING: Verbatim paragraph in NAME section in file reports/reserves_stats.pl
It looks like a few have been missed? ... C4/Output.pm: authnotrequired => 1, Koha/Plugins/Base.pm: authnotrequired => 1, admin/credit_types.pl: authnotrequired => 0, catalogue/item-export.pl: authnotrequired => 0, errors/400.pl: authnotrequired => 1, errors/401.pl: authnotrequired => 1, errors/402.pl: authnotrequired => 1, errors/403.pl: authnotrequired => 1, errors/500.pl: authnotrequired => 1, help.pl: authnotrequired => 1,
(In reply to Katrin Fischer from comment #13) > After running the QA tools almost seemed to kill my computer... I am going > to share the findings: > > FAIL svc/members/search > FAIL pod > *** ERROR: =back without previous =over in file svc/members/search > > FAIL reports/reserves_stats.pl > FAIL pod > *** WARNING: > Verbatim paragraph in NAME section > > in file reports/reserves_stats.pl This patch does not modify POD. No related. (In reply to Katrin Fischer from comment #14) > It looks like a few have been missed? > > ... > C4/Output.pm: authnotrequired => 1, to output the error > Koha/Plugins/Base.pm: authnotrequired => 1, That's for plugin, I guess that's correct. > admin/credit_types.pl: authnotrequired => 0, > catalogue/item-export.pl: authnotrequired => 0, 2 newly added scripts, follow-up coming. > errors/400.pl: authnotrequired => 1, > errors/401.pl: authnotrequired => 1, > errors/402.pl: authnotrequired => 1, > errors/403.pl: authnotrequired => 1, > errors/500.pl: authnotrequired => 1, For errors. > help.pl: authnotrequired => 1, For help page, cannot hurt I suppose (?)
Created attachment 99859 [details] [review] Bug 24663: (follow-up) Remove authnotrequired if set to 0 2 newly added scripts
I have added a quick test to the QA script: https://gitlab.com/koha-community/qa-test-tools/issues/27
I like the whitelist approach here... looking forward to the final QA :)
Created attachment 100972 [details] [review] Bug 24663: Test OpacPublic for all OPAC scripts Prior to this patchset there were 3 different calls to get_template_and_user (or checkauth) with the authnotrequired param: * authnotrequired => 0 * authnotrequired => 1 * authnotrequired => ( C4::Context->preference("OpacPublic") ? 1 : 0 ) The first one says that an unauthenticated user can access the page, the second that the user has to be authenticated, and the last one that it depends on the OpacPublic syspref. Actually we must replace the first one with the third one, if the OPAC is not public, the authentication must be forced. To do so we are going to remove the "authnotrequired => 0" occurrences, and check the OpacPublic syspref's value in C4::Auth Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 100973 [details] [review] Bug 24663: Remove authnotrequired if set to 0 It defaults to 0 in get_template_and_user Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 100974 [details] [review] Bug 24663: Force auth in adveditorshortcuts.pl This was wrong! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 100975 [details] [review] Bug 24663: Force authentication in svc/records/preview (?) Was this wrong? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 100976 [details] [review] Bug 24663: Handle special cases for recovery password and selfreg The password recovery and self-registration features need to be accessible at the OPAC even if not public. Test plan: Self register a new account, then ask for a new password with OpacPublic turned off Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 100977 [details] [review] Bug 24663: (follow-up) Remove authnotrequired if set to 0 2 newly added scripts
Created attachment 100978 [details] [review] Bug 24663: Force authentication in svc/records/preview (?) Was this wrong? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 100979 [details] [review] Bug 24663: Handle special cases for recovery password and selfreg The password recovery and self-registration features need to be accessible at the OPAC even if not public. Test plan: Self register a new account, then ask for a new password with OpacPublic turned off Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 100980 [details] [review] Bug 24663: (follow-up) Remove authnotrequired if set to 0 2 newly added scripts Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
does not apply to 19.11.x branch. please rebase
Created attachment 105508 [details] [review] Bug 24663: [19.11.x] Test OpacPublic for all OPAC scripts Prior to this patchset there were 3 different calls to get_template_and_user (or checkauth) with the authnotrequired param: * authnotrequired => 0 * authnotrequired => 1 * authnotrequired => ( C4::Context->preference("OpacPublic") ? 1 : 0 ) The first one says that an unauthenticated user can access the page, the second that the user has to be authenticated, and the last one that it depends on the OpacPublic syspref. Actually we must replace the first one with the third one, if the OPAC is not public, the authentication must be forced. To do so we are going to remove the "authnotrequired => 0" occurrences, and check the OpacPublic syspref's value in C4::Auth Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 105509 [details] [review] Bug 24663: [19.11.x] Remove authnotrequired if set to 0 It defaults to 0 in get_template_and_user Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 105510 [details] [review] Bug 24663: [19.11.x] Force auth in adveditorshortcuts.pl This was wrong! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 105511 [details] [review] Bug 24663: [19.11.x] Force authentication in svc/records/preview (?) Was this wrong? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 105512 [details] [review] Bug 24663: [19.11.x] Handle special cases for recovery password and selfreg The password recovery and self-registration features need to be accessible at the OPAC even if not public. Test plan: Self register a new account, then ask for a new password with OpacPublic turned off Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 105513 [details] [review] Bug 24663: [19.05.x] Test OpacPublic for all OPAC scripts Prior to this patchset there were 3 different calls to get_template_and_user (or checkauth) with the authnotrequired param: * authnotrequired => 0 * authnotrequired => 1 * authnotrequired => ( C4::Context->preference("OpacPublic") ? 1 : 0 ) The first one says that an unauthenticated user can access the page, the second that the user has to be authenticated, and the last one that it depends on the OpacPublic syspref. Actually we must replace the first one with the third one, if the OPAC is not public, the authentication must be forced. To do so we are going to remove the "authnotrequired => 0" occurrences, and check the OpacPublic syspref's value in C4::Auth Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 105514 [details] [review] Bug 24663: [19.05.x] Remove authnotrequired if set to 0 It defaults to 0 in get_template_and_user Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 105515 [details] [review] Bug 24663: [19.05.x] Force auth in adveditorshortcuts.pl This was wrong! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 105516 [details] [review] Bug 24663: [19.05.x] Force authentication in svc/records/preview (?) Was this wrong? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 105517 [details] [review] Bug 24663: [19.05.x] Handle special cases for recovery password and selfreg The password recovery and self-registration features need to be accessible at the OPAC even if not public. Test plan: Self register a new account, then ask for a new password with OpacPublic turned off Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 105518 [details] [review] Bug 24663: [18.11.x] Test OpacPublic for all OPAC scripts Prior to this patchset there were 3 different calls to get_template_and_user (or checkauth) with the authnotrequired param: * authnotrequired => 0 * authnotrequired => 1 * authnotrequired => ( C4::Context->preference("OpacPublic") ? 1 : 0 ) The first one says that an unauthenticated user can access the page, the second that the user has to be authenticated, and the last one that it depends on the OpacPublic syspref. Actually we must replace the first one with the third one, if the OPAC is not public, the authentication must be forced. To do so we are going to remove the "authnotrequired => 0" occurrences, and check the OpacPublic syspref's value in C4::Auth Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 105519 [details] [review] Bug 24663: [18.11.x] Remove authnotrequired if set to 0 It defaults to 0 in get_template_and_user Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 105520 [details] [review] Bug 24663: [18.11.x] Force authentication in svc/records/preview (?) Was this wrong? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 105521 [details] [review] Bug 24663: [18.11.x] Handle special cases for recovery password and selfreg The password recovery and self-registration features need to be accessible at the OPAC even if not public. Test plan: Self register a new account, then ask for a new password with OpacPublic turned off Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
For 19.11.x - can you please clarify how these cases are covered in this patchset? Just for my own peace of mind - feels like there's a lot leftover/missed. Thank you! $ git grep -P 'authnotrequired\s*=>\s*1' C4/InstallAuth.pm: authnotrequired => 1, C4/InstallAuth.pm: authnotrequired => 1, C4/Output.pm: authnotrequired => 1, Koha/Plugins/Base.pm: authnotrequired => 1, errors/400.pl: authnotrequired => 1, errors/401.pl: authnotrequired => 1, errors/402.pl: authnotrequired => 1, errors/403.pl: authnotrequired => 1, errors/500.pl: authnotrequired => 1, help.pl: authnotrequired => 1, opac/errors/400.pl: authnotrequired => 1, opac/errors/401.pl: authnotrequired => 1, opac/errors/402.pl: authnotrequired => 1, opac/errors/403.pl: authnotrequired => 1, opac/errors/404.pl: authnotrequired => 1, opac/errors/500.pl: authnotrequired => 1, opac/ilsdi.pl: authnotrequired => 1, opac/ilsdi.pl: authnotrequired => 1, opac/opac-authorities-home.pl: authnotrequired => 1, opac/opac-course-details.pl: authnotrequired => 1, opac/opac-course-reserves.pl: authnotrequired => 1, opac/opac-memberentry.pl: authnotrequired => 1, opac/opac-memberentry.pl: authnotrequired => 1, opac/opac-memberentry.pl: authnotrequired => 1, opac/opac-memberentry.pl: authnotrequired => 1, opac/opac-overdrive-search.pl: authnotrequired => 1, opac/opac-password-recovery.pl: authnotrequired => 1, opac/opac-recordedbooks-search.pl: authnotrequired => 1, opac/opac-registration-verify.pl: authnotrequired => 1, opac/opac-registration-verify.pl: authnotrequired => 1, opac/opac-retrieve-file.pl: authnotrequired => 1, opac/opac-sendshelf.pl: authnotrequired => 1, opac/opac-serial-issues.pl: authnotrequired => 1, opac/opac-serial-issues.pl: authnotrequired => 1, opac/sco/help.pl: authnotrequired => 1, opac/svc/checkout_notes: authnotrequired => 1, t/db_dependent/Auth.t: authnotrequired => 1, t/db_dependent/Auth.t: authnotrequired => 1, t/db_dependent/Auth.t: authnotrequired => 1, t/db_dependent/Auth.t: authnotrequired => 1, t/db_dependent/Auth.t: authnotrequired => 1, t/db_dependent/Auth.t: authnotrequired => 1, t/db_dependent/Auth.t: authnotrequired => 1, t/db_dependent/Auth.t: authnotrequired => 1, t/db_dependent/Templates.t: authnotrequired => 1,
Hi Aleisha, The idea of this change is to unify the way "authnotrequired" is handled, and make C4::Auth guess its value if it's not passed. By default we consider that the authentication is required to access whichever pages, and that if an OPAC script passes authnotrequired => 1 when OpacPublic is off, then authnotrequired is turned off by C4::Auth::checkauth. We are then removing the need to test OpacPublic in all our OPAC scripts, and the need to pass authnotrequired => 0 from scripts where auth is required (default). You will note that there are still room for improvements, for instance: opac/opac-detail.pl: authnotrequired => ( C4::Context->preference("OpacPublic") ? 1 : 0 ), Can be replaced with opac/opac-detail.pl: authnotrequired => 1, Finally, to answer your question, it's totally find to have "authnotquired => 1" at the OPAC. We explicitelly tell that this script can be accessed to an unauthenticated user (or, in specific case, that the auth has been done already, like in opac/svc/checkout_notes). Of course a double check of all these occurrences could be done (the more eyes that review these patches the better!)
Here is the test plan I could come up with. (and tried on 19.05.x) This patchset feels more secure now :D == Test plan == 1. Basic functionality 1. Apply the patch 2. Go to the OPAC: is should work 3. OpacPublic => false 4. Go to the OPAC: login required 2. Self reg and password reset 1. OpacPublic => false 2. Self register a new account, it should work 3. OpacResetPassword => true 4. ask for a new password, it should work 3. General regressions 1. OpacPublic => false 2. The following direct accesses should fail (auth required) 3. OPAC_URL/cgi-bin/koha/opac-search.pl?q=test 4. OPAC_URL/cgi-bin/koha/opac-shelves.pl 5. OPAC_URL/cgi-bin/koha/opac-shelves.pl?op=list 6. OPAC_URL/cgi-bin/koha/opac-search.pl 7. OPAC_URL/cgi-bin/koha/opac-course-reserves.pl 8. OPAC_URL/cgi-bin/koha/opac-authorities-home.pl 9. OPAC_URL/cgi-bin/koha/opac-tags.pl 10. The API should still work: OPAC_URL/api/v1/ 11. OpacPublic => false 12. Retry the above and they should not ask for authentication
backported to 19.11.x for 19.11.09
backported to 20.05.x for 20.05.03
Pushed to master for 20.11, thanks to everybody involved!
Similar definition is found on intranet. In some calls to get_template_and_user authnotrequired => 1 and in others is 0 Shall that also need to be fixed?
(In reply to Alvaro Cornejo from comment #49) > Similar definition is found on intranet. > > In some calls to get_template_and_user authnotrequired => 1 and in others is > 0 > > Shall that also need to be fixed? Which files exactly are you talking about?
Hi In 19.11.09 /usr/share/koha/intranet/cgi-bin/help.pl: authnotrequired => 1, /usr/share/koha/intranet/cgi-bin/acqui/neworderbiblio.pl: authnotrequired => 0, /usr/share/koha/intranet/cgi-bin/errors/403.pl: authnotrequired => 1, /usr/share/koha/intranet/cgi-bin/errors/401.pl: authnotrequired => 1, /usr/share/koha/intranet/cgi-bin/errors/500.pl: authnotrequired => 1, /usr/share/koha/intranet/cgi-bin/errors/400.pl: authnotrequired => 1, /usr/share/koha/intranet/cgi-bin/errors/402.pl: authnotrequired => 1, /usr/share/koha/intranet/cgi-bin/tools/batch_extend_due_dates.pl: authnotrequired => 0, /usr/share/koha/intranet/cgi-bin/admin/printers.pl: authnotrequired => 0,
Backported to 19.05.x branch for 19.05.14 that was released last week.
(In reply to Alvaro Cornejo from comment #51) Those authnotrequired => 1 are for error messages and help only, that should not be a problem: > /usr/share/koha/intranet/cgi-bin/help.pl: authnotrequired => 1, > /usr/share/koha/intranet/cgi-bin/errors/403.pl: authnotrequired => 1, > /usr/share/koha/intranet/cgi-bin/errors/401.pl: authnotrequired => 1, > /usr/share/koha/intranet/cgi-bin/errors/500.pl: authnotrequired => 1, > /usr/share/koha/intranet/cgi-bin/errors/400.pl: authnotrequired => 1, > /usr/share/koha/intranet/cgi-bin/errors/402.pl: authnotrequired => 1, This is in master, I will follow-up (no need to backport it): > /usr/share/koha/intranet/cgi-bin/tools/batch_extend_due_dates.pl: > authnotrequired => 0, Those 2 ones are in stable releases only, can stay as it: > /usr/share/koha/intranet/cgi-bin/acqui/neworderbiblio.pl: > authnotrequired => 0, > /usr/share/koha/intranet/cgi-bin/admin/printers.pl: authnotrequired > => 0,
Created attachment 109793 [details] [review] Bug 24663: Fix one missing occurrence pushed in the meanwhile
(In reply to Jonathan Druart from comment #54) > Created attachment 109793 [details] [review] [review] > Bug 24663: Fix one missing occurrence > > pushed in the meanwhile Pushed to master for 20.11 (no need to backport)