And it looks like there are a bunch of questionable uses of the authnotrequired flag. Here is a list, note that at least some of these are likely OK. As far as the OPAC is concerned, at least some of them should be checking on OpacPublic but don't, currently. C4/Auth.pm: authnotrequired => 1, C4/Auth.pm: authnotrequired => 1, C4/Auth.pm:This call passes the C<query>, C<flagsrequired> and C<authnotrequired> C4/Auth.pm: $in->{'authnotrequired'}, C4/Auth.pm: # $authnotrequired will be set for scripts which will run without authentication C4/Auth.pm: my $authnotrequired = shift; C4/Auth.pm: if ( $loggedin || $authnotrequired ) Koha/Plugins/Base.pm: authnotrequired => 1, acqui/newordersuggestion.pl: authnotrequired => 1, acqui/transferorder.pl: authnotrequired => 1, acqui/z3950_search.pl: authnotrequired => 1, catalogue/cardview.pl: authnotrequired => 1, cataloguing/z3950_auth_search.pl: authnotrequired => 1, cataloguing/z3950_search.pl: authnotrequired => 1, errors/400.pl: authnotrequired => 1, errors/401.pl: authnotrequired => 1, errors/402.pl: authnotrequired => 1, errors/403.pl: authnotrequired => 1, errors/404.pl: authnotrequired => 1, errors/500.pl: authnotrequired => 1, installer/InstallAuth.pm: authnotrequired => 1, installer/InstallAuth.pm: authnotrequired => 1, installer/InstallAuth.pm: This call passes the C<query>, C<flagsrequired> and C<authnotrequired> installer/InstallAuth.pm: $in->{'authnotrequired'}, installer/InstallAuth.pm:# $authnotrequired will be set for scripts which will run without authentication installer/InstallAuth.pm: my $authnotrequired = shift; 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/maintenance.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-downloadcart.pl: authnotrequired => 1, opac/opac-downloadshelf.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-registration-verify.pl: authnotrequired => 1, opac/opac-registration-verify.pl: authnotrequired => 1, opac/opac-search-history.pl: authnotrequired => 1, opac/opac-sendshelf.pl: authnotrequired => 1, opac/opac-serial-issues.pl: authnotrequired => 1, opac/opac-serial-issues.pl: authnotrequired => 1, opac/opac-tags.pl: authnotrequired => ($add_op ? 0 : 1), # auth required to add tags opac/sco/help.pl: authnotrequired => 1, opac/search.pl: authnotrequired => 1, reports/stats.screen.pl: authnotrequired => 1, serials/serial-issues.pl: authnotrequired => 1, services/itemrecorddisplay.pl: authnotrequired => 1,
Since 99% of the time, if not 100% of the time, the staff interface should always require authentication, passing authnotrequired to get_template_and_user() by staff scripts invites errors. I propose that get_template_and_user() initialize that flag to false by default, allow more scripts to not pass it in at all.
And for the OPAC (as determined by the type argument), the default value of authnotrequired could check the OpacPublic system preference. That way, for most staff scripts the get_template_call would look like: get_template_and_user({ template_name => "XXX", type => "intranet", query => $input, flagsrequired => { ... } }); while for OPAC it would look like get_template_and_user({ template_name => "XXX", type => "opac", query => $input, flagsrequired => { ... } }); 'authonotrequired' would be explicitly passed only in the few cases where a staff-side script legitimately requires no authentication or an OPAC-side script requires a value that does not depend on OpacPublic.
Do we want to hang bugs off this, or just a series of patches on here?
(In reply to Chris Cormack from comment #5) > Do we want to hang bugs off this, or just a series of patches on here? Im happy to work on this, just want to know if you want me to do a bunch of patches here, or one per page?
(In reply to Chris Cormack from comment #6) > (In reply to Chris Cormack from comment #5) > > Do we want to hang bugs off this, or just a series of patches on here? > > Im happy to work on this, just want to know if you want me to do a bunch of > patches here, or one per page? Thanks. Please do a series of patches attached to this one.
Created attachment 26039 [details] [review] Bug 11715 : Set the default of authnotrequired = 0 in get_template_and_user To test: This patch should be a no op Nothing should change, please try a few pages and check that auth behaviing correctly Follow up patches are to remove all the unnessecary setting of this value, so that the only places we set are when we do want authnotrequired=1
Created attachment 26040 [details] [review] Bug 11715 : /cgi-bin/koha/cataloguing/z3950_search.pl doesn't need auth To test: 1/ go to /cgi-bin/koha/cataloguing/z3950_search.pl when you are not logged in, notice no log in is enforced 2/ Apply patch 3/ go to /cgi-bin/koha/cataloguing/z3950_search.pl notice you now need to login
Should this be needs sign off? Not sure if it can be tested yet.
They can be tested, but I plan to add a bunch more, however it doesnt hurt to test these two.
Created attachment 26388 [details] [review] Bug 11715 : Fixing no auth on cataloguing/z3950_auth_search.pl To test: 1/ go to /cgi-bin/koha/cataloguing/z3950_auth_search.pl when you are not logged in, notice no log in is enforced 2/ Apply patch 3/ go to /cgi-bin/koha/cataloguing/z3950_auth_search.pl notice you now need to login
Created attachment 27393 [details] [review] [SIGNED-OFF] Bug 11715 : Set the default of authnotrequired = 0 in get_template_and_user To test: This patch should be a no op Nothing should change, please try a few pages and check that auth behaviing correctly Follow up patches are to remove all the unnessecary setting of this value, so that the only places we set are when we do want authnotrequired=1 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27394 [details] [review] [SIGNED-OFF] Bug 11715 : /cgi-bin/koha/cataloguing/z3950_search.pl doesn't need auth To test: 1/ go to /cgi-bin/koha/cataloguing/z3950_search.pl when you are not logged in, notice no log in is enforced 2/ Apply patch 3/ go to /cgi-bin/koha/cataloguing/z3950_search.pl notice you now need to login Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27395 [details] [review] [SIGNED-OFF] Bug 11715 : Fixing no auth on cataloguing/z3950_auth_search.pl To test: 1/ go to /cgi-bin/koha/cataloguing/z3950_auth_search.pl when you are not logged in, notice no log in is enforced 2/ Apply patch 3/ go to /cgi-bin/koha/cataloguing/z3950_auth_search.pl notice you now need to login Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27684 [details] [review] [PASSED QA] Bug 11715 : Set the default of authnotrequired = 0 in get_template_and_user To test: This patch should be a no op Nothing should change, please try a few pages and check that auth behaviing correctly Follow up patches are to remove all the unnessecary setting of this value, so that the only places we set are when we do want authnotrequired=1 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> No regressions found.
Created attachment 27685 [details] [review] [PASSED QA] Bug 11715 : /cgi-bin/koha/cataloguing/z3950_search.pl doesn't need auth To test: 1/ go to /cgi-bin/koha/cataloguing/z3950_search.pl when you are not logged in, notice no log in is enforced 2/ Apply patch 3/ go to /cgi-bin/koha/cataloguing/z3950_search.pl notice you now need to login Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Confirmed bug and that the patch fixes it.
Created attachment 27686 [details] [review] [PASSED QA] Bug 11715 : Fixing no auth on cataloguing/z3950_auth_search.pl To test: 1/ go to /cgi-bin/koha/cataloguing/z3950_auth_search.pl when you are not logged in, notice no log in is enforced 2/ Apply patch 3/ go to /cgi-bin/koha/cataloguing/z3950_auth_search.pl notice you now need to login Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Confirmed bug and that patch fixes it.
Created attachment 27775 [details] [review] Bug 11715: Set the default of authnotrequired = 0 in get_template_and_user To test: Verify that pages in the OPAC and staff interface display correctly. Note that there are cases where 'authnotrequired' was not passed at all to get_template_and_user, so there may be pages that start requiring authentication. Whether that is correct or not depends on context. Follow up patches are to remove all the unnessecary setting of this value, so that the only places we set are when we do want authnotrequired=1 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Created attachment 27776 [details] [review] Bug 11715: require authentication for the cataloging Z39.50 search To test: 1/ go to /cgi-bin/koha/cataloguing/z3950_search.pl when you are not logged in, notice no log in is enforced 2/ Apply patch 3/ go to /cgi-bin/koha/cataloguing/z3950_search.pl notice you now need to login Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Created attachment 27777 [details] [review] Bug 11715 : require authentication for the cataloging authorities Z39.50 search To test: 1/ go to /cgi-bin/koha/cataloguing/z3950_auth_search.pl when you are not logged in, notice no log in is enforced 2/ Apply patch 3/ go to /cgi-bin/koha/cataloguing/z3950_auth_search.pl notice you now need to login Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Created attachment 27778 [details] [review] Bug 11715: require authentication for various staff scripts To test: [1] Verify that item record creating and editing during the acquisitions process continues to work. [2] Verify that calling services/itemrecorddisplay.pl without a valid user session fails. [3] Verify that authentication is required for making a new order from a suggestion, transferring an order, doing a Z39.50 search from acquisitions, displaying the record card view in the staff interface, and running the till reconciliation report (/cgi-bin/koha/reports/stats.screen.pl) Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Created attachment 27912 [details] [review] Bug 11715: Set the default of authnotrequired = 0 in get_template_and_user To test: Verify that pages in the OPAC and staff interface display correctly. Note that there are cases where 'authnotrequired' was not passed at all to get_template_and_user, so there may be pages that start requiring authentication. Whether that is correct or not depends on context. Follow up patches are to remove all the unnessecary setting of this value, so that the only places we set are when we do want authnotrequired=1 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 27913 [details] [review] Bug 11715: require authentication for the cataloging Z39.50 search To test: 1/ go to /cgi-bin/koha/cataloguing/z3950_search.pl when you are not logged in, notice no log in is enforced 2/ Apply patch 3/ go to /cgi-bin/koha/cataloguing/z3950_search.pl notice you now need to login Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 27914 [details] [review] Bug 11715 : require authentication for the cataloging authorities Z39.50 search To test: 1/ go to /cgi-bin/koha/cataloguing/z3950_auth_search.pl when you are not logged in, notice no log in is enforced 2/ Apply patch 3/ go to /cgi-bin/koha/cataloguing/z3950_auth_search.pl notice you now need to login Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 27915 [details] [review] Bug 11715: require authentication for various staff scripts To test: [1] Verify that item record creating and editing during the acquisitions process continues to work. [2] Verify that calling services/itemrecorddisplay.pl without a valid user session fails. [3] Verify that authentication is required for making a new order from a suggestion, transferring an order, doing a Z39.50 search from acquisitions, displaying the record card view in the staff interface, and running the till reconciliation report (/cgi-bin/koha/reports/stats.screen.pl) Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 27916 [details] [review] Bug 11715: Update POD for get_template_and_user If flagsrequired is set, authnotrequired should be 0. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
What about forcing authnotrequired = 0 if flagsrequired is set, in get_template_and_user?
(In reply to Jonathan Druart from comment #28) > What about forcing authnotrequired = 0 if flagsrequired is set, in > get_template_and_user? Good idea. i will test tomorrow.
(In reply to Galen Charlton from comment #29) > (In reply to Jonathan Druart from comment #28) > > What about forcing authnotrequired = 0 if flagsrequired is set, in > > get_template_and_user? > > Good idea. i will test tomorrow. Hum, what is the next step here ? My last patch only updates POD. My suggestion (comment 28) can be done in another bug, since this one is a security issue and major.
(In reply to Jonathan Druart from comment #30) > (In reply to Galen Charlton from comment #29) > > (In reply to Jonathan Druart from comment #28) > > > What about forcing authnotrequired = 0 if flagsrequired is set, in > > > get_template_and_user? > > > > Good idea. i will test tomorrow. > > Hum, what is the next step here ? My last patch only updates POD. My > suggestion (comment 28) can be done in another bug, since this one is a > security issue and major. I agree, can we get the patches here passed qa, and pushed, then work on another bug.
Created attachment 29063 [details] [review] Bug 11715: Set the default of authnotrequired = 0 in get_template_and_user To test: Verify that pages in the OPAC and staff interface display correctly. Note that there are cases where 'authnotrequired' was not passed at all to get_template_and_user, so there may be pages that start requiring authentication. Whether that is correct or not depends on context. Follow up patches are to remove all the unnessecary setting of this value, so that the only places we set are when we do want authnotrequired=1 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 29064 [details] [review] Bug 11715: require authentication for the cataloging Z39.50 search To test: 1/ go to /cgi-bin/koha/cataloguing/z3950_search.pl when you are not logged in, notice no log in is enforced 2/ Apply patch 3/ go to /cgi-bin/koha/cataloguing/z3950_search.pl notice you now need to login Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 29065 [details] [review] Bug 11715 : require authentication for the cataloging authorities Z39.50 search To test: 1/ go to /cgi-bin/koha/cataloguing/z3950_auth_search.pl when you are not logged in, notice no log in is enforced 2/ Apply patch 3/ go to /cgi-bin/koha/cataloguing/z3950_auth_search.pl notice you now need to login Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 29066 [details] [review] [PASSED QA] Bug 11715: require authentication for various staff scripts To test: [1] Verify that item record creating and editing during the acquisitions process continues to work. [2] Verify that calling services/itemrecorddisplay.pl without a valid user session fails. [3] Verify that authentication is required for making a new order from a suggestion, transferring an order, doing a Z39.50 search from acquisitions, displaying the record card view in the staff interface, and running the till reconciliation report (/cgi-bin/koha/reports/stats.screen.pl) Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Verified all changed scripts are not accessible witout a valid user session, but are with one.
Created attachment 29067 [details] [review] [PASSED QA] Bug 11715: Update POD for get_template_and_user If flagsrequired is set, authnotrequired should be 0. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 29068 [details] [review] Bug 11715: Set the default of authnotrequired = 0 in get_template_and_user To test: Verify that pages in the OPAC and staff interface display correctly. Note that there are cases where 'authnotrequired' was not passed at all to get_template_and_user, so there may be pages that start requiring authentication. Whether that is correct or not depends on context. Follow up patches are to remove all the unnessecary setting of this value, so that the only places we set are when we do want authnotrequired=1 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 29069 [details] [review] Bug 11715: require authentication for the cataloging Z39.50 search To test: 1/ go to /cgi-bin/koha/cataloguing/z3950_search.pl when you are not logged in, notice no log in is enforced 2/ Apply patch 3/ go to /cgi-bin/koha/cataloguing/z3950_search.pl notice you now need to login Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 29070 [details] [review] Bug 11715 : require authentication for the cataloging authorities Z39.50 search To test: 1/ go to /cgi-bin/koha/cataloguing/z3950_auth_search.pl when you are not logged in, notice no log in is enforced 2/ Apply patch 3/ go to /cgi-bin/koha/cataloguing/z3950_auth_search.pl notice you now need to login Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 29071 [details] [review] [PASSED QA] Bug 11715: require authentication for various staff scripts To test: [1] Verify that item record creating and editing during the acquisitions process continues to work. [2] Verify that calling services/itemrecorddisplay.pl without a valid user session fails. [3] Verify that authentication is required for making a new order from a suggestion, transferring an order, doing a Z39.50 search from acquisitions, displaying the record card view in the staff interface, and running the till reconciliation report (/cgi-bin/koha/reports/stats.screen.pl) Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Verified all changed scripts are not accessible witout a valid user session, but are with one.
Created attachment 29072 [details] [review] [PASSED QA] Bug 11715: Update POD for get_template_and_user If flagsrequired is set, authnotrequired should be 0. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Patch pushed to master. I've notified the current RMaints for inclusion in the stable branches. Thanks Chris and everyone involved.
Pushed to 3.16.x for inclusion in 3.16.1.
Pushed to 3.14.x, will be in 3.14.08