From 184ff02240c3961770d057f80dca3d6530f02777 Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Thu, 12 May 2016 11:50:30 +0300 Subject: [PATCH] =?UTF-8?q?=D0=98=D0=B7=D0=BC=D0=B5=D0=BD=D1=91=D0=BD?= =?UTF-8?q?=D0=B0=20=D0=BA=D0=BE=D0=B4=D0=B8=D1=80=D0=BE=D0=B2=D0=BA=D0=B0?= =?UTF-8?q?=20=D1=81=D1=82=D0=BE=D0=BB=D0=B1=D1=86=D0=B0=20username=20?= =?UTF-8?q?=D0=B2=20usernames=5Fhistory=20=D0=B4=D0=BB=D1=8F=20=D0=BE?= =?UTF-8?q?=D1=80=D0=B3=D0=B0=D0=BD=D0=B8=D0=B7=D0=B0=D1=86=D0=B8=D0=B8=20?= =?UTF-8?q?=D0=B1=D0=B8=D0=BD=D0=B0=D1=80=D0=BD=D0=BE=D0=B3=D0=BE=20=D0=BF?= =?UTF-8?q?=D0=BE=D0=B8=D1=81=D0=BA=D0=B0=20=D0=98=D0=B7=20Account=20?= =?UTF-8?q?=D0=B2=D1=8B=D0=BD=D0=B5=D1=81=D0=B5=D0=BD=D0=B0=20=D0=BB=D0=BE?= =?UTF-8?q?=D0=B3=D0=B8=D0=BA=D0=B0=20=D0=B0=D1=83=D1=82=D0=B5=D0=BD=D1=82?= =?UTF-8?q?=D0=B8=D1=84=D0=B8=D0=BA=D0=B0=D1=86=D0=B8=D0=B8=20=D0=B2=20?= =?UTF-8?q?=D0=B4=D0=BE=D1=87=D0=B5=D1=80=D0=BD=D0=B8=D0=B9=20AccountIdent?= =?UTF-8?q?ity=20=D0=98=D1=81=D0=BF=D1=80=D0=B0=D0=B2=D0=BB=D0=B5=D0=BD?= =?UTF-8?q?=D0=B0=20=D0=BB=D0=BE=D0=B3=D0=B8=D0=BA=D0=B0=20=D0=B2=D0=B0?= =?UTF-8?q?=D0=BB=D0=B8=D0=B4=D0=B0=D1=86=D0=B8=D0=B8=20=D0=BF=D1=80=D0=B8?= =?UTF-8?q?=20=D0=B2=D1=8B=D0=B7=D0=BE=D0=B2=D0=B5=20=D0=BD=D0=B0=20=D0=BD?= =?UTF-8?q?=D0=B5=D0=B8=D0=B7=D0=BC=D0=B5=D0=BD=D1=91=D0=BD=D0=BD=D0=BE?= =?UTF-8?q?=D0=BC=20=D0=BD=D0=B8=D0=BA=D0=B5=20=D0=B4=D0=BB=D1=8F=20=D1=84?= =?UTF-8?q?=D0=BE=D1=80=D0=BC=D1=8B=20=D1=81=D0=BC=D0=B5=D0=BD=D1=8B=20?= =?UTF-8?q?=D0=BD=D0=B8=D0=BA=D0=B0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- api/config/main.php | 4 +- api/models/AccountIdentity.php | 37 ++++++ api/models/ChangeUsernameForm.php | 3 +- common/models/Account.php | 121 +++--------------- ...0512_080955_usernames_history_encoding.php | 18 +++ .../unit/models/ChangeUsernameFormTest.php | 76 ++++++++--- 6 files changed, 138 insertions(+), 121 deletions(-) create mode 100644 api/models/AccountIdentity.php create mode 100644 console/migrations/m160512_080955_usernames_history_encoding.php diff --git a/api/config/main.php b/api/config/main.php index 63c9182..ff3af4c 100644 --- a/api/config/main.php +++ b/api/config/main.php @@ -13,7 +13,7 @@ return [ 'controllerNamespace' => 'api\controllers', 'components' => [ 'user' => [ - 'identityClass' => \common\models\Account::class, + 'identityClass' => \api\models\AccountIdentity::class, 'enableSession' => false, 'loginUrl' => null, ], @@ -21,7 +21,7 @@ return [ 'traceLevel' => YII_DEBUG ? 3 : 0, 'targets' => [ [ - 'class' => 'yii\log\FileTarget', + 'class' => \yii\log\FileTarget::class, 'levels' => ['error', 'warning'], ], ], diff --git a/api/models/AccountIdentity.php b/api/models/AccountIdentity.php new file mode 100644 index 0000000..e2f9527 --- /dev/null +++ b/api/models/AccountIdentity.php @@ -0,0 +1,37 @@ +getAuthKey() === $authKey; + } + +} diff --git a/api/models/ChangeUsernameForm.php b/api/models/ChangeUsernameForm.php index a98b957..f89bb84 100644 --- a/api/models/ChangeUsernameForm.php +++ b/api/models/ChangeUsernameForm.php @@ -3,7 +3,6 @@ namespace api\models; use api\models\base\PasswordProtectedForm; use common\helpers\Amqp; -use common\models\Account; use common\models\amqp\UsernameChanged; use common\models\UsernameHistory; use PhpAmqpLib\Message\AMQPMessage; @@ -23,7 +22,7 @@ class ChangeUsernameForm extends PasswordProtectedForm { } public function validateUsername($attribute) { - $account = new Account(); + $account = $this->getAccount(); $account->username = $this->$attribute; if (!$account->validate(['username'])) { $this->addErrors($account->getErrors()); diff --git a/common/models/Account.php b/common/models/Account.php index 73a98ea..d6d85ea 100644 --- a/common/models/Account.php +++ b/common/models/Account.php @@ -2,28 +2,26 @@ namespace common\models; use common\components\UserPass; -use damirka\JWT\UserTrait; +use damirka\JWT\UserTrait as UserJWTTrait; use Ely\Yii2\TempmailValidator; use Yii; use yii\base\InvalidConfigException; -use yii\base\NotSupportedException; use yii\behaviors\TimestampBehavior; use yii\db\ActiveRecord; -use yii\web\IdentityInterface; /** * Поля модели: * @property integer $id * @property string $uuid * @property string $username - * @property string $email - * @property string $password_hash - * @property integer $password_hash_strategy - * @property string $password_reset_token - * @property integer $status - * @property integer $created_at - * @property integer $updated_at - * @property integer $password_changed_at + * @property string $email + * @property string $password_hash + * @property integer $password_hash_strategy + * @property string $password_reset_token + * @property integer $status + * @property integer $created_at + * @property integer $updated_at + * @property integer $password_changed_at * * Геттеры-сеттеры: * @property string $password пароль пользователя (только для записи) @@ -36,8 +34,8 @@ use yii\web\IdentityInterface; * Поведения: * @mixin TimestampBehavior */ -class Account extends ActiveRecord implements IdentityInterface { - use UserTrait; +class Account extends ActiveRecord { + use UserJWTTrait; const STATUS_DELETED = -10; const STATUS_REGISTERED = 0; @@ -52,7 +50,7 @@ class Account extends ActiveRecord implements IdentityInterface { public function behaviors() { return [ - TimestampBehavior::className(), + TimestampBehavior::class, ]; } @@ -78,74 +76,6 @@ class Account extends ActiveRecord implements IdentityInterface { ]; } - /** - * @inheritdoc - */ - public static function findIdentity($id) { - return static::findOne(['id' => $id]); - } - - /** - * Finds user by password reset token - * - * @param string $token password reset token - * - * @return static|null - * - * TODO: этот метод нужно убрать из базовой модели - */ - public static function findByPasswordResetToken($token) { - if (!static::isPasswordResetTokenValid($token)) { - return null; - } - - return static::findOne([ - 'password_reset_token' => $token, - 'status' => self::STATUS_ACTIVE, - ]); - } - - /** - * Finds out if password reset token is valid - * - * @param string $token password reset token - * - * @return boolean - * - * TODO: этот метод нужно убрать из базовой модели - */ - public static function isPasswordResetTokenValid($token) { - if (empty($token)) { - return false; - } - - $timestamp = (int) substr($token, strrpos($token, '_') + 1); - $expire = Yii::$app->params['user.passwordResetTokenExpire']; - - return $timestamp + $expire >= time(); - } - - /** - * @inheritdoc - */ - public function getId() { - return $this->getPrimaryKey(); - } - - /** - * @inheritdoc - */ - public function getAuthKey() { - throw new NotSupportedException('This method used for cookie auth, except we using JWT tokens'); - } - - /** - * @inheritdoc - */ - public function validateAuthKey($authKey) { - return $this->getAuthKey() === $authKey; - } - /** * Validates password * @@ -183,24 +113,6 @@ class Account extends ActiveRecord implements IdentityInterface { $this->password_changed_at = time(); } - /** - * Generates new password reset token - * - * TODO: этот метод нужно отсюда убрать - */ - public function generatePasswordResetToken() { - $this->password_reset_token = Yii::$app->security->generateRandomString() . '_' . time(); - } - - /** - * Removes password reset token - * - * TODO: этот метод нужно отсюда убрать - */ - public function removePasswordResetToken() { - $this->password_reset_token = null; - } - public function getEmailActivations() { return $this->hasMany(EmailActivation::class, ['account_id' => 'id']); } @@ -266,4 +178,13 @@ class Account extends ActiveRecord implements IdentityInterface { ->exists(); } + /** + * TODO: нужно создать PR в UserTrait репо, чтобы этот метод сделали абстрактным + * + * @return int + */ + public function getId() { + return $this->getPrimaryKey(); + } + } diff --git a/console/migrations/m160512_080955_usernames_history_encoding.php b/console/migrations/m160512_080955_usernames_history_encoding.php new file mode 100644 index 0000000..2fb26c8 --- /dev/null +++ b/console/migrations/m160512_080955_usernames_history_encoding.php @@ -0,0 +1,18 @@ +getDb()->createCommand(' + ALTER TABLE {{%usernames_history}} + MODIFY username VARCHAR(255) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL + ')->execute(); + } + + public function safeDown() { + $this->alterColumn('{{%usernames_history}}', 'username', $this->string()->notNull()); + } + +} diff --git a/tests/codeception/api/unit/models/ChangeUsernameFormTest.php b/tests/codeception/api/unit/models/ChangeUsernameFormTest.php index 36247a4..a2b0b02 100644 --- a/tests/codeception/api/unit/models/ChangeUsernameFormTest.php +++ b/tests/codeception/api/unit/models/ChangeUsernameFormTest.php @@ -10,7 +10,7 @@ use tests\codeception\common\fixtures\AccountFixture; use Yii; /** - * @property array $accounts + * @property AccountFixture $accounts */ class ChangeUsernameFormTest extends DbTestCase { use Specify; @@ -26,52 +26,94 @@ class ChangeUsernameFormTest extends DbTestCase { public function testChange() { $this->specify('successfully change username to new one', function() { - $model = new DummyChangeUsernameForm([ + $model = $this->createModel([ 'password' => 'password_0', 'username' => 'my_new_nickname', ]); expect($model->change())->true(); - expect(Account::findOne(1)->username)->equals('my_new_nickname'); + expect(Account::findOne($this->getAccountId())->username)->equals('my_new_nickname'); expect(UsernameHistory::findOne(['username' => 'my_new_nickname']))->isInstanceOf(UsernameHistory::class); }); } - public function testUsernameUnavailable() { + public function testChangeWithoutChange() { + $this->scenario->incomplete('This test is written invalid'); + return; + + // TODO: этот тест написан неправильно - запись всё равно добавляется в базу данных, но тест не замечает + /** @noinspection PhpUnreachableStatementInspection */ + $this->specify('no new UsernameHistory record, if we don\'t change nickname', function() { + $model = $this->createModel([ + 'password' => 'password_0', + 'username' => $this->accounts['admin']['username'], + ]); + $callTime = time(); + expect($model->change())->true(); + expect(UsernameHistory::findOne([ + 'AND', + 'username' => $this->accounts['admin']['username'], + ['>=', 'applied_in', $callTime - 5], + ]))->null(); + }); + } + + public function testChangeCase() { + $this->specify('username should change, if we change case of some letters', function() { + $newUsername = mb_strtoupper($this->accounts['admin']['username']); + $model = $this->createModel([ + 'password' => 'password_0', + 'username' => $newUsername, + ]); + expect($model->change())->true(); + expect(Account::findOne($this->getAccountId())->username)->equals($newUsername); + expect(UsernameHistory::findOne(['username' => $newUsername]))->isInstanceOf(UsernameHistory::class); + }); + } + + public function testValidateUsername() { $this->specify('error.username_not_available expected if username is already taken', function() { - $model = new DummyChangeUsernameForm([ + $model = $this->createModel([ 'password' => 'password_0', 'username' => 'Jon', ]); - $model->validate(); + $model->validateUsername('username'); expect($model->getErrors('username'))->equals(['error.username_not_available']); }); $this->specify('error.username_not_available is NOT expected if username is already taken by CURRENT user', function() { - $model = new DummyChangeUsernameForm([ + $model = $this->createModel([ 'password' => 'password_0', - 'username' => 'Admin', + 'username' => $this->accounts['admin']['username'], ]); - $model->validate(); - expect($model->getErrors('username'))->equals([]); + $model->validateUsername('username'); + expect($model->getErrors('username'))->isEmpty(); }); } public function testCreateTask() { - $model = new DummyChangeUsernameForm(); + $model = $this->createModel(); $model->createTask('1', 'test1', 'test'); // TODO: у меня пока нет идей о том, чтобы это как-то успешно протестировать, увы // но по крайней мере можно убедиться, что оно не падает где-то на этом шаге } -} + private function createModel(array $params = []) : ChangeUsernameForm { + /** @noinspection PhpUnusedLocalVariableInspection */ + $params = array_merge($params, [ + 'accountId' => $this->getAccountId(), + ]); -// TODO: тут образуется магическая переменная 1, что не круто. После перехода на php7 можно заюзать анонимный класс -// и создавать модель прямо внутри теста, где доступен объект фикстур с именами переменных + return new class($params) extends ChangeUsernameForm { + public $accountId; -class DummyChangeUsernameForm extends ChangeUsernameForm { + protected function getAccount() { + return Account::findOne($this->accountId); + } + }; + } - protected function getAccount() { - return Account::findOne(1); + private function getAccountId() { + return $this->accounts['admin']['id']; } }