plone / plone.protect

HTTP protection utilities for the Plone CMS
https://pypi.org/project/plone.protect/
7 stars 8 forks source link

plone.protect.authenticator.verify does not respect IDisableCSRFProtection #41

Closed thet closed 7 years ago

thet commented 8 years ago

When disabling CSRF protection in tests like so:

from plone.protect.authenticator import IDisableCSRFProtection
self.request = self.layer['request']
alsoProvides(self.request, IDisableCSRFProtection)

this check will raise unauthorized (like here: https://github.com/plone/plone.app.content/blob/master/plone/app/content/browser/contents/__init__.py#L76 ):

authenticator = getMultiAdapter((self.context, self.request), name='authenticator')
if not authenticator.verify():
    raise Unauthorized
vangheem commented 8 years ago

I guess IDisableCSRFProtection was really only meant for automatic CSRF protection. I hesitate adding the check int the authenticator.verify method at this point in case it affects other things that should require it--there might be cases where automatic protection is not needed but manual protection still is.

mauritsvanrees commented 8 years ago

Maybe it is better if code that manually checks the authenticator adds its own check for the interface? So like this in the case of plone.app.content:

def protect(self):
    if not IDisableCSRFProtection.providedBy(self.request):
        authenticator = getMultiAdapter((self.context, self.request),
                                        name='authenticator')
        if not authenticator.verify():
            raise Unauthorized
    checkpost(self.request)

Or perhaps when csrf is disabled but there is still an authenticator in the request, do the check anyway for good measure:

    if not IDisableCSRFProtection.providedBy(self.request) or '_authenticator' in self.request:
        ...

I think it is best not to add such a check in plone.protect at this point.