Bug 31042 - OVERRIDE_SYSPREF does not work for REST API
Summary: OVERRIDE_SYSPREF does not work for REST API
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Bugs List
QA Contact:
URL:
Keywords:
Depends on: 16520
Blocks:
  Show dependency treegraph
 
Reported: 2022-06-24 14:09 UTC by Jonathan Druart
Modified: 2023-09-21 02:33 UTC (History)
5 users (show)

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


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2022-06-24 14:09:53 UTC
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.
Comment 1 Jonathan Druart 2022-06-24 14:11:24 UTC
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?
Comment 2 David Cook 2022-10-12 01:29:53 UTC
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
Comment 3 David Cook 2022-10-12 02:09:53 UTC
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...)

--
Comment 4 David Cook 2022-10-12 02:39:18 UTC
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.)

--
Comment 5 David Cook 2022-10-12 03:43:23 UTC
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.)
Comment 6 David Cook 2022-10-12 03:44:12 UTC
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...
Comment 7 David Cook 2023-09-18 07:16:22 UTC
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...
Comment 8 Jonathan Druart 2023-09-20 10:32:09 UTC
(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.
Comment 9 David Cook 2023-09-21 02:11:49 UTC
(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.
Comment 10 David Cook 2023-09-21 02:33:13 UTC
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.)