born05 / craft-twofactorauthentication

Craft plugin for two-factor or two-step login using Time Based OTP.
MIT License
36 stars 26 forks source link

Call to a member function getTimestamp() on null #77

Closed joepagan closed 1 year ago

joepagan commented 1 year ago

Hey, some content editors on an app which uses this plugin experienced the following error this morning:

Call to a member function getTimestamp() on null in /var/www/html/cms/vendor/born05/craft-twofactorauthentication/src/services/Verify.php:44 Stack trace: #0 /var/www/html/cms/vendor/born05/craft-twofactorauthentication/src/services/Request.php(32): born05\twofactorauthentication\services\Verify->isVerified() #1 /var/www/html/cms/vendor/born05/craft-twofactorauthentication/src/Plugin.php(61): born05\twofactorauthentication\services\Request->validateRequest() #2 [internal function]: born05\twofactorauthentication\Plugin->born05\twofactorauthentication{closure}() #3 /var/www/html/cms/vendor/yiisoft/yii2/base/Event.php(312): call_user_func() #4 /var/www/html/cms/vendor/yiisoft/yii2/base/Component.php(642): yii\base\Event::trigger() #5 /var/www/html/cms/vendor/craftcms/cms/src/base/ApplicationTrait.php(1500): yii\base\Component->trigger() #6 /var/www/html/cms/vendor/craftcms/cms/src/web/Application.php(105): craft\web\Application->_postInit() #7 /var/www/html/cms/vendor/yiisoft/yii2/base/BaseObject.php(109): craft\web\Application->init() #8 /var/www/html/cms/vendor/yiisoft/yii2/base/Application.php(204): yii\base\BaseObject->construct() #9 [internal function]: yii\base\Application->construct() #10 /var/www/html/cms/vendor/yiisoft/yii2/di/Container.php(419): ReflectionClass->newInstanceArgs() #11 /var/www/html/cms/vendor/yiisoft/yii2/di/Container.php(170): yii\di\Container->build() #12 /var/www/html/cms/vendor/yiisoft/yii2/BaseYii.php(365): yii\di\Container->get() #13 /var/www/html/cms/vendor/craftcms/cms/src/Craft.php(53): yii\BaseYii::createObject() #14 /var/www/html/cms/vendor/craftcms/cms/bootstrap/bootstrap.php(252): Craft::createObject() #15 /var/www/html/cms/vendor/craftcms/cms/bootstrap/web.php(40): require('...') #16 /var/www/html/cms/web/index.php(23): require

Not sure how this is possible but wanted to let you know.

Their claim was several of the content editors were met with the same error in a 15minute period, though we were not able to replicate, though could have been resolved by the time our agency staff were able to test.

niektenhoopen commented 1 year ago

@joepagan That line of code gets the timestamp from the last login date. Are you sure these users have logged in before?

joepagan commented 1 year ago

Hey @niektenhoopen I think I figured this out!

2 users existed with the same username (emails as usernames), I presume this happened because 1 user account was deleted (with content kept) and then a new account was made. At the time I remember not being able to see the old user in the backend, but there was a db row for it!

Untested but I presume this is how craft keeps a reference to this user in the users DB table? I am not sure if that's actually what happens, or something really weird has gone on with this database... Just checked this specific database now a couple of months later, this database table user row no longer exists... not sure what that is about.

I resolved it at the time by updating the old username directly in the db table by adding a 1 suffix.

I guess the plugin may not account for this scenario and it was trying to login with the deleted user.

This is no longer a problem for the app I manage but I will leave it open if you wanted to investigate this further and code against this edge case

roelvanhintum commented 1 year ago

@joepagan thanks for your work. Sorry, for some reason i've missed this issue. The lastLoginDate should also work when the user has just logged in, but adding an extra check when it's null should solve this specific bug.

roelvanhintum commented 1 year ago

@joepagan just released a fix (hopefully) as 3.2.1 Please reopen the issue occurs again.