[Proposal] Better ignore system

[dgw]

I noticed this weekend that .blocks add hostmask <regex> actually adds a host block, and doesn't work as expected if given an actual hostmask, e.g. *!*@bad-domain.tld. Ignoring the fact that I gave an invalid regular expression (starts with a quantifier), .*!.*@bad-domain\.tld won't work as expected either.

What .blocks add hostmask actually does is add to the host_blocks, which does exactly what it says on the tin: It blocks based on the line originator's host. It's a bit confusing, due to the command naming.

The simple solution would be to rename the .blocks ___ hostmask commands to .blocks ___ host, but I'd like to do more than that:

• [x] Rename .blocks ___ hostmask commands to .blocks ___ host (#2344, for 8.0)
• [ ] Implement a real .blocks ___ hostmask command set that actually works on hostmasks
• [ ] Validate ignore masks before adding them (at present, adding an invalid entry can crash Sopel, but without saving the bad entry to the config file; implementing #1354's config auto-save would make bad ignore entries a serious pitfall for new and fat-fingered bot owners alike)
• [ ] Add an API (perhaps bot.is_ignored(Identifier)?) to check an arbitrary user Sopel knows about
  • Would need to handle unknown (offline) users somehow… probably with a specific exception. Sopel deletes users who part all common channels or quit IRC, so checking their blocked status would be incomplete at best
• [ ] Add Trigger attribute(s) so @plugin.unblockable callables can tell if they've been called in spite of a block entry
• [ ] Make regex syntax optional for ignore-list entries, e.g. /nick-?regex/ vs. nickname (/ will never appear in a valid hostmask, hostname, or nickname, so it's a logical choice)
  • [ ] Ideally, the new, default, non-regex syntax would support conventional wildcards identical to how ban masks are interpreted by IRC servers (* and ?)

The first three items definitely can be done in Sopel 6.x 7.x, as the only users who can actually use the ignore-system commands are bot owners/admins who read the changelogs before upgrading. (You do read changelogs before upgrading, riiight? :stuck_out_tongue:). Validation would be a nice non-breaking enhancement, too.

I'm less inclined to change how config file entries are interpreted in a point update, so making regex syntax optional might be a Sopel 7 8 thing. But if there's a clean way to just quietly tweak existing configs on first start and make all existing ignore entries into the regex variety (however that's represented in the file), it doesn't have to wait. The biggest barrier to that is that Sopel's config file isn't versioned—but determining the config format version is easy if it doesn't contain a version! :grin:

Self-assigned this because I already started thinking about the implementation, but no code has been committed Yet™. Therefore, it's also Patches Welcome until I actually prototype (some of) the changes.

[alanhuang122]

I rewrote the blocks module in alanhuang122/sopel@0d0b73a to take IRC hostmasks only. (ignore the change on line 303; it's hardcoded specific minor behavior). Perhaps that could be useful as a reference?

[dgw]

Could be a good start, yeah. Thanks for the pointer!

I'm just shaking my head a bit that you didn't even have to change the docstring for core.hostmask_blocks in that commit, because core.host_blocks already says "hostmasks". :roll_eyes: Lies!

[dgw]

Pushing off to Sopel 7. Version 6.6.0 has gotten too bloated, and this can wait until the next major release.

[dgw]

Just a few weeks left before target release date for 7.0 and this isn't even started. It can wait.

[half-duplex]

Is there a reason not to turn both host_blocks and nick_blocks into hostmask_blocks? E.g. adding a "nick" block could turn into a hostmask "{nick}!.@." block, unless we're worried about regex matching overhead. My only other thought is that some things (clients, services packages) have default ban types, where /ban baduser would automatically ban them with a configurable specificity, like baduser*!*@*.res.blah. That may be useful enough to implement, or confusion between bot block syntax and irc block syntax may be enough to make it worth implementing (at least if we can support eg. naked *).

[dgw]

From an internals perspective, I think yes: matching "host_blocks" on host and "nick_blocks" on nick gives us less overhead, because we're already parsing those components out of the incoming line to populate the trigger object. It's done for free, and it reduces the amount of text each block type needs to process.

That said, one could also say that having three different lists to process gives us higher overhead anyway, just of a different type.

The idea of having default ban types that wildcard out part of the hostmask when given a nickname is great, but I think not for .blocks. It sounds like a good fit for adminchannel, in the .ban/.kickban commands. (There's already a ton of "guess how to normalize this" regex logic in that plugin's configureHostMask() function.)

[deathbybandaid]

Some more thoughts regarding the ignore/blocks system, from things I do with my own bot.

• blocks per-channel as well as per-nick/hostmask for individual commands, sometimes you have a command that you want to live-disable to prevent abuse
• I like to store a database value of a dict that contains a timestamp, who disabled it, and a reason given for the disabling (if given)
• allow channel OPs to determine some blocking as per #1229 . (prolly just close that issue in favor of a more robust .blocks anyway)

[dgw]

Blocking by command is sort of possible for channels via config. It's clunky though. I'm hoping most of that could be implemented by a plugin once we get hooks (#1546) in, though, rather than forcing core to handle it all.

[Exirel]

So, just a quick note: we can use the fnmatch built-in module for that. In particular, we can use the translate function to take a file-like pattern (using * and ? wildcard), to get a regex, then use that regex to match on full nick/hostname.

[dgw]

I was going back through really old issues I opened and came across dgw/sopel-BombBot#12 and dgw/sopel-BombBot#13. These still remain unimplemented after almost 5 years mostly because there's no way I, as a plugin developer, want to deal with using Sopel's various block lists to check whether a potential target's attempts to win the game would be ignored.

Seems like a possible new API feature, loosely related to this overhaul of the ignore system. Whether it's part of the ignore system, I don't know/care. It could be a new field on tools.target.User objects, or a new method somewhere that takes a nickname (Identifier) or User and checks.

Any immediate "PLS NO" thoughts from the crew on this issue before I write up that proposal for 8.0?

[Exirel]

Something like bot.is_ignored(Identifier) -> bool? It would make sense, yes.

[dgw]

Added bot.is_ignored(Identifier) idea and @cottongin's semi-request for @plugin.unblockable functions to see if they should have been blocked (via IRC) to the OP task list.