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

[ticket/9687] Refactoring banning #5393

Closed
wants to merge 11 commits into from

Conversation

Elsensee
Copy link
Contributor

@Elsensee Elsensee commented Oct 1, 2018

Okay, this is my third and final try on doing this. I think taking a step back (even though, involuntarily) was a good thing, because this time I didn't really look at the old code except after I'm done with a code part to see if I forgot something that the old code was watching out for.
Even though, I'm further into finishing it, this is still a WIP, a few things are still not done, like:

  • Remove the usage of the old - now deprecated - functions user_ban() and user_unban().
  • Adjust the ACP and MCP modules
  • Remove old language variables and add new ones (e.g. for exceptions, that's why they're still TODO)
  • Fix remaining bugs and quirks

Also, I want to hear some comments from you about a few decisions I made.

  • I removed the IP banning feature. To do it the right way is hard, it's a source of bugs and not very effective. If administrators want to ban by IP they should not rely on board functionality but on other programs. IP banning doesn't make sense as it doesn't really reduce load on the board (which is probably on of the things you'd like to achieve). Also, I believe, I heard from a few support team members that it's a PITA to support this? Maybe I remember this wrong, though. (However, extensions can be created to handle IP banning. The system I wrote is extensible and can be hooked into with very few effort)
  • I removed the excluding feature. I didn't see a reason to keep it. It would make banning more complicated from the administrator's point of view. Also, if you have to exclude something, you're banning to much. Banning shouldn't be the default, not banning should be.

Discuss! Also, you can start reviewing, the more stuff you catch now, the less stuff you catch later. Docblocks are in place (to my knowledge). You don't have to comment on the stuff that's still on my todo list (see above) but you can, I won't be mad at you.

Checklist:

  • Correct branch: master for new features; 3.2.x for fixes
  • Tests pass
  • Code follows coding guidelines: master and 3.2.x
  • Commit follows commit message format

Tracker ticket:
https://tracker.phpbb.com/browse/PHPBB3-9616

@Elsensee Elsensee added this to the 3.3.0-a1 milestone Oct 1, 2018
@Elsensee Elsensee mentioned this pull request Oct 1, 2018
12 tasks
@Elsensee Elsensee changed the title [ticket/9687] Refactoring banning [ticket/9616] Refactoring banning Oct 1, 2018
@Elsensee Elsensee changed the title [ticket/9616] Refactoring banning [ticket/9687] Refactoring banning Oct 1, 2018
@Elsensee Elsensee force-pushed the ticket/9687-new branch 2 times, most recently from bcaee68 to 35b2b65 Compare October 1, 2018 22:59
@Louis7777
Copy link
Contributor

* I removed the IP banning feature. To do it the right way is hard, it's a source of bugs and not very effective.

Please don't remove features that have been there like forever, and that don't seem to be deprecated or replaced by any better ways (unless I am missing something). IP banning may not be the most effective way to protect a forum against persistent unwanted people, but it may discourage a few people who are not the persistent or knowledgeable kind. Plus, one can use IP range bans (e.g. to ban some foreign ISPs) if everything else fails - still won't keep away the really persistent people but it will keep away more than previously.

Unless these features that you are planning to remove become part of an official extension, I will personally be on the annoyed users' side. Imo, even if it's not well-made and a source of bugs, I prefer having this feature than... not having it at all.

@Elsensee
Copy link
Contributor Author

Elsensee commented Oct 2, 2018

Please don't remove features that have been there like forever, and that don't seem to be deprecated or replaced by any better ways (unless I am missing something).

IP banning is kind of deprecated.. there's a better way: banning via .htaccess.. however, there are better ways to achieve whatever you're trying to achieve without IP banning, as stated by members of the phpBB support team in these posts:
https://www.phpbb.com/community/viewtopic.php?p=15096191#p15096191
https://www.phpbb.com/community/viewtopic.php?p=15028986#p15028986
https://www.phpbb.com/community/viewtopic.php?p=14389756#p14389756

Imo, even if it's not well-made and a source of bugs, I prefer having this feature than... not having it at all.

Isn't it more annoying to have a feature that's full of bugs and issues than not having this specific feature at all?

@Louis7777
Copy link
Contributor

IP banning is kind of deprecated.. there's a better way: banning via .htaccess.. however, there are better ways to achieve whatever you're trying to achieve without IP banning, as stated by members of the phpBB support team in these posts:
https://www.phpbb.com/community/viewtopic.php?p=15096191#p15096191
https://www.phpbb.com/community/viewtopic.php?p=15028986#p15028986
https://www.phpbb.com/community/viewtopic.php?p=14389756#p14389756

What exactly are you going to ban via an .htaccess? IP addresses? Because if that's the case, banning via a control panel versus adding lines to an .htaccess file, feels superior and is much more user friendly.

Also, I don't see any better ways stated by the support team in the posts that you referenced. One says " If you have spam issues, post back and we can help you". Is that better? No, it's not. The next one says "Banning ip addresses is a waste of time. Use a good Q and A". Is that better? Against bots, it is. Against human spammers and unwanted people, it is not. The last post is about bots as well. What about humans? What's the better way that made IP banning deprecated (in conjunction with e-mail and account banning)?

The support team has some fair points, but I don't see them outlining any superior methods. IP banning may not keep away a persistent person (e.g. one who is not tired of resetting his dynamic IP or using VPNs and proxies), but it will keep away a lot of less persistent and less knowledgeable people. I'd rather have that weapon - which has existed like forever- in my arsenal than... not having it.

Isn't it more annoying to have a feature that's full of bugs and issues than not having this specific feature at all?

As long as it doesn't harm my board / website, then it's fine even if it doesn't work perfectly. Not annoying whatsoever, although I'd wish it could be perfect with wildcard IP range bans and everything working as intended.

@Elsensee Elsensee force-pushed the ticket/9687-new branch 5 times, most recently from b75781b to e41a1ec Compare October 2, 2018 18:29
@Elsensee
Copy link
Contributor Author

What exactly are you going to ban via an .htaccess? IP addresses? Because if that's the case, banning via a control panel versus adding lines to an .htaccess file, feels superior and is much more user friendly.

I'm sure there is a way to make phpBB be able to add lines to the .htaccess. Adding IP bans to the database doesn't do any good.
Things, that I'd expect from IP banning:

  • Less bot spam
  • Less user spam
  • Less DoS attack possiblites (through mass of packets or hack attempts)

What IP banning through phpBB gives me:

  • Less bot spam
  • almost the same user spam
  • More DoS attack possibilites (if the IP banlist is large, phpBB will spend a significant amount of time on checking the IP address)
  • False positives

One says " If you have spam issues, post back and we can help you". Is that better? No, it's not. The next one says "Banning ip addresses is a waste of time. Use a good Q and A". Is that better? Against bots, it is. Against human spammers and unwanted people, it is not. The last post is about bots as well. What about humans? What's the better way that made IP banning deprecated (in conjunction with e-mail and account banning)?

Well, IP banning is also not a good tool against human spammers, they're just more clever than bots.

I'd rather have that weapon - which has existed like forever- in my arsenal than... not having it.

The web evolves and even though, IP banning might have been a good idea to include into phpBB back then (because of more static IP addresses), now it's not a good tool anymore. It's as if you'd still have a 56k-connection. Back then, it was okay, but now...

@gooof
Copy link

gooof commented Oct 10, 2018

More DoS attack possibilites (if the IP banlist is large, phpBB will spend a significant amount of time on checking the IP address)

The currently slowness comes from the cache, its not the IP banning or the table.
Via .htaccess is a pretty bad idea to do all that.

@Elsensee
Copy link
Contributor Author

The currently slowness comes from the cache, its not the IP banning or the table.

Well, if there are tens of thousands of entries in the table, you can cache all you like, you still have to go through that list.

Via .htaccess is a pretty bad idea to do all that.

Why is that?

@gooof
Copy link

gooof commented Oct 10, 2018

You misread my text, the sql cache is the current problem. Let the database handle the banning not php and save the result for a few minutes to the sessions_table and the IP ban speed problems are gone.

@Elsensee
Copy link
Contributor Author

While this might work for IPv4 this is quickly getting out of control for IPv6. Especially when you want to consider CIDR range support, etc.

@Louis7777
Copy link
Contributor

Well, IP banning is also not a good tool against human spammers, they're just more clever than bots.

There are people who are not knowledgeable or persistent enough and IP banning is very good against them. I'll just say - to conclude - that the phpBB support team has not really suggested any superior methods against humans.

@gooof
Copy link

gooof commented Oct 11, 2018

But ipv6 banning may not be as hard as it looks. Example: https://stackoverflow.com/a/14362786
This could work great with a few tweaks.

@JimMH
Copy link
Contributor

JimMH commented Oct 11, 2018

Well, IP banning is also not a good tool against human spammers, they're just more clever than bots.

There are people who are not knowledgeable or persistent enough and IP banning is very good against them. I'll just say - to conclude - that the phpBB support team has not really suggested any superior methods against humans.

Honestly, my experience as a moderator on phpBB.com and having done lots of support in the last years, is that by IP banning (and especially banning IP ranges) usually gets more false positives than it really keeping somebody away that wants to come back. In a time with lots of dynamic IPs, IP banning has (in my opinion) been rendered completely obsolete.

@Louis7777
Copy link
Contributor

In a time with lots of dynamic IPs, IP banning has (in my opinion) been rendered completely obsolete.

There are people who are not knowledgeable or persistent enough. There are people who will not reset their connection to obtain a new IP (maybe they are in the middle of a download or a Skype video call?), if they are even aware of being able to do that. There are others who will not use VPN if you end up banning their entire ISP. Because they are bored, because they lack motive, because they don't want to waste more time. IP banning will discourage them, permanently or temporarily, and in conjunction with e-mail and account banning. As long as it can discourage people even for a while and as long as it has not been replaced by something else, something superior, it cannot be considered obsolete - let alone "completely obsolete".

I understand that it may be hard to make this feature work properly, however, let's not name long-standing features useless or obsolete just because programming life would be easier without them.

@JimMH
Copy link
Contributor

JimMH commented Oct 12, 2018

While it might indeed not be completely obsolete (a slight exaggeration), there come times when you'd like to improve a system where you might have to lose a few minor features just because it's impossible to keep supporting everything while re-factoring a system.

What I wanted to point out with my comment is that it's essentially a broken system that's easily by-passable (even though some users might indeed not take the effort) and therefore for me not much of a priority when looking at what should absolutely be kept and what could be improved.

@Elsensee
Copy link
Contributor Author

I understand that it may be hard to make this feature work properly, however, let's not name long-standing features useless or obsolete just because programming life would be easier without them.

We're not saying a feature is obsolete, just because it's hard to do. If that would be the case, we wouldn't do something like making a new style and that's gonna happen.. ;)

I'm just not considering doing it, because it's doing more harm than any good. Banning IP ranges will always generate false positives and banning single IPs wouldn't be effective against non-lazy spammers. I don't wanna do this just for the few spammers who are lazy or not as good with computers.

@EA117
Copy link
Contributor

EA117 commented Oct 7, 2019

Came here while checking to see whether we already had a bug that would improve the existing IP banning interface with CIDR notation support. I too happen to consider that a decision to "drop IP banning from phpBB" is simply making an already difficult story unnecessarily harder for the phpBB administrator, not withstanding that "there are also other ways to do it" and "banning just a single IP address is no longer effective in today's world."

But regardless of that, this seems like a discussion which needs to be surfaced out to Area51 rather than "buried" here. Improvement of the IP banning interface to support CIDR notation is unexpectedly "blocked" by a proposal to simply remove IP banning altogether; even though that's not what https://tracker.phpbb.com/browse/PHPBB3-9687 or http://area51.phpbb.com/phpBB/viewtopic.php?f=108&t=33210 had concluded or would say is currently pending.

@marc1706 marc1706 closed this Jul 29, 2023
@marc1706 marc1706 mentioned this pull request Jul 29, 2023
4 tasks
@marc1706
Copy link
Member

Dicussions should continue in #6518

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

Successfully merging this pull request may close these issues.

6 participants