Bug 29744 - Harmonize psgi/plack detection methods
Summary: Harmonize psgi/plack detection methods
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
: 27555 (view as bug list)
Depends on: 29420
Blocks: 31468
  Show dependency treegraph
 
Reported: 2021-12-21 02:27 UTC by David Cook
Modified: 2022-09-27 13:02 UTC (History)
4 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:
22.11.00


Attachments
Bug 29744: Harmonize psgi/plack detection methods (4.87 KB, patch)
2022-03-28 09:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29744: Harmonize psgi/plack detection methods (4.95 KB, patch)
2022-04-05 13:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29744: Harmonize psgi/plack detection methods (4.99 KB, patch)
2022-04-20 05:28 UTC, David Cook
Details | Diff | Splinter Review
Bug 29744: Remove unnecessary condition in C4::Auth::safe_exit (769 bytes, patch)
2022-04-20 05:28 UTC, David Cook
Details | Diff | Splinter Review
Bug 29744: Harmonize psgi/plack detection methods (5.09 KB, patch)
2022-09-16 07:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29744: Remove unnecessary condition in C4::Auth::safe_exit (1.15 KB, patch)
2022-09-16 07:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29744: (QA follow-up) Add underscore test, move to OO (3.10 KB, patch)
2022-09-16 07:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29744: (QA follow-up) Call psgi_env in OO style (3.24 KB, patch)
2022-09-16 07:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29744: (QA follow-up) Only consider PLACK_ENV for underscore (3.55 KB, patch)
2022-09-23 15:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2021-12-21 02:27:28 UTC
In Bug 29420, C4::Context->is_psgi_or_plack() is introduced to check for psgi/plack environmental variables, since C4::Auth::psgi_env doesn't check for "plack" environmental variables which means psgi/plack detection fails. 

We should replace C4::Auth::psgi_env calls with C4::Context->is_psgi_or_plack() if possible. 

We should also use C4::Context->is_psgi_or_plack() instead of manually scanning the %ENV hash for psgi/plack variables.
Comment 1 Martin Renvoize 2022-03-28 09:26:16 UTC
Created attachment 132299 [details] [review]
Bug 29744: Harmonize psgi/plack detection methods

This patch updates and moves the existing psgi_env method out of Auth
and into Context and then replaces any manual references of the same
code to use the new method.
Comment 2 Martin Renvoize 2022-03-28 09:27:09 UTC
*** Bug 27555 has been marked as a duplicate of this bug. ***
Comment 3 Jonathan Druart 2022-04-05 13:35:09 UTC
Bug 27555 comment 3

"""
I don't understand why psgi_env and safe_exit were in the BEGIN block.
Should we expect a regression from CAS or Shib auth here?
"""

Will see once it's pushed!
Comment 4 Jonathan Druart 2022-04-05 13:35:33 UTC
Created attachment 132975 [details] [review]
Bug 29744: Harmonize psgi/plack detection methods

This patch updates and moves the existing psgi_env method out of Auth
and into Context and then replaces any manual references of the same
code to use the new method.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 5 Fridolin Somers 2022-04-13 13:40:51 UTC
I cant get this working.
With Bug 29420 and this one I get directly the error :

psgi:exit at /kohadevbox/koha/C4/Auth.pm line 59
       59:     if   (C4::Context::psgi_env) { die 'psgi:exit' }

I tried to remove manually the session cookie but same problem.
Comment 6 David Cook 2022-04-20 04:54:42 UTC
(In reply to Fridolin Somers from comment #5)
> I cant get this working.
> With Bug 29420 and this one I get directly the error :
> 
> psgi:exit at /kohadevbox/koha/C4/Auth.pm line 59
>        59:     if   (C4::Context::psgi_env) { die 'psgi:exit' }
> 
> I tried to remove manually the session cookie but same problem.

I just applied bug 29420 and bug 29744 to master, ran "koha-plack --restart kohadev", and navigated through both the OPAC and staff interface with no problems.

However, when I logged out of the Staff Interface, I encountered the same problem as Fridolin (as the logout code calls C4::Auth's safe_exit function).

I'll do a little bit of troubleshooting on this, but I think the answer is to not change the BEGIN block in this bug report, since it's really a secondary change that doesn't really relate to the majority of what this bug report is trying to accomplish...
Comment 7 David Cook 2022-04-20 05:16:12 UTC
Comment on attachment 132975 [details] [review]
Bug 29744: Harmonize psgi/plack detection methods

Review of attachment 132975 [details] [review]:
-----------------------------------------------------------------

::: C4/Auth.pm
@@ +56,3 @@
>  
> +sub safe_exit {
> +    if   (C4::Context::psgi_env) { die 'psgi:exit' }

Ok, so the problem here isn't the BEGIN block, but rather that the old psgi_env() checked only for "psgi." keys whereas the new C4::Context::psgi_env checks for "psgi." keys or "plack." keys.

This is significant because CGI::Emulate::PSGI (which runs our CGI scripts as PSGI functions) removes all "psgi." keys from the environment: https://metacpan.org/dist/CGI-Emulate-PSGI/source/lib/CGI/Emulate/PSGI.pm#L53

That's why to detect Plack/PSGI in Koha we have to look for those "plack." variables like plack.file.SCRIPT_NAME and plack.file.PATH_INFO, which are added by Plack::App::File (via Plack::App::CGIBin.

It's fine for us to "exit" because CGI::Compile (used in Plack::App::WrapCGI) redefines "exit" for us automatically.

Since we only seem to use C4::Auth::safe_exit in a CGI context, we don't actually need this PSGI detection at all here.
Comment 8 David Cook 2022-04-20 05:21:11 UTC
(In reply to David Cook from comment #7)
> Since we only seem to use C4::Auth::safe_exit in a CGI context, we don't
> actually need this PSGI detection at all here.

Tracked the origin of this back to 

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5630#c10

https://lists.koha-community.org/pipermail/koha-patches/2011-March/014376.html

It looks like antique code that probably hasn't done anything useful for many years.
Comment 9 David Cook 2022-04-20 05:23:26 UTC
(In reply to David Cook from comment #8)
> It looks like antique code that probably hasn't done anything useful for
> many years.

This is made all the more obvious by the usage of "exit" all over our CGI code and our very slim usage of "safe_exit" (limited only to C4::Auth).
Comment 10 David Cook 2022-04-20 05:28:44 UTC
Created attachment 133429 [details] [review]
Bug 29744: Harmonize psgi/plack detection methods

This patch updates and moves the existing psgi_env method out of Auth
and into Context and then replaces any manual references of the same
code to use the new method.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 11 David Cook 2022-04-20 05:28:48 UTC
Created attachment 133430 [details] [review]
Bug 29744: Remove unnecessary condition in C4::Auth::safe_exit
Comment 12 David Cook 2022-04-20 05:29:21 UTC
I've signed off Martin's patch, added my own to fix the issue Frido found, and moved back to Needs Signoff.
Comment 13 Marcel de Rooy 2022-09-16 06:49:19 UTC
(In reply to David Cook from comment #7)

> It's fine for us to "exit" because CGI::Compile (used in
> Plack::App::WrapCGI) redefines "exit" for us automatically.
> 
> Since we only seem to use C4::Auth::safe_exit in a CGI context, we don't
> actually need this PSGI detection at all here.

These comments should have been included in the commit message. I will adjust it.
Comment 14 Marcel de Rooy 2022-09-16 06:50:34 UTC
(In reply to David Cook from comment #12)
> I've signed off Martin's patch, added my own to fix the issue Frido found,
> and moved back to Needs Signoff.

I will consider this patch as Signed off and QA.
Comment 15 Marcel de Rooy 2022-09-16 07:02:42 UTC
Putting bug 31468 (from myself) in the picture here too:

When running API, we need to look for $ENV{PLACK_ENV}, but the
underscore is not included in the Context sub.

 sub is_psgi_or_plack {
     my $is_psgi_or_plack = 0;
-    if ( any { /(^psgi\.|^plack\.)/i } keys %ENV ) {
+    if ( any { /^(psgi|plack)[._]/i } keys %ENV ) {

And note, with unit tests :)
Comment 16 Marcel de Rooy 2022-09-16 07:06:36 UTC
Also note that C4::Context routines should normally be OO (at least it was designed that way).
Moving the test from 31468 here too
Comment 17 Marcel de Rooy 2022-09-16 07:14:18 UTC
(In reply to Marcel de Rooy from comment #16)
> Also note that C4::Context routines should normally be OO (at least it was
> designed that way).
> Moving the test from 31468 here too

errors/400.pl:if ( C4::Context->is_internal_PSGI_request() ) {
errors/401.pl:if ( C4::Context->is_internal_PSGI_request() ) {
errors/402.pl:if ( C4::Context->is_internal_PSGI_request() ) {
errors/403.pl:if ( C4::Context->is_internal_PSGI_request() ) {
errors/404.pl:if ( C4::Context->is_internal_PSGI_request() ) {
errors/500.pl:if ( C4::Context->is_internal_PSGI_request() ) {
opac/errors/400.pl:if ( C4::Context->is_internal_PSGI_request() ) {
opac/errors/401.pl:if ( C4::Context->is_internal_PSGI_request() ) {
opac/errors/402.pl:if ( C4::Context->is_internal_PSGI_request() ) {
opac/errors/403.pl:if ( C4::Context->is_internal_PSGI_request() ) {
opac/errors/404.pl:if ( C4::Context->is_internal_PSGI_request() ) {
opac/errors/500.pl:if ( C4::Context->is_internal_PSGI_request() ) {
Comment 18 Marcel de Rooy 2022-09-16 07:17:16 UTC
This might be slightly confusing, but not touching it here:

Koha/App/Plugin/CGIBinKoha.pm:sub _psgi_env {

Kind of name clash..
Comment 19 Marcel de Rooy 2022-09-16 07:53:57 UTC
Created attachment 140689 [details] [review]
Bug 29744: Harmonize psgi/plack detection methods

This patch updates and moves the existing psgi_env method out of Auth
and into Context and then replaces any manual references of the same
code to use the new method.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2022-09-16 07:54:01 UTC
Created attachment 140690 [details] [review]
Bug 29744: Remove unnecessary condition in C4::Auth::safe_exit

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Adding David's comments from Bugzilla to safe_exit here.
Comment 21 Marcel de Rooy 2022-09-16 07:54:06 UTC
Created attachment 140691 [details] [review]
Bug 29744: (QA follow-up) Add underscore test, move to OO

The underscore test comes from bug 31468.
Context methods should actually be OO.

Test plan:
Run t/Context.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2022-09-16 07:54:10 UTC
Created attachment 140692 [details] [review]
Bug 29744: (QA follow-up) Call psgi_env in OO style

Result of git grep -l -E "::psgi_env" | xargs sed -i -e's/::psgi_env/->psgi_env/g'

Also resolving this warn from Auth.t:
Use of uninitialized value $ENV{"SCRIPT_NAME"} in pattern match (m//) at /usr/share/koha/Koha/AuthUtils.pm line 211.

Note that the following warn is resolved on report 30588 (underway).
Use of uninitialized value $return in numeric gt (>) at /usr/share/koha/C4/Auth.pm line 1154.

Test plan:
Run t/db_dependent/Auth.t
Hit some opac, intranet pages. API call.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Tomás Cohen Arazi 2022-09-22 11:24:56 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 24 Tomás Cohen Arazi 2022-09-23 15:08:35 UTC
Created attachment 140922 [details] [review]
Bug 29744: (QA follow-up) Only consider PLACK_ENV for underscore

When run under certain circumstances (in jenkins, for example) some ENV
variables are set for convenience, like PLACK_WORKERS and
PLACK_MAX_REQUESTS and is causing some tests to fail (notably, shib
ones).

This patch makes the regex only consider PLACK_ENV when testing for the
underscore case.

Tests are updated accordingly, and they are also rewritten to test for
boolean values instead of empty string, or zero or one, etc. The
implementation shouldn't matter as long as the boolean evaluation is
correct and it is clearer for devs what to expect.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Marcel de Rooy 2022-09-23 15:36:43 UTC
(In reply to Tomás Cohen Arazi from comment #24)
> Created attachment 140922 [details] [review] [review]
> Bug 29744: (QA follow-up) Only consider PLACK_ENV for underscore
> 
> When run under certain circumstances (in jenkins, for example) some ENV
> variables are set for convenience, like PLACK_WORKERS and
> PLACK_MAX_REQUESTS and is causing some tests to fail (notably, shib
> ones).
> 
> This patch makes the regex only consider PLACK_ENV when testing for the
> underscore case.
> 
> Tests are updated accordingly, and they are also rewritten to test for
> boolean values instead of empty string, or zero or one, etc. The
> implementation shouldn't matter as long as the boolean evaluation is
> correct and it is clearer for devs what to expect.
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Thx for following up. I will still have a look next week. This might break something else again? ;)
As done in one of those tests, we may just need to localize $ENV in Shibboleth tests?
Comment 26 Marcel de Rooy 2022-09-27 13:02:21 UTC
Looks fine to me, Tomas. I needed PLACK_ENV at least..
As a side note, it looks to me that t/Auth_with_shibboleth makes a few assumptions about its environment that could have been baken into the test?