eggheads / eggdrop

The Eggdrop IRC Bot
GNU General Public License v2.0
506 stars 84 forks source link

Adding delbot tcl-command #1343

Closed crazycatdevs closed 2 years ago

crazycatdevs commented 2 years ago

Egdgrop has the following commands: addbot, adduser and deluser. To delete a bot, the command to use is deluser but no check is done to verify which kind of user you del.

The delbot command will act as deluser but with the good check. Tcl version:

proc delbot {handle} {
   if {[validuser $handle] && [matchattr ||+b $handle]} {
      deluser $handle
   } else {
      putlog "$handle is not a bot"
   }
}
vanosg commented 2 years ago

From a use case scenario, what benefit does this add? Why would a user want to use this command over deluser? What "bad thing" would delbot prevent?

crazycatdevs commented 2 years ago

Well, it's more a protection for badly created scripts. The intention is to have deluser which only removes users (check if it's not a bot) and delbot only removes bots. I think it could be dangerous to not having these protections.

More advanced thing: I'm actually working on a botnet management script, and it let me see the danger: imagine you have an user lambda in an eggdrop and in another (linked) you have a bot called lambda. You may remove user and bot (if not actually linked) when you only want to remove one of them

vanosg commented 2 years ago

Unfortunately, we can't change the functionality of deluser to not remove bots for backwards compatibility reasons, so it will always be able to remove bots- so for that reason, I think this isn't something that is going to gain traction. I think the best solution for developers to implement something like this is to check for the existence of the +b flag before running the deluser command. That would give you the same functionality as what you're proposing.

crazycatdevs commented 2 years ago

No trouble, I'll manage it with tcl, as I already do.

ZarTek-Creole commented 2 years ago

I support crazycat's proposal. For the simple reason that addbot exists, the logic would be that its opposite also exists. The existence of delbot with verification makes sense.

We can see here that addbot does add a bot flag. a delbot which would remove only bots, by checking that they have the flag +b beforehand with a status provided for this purpose seems to me to be obvious "logic".

You add a user with adduser, you delete it with deluser.

You add a bot with addbot, you delete it with deluser, uh with delbot !

delbot <handle>
Description: Attempts to clear the bot record for a handle

Returns: 1 if successful, 0 if no such bot exists, -1 if not a bot

Module: core
vanosg commented 2 years ago

The "logic" for addbot existing separate from adduser is that a bot record has extra flags that need to be associated with it when it is created. But, bots and users are removed the same way, without regard to the flag. We can have opinions if that is the right way to do it or not, but like it or not, deluser removes any record without checking and that functionality won't be removed because it would break 30 years of scripts. If you want to check if its a bot before removing, use the matchattr command.

ZarTek-Creole commented 2 years ago

I do not speak anywhere (for my part) of modifying deluser. Only logical to have the command opposed to addbot which also takes as an argument the address of the link bot, and also sets the flags +b (bot). I support the idea of adding a delbot with the indicated behavior:

delbot <handle>
Description: Attempts to clear the bot record for a handle

Returns: 1 if successful, 0 if no such bot exists, -1 if not a bot

Module: core

The title of this issue talks about a delbot, as well as the content of CrazyCat. This says that deluser does not check any flags while delbot should check that it is indeed a user with the bot flag. This way, deluser removes any type of user (+b or not set) and delbot does a check that it is indeed a user +b If delbot is used on a user without the +b flag it will return -1 (bot flag not found)

Pay no attention to this: "The intention is to have deluser which only removes users (check if it's not a bot)" from CrazyCat for the reasons you mentioned. The rest is acceptable

the delbot will ensure that we are indeed dealing with a bot and not a user (without +b). On the other hand, we do not change anything in deluser, because a bot remains a user in a broader way and like this, we do not break backward compatibility