-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add marker interface to disable automatic field insertion in forms #10
base: master
Are you sure you want to change the base?
Conversation
@mauritsvanrees please review also this one ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some remarks:
-
There is a conflict in the requirements files because you made similar changes in your other PR that I have merged today. Also, I have now created PR Restructure the requirements and buildout files. #11 which touches the requirements files. Could you check and merge that, and than merge master in here or do a rebase?
-
It would be nice if this only affected one form, and still keep the protection on the other forms, but that seems hard to do. It is now similar to
IDisableCSRFProtection
fromplone.protect
: this also disables csrf protection on the entire request. So let's say restricting it to a single form would be a nice future addition. For now, could you make this clearer in the changelog entry and the readme? -
Wait a minute, what about the optional
EXTRA_PROTECTED_ACTIONS
? I think those would be broken by this change. From the readme: "For these form actions the honeypot field is required: the field must be in the posted request, though it of course still must be empty." When honeypot is disabled on request A, then the honeypot field will not be added. This means that a POST in request B to one of the extra protected actions, will fail. I suppose this option is not used much, so it might be okay to just add this as extra warning in the readme and changelog. -
I think your check for
IHoneypotDisabledForm
also needs to be added to our override for the_authenticator
view. Wait, this is not actually used anymore? No zcml refers to this. Okay, this was done during the 2.0 refactoring in PR plone 52 / python 3 compatibility #4. I wonder if we should restore this. But never mind for now.
What I should have asked earlier: can you explain your use case? :-)
@mamico Is this good to be merged? Also in respect to @mauritsvanrees review? |
I am double-checking with @cekk. But as far as I know, for us the use case was that some forms were strictly with data, and an unintended field (the honeypot field without data) was a problem. I think we also have many others way to solve the problem. @thet do you have a use case where this PR is useful? |
@mamico No, I don't have a use case yet. I can only imagine that this feature would be useful. |
@cekk @thet @mauritsvanrees probably the simplest approach would be to add here https://github.com/collective/collective.honeypot/blob/master/collective/honeypot/auto.py#L91 a check on Same result, no new environment, no change to the use case. What are your thoughts on this? |
No description provided.