From bfb51328feeb5c017653edeb8cc626597428e3b1 Mon Sep 17 00:00:00 2001 From: Steven Nguyen <1477010+stnguyen90@users.noreply.github.com> Date: Mon, 29 Apr 2024 22:50:20 +0000 Subject: [PATCH 1/9] fix: version in error response The APP_VERSION_STABLE constant should be used for the version in API errors so that developers see a version that makes more sense (such as 1.5.5). --- app/controllers/general.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/general.php b/app/controllers/general.php index 71ecc80d54..cd7bd6e1b9 100644 --- a/app/controllers/general.php +++ b/app/controllers/general.php @@ -742,12 +742,12 @@ App::error() 'file' => $file, 'line' => $line, 'trace' => \json_encode($trace, JSON_UNESCAPED_UNICODE) === false ? [] : $trace, // check for failing encode - 'version' => $version, + 'version' => APP_VERSION_STABLE, 'type' => $type, ] : [ 'message' => $message, 'code' => $code, - 'version' => $version, + 'version' => APP_VERSION_STABLE, 'type' => $type, ]; From 98d18ecc473d4a112e4935120f90ec57dcb9b585 Mon Sep 17 00:00:00 2001 From: Steven Nguyen Date: Mon, 6 May 2024 17:16:56 -0700 Subject: [PATCH 2/9] refactor(auth): remove auth duration from Auth::sessionVerify() calls The paramter was removed from the method so we don't need to pass it in anymore. --- app/controllers/api/account.php | 6 ++---- app/init.php | 7 +++---- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/app/controllers/api/account.php b/app/controllers/api/account.php index 90a35ede78..500b0842ab 100644 --- a/app/controllers/api/account.php +++ b/app/controllers/api/account.php @@ -3665,8 +3665,7 @@ App::put('/v1/account/mfa/authenticators/:type') $dbForProject->updateDocument('authenticators', $authenticator->getId(), $authenticator); $dbForProject->purgeCachedDocument('users', $user->getId()); - $authDuration = $project->getAttribute('auths', [])['duration'] ?? Auth::TOKEN_EXPIRATION_LOGIN_LONG; - $sessionId = Auth::sessionVerify($user->getAttribute('sessions', []), Auth::$secret, $authDuration); + $sessionId = Auth::sessionVerify($user->getAttribute('sessions', []), Auth::$secret); $session = $dbForProject->getDocument('sessions', $sessionId); $dbForProject->updateDocument('sessions', $sessionId, $session->setAttribute('factors', $type, Document::SET_TYPE_APPEND)); @@ -4105,8 +4104,7 @@ App::put('/v1/account/mfa/challenge') $dbForProject->deleteDocument('challenges', $challengeId); $dbForProject->purgeCachedDocument('users', $user->getId()); - $authDuration = $project->getAttribute('auths', [])['duration'] ?? Auth::TOKEN_EXPIRATION_LOGIN_LONG; - $sessionId = Auth::sessionVerify($user->getAttribute('sessions', []), Auth::$secret, $authDuration); + $sessionId = Auth::sessionVerify($user->getAttribute('sessions', []), Auth::$secret); $session = $dbForProject->getDocument('sessions', $sessionId); $session = $session diff --git a/app/init.php b/app/init.php index 5165f64d7f..8bf5d682cd 100644 --- a/app/init.php +++ b/app/init.php @@ -1239,14 +1239,13 @@ App::setResource('project', function ($dbForConsole, $request, $console) { return $project; }, ['dbForConsole', 'request', 'console']); -App::setResource('session', function (Document $user, Document $project) { +App::setResource('session', function (Document $user) { if ($user->isEmpty()) { return; } $sessions = $user->getAttribute('sessions', []); - $authDuration = $project->getAttribute('auths', [])['duration'] ?? Auth::TOKEN_EXPIRATION_LOGIN_LONG; - $sessionId = Auth::sessionVerify($user->getAttribute('sessions'), Auth::$secret, $authDuration); + $sessionId = Auth::sessionVerify($user->getAttribute('sessions'), Auth::$secret); if (!$sessionId) { return; @@ -1259,7 +1258,7 @@ App::setResource('session', function (Document $user, Document $project) { } return; -}, ['user', 'project']); +}, ['user']); App::setResource('console', function () { return new Document([ From 7e07f6b95814f7a2b4356db541b71edafcaaf597 Mon Sep 17 00:00:00 2001 From: Steven Nguyen Date: Mon, 6 May 2024 17:43:34 -0700 Subject: [PATCH 3/9] feat(auth): ensure user isn't kicked out after enabling MFA User's were kicked out and forced to verify their session after enabling MFA if they already had factors enabled. This change ensures that they are not kicked out of their current session after MFA is enabled by adding all relevant factors to the session. --- app/controllers/api/account.php | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/account.php b/app/controllers/api/account.php index 500b0842ab..b57b7cb891 100644 --- a/app/controllers/api/account.php +++ b/app/controllers/api/account.php @@ -3495,14 +3495,33 @@ App::patch('/v1/account/mfa') ->inject('requestTimestamp') ->inject('response') ->inject('user') + ->inject('session') ->inject('dbForProject') ->inject('queueForEvents') - ->action(function (bool $mfa, ?\DateTime $requestTimestamp, Response $response, Document $user, Database $dbForProject, Event $queueForEvents) { + ->action(function (bool $mfa, ?\DateTime $requestTimestamp, Response $response, Document $user, Document $session, Database $dbForProject, Event $queueForEvents) { $user->setAttribute('mfa', $mfa); $user = $dbForProject->withRequestTimestamp($requestTimestamp, fn () => $dbForProject->updateDocument('users', $user->getId(), $user)); + if ($mfa) { + $factors = $session->getAttribute('factors', []); + $totp = TOTP::getAuthenticatorFromUser($user); + if ($totp !== null && $totp->getAttribute('verified', false)) { + $factors[] = Type::TOTP; + } + if ($user->getAttribute('email', false) && $user->getAttribute('emailVerification', false)) { + $factors[] = Type::EMAIL; + } + if ($user->getAttribute('phone', false) && $user->getAttribute('phoneVerification', false)) { + $factors[] = Type::PHONE; + } + $factors = \array_unique($factors); + + $session->setAttribute('factors', $factors); + $dbForProject->updateDocument('sessions', $session->getId(), $session); + } + $queueForEvents->setParam('userId', $user->getId()); $response->dynamic($user, Response::MODEL_ACCOUNT); From 5b5505cf97fe12cc35df38962724245d7e86068a Mon Sep 17 00:00:00 2001 From: Steven Nguyen Date: Mon, 6 May 2024 17:48:44 -0700 Subject: [PATCH 4/9] fix(auth): ensure session factors don't contain duplicates --- app/controllers/api/account.php | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/app/controllers/api/account.php b/app/controllers/api/account.php index b57b7cb891..8c0f25be09 100644 --- a/app/controllers/api/account.php +++ b/app/controllers/api/account.php @@ -3652,10 +3652,10 @@ App::put('/v1/account/mfa/authenticators/:type') ->param('otp', '', new Text(256), 'Valid verification token.') ->inject('response') ->inject('user') - ->inject('project') + ->inject('session') ->inject('dbForProject') ->inject('queueForEvents') - ->action(function (string $type, string $otp, Response $response, Document $user, Document $project, Database $dbForProject, Event $queueForEvents) { + ->action(function (string $type, string $otp, Response $response, Document $user, Document $session, Database $dbForProject, Event $queueForEvents) { $authenticator = (match ($type) { Type::TOTP => TOTP::getAuthenticatorFromUser($user), @@ -3684,9 +3684,12 @@ App::put('/v1/account/mfa/authenticators/:type') $dbForProject->updateDocument('authenticators', $authenticator->getId(), $authenticator); $dbForProject->purgeCachedDocument('users', $user->getId()); - $sessionId = Auth::sessionVerify($user->getAttribute('sessions', []), Auth::$secret); - $session = $dbForProject->getDocument('sessions', $sessionId); - $dbForProject->updateDocument('sessions', $sessionId, $session->setAttribute('factors', $type, Document::SET_TYPE_APPEND)); + $factors = $session->getAttribute('factors', []); + $factors[] = $type; + $factors = \array_unique($factors); + + $session->setAttribute('factors', $factors); + $dbForProject->updateDocument('sessions', $session->getId(), $session); $queueForEvents->setParam('userId', $user->getId()); @@ -4075,9 +4078,10 @@ App::put('/v1/account/mfa/challenge') ->inject('project') ->inject('response') ->inject('user') + ->inject('session') ->inject('dbForProject') ->inject('queueForEvents') - ->action(function (string $challengeId, string $otp, Document $project, Response $response, Document $user, Database $dbForProject, Event $queueForEvents) { + ->action(function (string $challengeId, string $otp, Document $project, Response $response, Document $user, Document $session, Database $dbForProject, Event $queueForEvents) { $challenge = $dbForProject->getDocument('challenges', $challengeId); @@ -4123,14 +4127,15 @@ App::put('/v1/account/mfa/challenge') $dbForProject->deleteDocument('challenges', $challengeId); $dbForProject->purgeCachedDocument('users', $user->getId()); - $sessionId = Auth::sessionVerify($user->getAttribute('sessions', []), Auth::$secret); - $session = $dbForProject->getDocument('sessions', $sessionId); + $factors = $session->getAttribute('factors', []); + $factors[] = $type; + $factors = \array_unique($factors); - $session = $session - ->setAttribute('factors', $type, Document::SET_TYPE_APPEND) + $session + ->setAttribute('factors', $factors) ->setAttribute('mfaUpdatedAt', DateTime::now()); - $dbForProject->updateDocument('sessions', $sessionId, $session); + $dbForProject->updateDocument('sessions', $session->getId(), $session); $queueForEvents ->setParam('userId', $user->getId()) From fabe6921b49e6b21287ea21d2601585aa320c77d Mon Sep 17 00:00:00 2001 From: Steven Nguyen Date: Mon, 6 May 2024 18:25:38 -0700 Subject: [PATCH 5/9] fix(auth): Fix MFA email verification code font The font family was set to Inter without any fallback and since the Inter isn't available in emails, the font rendered the default font, Times New Roman. This commit adds a fallback font to the font-family ensuring an sans-serif font like Inter is used. --- app/config/locale/templates/email-mfa-challenge.tpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/config/locale/templates/email-mfa-challenge.tpl b/app/config/locale/templates/email-mfa-challenge.tpl index cf09448ca5..e3cb6b444d 100644 --- a/app/config/locale/templates/email-mfa-challenge.tpl +++ b/app/config/locale/templates/email-mfa-challenge.tpl @@ -5,7 +5,7 @@
-

{{otp}}

+

{{otp}}
From df064adce3f2819881172e36882e990c1888c6fd Mon Sep 17 00:00:00 2001 From: Steven Nguyen <1477010+stnguyen90@users.noreply.github.com> Date: Wed, 8 May 2024 17:36:46 +0000 Subject: [PATCH 6/9] feat(auth): forward OAuth2 callback params The only place Apple includes the user's name is in the params so we need to forward the params to the redirect endpoint so they can be used when creating the user. --- app/controllers/api/account.php | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/app/controllers/api/account.php b/app/controllers/api/account.php index 8c0f25be09..11af53008e 100644 --- a/app/controllers/api/account.php +++ b/app/controllers/api/account.php @@ -1069,17 +1069,15 @@ App::get('/v1/account/sessions/oauth2/callback/:provider/:projectId') $domain = $request->getHostname(); $protocol = $request->getProtocol(); + $params = $request->getParams(); + $params['project'] = $projectId; + unset($params['projectId']); + $response ->addHeader('Cache-Control', 'no-store, no-cache, must-revalidate, max-age=0') ->addHeader('Pragma', 'no-cache') ->redirect($protocol . '://' . $domain . '/v1/account/sessions/oauth2/' . $provider . '/redirect?' - . \http_build_query([ - 'project' => $projectId, - 'code' => $code, - 'state' => $state, - 'error' => $error, - 'error_description' => $error_description - ])); + . \http_build_query($params)); }); App::post('/v1/account/sessions/oauth2/callback/:provider/:projectId') @@ -1102,17 +1100,15 @@ App::post('/v1/account/sessions/oauth2/callback/:provider/:projectId') $domain = $request->getHostname(); $protocol = $request->getProtocol(); + $params = $request->getParams(); + $params['project'] = $projectId; + unset($params['projectId']); + $response ->addHeader('Cache-Control', 'no-store, no-cache, must-revalidate, max-age=0') ->addHeader('Pragma', 'no-cache') ->redirect($protocol . '://' . $domain . '/v1/account/sessions/oauth2/' . $provider . '/redirect?' - . \http_build_query([ - 'project' => $projectId, - 'code' => $code, - 'state' => $state, - 'error' => $error, - 'error_description' => $error_description - ])); + . \http_build_query($params)); }); App::get('/v1/account/sessions/oauth2/:provider/redirect') From c76e29077cac774100bcbd150b151bc144c054ea Mon Sep 17 00:00:00 2001 From: Steven Nguyen <1477010+stnguyen90@users.noreply.github.com> Date: Wed, 8 May 2024 17:51:45 +0000 Subject: [PATCH 7/9] feat(auth): try to get user name from request param if not from oauth2 This is only applicable for Apple OAuth2 because this is the only provider that does not return user name from an API call and only returns the name in the callback URL. Reference: * https://developer.apple.com/documentation/sign_in_with_apple/sign_in_with_apple_js/incorporating_sign_in_with_apple_into_other_platforms#3332115 --- app/controllers/api/account.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/account.php b/app/controllers/api/account.php index 11af53008e..f379dd3023 100644 --- a/app/controllers/api/account.php +++ b/app/controllers/api/account.php @@ -1235,7 +1235,17 @@ App::get('/v1/account/sessions/oauth2/:provider/redirect') $failureRedirect(Exception::USER_MISSING_ID); } - $name = $oauth2->getUserName($accessToken); + $name = ''; + $nameOAuth = $oauth2->getUserName($accessToken); + $userParam = \json_decode($request->getParam('user'), true); + if (!empty($nameOAuth)) { + $name = $nameOAuth; + } elseif (is_array($userParam)) { + $nameParam = $userParam['name']; + if (is_array($nameParam) && isset($nameParam['firstName']) && isset($nameParam['lastName'])) { + $name = $nameParam['firstName'] . ' ' . $nameParam['lastName']; + } + } $email = $oauth2->getUserEmail($accessToken); // Check if this identity is connected to a different user From c52bf2a0a125f5fb658a00ede709f56e217b4de5 Mon Sep 17 00:00:00 2001 From: Steven Nguyen <1477010+stnguyen90@users.noreply.github.com> Date: Wed, 8 May 2024 17:57:10 +0000 Subject: [PATCH 8/9] fix(auth): Don't use email in place for name for Apple OAuth2 Apple OAuth2 does not return the user's name in the claims and so we used email instead, but this can look broken to users and developers to see an email where the name should be. --- src/Appwrite/Auth/OAuth2/Apple.php | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/Appwrite/Auth/OAuth2/Apple.php b/src/Appwrite/Auth/OAuth2/Apple.php index 2abf61c947..0b4ec50881 100644 --- a/src/Appwrite/Auth/OAuth2/Apple.php +++ b/src/Appwrite/Auth/OAuth2/Apple.php @@ -160,15 +160,6 @@ class Apple extends OAuth2 */ public function getUserName(string $accessToken): string { - if ( - isset($this->claims['email']) && - !empty($this->claims['email']) && - isset($this->claims['email_verified']) && - $this->claims['email_verified'] === 'true' - ) { - return $this->claims['email']; - } - return ''; } From 1626168d3794cbacc31c9244c43436c2642dec68 Mon Sep 17 00:00:00 2001 From: Steven Nguyen <1477010+stnguyen90@users.noreply.github.com> Date: Mon, 13 May 2024 14:17:39 -0700 Subject: [PATCH 9/9] fix(project): set limit to retrieve all stats for the usage range Because limit was not passed for the find() query, the limit defaulted to 25. As such, when requesting stats for the last 30 days, only the last 25 were retrieved. --- app/controllers/api/project.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/project.php b/app/controllers/api/project.php index 208e288614..a3e07927d6 100644 --- a/app/controllers/api/project.php +++ b/app/controllers/api/project.php @@ -70,7 +70,7 @@ App::get('/v1/project/usage') '1d' => 'Y-m-d\T00:00:00.000P', }; - Authorization::skip(function () use ($dbForProject, $firstDay, $lastDay, $period, $metrics, &$total, &$stats) { + Authorization::skip(function () use ($dbForProject, $firstDay, $lastDay, $period, $metrics, $limit, &$total, &$stats) { foreach ($metrics['total'] as $metric) { $result = $dbForProject->findOne('stats', [ Query::equal('metric', [$metric]), @@ -85,6 +85,7 @@ App::get('/v1/project/usage') Query::equal('period', [$period]), Query::greaterThanEqual('time', $firstDay), Query::lessThan('time', $lastDay), + Query::limit($limit), Query::orderDesc('time'), ]);