From db321db0fb5d4cb7f0278df3b243fd2653ba18c2 Mon Sep 17 00:00:00 2001 From: Torsten Dittmann Date: Fri, 2 Feb 2024 13:42:15 +0100 Subject: [PATCH] fix: code review --- app/config/errors.php | 2 +- app/controllers/api/account.php | 34 +++++++------------ composer.lock | 12 +++---- .../Utopia/Response/Model/MFAProvider.php | 6 ++-- .../Utopia/Response/Model/Session.php | 2 +- 5 files changed, 24 insertions(+), 32 deletions(-) diff --git a/app/config/errors.php b/app/config/errors.php index b0b9e64d0d..1f551b1ca9 100644 --- a/app/config/errors.php +++ b/app/config/errors.php @@ -239,7 +239,7 @@ return [ ], Exception::USER_MORE_FACTORS_REQUIRED => [ 'name' => Exception::USER_MORE_FACTORS_REQUIRED, - 'description' => null, + 'description' => 'More factors are required to complete the sign in process.', 'code' => 400, ], Exception::USER_OAUTH2_BAD_REQUEST => [ diff --git a/app/controllers/api/account.php b/app/controllers/api/account.php index d4f750b5cc..b28f457397 100644 --- a/app/controllers/api/account.php +++ b/app/controllers/api/account.php @@ -3437,7 +3437,6 @@ App::patch('/v1/account/mfa') ->label('audits.event', 'user.update') ->label('audits.resource', 'user/{response.$id}') ->label('audits.userId', '{response.$id}') - ->label('usage.metric', 'users.{scope}.requests.update') ->label('sdk.auth', [APP_AUTH_TYPE_SESSION, APP_AUTH_TYPE_JWT]) ->label('sdk.namespace', 'account') ->label('sdk.method', 'updateMFA') @@ -3464,14 +3463,13 @@ App::patch('/v1/account/mfa') $response->dynamic($user, Response::MODEL_ACCOUNT); }); -App::get('/v1/account/mfa/providers') - ->desc('List Providers') +App::get('/v1/account/mfa/factors') + ->desc('List Factors') ->groups(['api', 'account', 'mfa']) ->label('scope', 'accounts.read') - ->label('usage.metric', 'users.{scope}.requests.read') ->label('sdk.auth', [APP_AUTH_TYPE_SESSION, APP_AUTH_TYPE_JWT]) ->label('sdk.namespace', 'account') - ->label('sdk.method', 'listProviders') + ->label('sdk.method', 'listFactors') ->label('sdk.description', '/docs/references/account/get.md') ->label('sdk.response.code', Response::STATUS_CODE_OK) ->label('sdk.response.type', Response::CONTENT_TYPE_JSON) @@ -3491,7 +3489,7 @@ App::get('/v1/account/mfa/providers') $response->dynamic($providers, Response::MODEL_MFA_PROVIDERS); }); -App::post('/v1/account/mfa/:provider') +App::post('/v1/account/mfa/:factor') ->desc('Add Authenticator') ->groups(['api', 'account']) ->label('event', 'users.[userId].update.mfa') @@ -3499,7 +3497,6 @@ App::post('/v1/account/mfa/:provider') ->label('audits.event', 'user.update') ->label('audits.resource', 'user/{response.$id}') ->label('audits.userId', '{response.$id}') - ->label('usage.metric', 'users.{scope}.requests.update') ->label('sdk.auth', [APP_AUTH_TYPE_SESSION, APP_AUTH_TYPE_JWT]) ->label('sdk.namespace', 'account') ->label('sdk.method', 'addAuthenticator') @@ -3509,16 +3506,16 @@ App::post('/v1/account/mfa/:provider') ->label('sdk.response.model', Response::MODEL_MFA_PROVIDER) ->label('sdk.offline.model', '/account') ->label('sdk.offline.key', 'current') - ->param('provider', null, new WhiteList(['totp']), 'Provider.') + ->param('factor', null, new WhiteList(['totp']), 'Factor.') ->inject('requestTimestamp') ->inject('response') ->inject('project') ->inject('user') ->inject('dbForProject') ->inject('queueForEvents') - ->action(function (string $provider, ?\DateTime $requestTimestamp, Response $response, Document $project, Document $user, Database $dbForProject, Event $queueForEvents) { + ->action(function (string $factor, ?\DateTime $requestTimestamp, Response $response, Document $project, Document $user, Database $dbForProject, Event $queueForEvents) { - $otp = match ($provider) { + $otp = match ($factor) { 'totp' => new TOTP(), default => throw new Exception(Exception::GENERAL_UNKNOWN, 'Unknown provider.') }; @@ -3551,7 +3548,7 @@ App::post('/v1/account/mfa/:provider') $response->dynamic($model, Response::MODEL_MFA_PROVIDER); }); -App::put('/v1/account/mfa/:provider') +App::put('/v1/account/mfa/:factor') ->desc('Verify Authenticator') ->groups(['api', 'account']) ->label('event', 'users.[userId].update.mfa') @@ -3559,7 +3556,6 @@ App::put('/v1/account/mfa/:provider') ->label('audits.event', 'user.update') ->label('audits.resource', 'user/{response.$id}') ->label('audits.userId', '{response.$id}') - ->label('usage.metric', 'users.{scope}.requests.update') ->label('sdk.auth', [APP_AUTH_TYPE_SESSION, APP_AUTH_TYPE_JWT]) ->label('sdk.namespace', 'account') ->label('sdk.method', 'verifyAuthenticator') @@ -3569,7 +3565,7 @@ App::put('/v1/account/mfa/:provider') ->label('sdk.response.model', Response::MODEL_USER) ->label('sdk.offline.model', '/account') ->label('sdk.offline.key', 'current') - ->param('provider', null, new WhiteList(['totp']), 'Provider.') + ->param('factor', null, new WhiteList(['totp']), 'Factor.') ->param('otp', '', new Text(256), 'Valid verification token.') ->inject('requestTimestamp') ->inject('response') @@ -3577,9 +3573,9 @@ App::put('/v1/account/mfa/:provider') ->inject('project') ->inject('dbForProject') ->inject('queueForEvents') - ->action(function (string $provider, string $otp, ?\DateTime $requestTimestamp, Response $response, Document $user, Document $project, Database $dbForProject, Event $queueForEvents) { + ->action(function (string $factor, string $otp, ?\DateTime $requestTimestamp, Response $response, Document $user, Document $project, Database $dbForProject, Event $queueForEvents) { - $success = match ($provider) { + $success = match ($factor) { 'totp' => Challenge\TOTP::verify($user, $otp), default => false }; @@ -3616,7 +3612,6 @@ App::delete('/v1/account/mfa/:provider') ->label('audits.event', 'user.update') ->label('audits.resource', 'user/{response.$id}') ->label('audits.userId', '{response.$id}') - ->label('usage.metric', 'users.{scope}.requests.update') ->label('sdk.auth', [APP_AUTH_TYPE_SESSION, APP_AUTH_TYPE_JWT]) ->label('sdk.namespace', 'account') ->label('sdk.method', 'deleteAuthenticator') @@ -3624,8 +3619,6 @@ App::delete('/v1/account/mfa/:provider') ->label('sdk.response.code', Response::STATUS_CODE_OK) ->label('sdk.response.type', Response::CONTENT_TYPE_JSON) ->label('sdk.response.model', Response::MODEL_USER) - ->label('sdk.offline.model', '/account') - ->label('sdk.offline.key', 'current') ->param('provider', null, new WhiteList(['totp']), 'Provider.') ->param('otp', '', new Text(256), 'Valid verification token.') ->inject('requestTimestamp') @@ -3764,7 +3757,6 @@ App::put('/v1/account/mfa/challenge') ->label('audits.event', 'challenges.update') ->label('audits.resource', 'user/{response.userId}') ->label('audits.userId', '{response.userId}') - ->label('usage.metric', 'users.{scope}.requests.update') ->label('sdk.auth', [APP_AUTH_TYPE_SESSION, APP_AUTH_TYPE_JWT]) ->label('sdk.namespace', 'account') ->label('sdk.method', 'updateChallenge') @@ -3784,7 +3776,7 @@ App::put('/v1/account/mfa/challenge') $challenge = $dbForProject->getDocument('challenges', $challengeId); - if (!$challenge) { + if ($challenge->isEmpty()) { throw new Exception(Exception::USER_INVALID_TOKEN); } @@ -3801,7 +3793,7 @@ App::put('/v1/account/mfa/challenge') } $dbForProject->deleteDocument('challenges', $challengeId); - $dbForProject->deleteCachedDocument('users', $user->getId()); + $dbForProject->purgeCachedDocument('users', $user->getId()); $authDuration = $project->getAttribute('auths', [])['duration'] ?? Auth::TOKEN_EXPIRATION_LOGIN_LONG; $sessionId = Auth::sessionVerify($user->getAttribute('sessions', []), Auth::$secret, $authDuration); diff --git a/composer.lock b/composer.lock index 17800352ad..a6e646e93c 100644 --- a/composer.lock +++ b/composer.lock @@ -1543,16 +1543,16 @@ }, { "name": "utopia-php/database", - "version": "0.48.0", + "version": "0.48.1", "source": { "type": "git", "url": "https://github.com/utopia-php/database.git", - "reference": "2651f41b9d3909dc123d26becfb6a3a44fb63077" + "reference": "52abe057180a76fe354a516300344b33f268f6ea" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/utopia-php/database/zipball/2651f41b9d3909dc123d26becfb6a3a44fb63077", - "reference": "2651f41b9d3909dc123d26becfb6a3a44fb63077", + "url": "https://api.github.com/repos/utopia-php/database/zipball/52abe057180a76fe354a516300344b33f268f6ea", + "reference": "52abe057180a76fe354a516300344b33f268f6ea", "shasum": "" }, "require": { @@ -1593,9 +1593,9 @@ ], "support": { "issues": "https://github.com/utopia-php/database/issues", - "source": "https://github.com/utopia-php/database/tree/0.48.0" + "source": "https://github.com/utopia-php/database/tree/0.48.1" }, - "time": "2024-01-19T08:17:22+00:00" + "time": "2024-02-02T04:54:13+00:00" }, { "name": "utopia-php/domains", diff --git a/src/Appwrite/Utopia/Response/Model/MFAProvider.php b/src/Appwrite/Utopia/Response/Model/MFAProvider.php index 10a0d262c1..fec6c94f5e 100644 --- a/src/Appwrite/Utopia/Response/Model/MFAProvider.php +++ b/src/Appwrite/Utopia/Response/Model/MFAProvider.php @@ -12,20 +12,20 @@ class MFAProvider extends Model $this ->addRule('backups', [ 'type' => self::TYPE_STRING, - 'description' => 'backup codes', + 'description' => 'Backup codes.', 'array' => true, 'default' => [], 'example' => true ]) ->addRule('secret', [ 'type' => self::TYPE_STRING, - 'description' => 'secret used for top auth', + 'description' => 'Secret token used for TOTP factor.', 'default' => '', 'example' => true ]) ->addRule('uri', [ 'type' => self::TYPE_STRING, - 'description' => 'uri for otp app', + 'description' => 'URI for authenticator apps.', 'default' => '', 'example' => true ]) diff --git a/src/Appwrite/Utopia/Response/Model/Session.php b/src/Appwrite/Utopia/Response/Model/Session.php index b55b92ee4c..b1e41e5419 100644 --- a/src/Appwrite/Utopia/Response/Model/Session.php +++ b/src/Appwrite/Utopia/Response/Model/Session.php @@ -162,7 +162,7 @@ class Session extends Model ]) ->addRule('factors', [ 'type' => self::TYPE_INTEGER, - 'description' => 'Returns true if this the current user session.', + 'description' => 'Returns a list of active session factors.', 'default' => 1, 'example' => 1, ])