Conversation
| FROM {tool_mfa} | ||
| WHERE userid = ? | ||
| AND factor = \'telegram\' | ||
| AND label LIKE \'telegram:%\''; |
There was a problem hiding this comment.
This should use $DB->sql_like() for cross db likes. But is this extra where clause even needed at all?
Under what conditions would the label not match this?
| public function possible_states($user) { | ||
| // Email can return all states. | ||
| return array( | ||
| \tool_mfa\plugininfo\factor::STATE_FAIL, |
There was a problem hiding this comment.
I can't see any code handling an explicit FAIL so I think this is best removed for the MVP
| @@ -0,0 +1,10 @@ | |||
| <?php | |||
| $path = "https://api.telegram.org/bot1204730014:AAEMpzxxRPRnQTLZ9eCMNgCAmBd0d9pX8iQ"; | |||
There was a problem hiding this comment.
It would be good to keep this file but turn it into an admin only 'test connection' in the admin settings page where you set the api
| * {@inheritDoc} | ||
| */ | ||
| public function setup_factor_form_definition($mform) { | ||
| $mform->addElement('text', 'telegramuserid', get_string('telegram:telegramuserid', 'factor_telegram')); |
There was a problem hiding this comment.
I found this a little confusing:
-
the form asks for an ID which I usually assume is a number. I could not find that number anywhere so I used myusername which appeared to work
-
there was no validation that I own this username. It should send a code to this account and you must validate / verify that code before the telegram record should be created. See the TOTP form for an example of this
There was a problem hiding this comment.
Ah as it turned out using my username didn't work at all. I had to go digging but eventually found my user id. I think this should accept a username and then make an API call the grab the userid and then store that so it works if people change their name.
Also the dud factor I setup using my username could not be revoked. Nor could the second one I made using my user id:
Lastly the 'label' for the dud was somehow my user agent string which is weird, it should show my username / name.
| // You should have received a copy of the GNU General Public License | ||
| // along with Moodle. If not, see <http://www.gnu.org/licenses/>. | ||
| /** | ||
| * Revoke email form. |
| $string['verificationcode_help'] = 'Verification code has been sent to your Telegram account'; | ||
| $string['summarycondition'] = 'has valid Telegram setup'; | ||
| $string['settings:telegrambottoken'] = 'Token of Telegram bot'; | ||
| $string['telegram:telegramuserid'] = 'Your Telegram user ID'; No newline at end of file |
There was a problem hiding this comment.
Should be 'Username' to match the telegram language
|
|
||
| $settings->add(new admin_setting_configtext('factor_telegram/telegrambottoken', | ||
| new lang_string('settings:telegrambottoken', 'factor_telegram'), | ||
| new lang_string('settings:telegrambottoken', 'factor_telegram'), null,PARAM_TEXT, 50)); |
There was a problem hiding this comment.
Bonus points, after saving the bottoken it should do a lookup to grab the bot username and bot name and then cache this and show it in an info box under this setting and then you know 100% that it worked
|
hi @Dagefoerde this looks like a great little factor 👍 I struggled a little with the bot api keys and in the end I could not get it working. I hacked the Telegram.php file with my key which got rid of the ugly stack trace I was getting, but still no message came out the other side in telegram :/ Perhaps this simply isn't written yet? |
|
|
||
| public function send_message($userid, $text) { | ||
| $path = "https://api.telegram.org/bot".$this->token; | ||
| file_get_contents($path."/sendmessage?chat_id=".$userid."&text=".$text); |
There was a problem hiding this comment.
Please wrap this in some error handling and use the curl library. I would expect the telegram to be pretty snappy so it could have a very short timeout of say 5-10 seconds and not the default which can hang for some time
| // Delete all telegram records except base record. | ||
| $selectsql = 'userid = ? | ||
| AND factor = ? | ||
| AND label NOT LIKE \'telegram:%\''; |
There was a problem hiding this comment.
I think there is some confusion around the intended data model and the handling of the 'secret'. You should never delete this record nor create it while logging in, just set the secret field and then unset just that one field. There should only ever be one factor record per device / account, and not new records made for each login.
I think this explains quite a few of the kinks in my testing and once fixed it should all be pretty smooth
There was a problem hiding this comment.
This is the main thing which will be affected by the planned refactor, so the post state will only remove the transient secrets and not touch the main mfa table.
|
@Dagefoerde I've had a big chat with @Peterburnett who is currently working on a new SMS factor. Some of the decisions that you have made in the telegram factor are further evidence to us that the internal MFA tables are not in the best shape they could be (no criticism at all on your patch). For instance both email and SMS factors the link to the external account is managed elsewhere, but for telegram this link (ie username) should be in the tool_mfa table, and there should be a very clean separation between the 'factor setup table' and the 'temporary factor secrets table' which is used when you login. Many of the existing factors so far like TOTP dont need this second table as the code is derived rather than stored. So we've decided that we'll do some refactoring of the core MFA tables and create a new second table to store the transient temporary code, and @Peterburnett will implement that some time in the next week or two for SMS. This should then pave the way to make the telegram factor to much simpler and nicer. So please disregard some of my comments on the PR, I have deleted the ones which will not make as much sense in a week or two. So there is some more cleanup you can do in the mean time but for other bits if you can wait a couple weeks you'll have a much nicer base to work off and use the SMS factor as the factor to copy rather than email (we'll also clean up email later on to move to the same new model too). |
|
Suddenly with no changes the telegram notification just started working on my local dev box. Maybe the bot needed some time to get fully setup or something? |
That's possible, but we did not experience something like this during our development. Sorry that I can't help with this. But anyway: It seems evident that the UX of the plugin is not that good as of now. It needs way more guidance and explanation on how to set up the bot and how to determine the Telegram username or user ID. Sorry that there was no time for that during our hackathon. Are you generally interested in adding this factor to your plugin as a built-in factor? If you are, I would polish it and improve the UX. |
|
hi @Dagefoerde yes I'd love to have this cleaned up and built in. Telegram is probably considered as more secure than SMS and its another great option to have available I'll make a couple issues for the proposed refactor and ping you so you have visibility / feedback on those |
|
@Dagefoerde the SMS factor has landed including the new secret api and also some UX things like a custom 6 digit code input, I'd love to see the telegram factor land if you still have time :) |
|
Closing in favour of #252. Thanks! |
3 participants
This plugin lets users define their Telegram ID. Upon login, it generates a six-digit code and sends it to the defined Telegram ID.
This plugin is a product of the #MootDACH20 DevCamp (https://moodlemootdach.org/course/view.php?id=13). @Laur0r and I have had a lot of fun adding this alternative to tool_mfa! 💯
Some polishing is required before merging the plugin. However, by creating a pull request early we hope to get some feedback from you.