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

Telegram master username improvements #5192

Closed
Gobberwart opened this issue Sep 4, 2016 · 16 comments
Closed

Telegram master username improvements #5192

Gobberwart opened this issue Sep 4, 2016 · 16 comments

Comments

@Gobberwart
Copy link
Contributor

I want to log this as an issue which needs further discussion/investigation. Basically, the issue is this:

The Telegram API does not support sending messages to a user by username. Instead, the user's id is needed.

We are currently trying to work around this by permitting users to enter their username in the config (instead of id), then performing various bits of voodoo to get the id from the username. This requires that the user send a message to the bot to initialize it, and only works if the user has specified a username in the Telegram app itself (not required by default and needs a settings change).

@DBa2016 has recently done some good work in making this id persistent in the database, so this initialization message only needs to be done once, and then it should be fine.

That's a big improvement, but it would be even better if no initialization message was needed, and if the bot could just lookup the user's id from their username.

My feeling is that we should stop supporting username, and force use of ID until we can make it work as desired, but it seems to be a contentious issue, and fair enough since some users have no doubt gone through the setup process with "username" and have it working, so removing that functionality would seem a backward step.

Would appreciate others' thoughts and possible suggestions to make this actually work the way it should.

@DBa2016
Copy link
Contributor

DBa2016 commented Sep 5, 2016

An addition here: sending user name is actually supported (as per documentation), but it does not work (bug in the Telegram bot API).

Usage of username within Telegram is a feature which allows people to communicate without disclosing their cell phone number (yes, some of us do use Telegram for its original purpose :)), so it is actually an additional security measure.

@Gobberwart
Copy link
Contributor Author

I don't actually see in the docs where username is supported.

https://core.telegram.org/method/messages.sendMessage

Sendmessage method requires a peer, being a user (not username) or chat.

@oralunal
Copy link
Contributor

oralunal commented Sep 5, 2016

@Gobberwart When I entered my username as master, Bot was showing my user id. Can't you use that ID? When you get the user ID save it in variable then make requests with that user ID, isn't this simple?

@Gobberwart
Copy link
Contributor Author

@oralunal Yes, that's what we're currently doing. I think it's pretty simple, but it has been criticized as being too complicated. I think the analogy used was something like having to pull out a car battery and identify its part number in order to start your car. (Not my analogy)

@askovpen
Copy link
Contributor

askovpen commented Sep 5, 2016

I don't understand, why need duck tape with usernames, if can use userid.

@rawgni
Copy link
Contributor

rawgni commented Sep 5, 2016

my bad. should have read the first post!

@Gobberwart
Copy link
Contributor Author

Gobberwart commented Sep 5, 2016

@askovpen My understanding of the thinking goes like this:

  • User id works properly
  • Making users find their user id is (a little bit) complicated, so...
  • Just use username instead, which would be great if it worked, but...
  • Username doesn't work properly, so...
  • Insert a lot of complex code to make username ALMOST work, but...
  • It still doesn't work 100% right, but it's easier (ish)

I don't agree with it myself. Finding the user id really isn't that hard, or at least really no harder than setting up a username in Telegram (you don't get one by default) and then sending a message to the bot to basically just store the id in the database for future use.

I feel that the complexity is about the same to get the correct end result, so... shrug. That's why I'm looking for ideas.

@Gobberwart
Copy link
Contributor Author

Gobberwart commented Sep 5, 2016

@rawgni I don't quite understand the point of adding a password. As long as you specify the correct userid (or username) in config then we know that any message received from that user is authorised. Why complicate it?

Or do you mean allow anyone to talk to the bot without having to specify master, so long as they provide the correct password you set in config, eg. "/start Password123"

If so, that's not a bad idea. IN fact, that's an awesome idea.

@rawgni
Copy link
Contributor

rawgni commented Sep 5, 2016

@Gobberwart yes, no need to specify master or id. If you know the password, you have access to the bot.

  1. people do not need to figure out their userid.
  2. no problem with username not working properly.

of course , you can limit it to only 1 account by making the bot ignore any /start command after the first successful one. Thus, only the first guy get access to the bot.

@rawgni
Copy link
Contributor

rawgni commented Sep 5, 2016

@Gobberwart or use the username found in auth.json as the password.

So, one would identify himself by doing /start <username>

@Gobberwart
Copy link
Contributor Author

Gobberwart commented Sep 5, 2016

If it's a separate password, that would allow multiple people to access the same bot without compromising the account password, so probably a separate one is better.

This really is a good idea of yours. Completely negates the need for anyone to find user ids, fiddle with usernames etc. Just

  • Set up the bot with Telegram api key, password (chosen by user) and "enabled": true
  • Send message to bot "/start PASSWORD"
  • Receive confirmation message

That's it.

Some thoughts:

  • If password is correct, bot responds to user to confirm
  • If password is incorrect, bot should not respond, instead write to log file
  • Cap incorrect password attempts at 3 then lock out for 10 minutes
  • Userid(s) saved in db
  • Alerts sent to (and commands accepted from) all stored userids
  • Probably need a /stop command to remove the current user as well

@DBa2016
Copy link
Contributor

DBa2016 commented Sep 5, 2016

@Gobberwart , @rawgni - requiring a password in order to authenticate with the bot is an awesome idea...

Suggestion on implementation (contradicts a bit with @Gobberwart ):

  • needs to be enabled in config file; can be in parallel with the current "master" config (or be mutual exclusive...in that case, an error message would be issued in case both are specified and the bot would be terminated...); individual password needs to be enforced one way or another.
  • if enabled, an unauthenticated user (=which has not communicated to the bot before) needs to send an "/login " command to the bot. The bot should respond with a "password incorrect", and upon 3 unsuccessful logins the user's id is blacklisted in the database until bot restart (blacklist to be cleared on bot initialization).
  • upon successful authentication, user's ID is stored in the persistent "authenticated IDs" list/table; from then on, user can issue commands to the bot
  • easy to implement because already existing:
  • - /info
  • - /subscribe (currently without parameters, subscribes the user ID to receive events as per config file; more granular config can be added later)
  • - /unsubscribe (guess what it does!)
  • - /logout (removes user's id from authenticated list)

If the bot has only one master, then /login and /subscribe will probably be mostly sent directly after each other. With multiple masters though, some might want to control the bot, others might want to receive notifications only.

Once my PR #5182 is merged, I can implement this.

I also envision adding more commands, like:

  • show top X pokemons sorted by CP/IV (...with candy counts)
  • show current stardust amount
  • upgrade a certain pokemon (once, X times, until highest possible level)
  • evolve a certain pokemon
  • transfer a certain pokemon
  • terminate the bot
  • immediately pu the bot to sleep for X minutes

@Gobberwart : the "@username" parameter is documented in the bot api, here: https://core.telegram.org/bots/api#sendmessage.

@Gobberwart
Copy link
Contributor Author

Gobberwart commented Sep 5, 2016

@DBa2016 Only a couple of minor points of difference, but otherwise awesome.

I'll tell you my thinking on those points and let you decide...

On incorrect password, do not respond via Telegram, but emit log message

Cuts down on potential for someone fishing for a bot. Shrug, I don't think this is critical and I think an "incorrect password" response via telegram is more user-friendly.

Password lockout timer (rather than bot restart)

Should be pretty trivial to implement a 10-minute lockout timer after 3 failed attempts. eg.

After 3 fails, "lockout_timer = now + 10 minutes" (pseudocode, obviously).
On subsequent attempts, if now < lockout_timer, don't even try to authenticate, just generate an appropriate message (log and/or response), eg. "Bot is not accepting new login attempts until $lockout_timer. Please wait and try again." (or similar)

Would be preferable to bot restart I think.

Re: "@username" parameter, that doc says "user or chat", meaning the id of a specific user or conversation (eg. group chat). Username isn't mentioned.

@DBa2016
Copy link
Contributor

DBa2016 commented Sep 5, 2016

The implementation is actually already pretty much ready. Still, it REALLY depends on #5182. Yes, it will also change those regex-based numeric checks to "isnumeric" which @sohje was so unhappy with. No, this does not eliminate "import re", because regexes are needed for command parsing.

@Gobberwart
Copy link
Contributor Author

@DBa2016 I'm happy with #5182 but a couple of others seem to have queries about it. Going to leave it a couple of hours for them to respond but if no further feedback, I'll merge it.

@Gobberwart
Copy link
Contributor Author

#5238 looks good. OK to close this.

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

No branches or pull requests

5 participants