From 0183e54442c3ecb3327185ce00b9581bffc5b00c Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Fri, 12 Jun 2020 00:27:02 +0300 Subject: [PATCH 1/3] Implemented account deletion. Not all cases covered with tests [skip ci] --- api/config/routes.php | 1 + api/models/base/BaseAccountForm.php | 7 +- .../accounts/actions/DeleteAccountAction.php | 14 +++ .../accounts/actions/RestoreAccountAction.php | 14 +++ .../controllers/DefaultController.php | 95 ++++++++++++------- api/modules/accounts/models/AccountInfo.php | 8 +- .../accounts/models/BanAccountForm.php | 2 +- .../accounts/models/DeleteAccountForm.php | 50 ++++++++++ .../accounts/models/RestoreAccountForm.php | 42 ++++++++ .../authserver/models/AuthenticationForm.php | 10 +- .../authserver/models/RefreshTokenForm.php | 2 +- .../validators/AccessTokenValidator.php | 5 +- api/modules/internal/helpers/Error.php | 3 + .../mojang/controllers/ApiController.php | 7 +- .../session/controllers/SessionController.php | 3 +- api/modules/session/models/JoinForm.php | 4 + api/rbac/Permissions.php | 4 + api/rbac/rules/AccountOwner.php | 8 +- api/tests/_pages/AuthenticationRoute.php | 4 - api/tests/functional/LoginCest.php | 26 +++++ api/tests/functional/LogoutCest.php | 21 ++-- api/tests/functional/accounts/DeleteCest.php | 89 +++++++++++++++++ api/tests/functional/accounts/GetCest.php | 23 ++++- api/tests/functional/accounts/RestoreCest.php | 51 ++++++++++ .../authserver/AuthorizationCest.php | 14 +++ .../functional/authserver/RefreshCest.php | 13 +++ .../functional/authserver/ValidateCest.php | 12 +++ .../functional/mojang/UsernameToUuidCest.php | 7 ++ .../mojang/UsernamesToUuidsCest.php | 4 +- .../mojang/UuidToUsernamesHistoryCest.php | 7 ++ api/tests/functional/oauth/AuthCodeCest.php | 11 +++ .../functional/sessionserver/JoinCest.php | 13 +++ .../sessionserver/JoinLegacyCest.php | 11 +++ .../functional/sessionserver/ProfileCest.php | 11 +++ .../accounts/models/DeleteAccountFormTest.php | 90 ++++++++++++++++++ .../models/RestoreAccountFormTest.php | 61 ++++++++++++ ...BanFormTest.php => BanAccountFormTest.php} | 20 ++-- .../unit/rbac/rules/AccountOwnerTest.php | 11 +++ api/validators/PasswordRequiredValidator.php | 12 +-- common/models/Account.php | 60 ++++++++---- common/models/AccountQuery.php | 4 + common/models/UsernameHistory.php | 29 +++--- common/tasks/ClearAccountSessions.php | 19 ++-- common/tasks/ClearOauthSessions.php | 24 ++--- common/tasks/CreateWebHooksDeliveries.php | 41 ++++---- common/tasks/DeleteAccount.php | 62 ++++++++++++ common/tests/fixtures/data/accounts.php | 31 ++++++ .../fixtures/data/minecraft-access-keys.php | 7 ++ .../tests/fixtures/data/usernames-history.php | 6 ++ common/tests/unit/models/AccountTest.php | 14 ++- .../unit/tasks/ClearAccountSessionsTest.php | 11 +-- .../unit/tasks/ClearOauthSessionsTest.php | 12 +-- .../tasks/CreateWebHooksDeliveriesTest.php | 6 +- common/tests/unit/tasks/DeleteAccountTest.php | 90 ++++++++++++++++++ console/controllers/RbacController.php | 8 ++ .../m200611_123017_accounts_deletion.php | 15 +++ 56 files changed, 1041 insertions(+), 188 deletions(-) create mode 100644 api/modules/accounts/actions/DeleteAccountAction.php create mode 100644 api/modules/accounts/actions/RestoreAccountAction.php create mode 100644 api/modules/accounts/models/DeleteAccountForm.php create mode 100644 api/modules/accounts/models/RestoreAccountForm.php create mode 100644 api/tests/functional/accounts/DeleteCest.php create mode 100644 api/tests/functional/accounts/RestoreCest.php create mode 100644 api/tests/unit/modules/accounts/models/DeleteAccountFormTest.php create mode 100644 api/tests/unit/modules/accounts/models/RestoreAccountFormTest.php rename api/tests/unit/modules/internal/models/{BanFormTest.php => BanAccountFormTest.php} (73%) create mode 100644 common/tasks/DeleteAccount.php create mode 100644 common/tests/unit/tasks/DeleteAccountTest.php create mode 100644 console/migrations/m200611_123017_accounts_deletion.php diff --git a/api/config/routes.php b/api/config/routes.php index f61cab1..34f5653 100644 --- a/api/config/routes.php +++ b/api/config/routes.php @@ -12,6 +12,7 @@ return [ // Accounts module routes 'GET /v1/accounts/' => 'accounts/default/get', + 'DELETE /v1/accounts/' => 'accounts/default/delete', 'GET /v1/accounts//two-factor-auth' => 'accounts/default/get-two-factor-auth-credentials', 'POST /v1/accounts//two-factor-auth' => 'accounts/default/enable-two-factor-auth', 'DELETE /v1/accounts//two-factor-auth' => 'accounts/default/disable-two-factor-auth', diff --git a/api/models/base/BaseAccountForm.php b/api/models/base/BaseAccountForm.php index 6ebd055..a128c7d 100644 --- a/api/models/base/BaseAccountForm.php +++ b/api/models/base/BaseAccountForm.php @@ -1,14 +1,13 @@ request->get('id'); - if ($id === 0) { - $identity = Yii::$app->user->getIdentity(); - if ($identity !== null) { - $account = $identity->getAccount(); - if ($account !== null) { - $id = $account->id; - } - } - } - - return [ - 'accountId' => $id, - ]; - }; - return ArrayHelper::merge(parent::behaviors(), [ 'access' => [ 'class' => AccessControl::class, @@ -40,29 +26,28 @@ class DefaultController extends Controller { 'allow' => true, 'actions' => ['get'], 'roles' => [P::OBTAIN_ACCOUNT_INFO], - 'roleParams' => function() use ($paramsCallback) { - return array_merge($paramsCallback(), [ - 'optionalRules' => true, - ]); - }, + 'roleParams' => $this->createParams([ + 'optionalRules' => true, + 'allowDeleted' => true, + ]), ], [ 'allow' => true, 'actions' => ['username'], 'roles' => [P::CHANGE_ACCOUNT_USERNAME], - 'roleParams' => $paramsCallback, + 'roleParams' => $this->createParams(), ], [ 'allow' => true, 'actions' => ['password'], 'roles' => [P::CHANGE_ACCOUNT_PASSWORD], - 'roleParams' => $paramsCallback, + 'roleParams' => $this->createParams(), ], [ 'allow' => true, 'actions' => ['language'], 'roles' => [P::CHANGE_ACCOUNT_LANGUAGE], - 'roleParams' => $paramsCallback, + 'roleParams' => $this->createParams(), ], [ 'allow' => true, @@ -72,17 +57,15 @@ class DefaultController extends Controller { 'new-email-verification', ], 'roles' => [P::CHANGE_ACCOUNT_EMAIL], - 'roleParams' => $paramsCallback, + 'roleParams' => $this->createParams(), ], [ 'allow' => true, 'actions' => ['rules'], 'roles' => [P::ACCEPT_NEW_PROJECT_RULES], - 'roleParams' => function() use ($paramsCallback) { - return array_merge($paramsCallback(), [ - 'optionalRules' => true, - ]); - }, + 'roleParams' => $this->createParams([ + 'optionalRules' => true, + ]), ], [ 'allow' => true, @@ -92,7 +75,7 @@ class DefaultController extends Controller { 'disable-two-factor-auth', ], 'roles' => [P::MANAGE_TWO_FACTOR_AUTH], - 'roleParams' => $paramsCallback, + 'roleParams' => $this->createParams(), ], [ 'allow' => true, @@ -101,8 +84,33 @@ class DefaultController extends Controller { 'pardon', ], 'roles' => [P::BLOCK_ACCOUNT], - 'roleParams' => $paramsCallback, + 'roleParams' => $this->createParams(), ], + [ + 'allow' => true, + 'actions' => ['delete'], + 'roles' => [P::DELETE_ACCOUNT], + 'roleParams' => $this->createParams([ + 'optionalRules' => true, + 'allowDeleted' => true, // This case will be validated by route handler + ]), + ], + [ + 'allow' => true, + 'actions' => ['restore'], + 'roles' => [P::RESTORE_ACCOUNT], + 'roleParams' => $this->createParams([ + 'optionalRules' => true, + 'allowDeleted' => true, + ]), + ], + ], + ], + 'verb' => [ + 'class' => VerbFilter::class, + 'actions' => [ + 'delete' => ['DELETE'], + 'restore' => ['POST'], ], ], ]); @@ -121,6 +129,8 @@ class DefaultController extends Controller { 'disable-two-factor-auth' => actions\DisableTwoFactorAuthAction::class, 'ban' => actions\BanAccountAction::class, 'pardon' => actions\PardonAccountAction::class, + 'delete' => actions\DeleteAccountAction::class, + 'restore' => actions\RestoreAccountAction::class, ]; } @@ -144,6 +154,25 @@ class DefaultController extends Controller { return parent::bindActionParams($action, $params); } + private function createParams(array $options = []): callable { + return function() use ($options): array { + $id = (int)Yii::$app->request->get('id'); + if ($id === 0) { + $identity = Yii::$app->user->getIdentity(); + if ($identity !== null) { + $account = $identity->getAccount(); + if ($account !== null) { + $id = $account->id; + } + } + } + + return array_merge([ + 'accountId' => $id, + ], $options); + }; + } + private function findAccount(int $id): Account { if ($id === 0) { /** @noinspection NullPointerExceptionInspection */ diff --git a/api/modules/accounts/models/AccountInfo.php b/api/modules/accounts/models/AccountInfo.php index 0086cf9..3064beb 100644 --- a/api/modules/accounts/models/AccountInfo.php +++ b/api/modules/accounts/models/AccountInfo.php @@ -1,4 +1,6 @@ user = Instance::ensure($this->user, User::class); } @@ -35,6 +37,7 @@ class AccountInfo extends BaseAccountForm { $authManagerParams = [ 'accountId' => $account->id, 'optionalRules' => true, + 'allowDeleted' => true, ]; if ($this->user->can(P::OBTAIN_ACCOUNT_EMAIL, $authManagerParams)) { @@ -42,7 +45,8 @@ class AccountInfo extends BaseAccountForm { } if ($this->user->can(P::OBTAIN_EXTENDED_ACCOUNT_INFO, $authManagerParams)) { - $response['isActive'] = $account->status === Account::STATUS_ACTIVE; + $response['isActive'] = !in_array($account->status, [Account::STATUS_REGISTERED, Account::STATUS_BANNED]); + $response['isDeleted'] = $account->status === Account::STATUS_DELETED; $response['passwordChangedAt'] = $account->password_changed_at; $response['hasMojangUsernameCollision'] = $account->hasMojangUsernameCollision(); $response['shouldAcceptRules'] = !$account->isAgreedWithActualRules(); diff --git a/api/modules/accounts/models/BanAccountForm.php b/api/modules/accounts/models/BanAccountForm.php index b88aee3..6408871 100644 --- a/api/modules/accounts/models/BanAccountForm.php +++ b/api/modules/accounts/models/BanAccountForm.php @@ -52,7 +52,7 @@ class BanAccountForm extends AccountActionForm { $account->status = Account::STATUS_BANNED; Assert::true($account->save(), 'Cannot ban account'); - Yii::$app->queue->push(ClearAccountSessions::createFromAccount($account)); + Yii::$app->queue->push(new ClearAccountSessions($account->id)); $transaction->commit(); diff --git a/api/modules/accounts/models/DeleteAccountForm.php b/api/modules/accounts/models/DeleteAccountForm.php new file mode 100644 index 0000000..ff2e5e7 --- /dev/null +++ b/api/modules/accounts/models/DeleteAccountForm.php @@ -0,0 +1,50 @@ + $this->getAccount()], + [['account'], 'validateAccountActivity'], + ]; + } + + public function validateAccountActivity(): void { + if ($this->getAccount()->status === Account::STATUS_DELETED) { + $this->addError('account', E::ACCOUNT_ALREADY_DELETED); + } + } + + public function performAction(): bool { + if (!$this->validate()) { + return false; + } + + $transaction = Yii::$app->db->beginTransaction(); + + $account = $this->getAccount(); + $account->status = Account::STATUS_DELETED; + $account->deleted_at = time(); + Assert::true($account->save(), 'Cannot delete account'); + + // Schedule complete account erasing + Yii::$app->queue->delay($account->getDeleteAt()->diffInRealSeconds())->push(new DeleteAccount($account->id)); + + $transaction->commit(); + + return true; + } + +} diff --git a/api/modules/accounts/models/RestoreAccountForm.php b/api/modules/accounts/models/RestoreAccountForm.php new file mode 100644 index 0000000..3517e2d --- /dev/null +++ b/api/modules/accounts/models/RestoreAccountForm.php @@ -0,0 +1,42 @@ +getAccount()->status !== Account::STATUS_DELETED) { + $this->addError('account', E::ACCOUNT_NOT_DELETED); + } + } + + public function performAction(): bool { + if (!$this->validate()) { + return false; + } + + $transaction = Yii::$app->db->beginTransaction(); + + $account = $this->getAccount(); + $account->status = Account::STATUS_ACTIVE; + $account->deleted_at = null; + Assert::true($account->save(), 'Cannot restore account'); + + $transaction->commit(); + + return true; + } + +} diff --git a/api/modules/authserver/models/AuthenticationForm.php b/api/modules/authserver/models/AuthenticationForm.php index 58f04c7..92dfdb9 100644 --- a/api/modules/authserver/models/AuthenticationForm.php +++ b/api/modules/authserver/models/AuthenticationForm.php @@ -52,10 +52,14 @@ class AuthenticationForm extends ApiForm { Authserver::info("Trying to authenticate user by login = '{$this->username}'."); + // The previous authorization server implementation used the nickname field instead of username, + // so we keep such behavior + $attribute = strpos($this->username, '@') === false ? 'nickname' : 'email'; + $loginForm = new LoginForm(); $loginForm->login = $this->username; $loginForm->password = $this->password; - if (!$loginForm->validate()) { + 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."); @@ -73,10 +77,6 @@ class AuthenticationForm extends ApiForm { Authserver::error("User with login = '{$this->username}' passed wrong password."); } - // The previous authorization server implementation used the nickname field instead of username, - // so we keep such behavior - $attribute = strpos($this->username, '@') === false ? 'nickname' : 'email'; - // TODO: эта логика дублируется с логикой в SignoutForm throw new ForbiddenOperationException("Invalid credentials. Invalid {$attribute} or password."); diff --git a/api/modules/authserver/models/RefreshTokenForm.php b/api/modules/authserver/models/RefreshTokenForm.php index bf86011..434e1f3 100644 --- a/api/modules/authserver/models/RefreshTokenForm.php +++ b/api/modules/authserver/models/RefreshTokenForm.php @@ -62,7 +62,7 @@ class RefreshTokenForm extends ApiForm { $account = Account::findOne(['id' => $tokenReader->getAccountId()]); } - if ($account === null) { + if ($account === null || $account->status === Account::STATUS_DELETED) { throw new ForbiddenOperationException('Invalid token.'); } diff --git a/api/modules/authserver/validators/AccessTokenValidator.php b/api/modules/authserver/validators/AccessTokenValidator.php index 535193d..8700c31 100644 --- a/api/modules/authserver/validators/AccessTokenValidator.php +++ b/api/modules/authserver/validators/AccessTokenValidator.php @@ -13,10 +13,7 @@ use yii\validators\Validator; class AccessTokenValidator extends Validator { - /** - * @var bool - */ - public $verifyExpiration = true; + public bool $verifyExpiration = true; /** * @param string $value diff --git a/api/modules/internal/helpers/Error.php b/api/modules/internal/helpers/Error.php index 7a7dec6..3267d4a 100644 --- a/api/modules/internal/helpers/Error.php +++ b/api/modules/internal/helpers/Error.php @@ -6,4 +6,7 @@ final class Error { public const ACCOUNT_ALREADY_BANNED = 'error.account_already_banned'; public const ACCOUNT_NOT_BANNED = 'error.account_not_banned'; + public const ACCOUNT_ALREADY_DELETED = 'error.account_already_deleted'; + public const ACCOUNT_NOT_DELETED = 'error.account_not_deleted'; + } diff --git a/api/modules/mojang/controllers/ApiController.php b/api/modules/mojang/controllers/ApiController.php index be19e61..e7aff75 100644 --- a/api/modules/mojang/controllers/ApiController.php +++ b/api/modules/mojang/controllers/ApiController.php @@ -32,7 +32,7 @@ class ApiController extends Controller { // who used the nickname has not changed it to something else $account = null; if ($record !== null) { - if ($record->account->username === $record->username || $record->findNext($at) !== null) { + if ($record->account->username === $record->username || $record->findNextOwnerUsername($at) !== null) { $account = $record->account; } } @@ -41,7 +41,7 @@ class ApiController extends Controller { $account = Account::findOne(['username' => $username]); } - if ($account === null) { + if ($account === null || $account->status === Account::STATUS_DELETED) { return $this->noContentResponse(); } @@ -58,7 +58,7 @@ class ApiController extends Controller { return $this->illegalArgumentResponse('Invalid uuid format.'); } - $account = Account::findOne(['uuid' => $uuid]); + $account = Account::find()->excludeDeleted()->andWhere(['uuid' => $uuid])->one(); if ($account === null) { return $this->noContentResponse(); } @@ -103,6 +103,7 @@ class ApiController extends Controller { /** @var Account[] $accounts */ $accounts = Account::find() ->andWhere(['username' => $usernames]) + ->excludeDeleted() ->orderBy(['username' => $usernames]) ->limit(count($usernames)) ->all(); diff --git a/api/modules/session/controllers/SessionController.php b/api/modules/session/controllers/SessionController.php index 4e1fc34..54abede 100644 --- a/api/modules/session/controllers/SessionController.php +++ b/api/modules/session/controllers/SessionController.php @@ -121,7 +121,8 @@ class SessionController extends Controller { throw new IllegalArgumentException('Invalid uuid format.'); } - $account = Account::findOne(['uuid' => $uuid]); + /** @var Account|null $account */ + $account = Account::find()->excludeDeleted()->andWhere(['uuid' => $uuid])->one(); if ($account === null) { throw new ForbiddenOperationException('Invalid uuid.'); } diff --git a/api/modules/session/models/JoinForm.php b/api/modules/session/models/JoinForm.php index 29764ca..c8472a7 100644 --- a/api/modules/session/models/JoinForm.php +++ b/api/modules/session/models/JoinForm.php @@ -165,6 +165,10 @@ class JoinForm extends Model { throw new ForbiddenOperationException('Invalid credentials'); } + if ($account->status === Account::STATUS_DELETED) { + throw new ForbiddenOperationException('Invalid credentials'); + } + $this->account = $account; } diff --git a/api/rbac/Permissions.php b/api/rbac/Permissions.php index 914905c..f8d0c8d 100644 --- a/api/rbac/Permissions.php +++ b/api/rbac/Permissions.php @@ -12,6 +12,8 @@ final class Permissions { public const CHANGE_ACCOUNT_PASSWORD = 'change_account_password'; public const CHANGE_ACCOUNT_EMAIL = 'change_account_email'; public const MANAGE_TWO_FACTOR_AUTH = 'manage_two_factor_auth'; + public const DELETE_ACCOUNT = 'delete_account'; + public const RESTORE_ACCOUNT = 'restore_account'; public const BLOCK_ACCOUNT = 'block_account'; public const COMPLETE_OAUTH_FLOW = 'complete_oauth_flow'; public const CREATE_OAUTH_CLIENTS = 'create_oauth_clients'; @@ -27,6 +29,8 @@ final class Permissions { public const CHANGE_OWN_ACCOUNT_PASSWORD = 'change_own_account_password'; public const CHANGE_OWN_ACCOUNT_EMAIL = 'change_own_account_email'; public const MANAGE_OWN_TWO_FACTOR_AUTH = 'manage_own_two_factor_auth'; + public const DELETE_OWN_ACCOUNT = 'delete_own_account'; + public const RESTORE_OWN_ACCOUNT = 'restore_own_account'; public const MINECRAFT_SERVER_SESSION = 'minecraft_server_session'; public const VIEW_OWN_OAUTH_CLIENTS = 'view_own_oauth_clients'; public const MANAGE_OWN_OAUTH_CLIENTS = 'manage_own_oauth_clients'; diff --git a/api/rbac/rules/AccountOwner.php b/api/rbac/rules/AccountOwner.php index 0aeb7e5..af361ee 100644 --- a/api/rbac/rules/AccountOwner.php +++ b/api/rbac/rules/AccountOwner.php @@ -8,7 +8,7 @@ use Webmozart\Assert\Assert; use Yii; use yii\rbac\Rule; -class AccountOwner extends Rule { +final class AccountOwner extends Rule { public $name = 'account_owner'; @@ -43,7 +43,11 @@ class AccountOwner extends Rule { return false; } - if ($account->status !== Account::STATUS_ACTIVE) { + $allowDeleted = $params['allowDeleted'] ?? false; + if ($account->status !== Account::STATUS_ACTIVE + // if deleted accounts are allowed, but the passed one is not in deleted state + && (!$allowDeleted || $account->status !== Account::STATUS_DELETED) + ) { return false; } diff --git a/api/tests/_pages/AuthenticationRoute.php b/api/tests/_pages/AuthenticationRoute.php index d926d0b..87783d1 100644 --- a/api/tests/_pages/AuthenticationRoute.php +++ b/api/tests/_pages/AuthenticationRoute.php @@ -24,10 +24,6 @@ class AuthenticationRoute extends BasePage { $this->getActor()->sendPOST('/api/authentication/login', $params); } - public function logout() { - $this->getActor()->sendPOST('/api/authentication/logout'); - } - public function forgotPassword($login = null, $token = null) { $this->getActor()->sendPOST('/api/authentication/forgot-password', [ 'login' => $login, diff --git a/api/tests/functional/LoginCest.php b/api/tests/functional/LoginCest.php index f1abfe7..9e13547 100644 --- a/api/tests/functional/LoginCest.php +++ b/api/tests/functional/LoginCest.php @@ -1,10 +1,13 @@ canSeeAuthCredentials(false); } + public function testLoginIntoDeletedAccount(FunctionalTester $I) { + $route = new AuthenticationRoute($I); + + $I->wantTo('login into account that marked for deleting'); + $route->login('DeletedAccount', 'password_0'); + $I->canSeeResponseContainsJson([ + 'success' => true, + ]); + } + + public function testLoginIntoBannedAccount(FunctionalTester $I) { + $route = new AuthenticationRoute($I); + + $I->wantTo('login into banned account'); + $route->login('Banned', 'password_0'); + $I->canSeeResponseContainsJson([ + 'success' => false, + 'errors' => [ + 'login' => 'error.account_banned', + ], + ]); + } + } diff --git a/api/tests/functional/LogoutCest.php b/api/tests/functional/LogoutCest.php index f0a0b69..922c1b4 100644 --- a/api/tests/functional/LogoutCest.php +++ b/api/tests/functional/LogoutCest.php @@ -1,19 +1,28 @@ amAuthenticated(); - $route->logout(); + /** + * @dataProvider getLogoutCases + */ + public function logout(FunctionalTester $I, Example $example) { + $I->amAuthenticated($example[0]); + $I->sendPOST('/api/authentication/logout'); $I->canSeeResponseContainsJson([ 'success' => true, ]); } + protected function getLogoutCases() { + yield 'active account' => ['admin']; + yield 'account that not accepted the rules' => ['Veleyaba']; + yield 'account marked for deleting' => ['DeletedAccount']; + } + } diff --git a/api/tests/functional/accounts/DeleteCest.php b/api/tests/functional/accounts/DeleteCest.php new file mode 100644 index 0000000..dd0725e --- /dev/null +++ b/api/tests/functional/accounts/DeleteCest.php @@ -0,0 +1,89 @@ +amAuthenticated(); + $I->sendDELETE("/api/v1/accounts/{$id}", [ + 'password' => 'password_0', + ]); + $I->canSeeResponseCodeIs(200); + $I->canSeeResponseContainsJson([ + 'success' => true, + ]); + + $I->sendGET("/api/v1/accounts/{$id}"); + $I->canSeeResponseCodeIs(200); + $I->canSeeResponseContainsJson([ + 'isDeleted' => true, + ]); + } + + public function deleteMyAccountWithNotAcceptedRules(FunctionalTester $I) { + $id = $I->amAuthenticated('Veleyaba'); + $I->sendDELETE("/api/v1/accounts/{$id}", [ + 'password' => 'password_0', + ]); + $I->canSeeResponseCodeIs(200); + $I->canSeeResponseContainsJson([ + 'success' => true, + ]); + + $I->sendGET("/api/v1/accounts/{$id}"); + $I->canSeeResponseCodeIs(200); + $I->canSeeResponseContainsJson([ + 'isDeleted' => true, + 'shouldAcceptRules' => true, + ]); + } + + public function deleteMyAccountWithInvalidPassword(FunctionalTester $I) { + $id = $I->amAuthenticated(); + $I->sendDELETE("/api/v1/accounts/{$id}", [ + 'password' => 'invalid_password', + ]); + $I->canSeeResponseCodeIs(200); + $I->canSeeResponseContainsJson([ + 'success' => false, + 'errors' => [ + 'password' => 'error.password_incorrect', + ], + ]); + } + + public function deleteAlreadyDeletedAccount(FunctionalTester $I) { + $id = $I->amAuthenticated('DeletedAccount'); + $I->sendDELETE("/api/v1/accounts/{$id}", [ + 'password' => 'password_0', + ]); + $I->canSeeResponseCodeIs(200); + $I->canSeeResponseContainsJson([ + 'success' => false, + 'errors' => [ + 'account' => 'error.account_already_deleted', + ], + ]); + } + + public function deleteNotMyAccount(FunctionalTester $I) { + $I->amAuthenticated(); + + $I->sendDELETE('/api/v1/accounts/2', [ + 'password' => 'password_0', + ]); + $I->canSeeResponseCodeIs(403); + $I->canSeeResponseContainsJson([ + 'name' => 'Forbidden', + 'message' => 'You are not allowed to perform this action.', + 'code' => 0, + 'status' => 403, + ]); + } + +} diff --git a/api/tests/functional/accounts/GetCest.php b/api/tests/functional/accounts/GetCest.php index 64c5f41..31d9ad6 100644 --- a/api/tests/functional/accounts/GetCest.php +++ b/api/tests/functional/accounts/GetCest.php @@ -1,4 +1,6 @@ route = new AccountsRoute($I); @@ -29,6 +28,7 @@ class GetCest { 'email' => 'admin@ely.by', 'lang' => 'en', 'isActive' => true, + 'isDeleted' => false, 'hasMojangUsernameCollision' => false, 'shouldAcceptRules' => false, 'elyProfileLink' => 'http://ely.by/u1', @@ -51,6 +51,7 @@ class GetCest { 'email' => 'admin@ely.by', 'lang' => 'en', 'isActive' => true, + 'isDeleted' => false, 'hasMojangUsernameCollision' => false, 'shouldAcceptRules' => false, 'elyProfileLink' => 'http://ely.by/u1', @@ -72,6 +73,7 @@ class GetCest { 'isOtpEnabled' => false, 'lang' => 'en', 'isActive' => true, + 'isDeleted' => false, 'hasMojangUsernameCollision' => false, 'shouldAcceptRules' => true, 'elyProfileLink' => 'http://ely.by/u9', @@ -79,6 +81,19 @@ class GetCest { $I->canSeeResponseJsonMatchesJsonPath('$.passwordChangedAt'); } + public function testGetInfoFromAccountMarkedForDeleting(FunctionalTester $I) { + // We're setting up a known expired token + $id = $I->amAuthenticated('DeletedAccount'); + + $I->sendGET("/api/v1/accounts/{$id}"); + $I->canSeeResponseCodeIs(200); + $I->canSeeResponseContainsJson([ + 'id' => $id, + 'isActive' => true, + 'isDeleted' => true, + ]); + } + public function testGetInfoWithExpiredToken(FunctionalTester $I) { // We're setting up a known expired token $I->amBearerAuthenticated( diff --git a/api/tests/functional/accounts/RestoreCest.php b/api/tests/functional/accounts/RestoreCest.php new file mode 100644 index 0000000..bc530b0 --- /dev/null +++ b/api/tests/functional/accounts/RestoreCest.php @@ -0,0 +1,51 @@ +amAuthenticated('DeletedAccount'); + $I->sendPOST("/api/v1/accounts/{$id}/restore"); + $I->canSeeResponseCodeIs(200); + $I->canSeeResponseContainsJson([ + 'success' => true, + ]); + + $I->sendGET("/api/v1/accounts/{$id}"); + $I->canSeeResponseCodeIs(200); + $I->canSeeResponseContainsJson([ + 'isDeleted' => false, + ]); + } + + public function restoreNotDeletedAccount(FunctionalTester $I) { + $id = $I->amAuthenticated(); + $I->sendPOST("/api/v1/accounts/{$id}/restore"); + $I->canSeeResponseCodeIs(200); + $I->canSeeResponseContainsJson([ + 'success' => false, + 'errors' => [ + 'account' => 'error.account_not_deleted', + ], + ]); + } + + public function restoreNotMyAccount(FunctionalTester $I) { + $I->amAuthenticated('DeletedAccount'); + + $I->sendPOST('/api/v1/accounts/1/restore'); + $I->canSeeResponseCodeIs(403); + $I->canSeeResponseContainsJson([ + 'name' => 'Forbidden', + 'message' => 'You are not allowed to perform this action.', + 'code' => 0, + 'status' => 403, + ]); + } + +} diff --git a/api/tests/functional/authserver/AuthorizationCest.php b/api/tests/functional/authserver/AuthorizationCest.php index 0d4f652..110e6bf 100644 --- a/api/tests/functional/authserver/AuthorizationCest.php +++ b/api/tests/functional/authserver/AuthorizationCest.php @@ -97,6 +97,20 @@ class AuthorizationCest { ]); } + public function deletedAccount(FunctionalTester $I) { + $I->wantTo('authenticate in account marked for deletion'); + $I->sendPOST('/api/authserver/authentication/authenticate', [ + 'username' => 'DeletedAccount', + 'password' => 'password_0', + 'clientToken' => Uuid::uuid4()->toString(), + ]); + $I->canSeeResponseCodeIs(401); + $I->canSeeResponseContainsJson([ + 'error' => 'ForbiddenOperationException', + 'errorMessage' => 'Invalid credentials. Invalid nickname or password.', + ]); + } + public function bannedAccount(FunctionalTester $I) { $I->wantTo('authenticate in suspended account'); $I->sendPOST('/api/authserver/authentication/authenticate', [ diff --git a/api/tests/functional/authserver/RefreshCest.php b/api/tests/functional/authserver/RefreshCest.php index 2f65350..9323405 100644 --- a/api/tests/functional/authserver/RefreshCest.php +++ b/api/tests/functional/authserver/RefreshCest.php @@ -93,6 +93,19 @@ class RefreshCest { ]); } + public function refreshTokenFromDeletedUser(AuthserverSteps $I) { + $I->wantTo('refresh token from account marked for deletion'); + $I->sendPOST('/api/authserver/authentication/refresh', [ + 'accessToken' => '239ba889-7020-4383-8d99-cd8c8aab4a2f', + 'clientToken' => '47443658-4ff8-45e7-b33e-dc8915ab6421', + ]); + $I->canSeeResponseCodeIs(401); + $I->canSeeResponseContainsJson([ + 'error' => 'ForbiddenOperationException', + 'errorMessage' => 'Invalid token.', + ]); + } + public function refreshTokenFromBannedUser(AuthserverSteps $I) { $I->wantTo('refresh token from suspended account'); $I->sendPOST('/api/authserver/authentication/refresh', [ diff --git a/api/tests/functional/authserver/ValidateCest.php b/api/tests/functional/authserver/ValidateCest.php index e374257..93e5ff8 100644 --- a/api/tests/functional/authserver/ValidateCest.php +++ b/api/tests/functional/authserver/ValidateCest.php @@ -79,4 +79,16 @@ class ValidateCest { ]); } + public function credentialsFromBannedAccount(AuthserverSteps $I) { + $I->wantTo('get error on expired legacy accessToken'); + $I->sendPOST('/api/authserver/authentication/validate', [ + 'accessToken' => '239ba889-7020-4383-8d99-cd8c8aab4a2f', + ]); + $I->canSeeResponseCodeIs(401); + $I->canSeeResponseContainsJson([ + 'error' => 'ForbiddenOperationException', + 'errorMessage' => 'Invalid token.', + ]); + } + } diff --git a/api/tests/functional/mojang/UsernameToUuidCest.php b/api/tests/functional/mojang/UsernameToUuidCest.php index 59a0756..54c0385 100644 --- a/api/tests/functional/mojang/UsernameToUuidCest.php +++ b/api/tests/functional/mojang/UsernameToUuidCest.php @@ -58,6 +58,13 @@ class UsernameToUuidCest { $I->canSeeResponseEquals(''); } + public function getUuidForDeletedAccount(FunctionalTester $I) { + $I->wantTo('get uuid for account that marked for deleting'); + $this->route->usernameToUuid('DeletedAccount'); + $I->canSeeResponseCodeIs(204); + $I->canSeeResponseEquals(''); + } + public function nonPassedUsername(FunctionalTester $I) { $I->wantTo('get 404 on not passed username'); $this->route->usernameToUuid(''); diff --git a/api/tests/functional/mojang/UsernamesToUuidsCest.php b/api/tests/functional/mojang/UsernamesToUuidsCest.php index 0f807b6..174c8c4 100644 --- a/api/tests/functional/mojang/UsernamesToUuidsCest.php +++ b/api/tests/functional/mojang/UsernamesToUuidsCest.php @@ -42,7 +42,7 @@ class UsernamesToUuidsCest { public function getUuidsByPartialNonexistentUsernames(FunctionalTester $I) { $I->wantTo('get uuids by few usernames and some nonexistent'); - $this->route->uuidsByUsernames(['Admin', 'not-exists-user']); + $this->route->uuidsByUsernames(['Admin', 'DeletedAccount', 'not-exists-user']); $I->canSeeResponseCodeIs(200); $I->canSeeResponseIsJson(); $I->canSeeResponseContainsJson([ @@ -51,6 +51,8 @@ class UsernamesToUuidsCest { 'name' => 'Admin', ], ]); + $I->cantSeeResponseJsonMatchesJsonPath('$.[?(@.name="DeletedAccount")]'); + $I->cantSeeResponseJsonMatchesJsonPath('$.[?(@.name="not-exists-user")]'); } public function passAllNonexistentUsernames(FunctionalTester $I) { diff --git a/api/tests/functional/mojang/UuidToUsernamesHistoryCest.php b/api/tests/functional/mojang/UuidToUsernamesHistoryCest.php index c7f5ab2..7b580a3 100644 --- a/api/tests/functional/mojang/UuidToUsernamesHistoryCest.php +++ b/api/tests/functional/mojang/UuidToUsernamesHistoryCest.php @@ -55,6 +55,13 @@ class UuidToUsernamesHistoryCest { $I->canSeeResponseEquals(''); } + public function passUuidOfDeletedAccount(FunctionalTester $I) { + $I->wantTo('get username by passing uuid of the account marked for deleting'); + $this->route->usernamesByUuid('6383de63-8f85-4ed5-92b7-5401a1fa68cd'); + $I->canSeeResponseCodeIs(204); + $I->canSeeResponseEquals(''); + } + public function passWrongUuidFormat(FunctionalTester $I) { $I->wantTo('call profile route with invalid uuid string'); $this->route->usernamesByUuid('bla-bla-bla'); diff --git a/api/tests/functional/oauth/AuthCodeCest.php b/api/tests/functional/oauth/AuthCodeCest.php index b5b9fb8..425ba23 100644 --- a/api/tests/functional/oauth/AuthCodeCest.php +++ b/api/tests/functional/oauth/AuthCodeCest.php @@ -195,4 +195,15 @@ class AuthCodeCest { $I->canSeeResponseJsonMatchesJsonPath('$.redirectUri'); } + public function finalizeByAccountMarkedForDeletion(FunctionalTester $I) { + $I->amAuthenticated('DeletedAccount'); + $I->sendPOST('/api/oauth2/v1/complete?' . http_build_query([ + 'client_id' => 'ely', + 'redirect_uri' => 'http://ely.by', + 'response_type' => 'code', + 'scope' => 'minecraft_server_session', + ]), ['accept' => true]); + $I->canSeeResponseCodeIs(403); + } + } diff --git a/api/tests/functional/sessionserver/JoinCest.php b/api/tests/functional/sessionserver/JoinCest.php index 5219187..76391dd 100644 --- a/api/tests/functional/sessionserver/JoinCest.php +++ b/api/tests/functional/sessionserver/JoinCest.php @@ -137,6 +137,19 @@ class JoinCest { ]); } + public function joinByAccountMarkedForDeletion(FunctionalTester $I) { + $this->route->join([ + 'accessToken' => '239ba889-7020-4383-8d99-cd8c8aab4a2f', + 'selectedProfile' => '6383de63-8f85-4ed5-92b7-5401a1fa68cd', + 'serverId' => uuid(), + ]); + $I->canSeeResponseCodeIs(401); + $I->canSeeResponseContainsJson([ + 'error' => 'ForbiddenOperationException', + 'errorMessage' => 'Invalid credentials', + ]); + } + private function expectSuccessResponse(FunctionalTester $I) { $I->seeResponseCodeIs(200); $I->seeResponseIsJson(); diff --git a/api/tests/functional/sessionserver/JoinLegacyCest.php b/api/tests/functional/sessionserver/JoinLegacyCest.php index 17dfeba..2aebd04 100644 --- a/api/tests/functional/sessionserver/JoinLegacyCest.php +++ b/api/tests/functional/sessionserver/JoinLegacyCest.php @@ -106,6 +106,17 @@ class JoinLegacyCest { $I->canSeeResponseContains('credentials can not be null.'); } + public function joinByAccountMarkedForDeletion(FunctionalTester $I) { + $I->wantTo('join to some server by legacy protocol with nil accessToken and selectedProfile'); + $this->route->joinLegacy([ + 'sessionId' => 'token:239ba889-7020-4383-8d99-cd8c8aab4a2f:6383de63-8f85-4ed5-92b7-5401a1fa68cd', + 'user' => 'DeletedAccount', + 'serverId' => uuid(), + ]); + $I->canSeeResponseCodeIs(401); + $I->canSeeResponseContains('Ely.by authorization required'); + } + private function expectSuccessResponse(FunctionalTester $I) { $I->seeResponseCodeIs(200); $I->canSeeResponseEquals('OK'); diff --git a/api/tests/functional/sessionserver/ProfileCest.php b/api/tests/functional/sessionserver/ProfileCest.php index e197351..f152304 100644 --- a/api/tests/functional/sessionserver/ProfileCest.php +++ b/api/tests/functional/sessionserver/ProfileCest.php @@ -58,4 +58,15 @@ class ProfileCest { ]); } + public function getProfileOfAccountMarkedForDeletion(FunctionalTester $I) { + $this->route->profile('6383de63-8f85-4ed5-92b7-5401a1fa68cd'); + $I->canSeeResponseCodeIs(401); + $I->canSeeResponseIsJson(); + $I->seeResponseIsJson(); + $I->canSeeResponseContainsJson([ + 'error' => 'ForbiddenOperationException', + 'errorMessage' => 'Invalid uuid.', + ]); + } + } diff --git a/api/tests/unit/modules/accounts/models/DeleteAccountFormTest.php b/api/tests/unit/modules/accounts/models/DeleteAccountFormTest.php new file mode 100644 index 0000000..90313f0 --- /dev/null +++ b/api/tests/unit/modules/accounts/models/DeleteAccountFormTest.php @@ -0,0 +1,90 @@ + AccountFixture::class, + ]; + } + + public function _before(): void { + parent::_before(); + + $this->queue = $this->createMock(Queue::class); + Yii::$app->set('queue', $this->queue); + } + + public function testPerformAction() { + /** @var Account $account */ + $account = $this->tester->grabFixture('accounts', 'admin'); + $this->queue + ->expects($this->once()) + ->method('delay') + ->with($this->equalToWithDelta(60 * 60 * 24 * 7, 5)) + ->willReturnSelf(); + $this->queue + ->expects($this->exactly(2)) + ->method('push') + ->withConsecutive( + [$this->callback(function(CreateWebHooksDeliveries $task) use ($account): bool { + $this->assertSame($account->id, $task->payloads['id']); + return true; + })], + [$this->callback(function(DeleteAccount $task) use ($account): bool { + $obj = new ReflectionObject($task); + $property = $obj->getProperty('accountId'); + $property->setAccessible(true); + $this->assertSame($account->id, $property->getValue($task)); + + return true; + })], + ); + + $model = new DeleteAccountForm($account, [ + 'password' => 'password_0', + ]); + $this->assertTrue($model->performAction()); + $this->assertSame(Account::STATUS_DELETED, $account->status); + $this->assertEqualsWithDelta(time(), $account->deleted_at, 5); + } + + public function testPerformActionWithInvalidPassword() { + /** @var Account $account */ + $account = $this->tester->grabFixture('accounts', 'admin'); + $model = new DeleteAccountForm($account, [ + 'password' => 'invalid password', + ]); + $this->assertFalse($model->performAction()); + $this->assertSame(['password' => ['error.password_incorrect']], $model->getErrors()); + } + + public function testPerformActionForAlreadyDeletedAccount() { + /** @var Account $account */ + $account = $this->tester->grabFixture('accounts', 'deleted-account'); + $model = new DeleteAccountForm($account, [ + 'password' => 'password_0', + ]); + $this->assertFalse($model->performAction()); + $this->assertSame(['account' => ['error.account_already_deleted']], $model->getErrors()); + } + +} diff --git a/api/tests/unit/modules/accounts/models/RestoreAccountFormTest.php b/api/tests/unit/modules/accounts/models/RestoreAccountFormTest.php new file mode 100644 index 0000000..bd30b51 --- /dev/null +++ b/api/tests/unit/modules/accounts/models/RestoreAccountFormTest.php @@ -0,0 +1,61 @@ + AccountFixture::class, + ]; + } + + public function _before(): void { + parent::_before(); + + $this->queue = $this->createMock(Queue::class); + Yii::$app->set('queue', $this->queue); + } + + public function testPerformAction() { + /** @var Account $account */ + $account = $this->tester->grabFixture('accounts', 'deleted-account'); + $this->queue + ->expects($this->once()) + ->method('push') + ->withConsecutive( + [$this->callback(function(CreateWebHooksDeliveries $task) use ($account): bool { + $this->assertSame($account->id, $task->payloads['id']); + return true; + })], + ); + + $model = new RestoreAccountForm($account); + $this->assertTrue($model->performAction()); + $this->assertSame(Account::STATUS_ACTIVE, $account->status); + $this->assertNull($account->deleted_at); + } + + public function testPerformActionForNotDeletedAccount() { + /** @var Account $account */ + $account = $this->tester->grabFixture('accounts', 'admin'); + $model = new RestoreAccountForm($account); + $this->assertFalse($model->performAction()); + $this->assertSame(['account' => ['error.account_not_deleted']], $model->getErrors()); + } + +} diff --git a/api/tests/unit/modules/internal/models/BanFormTest.php b/api/tests/unit/modules/internal/models/BanAccountFormTest.php similarity index 73% rename from api/tests/unit/modules/internal/models/BanFormTest.php rename to api/tests/unit/modules/internal/models/BanAccountFormTest.php index 8dcd328..8bc6ea1 100644 --- a/api/tests/unit/modules/internal/models/BanFormTest.php +++ b/api/tests/unit/modules/internal/models/BanAccountFormTest.php @@ -1,4 +1,6 @@ getMockBuilder(Account::class) - ->setMethods(['save']) - ->getMock(); - - $account->expects($this->once()) - ->method('save') - ->willReturn(true); + $account = $this->createPartialMock(Account::class, ['save']); + $account->expects($this->once())->method('save')->willReturn(true); + $account->id = 123; $model = new BanAccountForm($account); $this->assertTrue($model->performAction()); @@ -39,7 +38,10 @@ class BanFormTest extends TestCase { /** @var ClearAccountSessions $job */ $job = $this->tester->grabLastQueuedJob(); $this->assertInstanceOf(ClearAccountSessions::class, $job); - $this->assertSame($job->accountId, $account->id); + $obj = new ReflectionObject($job); + $property = $obj->getProperty('accountId'); + $property->setAccessible(true); + $this->assertSame(123, $property->getValue($job)); } } diff --git a/api/tests/unit/rbac/rules/AccountOwnerTest.php b/api/tests/unit/rbac/rules/AccountOwnerTest.php index 5504cd1..4bc9410 100644 --- a/api/tests/unit/rbac/rules/AccountOwnerTest.php +++ b/api/tests/unit/rbac/rules/AccountOwnerTest.php @@ -39,14 +39,25 @@ class AccountOwnerTest extends TestCase { Yii::$app->user->setIdentity($identity); + // Assert that account id matches $this->assertFalse($rule->execute('token', $item, ['accountId' => 2])); $this->assertFalse($rule->execute('token', $item, ['accountId' => '2'])); $this->assertTrue($rule->execute('token', $item, ['accountId' => 1])); $this->assertTrue($rule->execute('token', $item, ['accountId' => '1'])); + + // Check accepted latest rules $account->rules_agreement_version = null; $this->assertFalse($rule->execute('token', $item, ['accountId' => 1])); $this->assertTrue($rule->execute('token', $item, ['accountId' => 1, 'optionalRules' => true])); $account->rules_agreement_version = LATEST_RULES_VERSION; + $this->assertTrue($rule->execute('token', $item, ['accountId' => 1])); + + // Check deleted account behavior + $account->status = Account::STATUS_DELETED; + $this->assertFalse($rule->execute('token', $item, ['accountId' => 1])); + $this->assertTrue($rule->execute('token', $item, ['accountId' => 1, 'allowDeleted' => true])); + + // Banned account should always be not allowed $account->status = Account::STATUS_BANNED; $this->assertFalse($rule->execute('token', $item, ['accountId' => 1])); $this->assertFalse($rule->execute('token', $item, ['accountId' => 1, 'optionalRules' => true])); diff --git a/api/validators/PasswordRequiredValidator.php b/api/validators/PasswordRequiredValidator.php index d46a1bf..47e1465 100644 --- a/api/validators/PasswordRequiredValidator.php +++ b/api/validators/PasswordRequiredValidator.php @@ -1,20 +1,18 @@ account instanceof Account) { - throw new InvalidConfigException('account should be instance of ' . Account::class); - } - $this->user = Instance::ensure($this->user, User::class); } diff --git a/common/models/Account.php b/common/models/Account.php index c5cbc12..d949c65 100644 --- a/common/models/Account.php +++ b/common/models/Account.php @@ -3,8 +3,11 @@ declare(strict_types=1); namespace common\models; +use Carbon\Carbon; use common\components\UserPass; use common\tasks\CreateWebHooksDeliveries; +use DateInterval; +use Webmozart\Assert\Assert; use Yii; use yii\base\InvalidConfigException; use yii\behaviors\TimestampBehavior; @@ -14,21 +17,22 @@ use const common\LATEST_RULES_VERSION; /** * Fields: - * @property int $id - * @property string $uuid - * @property string $username - * @property string $email - * @property string $password_hash - * @property int $password_hash_strategy - * @property string $lang - * @property int $status - * @property int $rules_agreement_version - * @property string $registration_ip - * @property string $otp_secret - * @property int $is_otp_enabled - * @property int $created_at - * @property int $updated_at - * @property int $password_changed_at + * @property int $id + * @property string $uuid + * @property string $username + * @property string $email + * @property string $password_hash + * @property int $password_hash_strategy + * @property string $lang + * @property int $status + * @property int|null $rules_agreement_version + * @property string|null $registration_ip + * @property string|null $otp_secret + * @property int $is_otp_enabled + * @property int $created_at + * @property int $updated_at + * @property int $password_changed_at + * @property int|null $deleted_at shows the time, when the account was marked as deleted * * Getters-setters: * @property-write string $password plain user's password @@ -55,8 +59,10 @@ class Account extends ActiveRecord { public const PASS_HASH_STRATEGY_OLD_ELY = 0; public const PASS_HASH_STRATEGY_YII2 = 1; + public const ACCOUNT_DELETION_DELAY = 'P7D'; + public static function tableName(): string { - return '{{%accounts}}'; + return 'accounts'; } public static function find(): AccountQuery { @@ -153,17 +159,24 @@ class Account extends ActiveRecord { return $this->registration_ip === null ? null : inet_ntop($this->registration_ip); } - public function afterSave($insert, $changedAttributes) { + public function getDeleteAt(): Carbon { + Assert::notNull($this->deleted_at, 'This method should not be called on not deleted records'); + return Carbon::createFromTimestamp($this->deleted_at)->add(new DateInterval(Account::ACCOUNT_DELETION_DELAY)); + } + + public function afterSave($insert, $changedAttributes): void { parent::afterSave($insert, $changedAttributes); if ($insert) { return; } - $meaningfulFields = ['username', 'email', 'uuid', 'status', 'lang']; - $meaningfulChangedAttributes = array_filter($changedAttributes, function(string $key) use ($meaningfulFields) { - return in_array($key, $meaningfulFields, true); - }, ARRAY_FILTER_USE_KEY); + $meaningfulFields = ['username', 'email', 'uuid', 'status', 'lang', 'deleted_at']; + $meaningfulChangedAttributes = array_filter( + $changedAttributes, + fn(string $key): bool => in_array($key, $meaningfulFields, true), + ARRAY_FILTER_USE_KEY, + ); if (empty($meaningfulChangedAttributes)) { return; } @@ -171,4 +184,9 @@ class Account extends ActiveRecord { Yii::$app->queue->push(CreateWebHooksDeliveries::createAccountEdit($this, $meaningfulChangedAttributes)); } + public function afterDelete(): void { + parent::afterDelete(); + Yii::$app->queue->push(CreateWebHooksDeliveries::createAccountDeletion($this)); + } + } diff --git a/common/models/AccountQuery.php b/common/models/AccountQuery.php index c94a351..bef3002 100644 --- a/common/models/AccountQuery.php +++ b/common/models/AccountQuery.php @@ -10,6 +10,10 @@ use yii\db\ActiveQuery; */ class AccountQuery extends ActiveQuery { + public function excludeDeleted(): self { + return $this->andWhere(['NOT', ['status' => Account::STATUS_DELETED]]); + } + public function andWhereLogin(string $login): self { return $this->andWhere([$this->getLoginAttribute($login) => $login]); } diff --git a/common/models/UsernameHistory.php b/common/models/UsernameHistory.php index 177ce3a..3e8912e 100644 --- a/common/models/UsernameHistory.php +++ b/common/models/UsernameHistory.php @@ -1,29 +1,32 @@ TimestampBehavior::class, @@ -33,19 +36,17 @@ class UsernameHistory extends ActiveRecord { ]; } - public function rules() { - return []; - } - - public function getAccount() { + public function getAccount(): ActiveQuery { return $this->hasOne(Account::class, ['id' => 'account_id']); } /** + * Find the username after the current of the account. + * * @param int $afterTime * @return UsernameHistory|null */ - public function findNext(int $afterTime = null): ?self { + public function findNextOwnerUsername(int $afterTime = null): ?self { return self::find() ->andWhere(['account_id' => $this->account_id]) ->andWhere(['>', 'applied_in', $afterTime ?: $this->applied_in]) diff --git a/common/tasks/ClearAccountSessions.php b/common/tasks/ClearAccountSessions.php index 86a5043..0dbe7c5 100644 --- a/common/tasks/ClearAccountSessions.php +++ b/common/tasks/ClearAccountSessions.php @@ -7,15 +7,12 @@ use common\models\Account; use Yii; use yii\queue\RetryableJobInterface; -class ClearAccountSessions implements RetryableJobInterface { +final class ClearAccountSessions implements RetryableJobInterface { - public $accountId; + private int $accountId; - public static function createFromAccount(Account $account): self { - $result = new static(); - $result->accountId = $account->id; - - return $result; + public function __construct(int $accountId) { + $this->accountId = $accountId; } /** @@ -40,23 +37,23 @@ class ClearAccountSessions implements RetryableJobInterface { * @throws \Exception */ public function execute($queue): void { - $account = Account::findOne($this->accountId); + $account = Account::findOne(['id' => $this->accountId]); if ($account === null) { return; } + /** @var \common\models\AccountSession $authSession */ foreach ($account->getSessions()->each(100, Yii::$app->unbufferedDb) as $authSession) { - /** @var \common\models\AccountSession $authSession */ $authSession->delete(); } + /** @var \common\models\MinecraftAccessKey $key */ foreach ($account->getMinecraftAccessKeys()->each(100, Yii::$app->unbufferedDb) as $key) { - /** @var \common\models\MinecraftAccessKey $key */ $key->delete(); } + /** @var \common\models\OauthSession $oauthSession */ foreach ($account->getOauthSessions()->each(100, Yii::$app->unbufferedDb) as $oauthSession) { - /** @var \common\models\OauthSession $oauthSession */ $oauthSession->delete(); } } diff --git a/common/tasks/ClearOauthSessions.php b/common/tasks/ClearOauthSessions.php index 4a54733..abb18de 100644 --- a/common/tasks/ClearOauthSessions.php +++ b/common/tasks/ClearOauthSessions.php @@ -7,26 +7,22 @@ use common\models\OauthClient; use Yii; use yii\queue\RetryableJobInterface; -class ClearOauthSessions implements RetryableJobInterface { +final class ClearOauthSessions implements RetryableJobInterface { + + public string $clientId; /** - * @var int + * @var int|null unix timestamp, that allows to limit this task to clear only some old sessions */ - public $clientId; + public ?int $notSince; - /** - * @var int unix timestamp, that allows to limit this task to clear only some old sessions - */ - public $notSince; + public function __construct(string $clientId, int $notSince = null) { + $this->clientId = $clientId; + $this->notSince = $notSince; + } public static function createFromOauthClient(OauthClient $client, int $notSince = null): self { - $result = new static(); - $result->clientId = $client->id; - if ($notSince !== null) { - $result->notSince = $notSince; - } - - return $result; + return new self($client->id, $notSince); } public function getTtr(): int { diff --git a/common/tasks/CreateWebHooksDeliveries.php b/common/tasks/CreateWebHooksDeliveries.php index 8ba6fef..ffc7e11 100644 --- a/common/tasks/CreateWebHooksDeliveries.php +++ b/common/tasks/CreateWebHooksDeliveries.php @@ -8,39 +8,46 @@ use common\models\WebHook; use Yii; use yii\queue\RetryableJobInterface; -class CreateWebHooksDeliveries implements RetryableJobInterface { +final class CreateWebHooksDeliveries implements RetryableJobInterface { - /** - * @var string - */ - public $type; + public string $type; - /** - * @var array - */ - public $payloads; + public array $payloads; + + public function __construct(string $type, array $payloads) { + $this->type = $type; + $this->payloads = $payloads; + } public static function createAccountEdit(Account $account, array $changedAttributes): self { - $result = new static(); - $result->type = 'account.edit'; - $result->payloads = [ + return new static('account.edit', [ 'id' => $account->id, 'uuid' => $account->uuid, 'username' => $account->username, 'email' => $account->email, 'lang' => $account->lang, 'isActive' => $account->status === Account::STATUS_ACTIVE, + 'isDeleted' => $account->status === Account::STATUS_DELETED, 'registered' => date('c', (int)$account->created_at), 'changedAttributes' => $changedAttributes, - ]; + ]); + } - return $result; + public static function createAccountDeletion(Account $account): self { + return new static('account.deletion', [ + 'id' => $account->id, + 'uuid' => $account->uuid, + 'username' => $account->username, + 'email' => $account->email, + 'registered' => date('c', (int)$account->created_at), + 'deleted' => date('c', (int)$account->deleted_at), + ]); } /** * @return int time to reserve in seconds */ - public function getTtr() { + public function getTtr(): int { return 10; } @@ -50,14 +57,14 @@ class CreateWebHooksDeliveries implements RetryableJobInterface { * * @return bool */ - public function canRetry($attempt, $error) { + public function canRetry($attempt, $error): bool { return true; } /** * @param \yii\queue\Queue $queue which pushed and is handling the job */ - public function execute($queue) { + public function execute($queue): void { /** @var WebHook[] $targets */ $targets = WebHook::find() ->joinWith('events e', false) diff --git a/common/tasks/DeleteAccount.php b/common/tasks/DeleteAccount.php new file mode 100644 index 0000000..405716e --- /dev/null +++ b/common/tasks/DeleteAccount.php @@ -0,0 +1,62 @@ +accountId = $accountId; + } + + public function getTtr(): int { + return PHP_INT_MAX; // Let it work as long as it needs to + } + + public function canRetry($attempt, $error): bool { + return true; + } + + /** + * @param \yii\queue\Queue $queue + * @throws \Throwable + */ + public function execute($queue): void { + $account = Account::findOne(['id' => $this->accountId]); + if ($account === null || $this->shouldAccountBeDeleted($account) === false) { + return; + } + + $transaction = Yii::$app->db->beginTransaction(); + + (new ClearAccountSessions($account->id))->execute($queue); + foreach ($account->oauthClients as $oauthClient) { + (new ClearOauthSessions($oauthClient->id))->execute($queue); + } + + /** @var \common\models\EmailActivation $emailActivation */ + foreach ($account->getEmailActivations()->each(100, Yii::$app->unbufferedDb) as $emailActivation) { + $emailActivation->delete(); + } + + /** @var \common\models\UsernameHistory $usernameHistoryEntry */ + foreach ($account->getUsernameHistory()->each(100, Yii::$app->unbufferedDb) as $usernameHistoryEntry) { + $usernameHistoryEntry->delete(); + } + + $account->delete(); + + $transaction->commit(); + } + + private function shouldAccountBeDeleted(Account $account): bool { + return $account->status === Account::STATUS_DELETED && $account->getDeleteAt()->subSecond()->isPast(); + } + +} diff --git a/common/tests/fixtures/data/accounts.php b/common/tests/fixtures/data/accounts.php index 4b8bfed..0031294 100644 --- a/common/tests/fixtures/data/accounts.php +++ b/common/tests/fixtures/data/accounts.php @@ -13,6 +13,7 @@ return [ 'created_at' => 1451775316, 'updated_at' => 1451775316, 'password_changed_at' => 1451775316, + 'deleted_at' => null, ], 'user-with-old-password-type' => [ 'id' => 2, @@ -27,6 +28,7 @@ return [ 'created_at' => 1385225069, 'updated_at' => 1385225069, 'password_changed_at' => 1385225069, + 'deleted_at' => null, ], 'not-activated-account' => [ 'id' => 3, @@ -41,6 +43,7 @@ return [ 'created_at' => 1453146616, 'updated_at' => 1453146616, 'password_changed_at' => 1453146616, + 'deleted_at' => null, ], 'not-activated-account-with-expired-message' => [ 'id' => 4, @@ -55,6 +58,7 @@ return [ 'created_at' => 1457890086, 'updated_at' => 1457890086, 'password_changed_at' => 1457890086, + 'deleted_at' => null, ], 'account-with-fresh-forgot-password-message' => [ 'id' => 5, @@ -69,6 +73,7 @@ return [ 'created_at' => 1462891432, 'updated_at' => 1462891432, 'password_changed_at' => 1462891432, + 'deleted_at' => null, ], 'account-with-expired-forgot-password-message' => [ 'id' => 6, @@ -83,6 +88,7 @@ return [ 'created_at' => 1462891612, 'updated_at' => 1462891612, 'password_changed_at' => 1462891612, + 'deleted_at' => null, ], 'account-with-change-email-init-state' => [ 'id' => 7, @@ -97,6 +103,7 @@ return [ 'created_at' => 1463427287, 'updated_at' => 1463427287, 'password_changed_at' => 1463427287, + 'deleted_at' => null, ], 'account-with-change-email-finish-state' => [ 'id' => 8, @@ -111,6 +118,7 @@ return [ 'created_at' => 1463349615, 'updated_at' => 1463349615, 'password_changed_at' => 1463349615, + 'deleted_at' => null, ], 'account-with-old-rules-version' => [ 'id' => 9, @@ -125,6 +133,7 @@ return [ 'created_at' => 1470499952, 'updated_at' => 1470499952, 'password_changed_at' => 1470499952, + 'deleted_at' => null, ], 'banned-account' => [ 'id' => 10, @@ -139,6 +148,7 @@ return [ 'created_at' => 1472682343, 'updated_at' => 1472682343, 'password_changed_at' => 1472682343, + 'deleted_at' => null, ], 'account-with-usernames-history' => [ 'id' => 11, @@ -153,6 +163,7 @@ return [ 'created_at' => 1474404139, 'updated_at' => 1474404149, 'password_changed_at' => 1474404149, + 'deleted_at' => null, ], 'account-with-otp-secret' => [ 'id' => 12, @@ -169,6 +180,7 @@ return [ 'created_at' => 1485124615, 'updated_at' => 1485124615, 'password_changed_at' => 1485124615, + 'deleted_at' => null, ], 'account-with-enabled-otp' => [ 'id' => 13, @@ -185,6 +197,7 @@ return [ 'created_at' => 1485124685, 'updated_at' => 1485124685, 'password_changed_at' => 1485124685, + 'deleted_at' => null, ], 'account-with-two-oauth-clients' => [ 'id' => 14, @@ -201,5 +214,23 @@ return [ 'created_at' => 1519487320, 'updated_at' => 1519487320, 'password_changed_at' => 1519487320, + 'deleted_at' => null, + ], + 'deleted-account' => [ + 'id' => 15, + 'uuid' => '6383de63-8f85-4ed5-92b7-5401a1fa68cd', + 'username' => 'DeletedAccount', + 'email' => 'deleted-account@ely.by', + 'password_hash' => '$2y$13$2rYkap5T6jG8z/mMK8a3Ou6aZxJcmAaTha6FEuujvHEmybSHRzW5e', # password_0 + 'password_hash_strategy' => \common\models\Account::PASS_HASH_STRATEGY_YII2, + 'lang' => 'ru', + 'status' => \common\models\Account::STATUS_DELETED, + 'rules_agreement_version' => \common\LATEST_RULES_VERSION, + 'otp_secret' => null, + 'is_otp_enabled' => false, + 'created_at' => 1591893532, + 'updated_at' => 1591893532, + 'password_changed_at' => 1591893532, + 'deleted_at' => time(), ], ]; diff --git a/common/tests/fixtures/data/minecraft-access-keys.php b/common/tests/fixtures/data/minecraft-access-keys.php index ca1ee26..4b19960 100644 --- a/common/tests/fixtures/data/minecraft-access-keys.php +++ b/common/tests/fixtures/data/minecraft-access-keys.php @@ -21,4 +21,11 @@ return [ 'created_at' => time() - 10, 'updated_at' => time() - 10, ], + 'deleted-token' => [ + 'access_token' => '239ba889-7020-4383-8d99-cd8c8aab4a2f', + 'client_token' => '47443658-4ff8-45e7-b33e-dc8915ab6421', + 'account_id' => 15, + 'created_at' => time() - 10, + 'updated_at' => time() - 10, + ], ]; diff --git a/common/tests/fixtures/data/usernames-history.php b/common/tests/fixtures/data/usernames-history.php index 26de303..bf6bd33 100644 --- a/common/tests/fixtures/data/usernames-history.php +++ b/common/tests/fixtures/data/usernames-history.php @@ -24,4 +24,10 @@ return [ 'account_id' => 11, 'applied_in' => 1474404143, ], + [ + 'id' => 5, + 'username' => 'DeletedAccount', + 'account_id' => 15, + 'applied_in' => 1591893532, + ], ]; diff --git a/common/tests/unit/models/AccountTest.php b/common/tests/unit/models/AccountTest.php index 4144b42..6bef817 100644 --- a/common/tests/unit/models/AccountTest.php +++ b/common/tests/unit/models/AccountTest.php @@ -133,7 +133,19 @@ class AccountTest extends TestCase { /** @var CreateWebHooksDeliveries $job */ $job = $this->tester->grabLastQueuedJob(); $this->assertInstanceOf(CreateWebHooksDeliveries::class, $job); - $this->assertSame($job->payloads['changedAttributes'], $changedAttributes); + $this->assertSame('account.edit', $job->type); + $this->assertSame($changedAttributes, $job->payloads['changedAttributes']); + } + + public function testAfterDeletePushEvent() { + $account = new Account(); + $account->id = 1; + $account->afterDelete(); + /** @var CreateWebHooksDeliveries $job */ + $job = $this->tester->grabLastQueuedJob(); + $this->assertInstanceOf(CreateWebHooksDeliveries::class, $job); + $this->assertSame('account.deletion', $job->type); + $this->assertSame(1, $job->payloads['id']); } } diff --git a/common/tests/unit/tasks/ClearAccountSessionsTest.php b/common/tests/unit/tasks/ClearAccountSessionsTest.php index 9c4d8bd..3a8fc6f 100644 --- a/common/tests/unit/tasks/ClearAccountSessionsTest.php +++ b/common/tests/unit/tasks/ClearAccountSessionsTest.php @@ -3,7 +3,6 @@ declare(strict_types=1); namespace common\tests\unit\tasks; -use common\models\Account; use common\tasks\ClearAccountSessions; use common\tests\fixtures; use common\tests\unit\TestCase; @@ -23,18 +22,10 @@ class ClearAccountSessionsTest extends TestCase { ]; } - public function testCreateFromAccount() { - $account = new Account(); - $account->id = 123; - $task = ClearAccountSessions::createFromAccount($account); - $this->assertSame(123, $task->accountId); - } - public function testExecute() { /** @var \common\models\Account $bannedAccount */ $bannedAccount = $this->tester->grabFixture('accounts', 'banned-account'); - $task = new ClearAccountSessions(); - $task->accountId = $bannedAccount->id; + $task = new ClearAccountSessions($bannedAccount->id); $task->execute($this->createMock(Queue::class)); $this->assertEmpty($bannedAccount->sessions); $this->assertEmpty($bannedAccount->minecraftAccessKeys); diff --git a/common/tests/unit/tasks/ClearOauthSessionsTest.php b/common/tests/unit/tasks/ClearOauthSessionsTest.php index 60dc01d..a7767f2 100644 --- a/common/tests/unit/tasks/ClearOauthSessionsTest.php +++ b/common/tests/unit/tasks/ClearOauthSessionsTest.php @@ -1,4 +1,6 @@ clientId = 'deleted-oauth-client-with-sessions'; - $task->notSince = 1519510065; + $task = new ClearOauthSessions('deleted-oauth-client-with-sessions', 1519510065); $task->execute($this->createMock(Queue::class)); $this->assertFalse(OauthSession::find()->andWhere(['legacy_id' => 3])->exists()); $this->assertTrue(OauthSession::find()->andWhere(['legacy_id' => 4])->exists()); - $task = new ClearOauthSessions(); - $task->clientId = 'deleted-oauth-client-with-sessions'; + $task = new ClearOauthSessions('deleted-oauth-client-with-sessions'); $task->execute($this->createMock(Queue::class)); $this->assertFalse(OauthSession::find()->andWhere(['legacy_id' => 4])->exists()); - $task = new ClearOauthSessions(); - $task->clientId = 'some-not-exists-client-id'; + $task = new ClearOauthSessions('some-not-exists-client-id'); $task->execute($this->createMock(Queue::class)); } diff --git a/common/tests/unit/tasks/CreateWebHooksDeliveriesTest.php b/common/tests/unit/tasks/CreateWebHooksDeliveriesTest.php index 4fc19fc..a8e5587 100644 --- a/common/tests/unit/tasks/CreateWebHooksDeliveriesTest.php +++ b/common/tests/unit/tasks/CreateWebHooksDeliveriesTest.php @@ -52,9 +52,7 @@ class CreateWebHooksDeliveriesTest extends TestCase { } public function testExecute() { - $task = new CreateWebHooksDeliveries(); - $task->type = 'account.edit'; - $task->payloads = [ + $task = new CreateWebHooksDeliveries('account.edit', [ 'id' => 123, 'uuid' => 'afc8dc7a-4bbf-4d3a-8699-68890088cf84', 'username' => 'mock-username', @@ -68,7 +66,7 @@ class CreateWebHooksDeliveriesTest extends TestCase { 'email' => 'old-email@ely.by', 'status' => 0, ], - ]; + ]); $task->execute($this->createMock(Queue::class)); /** @var DeliveryWebHook[] $tasks */ $tasks = $this->tester->grabQueueJobs(); diff --git a/common/tests/unit/tasks/DeleteAccountTest.php b/common/tests/unit/tasks/DeleteAccountTest.php new file mode 100644 index 0000000..ae6d609 --- /dev/null +++ b/common/tests/unit/tasks/DeleteAccountTest.php @@ -0,0 +1,90 @@ + fixtures\AccountFixture::class, + 'authSessions' => fixtures\AccountSessionFixture::class, + 'emailActivations' => fixtures\EmailActivationFixture::class, + 'minecraftAccessKeys' => fixtures\MinecraftAccessKeyFixture::class, + 'usernamesHistory' => fixtures\UsernameHistoryFixture::class, + 'oauthClients' => fixtures\OauthClientFixture::class, + 'oauthSessions' => fixtures\OauthSessionFixture::class, + ]; + } + + public function testExecute() { + /** @var Account $account */ + $account = $this->tester->grabFixture('accounts', 'admin'); + $account->status = Account::STATUS_DELETED; + $account->deleted_at = time() - 60 * 60 * 24 * 7; + $account->save(); + + $task = new DeleteAccount($account->id); + $task->execute($this->createMock(Queue::class)); + $this->assertEmpty($account->emailActivations); + $this->assertEmpty($account->sessions); + $this->assertEmpty($account->minecraftAccessKeys); + $this->assertEmpty($account->oauthSessions); + $this->assertEmpty($account->usernameHistory); + $this->assertEmpty($account->oauthClients); + $this->assertFalse($account->refresh()); + } + + /** + * When a user restores his account back, the task doesn't removed + * @throws \Throwable + */ + public function testExecuteOnNotDeletedAccount() { + /** @var Account $account */ + $account = $this->tester->grabFixture('accounts', 'admin'); + // By default, this account is active + + $task = new DeleteAccount($account->id); + $task->execute($this->createMock(Queue::class)); + $this->assertNotEmpty($account->emailActivations); + $this->assertNotEmpty($account->sessions); + $this->assertNotEmpty($account->minecraftAccessKeys); + $this->assertNotEmpty($account->oauthSessions); + $this->assertNotEmpty($account->usernameHistory); + $this->assertNotEmpty($account->oauthClients); + $this->assertTrue($account->refresh()); + } + + /** + * User also might delete his account, restore it and delete again. + * For each deletion the job will be created, so assert, that job for restored deleting will not work + * @throws \Throwable + */ + public function testExecuteOnDeletedAccountWhichWasRestoredAndThenDeletedAgain() { + /** @var Account $account */ + $account = $this->tester->grabFixture('accounts', 'admin'); + $account->status = Account::STATUS_DELETED; + $account->deleted_at = time() - 60 * 60 * 24 * 2; + $account->save(); + + $task = new DeleteAccount($account->id); + $task->execute($this->createMock(Queue::class)); + $this->assertNotEmpty($account->emailActivations); + $this->assertNotEmpty($account->sessions); + $this->assertNotEmpty($account->minecraftAccessKeys); + $this->assertNotEmpty($account->oauthSessions); + $this->assertNotEmpty($account->usernameHistory); + $this->assertNotEmpty($account->oauthClients); + $this->assertTrue($account->refresh()); + } + +} diff --git a/console/controllers/RbacController.php b/console/controllers/RbacController.php index 2d0b8a0..1b4846b 100644 --- a/console/controllers/RbacController.php +++ b/console/controllers/RbacController.php @@ -31,6 +31,8 @@ class RbacController extends Controller { $permChangeAccountPassword = $this->createPermission(P::CHANGE_ACCOUNT_PASSWORD); $permChangeAccountEmail = $this->createPermission(P::CHANGE_ACCOUNT_EMAIL); $permManageTwoFactorAuth = $this->createPermission(P::MANAGE_TWO_FACTOR_AUTH); + $permDeleteAccount = $this->createPermission(P::DELETE_ACCOUNT); + $permRestoreAccount = $this->createPermission(P::RESTORE_ACCOUNT); $permBlockAccount = $this->createPermission(P::BLOCK_ACCOUNT); $permCreateOauthClients = $this->createPermission(P::CREATE_OAUTH_CLIENTS); $permViewOauthClients = $this->createPermission(P::VIEW_OAUTH_CLIENTS); @@ -48,6 +50,8 @@ class RbacController extends Controller { $permChangeOwnAccountPassword = $this->createPermission(P::CHANGE_OWN_ACCOUNT_PASSWORD, AccountOwner::class); $permChangeOwnAccountEmail = $this->createPermission(P::CHANGE_OWN_ACCOUNT_EMAIL, AccountOwner::class); $permManageOwnTwoFactorAuth = $this->createPermission(P::MANAGE_OWN_TWO_FACTOR_AUTH, AccountOwner::class); + $permDeleteOwnAccount = $this->createPermission(P::DELETE_OWN_ACCOUNT, AccountOwner::class); + $permRestoreOwnAccount = $this->createPermission(P::RESTORE_OWN_ACCOUNT, AccountOwner::class); $permMinecraftServerSession = $this->createPermission(P::MINECRAFT_SERVER_SESSION); $permViewOwnOauthClients = $this->createPermission(P::VIEW_OWN_OAUTH_CLIENTS, OauthClientOwner::class); $permManageOwnOauthClients = $this->createPermission(P::MANAGE_OWN_OAUTH_CLIENTS, OauthClientOwner::class); @@ -63,6 +67,8 @@ class RbacController extends Controller { $authManager->addChild($permChangeOwnAccountPassword, $permChangeAccountPassword); $authManager->addChild($permChangeOwnAccountEmail, $permChangeAccountEmail); $authManager->addChild($permManageOwnTwoFactorAuth, $permManageTwoFactorAuth); + $authManager->addChild($permDeleteOwnAccount, $permDeleteAccount); + $authManager->addChild($permRestoreOwnAccount, $permRestoreAccount); $authManager->addChild($permViewOwnOauthClients, $permViewOauthClients); $authManager->addChild($permManageOwnOauthClients, $permManageOauthClients); @@ -76,6 +82,8 @@ class RbacController extends Controller { $authManager->addChild($roleAccountsWebUser, $permChangeOwnAccountPassword); $authManager->addChild($roleAccountsWebUser, $permChangeOwnAccountEmail); $authManager->addChild($roleAccountsWebUser, $permManageOwnTwoFactorAuth); + $authManager->addChild($roleAccountsWebUser, $permDeleteOwnAccount); + $authManager->addChild($roleAccountsWebUser, $permRestoreOwnAccount); $authManager->addChild($roleAccountsWebUser, $permCompleteOauthFlow); $authManager->addChild($roleAccountsWebUser, $permCreateOauthClients); $authManager->addChild($roleAccountsWebUser, $permViewOwnOauthClients); diff --git a/console/migrations/m200611_123017_accounts_deletion.php b/console/migrations/m200611_123017_accounts_deletion.php new file mode 100644 index 0000000..e9d651b --- /dev/null +++ b/console/migrations/m200611_123017_accounts_deletion.php @@ -0,0 +1,15 @@ +addColumn('accounts', 'deleted_at', $this->integer(11)->unsigned()); + } + + public function safeDown() { + $this->dropColumn('accounts', 'deleted_at'); + } + +} From 17f1794a4efd61d7d8cfd45e9aebfaf32665a090 Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Sat, 13 Jun 2020 01:55:02 +0300 Subject: [PATCH 2/3] 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', ]; } From fb452901b8f42f7ad388eb40ee62dd938cb050eb Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Sun, 14 Jun 2020 01:20:31 +0300 Subject: [PATCH 3/3] Rework the webhooks table, allow to update exists webhooks --- common/models/WebHook.php | 11 +-- common/models/WebHookEvent.php | 27 ------ common/tasks/CreateWebHooksDeliveries.php | 6 +- .../tests/fixtures/WebHooksEventsFixture.php | 19 ---- .../tests/fixtures/data/webhooks-events.php | 11 --- common/tests/fixtures/data/webhooks.php | 3 + .../tasks/CreateWebHooksDeliveriesTest.php | 1 - console/controllers/WebhooksController.php | 89 ++++++++++++++----- ...13_204832_remove_webhooks_events_table.php | 51 +++++++++++ console/models/WebHookForm.php | 17 +--- 10 files changed, 131 insertions(+), 104 deletions(-) delete mode 100644 common/models/WebHookEvent.php delete mode 100644 common/tests/fixtures/WebHooksEventsFixture.php delete mode 100644 common/tests/fixtures/data/webhooks-events.php create mode 100644 console/migrations/m200613_204832_remove_webhooks_events_table.php diff --git a/common/models/WebHook.php b/common/models/WebHook.php index 586fa9e..8b6c6d7 100644 --- a/common/models/WebHook.php +++ b/common/models/WebHook.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace common\models; use yii\behaviors\TimestampBehavior; -use yii\db\ActiveQueryInterface; use yii\db\ActiveRecord; /** @@ -12,18 +11,16 @@ use yii\db\ActiveRecord; * @property int $id * @property string $url * @property string|null $secret + * @property string[] $events * @property int $created_at * - * Relations: - * @property WebHookEvent[] $events - * * Behaviors: * @mixin TimestampBehavior */ class WebHook extends ActiveRecord { public static function tableName(): string { - return '{{%webhooks}}'; + return 'webhooks'; } public function behaviors(): array { @@ -35,8 +32,4 @@ class WebHook extends ActiveRecord { ]; } - public function getEvents(): ActiveQueryInterface { - return $this->hasMany(WebHookEvent::class, ['webhook_id' => 'id']); - } - } diff --git a/common/models/WebHookEvent.php b/common/models/WebHookEvent.php deleted file mode 100644 index 280d4ab..0000000 --- a/common/models/WebHookEvent.php +++ /dev/null @@ -1,27 +0,0 @@ -hasOne(WebHook::class, ['id' => 'webhook_id']); - } - -} diff --git a/common/tasks/CreateWebHooksDeliveries.php b/common/tasks/CreateWebHooksDeliveries.php index ffc7e11..ac604a8 100644 --- a/common/tasks/CreateWebHooksDeliveries.php +++ b/common/tasks/CreateWebHooksDeliveries.php @@ -6,6 +6,7 @@ namespace common\tasks; use common\models\Account; use common\models\WebHook; use Yii; +use yii\db\Expression; use yii\queue\RetryableJobInterface; final class CreateWebHooksDeliveries implements RetryableJobInterface { @@ -67,8 +68,9 @@ final class CreateWebHooksDeliveries implements RetryableJobInterface { public function execute($queue): void { /** @var WebHook[] $targets */ $targets = WebHook::find() - ->joinWith('events e', false) - ->andWhere(['e.event_type' => $this->type]) + // It's very important to use exactly single quote to begin the string + // and double quote to specify the string value + ->andWhere(new Expression("JSON_CONTAINS(`events`, '\"{$this->type}\"')")) ->all(); foreach ($targets as $target) { $job = new DeliveryWebHook(); diff --git a/common/tests/fixtures/WebHooksEventsFixture.php b/common/tests/fixtures/WebHooksEventsFixture.php deleted file mode 100644 index 2b95a4f..0000000 --- a/common/tests/fixtures/WebHooksEventsFixture.php +++ /dev/null @@ -1,19 +0,0 @@ - 1, - 'event_type' => 'account.edit', - ], - [ - 'webhook_id' => 2, - 'event_type' => 'account.edit', - ], -]; diff --git a/common/tests/fixtures/data/webhooks.php b/common/tests/fixtures/data/webhooks.php index 238c90a..1ca2b03 100644 --- a/common/tests/fixtures/data/webhooks.php +++ b/common/tests/fixtures/data/webhooks.php @@ -4,18 +4,21 @@ return [ 'id' => 1, 'url' => 'http://localhost:80/webhooks/ely', 'secret' => 'my-secret', + 'events' => ['account.edit'], 'created_at' => 1531054333, ], 'webhook-without-secret' => [ 'id' => 2, 'url' => 'http://localhost:81/webhooks/ely', 'secret' => null, + 'events' => ['account.edit'], 'created_at' => 1531054837, ], 'webhook-without-events' => [ 'id' => 3, 'url' => 'http://localhost:82/webhooks/ely', 'secret' => null, + 'events' => [], 'created_at' => 1531054990, ], ]; diff --git a/common/tests/unit/tasks/CreateWebHooksDeliveriesTest.php b/common/tests/unit/tasks/CreateWebHooksDeliveriesTest.php index a8e5587..791ce59 100644 --- a/common/tests/unit/tasks/CreateWebHooksDeliveriesTest.php +++ b/common/tests/unit/tasks/CreateWebHooksDeliveriesTest.php @@ -18,7 +18,6 @@ class CreateWebHooksDeliveriesTest extends TestCase { public function _fixtures(): array { return [ 'webhooks' => fixtures\WebHooksFixture::class, - 'webhooksEvents' => fixtures\WebHooksEventsFixture::class, ]; } diff --git a/console/controllers/WebhooksController.php b/console/controllers/WebhooksController.php index 3855b3b..1008560 100644 --- a/console/controllers/WebhooksController.php +++ b/console/controllers/WebhooksController.php @@ -7,15 +7,46 @@ use common\models\WebHook; use console\models\WebHookForm; use yii\console\Controller; use yii\console\ExitCode; -use yii\helpers\Console; +use yii\console\widgets\Table; +use yii\helpers\Console as C; class WebhooksController extends Controller { - public function actionCreate(): int { - $form = new WebHookForm(new WebHook()); + public $defaultAction = 'list'; - $url = Console::prompt('Enter webhook url:', [ + public function actionList(): void { + $rows = []; + /** @var WebHook $webHook */ + foreach (WebHook::find()->with('events')->all() as $webHook) { + $rows[] = [$webHook->id, $webHook->url, $webHook->secret, implode(', ', $webHook->events)]; + } + + echo (new Table([ + 'headers' => ['id', 'url', 'secret', 'events'], + 'rows' => $rows, + ]))->run(); + } + + public function actionCreate(): int { + return $this->runForm(new WebHookForm(new WebHook())); + } + + public function actionUpdate(int $id): int { + /** @var WebHook|null $webHook */ + $webHook = WebHook::findOne(['id' => $id]); + if ($webHook === null) { + C::error("Entity with id {$id} isn't found."); + + return ExitCode::DATAERR; + } + + return $this->runForm(new WebHookForm($webHook)); + } + + private function runForm(WebHookForm $form): int { + C::prompt(C::ansiFormat('Enter webhook url:', [C::FG_GREY]), [ 'required' => true, + 'default' => $form->url, 'validator' => function(string $input, ?string &$error) use ($form): bool { $form->url = $input; if (!$form->validate('url')) { @@ -26,34 +57,48 @@ class WebhooksController extends Controller { return true; }, ]); - $secret = Console::prompt('Enter webhook secret (empty to no secret):'); - $options = $form::getEvents(); - $options[''] = 'Finish input'; // It's needed to allow finish input cycle - $events = []; - - do { - $availableOptions = array_diff($options, $events); - $eventIndex = Console::select('Choose wanted events (submit no input to finish):', $availableOptions); - if ($eventIndex !== '') { - $events[] = $options[$eventIndex]; - } - } while ($eventIndex !== '' || empty($events)); - - $form->url = $url; - $form->events = $events; + $secret = C::prompt(C::ansiFormat('Enter webhook secret (empty to no secret):', [C::FG_GREY]), [ + 'default' => $form->secret, + ]); if ($secret !== '') { $form->secret = $secret; } + $allEvents = WebHookForm::getEvents(); + do { + $options = []; + foreach ($allEvents as $id => $option) { + if (in_array($option, $form->events, true)) { + $options["-{$id}"] = $option; // Cast to string to create "-0" index + } else { + $options[$id] = $option; + } + } + + $options[''] = 'Finish input'; // This needed to allow finish input cycle + + $eventIndex = C::select( + C::ansiFormat('Choose wanted events (submit no input to finish):', [C::FG_GREY]), + $options, + ); + if ($eventIndex === '') { + continue; + } + + if ($eventIndex[0] === '-') { + unset($form->events[array_search($options[$eventIndex], $form->events, true)]); + } else { + $form->events[] = $options[$eventIndex]; + } + } while ($eventIndex !== '' || empty($form->events)); + if (!$form->save()) { - Console::error('Unable to create new webhook. Check errors list below' . PHP_EOL . Console::errorSummary($form)); + C::error('Unable to create new webhook. Check errors list below' . PHP_EOL . C::errorSummary($form)); return ExitCode::UNSPECIFIED_ERROR; } return ExitCode::OK; } - // TODO: add action to modify the webhook events - } diff --git a/console/migrations/m200613_204832_remove_webhooks_events_table.php b/console/migrations/m200613_204832_remove_webhooks_events_table.php new file mode 100644 index 0000000..ad115af --- /dev/null +++ b/console/migrations/m200613_204832_remove_webhooks_events_table.php @@ -0,0 +1,51 @@ +addColumn('webhooks', 'events', $this->json()->toString('events') . ' AFTER `secret`'); + $webhooksIds = $this->db->createCommand('SELECT id FROM webhooks')->queryColumn(); + foreach ($webhooksIds as $webhookId) { + $events = $this->db->createCommand("SELECT event_type FROM webhooks_events WHERE webhook_id = {$webhookId}")->queryColumn(); + if (empty($events)) { + continue; + } + + $this->execute('UPDATE webhooks SET events = JSON_ARRAY("' . implode('","', $events) . '")'); + } + + $this->dropTable('webhooks_events'); + } + + public function safeDown() { + $this->createTable('webhooks_events', [ + 'webhook_id' => $this->db->getTableSchema('webhooks')->getColumn('id')->dbType . ' NOT NULL', + 'event_type' => $this->string()->notNull(), + $this->primary('webhook_id', 'event_type'), + ]); + $this->addForeignKey('FK_webhook_event_to_webhook', 'webhooks_events', 'webhook_id', 'webhooks', 'id', 'CASCADE', 'CASCADE'); + + $webhooks = $this->db->createCommand('SELECT id, `events` FROM webhooks')->queryAll(); + foreach ($webhooks as $webhook) { + if (empty($webhook['events'])) { + continue; + } + + $events = json_decode($webhook['events'], true); + if (empty($events)) { + continue; + } + + $this->batchInsert( + 'webhooks_events', + ['webhook_id', 'event_type'], + array_map(fn($event) => [$webhook['id'], $event], $events), + ); + } + + $this->dropColumn('webhooks', 'events'); + } + +} diff --git a/console/models/WebHookForm.php b/console/models/WebHookForm.php index bc84757..0d971b2 100644 --- a/console/models/WebHookForm.php +++ b/console/models/WebHookForm.php @@ -4,9 +4,7 @@ declare(strict_types=1); namespace console\models; use common\models\WebHook; -use common\models\WebHookEvent; use Webmozart\Assert\Assert; -use Yii; use yii\base\Model; class WebHookForm extends Model { @@ -22,6 +20,9 @@ class WebHookForm extends Model { public function __construct(WebHook $webHook, array $config = []) { parent::__construct($config); $this->webHook = $webHook; + $this->url = $webHook->url; + $this->secret = $webHook->secret; + $this->events = (array)$webHook->events; } public function rules(): array { @@ -38,22 +39,12 @@ class WebHookForm extends Model { return false; } - $transaction = Yii::$app->db->beginTransaction(); - $webHook = $this->webHook; $webHook->url = $this->url; $webHook->secret = $this->secret; + $webHook->events = array_values($this->events); // Drop the keys order Assert::true($webHook->save(), 'Cannot save webhook.'); - foreach ($this->events as $event) { - $eventModel = new WebHookEvent(); - $eventModel->webhook_id = $webHook->id; - $eventModel->event_type = $event; - Assert::true($eventModel->save(), 'Cannot save webhook event.'); - } - - $transaction->commit(); - return true; }