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.
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.
*** Bug 27555 has been marked as a duplicate of this bug. ***
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!
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>
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.
(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 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.
(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.
(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).
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>
Created attachment 133430 [details] [review] Bug 29744: Remove unnecessary condition in C4::Auth::safe_exit
I've signed off Martin's patch, added my own to fix the issue Frido found, and moved back to Needs Signoff.
(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.
(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.
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 :)
Also note that C4::Context routines should normally be OO (at least it was designed that way). Moving the test from 31468 here too
(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() ) {
This might be slightly confusing, but not touching it here: Koha/App/Plugin/CGIBinKoha.pm:sub _psgi_env { Kind of name clash..
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>
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.
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>
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>
Pushed to master for 22.11. Nice work everyone, thanks!
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>
(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?
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?
Enhancement will not be backported for 22.05.x