From 17f1794a4efd61d7d8cfd45e9aebfaf32665a090 Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Sat, 13 Jun 2020 01:55:02 +0300 Subject: [PATCH] Covered all cases, fixed CS, added a new TODO --- .../authserver/models/RefreshTokenForm.php | 6 +--- .../validators/AccessTokenValidator.php | 35 +++++++++++++++---- api/tests/functional/accounts/DeleteCest.php | 1 - api/tests/functional/accounts/RestoreCest.php | 1 - .../functional/authserver/RefreshCest.php | 2 +- common/models/MinecraftAccessKey.php | 4 +-- console/controllers/WebhooksController.php | 2 ++ console/models/WebHookForm.php | 1 + 8 files changed, 35 insertions(+), 17 deletions(-) diff --git a/api/modules/authserver/models/RefreshTokenForm.php b/api/modules/authserver/models/RefreshTokenForm.php index 434e1f3..cf21657 100644 --- a/api/modules/authserver/models/RefreshTokenForm.php +++ b/api/modules/authserver/models/RefreshTokenForm.php @@ -62,14 +62,10 @@ class RefreshTokenForm extends ApiForm { $account = Account::findOne(['id' => $tokenReader->getAccountId()]); } - if ($account === null || $account->status === Account::STATUS_DELETED) { + if ($account === null) { throw new ForbiddenOperationException('Invalid token.'); } - if ($account->status === Account::STATUS_BANNED) { - throw new ForbiddenOperationException('This account has been suspended.'); - } - $token = Yii::$app->tokensFactory->createForMinecraftAccount($account, $this->clientToken); // TODO: This behavior duplicates with the AuthenticationForm. Need to find a way to avoid duplication. diff --git a/api/modules/authserver/validators/AccessTokenValidator.php b/api/modules/authserver/validators/AccessTokenValidator.php index 8700c31..355bba6 100644 --- a/api/modules/authserver/validators/AccessTokenValidator.php +++ b/api/modules/authserver/validators/AccessTokenValidator.php @@ -3,18 +3,24 @@ declare(strict_types=1); namespace api\modules\authserver\validators; +use api\components\Tokens\TokenReader; use api\modules\authserver\exceptions\ForbiddenOperationException; use Carbon\Carbon; +use common\models\Account; use common\models\MinecraftAccessKey; use Exception; -use Lcobucci\JWT\ValidationData; use Yii; use yii\validators\Validator; class AccessTokenValidator extends Validator { + private const INVALID_TOKEN = 'Invalid token.'; + private const TOKEN_EXPIRED = 'Token expired.'; + public bool $verifyExpiration = true; + public bool $verifyAccount = true; + /** * @param string $value * @@ -29,15 +35,19 @@ class AccessTokenValidator extends Validator { try { $token = Yii::$app->tokens->parse($value); } catch (Exception $e) { - throw new ForbiddenOperationException('Invalid token.'); + throw new ForbiddenOperationException(self::INVALID_TOKEN); } if (!Yii::$app->tokens->verify($token)) { - throw new ForbiddenOperationException('Invalid token.'); + throw new ForbiddenOperationException(self::INVALID_TOKEN); } - if ($this->verifyExpiration && !$token->validate(new ValidationData(Carbon::now()->getTimestamp()))) { - throw new ForbiddenOperationException('Token expired.'); + if ($this->verifyExpiration && $token->isExpired(Carbon::now())) { + throw new ForbiddenOperationException(self::TOKEN_EXPIRED); + } + + if ($this->verifyAccount && !$this->validateAccount((new TokenReader($token))->getAccountId())) { + throw new ForbiddenOperationException(self::INVALID_TOKEN); } return null; @@ -53,14 +63,25 @@ class AccessTokenValidator extends Validator { /** @var MinecraftAccessKey|null $result */ $result = MinecraftAccessKey::findOne(['access_token' => $value]); if ($result === null) { - throw new ForbiddenOperationException('Invalid token.'); + throw new ForbiddenOperationException(self::INVALID_TOKEN); } if ($this->verifyExpiration && $result->isExpired()) { - throw new ForbiddenOperationException('Token expired.'); + throw new ForbiddenOperationException(self::TOKEN_EXPIRED); + } + + if ($this->verifyAccount && !$this->validateAccount($result->account_id)) { + throw new ForbiddenOperationException(self::INVALID_TOKEN); } return null; } + private function validateAccount(int $accountId): bool { + /** @var Account|null $account */ + $account = Account::find()->excludeDeleted()->andWhere(['id' => $accountId])->one(); + + return $account !== null && $account->status !== Account::STATUS_BANNED; + } + } diff --git a/api/tests/functional/accounts/DeleteCest.php b/api/tests/functional/accounts/DeleteCest.php index dd0725e..aadc306 100644 --- a/api/tests/functional/accounts/DeleteCest.php +++ b/api/tests/functional/accounts/DeleteCest.php @@ -3,7 +3,6 @@ declare(strict_types=1); namespace api\tests\functional\accounts; -use api\tests\_pages\AccountsRoute; use api\tests\FunctionalTester; class DeleteCest { diff --git a/api/tests/functional/accounts/RestoreCest.php b/api/tests/functional/accounts/RestoreCest.php index bc530b0..0e36dd8 100644 --- a/api/tests/functional/accounts/RestoreCest.php +++ b/api/tests/functional/accounts/RestoreCest.php @@ -3,7 +3,6 @@ declare(strict_types=1); namespace api\tests\functional\accounts; -use api\tests\_pages\AccountsRoute; use api\tests\FunctionalTester; class RestoreCest { diff --git a/api/tests/functional/authserver/RefreshCest.php b/api/tests/functional/authserver/RefreshCest.php index 9323405..fb9323c 100644 --- a/api/tests/functional/authserver/RefreshCest.php +++ b/api/tests/functional/authserver/RefreshCest.php @@ -115,7 +115,7 @@ class RefreshCest { $I->canSeeResponseCodeIs(401); $I->canSeeResponseContainsJson([ 'error' => 'ForbiddenOperationException', - 'errorMessage' => 'This account has been suspended.', + 'errorMessage' => 'Invalid token.', ]); } diff --git a/common/models/MinecraftAccessKey.php b/common/models/MinecraftAccessKey.php index d1a18fe..8eb64db 100644 --- a/common/models/MinecraftAccessKey.php +++ b/common/models/MinecraftAccessKey.php @@ -4,7 +4,6 @@ namespace common\models; use common\behaviors\PrimaryKeyValueBehavior; use Ramsey\Uuid\Uuid; use yii\behaviors\TimestampBehavior; -use yii\db\ActiveQuery; use yii\db\ActiveRecord; /** @@ -52,7 +51,8 @@ class MinecraftAccessKey extends ActiveRecord { ]; } - public function getAccount(): ActiveQuery { + public function getAccount(): AccountQuery { + /** @noinspection PhpIncompatibleReturnTypeInspection */ return $this->hasOne(Account::class, ['id' => 'account_id']); } diff --git a/console/controllers/WebhooksController.php b/console/controllers/WebhooksController.php index 1dcd731..3855b3b 100644 --- a/console/controllers/WebhooksController.php +++ b/console/controllers/WebhooksController.php @@ -54,4 +54,6 @@ class WebhooksController extends Controller { return ExitCode::OK; } + // TODO: add action to modify the webhook events + } diff --git a/console/models/WebHookForm.php b/console/models/WebHookForm.php index 806bcc3..bc84757 100644 --- a/console/models/WebHookForm.php +++ b/console/models/WebHookForm.php @@ -60,6 +60,7 @@ class WebHookForm extends Model { public static function getEvents(): array { return [ 'account.edit', + 'account.deletion', ]; }