From 11477d67c949245dd8d9ef2bec1f6d8e27c9a53e Mon Sep 17 00:00:00 2001
From: Khushboo Verma <43381712+vermakhushboo@users.noreply.github.com>
Date: Tue, 11 Feb 2025 18:20:54 +0530
Subject: [PATCH] Address PR comments
---
app/config/errors.php | 2 +-
app/controllers/api/functions.php | 4 +--
app/controllers/api/project.php | 4 +--
.../Modules/Sites/Http/Variables/Create.php | 2 +-
.../Modules/Sites/Http/Variables/Update.php | 2 +-
.../Functions/FunctionsConsoleClientTest.php | 9 +++---
.../Functions/FunctionsCustomServerTest.php | 6 ++--
.../Projects/ProjectsConsoleClientTest.php | 13 ++++++++
.../Services/Sites/SitesCustomServerTest.php | 30 +++++++------------
.../sites/astro/src/pages/index.astro | 4 +--
10 files changed, 41 insertions(+), 35 deletions(-)
diff --git a/app/config/errors.php b/app/config/errors.php
index 4aaa5b77ae..d21f503d5f 100644
--- a/app/config/errors.php
+++ b/app/config/errors.php
@@ -870,7 +870,7 @@ return [
],
Exception::VARIABLE_CANNOT_UNSET_SECRET => [
'name' => Exception::VARIABLE_CANNOT_UNSET_SECRET,
- 'description' => 'Variable is a secret and cannot be unset to non-secret.',
+ 'description' => 'Secret variables cannot be marked as non-secret. Please re-create the variable if this is your intention.',
'code' => 400,
],
Exception::GRAPHQL_NO_QUERY => [
diff --git a/app/controllers/api/functions.php b/app/controllers/api/functions.php
index 8bdf4d9a9e..f6c9d472cf 100644
--- a/app/controllers/api/functions.php
+++ b/app/controllers/api/functions.php
@@ -1585,7 +1585,7 @@ App::post('/v1/functions/:functionId/variables')
->param('functionId', '', new UID(), 'Function unique ID.', false)
->param('key', null, new Text(Database::LENGTH_KEY), 'Variable key. Max length: ' . Database::LENGTH_KEY . ' chars.', false)
->param('value', null, new Text(8192, 0), 'Variable value. Max length: 8192 chars.', false)
- ->param('secret', true, new Boolean(), 'Is secret? Secret variables can only be updated or deleted, they cannot be read.', true)
+ ->param('secret', true, new Boolean(), 'Secret variables can be updated or deleted, but only functions can read them during build and runtime.', true)
->inject('response')
->inject('dbForProject')
->inject('dbForPlatform')
@@ -1729,7 +1729,7 @@ App::put('/v1/functions/:functionId/variables/:variableId')
->param('variableId', '', new UID(), 'Variable unique ID.', false)
->param('key', null, new Text(255), 'Variable key. Max length: 255 chars.', false)
->param('value', null, new Text(8192, 0), 'Variable value. Max length: 8192 chars.', true)
- ->param('secret', null, new Boolean(), 'Is secret? Secret variables can only be updated or deleted, they cannot be read.', true)
+ ->param('secret', null, new Boolean(), 'Secret variables can be updated or deleted, but only functions can read them during build and runtime.', true)
->inject('response')
->inject('dbForProject')
->inject('dbForPlatform')
diff --git a/app/controllers/api/project.php b/app/controllers/api/project.php
index 828d03a1dc..22c3337203 100644
--- a/app/controllers/api/project.php
+++ b/app/controllers/api/project.php
@@ -388,7 +388,7 @@ App::post('/v1/project/variables')
))
->param('key', null, new Text(Database::LENGTH_KEY), 'Variable key. Max length: ' . Database::LENGTH_KEY . ' chars.', false)
->param('value', null, new Text(8192, 0), 'Variable value. Max length: 8192 chars.', false)
- ->param('secret', true, new Boolean(), 'Is secret? Secret variables can only be updated or deleted, they cannot be read.', true)
+ ->param('secret', true, new Boolean(), 'Secret variables can be updated or deleted, but only projects can read them during build and runtime.', true)
->inject('project')
->inject('response')
->inject('dbForProject')
@@ -509,7 +509,7 @@ App::put('/v1/project/variables/:variableId')
->param('variableId', '', new UID(), 'Variable unique ID.', false)
->param('key', null, new Text(255), 'Variable key. Max length: 255 chars.', false)
->param('value', null, new Text(8192, 0), 'Variable value. Max length: 8192 chars.', true)
- ->param('secret', null, new Boolean(), 'Is secret? Secret variables can only be updated or deleted, they cannot be read.', true)
+ ->param('secret', null, new Boolean(), 'Secret variables can be updated or deleted, but only projects can read them during build and runtime.', true)
->inject('project')
->inject('response')
->inject('dbForProject')
diff --git a/src/Appwrite/Platform/Modules/Sites/Http/Variables/Create.php b/src/Appwrite/Platform/Modules/Sites/Http/Variables/Create.php
index eefad0d92d..2d3cf76f5c 100644
--- a/src/Appwrite/Platform/Modules/Sites/Http/Variables/Create.php
+++ b/src/Appwrite/Platform/Modules/Sites/Http/Variables/Create.php
@@ -56,7 +56,7 @@ class Create extends Base
->param('siteId', '', new UID(), 'Site unique ID.', false)
->param('key', null, new Text(Database::LENGTH_KEY), 'Variable key. Max length: ' . Database::LENGTH_KEY . ' chars.', false)
->param('value', null, new Text(8192, 0), 'Variable value. Max length: 8192 chars.', false)
- ->param('secret', true, new Boolean(), 'Is secret? Secret variables can only be updated or deleted, they cannot be read.', true)
+ ->param('secret', true, new Boolean(), 'Secret variables can be updated or deleted, but only sites can read them during build and runtime.', true)
->inject('response')
->inject('dbForProject')
->callback([$this, 'action']);
diff --git a/src/Appwrite/Platform/Modules/Sites/Http/Variables/Update.php b/src/Appwrite/Platform/Modules/Sites/Http/Variables/Update.php
index 7e8df5ddee..4040a8ae71 100644
--- a/src/Appwrite/Platform/Modules/Sites/Http/Variables/Update.php
+++ b/src/Appwrite/Platform/Modules/Sites/Http/Variables/Update.php
@@ -53,7 +53,7 @@ class Update extends Base
->param('variableId', '', new UID(), 'Variable unique ID.', false)
->param('key', null, new Text(255), 'Variable key. Max length: 255 chars.', false)
->param('value', null, new Text(8192, 0), 'Variable value. Max length: 8192 chars.', true)
- ->param('secret', null, new Boolean(), 'Is secret? Secret variables can only be updated or deleted, they cannot be read.', true)
+ ->param('secret', null, new Boolean(), 'Secret variables can be updated or deleted, but only sites can read them during build and runtime.', true)
->inject('response')
->inject('dbForProject')
->callback([$this, 'action']);
diff --git a/tests/e2e/Services/Functions/FunctionsConsoleClientTest.php b/tests/e2e/Services/Functions/FunctionsConsoleClientTest.php
index 0dbe22e038..0314651932 100644
--- a/tests/e2e/Services/Functions/FunctionsConsoleClientTest.php
+++ b/tests/e2e/Services/Functions/FunctionsConsoleClientTest.php
@@ -356,7 +356,7 @@ class FunctionsConsoleClientTest extends Scope
]);
$this->assertEquals(400, $response['headers']['status-code']);
-
+ $this->assertEquals('Secret variables cannot be marked as non-secret. Please re-create the variable if this is your intention.', $response['body']['message']);
/**
* Test for FAILURE
@@ -464,12 +464,12 @@ class FunctionsConsoleClientTest extends Scope
$this->assertFalse($function['body']['logging']);
$this->assertNotEmpty($function['body']['$id']);
- $functionId = $functionId = $function['body']['$id'] ?? '';
+ $functionId = $function['body']['$id'] ?? '';
// create variable
$variable = $this->createVariable($functionId, [
'key' => 'CUSTOM_VARIABLE',
- 'value' => 'test',
+ 'value' => 'a_secret_value',
'secret' => true,
]);
@@ -492,7 +492,8 @@ class FunctionsConsoleClientTest extends Scope
$this->assertEquals(201, $execution['headers']['status-code']);
$this->assertEmpty($execution['body']['logs']);
$this->assertEmpty($execution['body']['errors']);
- $this->assertStringContainsString('"CUSTOM_VARIABLE":"test"', $execution['body']['responseBody']);
+ $body = json_decode($execution['body']['responseBody']);
+ $this->assertEquals('a_secret_value', $body['CUSTOM_VARIABLE']);
$this->cleanupFunction($functionId);
}
diff --git a/tests/e2e/Services/Functions/FunctionsCustomServerTest.php b/tests/e2e/Services/Functions/FunctionsCustomServerTest.php
index 712d6d3948..98e03af1b8 100644
--- a/tests/e2e/Services/Functions/FunctionsCustomServerTest.php
+++ b/tests/e2e/Services/Functions/FunctionsCustomServerTest.php
@@ -38,7 +38,7 @@ class FunctionsCustomServerTest extends Scope
'timeout' => 10,
]);
- $functionId = $functionId = $function['body']['$id'] ?? '';
+ $functionId = $function['body']['$id'] ?? '';
$dateValidator = new DatetimeValidator();
$this->assertEquals(201, $function['headers']['status-code']);
@@ -356,7 +356,7 @@ class FunctionsCustomServerTest extends Scope
$this->assertEquals(201, $function['headers']['status-code']);
$this->assertNotEmpty($function['body']['$id']);
- $functionId = $functionId = $function['body']['$id'] ?? '';
+ $functionId = $function['body']['$id'] ?? '';
$deployments = $this->listDeployments($functionId);
@@ -1898,7 +1898,7 @@ class FunctionsCustomServerTest extends Scope
$this->assertFalse($function['body']['logging']);
$this->assertNotEmpty($function['body']['$id']);
- $functionId = $functionId = $function['body']['$id'] ?? '';
+ $functionId = $function['body']['$id'] ?? '';
$this->setupDeployment($functionId, [
'code' => $this->packageFunction('node'),
diff --git a/tests/e2e/Services/Projects/ProjectsConsoleClientTest.php b/tests/e2e/Services/Projects/ProjectsConsoleClientTest.php
index f07e70a836..77c7ecd7a2 100644
--- a/tests/e2e/Services/Projects/ProjectsConsoleClientTest.php
+++ b/tests/e2e/Services/Projects/ProjectsConsoleClientTest.php
@@ -4051,6 +4051,7 @@ class ProjectsConsoleClientTest extends Scope
$this->assertEquals(200, $response['headers']['status-code']);
$this->assertEquals("APP_TEST_1", $response['body']['key']);
$this->assertEmpty($response['body']['value']);
+ $this->assertTrue($response['body']['secret']);
/**
* Test for FAILURE
@@ -4122,6 +4123,18 @@ class ProjectsConsoleClientTest extends Scope
$this->assertEquals("APP_TEST_UPDATE_1", $variable['body']['key']);
$this->assertEmpty($variable['body']['value']);
+ $response = $this->client->call(Client::METHOD_PUT, '/project/variables/' . $data['secretVariableId'], array_merge([
+ 'content-type' => 'application/json',
+ 'x-appwrite-project' => $data['projectId'],
+ 'x-appwrite-mode' => 'admin',
+ ], $this->getHeaders()), [
+ 'key' => 'APP_TEST_UPDATE_1',
+ 'secret' => false,
+ ]);
+
+ $this->assertEquals(400, $response['headers']['status-code']);
+ $this->assertEquals('Secret variables cannot be marked as non-secret. Please re-create the variable if this is your intention.', $response['body']['message']);
+
$response = $this->client->call(Client::METHOD_GET, '/project/variables', array_merge([
'content-type' => 'application/json',
'x-appwrite-project' => $data['projectId'],
diff --git a/tests/e2e/Services/Sites/SitesCustomServerTest.php b/tests/e2e/Services/Sites/SitesCustomServerTest.php
index 321af4199f..ecce9399c1 100644
--- a/tests/e2e/Services/Sites/SitesCustomServerTest.php
+++ b/tests/e2e/Services/Sites/SitesCustomServerTest.php
@@ -67,7 +67,6 @@ class SitesCustomServerTest extends Scope
public function testVariables(): void
{
- // create site
$site = $this->createSite([
'buildRuntime' => 'ssr-22',
'fallbackFile' => null,
@@ -83,7 +82,6 @@ class SitesCustomServerTest extends Scope
$this->assertNotEmpty($site['body']['$id']);
$this->assertEquals('Test Site', $site['body']['name']);
- // create variable
$variable = $this->createVariable($siteId, [
'key' => 'siteKey1',
'value' => 'siteValue1',
@@ -108,7 +106,6 @@ class SitesCustomServerTest extends Scope
$this->assertEquals('siteValue2', $variable2['body']['value']);
$this->assertEquals(false, $variable2['body']['secret']);
- // create secret variable
$secretVariable = $this->createVariable($siteId, [
'key' => 'siteKey3',
'value' => 'siteValue3',
@@ -121,7 +118,6 @@ class SitesCustomServerTest extends Scope
$this->assertEquals('', $secretVariable['body']['value']);
$this->assertEquals(true, $secretVariable['body']['secret']);
- // get variable
$variable = $this->getVariable($siteId, $variable['body']['$id']);
$this->assertEquals(200, $variable['headers']['status-code']);
@@ -130,7 +126,6 @@ class SitesCustomServerTest extends Scope
$this->assertEquals('siteValue1', $variable['body']['value']);
$this->assertEquals(false, $variable['body']['secret']);
- // get secret variable
$secretVariable = $this->getVariable($siteId, $secretVariable['body']['$id']);
$this->assertEquals(200, $secretVariable['headers']['status-code']);
@@ -139,7 +134,6 @@ class SitesCustomServerTest extends Scope
$this->assertEquals('', $secretVariable['body']['value']);
$this->assertEquals(true, $secretVariable['body']['secret']);
- // update variable
$variable = $this->updateVariable($siteId, $variable['body']['$id'], [
'key' => 'siteKey1Updated',
'value' => 'siteValue1Updated',
@@ -151,7 +145,6 @@ class SitesCustomServerTest extends Scope
$this->assertEquals('siteValue1Updated', $variable['body']['value']);
$this->assertEquals(false, $variable['body']['secret']);
- // update variable to secret
$variable = $this->updateVariable($siteId, $variable['body']['$id'], [
'key' => 'siteKey1Updated',
'secret' => true,
@@ -163,7 +156,6 @@ class SitesCustomServerTest extends Scope
$this->assertEquals('', $variable['body']['value']);
$this->assertEquals(true, $variable['body']['secret']);
- // update value of secret variable
$secretVariable = $this->updateVariable($siteId, $secretVariable['body']['$id'], [
'key' => 'siteKey3',
'value' => 'siteValue3Updated',
@@ -175,15 +167,14 @@ class SitesCustomServerTest extends Scope
$this->assertEquals('', $secretVariable['body']['value']);
$this->assertEquals(true, $secretVariable['body']['secret']);
- // update secret variable to non-secret
- $temp = $this->updateVariable($siteId, $secretVariable['body']['$id'], [
+ $response = $this->updateVariable($siteId, $secretVariable['body']['$id'], [
'key' => 'siteKey3',
'secret' => false,
]);
- $this->assertEquals(400, $temp['headers']['status-code']);
+ $this->assertEquals(400, $response['headers']['status-code']);
+ $this->assertEquals('Secret variables cannot be marked as non-secret. Please re-create the variable if this is your intention.', $response['body']['message']);
- // get secret variable
$secretVariable = $this->getVariable($siteId, $secretVariable['body']['$id']);
$this->assertEquals(200, $secretVariable['headers']['status-code']);
@@ -192,18 +183,18 @@ class SitesCustomServerTest extends Scope
$this->assertEquals('', $secretVariable['body']['value']);
$this->assertEquals(true, $secretVariable['body']['secret']);
- // list variables
$variables = $this->listVariables($siteId);
$this->assertEquals(200, $variables['headers']['status-code']);
$this->assertCount(3, $variables['body']['variables']);
- // delete variable
- $this->deleteVariable($siteId, $variable['body']['$id']);
- $this->deleteVariable($siteId, $variable2['body']['$id']);
- $this->deleteVariable($siteId, $secretVariable['body']['$id']);
+ $response = $this->deleteVariable($siteId, $variable['body']['$id']);
+ $this->assertEquals(204, $response['headers']['status-code']);
+ $response = $this->deleteVariable($siteId, $variable2['body']['$id']);
+ $this->assertEquals(204, $response['headers']['status-code']);
+ $response = $this->deleteVariable($siteId, $secretVariable['body']['$id']);
+ $this->assertEquals(204, $response['headers']['status-code']);
- // list variables
$variables = $this->listVariables($siteId);
$this->assertEquals(200, $variables['headers']['status-code']);
@@ -256,7 +247,8 @@ class SitesCustomServerTest extends Scope
]));
$this->assertEquals(200, $response['headers']['status-code']);
- $this->assertStringContainsString("Message from ENV: Appwrite", $response['body']);
+ $this->assertStringContainsString("Env variable is Appwrite", $response['body']);
+ $this->assertStringNotContainsString("Variable not found", $response['body']);
$this->cleanupSite($siteId);
}
diff --git a/tests/resources/sites/astro/src/pages/index.astro b/tests/resources/sites/astro/src/pages/index.astro
index 96012f699b..cc8fd9411f 100644
--- a/tests/resources/sites/astro/src/pages/index.astro
+++ b/tests/resources/sites/astro/src/pages/index.astro
@@ -1,5 +1,5 @@
---
-const message = import.meta.env.name || 'Default message';
+const value = import.meta.env.name || 'Variable not found';
---
@@ -10,6 +10,6 @@ const message = import.meta.env.name || 'Default message';
Astro SSR
- Message from ENV: {message}
+ Env variable is {value}