On Mon, Jun 27, 2022 at 02:10:06PM +0000, John Rouillard wrote:
> 
> John Rouillard added the comment:
> 
> Hi Marcus:
> 
> How well have you tested your changes to make sure that the lack of
> reinitialization can't cause security issues?

I don't think this will cause security implications. Only the instance
is cached, not the database. Note how the client calls instance.open
with a username in Client.opendb.
This means we do not reload all reactors and actions for each request.
But we *do* open the database with a new user in each request.

> It looks like a new client is created on every connect which I think
> should guarantee that database access by a client is only done with
> proper access restrictions but the comment in
> instance.open that says:
> 
>         # load the database schema
>         # we cannot skip this part even if self.optimize is set
>         # because the schema has security settings that must be
>         # applied to each database instance
> 
> could a lack of a call to instance.open result in an incorrectly
> applied schema being used?

See above: instance.open seems to be called for each request. Of course
we should test this, so technically the patch *is* lacking a test that
makes sure that for each request an instance.open is called. I'm
currently not sure how to test the wsgi in a test, though.

> Ralf ideas/concerns about applying this patch without tests less than
> three weeks before the 2.2.0 release?

I didn't apply that particular patch at the time because I didn't fully
understand it (and the i18n stuff was unrelated and worked without the
patch). But I'm fairly sure it will not cause major breakage. But we can
put it in the next release, I'm running with intermediate versions all
the time. Then I'll probably have experience with a big deployment for
the next release :-)

> Ideas on how to test this? We do have test_live_server
> but I am not sure how do test any side effects from this patch.
> 
> Also what is the effect of:
> 
>    with self.get_tracker() as tracker:
> 
> as a context handler in the original code? Does it do any
> close/cleanup/release of resources?

I think that was the original intention.

> It looks like preload() just calls get_tracker(). Is it's role simply
> to prime the pump and preload/precompile the tracker before the call
> to:
> 
>    with self.get_tracker() as tracker:
> 
> so that the cost isn't paid when the first connection is made?

This might have been the intention but I don't think this has ever
worked (for wsgi!) because the instance is re-created on every request.
Which means the template instantiation is *not* retained for the next
request independent of the setting of 'optimize' in the instance.

You might also want to compare Marcus' change to the wsgi with the
implementation of roundup_server in scripts/roundup_server.py: This
*does also* cache the instance, see how the get_tracker method of
RoundupRequestHandler caches and re-uses the instance in the TRACKERS
(line 200). Its a little complicated how TRACKERS is only initialized in
a sub-class but looks like the same functionality to me.

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com