Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cekk
Copy link
Contributor

@cekk cekk commented Sep 23, 2022

No description provided.

@mamico mamico self-requested a review September 23, 2022 14:19
@cekk
Copy link
Contributor Author

cekk commented Nov 3, 2022

@mauritsvanrees please review also this one ;)

Copy link
Member

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some remarks:

  1. 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?

  2. 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 from plone.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?

  3. 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.

  4. 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? :-)

@thet
Copy link
Member

thet commented Nov 27, 2023

@mamico Is this good to be merged? Also in respect to @mauritsvanrees review?

@mamico
Copy link
Contributor

mamico commented Nov 27, 2023

@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?

@thet
Copy link
Member

thet commented Nov 27, 2023

@mamico No, I don't have a use case yet. I can only imagine that this feature would be useful.

@mamico
Copy link
Contributor

mamico commented Nov 27, 2023

@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 WHITELISTED_ACTIONS and WHITELISTED_START.

Same result, no new environment, no change to the use case. What are your thoughts on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants