[Bug 31042] New: OVERRIDE_SYSPREF does not work for REST API
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31042 Bug ID: 31042 Summary: OVERRIDE_SYSPREF does not work for REST API Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: jonathan.druart+koha@gmail.com CC: tomascohen@gmail.com We are supposed to be able to override syspref's values using the OVERRIDE_SYSPREF trick, but it's not working for the REST API. To recreate, open /etc/koha/apache-shared-intranet-plack.conf and add RequestHeader add X-Koha-SetEnv "OVERRIDE_SYSPREF_Foo Bar" Add some warn, mainpage.pl and Koha/REST/V1/Patrons.pm (sub list) for instance warn C4::Context->preference('Foo'); Hit the mainpage, look at the log, you see "Bar". Search for patrons: "Use of uninitialized value in warn" => The syspref hasn't been overridden. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31042 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au, | |julian.maurice@biblibre.com | |, | |martin.renvoize@ptfs-europe | |.com, | |nick@bywatersolutions.com --- Comment #1 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- The only thing I see is that we are using Plack::App::CGIBin for the intranet and Mojo::Server::PSGI for the API. Could that be a lead? -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31042 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |16520 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16520 [Bug 16520] Per-virtualhost SetEnvs don't work with Plack -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31042 --- Comment #2 from David Cook <dcook@prosentient.com.au> --- That's very interesting. I thought maybe the problem was with RequestHeader/RewriteRule not mixing, but the issue exists for both http://localhost:8081/api/v1/patrons and http://localhost:8081/api/v1/app.pl/api/v1/patrons When I dump out the variables from Koha/Middleware/SetEnv.pm I see the data coming through for both the mainpage.pl and the API: 'HTTP_X_KOHA_SETENV' => 'OVERRIDE_SYSPREF_Foo Bar', When I dump %ENV for mainpage.pl, I see lots of HTTP_X headers including: 'HTTP_X_KOHA_SETENV' => 'OVERRIDE_SYSPREF_Foo Bar', When I dump %ENV for API, I don't see any HTTP_X headers... which look like they're just the environmental variables Starman started with... Yep... you need to access the Plack/PSGI-level environmental variables in Mojo::Server::PSGI using $c->req->env. Plack::App::CGIBin uses Plack::App::WrapCGI which resets the %ENV variables at this line: https://metacpan.org/dist/Plack/source/lib/Plack/App/WrapCGI.pm#L48 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31042 --- Comment #3 from David Cook <dcook@prosentient.com.au> --- So... that's interesting. Since %ENV is a global variable, anything we do to it will affect the whole life of the Starman worker process. It looks like we're setting $ENV{REMOTE_ADDR} in Koha/Middleware/RealIP.pm so we're already meddling with that global variable. Since REMOTE_ADDR gets set by every request, I suppose there's no real problem... I suppose we could update Koha/Middleware/SetEnv.pm or Koha/REST/V1.pm, to update %ENV... and folk would need to know they'd need to restart Starman if they change their RequestHeader directives in Apache... -- (As an aside, are we actually flushing L1 caches with the REST API? I don't see anything for it at a glance...) -- -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31042 --- Comment #4 from David Cook <dcook@prosentient.com.au> --- I'm looking at custom code right now but there's a little over 100 instances of $ENV in the code I'm looking at. For CGI-style .pl scripts, that should be fine. For Perl modules, it's more of a problem since they can be used by Mojolicious code and create bugs as a result. Like we're seeing now. -- I suppose one option would be to move $ENV calls to C4::Context->env(...) which could check %ENV and/or a $req_env variable which could be set per request (using various mechanisms). That seems like a bit of a hack though. The shortest path is just to pollute %ENV like we're doing with REMOTE_ADDR. -- Another idea for this very particular use case would be to create a cache in C4::Context and then use before_dispatch hook in Koha/REST/V1.pm to set that C4::Context cache from the $env variable. (That works while we're using synchronous requests, but if we used Mojo asynchronously then that would probably be vulnerable to a race condition.) -- -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31042 --- Comment #5 from David Cook <dcook@prosentient.com.au> --- Over time, we'll find global variables and singletons like C4::Context are going to cause more and more headaches. Yet, we've deeply embedded C4::Context->preference throughout all our code... For example, Koha::Patron->store() is riddled with syspref calls. Over time, it would be an idea to switch from C4::Context->preference() to $c->syspref() and to pass preferences through function/method calls. (That would help support multitenancy and non-blocking programming in the future too.) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31042 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- And I realized that just using %ENV could be a problem since the intranet, opac, and API all are served by the same workers. Unless care is taken to clean the %ENV each time then even the shortest path is fraught... -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31042 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=31908 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31042 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |In Discussion --- Comment #7 from David Cook <dcook@prosentient.com.au> --- You know what... I wonder if this is still a problem. I discovered a problem with Koha/Middleware/SetEnv.pm a while back and fixed it in bug 33967. See my comment at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23068#c54 That said, it might not be related... something to investigate... -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31042 --- Comment #8 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to David Cook from comment #7)
You know what... I wonder if this is still a problem.
Yes it is, I still get "Use of uninitialized value in warn at /kohadevbox/koha/Koha/REST/V1/Patrons.pm" when redoing comment 0. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31042 --- Comment #9 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #0)
We are supposed to be able to override syspref's values using the OVERRIDE_SYSPREF trick, but it's not working for the REST API.
To recreate, open /etc/koha/apache-shared-intranet-plack.conf and add RequestHeader add X-Koha-SetEnv "OVERRIDE_SYSPREF_Foo Bar"
Add some warn, mainpage.pl and Koha/REST/V1/Patrons.pm (sub list) for instance warn C4::Context->preference('Foo');
Hit the mainpage, look at the log, you see "Bar". Search for patrons: "Use of uninitialized value in warn" => The syspref hasn't been overridden.
tail -f /var/log/koha/kohadev/plack-intranet-error.log [2023/09/21 02:10:38] [WARN] Bar at /kohadevbox/koha/mainpage.pl line 42. tail -f /var/log/koha/kohadev/plack-api-error.log [2023/09/21 02:11:27] [WARN] Use of uninitialized value in warn at /kohadevbox/koha/Koha/REST/V1/Patrons.pm line 46. [2023/09/21 02:11:27] [WARN] Warning: something's wrong at /kohadevbox/koha/Koha/REST/V1/Patrons.pm line 46. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31042 --- Comment #10 from David Cook <dcook@prosentient.com.au> --- So options... 1. Like Koha/Middleware/RealIP.pm we could update Koha/Middleware/SetEnv.pm to work with the %ENV and not just the $env. That's probably the shortest path. 2. Add code to Koha/REST/V1.pm to update %ENV like https://metacpan.org/dist/Plack/source/lib/Plack/App/WrapCGI.pm#L48 or another data structure that could be used by C4::Context->preference This is probably more logical and not too much longer -- But really this shows how problematic our use of C4::Context->preference really is. (It's also why in other bugs I've said that I don't think it's a great idea for us to embed C4::Context as deeply in Koha modules as we do.) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31042 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |magnus@libriotech.no --- Comment #11 from Magnus Enger <magnus@libriotech.no> --- Ran into this in the wild recently. I did a quick fix (for the OPAC) by changing /usr/share/koha/lib/Koha/REST/V1/OAuth/Client.pm from this: 58 if ( $interface eq 'opac' ) { 62 $redirect_url = C4::Context->preference('OPACBaseURL') . '/api/v1/public/oauth/login/'; 64 if ( C4::Context->preference('OpacPublic') ) { 65 $uri = '/cgi-bin/koha/opac-user.pl'; 66 } else { 67 $uri = '/cgi-bin/koha/opac-main.pl'; 68 } 69 } else { 70 $redirect_url = C4::Context->preference('staffClientBaseURL') . '/api/v1/oauth/login/'; 71 $uri = '/cgi-bin/koha/mainpage.pl'; 72 } to this: 58 if ( $interface eq 'opac' ) { 59 if ( defined $c->tx->req->env->{ 'OVERRIDE_SYSPREF_OPACBaseURL' } && $c->tx->req->env->{ 'OVERRIDE_SYSPREF_OPACBaseURL' } ne '' ) { 60 $redirect_url = $c->tx->req->env->{ 'OVERRIDE_SYSPREF_OPACBaseURL' } . '/api/v1/public/oauth/login/'; 61 } else { 62 $redirect_url = C4::Context->preference('OPACBaseURL') . '/api/v1/public/oauth/login/'; 63 } 64 if ( C4::Context->preference('OpacPublic') ) { 65 $uri = '/cgi-bin/koha/opac-user.pl'; 66 } else { 67 $uri = '/cgi-bin/koha/opac-main.pl'; 68 } 69 } else { 70 $redirect_url = C4::Context->preference('staffClientBaseURL') . '/api/v1/oauth/login/'; 71 $uri = '/cgi-bin/koha/mainpage.pl'; 72 } It works, but a solution that goes more to the root of the problem would be good. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org