Conversation
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
| @@ -265,10 +265,23 @@ | |||
|
|
|||
| $scopes = []; // Reset scope if admin | |||
| foreach ($adminRoles as $role) { | |||
There was a problem hiding this comment.
Here is example of permissions set on project document:
// Team-wide permissions
Permission::read(Role::team(ID::custom($teamId), 'owner')),
Permission::read(Role::team(ID::custom($teamId), 'developer')),
Permission::update(Role::team(ID::custom($teamId), 'owner')),
Permission::update(Role::team(ID::custom($teamId), 'developer')),
Permission::delete(Role::team(ID::custom($teamId), 'owner')),
Permission::delete(Role::team(ID::custom($teamId), 'developer')),
// Project-wide permissions
Permission::read(Role::team(ID::custom($teamId), "project-{$projectId}-owner")),
Permission::read(Role::team(ID::custom($teamId), "project-{$projectId}-developer")),
Permission::update(Role::team(ID::custom($teamId), "project-{$projectId}-owner")),
Permission::update(Role::team(ID::custom($teamId), "project-{$projectId}-developer")),
Permission::delete(Role::team(ID::custom($teamId), "project-{$projectId}-owner")),
Permission::delete(Role::team(ID::custom($teamId), "project-{$projectId}-developer")),
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
518b37c to
2f5a5e3
Compare
📝 WalkthroughWalkthroughThis pull request introduces project-specific permission handling and refactors authorization flows across the Appwrite platform. Changes include normalizing admin role scoping in API controllers, adding permission calculation methods to compute and project action modules, expanding test infrastructure with conditional team creation and membership setup helpers, implementing three new end-to-end tests validating fine-grained project access controls, and adjusting role resolution logic in the User document class to gate standard roles based on authorization scope rather than privilege status. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/Appwrite/Platform/Modules/Projects/Http/Projects/Create.php`:
- Around line 135-151: After filtering database pool entries for the requested
$region (the block using System::getEnv('_APP_DATABASE_KEYS') and array_filter
on $keys into $databases) add a guard that verifies $databases is not empty
before attempting array_search/array_rand; if empty, return or throw a clear
error response (instead of allowing array_rand to run) indicating no databases
are configured for that region. Ensure this check sits just after the
array_filter block and before the existing array_search($databaseOverride,
$databases) logic so $databaseOverride/$dsn selection only runs when $databases
contains entries.
In `@src/Appwrite/Platform/Modules/Projects/Http/Projects/Team/Update.php`:
- Around line 60-105: The find() calls that update permissions for
'installations', 'repositories', and 'vcsComments' in the action() method are
not paginated and may only update a subset; change each
$dbForPlatform->find(...) call to use Query::limit(100) and iterate in a loop
fetching batches until no results remain, applying the same
setAttribute('$permissions', $permissions) + updateDocument(...) for each item
in every batch; update the three blocks that reference $installations,
$repositories, and $vcsComments in action() to this paginated pattern so all
documents for projectInternalId are processed.
In `@tests/e2e/Services/Projects/ProjectsBase.php`:
- Around line 80-83: The test currently uses tokens from
extractQueryParamsFromEmailLink($lastEmail['html']) without asserting the keys
exist, causing undefined-index errors; update the test around getLastEmail() /
extractQueryParamsFromEmailLink to assert that $tokens is an array and that keys
'userId' and 'secret' (and any other expected keys used later) are present
before assigning $userId and $secret, and add the same assertions for the other
occurrence around lines where $tokens are used (the block referenced at 121-123)
so parsing failures surface as explicit test failures.
- Around line 10-32: The setupProject helper can pass a null $teamId when called
with $newTeam = false, causing confusing failures; update setupProject to
validate inputs: inside setupProject (function name) add a guard that if
$newTeam === false and $teamId === null throw an InvalidArgumentException (or
return a clear error) stating a non-null teamId is required when not creating a
new team, and then continue to use $teamId when building the project payload
(the array passed to the POST /projects call) so you never send a null 'teamId'
value.
🧹 Nitpick comments (4)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
25-50: Consider consolidating the shared permissions builder to avoid drift.This helper duplicates the same permissions list added in
src/Appwrite/Platform/Modules/Projects/Http/Projects/Action.php. A shared trait/helper would reduce future divergence.src/Appwrite/Platform/Modules/Projects/Http/Projects/Create.php (1)
12-12: Remove unusedRequestinjection/parameter.It isn’t used in the handler body; keeping it triggers lint noise.
🧹 Suggested cleanup
-use Appwrite\Utopia\Request; @@ - ->inject('request') ->inject('response') ->inject('dbForPlatform') ->inject('cache') ->inject('pools') ->inject('hooks') ->callback($this->action(...)); @@ -public function action(string $projectId, string $name, string $teamId, string $region, string $description, string $logo, string $url, string $legalName, string $legalCountry, string $legalState, string $legalCity, string $legalAddress, string $legalTaxId, Request $request, Response $response, Database $dbForPlatform, Cache $cache, Group $pools, Hooks $hooks) +public function action(string $projectId, string $name, string $teamId, string $region, string $description, string $logo, string $url, string $legalName, string $legalCountry, string $legalState, string $legalCity, string $legalAddress, string $legalTaxId, Response $response, Database $dbForPlatform, Cache $cache, Group $pools, Hooks $hooks)Also applies to: 86-96
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (2)
5598-5677: Factor session creation into a helper to reduce duplication and improve failure clarity.The same session creation block appears across these new permission tests. Extracting it into a small helper with an explicit status-code assertion will make failures clearer and reduce copy/paste.
Proposed refactor (helper + usage)
+ private function createUserSession(string $email): string + { + $session = $this->client->call(Client::METHOD_POST, '/account/sessions/email', [ + 'origin' => 'http://localhost', + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], [ + 'email' => $email, + 'password' => 'password', + ]); + $this->assertEquals(201, $session['headers']['status-code']); + return $session['cookies']['a_session_' . $this->getProject()['$id']]; + }- $session = $this->client->call(Client::METHOD_POST, '/account/sessions/email', [ - 'origin' => 'http://localhost', - 'content-type' => 'application/json', - 'x-appwrite-project' => $this->getProject()['$id'], - ], [ - 'email' => $testCase['userEmail'], - 'password' => 'password', - ]); - $token = $session['cookies']['a_session_' . $this->getProject()['$id']]; + $token = $this->createUserSession($testCase['userEmail']);
5778-5881: Consider cleaning up the team/memberships after the delete-permissions test.Projects are deleted, but the team and user memberships remain. Adding a cleanup block helps avoid data accumulation and potential side effects in long-running E2E suites.
Optional cleanup pattern
- foreach ($testCases as $testCase) { - ... - } + try { + foreach ($testCases as $testCase) { + ... + } + } finally { + $this->client->call(Client::METHOD_DELETE, '/teams/' . $teamId, array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders())); + }
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Appwrite/Utopia/Database/Documents/User.php (1)
177-180:⚠️ Potential issue | 🔴 CriticalDuplicate
return false;statement - unreachable code.Line 179 is unreachable due to the
return false;on line 178. This appears to be a merge artifact or copy-paste error.🐛 Proposed fix
return false; - - return false; } }
🤖 Fix all issues with AI agents
In `@app/controllers/shared/api.php`:
- Around line 299-307: The authorization status is enabled before checking
project permissions and never restored if $authorization->isValid($input)
throws, leaving authorization enabled for the rest of the request; wrap the
enable/validation logic in a try/finally so that
$authorization->setStatus($initialStatus) always runs: capture $initialStatus =
$authorization->getStatus(), call $authorization->enable(), perform the
$authorization->isValid($input) check in the try block (throwing the same
Exception on failure), and call $authorization->setStatus($initialStatus) in the
finally block to guarantee restoration (references: $project->getId(),
APP_MODE_ADMIN, Input, Database::PERMISSION_READ, $authorization->getStatus(),
$authorization->enable(), $authorization->isValid(),
$authorization->setStatus()).
- Around line 266-273: The loop handling $adminRoles assumes after stripping
'project-' the base role exists in $roles and directly accesses
$roles[$role]['scopes'], which can cause an undefined index; update the loop in
this controller to first check isset($roles[$role]) (or array_key_exists) after
computing $role and if the role is missing fail fast (throw an Exception or
return a 4xx response) instead of indexing into $roles, then only merge scopes
into $scopes and call $authorization->addRole($role) for known roles.
Towards SER-539
Should go after #11208