search

zopefoundation / Products.PluggableAuthService

Pluggable Zope authentication / authorization framework
Other
9 stars 18 forks source link

Make Products.Sessions dependency optional #72

Closed mauritsvanrees closed 4 years ago

mauritsvanrees commented 4 years ago

Or get rid of it entirely. It uses the deprecated tempstorage. PAS uses it in SessionAuthHelper and for CSRF protection of various plugins. In the context of Plone, the SessionAuthHelper is not used by default, and the CSRF protection is overridden by plone.protect. For details, see my comment.

I am not sure if the CSRF protection can and needs to be replaced with something else.

Easiest would be to make the dependency optional. Maybe I could work on this, but I have no urgent need, so no immediate plans.

As for a time frame when to do this, I would say around the release of Zope 5. But wait: Zope itself is not using it. Nor does CMF use it. Are others using this? Time frame may not matter much if Plone is the only user.

dataflake commented 4 years ago

There is a dependency here on Products.Sessions, that's correct, but no hard dependency on Products.TemporaryFolder, which would pull in tempstorage. The only requirement for Products.TemporaryFolder inside Products.Sessions is for the tests extra. It's purely optional and only for the unit tests.

mauritsvanrees commented 4 years ago

[Edited away duplicate text.]

Products.Sessions has code run at Zope startup which creates a session data manager. This object relies on something being available at /temp_folder/session_data. Traditionally, this something is a folder from Products.TemporaryFolder. Theoretically this could be something else, but I doubt anyone has created such a thing. I remember https://github.com/collective/Products.BeakerSessionDataManager, but that is an alternative to Products.Sessions, and not an alternative to the temp_folder.

So it is nice that Products.TemporaryStorage is not required, but that does mean that Products.Sessions is useless out of the box, without extra code to fill the gap in /temp_folder/session_data.

dataflake commented 4 years ago

You may not be aware, but there are other session implementations that act as a drop-in, such as the Memcache-based Products.mcdutils. We have used that successfully for a long time.

Even though the implementation is somewhat dubious, mostly because it is very old and that's how things were done then, a better solution would be to change Products.Sessions to not create these objects, or to make them not blow up if that path doesn't exist. That path happens to be the central "plugin point" for both the tempfolder and all alternative session implementations.

dataflake commented 4 years ago

P.S.: So I'm saying the dependency in Products.PluggableAuthService isn't so much the problem. Just the behavior of Products.Sessions is.

d-maurer commented 4 years ago

Maurits van Rees wrote at 2020-8-20 07:25 -0700:

.. I remember https://github.com/collective/Products.BeakerSessionDataManager, but that is an alternative to Products.Sessions, and not an alternative to the temp_folder.

A client has a normal mounted ClientStorage there. Looks like a good replacement for TemporaryStorage.

mauritsvanrees commented 4 years ago

Thanks for the thoughts and the hints to other session solutions.

P.S.: So I'm saying the dependency in Products.PluggableAuthService isn't so much the problem. Just the behavior of Products.Sessions is.

Fair enough. I will open an issue there.