Since Drupal has a nice API to provide some brute force protection, I think we should use it for increased security in this module.

CommentFileSizeAuthor
#4 protected_node-flood_control-2548229-4.patch4.01 KBdigitgopher
#3 protected_node-flood_control-2548229-3.patch3.04 KBgrimreaper
#2 protected_node-flood_control-2548229-1.patch5.27 KBdigitgopher

Comments

digitgopher created an issue. See original summary.

digitgopher’s picture

digitgopher commented
Status: Active » Needs review
StatusFileSize
new protected_node-flood_control-2548229-1.patch5.27 KB

I'm attaching a patch to get things started here.

I propose that Protected node should have its own flood event name, but we should use the user_failed_login_ip_window and user_failed_login_ip_limit variables for integration with site wide configs.

Shouldn't need to clear the events since access is saved in a session.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Needs review » Needs work
StatusFileSize
new protected_node-flood_control-2548229-3.patch3.04 KB

Hello digitgopher,

Very good idea, thank you.

I have reviewed and tested your patch. It works but before I merge it I think there is still some work to be done.

  • I have improved the documentation with @see so you can see quickly the function with your IDE: see the attached patch.
  • I have used variables to avoid double call to variable_get for flood_is_allowed and flood_register_event. And also to make it clearer: see the attached patch.
  • I don't know why, but I was unable to apply your patch with git apply patch_name. Did you use the command git format-patch --stdout origin/7.x-1.x > patch_name to make your patch?
  • Minor coding standards error: if( => if ( and a double space somewhere maybe due to copy/paste. see the attached patch
  • I think that I will be nice to have a fieldset "Flood control" in the protected node admin page to set the limit. And even if as default value we use the same as user_failed_login_ip_window and user_failed_login_ip_limit, I think they must be different.
  • To avoid behavior issue on exisitng sites, I wonder if a checkbox "Add flood control" should be made in the config page and so a hook_update_n.
  • About the cleaning if the password is right. Currently maybe not. Later if people need it.
  • (optionnal) Please provide a test for this feature.

And again thank you for your effort.

digitgopher’s picture

digitgopher commented
Status: Needs work » Needs review
StatusFileSize
new protected_node-flood_control-2548229-4.patch4.01 KB

Thank you Grimreaper.

I added a Flood control configuration fieldset. I looked at the Flood Control module for inspiration for the interface.

I don't think this feature should be opt-in at this point, for a number of reasons - 1) this is a security issue, 2) there is the option to disable it by setting the window to zero, and 3) I don't see how this feature would cause issues on existing sites. If you think this is a problem, or there are errors in my thinking, please let me know. And explain a little more because I don't know why we would need a hook_update_n.

As an aside, why does the testbot sometimes not want to run?

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Needs review » Fixed

Thanks, this is merged now.

As you can disable it by putting the variable to 0. Ok no problem. I don't know which possible side effect.

About the testbot, I don't know. Maybe, it is configured to prioritize D8 tests and when there are events with sprints for D8, there is no more resources for D7 tests. It is purely a supposition.

Status: Fixed » Needs work

The last submitted patch, 4: protected_node-flood_control-2548229-4.patch, failed testing.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Needs work » Fixed

Ah, it seems that I was not the only one not able to apply your patch with git apply.

Anyway, this issue is fixed.

digitgopher’s picture

digitgopher commented

Documentation updated. https://www.drupal.org/node/2176645.

Strange, I thought 1.patch was bad and 4.patch was good. Sorry about that anyway.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Thanks for the documentation update.

I was wondering, maybe it is not very valuable to maintain it and maybe a documentation directly in the UI (#description for forms, help texts) is better. That's just an idea.

Do you make your patch using the command line?

digitgopher’s picture

digitgopher commented

You're right, ideally the code is self-documenting, and we can keep the documentation pages as lean as possible. But right now it is nice how thorough the documentation pages are.

Yes, I do 'git diff 7.x-1.x > patch_name'. For the first patch here though I was on a different machine from my normal one and didn't check my git configurations first.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Ok, at the begging I also use "git diff....". But the problem I realized with that is for very long issues. Long in time. Patches are no more appliable if other issues are merged. And doing that it is impossible to reroll the patch (I think, that's not something I do frequently or already done once)

If you commit and then create the patch from this commit. Git add the information of the previous patch hash so you can reroll using this information.

So my workflow is :
- create a new branch to test a patch or to made a new one : git checkout -b "issue_number" --track origin/7.x-1.x
- doing the code changes
- git add etc..
- git commit
- git format-patch --stdout origin/7.x-1.x > patch_name

digitgopher’s picture

digitgopher commented

Thank you that is great to know. I will try it.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.