Allow for removing bots from the webhook server

[PaulSonOfLars]

What

This PR fixes an issue where adding a bot twice with the same token would cause a panic at the HTTP Handler level. This was caused by the fact that we never remove routes for bots that get removed, because the http router only supports adding routes, not removing them.

So, rather than continuously increase the number of bots (some of them dead) on the mux, it would be helpful to remove them completely. Luckily, this can easily be achieved by simply reusing the existing botMapping functionality, which actually handles exactly that usecase. Voila.

Impact

• Are your changes backwards compatible? Yes
• Have you included documentation, or updated existing documentation? Yes
• Do errors and log messages provide enough context? Yes

[svex99]

It looks good and it worked on my project.

I have only found an issue, and it's when is used a custom URL. Right now, if we add a custom URL in the webhook, it doesn't work because it uses explicitly the token from the root URL.

// This code didn't receive updates after the webhook was set.

if err := manager.updater.AddWebhook(bot, "/custom-url/"+token, ext.WebhookOpts{
	SecretToken: manager.webhookSecret,
}); err != nil {
	return nil, err
}

bot.SetWebhook(manager.webhookUrl+"/custom-url/"+token, &gotgbot.SetWebhookOpts{
	MaxConnections:     100,
	AllowedUpdates:     nil,
	DropPendingUpdates: true,
	SecretToken:        manager.webhookSecret,
})

I think the issue comes from here. The ext.botData.urlPath field is being ignored.

Close to this issue, I wonder if we could customize the identifier for the webhook of a bot. With this in mind, we can have a base URL for all webhooks and a custom identifier for each bot, mapping from the URL: identifier -> bot. Or we can have a custom URL for each bot, mapping full URL -> bot.

What I want to achieve with this is not to be forced to set the full bot token as identifier in the webhook for a bot. Right now is mandatory, since the bot mapping is token -> bot. There are some ideas to not encourage this, but focusing on the actual implementation we are protecting the webhook endpoint with a secret, when getting access to the URL is enough to get control over the bot.

[PaulSonOfLars]

@svex99 You are entirely correct, thats my oversight entirely. Very good catch, thank you for that. I guess thats what I deserve for trying to do quick fixes.

Let me take another proper look at the issue, shouldn't be too difficult to prepare!

[PaulSonOfLars]

Have added an additional mapping of urlPath -> botToken so we can handle incoming URLs better. Let me know if that causes any other issues!

[svex99]

@PaulSonOfLars, thank you for your hard work!

Now we have an issue because the StartPolling method of the updater adds the bots with empty webhooks URLs. Running the echoMultiBot sample with two bots shows the error.

I was thinking about using a single map ID -> bot that can work for polling and webhooks. As such, if we add a bot from StartPolling or AddWebHook we can get the ID right away.

In the case of the webhooks, when calling StartWebhook, implicitly we can append the bot ID to the end of the URL. After that, we can get the ID from the last segment of the URL when handling requests. So, we don't even need to store a custom urlPath, and still are able to set any webhook, and route to the right bot.

Also, for bot removal, we just need the ID of the bot in both cases.

I don't expect you to implement this solution, it may introduce some side effects I don't see right now.

Thank you!

[PaulSonOfLars]

Hey @svex99 - thanks again for the hard work in testing my tired and shoddy code. Excellent work 😆.

I've gone ahead and patched the things you mentioned, as well as written a lot of new tests to cover those cases, and I hope I've now covered everything. At the very least, test coverage is higher than it was :p

Ended up going with a second map of urlPath -> botId instead of what you suggested, purely because I think it makes it clearer, and avoids doing any unexpected URL magic. Your suggestion would probably have worked too though.

Anyway, third time the charm!

[svex99]

Great @PaulSonOfLars! I'll be given a try to this soon.

Thank you for your hard work!

[PaulSonOfLars]

@svex99 Did you have the time to test that the above worked for you?

[svex99]

@PaulSonOfLars everything looks good right now!