Bug 24663 - OPACPublic must be tested for all opac scripts
Summary: OPACPublic must be tested for all opac scripts
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-02-14 13:16 UTC by Jonathan Druart
Modified: 2020-09-09 13:42 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.03, 19.11.09, 19.05.14


Attachments
Bug 24663: Test OpacPublic for all OPAC scripts (1.31 KB, patch)
2020-02-17 12:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: Remove authnotrequired if set to 0 (235.95 KB, patch)
2020-02-17 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: Force auth in adveditorshortcuts.pl (873 bytes, patch)
2020-02-17 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: Force authentication in svc/records/preview (?) (780 bytes, patch)
2020-02-17 12:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: Handle special cases for recovery password and selfreg (2.34 KB, patch)
2020-02-17 15:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: Test OpacPublic for all OPAC scripts (1.37 KB, patch)
2020-02-19 16:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24663: Remove authnotrequired if set to 0 (236.02 KB, patch)
2020-02-19 16:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24663: Force auth in adveditorshortcuts.pl (939 bytes, patch)
2020-02-19 16:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24663: Force authentication in svc/records/preview (?) (846 bytes, patch)
2020-02-19 16:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24663: Handle special cases for recovery password and selfreg (2.40 KB, patch)
2020-02-19 16:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24663: (follow-up) Remove authnotrequired if set to 0 (1.29 KB, patch)
2020-03-02 09:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: Test OpacPublic for all OPAC scripts (1.42 KB, patch)
2020-03-18 17:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24663: Remove authnotrequired if set to 0 (235.55 KB, patch)
2020-03-18 17:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24663: Force auth in adveditorshortcuts.pl (939 bytes, patch)
2020-03-18 17:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24663: Force authentication in svc/records/preview (?) (846 bytes, patch)
2020-03-18 17:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24663: Handle special cases for recovery password and selfreg (2.40 KB, patch)
2020-03-18 17:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24663: (follow-up) Remove authnotrequired if set to 0 (1.29 KB, patch)
2020-03-18 17:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24663: Force authentication in svc/records/preview (?) (993 bytes, patch)
2020-03-18 18:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24663: Handle special cases for recovery password and selfreg (2.55 KB, patch)
2020-03-18 18:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24663: (follow-up) Remove authnotrequired if set to 0 (1.43 KB, patch)
2020-03-18 18:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24663: [19.11.x] Test OpacPublic for all OPAC scripts (1.43 KB, patch)
2020-06-03 11:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: [19.11.x] Remove authnotrequired if set to 0 (234.05 KB, patch)
2020-06-03 11:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: [19.11.x] Force auth in adveditorshortcuts.pl (949 bytes, patch)
2020-06-03 11:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: [19.11.x] Force authentication in svc/records/preview (?) (1004 bytes, patch)
2020-06-03 11:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: [19.11.x] Handle special cases for recovery password and selfreg (2.56 KB, patch)
2020-06-03 11:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: [19.05.x] Test OpacPublic for all OPAC scripts (1.43 KB, patch)
2020-06-03 11:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: [19.05.x] Remove authnotrequired if set to 0 (232.92 KB, patch)
2020-06-03 11:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: [19.05.x] Force auth in adveditorshortcuts.pl (949 bytes, patch)
2020-06-03 11:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: [19.05.x] Force authentication in svc/records/preview (?) (1004 bytes, patch)
2020-06-03 11:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: [19.05.x] Handle special cases for recovery password and selfreg (2.56 KB, patch)
2020-06-03 11:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: [18.11.x] Test OpacPublic for all OPAC scripts (1.43 KB, patch)
2020-06-03 11:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: [18.11.x] Remove authnotrequired if set to 0 (231.19 KB, patch)
2020-06-03 11:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: [18.11.x] Force authentication in svc/records/preview (?) (1004 bytes, patch)
2020-06-03 11:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: [18.11.x] Handle special cases for recovery password and selfreg (2.56 KB, patch)
2020-06-03 11:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24663: Fix one missing occurrence (860 bytes, patch)
2020-09-09 12:48 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-02-14 13:16:39 UTC
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
Comment 1 Jonathan Druart 2020-02-17 12:38:57 UTC
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
Comment 2 Jonathan Druart 2020-02-17 12:39:01 UTC
Created attachment 99085 [details] [review]
Bug 24663: Remove authnotrequired if set to 0

It defaults to 0 in get_template_and_user
Comment 3 Jonathan Druart 2020-02-17 12:39:04 UTC
Created attachment 99086 [details] [review]
Bug 24663: Force auth in adveditorshortcuts.pl

This was wrong!
Comment 4 Jonathan Druart 2020-02-17 12:39:08 UTC
Created attachment 99087 [details] [review]
Bug 24663: Force authentication in svc/records/preview (?)

Was this wrong?
Comment 5 Jonathan Druart 2020-02-17 15:46:07 UTC
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
Comment 6 Jonathan Druart 2020-02-17 15:47:41 UTC
(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!
Comment 7 Martin Renvoize 2020-02-19 16:35:05 UTC
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>
Comment 8 Martin Renvoize 2020-02-19 16:35:09 UTC
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>
Comment 9 Martin Renvoize 2020-02-19 16:35:12 UTC
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>
Comment 10 Martin Renvoize 2020-02-19 16:35:15 UTC
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>
Comment 11 Martin Renvoize 2020-02-19 16:35:18 UTC
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>
Comment 12 Martin Renvoize 2020-02-19 16:36:50 UTC
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.
Comment 13 Katrin Fischer 2020-02-29 22:15:55 UTC
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
Comment 14 Katrin Fischer 2020-02-29 22:29:28 UTC
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,
Comment 15 Jonathan Druart 2020-03-02 09:06:30 UTC
(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 (?)
Comment 16 Jonathan Druart 2020-03-02 09:08:43 UTC
Created attachment 99859 [details] [review]
Bug 24663: (follow-up) Remove authnotrequired if set to 0

2 newly added scripts
Comment 17 Jonathan Druart 2020-03-02 13:24:59 UTC
I have added a quick test to the QA script: https://gitlab.com/koha-community/qa-test-tools/issues/27
Comment 18 Martin Renvoize 2020-03-11 10:35:03 UTC
I like the whitelist approach here... looking forward to the final QA :)
Comment 19 Tomás Cohen Arazi 2020-03-18 17:56:17 UTC
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>
Comment 20 Tomás Cohen Arazi 2020-03-18 17:56:30 UTC
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>
Comment 21 Tomás Cohen Arazi 2020-03-18 17:56:43 UTC
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>
Comment 22 Tomás Cohen Arazi 2020-03-18 17:57:04 UTC
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>
Comment 23 Tomás Cohen Arazi 2020-03-18 17:57:14 UTC
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>
Comment 24 Tomás Cohen Arazi 2020-03-18 17:57:21 UTC
Created attachment 100977 [details] [review]
Bug 24663: (follow-up) Remove authnotrequired if set to 0

2 newly added scripts
Comment 25 Tomás Cohen Arazi 2020-03-18 18:23:13 UTC
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>
Comment 26 Tomás Cohen Arazi 2020-03-18 18:23:21 UTC
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>
Comment 27 Tomás Cohen Arazi 2020-03-18 18:23:29 UTC
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>
Comment 28 Joy Nelson 2020-04-17 19:07:58 UTC
does not apply to 19.11.x branch.  please rebase
Comment 29 Jonathan Druart 2020-06-03 11:14:43 UTC
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>
Comment 30 Jonathan Druart 2020-06-03 11:15:06 UTC
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>
Comment 31 Jonathan Druart 2020-06-03 11:15:09 UTC
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>
Comment 32 Jonathan Druart 2020-06-03 11:15:13 UTC
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>
Comment 33 Jonathan Druart 2020-06-03 11:15:19 UTC
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>
Comment 34 Jonathan Druart 2020-06-03 11:19:09 UTC
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>
Comment 35 Jonathan Druart 2020-06-03 11:19:36 UTC
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>
Comment 36 Jonathan Druart 2020-06-03 11:19:40 UTC
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>
Comment 37 Jonathan Druart 2020-06-03 11:19:43 UTC
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>
Comment 38 Jonathan Druart 2020-06-03 11:19:47 UTC
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>
Comment 39 Jonathan Druart 2020-06-03 11:22:54 UTC
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>
Comment 40 Jonathan Druart 2020-06-03 11:23:21 UTC
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>
Comment 41 Jonathan Druart 2020-06-03 11:23:25 UTC
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>
Comment 42 Jonathan Druart 2020-06-03 11:23:29 UTC
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>
Comment 43 Aleisha Amohia 2020-08-18 00:24:38 UTC
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,
Comment 44 Jonathan Druart 2020-08-18 08:17:25 UTC
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!)
Comment 45 Victor Grousset/tuxayo 2020-08-24 16:02:04 UTC
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
Comment 46 Aleisha Amohia 2020-08-26 04:39:59 UTC
backported to 19.11.x for 19.11.09
Comment 47 Lucas Gass 2020-08-31 21:17:43 UTC
backported to 20.05.x for 20.05.03
Comment 48 Jonathan Druart 2020-09-03 08:43:11 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 49 Alvaro Cornejo 2020-09-05 01:38:27 UTC
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?
Comment 50 Jonathan Druart 2020-09-07 08:11:06 UTC
(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?
Comment 51 Alvaro Cornejo 2020-09-07 16:47:43 UTC
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,
Comment 52 Victor Grousset/tuxayo 2020-09-07 21:10:41 UTC
Backported to 19.05.x branch for 19.05.14 that was released last week.
Comment 53 Jonathan Druart 2020-09-09 12:46:28 UTC
(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,
Comment 54 Jonathan Druart 2020-09-09 12:48:14 UTC
Created attachment 109793 [details] [review]
Bug 24663: Fix one missing occurrence

pushed in the meanwhile
Comment 55 Jonathan Druart 2020-09-09 13:42:41 UTC
(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)