Improvements to bans #574
Improvements to bans #574
Conversation
* Split ban table into multiple columns for different ban types * Add a new netmask table for doing CIDR calculations more easily in the database * Add a bunch of additional fields to ban table for future use * Change how bans are processed on request * Bump MariaDB version requirement to 10.2.1 due to use of CHECK constraints. This also allows us window functions and common table expressions.
* Allow setting multiple parameters at once on a ban * New template to describe ban targets * Fix duplicate ban checking * Fix IP range checking * Move "can unban" checks to BanHelper; ensures that a user can fully see a ban before unbanning it. Note that you still need the unban user right to remove bans. * Don't include ban target in log entries
* Add links to view specific ban from log
* New field on new ban form * New column on ban list * Actions: - "Defer" sends the request to a specific queue automatically - "Drop" drops the request automatically and silently to the user - "Block" stops the request from being submitted - "Do nothing" does, well, nothing.
Codecov Report
@@ Coverage Diff @@
## master #574 +/- ##
===========================================
- Coverage 5.90% 5.25% -0.65%
- Complexity 2247 2318 +71
===========================================
Files 173 173
Lines 11134 11471 +337
===========================================
- Hits 657 603 -54
- Misses 10477 10868 +391
Continue to review full report at Codecov.
|
This reverts commit edfbb72
|
I've updated the database instances on wmflabs to MariaDB 10.3 in preparation for this. |
Conflicts: includes/Pages/PageEditComment.php
|
I've also just cloned the RC environment to https://accounts-dev.wmflabs.org/bans/internal.php if you want to test this without running your own test environment |
|
|
||
| return $errorList; | ||
| $bans = $this->banHelper->getBans($request); |
There was a problem hiding this comment.
Just wishing to verify that this goes through only the active bans without dumpster diving the code. Resolve if it does. Actually i'm not sure what this code is doing. Could I get a clarify here?
There was a problem hiding this comment.
BanHelper gets a list of bans which apply to supplied Request object.
The getBans(Request) call is a caching wrapper around getBansForRequestFromDatabase(Request), which in turn runs a database query to find any matching bans. For most attributes, such as names, this is a simple coalesce(:name rlike name, true) - in other words, match the requested name to a regex stored in the ban table, and if the result is null, return true. IP ranges are a little more complex, as it involves a join to the new netmask table to check ranges, but it's essentially doing the same thing. We also filter where active = 1 and duration isn't in the past. See L157 of BanHelper for the full SQL query.
We then take that list of applicable bans, and loop through them, deciding what to do based on the type of ban. If it's a "do nothing ban", we do nothing. If it's a "defer" ban, we defer it to the relevant queue. If it's a drop ban, we do the relevant drop logic with relevant log entries so it is traceable that a request was created and silently dropped according to the user. Note that "block"-type bans are handled as a pre-save validation, not as a post-save validation, so we don't need to worry about them at this stage.
There was a problem hiding this comment.
- Is templates/bans/main.tpl supposed to mirror almost showbans.tpl?
- Functionality of comment saving on requests is counter intuitive giving the potential need to edit comments. You must select the special save option after typing your comment.
- Bans that drop or defer should be limited to /20s and /48s. I know I originally said /16s, but big ranges should not be automatically banned as we are a tool of last resort for making an account. I don't see where a drop or ban would be beneficial beyond this size.
TODO: Add 2 issues:
- One requesting Ban method to be modifiable. (Priority: Low)
- A modification interface for the rest (Priority: Wishlist)
Codecov Report
@@ Coverage Diff @@
## master #574 +/- ##
===========================================
- Coverage 5.83% 5.22% -0.61%
- Complexity 2253 2346 +93
===========================================
Files 173 173
Lines 11257 11534 +277
===========================================
- Hits 657 603 -54
- Misses 10600 10931 +331
Continue to review full report at Codecov.
|
Hrm. I guess it was split out because it is different, but the differences can be handled with a boolean variable, so I've merged them.
Yeah, I'm not entirely sure on the UI to use here. My theory was that restricted-access comments are so rarely used that it's probably not too disruptive to have a slightly harder-to-use system than I'd ideally like. For now, I'm going to leave this as an open point and have a think about it.
Drop or defer? I presume you mean drop or block? I'm quite tempted to still permit huge ranges, but hidden behind a tool-root-only flag on the off-chance that it is needed. It'll be easier to find a tool root to apply a relevant ban than it would be to make a config change to allow larger ranges temporarily. I'm also happy to remove this flag from the tool root group at some point in the future, but I'm not entirely comfortable with removing it from the code entirely again. I'm also applying a limit for defer/nothing actions; both of these are configurable in case we want to change the limits around at some point.
I presume these are TODOs for you, or are you wanting me to raise the relevant issues for later consideration? I've also found/fixed a bug with the "Ban Name" button on the view request page... 🤦 |
* Add limits to IP range bans * Add tool root user right to bypass IP range limits * Simplify duplicated request lists * Remove unused template * Fix bug with name bans when based on a request
Redesign visibility into a dropdown of radios
|
OK, I've redesigned the comment save thing into something which is hopefully more intuitive. By default, this is what you see:
Clicking the padlock gives you this menu, from which you can choose a relevant protection type:
Choosing an option other than the default "locks" the padlock, and changes the button to a different colour to indicate an option has been chosen. The chosen colour matches the hue used as a highlight for the protected rows.
Does this feel like a better design to you? |
|
|
stwalkerster
added
the
actually quite difficult
and again. |
|
#641 and #642 have been created from my to do.
If I remember our conversation last time, I do mean drop and block. I'm fine with them being hid behind the root flag. |
On the back-end:
bantable has grown substantially wider; each ban how has a column for each type, as well as new columns for the new functionalitynetmasktable was added, containing CIDR prefix to subnet mask conversions, for use in address-in-range queries. This table will never be changed, and is intended to be entirely read-only.