From b904d5d314c13c8f627f457bc5140f290416a3ec Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Wed, 30 Sep 2020 20:30:04 +0300 Subject: [PATCH 1/3] Implemented features to revoke access for previously authorized OAuth 2.0 clients --- api/config/routes.php | 2 + .../authserver/models/AuthenticationForm.php | 10 +- .../authserver/models/RefreshTokenForm.php | 10 +- .../oauth/controllers/ClientsController.php | 120 ++++++++++++++---- api/modules/oauth/models/OauthProcess.php | 6 +- api/rbac/Permissions.php | 2 + .../accounts/GetAuthorizedClientsCest.php | 40 ++++++ .../accounts/RevokeAuthorizedClientCest.php | 45 +++++++ common/models/OauthSession.php | 10 +- common/tests/fixtures/data/oauth-sessions.php | 6 + console/controllers/RbacController.php | 4 + ...add_oauth_sessions_last_used_at_column.php | 19 +++ 12 files changed, 240 insertions(+), 34 deletions(-) create mode 100644 api/tests/functional/accounts/GetAuthorizedClientsCest.php create mode 100644 api/tests/functional/accounts/RevokeAuthorizedClientCest.php create mode 100644 console/migrations/m200925_224423_add_oauth_sessions_last_used_at_column.php diff --git a/api/config/routes.php b/api/config/routes.php index 34f5653..be93a8c 100644 --- a/api/config/routes.php +++ b/api/config/routes.php @@ -8,6 +8,8 @@ return [ 'DELETE /v1/oauth2/' => 'oauth/clients/delete', 'POST /v1/oauth2//reset' => 'oauth/clients/reset', 'GET /v1/accounts//oauth2/clients' => 'oauth/clients/get-per-account', + 'GET /v1/accounts//oauth2/authorized' => 'oauth/clients/get-authorized-clients', + 'DELETE /v1/accounts//oauth2/authorized/' => 'oauth/clients/revoke-client', '/account/v1/info' => 'oauth/identity/index', // Accounts module routes diff --git a/api/modules/authserver/models/AuthenticationForm.php b/api/modules/authserver/models/AuthenticationForm.php index 92dfdb9..7257ecd 100644 --- a/api/modules/authserver/models/AuthenticationForm.php +++ b/api/modules/authserver/models/AuthenticationForm.php @@ -87,17 +87,19 @@ class AuthenticationForm extends ApiForm { $token = Yii::$app->tokensFactory->createForMinecraftAccount($account, $this->clientToken); $dataModel = new AuthenticateData($account, (string)$token, $this->clientToken); /** @var OauthSession|null $minecraftOauthSession */ - $hasMinecraftOauthSession = $account->getOauthSessions() + $minecraftOauthSession = $account->getOauthSessions() ->andWhere(['client_id' => OauthClient::UNAUTHORIZED_MINECRAFT_GAME_LAUNCHER]) - ->exists(); - if ($hasMinecraftOauthSession === false) { + ->one(); + if ($minecraftOauthSession === null) { $minecraftOauthSession = new OauthSession(); $minecraftOauthSession->account_id = $account->id; $minecraftOauthSession->client_id = OauthClient::UNAUTHORIZED_MINECRAFT_GAME_LAUNCHER; $minecraftOauthSession->scopes = [P::MINECRAFT_SERVER_SESSION]; - Assert::true($minecraftOauthSession->save()); } + $minecraftOauthSession->last_used_at = time(); + Assert::true($minecraftOauthSession->save()); + Authserver::info("User with id = {$account->id}, username = '{$account->username}' and email = '{$account->email}' successfully logged in."); return $dataModel; diff --git a/api/modules/authserver/models/RefreshTokenForm.php b/api/modules/authserver/models/RefreshTokenForm.php index cf21657..cf632fa 100644 --- a/api/modules/authserver/models/RefreshTokenForm.php +++ b/api/modules/authserver/models/RefreshTokenForm.php @@ -70,17 +70,19 @@ class RefreshTokenForm extends ApiForm { // TODO: This behavior duplicates with the AuthenticationForm. Need to find a way to avoid duplication. /** @var OauthSession|null $minecraftOauthSession */ - $hasMinecraftOauthSession = $account->getOauthSessions() + $minecraftOauthSession = $account->getOauthSessions() ->andWhere(['client_id' => OauthClient::UNAUTHORIZED_MINECRAFT_GAME_LAUNCHER]) - ->exists(); - if ($hasMinecraftOauthSession === false) { + ->one(); + if ($minecraftOauthSession === null) { $minecraftOauthSession = new OauthSession(); $minecraftOauthSession->account_id = $account->id; $minecraftOauthSession->client_id = OauthClient::UNAUTHORIZED_MINECRAFT_GAME_LAUNCHER; $minecraftOauthSession->scopes = [P::MINECRAFT_SERVER_SESSION]; - Assert::true($minecraftOauthSession->save()); } + $minecraftOauthSession->last_used_at = time(); + Assert::true($minecraftOauthSession->save()); + return new AuthenticateData($account, (string)$token, $this->clientToken); } diff --git a/api/modules/oauth/controllers/ClientsController.php b/api/modules/oauth/controllers/ClientsController.php index 3b42ffe..1888fae 100644 --- a/api/modules/oauth/controllers/ClientsController.php +++ b/api/modules/oauth/controllers/ClientsController.php @@ -1,4 +1,6 @@ ['update', 'delete', 'reset'], 'allow' => true, 'permissions' => [P::MANAGE_OAUTH_CLIENTS], - 'roleParams' => function() { - return [ - 'clientId' => Yii::$app->request->get('clientId'), - ]; - }, + 'roleParams' => fn() => [ + 'clientId' => Yii::$app->request->get('clientId'), + ], ], [ 'actions' => ['get'], 'allow' => true, 'permissions' => [P::VIEW_OAUTH_CLIENTS], - 'roleParams' => function() { - return [ - 'clientId' => Yii::$app->request->get('clientId'), - ]; - }, + 'roleParams' => fn() => [ + 'clientId' => Yii::$app->request->get('clientId'), + ], ], [ 'actions' => ['get-per-account'], 'allow' => true, 'permissions' => [P::VIEW_OAUTH_CLIENTS], - 'roleParams' => function() { - return [ - 'accountId' => Yii::$app->request->get('accountId'), - ]; - }, + 'roleParams' => fn() => [ + 'accountId' => Yii::$app->request->get('accountId'), + ], ], + [ + 'actions' => ['get-authorized-clients', 'revoke-client'], + 'allow' => true, + 'permissions' => [P::MANAGE_OAUTH_SESSIONS], + 'roleParams' => fn() => [ + 'accountId' => Yii::$app->request->get('accountId'), + ], + ], + ], + ], + 'verb' => [ + 'class' => VerbFilter::class, + 'actions' => [ + 'get' => ['GET'], + 'create' => ['POST'], + 'update' => ['PUT'], + 'delete' => ['DELETE'], + 'reset' => ['POST'], + 'get-per-account' => ['GET'], + 'get-authorized-clients' => ['GET'], + 'revoke-client' => ['DELETE'], ], ], ]); @@ -128,18 +148,60 @@ class ClientsController extends Controller { } public function actionGetPerAccount(int $accountId): array { - /** @var Account|null $account */ - $account = Account::findOne(['id' => $accountId]); - if ($account === null) { - throw new NotFoundHttpException(); - } - /** @var OauthClient[] $clients */ - $clients = $account->getOauthClients()->orderBy(['created_at' => SORT_ASC])->all(); + $clients = $this->findAccount($accountId)->getOauthClients()->orderBy(['created_at' => SORT_ASC])->all(); return array_map(fn(OauthClient $client): array => $this->formatClient($client), $clients); } + public function actionGetAuthorizedClients(int $accountId): array { + $account = $this->findAccount($accountId); + + $result = []; + /** @var \common\models\OauthSession[] $oauthSessions */ + $oauthSessions = $account->getOauthSessions() + ->innerJoinWith(['client c' => function(ActiveQuery $query): void { + $query->andOnCondition(['c.type' => OauthClient::TYPE_APPLICATION]); + }]) + ->andWhere([ + 'OR', + ['revoked_at' => null], + ['>', 'last_used_at', new Expression('`revoked_at`')], + ]) + ->all(); + foreach ($oauthSessions as $oauthSession) { + $client = $oauthSession->client; + if ($client === null) { + continue; + } + + $result[] = [ + 'id' => $client->id, + 'name' => $client->name, + 'description' => $client->description, + 'scopes' => $oauthSession->getScopes(), + 'authorizedAt' => $oauthSession->created_at, + 'lastUsedAt' => $oauthSession->last_used_at, + ]; + } + + return $result; + } + + public function actionRevokeClient(int $accountId, string $clientId): ?array { + $account = $this->findAccount($accountId); + $client = $this->findOauthClient($clientId); + + /** @var \common\models\OauthSession|null $session */ + $session = $account->getOauthSessions()->andWhere(['client_id' => $client->id])->one(); + if ($session !== null && !$session->isRevoked()) { + $session->revoked_at = time(); + Assert::true($session->save()); + } + + return ['success' => true]; + } + private function formatClient(OauthClient $client): array { $result = [ 'clientId' => $client->id, @@ -168,16 +230,26 @@ class ClientsController extends Controller { try { $model = OauthClientFormFactory::create($client); } catch (UnsupportedOauthClientType $e) { - Yii::warning('Someone tried use ' . $client->type . ' type of oauth form.'); + Yii::warning('Someone tried to use ' . $client->type . ' type of oauth form.'); throw new NotFoundHttpException(null, 0, $e); } return $model; } + private function findAccount(int $id): Account { + /** @var Account|null $account */ + $account = Account::findOne(['id' => $id]); + if ($account === null) { + throw new NotFoundHttpException(); + } + + return $account; + } + private function findOauthClient(string $clientId): OauthClient { /** @var OauthClient|null $client */ - $client = OauthClient::findOne($clientId); + $client = OauthClient::findOne(['id' => $clientId]); if ($client === null) { throw new NotFoundHttpException(); } diff --git a/api/modules/oauth/models/OauthProcess.php b/api/modules/oauth/models/OauthProcess.php index 1a3b674..16cac16 100644 --- a/api/modules/oauth/models/OauthProcess.php +++ b/api/modules/oauth/models/OauthProcess.php @@ -223,6 +223,10 @@ class OauthProcess { return false; } + if ($session->isRevoked()) { + return false; + } + return empty(array_diff($this->getScopesList($request), $session->getScopes())); } @@ -235,6 +239,7 @@ class OauthProcess { } $session->scopes = array_unique(array_merge($session->getScopes(), $this->getScopesList($request))); + $session->last_used_at = time(); Assert::true($session->save()); } @@ -346,7 +351,6 @@ class OauthProcess { } private function findOauthSession(Account $account, OauthClient $client): ?OauthSession { - /** @noinspection PhpIncompatibleReturnTypeInspection */ return $account->getOauthSessions()->andWhere(['client_id' => $client->id])->one(); } diff --git a/api/rbac/Permissions.php b/api/rbac/Permissions.php index f8d0c8d..9edaccc 100644 --- a/api/rbac/Permissions.php +++ b/api/rbac/Permissions.php @@ -16,6 +16,7 @@ final class Permissions { public const RESTORE_ACCOUNT = 'restore_account'; public const BLOCK_ACCOUNT = 'block_account'; public const COMPLETE_OAUTH_FLOW = 'complete_oauth_flow'; + public const MANAGE_OAUTH_SESSIONS = 'manage_oauth_sessions'; public const CREATE_OAUTH_CLIENTS = 'create_oauth_clients'; public const VIEW_OAUTH_CLIENTS = 'view_oauth_clients'; public const MANAGE_OAUTH_CLIENTS = 'manage_oauth_clients'; @@ -32,6 +33,7 @@ final class Permissions { 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 MANAGE_OWN_OAUTH_SESSIONS = 'manage_own_oauth_sessions'; public const VIEW_OWN_OAUTH_CLIENTS = 'view_own_oauth_clients'; public const MANAGE_OWN_OAUTH_CLIENTS = 'manage_own_oauth_clients'; diff --git a/api/tests/functional/accounts/GetAuthorizedClientsCest.php b/api/tests/functional/accounts/GetAuthorizedClientsCest.php new file mode 100644 index 0000000..a0c88a2 --- /dev/null +++ b/api/tests/functional/accounts/GetAuthorizedClientsCest.php @@ -0,0 +1,40 @@ +amAuthenticated('admin'); + $I->sendGET("/api/v1/accounts/{$id}/oauth2/authorized"); + $I->canSeeResponseCodeIs(200); + $I->canSeeResponseIsJson(); + $I->canSeeResponseContainsJson([ + [ + 'id' => 'test1', + 'name' => 'Test1', + 'description' => 'Some description', + 'scopes' => ['minecraft_server_session', 'obtain_own_account_info'], + 'authorizedAt' => 1479944472, + 'lastUsedAt' => 1479944472, + ], + ]); + $I->cantSeeResponseJsonMatchesJsonPath('$.[?(@.id="tlauncher")]'); + } + + public function testGetForNotOwnIdentity(FunctionalTester $I) { + $I->amAuthenticated('admin'); + $I->sendGET('/api/v1/accounts/2/oauth2/authorized'); + $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/RevokeAuthorizedClientCest.php b/api/tests/functional/accounts/RevokeAuthorizedClientCest.php new file mode 100644 index 0000000..d49f5cf --- /dev/null +++ b/api/tests/functional/accounts/RevokeAuthorizedClientCest.php @@ -0,0 +1,45 @@ +amAuthenticated('admin'); + $I->sendDELETE("/api/v1/accounts/{$id}/oauth2/authorized/test1"); + $I->canSeeResponseCodeIs(200); + $I->canSeeResponseIsJson(); + $I->canSeeResponseContainsJson([ + 'success' => true, + ]); + + $I->sendGET("/api/v1/accounts/{$id}/oauth2/authorized"); + $I->cantSeeResponseJsonMatchesJsonPath('$.[?(@.id="test1")]'); + } + + public function testRevokeAlreadyRevokedClient(FunctionalTester $I) { + $id = $I->amAuthenticated('admin'); + $I->sendDELETE("/api/v1/accounts/{$id}/oauth2/authorized/tlauncher"); + $I->canSeeResponseCodeIs(200); + $I->canSeeResponseIsJson(); + $I->canSeeResponseContainsJson([ + 'success' => true, + ]); + } + + public function testRevokeForNotOwnIdentity(FunctionalTester $I) { + $I->amAuthenticated('admin'); + $I->sendDELETE('/api/v1/accounts/2/oauth2/authorized/test1'); + $I->canSeeResponseCodeIs(403); + $I->canSeeResponseContainsJson([ + 'name' => 'Forbidden', + 'message' => 'You are not allowed to perform this action.', + 'code' => 0, + 'status' => 403, + ]); + } + +} diff --git a/common/models/OauthSession.php b/common/models/OauthSession.php index d3f37d2..c87b93b 100644 --- a/common/models/OauthSession.php +++ b/common/models/OauthSession.php @@ -16,10 +16,14 @@ use yii\db\ActiveRecord; * @property array $scopes * @property int $created_at * @property int|null $revoked_at + * @property int $last_used_at * * Relations: - * @property-read OauthClient $client + * @property-read OauthClient|null $client * @property-read Account $account + * + * Mixins: + * @mixin TimestampBehavior */ class OauthSession extends ActiveRecord { @@ -36,6 +40,10 @@ class OauthSession extends ActiveRecord { ]; } + public function isRevoked(): bool { + return $this->revoked_at > $this->last_used_at; + } + public function getClient(): ActiveQuery { return $this->hasOne(OauthClient::class, ['id' => 'client_id']); } diff --git a/common/tests/fixtures/data/oauth-sessions.php b/common/tests/fixtures/data/oauth-sessions.php index 70023bc..6cbbce0 100644 --- a/common/tests/fixtures/data/oauth-sessions.php +++ b/common/tests/fixtures/data/oauth-sessions.php @@ -7,6 +7,7 @@ return [ 'scopes' => null, 'created_at' => 1479944472, 'revoked_at' => null, + 'last_used_at' => 1479944472, ], 'revoked-tlauncher' => [ 'account_id' => 1, @@ -15,6 +16,7 @@ return [ 'scopes' => null, 'created_at' => Carbon\Carbon::create(2019, 8, 1, 0, 0, 0, 'Europe/Minsk')->unix(), 'revoked_at' => Carbon\Carbon::create(2019, 8, 1, 1, 2, 0, 'Europe/Minsk')->unix(), + 'last_used_at' => Carbon\Carbon::create(2019, 8, 1, 0, 0, 0, 'Europe/Minsk')->unix(), ], 'revoked-minecraft-game-launchers' => [ 'account_id' => 1, @@ -23,6 +25,7 @@ return [ 'scopes' => null, 'created_at' => Carbon\Carbon::create(2019, 8, 1, 0, 0, 0, 'Europe/Minsk')->unix(), 'revoked_at' => Carbon\Carbon::create(2019, 8, 1, 1, 2, 0, 'Europe/Minsk')->unix(), + 'last_used_at' => Carbon\Carbon::create(2019, 8, 1, 0, 0, 0, 'Europe/Minsk')->unix(), ], 'banned-account-session' => [ 'account_id' => 10, @@ -31,6 +34,7 @@ return [ 'scopes' => null, 'created_at' => 1481421663, 'revoked_at' => null, + 'last_used_at' => 1481421663, ], 'deleted-client-session' => [ 'account_id' => 1, @@ -39,6 +43,7 @@ return [ 'scopes' => null, 'created_at' => 1519510065, 'revoked_at' => null, + 'last_used_at' => 1519510065, ], 'actual-deleted-client-session' => [ 'account_id' => 2, @@ -47,5 +52,6 @@ return [ 'scopes' => null, 'created_at' => 1519511568, 'revoked_at' => null, + 'last_used_at' => 1519511568, ], ]; diff --git a/console/controllers/RbacController.php b/console/controllers/RbacController.php index 1b4846b..bff6673 100644 --- a/console/controllers/RbacController.php +++ b/console/controllers/RbacController.php @@ -38,6 +38,7 @@ class RbacController extends Controller { $permViewOauthClients = $this->createPermission(P::VIEW_OAUTH_CLIENTS); $permManageOauthClients = $this->createPermission(P::MANAGE_OAUTH_CLIENTS); $permCompleteOauthFlow = $this->createPermission(P::COMPLETE_OAUTH_FLOW, AccountOwner::class); + $permManageOauthSessions = $this->createPermission(P::MANAGE_OAUTH_SESSIONS); $permObtainAccountEmail = $this->createPermission(P::OBTAIN_ACCOUNT_EMAIL); $permObtainExtendedAccountInfo = $this->createPermission(P::OBTAIN_EXTENDED_ACCOUNT_INFO); @@ -53,6 +54,7 @@ class RbacController extends Controller { $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); + $permManageOwnOauthSessions = $this->createPermission(P::MANAGE_OWN_OAUTH_SESSIONS, AccountOwner::class); $permViewOwnOauthClients = $this->createPermission(P::VIEW_OWN_OAUTH_CLIENTS, OauthClientOwner::class); $permManageOwnOauthClients = $this->createPermission(P::MANAGE_OWN_OAUTH_CLIENTS, OauthClientOwner::class); @@ -69,6 +71,7 @@ class RbacController extends Controller { $authManager->addChild($permManageOwnTwoFactorAuth, $permManageTwoFactorAuth); $authManager->addChild($permDeleteOwnAccount, $permDeleteAccount); $authManager->addChild($permRestoreOwnAccount, $permRestoreAccount); + $authManager->addChild($permManageOwnOauthSessions, $permManageOauthSessions); $authManager->addChild($permViewOwnOauthClients, $permViewOauthClients); $authManager->addChild($permManageOwnOauthClients, $permManageOauthClients); @@ -86,6 +89,7 @@ class RbacController extends Controller { $authManager->addChild($roleAccountsWebUser, $permRestoreOwnAccount); $authManager->addChild($roleAccountsWebUser, $permCompleteOauthFlow); $authManager->addChild($roleAccountsWebUser, $permCreateOauthClients); + $authManager->addChild($roleAccountsWebUser, $permManageOwnOauthSessions); $authManager->addChild($roleAccountsWebUser, $permViewOwnOauthClients); $authManager->addChild($roleAccountsWebUser, $permManageOwnOauthClients); } diff --git a/console/migrations/m200925_224423_add_oauth_sessions_last_used_at_column.php b/console/migrations/m200925_224423_add_oauth_sessions_last_used_at_column.php new file mode 100644 index 0000000..4985d24 --- /dev/null +++ b/console/migrations/m200925_224423_add_oauth_sessions_last_used_at_column.php @@ -0,0 +1,19 @@ +addColumn('oauth_sessions', 'last_used_at', $this->integer(11)->unsigned()->notNull()); + $this->update('oauth_sessions', [ + 'last_used_at' => new Expression('`created_at`'), + ]); + } + + public function safeDown() { + $this->dropColumn('oauth_sessions', 'last_used_at'); + } + +} From 5fc97fdd7a18fc0af9c64896dcf08387e853ae7b Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Thu, 1 Oct 2020 01:40:28 +0300 Subject: [PATCH 2/3] Implemented oauth session revocation notification. Reworked webhooks notifications constructors --- .../oauth/controllers/ClientsController.php | 4 + common/models/Account.php | 7 +- .../AccountDeletedNotification.php | 33 +++++++ .../notifications/AccountEditNotification.php | 37 ++++++++ .../notifications/NotificationInterface.php | 12 +++ .../OAuthSessionRevokedNotification.php | 30 +++++++ common/tasks/CreateWebHooksDeliveries.php | 60 +++---------- common/tasks/DeliveryWebHook.php | 29 +----- .../AccountDeletedNotificationTest.php | 36 ++++++++ .../AccountEditNotificationTest.php | 46 ++++++++++ .../OAuthSessionRevokedNotificationTest.php | 30 +++++++ .../tasks/CreateWebHooksDeliveriesTest.php | 90 +++++++------------ console/models/WebHookForm.php | 6 +- 13 files changed, 283 insertions(+), 137 deletions(-) create mode 100644 common/notifications/AccountDeletedNotification.php create mode 100644 common/notifications/AccountEditNotification.php create mode 100644 common/notifications/NotificationInterface.php create mode 100644 common/notifications/OAuthSessionRevokedNotification.php create mode 100644 common/tests/unit/notifications/AccountDeletedNotificationTest.php create mode 100644 common/tests/unit/notifications/AccountEditNotificationTest.php create mode 100644 common/tests/unit/notifications/OAuthSessionRevokedNotificationTest.php diff --git a/api/modules/oauth/controllers/ClientsController.php b/api/modules/oauth/controllers/ClientsController.php index 1888fae..9992f94 100644 --- a/api/modules/oauth/controllers/ClientsController.php +++ b/api/modules/oauth/controllers/ClientsController.php @@ -11,6 +11,8 @@ use api\modules\oauth\models\OauthClientTypeForm; use api\rbac\Permissions as P; use common\models\Account; use common\models\OauthClient; +use common\notifications\OAuthSessionRevokedNotification; +use common\tasks\CreateWebHooksDeliveries; use Webmozart\Assert\Assert; use Yii; use yii\db\ActiveQuery; @@ -197,6 +199,8 @@ class ClientsController extends Controller { if ($session !== null && !$session->isRevoked()) { $session->revoked_at = time(); Assert::true($session->save()); + + Yii::$app->queue->push(new CreateWebHooksDeliveries(new OAuthSessionRevokedNotification($session))); } return ['success' => true]; diff --git a/common/models/Account.php b/common/models/Account.php index d949c65..d5ad0f8 100644 --- a/common/models/Account.php +++ b/common/models/Account.php @@ -5,6 +5,8 @@ namespace common\models; use Carbon\Carbon; use common\components\UserPass; +use common\notifications\AccountDeletedNotification; +use common\notifications\AccountEditNotification; use common\tasks\CreateWebHooksDeliveries; use DateInterval; use Webmozart\Assert\Assert; @@ -181,12 +183,13 @@ class Account extends ActiveRecord { return; } - Yii::$app->queue->push(CreateWebHooksDeliveries::createAccountEdit($this, $meaningfulChangedAttributes)); + $notification = new AccountEditNotification($this, $meaningfulChangedAttributes); + Yii::$app->queue->push(new CreateWebHooksDeliveries($notification)); } public function afterDelete(): void { parent::afterDelete(); - Yii::$app->queue->push(CreateWebHooksDeliveries::createAccountDeletion($this)); + Yii::$app->queue->push(new CreateWebHooksDeliveries(new AccountDeletedNotification($this))); } } diff --git a/common/notifications/AccountDeletedNotification.php b/common/notifications/AccountDeletedNotification.php new file mode 100644 index 0000000..b045b2c --- /dev/null +++ b/common/notifications/AccountDeletedNotification.php @@ -0,0 +1,33 @@ +deleted_at, 'Account must be deleted'); + $this->account = $account; + } + + public static function getType(): string { + return 'account.deletion'; + } + + public function getPayloads(): array { + return [ + 'id' => $this->account->id, + 'uuid' => $this->account->uuid, + 'username' => $this->account->username, + 'email' => $this->account->email, + 'registered' => date('c', $this->account->created_at), + 'deleted' => date('c', $this->account->deleted_at), + ]; + } + +} diff --git a/common/notifications/AccountEditNotification.php b/common/notifications/AccountEditNotification.php new file mode 100644 index 0000000..c17070e --- /dev/null +++ b/common/notifications/AccountEditNotification.php @@ -0,0 +1,37 @@ +account = $account; + $this->changedAttributes = $changedAttributes; + } + + public static function getType(): string { + return 'account.edit'; + } + + public function getPayloads(): array { + return [ + 'id' => $this->account->id, + 'uuid' => $this->account->uuid, + 'username' => $this->account->username, + 'email' => $this->account->email, + 'lang' => $this->account->lang, + 'isActive' => $this->account->status === Account::STATUS_ACTIVE, + 'isDeleted' => $this->account->status === Account::STATUS_DELETED, + 'registered' => date('c', (int)$this->account->created_at), + 'changedAttributes' => $this->changedAttributes, + ]; + } + +} diff --git a/common/notifications/NotificationInterface.php b/common/notifications/NotificationInterface.php new file mode 100644 index 0000000..445688d --- /dev/null +++ b/common/notifications/NotificationInterface.php @@ -0,0 +1,12 @@ +revoked_at, 'OAuth session must be revoked'); + $this->oauthSession = $oauthSession; + } + + public static function getType(): string { + return 'oauth2.session_revoked'; + } + + public function getPayloads(): array { + return [ + 'accountId' => $this->oauthSession->account_id, + 'clientId' => $this->oauthSession->client_id, + 'revoked' => date('c', $this->oauthSession->revoked_at), + ]; + } + +} diff --git a/common/tasks/CreateWebHooksDeliveries.php b/common/tasks/CreateWebHooksDeliveries.php index ac604a8..c452633 100644 --- a/common/tasks/CreateWebHooksDeliveries.php +++ b/common/tasks/CreateWebHooksDeliveries.php @@ -3,82 +3,44 @@ declare(strict_types=1); namespace common\tasks; -use common\models\Account; use common\models\WebHook; -use Yii; +use common\notifications\NotificationInterface; use yii\db\Expression; use yii\queue\RetryableJobInterface; final class CreateWebHooksDeliveries implements RetryableJobInterface { - public string $type; + private NotificationInterface $notification; - public array $payloads; - - public function __construct(string $type, array $payloads) { - $this->type = $type; - $this->payloads = $payloads; + public function __construct(NotificationInterface $notification) { + $this->notification = $notification; } - public static function createAccountEdit(Account $account, array $changedAttributes): self { - 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, - ]); - } - - 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(): int { return 10; } - /** - * @param int $attempt number - * @param \Exception|\Throwable $error from last execute of the job - * - * @return bool - */ public function canRetry($attempt, $error): bool { return true; } - /** - * @param \yii\queue\Queue $queue which pushed and is handling the job - */ public function execute($queue): void { + $type = $this->notification::getType(); + $payloads = $this->notification->getPayloads(); + /** @var WebHook[] $targets */ $targets = WebHook::find() // 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}\"')")) + ->andWhere(new Expression("JSON_CONTAINS(`events`, '\"{$type}\"')")) ->all(); foreach ($targets as $target) { $job = new DeliveryWebHook(); - $job->type = $this->type; + $job->type = $type; $job->url = $target->url; $job->secret = $target->secret; - $job->payloads = $this->payloads; - Yii::$app->queue->push($job); + $job->payloads = $payloads; + $queue->push($job); } } diff --git a/common/tasks/DeliveryWebHook.php b/common/tasks/DeliveryWebHook.php index a3d639b..102be6a 100644 --- a/common/tasks/DeliveryWebHook.php +++ b/common/tasks/DeliveryWebHook.php @@ -16,39 +16,18 @@ use yii\queue\RetryableJobInterface; class DeliveryWebHook implements RetryableJobInterface { - /** - * @var string - */ - public $type; + public string $type; - /** - * @var string - */ - public $url; + public string $url; - /** - * @var string|null - */ - public $secret; + public ?string $secret; - /** - * @var array - */ - public $payloads; + public array $payloads; - /** - * @return int time to reserve in seconds - */ public function getTtr(): int { return 65; } - /** - * @param int $attempt number - * @param \Exception|\Throwable $error from last execute of the job - * - * @return bool - */ public function canRetry($attempt, $error): bool { if ($attempt >= 5) { return false; diff --git a/common/tests/unit/notifications/AccountDeletedNotificationTest.php b/common/tests/unit/notifications/AccountDeletedNotificationTest.php new file mode 100644 index 0000000..796a4c4 --- /dev/null +++ b/common/tests/unit/notifications/AccountDeletedNotificationTest.php @@ -0,0 +1,36 @@ +id = 123; + $account->username = 'mock-username'; + $account->uuid = 'afc8dc7a-4bbf-4d3a-8699-68890088cf84'; + $account->email = 'mock@ely.by'; + $account->created_at = 1531008814; + $account->deleted_at = 1601501781; + + $notification = new AccountDeletedNotification($account); + $this->assertSame('account.deletion', $notification::getType()); + $this->assertSame([ + 'id' => 123, + 'uuid' => 'afc8dc7a-4bbf-4d3a-8699-68890088cf84', + 'username' => 'mock-username', + 'email' => 'mock@ely.by', + 'registered' => '2018-07-08T00:13:34+00:00', + 'deleted' => '2020-09-30T21:36:21+00:00', + ], $notification->getPayloads()); + } + +} diff --git a/common/tests/unit/notifications/AccountEditNotificationTest.php b/common/tests/unit/notifications/AccountEditNotificationTest.php new file mode 100644 index 0000000..fb22997 --- /dev/null +++ b/common/tests/unit/notifications/AccountEditNotificationTest.php @@ -0,0 +1,46 @@ +id = 123; + $account->username = 'mock-username'; + $account->uuid = 'afc8dc7a-4bbf-4d3a-8699-68890088cf84'; + $account->email = 'mock@ely.by'; + $account->lang = 'en'; + $account->status = Account::STATUS_ACTIVE; + $account->created_at = 1531008814; + $changedAttributes = [ + 'username' => 'old-username', + 'uuid' => 'e05d33e9-ff91-4d26-9f5c-8250f802a87a', + 'email' => 'old-email@ely.by', + 'status' => 0, + ]; + + $notification = new AccountEditNotification($account, $changedAttributes); + $this->assertSame('account.edit', $notification::getType()); + $this->assertSame([ + 'id' => 123, + 'uuid' => 'afc8dc7a-4bbf-4d3a-8699-68890088cf84', + 'username' => 'mock-username', + 'email' => 'mock@ely.by', + 'lang' => 'en', + 'isActive' => true, + 'isDeleted' => false, + 'registered' => '2018-07-08T00:13:34+00:00', + 'changedAttributes' => $changedAttributes, + ], $notification->getPayloads()); + } + +} diff --git a/common/tests/unit/notifications/OAuthSessionRevokedNotificationTest.php b/common/tests/unit/notifications/OAuthSessionRevokedNotificationTest.php new file mode 100644 index 0000000..5bea759 --- /dev/null +++ b/common/tests/unit/notifications/OAuthSessionRevokedNotificationTest.php @@ -0,0 +1,30 @@ +account_id = 1; + $oauthSession->client_id = 'mock-client'; + $oauthSession->revoked_at = 1601504074; + + $notification = new OAuthSessionRevokedNotification($oauthSession); + $this->assertSame('oauth2.session_revoked', $notification::getType()); + $this->assertSame([ + 'accountId' => 1, + 'clientId' => 'mock-client', + 'revoked' => '2020-09-30T22:14:34+00:00', + ], $notification->getPayloads()); + } + +} diff --git a/common/tests/unit/tasks/CreateWebHooksDeliveriesTest.php b/common/tests/unit/tasks/CreateWebHooksDeliveriesTest.php index 791ce59..52cdb48 100644 --- a/common/tests/unit/tasks/CreateWebHooksDeliveriesTest.php +++ b/common/tests/unit/tasks/CreateWebHooksDeliveriesTest.php @@ -3,7 +3,7 @@ declare(strict_types=1); namespace common\tests\unit\tasks; -use common\models\Account; +use common\notifications\NotificationInterface; use common\tasks\CreateWebHooksDeliveries; use common\tasks\DeliveryWebHook; use common\tests\fixtures; @@ -21,67 +21,39 @@ class CreateWebHooksDeliveriesTest extends TestCase { ]; } - public function testCreateAccountEdit() { - $account = new Account(); - $account->id = 123; - $account->username = 'mock-username'; - $account->uuid = 'afc8dc7a-4bbf-4d3a-8699-68890088cf84'; - $account->email = 'mock@ely.by'; - $account->lang = 'en'; - $account->status = Account::STATUS_ACTIVE; - $account->created_at = 1531008814; - $changedAttributes = [ - 'username' => 'old-username', - 'uuid' => 'e05d33e9-ff91-4d26-9f5c-8250f802a87a', - 'email' => 'old-email@ely.by', - 'status' => 0, - ]; - $result = CreateWebHooksDeliveries::createAccountEdit($account, $changedAttributes); - $this->assertSame('account.edit', $result->type); - $this->assertEmpty(array_diff_assoc([ - 'id' => 123, - 'uuid' => 'afc8dc7a-4bbf-4d3a-8699-68890088cf84', - 'username' => 'mock-username', - 'email' => 'mock@ely.by', - 'lang' => 'en', - 'isActive' => true, - 'registered' => '2018-07-08T00:13:34+00:00', - ], $result->payloads)); - $this->assertSame($changedAttributes, $result->payloads['changedAttributes']); - } - public function testExecute() { - $task = new CreateWebHooksDeliveries('account.edit', [ - 'id' => 123, - 'uuid' => 'afc8dc7a-4bbf-4d3a-8699-68890088cf84', - 'username' => 'mock-username', - 'email' => 'mock@ely.by', - 'lang' => 'en', - 'isActive' => true, - 'registered' => '2018-07-08T00:13:34+00:00', - 'changedAttributes' => [ - 'username' => 'old-username', - 'uuid' => 'e05d33e9-ff91-4d26-9f5c-8250f802a87a', - 'email' => 'old-email@ely.by', - 'status' => 0, - ], - ]); - $task->execute($this->createMock(Queue::class)); - /** @var DeliveryWebHook[] $tasks */ - $tasks = $this->tester->grabQueueJobs(); - $this->assertCount(2, $tasks); + $notification = new class implements NotificationInterface { + public static function getType(): string { + return 'account.edit'; + } - $this->assertInstanceOf(DeliveryWebHook::class, $tasks[0]); - $this->assertSame($task->type, $tasks[0]->type); - $this->assertSame($task->payloads, $tasks[0]->payloads); - $this->assertSame('http://localhost:80/webhooks/ely', $tasks[0]->url); - $this->assertSame('my-secret', $tasks[0]->secret); + public function getPayloads(): array { + return ['key' => 'value']; + } + }; - $this->assertInstanceOf(DeliveryWebHook::class, $tasks[1]); - $this->assertSame($task->type, $tasks[1]->type); - $this->assertSame($task->payloads, $tasks[1]->payloads); - $this->assertSame('http://localhost:81/webhooks/ely', $tasks[1]->url); - $this->assertNull($tasks[1]->secret); + $queue = $this->createMock(Queue::class); + $queue->expects($this->exactly(2))->method('push')->withConsecutive( + [$this->callback(function(DeliveryWebHook $task): bool { + $this->assertSame('account.edit', $task->type); + $this->assertSame(['key' => 'value'], $task->payloads); + $this->assertSame('http://localhost:80/webhooks/ely', $task->url); + $this->assertSame('my-secret', $task->secret); + + return true; + })], + [$this->callback(function(DeliveryWebHook $task): bool { + $this->assertSame('account.edit', $task->type); + $this->assertSame(['key' => 'value'], $task->payloads); + $this->assertSame('http://localhost:81/webhooks/ely', $task->url); + $this->assertNull($task->secret); + + return true; + })], + ); + + $task = new CreateWebHooksDeliveries($notification); + $task->execute($queue); } } diff --git a/console/models/WebHookForm.php b/console/models/WebHookForm.php index 0d971b2..73841ce 100644 --- a/console/models/WebHookForm.php +++ b/console/models/WebHookForm.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace console\models; use common\models\WebHook; +use common\notifications; use Webmozart\Assert\Assert; use yii\base\Model; @@ -50,8 +51,9 @@ class WebHookForm extends Model { public static function getEvents(): array { return [ - 'account.edit', - 'account.deletion', + notifications\AccountEditNotification::getType(), + notifications\AccountDeletedNotification::getType(), + notifications\OAuthSessionRevokedNotification::getType(), ]; } From 7da6a952eee83098ba62b41377a42ddc168082df Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Fri, 2 Oct 2020 18:14:43 +0300 Subject: [PATCH 3/3] Fix tests --- .../accounts/models/DeleteAccountFormTest.php | 9 ++++++- .../models/RestoreAccountFormTest.php | 9 ++++++- common/models/Account.php | 4 ++- common/tests/unit/models/AccountTest.php | 25 ++++++++++++++++--- 4 files changed, 40 insertions(+), 7 deletions(-) diff --git a/api/tests/unit/modules/accounts/models/DeleteAccountFormTest.php b/api/tests/unit/modules/accounts/models/DeleteAccountFormTest.php index 90313f0..0d80dbe 100644 --- a/api/tests/unit/modules/accounts/models/DeleteAccountFormTest.php +++ b/api/tests/unit/modules/accounts/models/DeleteAccountFormTest.php @@ -5,7 +5,9 @@ namespace api\tests\unit\modules\accounts\models; use api\modules\accounts\models\DeleteAccountForm; use api\tests\unit\TestCase; +use Codeception\Util\ReflectionHelper; use common\models\Account; +use common\notifications\AccountEditNotification; use common\tasks\CreateWebHooksDeliveries; use common\tasks\DeleteAccount; use common\tests\fixtures\AccountFixture; @@ -46,7 +48,12 @@ class DeleteAccountFormTest extends TestCase { ->method('push') ->withConsecutive( [$this->callback(function(CreateWebHooksDeliveries $task) use ($account): bool { - $this->assertSame($account->id, $task->payloads['id']); + /** @var AccountEditNotification $notification */ + $notification = ReflectionHelper::readPrivateProperty($task, 'notification'); + $this->assertInstanceOf(AccountEditNotification::class, $notification); + $this->assertSame($account->id, $notification->getPayloads()['id']); + $this->assertTrue($notification->getPayloads()['isDeleted']); + return true; })], [$this->callback(function(DeleteAccount $task) use ($account): bool { diff --git a/api/tests/unit/modules/accounts/models/RestoreAccountFormTest.php b/api/tests/unit/modules/accounts/models/RestoreAccountFormTest.php index bd30b51..1f3b03b 100644 --- a/api/tests/unit/modules/accounts/models/RestoreAccountFormTest.php +++ b/api/tests/unit/modules/accounts/models/RestoreAccountFormTest.php @@ -5,7 +5,9 @@ namespace api\tests\unit\modules\accounts\models; use api\modules\accounts\models\RestoreAccountForm; use api\tests\unit\TestCase; +use Codeception\Util\ReflectionHelper; use common\models\Account; +use common\notifications\AccountEditNotification; use common\tasks\CreateWebHooksDeliveries; use common\tests\fixtures\AccountFixture; use Yii; @@ -39,7 +41,12 @@ class RestoreAccountFormTest extends TestCase { ->method('push') ->withConsecutive( [$this->callback(function(CreateWebHooksDeliveries $task) use ($account): bool { - $this->assertSame($account->id, $task->payloads['id']); + /** @var AccountEditNotification $notification */ + $notification = ReflectionHelper::readPrivateProperty($task, 'notification'); + $this->assertInstanceOf(AccountEditNotification::class, $notification); + $this->assertSame($account->id, $notification->getPayloads()['id']); + $this->assertFalse($notification->getPayloads()['isDeleted']); + return true; })], ); diff --git a/common/models/Account.php b/common/models/Account.php index d5ad0f8..31ab1e6 100644 --- a/common/models/Account.php +++ b/common/models/Account.php @@ -189,7 +189,9 @@ class Account extends ActiveRecord { public function afterDelete(): void { parent::afterDelete(); - Yii::$app->queue->push(new CreateWebHooksDeliveries(new AccountDeletedNotification($this))); + if ($this->status !== self::STATUS_REGISTERED) { + Yii::$app->queue->push(new CreateWebHooksDeliveries(new AccountDeletedNotification($this))); + } } } diff --git a/common/tests/unit/models/AccountTest.php b/common/tests/unit/models/AccountTest.php index 6bef817..87bc15f 100644 --- a/common/tests/unit/models/AccountTest.php +++ b/common/tests/unit/models/AccountTest.php @@ -3,7 +3,10 @@ declare(strict_types=1); namespace common\tests\unit\models; +use Codeception\Util\ReflectionHelper; use common\models\Account; +use common\notifications\AccountDeletedNotification; +use common\notifications\AccountEditNotification; use common\tasks\CreateWebHooksDeliveries; use common\tests\fixtures\MojangUsernameFixture; use common\tests\unit\TestCase; @@ -129,23 +132,37 @@ class AccountTest extends TestCase { ]; $account = new Account(); + $account->id = 123; $account->afterSave(false, $changedAttributes); /** @var CreateWebHooksDeliveries $job */ $job = $this->tester->grabLastQueuedJob(); $this->assertInstanceOf(CreateWebHooksDeliveries::class, $job); - $this->assertSame('account.edit', $job->type); - $this->assertSame($changedAttributes, $job->payloads['changedAttributes']); + /** @var AccountEditNotification $notification */ + $notification = ReflectionHelper::readPrivateProperty($job, 'notification'); + $this->assertInstanceOf(AccountEditNotification::class, $notification); + $this->assertSame(123, $notification->getPayloads()['id']); + $this->assertSame($changedAttributes, $notification->getPayloads()['changedAttributes']); } public function testAfterDeletePushEvent() { $account = new Account(); $account->id = 1; + $account->status = Account::STATUS_REGISTERED; + $account->created_at = time() - 60 * 60 * 24; + $account->deleted_at = time(); + + $account->afterDelete(); + $this->assertNull($this->tester->grabLastQueuedJob()); + + $account->status = Account::STATUS_ACTIVE; $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']); + /** @var AccountDeletedNotification $notification */ + $notification = ReflectionHelper::readPrivateProperty($job, 'notification'); + $this->assertInstanceOf(AccountDeletedNotification::class, $notification); + $this->assertSame(1, $notification->getPayloads()['id']); } }