Skip to content

Populate project-specific roles#11209

Open
hmacr wants to merge 6 commits into1.8.xfrom
ser-539-permissions
Open

Populate project-specific roles#11209
hmacr wants to merge 6 commits into1.8.xfrom
ser-539-permissions

Conversation

@hmacr
Copy link
Contributor

@hmacr hmacr commented Jan 29, 2026

Towards SER-539

Should go after #11208

@github-actions
Copy link

github-actions bot commented Jan 29, 2026

Security Scan Results for PR

Docker Image Scan Results

Package Version Vulnerability Severity
libcrypto3 3.5.4-r0 CVE-2025-15467 CRITICAL
libcrypto3 3.5.4-r0 CVE-2025-69419 HIGH
libcrypto3 3.5.4-r0 CVE-2025-69421 HIGH
libexpat 2.7.3-r0 CVE-2026-24515 CRITICAL
libpng 1.6.51-r0 CVE-2025-66293 HIGH
libpng 1.6.51-r0 CVE-2026-22695 HIGH
libpng 1.6.51-r0 CVE-2026-22801 HIGH
libpng-dev 1.6.51-r0 CVE-2025-66293 HIGH
libpng-dev 1.6.51-r0 CVE-2026-22695 HIGH
libpng-dev 1.6.51-r0 CVE-2026-22801 HIGH
libssl3 3.5.4-r0 CVE-2025-15467 CRITICAL
libssl3 3.5.4-r0 CVE-2025-69419 HIGH
libssl3 3.5.4-r0 CVE-2025-69421 HIGH
libxml2 2.13.8-r0 CVE-2025-49794 CRITICAL
libxml2 2.13.8-r0 CVE-2025-49796 CRITICAL
libxml2 2.13.8-r0 CVE-2025-49795 HIGH
libxml2 2.13.8-r0 CVE-2025-6021 HIGH
openssl 3.5.4-r0 CVE-2025-15467 CRITICAL
openssl 3.5.4-r0 CVE-2025-69419 HIGH
openssl 3.5.4-r0 CVE-2025-69421 HIGH
openssl-dev 3.5.4-r0 CVE-2025-15467 CRITICAL
openssl-dev 3.5.4-r0 CVE-2025-69419 HIGH
openssl-dev 3.5.4-r0 CVE-2025-69421 HIGH
py3-urllib3 1.26.20-r0 CVE-2026-21441 HIGH
py3-urllib3-pyc 1.26.20-r0 CVE-2026-21441 HIGH
github.com/containerd/containerd/v2 v2.0.2 CVE-2024-25621 HIGH
golang.org/x/crypto v0.31.0 CVE-2025-22869 HIGH
golang.org/x/oauth2 v0.24.0 CVE-2025-22868 HIGH
stdlib 1.22.10 CVE-2025-47907 HIGH
stdlib 1.22.10 CVE-2025-58183 HIGH
stdlib 1.22.10 CVE-2025-61726 HIGH
stdlib 1.22.10 CVE-2025-61728 HIGH
stdlib 1.22.10 CVE-2025-61729 HIGH

Source Code Scan Results

🎉 No vulnerabilities found!

@github-actions
Copy link

github-actions bot commented Jan 29, 2026

✨ Benchmark results

  • Requests per second: 2,631
  • Requests with 200 status code: 473,608
  • P99 latency: 0.059051862

⚡ Benchmark Comparison

Metric This PR Latest version
RPS 2,631 1,286
200 473,608 231,554
P99 0.059051862 0.156900492

@@ -265,10 +265,23 @@

$scopes = []; // Reset scope if admin
foreach ($adminRoles as $role) {
Copy link
Contributor

@Meldiron Meldiron Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hmacr hmacr force-pushed the ser-539-permissions branch from 518b37c to 2f5a5e3 Compare January 30, 2026 09:49
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 30, 2026

📝 Walkthrough

Walkthrough

This 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)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Populate project-specific roles' accurately reflects the main changeset focus: adding project-level permission handling and role-specific authorization across multiple files.
Description check ✅ Passed The description references SER-539 and indicates dependency on PR #11208, which is related to the changeset's permission and role implementation work.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ser-539-permissions

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 unused Request injection/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()));
+        }

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🔴 Critical

Duplicate 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.

@Meldiron Meldiron changed the base branch from 1.8.x to ser-539-modules January 30, 2026 12:06
@hmacr hmacr changed the base branch from ser-539-modules to 1.8.x February 5, 2026 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants