From cba20d8b96bc7b0b4f9156347231cb4d52d25a0f Mon Sep 17 00:00:00 2001 From: Khushboo Verma <43381712+vermakhushboo@users.noreply.github.com> Date: Fri, 27 Oct 2023 19:38:33 +0530 Subject: [PATCH] Addressed PR comments --- .env | 2 +- .github/workflows/tests.yml | 1 + app/controllers/api/functions.php | 18 ++++--- app/controllers/api/vcs.php | 47 +++++++++++++++++-- tests/e2e/Services/VCS/VCSBase.php | 10 ++++ .../e2e/Services/VCS/VCSConsoleClientTest.php | 18 +++---- .../e2e/Services/VCS/VCSCustomClientTest.php | 24 ++++++++++ .../e2e/Services/VCS/VCSCustomServerTest.php | 25 ++++++++++ 8 files changed, 124 insertions(+), 21 deletions(-) create mode 100644 tests/e2e/Services/VCS/VCSCustomClientTest.php create mode 100644 tests/e2e/Services/VCS/VCSCustomServerTest.php diff --git a/.env b/.env index feeb74d849..ad551e705a 100644 --- a/.env +++ b/.env @@ -91,7 +91,7 @@ _APP_GRAPHQL_MAX_DEPTH=3 _APP_DOCKER_HUB_USERNAME= _APP_DOCKER_HUB_PASSWORD= _APP_VCS_GITHUB_APP_NAME= -_APP_VCS_GITHUB_PRIVATE_KEY="" +_APP_VCS_GITHUB_PRIVATE_KEY=disabled _APP_VCS_GITHUB_APP_ID= _APP_VCS_GITHUB_CLIENT_ID= _APP_VCS_GITHUB_CLIENT_SECRET= diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index ed9d7d1f19..842d61ff1c 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -98,6 +98,7 @@ jobs: Teams, Users, Webhooks, + VCS, ] steps: diff --git a/app/controllers/api/functions.php b/app/controllers/api/functions.php index 4a96308492..d370a7109c 100644 --- a/app/controllers/api/functions.php +++ b/app/controllers/api/functions.php @@ -46,6 +46,7 @@ use Utopia\Validator\Boolean; use Utopia\Database\Exception\Duplicate as DuplicateException; use MaxMind\Db\Reader; use Utopia\VCS\Adapter\Git\GitHub; +use Utopia\VCS\Exception\RepositoryNotFound; include_once __DIR__ . '/../shared/api.php'; @@ -58,7 +59,14 @@ $redeployVcs = function (Request $request, Document $function, Document $project $github->initializeVariables($providerInstallationId, $privateKey, $githubAppId); $owner = $github->getOwnerName($providerInstallationId); $providerRepositoryId = $function->getAttribute('providerRepositoryId', ''); - $repositoryName = $github->getRepositoryName($providerRepositoryId); + try { + $repositoryName = $github->getRepositoryName($providerRepositoryId) ?? ''; + if (empty($repositoryName)) { + throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); + } + } catch (RepositoryNotFound $e) { + throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); + } $providerBranch = $function->getAttribute('providerBranch', 'main'); $authorUrl = "https://github.com/$owner"; $repositoryUrl = "https://github.com/$owner/$repositoryName"; @@ -287,9 +295,8 @@ App::post('/v1/functions') $ruleModel = new Rule(); $ruleCreate = $queueForEvents - ->setClass(Event::WEBHOOK_CLASS_NAME) - ->setQueue(Event::WEBHOOK_QUEUE_NAME) - ; + ->setClass(Event::WEBHOOK_CLASS_NAME) + ->setQueue(Event::WEBHOOK_QUEUE_NAME); $ruleCreate ->setProject($project) @@ -870,8 +877,7 @@ App::get('/v1/functions/:functionId/deployments/:deploymentId/download') ->setContentType('application/gzip') ->addHeader('Expires', \date('D, d M Y H:i:s', \time() + (60 * 60 * 24 * 45)) . ' GMT') // 45 days cache ->addHeader('X-Peak', \memory_get_peak_usage()) - ->addHeader('Content-Disposition', 'attachment; filename="' . $deploymentId . '.tar.gz"') - ; + ->addHeader('Content-Disposition', 'attachment; filename="' . $deploymentId . '.tar.gz"'); $size = $deviceFunctions->getFileSize($path); $rangeHeader = $request->getHeader('range'); diff --git a/app/controllers/api/vcs.php b/app/controllers/api/vcs.php index 0c18a25b66..8b61580b76 100644 --- a/app/controllers/api/vcs.php +++ b/app/controllers/api/vcs.php @@ -66,7 +66,14 @@ $createGitDeployments = function (GitHub $github, string $providerInstallationId } $owner = $github->getOwnerName($providerInstallationId) ?? ''; - $repositoryName = $github->getRepositoryName($providerRepositoryId) ?? ''; + try { + $repositoryName = $github->getRepositoryName($providerRepositoryId) ?? ''; + if (empty($repositoryName)) { + throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); + } + } catch (RepositoryNotFound $e) { + throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); + } if (empty($repositoryName)) { throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); @@ -154,7 +161,14 @@ $createGitDeployments = function (GitHub $github, string $providerInstallationId $message = 'Authorization required for external contributor.'; $providerRepositoryId = $resource->getAttribute('providerRepositoryId'); - $repositoryName = $github->getRepositoryName($providerRepositoryId); + try { + $repositoryName = $github->getRepositoryName($providerRepositoryId) ?? ''; + if (empty($repositoryName)) { + throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); + } + } catch (RepositoryNotFound $e) { + throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); + } $owner = $github->getOwnerName($providerInstallationId); $github->updateCommitStatus($repositoryName, $providerCommitHash, $owner, 'failure', $message, $authorizeUrl, $name); continue; @@ -206,7 +220,14 @@ $createGitDeployments = function (GitHub $github, string $providerInstallationId $message = 'Starting...'; $providerRepositoryId = $resource->getAttribute('providerRepositoryId'); - $repositoryName = $github->getRepositoryName($providerRepositoryId); + try { + $repositoryName = $github->getRepositoryName($providerRepositoryId) ?? ''; + if (empty($repositoryName)) { + throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); + } + } catch (RepositoryNotFound $e) { + throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); + } $owner = $github->getOwnerName($providerInstallationId); $providerTargetUrl = $request->getProtocol() . '://' . $request->getHostname() . "/console/project-$projectId/functions/function-$functionId"; @@ -459,7 +480,10 @@ App::post('/v1/vcs/github/installations/:installationId/providerRepositories/:pr $owner = $github->getOwnerName($providerInstallationId); try { - $repositoryName = $github->getRepositoryName($providerRepositoryId); + $repositoryName = $github->getRepositoryName($providerRepositoryId) ?? ''; + if (empty($repositoryName)) { + throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); + } } catch (RepositoryNotFound $e) { throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); } @@ -722,6 +746,9 @@ App::get('/v1/vcs/github/installations/:installationId/providerRepositories/:pro $owner = $github->getOwnerName($providerInstallationId) ?? ''; try { $repositoryName = $github->getRepositoryName($providerRepositoryId) ?? ''; + if (empty($repositoryName)) { + throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); + } } catch (RepositoryNotFound $e) { throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); } @@ -768,6 +795,9 @@ App::get('/v1/vcs/github/installations/:installationId/providerRepositories/:pro $owner = $github->getOwnerName($providerInstallationId) ?? ''; try { $repositoryName = $github->getRepositoryName($providerRepositoryId) ?? ''; + if (empty($repositoryName)) { + throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); + } } catch (RepositoryNotFound $e) { throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); } @@ -1090,7 +1120,14 @@ App::patch('/v1/vcs/github/installations/:installationId/repositories/:repositor $providerRepositoryId = $repository->getAttribute('providerRepositoryId'); $owner = $github->getOwnerName($providerInstallationId); - $repositoryName = $github->getRepositoryName($providerRepositoryId); + try { + $repositoryName = $github->getRepositoryName($providerRepositoryId) ?? ''; + if (empty($repositoryName)) { + throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); + } + } catch (RepositoryNotFound $e) { + throw new Exception(Exception::PROVIDER_REPOSITORY_NOT_FOUND); + } $pullRequestResponse = $github->getPullRequest($owner, $repositoryName, $providerPullRequestId); $providerBranch = \explode(':', $pullRequestResponse['head']['label'])[1] ?? ''; diff --git a/tests/e2e/Services/VCS/VCSBase.php b/tests/e2e/Services/VCS/VCSBase.php index 66f1158027..65ff84445e 100644 --- a/tests/e2e/Services/VCS/VCSBase.php +++ b/tests/e2e/Services/VCS/VCSBase.php @@ -2,6 +2,16 @@ namespace Tests\E2E\Services\VCS; +use Utopia\App; + trait VCSBase { + protected function setUp(): void + { + parent::setUp(); + + if (App::getEnv('_APP_VCS_GITHUB_PRIVATE_KEY') === 'disabled') { + $this->markTestSkipped('VCS is not enabled.'); + } + } } diff --git a/tests/e2e/Services/VCS/VCSConsoleClientTest.php b/tests/e2e/Services/VCS/VCSConsoleClientTest.php index 114311d9b8..697fbdba91 100644 --- a/tests/e2e/Services/VCS/VCSConsoleClientTest.php +++ b/tests/e2e/Services/VCS/VCSConsoleClientTest.php @@ -23,7 +23,7 @@ class VCSConsoleClientTest extends Scope public string $providerRepositoryId = '705764267'; public string $providerRepositoryId2 = '708688544'; - public function testGitHubAuthorize() + public function testGitHubAuthorize(): string { /** * Test for SUCCESS @@ -43,7 +43,7 @@ class VCSConsoleClientTest extends Scope /** * @depends testGitHubAuthorize */ - public function testGetInstallation(string $installationId) + public function testGetInstallation(string $installationId): void { /** * Test for SUCCESS @@ -61,7 +61,7 @@ class VCSConsoleClientTest extends Scope /** * @depends testGitHubAuthorize */ - public function testDetectRuntime(string $installationId) + public function testDetectRuntime(string $installationId): void { /** * Test for SUCCESS @@ -88,7 +88,7 @@ class VCSConsoleClientTest extends Scope /** * @depends testGitHubAuthorize */ - public function testListRepositories(string $installationId) + public function testListRepositories(string $installationId): void { /** * Test for SUCCESS @@ -131,7 +131,7 @@ class VCSConsoleClientTest extends Scope /** * @depends testGitHubAuthorize */ - public function testGetRepository(string $installationId) + public function testGetRepository(string $installationId): void { /** * Test for SUCCESS @@ -169,7 +169,7 @@ class VCSConsoleClientTest extends Scope /** * @depends testGitHubAuthorize */ - public function testListRepositoryBranches(string $installationId) + public function testListRepositoryBranches(string $installationId): void { /** * Test for SUCCESS @@ -198,7 +198,7 @@ class VCSConsoleClientTest extends Scope /** * @depends testGitHubAuthorize */ - public function testCreateFunctionUsingVCS(string $installationId) + public function testCreateFunctionUsingVCS(string $installationId): array { $function = $this->client->call(Client::METHOD_POST, '/functions', array_merge([ 'content-type' => 'application/json', @@ -236,7 +236,7 @@ class VCSConsoleClientTest extends Scope /** * @depends testCreateFunctionUsingVCS */ - public function testUpdateFunctionUsingVCS(array $data) + public function testUpdateFunctionUsingVCS(array $data): string { $function = $this->client->call(Client::METHOD_PUT, '/functions/' . $data['functionId'], array_merge([ 'content-type' => 'application/json', @@ -271,7 +271,7 @@ class VCSConsoleClientTest extends Scope /** * @depends testGitHubAuthorize */ - public function testCreateRepository(string $installationId) + public function testCreateRepository(string $installationId): void { /** * Test for SUCCESS diff --git a/tests/e2e/Services/VCS/VCSCustomClientTest.php b/tests/e2e/Services/VCS/VCSCustomClientTest.php new file mode 100644 index 0000000000..084255cd69 --- /dev/null +++ b/tests/e2e/Services/VCS/VCSCustomClientTest.php @@ -0,0 +1,24 @@ +client->call(Client::METHOD_GET, '/vcs/installations/' . $installationId, array_merge([ + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders())); + + $this->assertEquals(401, $installation['headers']['status-code']); + } +} diff --git a/tests/e2e/Services/VCS/VCSCustomServerTest.php b/tests/e2e/Services/VCS/VCSCustomServerTest.php new file mode 100644 index 0000000000..dee4cd7ced --- /dev/null +++ b/tests/e2e/Services/VCS/VCSCustomServerTest.php @@ -0,0 +1,25 @@ +client->call(Client::METHOD_GET, '/vcs/installations/' . $installationId, array_merge([ + 'x-appwrite-project' => $this->getProject()['$id'], + 'x-appwrite-key' => $this->getProject()['apiKey'], + ], $this->getHeaders())); + + $this->assertEquals(401, $installation['headers']['status-code']); + } +}