Bug 11715 - Z39.50 search page does not check authorization
Summary: Z39.50 search page does not check authorization
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Galen Charlton
QA Contact: Testopia
URL: /cgi-bin/koha/cataloguing/z3950_searc...
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-07 16:57 UTC by Jared Camins-Esakov
Modified: 2015-12-03 22:02 UTC (History)
8 users (show)

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


Attachments
Bug 11715 : Set the default of authnotrequired = 0 in get_template_and_user (971 bytes, patch)
2014-03-10 21:53 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11715 : /cgi-bin/koha/cataloguing/z3950_search.pl doesn't need auth (1016 bytes, patch)
2014-03-10 21:58 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11715 : Fixing no auth on cataloguing/z3950_auth_search.pl (1.02 KB, patch)
2014-03-16 20:17 UTC, Chris Cormack
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11715 : Set the default of authnotrequired = 0 in get_template_and_user (1.02 KB, patch)
2014-04-22 11:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11715 : /cgi-bin/koha/cataloguing/z3950_search.pl doesn't need auth (1.08 KB, patch)
2014-04-22 11:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11715 : Fixing no auth on cataloguing/z3950_auth_search.pl (1.11 KB, patch)
2014-04-22 11:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11715 : Set the default of authnotrequired = 0 in get_template_and_user (1.09 KB, patch)
2014-04-27 21:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11715 : /cgi-bin/koha/cataloguing/z3950_search.pl doesn't need auth (1.16 KB, patch)
2014-04-27 21:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11715 : Fixing no auth on cataloguing/z3950_auth_search.pl (1.18 KB, patch)
2014-04-27 21:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11715: Set the default of authnotrequired = 0 in get_template_and_user (1.27 KB, patch)
2014-04-29 17:13 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 11715: require authentication for the cataloging Z39.50 search (1.15 KB, patch)
2014-04-29 17:13 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 11715 : require authentication for the cataloging authorities Z39.50 search (1.21 KB, patch)
2014-04-29 17:13 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 11715: require authentication for various staff scripts (4.73 KB, patch)
2014-04-29 17:13 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 11715: Set the default of authnotrequired = 0 in get_template_and_user (1.33 KB, patch)
2014-05-02 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11715: require authentication for the cataloging Z39.50 search (1.21 KB, patch)
2014-05-02 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11715 : require authentication for the cataloging authorities Z39.50 search (1.27 KB, patch)
2014-05-02 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11715: require authentication for various staff scripts (4.80 KB, patch)
2014-05-02 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11715: Update POD for get_template_and_user (1.21 KB, patch)
2014-05-02 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11715: Set the default of authnotrequired = 0 in get_template_and_user (1.28 KB, patch)
2014-06-19 21:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11715: require authentication for the cataloging Z39.50 search (1.21 KB, patch)
2014-06-19 21:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11715 : require authentication for the cataloging authorities Z39.50 search (1.26 KB, patch)
2014-06-19 21:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11715: require authentication for various staff scripts (4.50 KB, patch)
2014-06-19 21:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11715: Update POD for get_template_and_user (1.27 KB, patch)
2014-06-19 21:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11715: Set the default of authnotrequired = 0 in get_template_and_user (1.28 KB, patch)
2014-06-19 22:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11715: require authentication for the cataloging Z39.50 search (1.21 KB, patch)
2014-06-19 22:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11715 : require authentication for the cataloging authorities Z39.50 search (1.26 KB, patch)
2014-06-19 22:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11715: require authentication for various staff scripts (4.50 KB, patch)
2014-06-19 22:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11715: Update POD for get_template_and_user (1.27 KB, patch)
2014-06-19 22:01 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Galen Charlton 2014-02-07 17:09:34 UTC
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,
Comment 3 Galen Charlton 2014-02-07 18:16:15 UTC
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.
Comment 4 Galen Charlton 2014-02-07 18:23:33 UTC
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.
Comment 5 Chris Cormack 2014-02-08 04:42:03 UTC
Do we want to hang bugs off this, or just a series of patches on here?
Comment 6 Chris Cormack 2014-03-02 23:14:11 UTC
(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?
Comment 7 Galen Charlton 2014-03-03 15:54:34 UTC
(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.
Comment 8 Chris Cormack 2014-03-10 21:53:28 UTC Comment hidden (obsolete)
Comment 9 Chris Cormack 2014-03-10 21:58:21 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2014-03-11 07:53:30 UTC
Should this be needs sign off? Not sure if it can be tested yet.
Comment 11 Chris Cormack 2014-03-11 19:03:47 UTC
They can be tested, but I plan to add a bunch more, however it doesnt hurt to test these two.
Comment 12 Chris Cormack 2014-03-16 20:17:12 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2014-04-22 11:31:21 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2014-04-22 11:32:31 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2014-04-22 11:32:33 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2014-04-27 21:07:39 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2014-04-27 21:07:50 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2014-04-27 21:08:00 UTC Comment hidden (obsolete)
Comment 19 Galen Charlton 2014-04-29 17:13:40 UTC Comment hidden (obsolete)
Comment 20 Galen Charlton 2014-04-29 17:13:45 UTC Comment hidden (obsolete)
Comment 21 Galen Charlton 2014-04-29 17:13:49 UTC Comment hidden (obsolete)
Comment 22 Galen Charlton 2014-04-29 17:13:54 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2014-05-02 10:08:22 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2014-05-02 10:08:26 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2014-05-02 10:08:30 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2014-05-02 10:08:34 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2014-05-02 10:08:38 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2014-05-02 10:09:30 UTC
What about forcing authnotrequired = 0 if flagsrequired is set, in get_template_and_user?
Comment 29 Galen Charlton 2014-05-26 02:55:58 UTC
(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.
Comment 30 Jonathan Druart 2014-06-06 15:17:19 UTC
(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.
Comment 31 Chris Cormack 2014-06-18 20:03:19 UTC
(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.
Comment 32 Katrin Fischer 2014-06-19 21:55:24 UTC Comment hidden (obsolete)
Comment 33 Katrin Fischer 2014-06-19 21:55:28 UTC Comment hidden (obsolete)
Comment 34 Katrin Fischer 2014-06-19 21:55:31 UTC Comment hidden (obsolete)
Comment 35 Katrin Fischer 2014-06-19 21:55:34 UTC Comment hidden (obsolete)
Comment 36 Katrin Fischer 2014-06-19 21:55:37 UTC Comment hidden (obsolete)
Comment 37 Katrin Fischer 2014-06-19 22:01:34 UTC
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>
Comment 38 Katrin Fischer 2014-06-19 22:01:37 UTC
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>
Comment 39 Katrin Fischer 2014-06-19 22:01:40 UTC
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>
Comment 40 Katrin Fischer 2014-06-19 22:01:43 UTC
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.
Comment 41 Katrin Fischer 2014-06-19 22:01:46 UTC
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>
Comment 42 Tomás Cohen Arazi 2014-06-25 15:34:47 UTC
Patch pushed to master. I've notified the current RMaints for inclusion in the stable branches.

Thanks Chris and everyone involved.
Comment 43 Galen Charlton 2014-06-25 22:44:20 UTC
Pushed to 3.16.x for inclusion in 3.16.1.
Comment 44 Fridolin Somers 2014-06-26 12:11:55 UTC
Pushed to 3.14.x, will be in 3.14.08