Move plone protect to plone layout

[cihanandac]

This PR moves the plone.protect.confirm.pt template to plone.app.layout.viewlets

[mister-roboto]

@cihanandac thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

[petschki]

Suggestion: this template is not a viewlet. Its the confirmation browser view. So I would not move it to plone.app.layout.viewlets but to a new module plone.app.layout.views

[cihanandac]

Suggestion: this template is not a viewlet. Its the confirmation browser view. So I would not move it to plone.app.layout.viewlets but to a new module plone.app.layout.views

Thanks @petschki for the suggestion. I have made changes to fix this.

[ale-rt]

I am pretty sure the confirm view should stay in this package because we have code that mentions it:

plone.protect/plone/protect/auto.py

Lines 304 to 308 in 213f1c4

	resp.redirect(
	"{}/@@confirm-action?{}".format(
	self.site.absolute_url(), urlencode(data)
	)
	)

Let me think about that.

CC @plone/classicui-team

[mauritsvanrees]

As a test, I have overwritten the _check method in auto.py to always redirect to @@confirm-action:

    def _check(self):
        if "confirm_action" not in self.request.URL:
            print("Overriding _check to always redirect to confirm-action.")
            print(f"original_url = {self.request.URL}")
            resp = self.request.response
            data = self.request.form.copy()
            data["original_url"] = self.request.URL
            resp.redirect(
                "{}/@@confirm-action?{}".format(
                    self.site.absolute_url(), urlencode(data)
                )
            )
            return False

This is basically the same code as further on in this method, we just execute this always, to see what happens.

Now start Volto, go to localhost:3000, and you get redirected to http://localhost:3000/@@confirm-action. This page gives a 403 Forbidden exception. This is raised by this line. The reason is that the original_url is http://localhost:3000/@site or similar. So you get a traceback in the logs because the url is not in the portal:

Forbidden: http://localhost:3000/@@confirm-action
Traceback (innermost last):
  Module ZPublisher.WSGIPublisher, line 181, in transaction_pubevents
  Module ZPublisher.WSGIPublisher, line 390, in publish_module
  Module ZPublisher.WSGIPublisher, line 284, in publish
  Module ZPublisher.mapply, line 98, in mapply
  Module ZPublisher.WSGIPublisher, line 68, in call_object
  Module plone.protect.views, line 14, in __call__
zExceptions.Forbidden: url not in portal:

This might work better on production, as the url may be fine there. But even if I comment out the line that raises Forbidden, I get a 500 server error, though nothing is visible in the logs.

In other words: redirecting to the confirm-action view seems useless in Volto, as the page simply does not work.

@davisagli Can you confirm this conclusion?

Would it make sense to somehow disable plone.protect completely for REST API calls? More or less this already seems the intention, as a lot of plone.restapi services have this line:

alsoProvides(self.request, plone.protect.interfaces.IDisableCSRFProtection)

So maybe plone.protect can be treated as Classic UI only, although it probably needs to remain a dependency of Products.CMFPlone. Note that it also helps protect the ZMI.

[ale-rt]

An interesting thing would be raising a CSRFExceptionOrNameItLikeYouPrefer exception instead of redirecting to a view.
Then the confirm-action page would rather be the index.html view for that custom exception:

See: https://github.com/plone/Products.CMFPlone/blob/6e2b2f55fc251a0e68b5ba65c09de6be867a9d39/Products/CMFPlone/browser/configure.zcml#L250-L257

I have no idea if that could even work, but sounds really sane to me.
Products.CMFPlone will raise a normal error page,
Classic UI -> will show the "index.html" with the form to confirm the action.
Volto -> Whatever

[davisagli]

@mauritsvanrees You're right, it doesn't make sense to redirect to the confirmation in plone.restapi. But I also don't want to remove the check entirely; it's useful for finding unexpected write-on-reads. I would suggest not doing the redirect if the request only has application/json in the Accept header.

@ale-rt Using exception views is a nice idea, but the check happens in the transform chain and runs on both PubBeforeCommit and PubBeforeAbort events in the publisher. We can raise an exception during PubBeforeCommit and the publisher will look up the exception view, but if we raise an exception during PubBeforeAbort it is too late. https://github.com/zopefoundation/Zope/blob/master/src/ZPublisher/WSGIPublisher.py#L233

[ale-rt]

@ale-rt Using exception views is a nice idea, but the check happens in the transform chain and runs on both PubBeforeCommit and PubBeforeAbort events in the publisher. We can raise an exception during PubBeforeCommit and the publisher will look up the exception view, but if we raise an exception during PubBeforeAbort it is too late. https://github.com/zopefoundation/Zope/blob/master/src/ZPublisher/WSGIPublisher.py#L233

Ouch! Bummer :)

[jensens]

@mauritsvanrees So maybe plone.protect can be treated as Classic UI only, although it probably needs to remain a dependency of Products.CMFPlone. Note that it also helps protect the ZMI.

Devils advocate question: If this is the case, why then move the templates to plone.app.layout?

[mauritsvanrees]

Devils advocate question: If this is the case, why then move the templates to plone.app.layout?

Reading David's answer, it seems plone.protect is still useful for Volto/RestApi as it prevents write-on-read. The problem is then only that the error message you get (the confirmation page) does not get through to the Volto frontend. So for application/json requests, we could return a json error response. And then Volto should somehow handle this.

Since on the ClassicUI side only one page template is involved, it could be fine to keep that here. It is not a template that anyone is likely to want to customize.

[ale-rt]

IMO, the best solution is to keep a simple template for covering whoever uses this package without classic UI, and then have a template targeted for ClassicUI in the frontend.
I am waiting for the pt to land in plone.app.layout to port there some markup improvements from a customer project:

(hoping that a good designer will make it less fleshy 🤣 )

[ale-rt]

I would stick using this class, see also:

• Move plone protect to plone layout plone.app.layout#386 (comment)
• Move plone protect to plone layout plone.app.layout#386 (comment)

We can always move it later.

[ale-rt]

IMO these tests should stay

[ale-rt]

Removing the dependency from the main template is super good.

If I check of the view renders, I see something like this:

It would be cool to have something a bit better.
At least I would keep the title and the description that we had before.

Top it would be to match the style of the Zope views