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

Add a Telegram factor option #252

Open
wants to merge 8 commits into
base: MOODLE_400_STABLE
Choose a base branch
from

Conversation

Dagefoerde
Copy link

[Replaces #231]

This plugin lets users define their Telegram ID. Upon login, it generates a six-digit code and sends it to the defined Telegram ID. The 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! 💯 Now that DevCamp is over, and some time has passed, I finally got around to do some more work. As there is a new secret manager API, I have created the plugin from scratch.

Still needs testing and a bit of improvement in the admin UI, so WIP.

@Dagefoerde Dagefoerde marked this pull request as ready for review January 21, 2021 09:56
@Dagefoerde
Copy link
Author

I have added help texts for admins and users, thus improving the UI overall. Also I have tested the plugin's functionality and believe that, in my opinion, it is production-ready now. Happy to hear your reviews.

@@ -224,7 +224,7 @@ public function not_enough_factors() {
$return = $this->output->notification($notification, 'notifyerror');

// Logout button.
$url = new \moodle_url('\admin\tool\mfa\auth.php', ['logout' => 1]);
$url = new \moodle_url('/admin/tool/mfa/auth.php', ['logout' => 1]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed bug this in master please remove it from here - thanks nice catch :)

@brendanheywood
Copy link
Contributor

brendanheywood commented Feb 5, 2021

hi @Dagefoerde this is looking pretty good. Setting up the bot as admin was smooth, and I really like the info bot which shows you your id. I did have some issues:

  1. I did run into a weird loop where I entered my setup code and then it asked for it again over and over. I revoked it and tried a second time and I got this error:
! ) Notice: Undefined property: stdClass::$telegramid in /var/www/moodle.local/admin/tool/mfa/factor/telegram/classes/factor.php on line 274
--


1 | 0.0000 | 365664 | {main}( ) | .../action.php:0
2 | 0.9307 | 7629320 | factor_telegram\factor->setup_user_factor( ) | .../action.php:93
  1. When you try to setup a telegram user, I could not get the username to work but I could use the telegram id. But if you try to register something which doesn't work, and you go back and start over, then it remembers the username or id from before and you can't enter a new one. I think both of these issues stem from using the session variable, I think this should be stateless and part of the form definition. ie the form has one element for the userid, you save it and it fails validation, re-renders the form and now adds the second form element based on the partial form $data rather than session. In theory you could register two telegram ids at the same this way and they won't clash.

After that gets fixed I think this is good to land, good stuff

@danmarsden danmarsden changed the base branch from master to MOODLE_400_STABLE June 7, 2023 23:52
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

Successfully merging this pull request may close these issues.

3 participants