The userenv is not clear at the beginning of a new request. If C4::Context->userenv is called before set_userenv we should get undef, not the userenv from the previous request!
I was scratching my head with bug 34755 and then I realized that this is the reason why. If you're doing an OpenID Connect login, and someone else logs into Koha and navigates enough to change the userenv in the worker that you were using before being redirected to the OIDC IdP, you'll get a wrong_csrf_token. Yiiiiiiiikes.
Created attachment 163039 [details] [review] Bug 36149: Unset userenv from middleware The userenv (logged in user's info) are stored in $C4::Context->context->{activeuser}, which persists in plack worker's memory. It's really bad in theory as we are not cleaning it before or after the HTTP request, but only when set_userenv is called (what we are doing commonly in C4::Auth::get_template_and_user). If C4::Context->userenv is called before set_userenv we should get undef, not the userenv from the previous request! In practice this should not be a problem, but well... who really knows? This patch suggests to have a middleware to deal with removing the userenv at the beginning of each request (maybe it should be after, right? - FIXME).
Created attachment 163040 [details] [review] Bug 36149: Remove _new_userenv TODO - better review C4::Auth's changes. Are all the removal of _new_userenv correct/enough?
Created attachment 163041 [details] [review] Bug 36149: Remove context stack We are not using it and it's confusing, let's remove the context stack.
Created attachment 163042 [details] [review] Bug 36149: Remove dbh_stack Same pattern, remove dbh stack
Created attachment 163043 [details] [review] Bug 36149: Remove schema_stack Same pattern in Koha::Database
This is going way too far. The first patch should be enough for this bug report and others moved to their own. For discussion, do I continue and get support? Any ideas how to correctly test the first patch?
(In reply to Jonathan Druart from comment #7) > This is going way too far. The first patch should be enough for this bug > report and others moved to their own. Yeah I think the other patches should be moved to different reports. > For discussion, do I continue and get support? I think we do need to fix this one for sure. > Any ideas how to correctly test the first patch? Since there is a change to C4::Context::_unset_userenv I think we'd want to make sure that the current usage of that function works correctly. Otherwise... it's a tough one. We run the unit tests, run some basic manual tests, try things that can do weird things with userenv like the self-checkout. We'd want to double-check SIP and other long-running processes to see if they use userenv... Overall, I suppose it rather break something than leak something, so worthwhile doing in any case. -- Another thought... what about having C4::Context->userenv use Koha::Cache::Memory::Lite instead of a global variable? That way we'd have our existing cache flushing take care of this for free. I suppose the downside of that would be for things like SIP which flush the cache per-request could have unexpected effects.
> Another thought... what about having C4::Context->userenv use > Koha::Cache::Memory::Lite instead of a global variable? I've first started with that. But it felt better to have a centralised place where we could add more to deal with the context/env per request.
Created attachment 163402 [details] [review] Bug 36149: Remove get_schema_cached
Created attachment 163403 [details] [review] Bug 36149: Remove flush_schema_cached
Created attachment 163404 [details] [review] Bug 36149: Unset userenv from middleware The userenv (logged in user's info) are stored in $C4::Context->context->{activeuser}, which persists in plack worker's memory. It's really bad in theory as we are not cleaning it before or after the HTTP request, but only when set_userenv is called (what we are doing commonly in C4::Auth::get_template_and_user). If C4::Context->userenv is called before set_userenv we should get undef, not the userenv from the previous request! In practice this should not be a problem, but well... who really knows? This patch suggests to have a middleware to deal with removing the userenv at the beginning of each request (maybe it should be after, right? - FIXME).
Created attachment 163406 [details] [review] Bug 36149: Remove _new_userenv TODO - better review C4::Auth's changes. Are all the removal of _new_userenv correct/enough?
Created attachment 163407 [details] [review] Bug 36149: Remove context stack We are not using it and it's confusing, let's remove the context stack.
Created attachment 163409 [details] [review] Bug 36149: Remove dbh_stack Same pattern, remove dbh stack
Created attachment 163412 [details] [review] Bug 36149: Remove schema_stack Same pattern in Koha::Database
Tests highlight some interesting changes here.. * unset_userenv sets the unserenve to an empty hashref now rather than deleteing/undefining it. (t/db_dependent/Auth.t line 429) * activeuser is part of the stack system.. we can probably just drop the tests around line 910 in t/db_dependent/Auth.t * More undef vs empty hashref tests around lines 936 and 943 * undef vs empty string in plugin registers (highlighted by line 57 in t/db_dependent/Koha/Template/Plugin/Registers.t) * Background jobs tests also failing for the same I'm generally happy with the approach here.. it simplifies the code a fair bit and I think that's a good thing. I'm happy to see us go the whole way here personally.. if you've got the appetite to continue Jonathan?
I still think we're trying to do too much in 1 report. I'd love to see just us clearing the userenv at the start (or end) of the HTTP request, and then backporting that as far back as we can, since that's a bug. Then the rest can be done as a separate refactoring report which doesn't need to be backported. -- On a side note, removing some of the stack stuff will cause conflicts for Bug 20630. That said, we'd need better mechanisms for managing context/userenv and persistent network connections (e.g. DB connection) anyway. Outside of a multitenancy scenario, I don't think we'd need the stack stuff at all though.
(In reply to Jonathan Druart from comment #2) > This patch suggests to have a middleware to deal with removing the > userenv at the beginning of each request (maybe it should be after, right? - > FIXME). I have a different app where I perform some ORM cleanup at the end of the HTTP request using a Plack middleware, so I'm tempted to do this userenv cleanup at the end of the HTTP request. But then doing it at the beginning of the request means we might be a bit more confident that we've reset the environment? -- I thought that we were flushing the caches at the end of the HTTP request, but looking at debian/templates/plack.psgi, it looks like we actually do that when CGI::new is called (which would be the beginning of most HTTP requests). At some point, we'll want to move the cache flushes to a Plack middleware as well.
(In reply to Martin Renvoize from comment #17) > I'm generally happy with the approach here.. it simplifies the code a fair > bit and I think that's a good thing. I'm happy to see us go the whole way > here personally.. if you've got the appetite to continue Jonathan? I will move those patches to a separate bug. (In reply to David Cook from comment #18) > On a side note, removing some of the stack stuff will cause conflicts for > Bug 20630. That said, we'd need better mechanisms for managing > context/userenv and persistent network connections (e.g. DB connection) > anyway. > > Outside of a multitenancy scenario, I don't think we'd need the stack stuff > at all though. Yes, must be implemented correctly if we need it back. (In reply to David Cook from comment #19) > (In reply to Jonathan Druart from comment #2) > > This patch suggests to have a middleware to deal with removing the > > userenv at the beginning of each request (maybe it should be after, right? - > > FIXME). > > I have a different app where I perform some ORM cleanup at the end of the > HTTP request using a Plack middleware, so I'm tempted to do this userenv > cleanup at the end of the HTTP request. > > But then doing it at the beginning of the request means we might be a bit > more confident that we've reset the environment? Yes, agreed, keeping like that then see later if it needs to be tweaked. > I thought that we were flushing the caches at the end of the HTTP request, > but looking at debian/templates/plack.psgi, it looks like we actually do > that when CGI::new is called (which would be the beginning of most HTTP > requests). > > At some point, we'll want to move the cache flushes to a Plack middleware as > well. Yes, what I meant in comment 9 ;)
Created attachment 163499 [details] [review] Bug 36149: Unset userenv from middleware The userenv (logged in user's info) are stored in $C4::Context->context->{activeuser}, which persists in plack worker's memory. It's really bad in theory as we are not cleaning it before or after the HTTP request, but only when set_userenv is called (what we are doing commonly in C4::Auth::get_template_and_user). If C4::Context->userenv is called before set_userenv we should get undef, not the userenv from the previous request! In practice this should not be a problem, but well... who really knows? This patch suggests to have a middleware to deal with removing the userenv at the beginning of each request (maybe it should be after, right? - FIXME).
(In reply to Jonathan Druart from comment #20) > (In reply to Martin Renvoize from comment #17) > > I'm generally happy with the approach here.. it simplifies the code a fair > > bit and I think that's a good thing. I'm happy to see us go the whole way > > here personally.. if you've got the appetite to continue Jonathan? > > I will move those patches to a separate bug. See bug 36367.
Created attachment 164145 [details] [review] Bug 36149: Unset userenv from middleware The userenv (logged in user's info) are stored in $C4::Context->context->{activeuser}, which persists in plack worker's memory. It's really bad in theory as we are not cleaning it before or after the HTTP request, but only when set_userenv is called (what we are doing commonly in C4::Auth::get_template_and_user). If C4::Context->userenv is called before set_userenv we should get undef, not the userenv from the previous request! In practice this should not be a problem, but well... who really knows? This patch suggests to have a middleware to deal with removing the userenv at the beginning of each request (maybe it should be after, right? - FIXME). To test: 1 - Edit /etc/koha/sites/kohadev/koha-conf.xml to set <plack_workers>1</plack_workers> 2 - Edit about.pl and add a line after: CGI->new: warn Data::Dumper::Dumper( C4::Cointext->userenv() ); 3 - tail -f /var/log/koha/kohadev/*.log 4 - View about.pl in staff interface, should get a "somethign's wrong" warning 5 - Reload, you get current user info 6 - Open an incognito tab, sign in as a different user and click some stuff 7 - Reload about.pl in other window 8 - You get the opac user info 9 - Apply patch 10 - Edit /etc/koha/sites/kohadev/plack.psgi and add the middleware after "RealIP": enable "+Koha::Middleware::UserEnv"; 11 - Restart all 12 - Reload about.pl - you get a "Something's wrong" warning 13 - Click things in opac on incognito window 14 - Reload about.pl - only "Something's wrong" - you no longer see any user info Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 164146 [details] [review] Bug 36149: (follow-up) POD and tidy
I faked it for testing, but this would have prevented bugs like bug 35518 It's an easy enough mistake to make (says the guy who made it) so this protection should be beneficial
Could this pattern help in post processing: sub call { my($self, $env) = @_; # pre-processing $env my $res = $self->app->($env); return Plack::Util::response_cb($res, sub { my $res = shift; # do something with $res; }); } Clear userenv in the callback ?
(In reply to Marcel de Rooy from comment #26) > Could this pattern help in post processing: > > sub call { > my($self, $env) = @_; > # pre-processing $env > my $res = $self->app->($env); > > return Plack::Util::response_cb($res, sub { > my $res = shift; > # do something with $res; > }); > } > > Clear userenv in the callback ? Yes, but do we really want it to be cleared after? If you do it before you are certain that you are processing the request with a clear env.
(In reply to Jonathan Druart from comment #27) > Yes, but do we really want it to be cleared after? If you do it before you > are certain that you are processing the request with a clear env. And what if we do both :)
Could be, if we don't trust ourselves. But IMO only before is needed.
(In reply to Jonathan Druart from comment #29) > Could be, if we don't trust ourselves. But IMO only before is needed. I agree. I think we only need before. Both is probably overkill.
I think that calling _unset_userenv is not enough. It only deletes $context->{activeuser} and not the corresponding data in $context->{userenv}, so $context->{userenv} continues to hold userenv of all previously logged in users. C4::Context doesn't seem to have any method to clean the userenv, so maybe _unset_userenv should be modified to do this ?
Created attachment 164555 [details] [review] Bug 36149: Add userenv middleware to app.psgi
Created attachment 164567 [details] [review] Bug 36149: Unset userenv from middleware The userenv (logged in user's info) are stored in $C4::Context->context->{activeuser}, which persists in plack worker's memory. It's really bad in theory as we are not cleaning it before or after the HTTP request, but only when set_userenv is called (what we are doing commonly in C4::Auth::get_template_and_user). If C4::Context->userenv is called before set_userenv we should get undef, not the userenv from the previous request! In practice this should not be a problem, but well... who really knows? This patch suggests to have a middleware to deal with removing the userenv at the beginning of each request (maybe it should be after, right? - FIXME). To test: 1 - Edit /etc/koha/sites/kohadev/koha-conf.xml to set <plack_workers>1</plack_workers> 2 - Edit about.pl and add a line after: CGI->new: warn Data::Dumper::Dumper( C4::Cointext->userenv() ); 3 - tail -f /var/log/koha/kohadev/*.log 4 - View about.pl in staff interface, should get a "somethign's wrong" warning 5 - Reload, you get current user info 6 - Open an incognito tab, sign in as a different user and click some stuff 7 - Reload about.pl in other window 8 - You get the opac user info 9 - Apply patch 10 - Edit /etc/koha/sites/kohadev/plack.psgi and add the middleware after "RealIP": enable "+Koha::Middleware::UserEnv"; 11 - Restart all 12 - Reload about.pl - you get a "Something's wrong" warning 13 - Click things in opac on incognito window 14 - Reload about.pl - only "Something's wrong" - you no longer see any user info Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 164568 [details] [review] Bug 36149: (follow-up) POD and tidy Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 164569 [details] [review] Bug 36149: Add userenv middleware to app.psgi Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to Julian Maurice from comment #31) > I think that calling _unset_userenv is not enough. It only deletes > $context->{activeuser} and not the corresponding data in > $context->{userenv}, so $context->{userenv} continues to hold userenv of all > previously logged in users. > C4::Context doesn't seem to have any method to clean the userenv, so maybe > _unset_userenv should be modified to do this ? We've gone PQA now here.. is Julian's comment not valid as I don't see any mebtion of it for change to the code in the subsequent QA process?
(In reply to Martin Renvoize from comment #36) > (In reply to Julian Maurice from comment #31) > > I think that calling _unset_userenv is not enough. It only deletes > > $context->{activeuser} and not the corresponding data in > > $context->{userenv}, so $context->{userenv} continues to hold userenv of all > > previously logged in users. > > C4::Context doesn't seem to have any method to clean the userenv, so maybe > > _unset_userenv should be modified to do this ? > > We've gone PQA now here.. is Julian's comment not valid as I don't see any > mebtion of it for change to the code in the subsequent QA process? Julian makes an interesting point, although I don't think it's a blocker here, and we can proceed with the PQA as is. In Bug 36367, Jonathan has a patch that undefs the whole of $context->{userenv} in C4::Context::unset_userenv, so I think that should suffice as a follow-up.
Yes, I first submitted a version with a full removal but I have been asked to provide a small bugfix here and move the cleaning part to a separate bug (see bug 36367). Please test and QA it :)
(In reply to David Cook from comment #37) > Julian makes an interesting point, although I don't think it's a blocker > here, and we can proceed with the PQA as is. Yes, not a blocker.
Applied to 22.05
Backported to 22.11
Pushed for 24.05! Well done everyone, thank you!