Implement project-specific permissions #11188
Conversation
📝 WalkthroughWalkthroughIntroduces project-scoped permissions across project creation, team updates, and project transfers by adding Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@app/controllers/shared/api.php`:
- Around line 247-265: The project-specific role extraction currently assumes
role names have exactly two dashes by using explode('-', $projectRole)[2];
update the logic in the block handling $projectSpecificRoles so $actualRole is
derived by stripping the "project-$projectId-" prefix (e.g., compute $prefix =
"project-$projectId-"; then take the substring after that prefix) and only use
it if the prefix matches; then merge scopes as before into $scopes from
$roles[$actualRole]['scopes']; ensure you fallback/skip if $actualRole is empty
or not present in $roles.
In `@composer.json`:
- Around line 55-56: Replace the dev-aliased dependency with a stable tag or
explicit commit reference: change the package declaration for
"utopia-php/database" from "dev-ser-541-tag-4.5.2 as 4.0.99" to either the
upstream tagged release (e.g., "4.5.2" or whatever exact semver tag exists) or
the explicit commit reference matching composer.lock
(71c0b9c44f7b4d47b3d7517f592374743eb604d6) using the commit-hash syntax; also
update the VCS repository configuration (the custom repository block lines
112–116) to reference the same tag or exact commit instead of a floating branch
so future composer update runs remain reproducible.
In `@src/Appwrite/Auth/Validator/Role.php`:
- Around line 73-94: The isValid method in Role.php incorrectly uses
explode("-", $value) which fails for project IDs that contain hyphens; update
Role::isValid to, when str_starts_with($value, "project-"), locate the last '-'
(e.g., with strrpos) after the "project-" prefix and extract the substring after
that last dash as $role, returning false if no trailing dash is found, then
continue checking membership against $this->roles; adjust any variable names
accordingly.
In `@src/Appwrite/Utopia/Database/Documents/User.php`:
- Around line 83-87: The code incorrectly splits $projectRole using explode('-',
...) which drops parts when project IDs contain hyphens; instead find the last
dash position (e.g., using strrpos) and derive the base role as the substring up
to that last dash, then pass that base into Role::team($node['teamId'],
$base)->toString() so project-role base mapping (variables
$projectSpecificRoles, $projectRole, Role::team, $roles, $node['teamId']) uses
the portion before the final '-' rather than only the first two dash-separated
parts.
In `@tests/e2e/Services/Projects/ProjectsBase.php`:
- Around line 10-32: The helper setupProject has a gap: when $newTeam is false
and no $teamId is provided it will pass null to the project creation call; add a
fast-fail guard at the start of setupProject to assert/throw if $newTeam ===
false and $teamId is null/empty (use PHPUnit assertion or throw
InvalidArgumentException) so that callers must supply a valid $teamId before
calling client->call for the '/projects' POST; reference the setupProject
method, the $newTeam and $teamId parameters, and the subsequent
client->call('/projects') to locate where to insert the check.
In `@tests/e2e/Services/Teams/TeamsConsoleClientTest.php`:
- Around line 343-359: The test deletes a team while a project may still
reference it, causing failures; before calling
$this->client->call(Client::METHOD_DELETE, '/teams/' . $teamId, ...) you should
delete the project that references the team (use the project id from
$this->getProject() or the variable holding the created project id) by calling
$this->client->call(Client::METHOD_DELETE, '/projects/' . $projectId, ...) and
asserting a 204 status-code, then proceed to delete the team and assert 204 as
currently done.
🧹 Nitpick comments (2)
tests/e2e/Services/Projects/ProjectsBase.php (1)
80-84: Assert invite email recipient before extracting tokens.
getLastEmail()can return unrelated messages from prior steps; asserting the recipient here reduces flakiness in the membership-confirmation flow.🧪 Suggested assertion
$lastEmail = $this->getLastEmail(); + $this->assertEquals($params['email'], $lastEmail['to'][0]['address']); $tokens = $this->extractQueryParamsFromEmailLink($lastEmail['html']);tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (1)
5598-5656: Consider cleanup to keep global project counts stable.These tests create two projects (and a team) but don’t clean them up, which can skew count-based assertions elsewhere if execution order changes. Suggest deleting the projects/team at the end (and similarly in the update test). The delete test already removes the projects but could still remove the team.
🧹 Example cleanup for this test
foreach ($users as $user) { $session = $this->client->call(Client::METHOD_POST, '/account/sessions/email', [ 'origin' => 'http://localhost', 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], ], [ 'email' => $user['email'], 'password' => 'password', ]); $token = $session['cookies']['a_session_' . $this->getProject()['$id']]; $response = $this->client->call(Client::METHOD_GET, '/projects', [ 'origin' => 'http://localhost', 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], 'cookie' => 'a_session_' . $this->getProject()['$id'] . '=' . $token, ]); $this->assertEquals(200, $response['headers']['status-code']); $this->assertNotEmpty($response['body']); $this->assertCount(1, $response['body']['projects']); $this->assertEquals($user['projectId'], $response['body']['projects'][0]['$id']); } + + // Cleanup + foreach ([$projectIdA, $projectIdB] as $pid) { + $this->client->call(Client::METHOD_DELETE, '/projects/' . $pid, array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders())); + } + $this->client->call(Client::METHOD_DELETE, '/teams/' . $teamId, array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()));
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 1
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)
204-207: Duplicatereturn false;statement causes dead code.Lines 205-206 contain two consecutive
return false;statements. The second one is unreachable dead code.🐛 Proposed fix
} return false; - - return false; } }
🤖 Fix all issues with AI agents
In `@app/controllers/shared/api.php`:
- Around line 256-259: The code extracts $projectId via explode('/', $uri)[3]
which can throw an undefined index for malformed URIs; update the logic around
$request->getURI() to defensively check that the segments array has at least 4
elements before accessing index 3 (e.g., get $segments = explode('/', $uri) and
verify isset($segments[3]) or count($segments) > 3), and handle the missing case
by returning a clear error/early return or setting $projectId to null; modify
the block that sets $projectId so it uses this safe check (reference:
$request->getURI(), $projectId).
🧹 Nitpick comments (1)
src/Appwrite/Utopia/Database/Documents/User.php (1)
86-97: Variable shadowing:$projectIdparameter is overwritten inside the loop.Line 93 reassigns
$projectId(the function parameter) to a locally extracted value. While this doesn't cause a functional bug in the current code since the parameter isn't used after the loop, it can lead to confusion and subtle bugs if future code expects the original parameter value.♻️ Suggested fix: use a distinct variable name
foreach ($projectSpecificRoles as $projectRole) { $rest = \substr($projectRole, \strlen($projectRolePrefix)); $lastDash = \strrpos($rest, '-'); if ($lastDash === false) { continue; } - $projectId = \substr($rest, 0, $lastDash); - if ($projectId === '') { + $extractedProjectId = \substr($rest, 0, $lastDash); + if ($extractedProjectId === '') { continue; } - $roles[] = Role::team($node['teamId'], "{$projectRolePrefix}{$projectId}")->toString(); // Populate project-wide base role. + $roles[] = Role::team($node['teamId'], "{$projectRolePrefix}{$extractedProjectId}")->toString(); // Populate project-wide base role. }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/controllers/shared/api.php`:
- Around line 249-271: The filter is matching project roles by prefix
"project-$projectId" which also matches longer IDs that only share a prefix;
change the check to require the dashed suffix (use "project-{$projectId}-") when
building $projectSpecificRoles and when trimming the role name for $actualRole,
adjust the substring length accordingly so you only accept roles that are
exactly for that project (then continue to validate existence in $roles before
merging scopes and calling $authorization->addRole).
♻️ Duplicate comments (1)
app/controllers/shared/api.php (1)
258-261: Add bounds check when extracting projectId from URI.
explode('/', $uri)[3]can trigger undefined index on malformed URIs.🔧 Defensive guard
if (str_starts_with($path, "/v1/projects/:projectId")) { $uri = $request->getURI(); - $projectId = explode('/', $uri)[3]; + $uriParts = explode('/', $uri); + if (isset($uriParts[3])) { + $projectId = $uriParts[3]; + } }
|
|
||
| $permissions = [ | ||
| // Team-wide permissions | ||
| Permission::read(Role::team(ID::custom($teamId))), |
There was a problem hiding this comment.
Lets check once with Eldad if this is expected behaviour; I believe this makes it so every member can automatically see and read all proejcts.
There was a problem hiding this comment.
This was the problem we discussed last week about populating base team:<teamId> role for all users and how team:<teamId>/member could solve it.
Instead, what this PR does is, it populates team:<teamId> conditionally (only for non-projects API) so the user can still access the team but not take view all projects here
Closes SER-539
Closes SER-543