From 7a80c44cabefe86eddac11473231dc393fe296ba Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Mon, 8 Mar 2021 22:21:10 +0100 Subject: [PATCH] Alternative implementation of passing totp to the legacy Minecraft authorization protocol to not break the yggdrasil's protocol [deploy] --- .../authserver/models/AuthenticationForm.php | 45 ++++++++++++------- .../authserver/AuthorizationCest.php | 3 +- .../models/AuthenticationFormTest.php | 43 ++++++++++++++---- 3 files changed, 65 insertions(+), 26 deletions(-) diff --git a/api/modules/authserver/models/AuthenticationForm.php b/api/modules/authserver/models/AuthenticationForm.php index e6775c9..c12df0b 100644 --- a/api/modules/authserver/models/AuthenticationForm.php +++ b/api/modules/authserver/models/AuthenticationForm.php @@ -29,11 +29,6 @@ class AuthenticationForm extends ApiForm { */ public $password; - /** - * @var string - */ - public $totp; - /** * @var string */ @@ -47,7 +42,6 @@ class AuthenticationForm extends ApiForm { public function rules(): array { return [ [['username', 'password', 'clientToken'], RequiredValidator::class], - [['totp'], 'string'], [['clientToken'], ClientTokenValidator::class], [['requestUser'], 'boolean'], ]; @@ -68,17 +62,31 @@ class AuthenticationForm extends ApiForm { // so we keep such behavior $attribute = strpos($this->username, '@') === false ? 'nickname' : 'email'; + $password = $this->password; + $totp = null; + if (preg_match('/.{8,}:(\d{6})$/', $password, $matches) === 1) { + $totp = $matches[1]; + $password = mb_substr($password, 0, -7); // :123456 - 7 chars + } + + login: + $loginForm = new LoginForm(); $loginForm->login = $this->username; - $loginForm->password = $this->password; - $loginForm->totp = $this->totp; - if (!$loginForm->validate() || $loginForm->getAccount()->status === Account::STATUS_DELETED) { - $errors = $loginForm->getFirstErrors(); - if (isset($errors['totp'])) { - Authserver::error("User with login = '{$this->username}' protected by two factor auth."); - throw new ForbiddenOperationException('Account protected with two factor auth.'); - } + $loginForm->password = $password; + $loginForm->totp = $totp; + $isValid = $loginForm->validate(); + // Handle case when user's password matches the template for totp via password + if (!$isValid && $totp !== null && $loginForm->getFirstError('password') === E::PASSWORD_INCORRECT) { + $password = "{$password}:{$totp}"; + $totp = null; + + goto login; + } + + if (!$isValid || $loginForm->getAccount()->status === Account::STATUS_DELETED) { + $errors = $loginForm->getFirstErrors(); if (isset($errors['login'])) { if ($errors['login'] === E::ACCOUNT_BANNED) { Authserver::error("User with login = '{$this->username}' is banned"); @@ -88,9 +96,14 @@ class AuthenticationForm extends ApiForm { Authserver::error("Cannot find user by login = '{$this->username}'"); } elseif (isset($errors['password'])) { Authserver::error("User with login = '{$this->username}' passed wrong password."); - } + } elseif (isset($errors['totp'])) { + if ($errors['totp'] === E::TOTP_REQUIRED) { + Authserver::error("User with login = '{$this->username}' protected by two factor auth."); + throw new ForbiddenOperationException('Account protected with two factor auth.'); + } - // TODO: эта логика дублируется с логикой в SignoutForm + Authserver::error("User with login = '{$this->username}' passed wrong totp token"); + } throw new ForbiddenOperationException("Invalid credentials. Invalid {$attribute} or password."); } diff --git a/api/tests/functional/authserver/AuthorizationCest.php b/api/tests/functional/authserver/AuthorizationCest.php index 65cd480..d5ebefa 100644 --- a/api/tests/functional/authserver/AuthorizationCest.php +++ b/api/tests/functional/authserver/AuthorizationCest.php @@ -95,8 +95,7 @@ class AuthorizationCest { public function byEmailWithEnabledTwoFactorAuthAndCorrectToken(FunctionalTester $I) { $I->sendPOST('/api/authserver/authentication/authenticate', [ 'username' => 'otp@gmail.com', - 'password' => 'password_0', - 'totp' => TOTP::create('BBBB')->now(), + 'password' => 'password_0:' . TOTP::create('BBBB')->now(), 'clientToken' => Uuid::uuid4()->toString(), ]); $I->canSeeResponseCodeIs(200); diff --git a/api/tests/unit/modules/authserver/models/AuthenticationFormTest.php b/api/tests/unit/modules/authserver/models/AuthenticationFormTest.php index 1fa674c..6cef3b0 100644 --- a/api/tests/unit/modules/authserver/models/AuthenticationFormTest.php +++ b/api/tests/unit/modules/authserver/models/AuthenticationFormTest.php @@ -6,12 +6,13 @@ namespace codeception\api\unit\modules\authserver\models; use api\modules\authserver\exceptions\ForbiddenOperationException; use api\modules\authserver\models\AuthenticationForm; use api\tests\unit\TestCase; +use common\models\Account; use common\models\OauthClient; use common\models\OauthSession; use common\tests\fixtures\AccountFixture; use common\tests\fixtures\OauthClientFixture; use OTPHP\TOTP; -use Ramsey\Uuid\Uuid; +use function Ramsey\Uuid\v4 as uuid4; class AuthenticationFormTest extends TestCase { @@ -26,7 +27,7 @@ class AuthenticationFormTest extends TestCase { $authForm = new AuthenticationForm(); $authForm->username = 'admin'; $authForm->password = 'password_0'; - $authForm->clientToken = Uuid::uuid4()->toString(); + $authForm->clientToken = uuid4(); $result = $authForm->authenticate()->getResponseData(); $this->assertMatchesRegularExpression('/^[\w=-]+\.[\w=-]+\.[\w=-]+$/', $result['accessToken']); $this->assertSame($authForm->clientToken, $result['clientToken']); @@ -55,9 +56,29 @@ class AuthenticationFormTest extends TestCase { public function testAuthenticateByValidCredentialsWith2FA() { $authForm = new AuthenticationForm(); $authForm->username = 'otp@gmail.com'; - $authForm->password = 'password_0'; - $authForm->totp = TOTP::create('BBBB')->now(); - $authForm->clientToken = Uuid::uuid4()->toString(); + $authForm->password = 'password_0:' . TOTP::create('BBBB')->now(); + $authForm->clientToken = uuid4(); + + // Just ensure that there is no exception + $this->expectNotToPerformAssertions(); + + $authForm->authenticate(); + } + + /** + * This is a special case which ensures that if the user has a password that looks like + * a two-factor code passed in the password field, than he can still log in into his account + */ + public function testAuthenticateEdgyCaseFor2FA() { + /** @var Account $account */ + $account = Account::findOne(['email' => 'admin@ely.by']); + $account->setPassword('password_0:123456'); + $account->save(); + + $authForm = new AuthenticationForm(); + $authForm->username = 'admin@ely.by'; + $authForm->password = 'password_0:123456'; + $authForm->clientToken = uuid4(); // Just ensure that there is no exception $this->expectNotToPerformAssertions(); @@ -68,14 +89,19 @@ class AuthenticationFormTest extends TestCase { /** * @dataProvider getInvalidCredentialsCases */ - public function testAuthenticateByWrongNicknamePass(string $expectedExceptionMessage, string $login, string $password) { + public function testAuthenticateByWrongCredentials( + string $expectedExceptionMessage, + string $login, + string $password, + string $totp = null + ) { $this->expectException(ForbiddenOperationException::class); $this->expectExceptionMessage($expectedExceptionMessage); $authForm = new AuthenticationForm(); $authForm->username = $login; - $authForm->password = $password; - $authForm->clientToken = Uuid::uuid4()->toString(); + $authForm->password = $password . ($totp ? ":{$totp}" : ''); + $authForm->clientToken = uuid4(); $authForm->authenticate(); } @@ -84,6 +110,7 @@ class AuthenticationFormTest extends TestCase { yield ['Invalid credentials. Invalid email or password.', 'wrong-email@ely.by', 'wrong-password']; yield ['This account has been suspended.', 'Banned', 'password_0']; yield ['Account protected with two factor auth.', 'AccountWithEnabledOtp', 'password_0']; + yield ['Invalid credentials. Invalid nickname or password.', 'AccountWithEnabledOtp', 'password_0', '123456']; } }