Refactor authorization handling across multiple modules to use the ne… #10682
Conversation
…w getAuthorization() method. Update CHANGES.md to reflect the change in usage worker authorization. Adjust composer.json to specify the correct version for utopia-php/database. Update plugin-api-version in composer.lock.
📝 WalkthroughWalkthroughIntroduces a per-request Authorization resource and threads it through resource initializers, database factories, controllers, platform modules, tasks, workers, and many HTTP handlers. Resources and factories (dbForPlatform, getProjectDB, getLogsDB, etc.) now accept Authorization and call setAuthorization() on Database instances. Route and module actions are updated to inject Authorization and replace static Authorization::... calls with instance methods ($authorization->skip(), $authorization->getRoles(), $authorization->isValid(new Input(...))). Transaction and transaction-state codepaths, validators, and many attribute/collection handlers are updated accordingly. composer.json bumps utopia-php/database from 3.* to 4.*. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus during review:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
…th-single-instance # Conflicts: # composer.lock
✨ Benchmark results
⚡ Benchmark Comparison
|
…orker modules. Adjust Docker configuration to include database source path. Modify error handling in general controller and enhance permission setting in document creation. Fix privileged user role retrieval in function execution. Ensure consistent authorization usage across various resources.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (27)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
498-504: Critical syntax errors: Fix malformed parentheses.Lines 498 and 502 contain syntax errors with double opening parentheses that will prevent compilation:
- Line 498:
getAttribute(('type')should begetAttribute('type')- Line 502:
getAttribute(('filter')should begetAttribute('filter')Apply this diff to fix the syntax errors:
- if ($attribute->getAttribute(('type') !== $type)) { + if ($attribute->getAttribute('type') !== $type)) { throw new Exception($this->getTypeInvalidException()); } - if ($attribute->getAttribute('type') === Database::VAR_STRING && $attribute->getAttribute(('filter') !== $filter)) { + if ($attribute->getAttribute('type') === Database::VAR_STRING && $attribute->getAttribute('filter') !== $filter)) { throw new Exception($this->getTypeInvalidException()); }src/Appwrite/Platform/Modules/Functions/Http/Functions/Delete.php (1)
84-91: Add a guard for missing/empty schedule before update.If the function has no scheduleId or the schedule doc is empty, updating it will fail. Guard it to make deletion robust.
- $schedule = $dbForPlatform->getDocument('schedules', $function->getAttribute('scheduleId')); - $schedule - ->setAttribute('resourceUpdatedAt', DateTime::now()) - ->setAttribute('active', false); - $dbForPlatform->getAuthorization()->skip(fn () => $dbForPlatform->updateDocument('schedules', $schedule->getId(), $schedule)); + $scheduleId = $function->getAttribute('scheduleId'); + if (!empty($scheduleId)) { + $schedule = $dbForPlatform->getDocument('schedules', $scheduleId); + if (!$schedule->isEmpty()) { + $schedule + ->setAttribute('resourceUpdatedAt', DateTime::now()) + ->setAttribute('active', false); + $dbForPlatform->getAuthorization()->skip( + fn () => $dbForPlatform->updateDocument('schedules', $schedule->getId(), $schedule) + ); + } + }src/Appwrite/Platform/Modules/Compute/Base.php (3)
207-227: Fix authorization instance mismatch.The authorization skip is called on
$dbForProjectbut the operation is performed on$dbForPlatform. The authorization instance should match the database being operated on.Apply this diff:
- $dbForProject->getAuthorization()->skip( + $dbForPlatform->getAuthorization()->skip( fn () => $dbForPlatform->createDocument('rules', new Document([
233-253: Fix authorization instance mismatch.Same issue: authorization skip is called on
$dbForProjectbut the operation is performed on$dbForPlatform.Apply this diff:
- $dbForProject->getAuthorization()->skip( + $dbForPlatform->getAuthorization()->skip( fn () => $dbForPlatform->createDocument('rules', new Document([
270-290: Fix authorization instance mismatch.Same issue: authorization skip is called on
$dbForProjectbut the operation is performed on$dbForPlatform.Apply this diff:
- $dbForProject->getAuthorization()->skip( + $dbForPlatform->getAuthorization()->skip( fn () => $dbForPlatform->createDocument('rules', new Document([src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php (1)
239-250: Inconsistent authorization context for platform database operation.This code uses
$dbForProject->getAuthorization()->skip(...)but then creates a document in$dbForPlatform. The authorization context should match the database being operated on.Apply this diff to use the correct authorization context:
-$schedule = $dbForProject->getAuthorization()->skip( +$schedule = $dbForPlatform->getAuthorization()->skip( fn () => $dbForPlatform->createDocument('schedules', new Document([src/Appwrite/Utopia/Response.php (1)
811-819: Potential fatal: $authorization may be uninitialized.output() reads $this->authorization->getRoles(); if setAuthorization() wasn’t called for a code path, PHP will throw “Typed property must not be accessed before initialization.” Add a safe default or nullability guard.
Apply this minimal defensive fix:
- private Authorization $authorization; + private ?Authorization $authorization = null; @@ - if ($rule['sensitive']) { - $roles = $this->authorization->getRoles(); + if ($rule['sensitive']) { + $roles = $this->authorization ? $this->authorization->getRoles() : []; $isPrivilegedUser = Auth::isPrivilegedUser($roles); $isAppUser = Auth::isAppUser($roles);This preserves behavior when authorization is injected and avoids fatals otherwise (defaulting to hiding sensitive fields).
Also applies to: 981-986
app/controllers/api/migrations.php (1)
346-351: Fix cross‑DB skip: bypass must match the DB you queryYou skip authorization on the platform DB but execute reads on the project DB inside the closure. The bypass won’t apply to $dbForProject, which can cause false 401/404 on private buckets/files. Use the matching DB’s authorization per branch.
Apply:
- $bucket = $dbForPlatform->getAuthorization()->skip(function () use ($internalFile, $dbForPlatform, $dbForProject, $bucketId) { - if ($internalFile) { - return $dbForPlatform->getDocument('buckets', 'default'); - } - return $dbForProject->getDocument('buckets', $bucketId); - }); + $bucket = $internalFile + ? $dbForPlatform->getAuthorization()->skip(fn () => $dbForPlatform->getDocument('buckets', 'default')) + : $dbForProject->getAuthorization()->skip(fn () => $dbForProject->getDocument('buckets', $bucketId));- $file = $dbForPlatform->getAuthorization()->skip(fn () => $internalFile ? $dbForPlatform->getDocument('bucket_' . $bucket->getSequence(), $fileId) : $dbForProject->getDocument('bucket_' . $bucket->getSequence(), $fileId)); + $file = $internalFile + ? $dbForPlatform->getAuthorization()->skip(fn () => $dbForPlatform->getDocument('bucket_' . $bucket->getSequence(), $fileId)) + : $dbForProject->getAuthorization()->skip(fn () => $dbForProject->getDocument('bucket_' . $bucket->getSequence(), $fileId));Also applies to: 357-357
app/controllers/api/vcs.php (2)
332-353: Use platform DB’s authorization when writing platform ‘rules’The closure creates documents on $dbForPlatform but you call skip() on $dbForProject. The bypass won’t apply to platform writes.
Apply:
- $dbForProject->getAuthorization()->skip( + $dbForPlatform->getAuthorization()->skip( fn () => $dbForPlatform->createDocument('rules', new Document([Repeat the same replacement for the branch preview and commit preview rule creations in this block.
Also applies to: 365-385, 397-416
1667-1667: Fix createGitDeployments call: wrong arity and orderingThe helper expects 19 parameters. You pass 11 in the wrong order; this will fatal.
Apply:
- $createGitDeployments($github, $providerInstallationId, $repositories, $providerBranch, $providerCommitHash, $providerPullRequestId, true, $dbForPlatform, $queueForBuilds, $getProjectDB, $request); + $providerBranchUrl = "https://github.com/{$owner}/{$repositoryName}/tree/{$providerBranch}"; + $providerRepositoryUrl = "https://github.com/{$owner}/{$repositoryName}"; + $createGitDeployments( + $github, + $providerInstallationId, + $repositories, + $providerBranch, + $providerBranchUrl, + $repositoryName, + $providerRepositoryUrl, + $owner, + $providerCommitHash, + '', // providerCommitAuthor + '', // providerCommitAuthorUrl + '', // providerCommitMessage + '', // providerCommitUrl + $providerPullRequestId, + true, + $dbForPlatform, + $queueForBuilds, + $getProjectDB, + $request + );app/controllers/api/graphql.php (1)
23-23: Wrong Authorization type imported; will crash at runtimeValidator\Authorization doesn’t expose getRoles() and won’t match the injected resource. Import the runtime Authorization class instead.
Apply:
- use Utopia\Database\Validator\Authorization; + use Utopia\Database\Authorization;Also applies to: 31-37
app/controllers/api/account.php (2)
1743-1747: Typo: TYPE::EMAIL should be Type::EMAILThis will fatal (class 'TYPE' not found).
Apply:
- 'factors' => [TYPE::EMAIL, 'oauth2'], // include a special oauth2 factor to bypass MFA checks + 'factors' => [Type::EMAIL, 'oauth2'], // include a special oauth2 factor to bypass MFA checks
1000-1120: Fix spacing in-> skip(to->skip(at line 1076One occurrence found. Remove space after
->beforeskip:$dbForProject->getAuthorization()->skip(fn () => $dbForProject->createDocument('users', $user));app/controllers/api/messaging.php (5)
2142-2164: APNS: credential key mismatch breaks enablementYou set credentials['bundle'] but the enable check expects 'bundleId'. This prevents enabling APNS providers.
- if (!empty($bundleId)) { - $credentials['bundle'] = $bundleId; - } + if (!empty($bundleId)) { + $credentials['bundleId'] = $bundleId; + }
2316-2331: Topics listing cursor: assigning a Document as array index causes fatalgetDocument returns a Document; using $cursorDocument[0] will error. Assign the Document directly.
- $cursor->setValue($cursorDocument[0]); + $cursor->setValue($cursorDocument);
4356-4395: Push update: wrong exception and mime whitelist drift (mirror of create)Same fixes as createPush.
- $file = $dbForProject->getDocument('bucket_' . $bucket->getSequence(), $fileId); - if ($file->isEmpty()) { - throw new Exception(Exception::STORAGE_BUCKET_NOT_FOUND); - } - - if (!\in_array($file->getAttribute('mimeType'), ['image/png', 'image/jpeg'])) { + $file = $dbForProject->getDocument('bucket_' . $bucket->getSequence(), $fileId); + if ($file->isEmpty()) { + throw new Exception(Exception::STORAGE_FILE_NOT_FOUND); + } + + if (!\in_array($file->getAttribute('mimeType'), ['image/png', 'image/jpeg', 'image/bmp'])) { throw new Exception(Exception::STORAGE_FILE_TYPE_UNSUPPORTED); }
396-405: MSG91 create: enable-gating check always fails due to nonexistentoptions['from']; align with update endpointThe enable check at lines 396–405 validates
array_key_exists('from', $options), but$optionsis initialized empty and never populated in the MSG91 create function. This causes the enable guard to always fail. The MSG91 update endpoint correctly checks fortemplateIdin credentials; the create endpoint should do the same, sincetemplateIdis an accepted parameter (line 371) and stored in credentials (line 385).- if ( - $enabled === true - && \array_key_exists('senderId', $credentials) - && \array_key_exists('authKey', $credentials) - && \array_key_exists('from', $options) - ) { + if ( + $enabled === true + && \array_key_exists('senderId', $credentials) + && \array_key_exists('authKey', $credentials) + && \array_key_exists('templateId', $credentials) + ) { $enabled = true; } else { $enabled = false; }
3359-3398: Fix push create: undefined $currentScheduledAt, wrong exception, and mime whitelist drift
- File: app/controllers/api/messaging.php (approx. lines 3359–3398)
- if ($file->isEmpty()) => throw new Exception(Exception::STORAGE_FILE_NOT_FOUND);
- Allow BMP consistently: add 'image/bmp' to the mime whitelist or remove "bmp" from the param/docs if downstream push adapters do not support BMP.
- Replace $scheduleTime = $currentScheduledAt ?? $scheduledAt; with $scheduleTime = $scheduledAt; ($currentScheduledAt is undefined in the create action).
Verify BMP support in downstream push adapters; if unsupported, remove BMP from docs instead of changing the whitelist.
app/realtime.php (2)
457-466: Role recomputation should use Auth::getRoles(...)Authorization->getRoles($user) is inconsistent with the rest of the codebase and may not exist. Use Auth::getRoles($user, $authorization).
- $roles = $consoleDatabase->getAuthorization()->getRoles($user); + $roles = Auth::getRoles($user, $consoleDatabase->getAuthorization());
73-121: Fix getProjectDB signature to accept and apply Authorization parameterThe function is called with two arguments at line 641 (
getProjectDB($project, $database->getAuthorization())) but only accepts one. This causes the authorization context to be dropped. Additionally, the cached Database instance never gets updated with per-request authorization, unlike the implementations inapp/worker.phpandapp/init/resources.phpwhich applysetAuthorization()on each call.Update
app/realtime.phplines 73–121 to accept and apply the Authorization parameter:-if (!function_exists('getProjectDB')) { - function getProjectDB(Document $project): Database +if (!function_exists('getProjectDB')) { + function getProjectDB(Document $project, ?Authorization $authorization = null): Database { global $register; static $databases = []; if (isset($databases[$project->getSequence()])) { - return $databases[$project->getSequence()]; + $database = $databases[$project->getSequence()]; + if ($authorization) { + $database->setAuthorization($authorization); + } + return $database; }Also update the return statement to apply authorization before caching:
$database ->setMetadata('host', \gethostname()) ->setMetadata('project', $project->getId()); + if ($authorization) { + $database->setAuthorization($authorization); + } + return $databases[$project->getSequence()] = $database;src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php (2)
125-130: Fix Datetime validator constant referenceWrong class name in constant reference will fatal.
- $validator = new DatetimeValidator(requireDateInFuture: true, precision: DateTimeValidator::PRECISION_MINUTES, offset: 60); + $validator = new DatetimeValidator(requireDateInFuture: true, precision: DatetimeValidator::PRECISION_MINUTES, offset: 60);
135-140: Parse assoc JSON params: condition checks 'headers' string instead of variableThe guard always evaluates truthy and will json_decode non-strings. Use the variable.
- foreach ($assocParams as $assocParam) { - if (!empty('headers') && !is_array($$assocParam)) { - $$assocParam = \json_decode($$assocParam, true); - } - } + foreach ($assocParams as $assocParam) { + if (!empty($$assocParam) && !is_array($$assocParam)) { + $$assocParam = \json_decode($$assocParam, true); + } + }app/controllers/general.php (3)
922-993: Always re-enable authorization — wrap disable/reset in try/finally.If an exception occurs between disable() and reset(), authorization can remain disabled for the remainder of the request. Use a finally block.
- $dbForPlatform->getAuthorization()->disable(); - ... - $domains[$domain->get()] = true; - - $dbForPlatform->getAuthorization()->reset(); // ensure authorization is re-enabled + $dbForPlatform->getAuthorization()->disable(); + try { + ... + $domains[$domain->get()] = true; + } finally { + // ensure authorization is re-enabled even on exceptions + $dbForPlatform->getAuthorization()->reset(); + }
700-736: Guard against undefined $executionResponse after exceptions.If the try block throws before $executionResponse is set, later loops read it unconditionally. Initialize it and null‑coalesce headers in the loop.
@@ - /** Execute function */ - try { + /** Execute function */ + // Default to safe fallback in case execution fails early + $executionResponse = [ + 'statusCode' => 500, + 'headers' => [], + 'body' => '', + 'duration' => 0, + 'logs' => '', + 'errors' => '', + ]; + try { @@ - foreach ($executionResponse['headers'] as $name => $values) { + foreach (($executionResponse['headers'] ?? []) as $name => $values) {Also applies to: 711-731, 512-525
1527-1529: Fix path traversal check (operator precedence bug).The negation is applied to the substring, not the comparison. This condition never triggers, weakening the safety check.
- if (!\substr($absolute, 0, \strlen($base)) === $base) { + if (\substr($absolute, 0, \strlen($base)) !== $base) { throw new AppwriteException(AppwriteException::GENERAL_UNAUTHORIZED_SCOPE, 'Invalid path'); }app/controllers/api/teams.php (1)
471-479: Fix roles whitelist: unused array_filter and stray semicolon allow reserved roles.array_filter result isn’t assigned; reserved roles may remain allowed. Also remove the no‑op semicolon.
Apply:
- if ($project->getId() === 'console') { - ; - $roles = array_keys(Config::getParam('roles', [])); - array_filter($roles, function ($role) { - return !in_array($role, [Auth::USER_ROLE_APPS, Auth::USER_ROLE_GUESTS, Auth::USER_ROLE_USERS]); - }); + if ($project->getId() === 'console') { + $roles = array_keys(Config::getParam('roles', [])); + $roles = array_values(array_filter($roles, function ($role) { + return !in_array($role, [Auth::USER_ROLE_APPS, Auth::USER_ROLE_GUESTS, Auth::USER_ROLE_USERS], true); + })); return new ArrayList(new WhiteList($roles), APP_LIMIT_ARRAY_PARAMS_SIZE);app/cli.php (1)
176-203: getLogsDB cache bug: $database captured by value; caching won’t persist.Use by‑reference capture so subsequent invocations reuse the instance.
-CLI::setResource('getLogsDB', function (Group $pools, Cache $cache, Authorization $authorization) { +CLI::setResource('getLogsDB', function (Group $pools, Cache $cache, Authorization $authorization) { $database = null; - return function (?Document $project = null) use ($pools, $cache, $database, $authorization) { + return function (?Document $project = null) use ($pools, $cache, &$database, $authorization) {
🧹 Nitpick comments (31)
src/Appwrite/Platform/Modules/Functions/Http/Variables/Delete.php (1)
80-82: Remove duplicate empty check.Lines 76–78 already verify that
$variableis not empty (among other conditions). This second empty check is unreachable and can be safely removed.Apply this diff:
- if ($variable === false || $variable->isEmpty()) { - throw new Exception(Exception::VARIABLE_NOT_FOUND); - } -src/Appwrite/Platform/Modules/Functions/Http/Functions/Deployment/Update.php (1)
114-120: Verify if the nestedskip()call is necessary.The
updateDocumentoperation on line 119 is already wrapped in askip()call on line 114 that encompasses the entireforeachoperation. The innerskip()on line 119 appears redundant, as authorization should already be bypassed within the outer closure's scope.Please verify whether the nested skip is necessary or if the inner call can be removed.
If the inner skip is confirmed redundant, apply this diff:
$dbForPlatform->getAuthorization()->skip(fn () => $dbForPlatform->foreach('rules', function (Document $rule) use ($dbForPlatform, $deployment) { $rule = $rule ->setAttribute('deploymentId', $deployment->getId()) ->setAttribute('deploymentInternalId', $deployment->getSequence()); - $dbForPlatform->getAuthorization()->skip(fn () => $dbForPlatform->updateDocument('rules', $rule->getId(), $rule)); + $dbForPlatform->updateDocument('rules', $rule->getId(), $rule); }, $queries));docker-compose.yml (1)
92-92: Guard the vendor source mount behind a compose profile.Great for iterating on utopia-php/database locally, but it can mismatch composer’s installed version. Recommend enabling this only under a dedicated profile to avoid accidental use outside dev.
Example:
appwrite: volumes: @@ - - ./vendor/utopia-php/database/src:/usr/src/code/vendor/utopia-php/database/src + # Enable only when hacking on utopia-php/database + - ./vendor/utopia-php/database/src:/usr/src/code/vendor/utopia-php/database/src + # profiles: + # - hack-dbThen run with: docker compose --profile hack-db up.
Please confirm this file isn’t used in any CI or prod-like environment.
src/Appwrite/Platform/Modules/Functions/Http/Variables/Update.php (1)
106-112: Guard schedule update when schedule is absent.Updating the platform schedule without ensuring scheduleId exists and the doc is present can fail.
- $schedule = $dbForPlatform->getDocument('schedules', $function->getAttribute('scheduleId')); - $schedule - ->setAttribute('resourceUpdatedAt', DateTime::now()) - ->setAttribute('schedule', $function->getAttribute('schedule')) - ->setAttribute('active', !empty($function->getAttribute('schedule')) && !empty($function->getAttribute('deploymentId'))); - $dbForPlatform->getAuthorization()->skip(fn () => $dbForPlatform->updateDocument('schedules', $schedule->getId(), $schedule)); + $scheduleId = $function->getAttribute('scheduleId'); + if (!empty($scheduleId)) { + $schedule = $dbForPlatform->getDocument('schedules', $scheduleId); + if (!$schedule->isEmpty()) { + $schedule + ->setAttribute('resourceUpdatedAt', DateTime::now()) + ->setAttribute('schedule', $function->getAttribute('schedule')) + ->setAttribute('active', !empty($function->getAttribute('schedule')) && !empty($function->getAttribute('deploymentId'))); + $dbForPlatform->getAuthorization()->skip( + fn () => $dbForPlatform->updateDocument('schedules', $schedule->getId(), $schedule) + ); + } + }Please confirm scheduleId is always set when variables change; if guaranteed, we can downgrade this to a no‑op.
src/Appwrite/Platform/Tasks/StatsResources.php (1)
63-66: Configure authorization once and use a single reference.Move disable() and setDefaultStatus(false) outside the loop (or guard with a one‑time flag) and consistently use $this->dbForPlatform inside the closure to avoid mixed references.
- Console::loop(function () use ($queue, $dbForPlatform) { - $dbForPlatform->getAuthorization()->disable(); - $dbForPlatform->getAuthorization()->setDefaultStatus(false); + // Configure auth once + $this->dbForPlatform->getAuthorization()->disable(); + $this->dbForPlatform->getAuthorization()->setDefaultStatus(false); + Console::loop(function () use ($queue) {src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/Action.php (1)
24-26: Auth check looks good; consider consistent permission access.Optional: use getPermissionsByType(Database::PERMISSION_READ) for consistency with other modules.
- if (!$dbForProject->getAuthorization()->isValid(new Input(Database::PERMISSION_READ, $bucket->getRead()))) { + if (!$dbForProject->getAuthorization()->isValid( + new Input(Database::PERMISSION_READ, $bucket->getPermissionsByType(Database::PERMISSION_READ)) + )) {src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/Create.php (1)
81-85: Return auth description on 401 and align permission accessor.Improve error context and consistency with sibling Action:
- $bucketPermission = $dbForProject->getAuthorization()->isValid(new Input(Database::PERMISSION_UPDATE, $bucket->getUpdate())); + $bucketPermission = $dbForProject->getAuthorization()->isValid( + new Input(Database::PERMISSION_UPDATE, $bucket->getPermissionsByType(Database::PERMISSION_UPDATE)) + ); @@ - $filePermission = $dbForProject->getAuthorization()->isValid(new Input(Database::PERMISSION_UPDATE, $file->getUpdate())); + $filePermission = $dbForProject->getAuthorization()->isValid( + new Input(Database::PERMISSION_UPDATE, $file->getPermissionsByType(Database::PERMISSION_UPDATE)) + ); @@ - if (!$bucketPermission && !$filePermission) { - throw new Exception(Exception::USER_UNAUTHORIZED); - } + if (!$bucketPermission && !$filePermission) { + throw new Exception(Exception::USER_UNAUTHORIZED, $dbForProject->getAuthorization()->getDescription()); + } @@ - } elseif (!$bucketPermission) { - throw new Exception(Exception::USER_UNAUTHORIZED); + } elseif (!$bucketPermission) { + throw new Exception(Exception::USER_UNAUTHORIZED, $dbForProject->getAuthorization()->getDescription()); }Also applies to: 88-90
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php (2)
145-150: Avoid unbounded scans; consider chunking.Query::limit(PHP_INT_MAX) and fetching database/collection metadata inside the main loop can be heavy on large transactions. Prefer paging logs (e.g., batched find with cursor/limit) and prefetching required metadata into a local cache keyed by internal IDs.
Also applies to: 286-293
233-261: Deduplicate repeated “set status=failed” updates.The same updateDocument('transactions', ..., ['status' => 'failed']) appears in multiple catch blocks. Extract a small helper/closure to reduce duplication and chances of drift.
tests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsGuestTest.php (1)
20-28: Cache the Authorization instance in testsReturn a stable instance to make cleanRoles()/addRole() deterministic across calls.
Apply:
- private $authorization; + private $authorization; @@ - public function getAuthorization(): Authorization - { - if (isset($this->authorization)) { - return $this->authorization; - } - return new Authorization(); - } + public function getAuthorization(): Authorization + { + if (!isset($this->authorization)) { + $this->authorization = new Authorization(); + } + return $this->authorization; + }Also applies to: 125-127, 147-149, 159-161, 235-237
tests/unit/Messaging/MessagingChannelsTest.php (1)
38-46: Return a stable Authorization instanceAvoid creating a new Authorization per call; cache it on first use.
- public function getAuthorization(): Authorization - { - if (isset($this->authorization)) { - return $this->authorization; - } - return new Authorization(); - } + public function getAuthorization(): Authorization + { + if (!isset($this->authorization)) { + $this->authorization = new Authorization(); + } + return $this->authorization; + }src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Get.php (1)
76-78: Minor: avoid duplicate getRoles() callsStore roles once to compute both flags.
- $isAPIKey = Auth::isAppUser($dbForProject->getAuthorization()->getRoles()); - $isPrivilegedUser = Auth::isPrivilegedUser($dbForProject->getAuthorization()->getRoles()); + $roles = $dbForProject->getAuthorization()->getRoles(); + $isAPIKey = Auth::isAppUser($roles); + $isPrivilegedUser = Auth::isPrivilegedUser($roles);src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Delete.php (1)
100-104: DRY: cache the Authorization instance locally.Reduce repeated getAuthorization() calls and ease future changes.
- $database = $dbForProject->getAuthorization()->skip(fn () => $dbForProject->getDocument('databases', $databaseId)); - $isAPIKey = Auth::isAppUser($dbForProject->getAuthorization()->getRoles()); - $isPrivilegedUser = Auth::isPrivilegedUser($dbForProject->getAuthorization()->getRoles()); + $auth = $dbForProject->getAuthorization(); + $database = $auth->skip(fn () => $dbForProject->getDocument('databases', $databaseId)); + $isAPIKey = Auth::isAppUser($auth->getRoles()); + $isPrivilegedUser = Auth::isPrivilegedUser($auth->getRoles());src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php (1)
78-82: Instance-based Authorization usage is consistent.Role checks and Authorization::skip(...) replacements via $dbForProject->getAuthorization() look correct.
Cache $auth locally to avoid repeated getAuthorization() calls:
- $isAPIKey = Auth::isAppUser($dbForProject->getAuthorization()->getRoles()); - $isPrivilegedUser = Auth::isPrivilegedUser($dbForProject->getAuthorization()->getRoles()); - $database = $dbForProject->getAuthorization()->skip(fn () => $dbForProject->getDocument('databases', $databaseId)); + $auth = $dbForProject->getAuthorization(); + $isAPIKey = Auth::isAppUser($auth->getRoles()); + $isPrivilegedUser = Auth::isPrivilegedUser($auth->getRoles()); + $database = $auth->skip(fn () => $dbForProject->getDocument('databases', $databaseId));Also applies to: 86-87, 114-115
app/init/database/filters.php (1)
347-357: Also skip auth when fetching targets to keep semantics consistent.You skip for subscribers, but not for the subsequent targets find. If callers rely on bypassing permissions for this derived lookup, wrap it too.
- if (\count($targetIds) > 0) { - return $database->skipValidation(fn () => $database->find('targets', [ - Query::equal('$sequence', $targetIds) - ])); - } + if (\count($targetIds) > 0) { + return $database->getAuthorization()->skip( + fn () => $database->skipValidation( + fn () => $database->find('targets', [ + Query::equal('$sequence', $targetIds) + ]) + ) + ); + }Please confirm whether the previous behavior also skipped auth for targets; adjust if necessary.
tests/unit/Auth/AuthTest.php (1)
32-34: tearDown relies on the cached instance.After fixing getAuthorization(), this remains correct. Optionally reset the property to null if you want a fresh instance per test.
public function tearDown(): void { $this->getAuthorization()->cleanRoles(); $this->getAuthorization()->addRole(Role::any()->toString()); + // Optionally: + // $this->authorization = null; }app/controllers/api/account.php (1)
5024-5052: Guard optional provider lookup; avoid empty-ID fetchOnly fetch provider when providerId is present; prevents unnecessary DB read and null internalId mismatches.
Apply:
- $provider = $dbForProject->getAuthorization()->skip(fn () => $dbForProject->getDocument('providers', $providerId)); + $provider = null; + if (!empty($providerId)) { + $provider = $dbForProject->getAuthorization()->skip(fn () => $dbForProject->getDocument('providers', $providerId)); + } @@ - 'providerId' => !empty($providerId) ? $providerId : null, - 'providerInternalId' => !empty($providerId) ? $provider->getSequence() : null, + 'providerId' => !empty($providerId) ? $providerId : null, + 'providerInternalId' => (!empty($providerId) && $provider && !$provider->isEmpty()) ? $provider->getSequence() : null,src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php (2)
79-79: Align permissions allowlist between validator and aggregationValidator allows WRITE, but aggregation only expands READ/UPDATE/DELETE. Make both consistent to avoid surprising behavior.
Option A: Allow WRITE in aggregation (and expand to the concrete permissions).
Option B (preferred here): Remove WRITE from the validator if not supported by this endpoint.Example (Option B):
- ->param('permissions', null, new Permissions(APP_LIMIT_ARRAY_PARAMS_SIZE, [Database::PERMISSION_READ, Database::PERMISSION_UPDATE, Database::PERMISSION_DELETE, Database::PERMISSION_WRITE]), 'An array of permissions strings. By default, the current permissions are inherited. [Learn more about permissions](https://appwrite.io/docs/permissions).', true) + ->param('permissions', null, new Permissions(APP_LIMIT_ARRAY_PARAMS_SIZE, [Database::PERMISSION_READ, Database::PERMISSION_UPDATE, Database::PERMISSION_DELETE]), 'An array of permissions strings. By default, the current permissions are inherited. [Learn more about permissions](https://appwrite.io/docs/permissions).', true)Also applies to: 130-135
99-103: Minor: cache $auth to reduce repetitionStore $auth = $dbForProject->getAuthorization() once and reuse. Small readability win.
Also applies to: 136-154, 151-155, 248-255
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php (2)
82-82: Align permissions allowlist with aggregationKeep validator and aggregation in sync to avoid accepting WRITE but never expanding it.
Apply same approach as suggested in Update.php (either add WRITE to aggregation or drop it from validator).
Also applies to: 126-127
107-116: Minor: cache $auth to reduce repetitionUse a local $auth = $dbForProject->getAuthorization() to simplify repeated calls.
Also applies to: 151-170, 202-205, 219-223, 255-258
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
180-182: Avoid repeated role lookups.Micro-optimization/readability: fetch roles once.
- $isAPIKey = Auth::isAppUser($dbForProject->getAuthorization()->getRoles()); - $isPrivilegedUser = Auth::isPrivilegedUser($dbForProject->getAuthorization()->getRoles()); + $roles = $dbForProject->getAuthorization()->getRoles(); + $isAPIKey = Auth::isAppUser($roles); + $isPrivilegedUser = Auth::isPrivilegedUser($roles);app/controllers/shared/api.php (1)
470-476: Reuse a single $roles variable per request block.You already compute $roles at Line 472; pass it through and reuse to avoid multiple getRoles() calls.
Also applies to: 572-576, 589-592, 871-873
app/controllers/api/storage.php (1)
431-442: Minor: cache roles and reuse across checks.Within each handler, fetch roles once and reuse for isAppUser/isPrivilegedUser and error messages.
- $isAPIKey = Auth::isAppUser($dbForProject->getAuthorization()->getRoles()); - $isPrivilegedUser = Auth::isPrivilegedUser($dbForProject->getAuthorization()->getRoles()); + $roles = $dbForProject->getAuthorization()->getRoles(); + $isAPIKey = Auth::isAppUser($roles); + $isPrivilegedUser = Auth::isPrivilegedUser($roles); @@ - $roles = $dbForProject->getAuthorization()->getRoles(); + // reuse $roles from aboveAlso applies to: 463-479
app/controllers/api/teams.php (2)
604-606: Prefer Role helper over string literal for owner check.Avoid string typos; use API:
-$isOwner = $dbForProject->getAuthorization()->hasRole('team:' . $team->getId() . '/owner'); +$isOwner = $dbForProject->getAuthorization()->hasRole(Role::team($team->getId(), 'owner')->toString());
1232-1266: Duplicate addRole for the same user; remove one.Role is added at Line 1236 and again at Line 1264. Keep one to avoid redundant mutations/log noise.
- $dbForProject->getAuthorization()->addRole(Role::user($user->getId())->toString()); + $dbForProject->getAuthorization()->addRole(Role::user($user->getId())->toString()); ... - $dbForProject->getAuthorization()->addRole(Role::user($userId)->toString()); + // already added aboveapp/init/resources.php (3)
457-491: Duplicate setAuthorization in getProjectDB (new-instance path).$configure() already sets authorization; the earlier call is redundant.
- $adapter = new DatabasePool($pools->get($dsn->getHost())); - $database = new Database($adapter, $cache); - $database->setAuthorization($authorization); + $adapter = new DatabasePool($pools->get($dsn->getHost())); + $database = new Database($adapter, $cache); $databases[$dsn->getHost()] = $database; $configure($database);
229-240: Use the injected Authorization directly for clarity.Since the same Authorization instance is set on both DBs, calling setDefaultStatus on it directly is clearer.
- $dbForPlatform->getAuthorization()->setDefaultStatus(true); + $authorization = $dbForPlatform->getAuthorization(); + $authorization->setDefaultStatus(true);
496-523: Authorization not set on adapter in init/resources.php.Verified: workers set
AuthorizationonDatabasePooladapter (app/worker.php:152), while all fourDatabasePoolinstances in app/init/resources.php (lines 399, 428, 486, 505) set it only on theDatabaseobject. The patterns differ consistently. If adapter-level authorization is required for feature parity, align the init/resources.php instances accordingly.app/worker.php (1)
58-68: Optional: add standard timeouts/metadata to dbForPlatform (parity with API).Not required, but aligning with API defaults (timeouts, metadata) could aid observability and consistency.
app/cli.php (1)
150-154: Set authorization on the adapter in DatabasePool instantiations for consistency with worker.php.Verification confirms that
DatabasePool::setAuthorizationexists and is called inapp/worker.php(line 152), but not inapp/cli.phporapp/init/resources.php. All three locations instantiateDatabasePoolbut only worker.php calls$adapter->setAuthorization($authorization)before wrapping it in aDatabaseobject.The adapter propagates its authorization to underlying pooled adapters when delegating calls, so aligning cli.php (lines 76, 150, 185) and init/resources.php (lines 399, 428, 486, 505) with the worker pattern would ensure consistent behavior across entry points.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Operations/Create.php
Outdated
Show resolved
Hide resolved
tests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsGuestTest.php
Show resolved
Hide resolved
…base, reflecting new versioning and aliases. Refactor document validation in Create operation to utilize the new authorization method for better security handling.
…h-single-instance
…nto refactor-auth-single-instance
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)
app/controllers/api/messaging.php (1)
2503-2509: Fix array indexing bug on cursor document.Line 2509 incorrectly uses array indexing
$cursorDocument[0]on a Document object. ThegetDocumentmethod returns a Document object, not an array. This pattern is inconsistent with similar code elsewhere in the file (lines 1104, 2915) and will cause runtime errors.Apply this diff to fix the issue:
- $cursor->setValue($cursorDocument[0]); + $cursor->setValue($cursorDocument);
♻️ Duplicate comments (1)
composer.json (1)
51-51: Remove or stabilize development branch dependencies before releasing.Both
utopia-php/auditandutopia-php/databaseare pinned to development branches (dev-feat-auth-single-instanceanddev-feat-authorization-instance). This is problematic for a production release:
- utopia-php/database (line 55): As noted in the previous review,
feat-authorization-instanceremains unmerged and diverged from main (31 commits ahead, 5 behind). Stable version 3.0.2 is already available.- utopia-php/audit (line 51): Same concern applies—dev branches introduce instability and obscure dependency management.
Before merging to a release branch (1.8.x), replace these with stable versions:
- "utopia-php/audit": "dev-feat-auth-single-instance as 1.1.0", + "utopia-php/audit": "^1.1.0",- "utopia-php/database": "dev-feat-authorization-instance as 3.0.2", + "utopia-php/database": "^3.0.2",Alternatively, merge both feature branches into their respective
mainbranches and publish stable releases before referencing them here.Also applies to: 55-55
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.github/labeler.yml(1 hunks).github/workflows/auto-label-issue.yml(1 hunks)app/controllers/api/messaging.php(13 hunks)app/controllers/api/vcs.php(17 hunks)composer.json(1 hunks)docker-compose.yml(1 hunks)src/Appwrite/Platform/Modules/Compute/Base.php(3 hunks)src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php(2 hunks)src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Appwrite/Platform/Modules/Compute/Base.php
- src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php
- src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
- app/controllers/api/vcs.php
- docker-compose.yml
🧰 Additional context used
🧬 Code graph analysis (2)
src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php (4)
tests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsGuestTest.php (1)
getAuthorization(22-28)tests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsGuestTest.php (1)
getAuthorization(22-28)tests/unit/Messaging/MessagingChannelsTest.php (1)
getAuthorization(40-46)tests/unit/Auth/AuthTest.php (1)
getAuthorization(18-24)
app/controllers/api/messaging.php (4)
tests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsGuestTest.php (1)
getAuthorization(22-28)tests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsGuestTest.php (1)
getAuthorization(22-28)tests/unit/Messaging/MessagingChannelsTest.php (1)
getAuthorization(40-46)tests/unit/Auth/AuthTest.php (1)
getAuthorization(18-24)
🪛 GitHub Actions: Linter
composer.json
[error] 1-1: Lock file is not up to date with composer.json and required package mismatch: utopia-php/detector (0.2.0) does not satisfy constraint "0.1.*". Run 'composer update' to resolve.
[error] 1-1: Commit contains merge/conflict edits to composer.json. Prefer using 'composer require' to modify dependencies rather than editing directly.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Benchmark
- GitHub Check: E2E Shared Mode Service Test (Webhooks, Shared V1)
- GitHub Check: E2E Shared Mode Service Test (Databases/TablesDB, Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Projects, Shared V1)
- GitHub Check: E2E Shared Mode Service Test (Projects, Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Databases/Legacy, Shared V2)
- GitHub Check: E2E Shared Mode Service Test (FunctionsSchedule, Shared V2)
- GitHub Check: E2E Shared Mode Service Test (GraphQL, Shared V1)
- GitHub Check: E2E Shared Mode Service Test (GraphQL, Shared V2)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V1)
🔇 Additional comments (12)
.github/workflows/auto-label-issue.yml (1)
1-21: Workflow configuration looks good.The setup is clean: proper permissions (minimal scope), correct trigger for automated issue labeling, and sensible action configuration to include both title and body in label matching.
.github/labeler.yml (1)
62-75: Overlapping keywords may cause issues to receive multiple labels.The keywords "domain", "ssl", and "certificate" appear in both "product / domains" (line 63) and "product / sites" (line 75). If an issue mentions these terms, it will likely receive both labels.
Verify whether this overlap is intentional (e.g., for issues spanning both domains and sites) or if the patterns should be made more specific to avoid double-labeling.
src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php (1)
284-284: LGTM! Clean refactoring to instance-based authorization.The change correctly replaces the static
Authorization::skip()call with$dbForPlatform->getAuthorization()->skip(), aligning with the PR's architectural shift to per-instance authorization. The logic and behavior remain unchanged.app/controllers/api/messaging.php (9)
38-38: LGTM! Import updated correctly for the new authorization pattern.The import change from
Authorizationvalidator toAuthorization\Inputvalidator correctly supports the refactored authorization handling throughout this file.
1098-1098: LGTM! Authorization skip correctly applied.The refactored authorization skip pattern correctly uses the per-instance authorization object with a closure.
2782-2789: LGTM! Authorization validation correctly refactored.The authorization validation pattern has been properly updated to use:
- Instance-based
$dbForProject->getAuthorization()->isValid(new Input(...))for validation$dbForProject->getAuthorization()->getDescription()for descriptive error messagesThis correctly implements the per-instance authorization approach.
2791-2797: LGTM! Authorization skip correctly applied for related document retrieval.The authorization skip patterns for fetching target and user documents are correctly implemented using the per-instance authorization object with closures.
2830-2834: LGTM! Authorization skip correctly applied for attribute modification.The authorization skip pattern for
increaseDocumentAttributeis properly implemented using the per-instance authorization object.
2886-2926: LGTM! Authorization skip patterns correctly applied.All authorization skip patterns in this section are correctly implemented:
- Topic, subscriber, target, and user document retrieval all properly use the per-instance authorization object
- Batch processing operations correctly maintain the authorization pattern within closures
3062-3075: LGTM! Authorization skip patterns consistently applied.The authorization skip patterns for topic, target, and user document retrieval are correctly implemented using the per-instance authorization approach.
3113-3141: LGTM! Authorization skip correctly applied for document operations.The authorization skip patterns for topic retrieval and
decreaseDocumentAttributeoperation are properly implemented using the per-instance authorization object.
3719-3719: LGTM! Authorization skip correctly applied.The authorization skip pattern for message document retrieval is properly implemented using the per-instance authorization object, consistent with the refactoring throughout the file.
…or, remove unused registers.php file, and clean up whitespace in cli.php and worker.php. Refactor database resource initialization in resources.php for consistency.
…th-single-instance # Conflicts: # composer.json # composer.lock
…nto refactor-auth-single-instance
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/cli.php (1)
24-24: Import DB Authorization (not validator) and reapply on reused DBs.
- Import the correct class so
disable()and DB wiring work.- When reusing a cached Database instance, reapply the current authorization to ensure roles/context propagate across tasks.
Apply:
- use Utopia\Database\Validator\Authorization; + use Utopia\Database\Authorization; @@ -CLI::setResource('authorization', function () { +CLI::setResource('authorization', function () { $authorization = new Authorization(); $authorization->disable(); - return $authorization; + return $authorization; }, []); @@ - if (isset($databases[$dsn->getHost()])) { - $database = $databases[$dsn->getHost()]; + if (isset($databases[$dsn->getHost()])) { + $database = $databases[$dsn->getHost()]; + // Ensure current auth context is applied on reuse + $database->setAuthorization($authorization); $sharedTables = \explode(',', System::getEnv('_APP_DATABASE_SHARED_TABLES', ''));Also applies to: 60-65, 116-174, 176-203
app/worker.php (1)
32-32: Fix Authorization import and remove invalid adapter->setAuthorization() callThe import must be
Utopia\Database\Authorization(not the Validator class), andsetAuthorization()can only be called onDatabaseinstances, not on the adapter.Changes required at line 32 and line 152:
- use Utopia\Database\Validator\Authorization; + use Utopia\Database\Authorization; @@ - $adapter = new DatabasePool($pools->get($dsn->getHost())); - $adapter->setAuthorization($authorization); - $database = new Database($adapter, $cache); - $database->setAuthorization($authorization); + $adapter = new DatabasePool($pools->get($dsn->getHost())); + $database = new Database($adapter, $cache); + $database->setAuthorization($authorization);
♻️ Duplicate comments (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Operations/Create.php (1)
169-181: Fix Input casing and normalize permission source.
- Use
new Input(...)(capital “I”); currentnew input(...)will fatally error.- Prefer a consistent source for roles. Passing full
$resource->getPermissions()with the requested type toInputavoids API mismatches.Apply:
- $collectionValid = $dbForProject->getAuthorization()->isValid( - new input($permissionType, $collection->getPermissionsByType($permissionType)) - ); + $collectionValid = $dbForProject->getAuthorization()->isValid( + new Input($permissionType, $collection->getPermissions()) + ); @@ - if ($permissionType === Database::PERMISSION_UPDATE) { - $documentValid = $dbForProject->getAuthorization()->isValid( - new input(Database::PERMISSION_UPDATE, $document->getUpdate()) - ); - } elseif ($permissionType === Database::PERMISSION_DELETE) { - $documentValid = $dbForProject->getAuthorization()->isValid( - new input(Database::PERMISSION_DELETE, $document->getDelete()) - ); - } + if ($permissionType === Database::PERMISSION_UPDATE) { + $documentValid = $dbForProject->getAuthorization()->isValid( + new Input(Database::PERMISSION_UPDATE, $document->getPermissions()) + ); + } elseif ($permissionType === Database::PERMISSION_DELETE) { + $documentValid = $dbForProject->getAuthorization()->isValid( + new Input(Database::PERMISSION_DELETE, $document->getPermissions()) + ); + }If you intentionally rely on per-type getters (e.g.,
getUpdate()), keep them but still fix the casing:- new input(Database::PERMISSION_UPDATE, $document->getUpdate()) + new Input(Database::PERMISSION_UPDATE, $document->getUpdate())Run to catch any remaining issues:
#!/bin/bash # 1) Lowercase Input constructors rg -nP --type=php 'new\s+input\s*\(' # 2) Mixed permission APIs - audit availability rg -nP --type=php '\->getPermissionsByType\(|\->get(Update|Delete)\(\)|\->getPermissions\(\)' src -C2
🧹 Nitpick comments (1)
app/init/resources.php (1)
229-341: Consider injecting the Authorization into Request/Response early.If not already done upstream, call
$request->setAuthorization($authorization)and$response->setAuthorization($authorization)during bootstrap to keep the per-request context consistent. As per provided setters.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
app/cli.php(7 hunks)app/init/registers.php(0 hunks)app/init/resources.php(18 hunks)app/worker.php(11 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Operations/Create.php(7 hunks)
💤 Files with no reviewable changes (1)
- app/init/registers.php
🧰 Additional context used
🧬 Code graph analysis (4)
app/cli.php (2)
src/Appwrite/Utopia/Request.php (1)
setAuthorization(239-242)src/Appwrite/Utopia/Response.php (1)
setAuthorization(983-986)
app/worker.php (3)
src/Appwrite/Utopia/Request.php (1)
setAuthorization(239-242)src/Appwrite/Utopia/Response.php (1)
setAuthorization(983-986)src/Appwrite/Auth/Auth.php (1)
getRoles(456-502)
app/init/resources.php (3)
tests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsGuestTest.php (1)
getAuthorization(22-28)src/Appwrite/Utopia/Request.php (1)
setAuthorization(239-242)src/Appwrite/Utopia/Response.php (1)
setAuthorization(983-986)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Operations/Create.php (2)
src/Appwrite/Auth/Auth.php (4)
Auth(18-515)isAppUser(441-448)getRoles(456-502)isPrivilegedUser(421-432)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
getGroupId(218-221)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: scan
🔇 Additional comments (1)
app/init/resources.php (1)
41-41: Review comment is incorrect; no issues found.The
Utopia\Database\Validator\Authorizationclass (imported at line 41) is the correct and only Authorization class available in the Utopia Database library. It has all required methods:skip(),getRoles(),hasRole(),setDefaultStatus(), anddisable(). The codebase extensively uses these methods across 100+ locations without issue, confirming the implementation is correct. No non-ValidatorUtopia\Database\Authorizationclass exists to migrate to.Likely an incorrect or invalid review comment.
…a-php/detector to version 0.2.*, and utopia-php/migration to version 1.3.3. Modify exception messages in storage API for better clarity on authorization failures. Adjust test assertions to reflect updated error messages.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
composer.json (1)
55-55: Remove development branch dependency for database package (persistent critical issue).This issue was flagged in the prior review but remains unresolved. Using
dev-feat-authorization-instancefor production code introduces instability. A stable release (3.0.2) is available and should be used instead.Change line 55 to use the stable release:
- "utopia-php/database": "dev-feat-authorization-instance as 3.0.2", + "utopia-php/database": "^3.0.2",If the stable 3.0.2 release does not include required authorization features, the feature branch must be merged into main, a new stable version released, and then pinned to that release before this PR can be merged.
Verify branch status:
#!/bin/bash # Confirm stable 3.0.2 is available gh api repos/utopia-php/database/releases --jq '.[] | select(.tag_name == "3.0.2")' # Check feature branch status gh api repos/utopia-php/database/branches/feat-authorization-instance --jq '{name, commit: .commit.sha}' # Check divergence gh api repos/utopia-php/database/compare/main...feat-authorization-instance --jq '{ahead_by, behind_by, status}'
🧹 Nitpick comments (2)
app/controllers/api/storage.php (1)
34-34: LGTM! Authorization refactor is well-executed.The transition from static Authorization methods to instance-based authorization accessed via
$dbForProject->getAuthorization()is consistent and correct. The Input validator import and usage are appropriate, and error messages now include descriptive details viagetDescription().Optional: Cache the authorization object for improved readability.
Throughout the file,
getAuthorization()is called multiple times in quick succession within the same endpoint. While this is not a bug (the method returns the same instance), caching it in a local variable would reduce repetition and improve readability:+$authorization = $dbForProject->getAuthorization(); -$bucket = $dbForProject->getAuthorization()->skip(fn () => $dbForProject->getDocument('buckets', $bucketId)); +$bucket = $authorization->skip(fn () => $dbForProject->getDocument('buckets', $bucketId)); -$isAPIKey = Auth::isAppUser($dbForProject->getAuthorization()->getRoles()); -$isPrivilegedUser = Auth::isPrivilegedUser($dbForProject->getAuthorization()->getRoles()); +$isAPIKey = Auth::isAppUser($authorization->getRoles()); +$isPrivilegedUser = Auth::isPrivilegedUser($authorization->getRoles()); -if (!$dbForProject->getAuthorization()->isValid(new Input(Database::PERMISSION_CREATE, $bucket->getCreate()))) { - throw new Exception(Exception::USER_UNAUTHORIZED, $dbForProject->getAuthorization()->getDescription()); +if (!$authorization->isValid(new Input(Database::PERMISSION_CREATE, $bucket->getCreate()))) { + throw new Exception(Exception::USER_UNAUTHORIZED, $authorization->getDescription()); }This pattern applies to many endpoints in the file (e.g., lines 789-801, 889-902, 970-995, 1161-1185, 1321-1345, 1474-1499, 1647-1660, 1760-1786).
Also applies to: 431-441
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Operations/Create.php (1)
174-181: Tidy doc-permission check (optional).Reduce duplication and make intent explicit with a small match-based selection.
- if ($document !== null && !$document->isEmpty() && $documentSecurity) { - if ($permissionType === Database::PERMISSION_UPDATE) { - $documentValid = $dbForProject->getAuthorization()->isValid( - new Input(Database::PERMISSION_UPDATE, $document->getUpdate()) - ); - } elseif ($permissionType === Database::PERMISSION_DELETE) { - $documentValid = $dbForProject->getAuthorization()->isValid( - new Input(Database::PERMISSION_DELETE, $document->getDelete()) - ); - } - } + if ($document !== null && !$document->isEmpty() && $documentSecurity) { + $docPerms = match ($permissionType) { + Database::PERMISSION_UPDATE => $document->getUpdate(), + Database::PERMISSION_DELETE => $document->getDelete(), + default => null, + }; + if ($docPerms !== null) { + $documentValid = $dbForProject->getAuthorization()->isValid( + new Input($permissionType, $docPerms) + ); + } + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
app/controllers/api/storage.php(23 hunks)composer.json(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Operations/Create.php(7 hunks)tests/e2e/Services/Tokens/TokensBase.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/controllers/api/storage.php (5)
tests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsGuestTest.php (1)
getAuthorization(22-28)tests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsGuestTest.php (1)
getAuthorization(22-28)tests/unit/Messaging/MessagingChannelsTest.php (1)
getAuthorization(40-46)tests/unit/Auth/AuthTest.php (1)
getAuthorization(18-24)src/Appwrite/Auth/Auth.php (4)
Auth(18-515)isAppUser(441-448)getRoles(456-502)isPrivilegedUser(421-432)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Operations/Create.php (4)
src/Appwrite/Auth/Auth.php (4)
Auth(18-515)isAppUser(441-448)getRoles(456-502)isPrivilegedUser(421-432)tests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsGuestTest.php (1)
getAuthorization(22-28)src/Appwrite/Databases/TransactionState.php (1)
getDocument(41-82)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
getGroupId(218-221)
🔇 Additional comments (7)
tests/e2e/Services/Tokens/TokensBase.php (1)
82-141: LGTM! Error message updates correctly reflect the new authorization system.The updated error message assertions are correct and represent an improvement. The new message "No permissions provided for action 'read'" is more specific and actionable than the previous generic message, clearly indicating:
- What's missing (permissions)
- The attempted action (read)
All four test scenarios (preview, custom preview, view, download) consistently use the same message, which is appropriate since they all test the same authorization check for anonymous users attempting read access.
app/controllers/api/storage.php (2)
697-700: Permission validation pattern is correct.The logic of validating CREATE permission before skipping authorization for document updates during chunked file uploads is sound. This ensures that users with create permission can complete chunked uploads even without explicit update permission, which is the intended behavior.
The pattern is consistently applied in both complete and incomplete upload scenarios.
Also applies to: 739-744
1866-1889: Usage statistics authorization handling looks good.The use of
skip()with closures to bypass authorization for internal statistics queries is appropriate and consistent with the refactor pattern. This ensures that usage stats can be retrieved without being filtered by the current user's permissions.Also applies to: 1962-1989
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Operations/Create.php (4)
75-82: Nice: consistent instance-based Authorization usage.Clean switch to $dbForProject->getAuthorization() including skip() around internal reads.
195-215: Permissions mutation guard looks good.Validates that requested $permissions roles are within caller’s roles; aligns with least-privilege.
169-181: API names verified—review comment is incorrect.All methods referenced in the code are valid and exist in
Utopia\Database\Document:getUpdate()(line 117),getDelete()(line 125), andgetPermissionsByType()(line 145). The same API patterns are used elsewhere in the codebase without issues. No runtime errors will occur.Likely an incorrect or invalid review comment.
95-103: Review comment valid but verification incomplete—plan structure is external to codebase.The key
databasesTransactionSizeis used consistently across 10 files, reducing likelihood of a typo. However, the$planarray is injected via DI container (external to this codebase), so we cannot definitively verify the key exists in the actual plan structure. The ?? fallback toAPP_LIMIT_DATABASE_TRANSACTIONprovides protection against missing keys. Manual verification needed to confirm the plan configuration includesdatabasesTransactionSizekey in the DI bootstrap/config layer.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Operations/Create.php
Show resolved
Hide resolved
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)
app/controllers/api/migrations.php (1)
500-518: Incomplete migration to instance-based authorization.The CSV export action still uses static
Authorization::skip()calls (lines 500, 513, 518), while the CSV import action above was migrated to instance-based authorization. This inconsistency should be addressed to complete the refactoring.Apply this diff to complete the migration:
- $bucket = Authorization::skip(fn () => $dbForProject->getDocument('buckets', $bucketId)); + $bucket = $dbForProject->getAuthorization()->skip(fn () => $dbForProject->getDocument('buckets', $bucketId));- $database = Authorization::skip(fn () => $dbForProject->getDocument('databases', $databaseId)); + $database = $dbForProject->getAuthorization()->skip(fn () => $dbForProject->getDocument('databases', $databaseId));- $collection = Authorization::skip(fn () => $dbForProject->getDocument('database_' . $database->getSequence(), $collectionId)); + $collection = $dbForProject->getAuthorization()->skip(fn () => $dbForProject->getDocument('database_' . $database->getSequence(), $collectionId));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/controllers/api/migrations.php(2 hunks)app/init/resources.php(18 hunks)app/worker.php(11 hunks)src/Appwrite/Utopia/Request/Filters/V20.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/Appwrite/Utopia/Request/Filters/V20.php (2)
tests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsGuestTest.php (1)
getAuthorization(22-28)src/Appwrite/Databases/TransactionState.php (1)
getDocument(41-82)
app/worker.php (3)
src/Appwrite/Utopia/Request.php (1)
setAuthorization(239-242)src/Appwrite/Utopia/Response.php (1)
setAuthorization(983-986)src/Appwrite/Auth/Auth.php (1)
getRoles(456-502)
app/controllers/api/migrations.php (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Operations/Create.php (1)
action(69-252)src/Appwrite/Platform/Modules/Console/Http/Resources/Get.php (1)
action(64-139)src/Appwrite/Auth/Auth.php (4)
Auth(18-515)isAppUser(441-448)getRoles(456-502)isPrivilegedUser(421-432)
app/init/resources.php (6)
tests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsGuestTest.php (1)
getAuthorization(22-28)tests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsGuestTest.php (1)
getAuthorization(22-28)tests/unit/Messaging/MessagingChannelsTest.php (1)
getAuthorization(40-46)tests/unit/Auth/AuthTest.php (1)
getAuthorization(18-24)src/Appwrite/Utopia/Request.php (1)
setAuthorization(239-242)src/Appwrite/Utopia/Response.php (1)
setAuthorization(983-986)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (12)
src/Appwrite/Utopia/Request/Filters/V20.php (2)
140-143: LGTM! Authorization refactoring correctly applied.The change from static
Authorization::skip()to instance-based$dbForProject->getAuthorization()->skip()is correct and consistent with the broader refactoring effort to use per-instance authorization objects.
152-155: LGTM! Consistent authorization pattern.The authorization refactoring is applied consistently with the same pattern used on line 140. Both calls appropriately bypass authorization to fetch metadata documents needed for the filter's backward compatibility logic.
app/controllers/api/migrations.php (1)
341-357: LGTM!The authorization refactoring is correctly implemented:
- Authorization checks use the instance-based
$dbForPlatform->getAuthorization()->getRoles()pattern- Proper guard added for internal file access requiring privileged user
- Authorization skip operations correctly scoped to the platform database instance
app/init/resources.php (6)
383-385: LGTM!The new
authorizationresource correctly creates a per-request Authorization instance with no dependencies, establishing the foundation for instance-based authorization throughout the application.
387-424: LGTM!The
dbForProjectresource correctly integrates per-request authorization:
- Accepts
Authorization $authorizationparameter- Propagates it to the database instance via
setAuthorization()- Properly declares the dependency
426-440: LGTM!The
dbForPlatformresource follows the same correct pattern asdbForProject, properly integrating per-request authorization.
442-494: LGTM!The
getProjectDBresource factory correctly propagates authorization through nested closures and sets it on database instances in all code paths.
496-522: LGTM!The
getLogsDBresource factory follows the same correct pattern, properly propagating authorization through the closure and setting it on the database instance.
203-1009: LGTM!All static
Authorization::skip()and authorization method calls have been correctly migrated to instance-based calls using the appropriate database context ($dbForPlatformor$dbForProject).app/worker.php (3)
52-56: LGTM!The worker's
authorizationresource correctly disables authorization by default, which is appropriate for background processing tasks that typically run with elevated privileges.
58-203: LGTM!All worker database resources (
dbForPlatform,dbForProject,getProjectDB,getLogsDB) correctly integrate per-request authorization following the same patterns as the main application resources. Authorization is properly set on both adapters and database instances where needed.
491-508: LGTM!The error handler correctly integrates instance-based authorization, enabling it to capture the authorization context (roles) in error logs using
$authorization->getRoles()instead of static calls.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsGuestTest.php (1)
20-30: LGTM! Proper per-instance Authorization implementation.The lazy initialization pattern correctly caches the Authorization instance, which is appropriate for per-instance usage in tests. This implementation is consistent with similar patterns in
AuthTest.phpandMessagingChannelsTest.php.Minor: The extra blank line at Line 27 can be removed for consistency:
$this->authorization = new Authorization(); - return $this->authorization; + return $this->authorization; } -
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsGuestTest.php(5 hunks)tests/unit/Auth/AuthTest.php(7 hunks)tests/unit/Messaging/MessagingChannelsTest.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/Auth/AuthTest.php
- tests/unit/Messaging/MessagingChannelsTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsGuestTest.php (4)
tests/unit/Auth/AuthTest.php (1)
getAuthorization(18-26)tests/unit/Messaging/MessagingChannelsTest.php (1)
getAuthorization(41-49)tests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsGuestTest.php (1)
getAuthorization(22-28)src/Appwrite/Auth/Auth.php (1)
getRoles(456-502)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: scan
🔇 Additional comments (2)
tests/e2e/Services/Databases/TablesDB/Permissions/DatabasesPermissionsGuestTest.php (2)
127-128: LGTM! Correct conversion to instance-based Authorization.The refactoring correctly replaces static Authorization calls with instance-based calls:
getRoles()andcleanRoles()properly retrieve and clear roles- Using
addRole()in the restoration loop is semantically correct for adding multiple roles backThe pattern of saving roles, testing as a guest (no roles), and then restoring roles is appropriate for guest permission testing.
Also applies to: 149-151
161-162: LGTM! Consistent Authorization pattern.The same correct refactoring pattern is applied consistently in
testWriteRow, matching the implementation intestReadRows. The role management logic correctly saves, clears, and restores roles for guest permission testing.Also applies to: 237-239
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (14)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
241-261: Fix callable typo on Authorization instance
$authorizationis an object; invoking it as$authorization()will throw a fatal “Call to undefined function” when these code paths execute. Replace both occurrences with$authorization->skip(...)to restore the intended behavior.Apply this diff:
- $authorization()->skip( + $authorization->skip( fn () => $dbForPlatform->createDocument('rules', new Document([ '$id' => $ruleId, @@ - $authorization()->skip( + $authorization->skip( fn () => $dbForPlatform->createDocument('rules', new Document([ '$id' => $ruleId, @@ - $authorization()->skip( + $authorization->skip( fn () => $dbForPlatform->createDocument('rules', new Document([ '$id' => $ruleId,Also applies to: 267-325
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Email/Create.php (1)
79-96: Pass Authorization through to createAttribute
createAttributenow requires anAuthorizationargument. Omitting it will raise a parameter count error the first time this endpoint runs. Thread the$authorizationdown as done in the sibling attribute endpoints.Apply this diff:
$response, $dbForProject, $queueForDatabase, - $queueForEvents + $queueForEvents, + $authorization );app/controllers/api/messaging.php (1)
2509-2516: Fix cursor document assignment onlistTopicscursor path
$dbForProject->getDocument()already returns aDocument, so indexing with[0]now yieldsnull. As a result, the cursor value stops being aDocument, and cursor pagination throws once the database layer tries to read$cursor->getValue(). Please pass theDocumentitself:- $cursor->setValue($cursorDocument[0]); + $cursor->setValue($cursorDocument);app/controllers/api/vcs.php (1)
1707-1725: Fix the extra argument passed togetDocument.
Utopia\Database\Database::getDocument()only accepts the collection name and document ID. Supplying the third parameter here will immediately throw a “Too many arguments” fatal, so this route will never reach the authorization check. Please switch back to a supported call pattern, e.g. fetch viafindOne()and keep the project filter in the query.- $repository = $authorization->skip(fn () => $dbForPlatform->getDocument('repositories', $repositoryId, [ - Query::equal('projectInternalId', [$project->getSequence()]) - ])); + $repository = $authorization->skip(fn () => $dbForPlatform->findOne('repositories', [ + Query::equal('$id', [$repositoryId]), + Query::equal('projectInternalId', [$project->getSequence()]) + ]));app/controllers/api/account.php (2)
1760-1775: Bug: wrong class constantTYPE::EMAIL; useType::EMAIL.
TYPEis undefined; this will fatally error.Apply:
- 'factors' => [TYPE::EMAIL, 'oauth2'], // include a special oauth2 factor to bypass MFA checks + 'factors' => [Type::EMAIL, 'oauth2'], // include a special oauth2 factor to bypass MFA checks
4064-4074: Bug: token verification checks the wrong user's tokens.Public confirmation route should verify against the target profile’s tokens, not the current
$user.Apply:
- $verifiedToken = Auth::tokenVerify($user->getAttribute('tokens', []), Auth::TOKEN_TYPE_PHONE, $secret); + $verifiedToken = Auth::tokenVerify($profile->getAttribute('tokens', []), Auth::TOKEN_TYPE_PHONE, $secret);app/controllers/api/avatars.php (1)
845-858: Fix$getUserGitHubargument order to avoid TypeError.
$getUserGitHubnow expectsAuthorization $authorizationbefore?Logger $logger, but this call still passes$loggerfirst. At runtime this will throw aTypeError. Please swap the arguments here (and in the other new call sites at lines 1053‑1066 and 1132‑1149).- $gitHub = $getUserGitHub($user->getId(), $project, $dbForProject, $dbForPlatform, $logger, $authorization); + $gitHub = $getUserGitHub($user->getId(), $project, $dbForProject, $dbForPlatform, $authorization, $logger);app/controllers/api/teams.php (5)
475-485: Console roles whitelist is ineffective (array_filter result unused).The filtered array isn’t assigned back, so restricted roles remain allowed.
Apply:
- $roles = array_keys(Config::getParam('roles', [])); - array_filter($roles, function ($role) { - return !in_array($role, [Auth::USER_ROLE_APPS, Auth::USER_ROLE_GUESTS, Auth::USER_ROLE_USERS]); - }); + $roles = array_keys(Config::getParam('roles', [])); + $roles = array_values(array_filter( + $roles, + fn ($role) => !in_array($role, [Auth::USER_ROLE_APPS, Auth::USER_ROLE_GUESTS, Auth::USER_ROLE_USERS], true) + ));
1462-1466: Remove hard-coded “Temp fix for logs” date filters.This silently drops logs between fixed dates and will age badly.
- // Temp fix for logs - $queries[] = Query::or([ - Query::greaterThan('$createdAt', DateTime::format(new \DateTime('2025-02-26T01:30+00:00'))), - Query::lessThan('$createdAt', DateTime::format(new \DateTime('2025-02-13T00:00+00:00'))), - ]); + // TODO: If log window filtering is needed, accept from client or config; avoid hard-coded dates.
1116-1133: Last-owner protection should consider only confirmed owners.Counts include unconfirmed memberships; can block legitimate changes or allow unsafe ones.
$ownersCount = $dbForProject->count( collection: 'memberships', queries: [ Query::contains('roles', ['owner']), Query::equal('teamInternalId', [$team->getSequence()]) + , Query::equal('confirm', [true]) ], max: 2 );Apply similarly in delete-membership owner check.
Also applies to: 1380-1398
1246-1275: Duplicate role addition on session creation.Role user:{id} is added twice.
- $authorization->addRole(Role::user($user->getId())->toString()); ... - $authorization->addRole(Role::user($userId)->toString()); + // Add once is enough + $authorization->addRole(Role::user($userId)->toString());
928-933: Wrap user document reads with$authorization->skip().Both endpoints conditionally fetch user documents based on privacy configuration flags, but without bypassing ACL checks. If a non-privileged team member has privacy flags enabled and the user document has restrictive read ACLs,
getDocument('users', ...)will fail with 403 despite the privacy configuration intending to allow exposure.Apply
$authorization->skip()to the user document reads on lines 928–933 and 1023 (approximately) to ensure they succeed when privacy flags are true.app/controllers/api/storage.php (2)
1114-1114: Do not log full request URI (token leakage).Preview logs include the full URI, leaking token/resourceToken in logs.
- Console::info("File preview rendered,project=" . $project->getId() . ",bucket=" . $bucketId . ",file=" . $file->getId() . ",uri=" . $request->getURI() . ",total=" . $totalTime . ",rendering=" . $renderingTime . ",decryption=" . $decryptionTime . ",decompression=" . $decompressionTime . ",download=" . $downloadTime); + $pathOnly = parse_url($request->getURI(), PHP_URL_PATH) ?? ''; + Console::info("File preview rendered,project={$project->getId()},bucket={$bucketId},file={$file->getId()},uri={$pathOnly},total={$totalTime},rendering={$renderingTime},decryption={$decryptionTime},decompression={$decompressionTime},download={$downloadTime}");
809-813: Add try/catch around query parsing (consistent error mapping).Other endpoints map QueryException to GENERAL_QUERY_INVALID; do the same here.
- $queries = Query::parseQueries($queries); + try { + $queries = Query::parseQueries($queries); + } catch (QueryException $e) { + throw new Exception(Exception::GENERAL_QUERY_INVALID, $e->getMessage()); + }
♻️ Duplicate comments (3)
app/controllers/api/account.php (1)
1101-1101: Fix: assign created user and remove stray space before method call.Created user must be captured to populate $sequence; also call should be
->skip(...)(no space).Apply:
- $authorization-> skip(fn () => $dbForProject->createDocument('users', $user)); + $user = $authorization->skip(fn () => $dbForProject->createDocument('users', $user));app/controllers/api/users.php (1)
2627-2628: Fix the DI signature for/v1/users/usage.Line 2627 still calls
->inject('register'), but the action signature on Line 2628 no longer accepts a$registerargument. Utopia will pass the extra dependency and trigger anArgumentCountErrorat runtime. Please drop the injection (or reintroduce the parameter) so the signature matches.- ->inject('authorization') - ->inject('register') - ->action(function (string $range, Response $response, Database $dbForProject, Authorization $authorization) { + ->inject('authorization') + ->action(function (string $range, Response $response, Database $dbForProject, Authorization $authorization) {app/controllers/api/migrations.php (1)
345-349: Restore API key access to internal migration files.The updated guard now blocks API key callers from internal buckets. Previously both privileged users and API keys could pull from the internal bucket; with this change API key flows will start throwing
USER_UNAUTHORIZED. Please reinstate the API key allowance.- if ($internalFile && !$isPrivilegedUser) { + if ($internalFile && !$isAPIKey && !$isPrivilegedUser) { throw new Exception(Exception::USER_UNAUTHORIZED); }
🧹 Nitpick comments (7)
src/Appwrite/Platform/Modules/Functions/Http/Variables/Delete.php (1)
83-85: Optional: Remove duplicate empty check.Lines 83-85 duplicate the same check performed on lines 79-81. Since line 81 throws an exception if the variable is empty, this second check is unreachable.
Apply this diff to remove the duplicate check:
- if ($variable === false || $variable->isEmpty()) { - throw new Exception(Exception::VARIABLE_NOT_FOUND); - } - $dbForProject->deleteDocument('variables', $variable->getId());app/controllers/api/account.php (1)
243-244: Reduce duplicate role adds.Multiple repeated
addRole(Role::user(...))calls; consider guarding withhasRole(...)or centralizing to keep the set idempotent.Example pattern:
if (!$authorization->hasRole(Role::user($user->getId())->toString())) { $authorization->addRole(Role::user($user->getId())->toString()); }Also applies to: 963-964, 1127-1128, 1654-1656, 1714-1715, 1736-1737, 2043-2044, 2315-2316, 2673-2674, 3373-3374, 3658-3659
app/controllers/api/teams.php (1)
90-92: Minor: reuse computed roles.Cache
$roles = $authorization->getRoles()once per handler to avoid repeated calls.Also applies to: 501-503, 1110-1113
app/cli.php (1)
177-204: Consistent closure caching style.In getLogsDB you capture
$databaseby value; in resources.php it’s by reference. Consider aligning for clarity.- return function (?Document $project = null) use ($pools, $cache, $database, $authorization) { + return function (?Document $project = null) use ($pools, $cache, &$database, $authorization) {app/controllers/api/storage.php (2)
845-854: Simplify count: pass queries directly; DB handles filter separation.You can avoid pre-splitting filters when calling count, even inside skip blocks. Based on learnings.
- $filterQueries = Query::groupByType($queries)['filters']; ... - $files = $authorization->skip(fn () => $dbForProject->find('bucket_' . $bucket->getSequence(), $queries)); - $total = $authorization->skip(fn () => $dbForProject->count('bucket_' . $bucket->getSequence(), $filterQueries, APP_LIMIT_COUNT)); + $files = $authorization->skip(fn () => $dbForProject->find('bucket_' . $bucket->getSequence(), $queries)); + $total = $authorization->skip(fn () => $dbForProject->count('bucket_' . $bucket->getSequence(), $queries, APP_LIMIT_COUNT));Based on learnings
222-236: Optional: same simplification for buckets list count.If running on newer database versions, pass
$queriesdirectly to count for consistency.- $filterQueries = Query::groupByType($queries)['filters']; ... - $total = $includeTotal ? $dbForProject->count('buckets', $filterQueries, APP_LIMIT_COUNT) : 0; + $total = $includeTotal ? $dbForProject->count('buckets', $queries, APP_LIMIT_COUNT) : 0;Based on learnings
app/init/resources.php (1)
229-241: PhpDoc type for $authorization is wrong class.Doc says Utopia\Database\Authorization; actual is Utopia\Database\Validator\Authorization.
- /** @var Utopia\Database\Authorization $authorization */ + /** @var Utopia\Database\Validator\Authorization $authorization */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (107)
app/cli.php(8 hunks)app/config/storage/resource_limits.php(1 hunks)app/controllers/api/account.php(49 hunks)app/controllers/api/avatars.php(8 hunks)app/controllers/api/messaging.php(17 hunks)app/controllers/api/migrations.php(7 hunks)app/controllers/api/storage.php(25 hunks)app/controllers/api/teams.php(19 hunks)app/controllers/api/users.php(2 hunks)app/controllers/api/vcs.php(23 hunks)app/controllers/shared/api.php(13 hunks)app/init/resources.php(27 hunks)app/worker.php(9 hunks)src/Appwrite/Migration/Migration.php(1 hunks)src/Appwrite/Platform/Modules/Compute/Base.php(4 hunks)src/Appwrite/Platform/Modules/Console/Http/Resources/Get.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php(4 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Create.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Datetime/Create.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Email/Create.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Enum/Create.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Float/Create.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/IP/Create.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Integer/Create.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Line/Create.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Point/Create.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Polygon/Create.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Relationship/Create.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/String/Create.php(4 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/URL/Create.php(4 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/XList.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php(10 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Delete.php(5 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Get.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php(9 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php(10 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/XList.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/XList.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php(8 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Boolean/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Boolean/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Datetime/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Datetime/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Delete.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Email/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Email/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Enum/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Enum/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Float/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Float/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Get.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/IP/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/IP/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Integer/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Integer/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Line/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Line/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Point/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Point/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Polygon/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Polygon/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Relationship/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Relationship/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/String/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/String/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/URL/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/URL/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/XList.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Delete.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Get.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Indexes/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Indexes/Delete.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Indexes/Get.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Indexes/XList.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Logs/XList.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Bulk/Delete.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Bulk/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Bulk/Upsert.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Column/Decrement.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Column/Increment.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Delete.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Get.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Logs/XList.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Upsert.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/XList.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Usage/Get.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/XList.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Transactions/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Transactions/Operations/Create.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Transactions/Update.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Usage/Get.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Usage/XList.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Executions/XList.php(2 hunks)src/Appwrite/Platform/Modules/Functions/Http/Variables/Create.php(3 hunks)src/Appwrite/Platform/Modules/Functions/Http/Variables/Delete.php(3 hunks)src/Appwrite/Platform/Modules/Functions/Http/Variables/Update.php(3 hunks)src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(5 hunks)src/Appwrite/Platform/Modules/Sites/Http/Deployments/Create.php(4 hunks)src/Appwrite/Platform/Modules/Sites/Http/Deployments/Template/Create.php(4 hunks)src/Appwrite/Platform/Modules/Sites/Http/Deployments/Vcs/Create.php(4 hunks)
⛔ Files not processed due to max files limit (12)
- src/Appwrite/Platform/Modules/Sites/Http/Sites/Deployment/Update.php
- src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/Action.php
- src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/Create.php
- src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/XList.php
- src/Appwrite/Platform/Tasks/Migrate.php
- src/Appwrite/Platform/Tasks/ScheduleBase.php
- src/Appwrite/Platform/Tasks/StatsResources.php
- src/Appwrite/Platform/Workers/Deletes.php
- src/Appwrite/Platform/Workers/Functions.php
- src/Appwrite/Platform/Workers/Migrations.php
- src/Appwrite/Utopia/Request/Filter.php
- src/Appwrite/Utopia/Request/Filters/V20.php
🚧 Files skipped from review as they are similar to previous changes (11)
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/XList.php
- src/Appwrite/Platform/Modules/Functions/Http/Variables/Update.php
- src/Appwrite/Platform/Modules/Functions/Http/Executions/XList.php
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Relationship/Create.php
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php
- src/Appwrite/Platform/Modules/Sites/Http/Deployments/Template/Create.php
- src/Appwrite/Platform/Modules/Sites/Http/Deployments/Create.php
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/XList.php
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/XList.php
- src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-08-11T10:32:38.142Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10297
File: app/controllers/api/users.php:649-657
Timestamp: 2025-08-11T10:32:38.142Z
Learning: In Appwrite's utopia-php/database (version 0.77.* and later), the database layer internally handles filter separation for count operations. When using `$dbForProject->count()` within a `skipFilters` closure, there's no need to manually separate filter queries using `Query::groupByType($queries)['filters']` as the database handles this automatically.
Applied to files:
app/controllers/api/users.phpapp/controllers/api/storage.phpapp/controllers/api/messaging.phpsrc/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/XList.phpsrc/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/XList.phpsrc/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Indexes/XList.php
📚 Learning: 2025-06-19T09:20:03.312Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Delete.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Get.phpsrc/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Update.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php
📚 Learning: 2025-10-02T10:16:48.075Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10023
File: src/Appwrite/Databases/TransactionState.php:174-219
Timestamp: 2025-10-02T10:16:48.075Z
Learning: In `src/Appwrite/Databases/TransactionState.php`, the `listDocuments` method intentionally applies queries only to the initial database fetch of committed documents. Transaction state changes (creates, updates, deletes) are then overlaid on top of the filtered committed set without re-evaluating the queries against the final merged result. This is the intended behavior for transaction-aware document listing.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Delete.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php
📚 Learning: 2025-06-25T06:25:45.054Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Indexes/Delete.php:40-40
Timestamp: 2025-06-25T06:25:45.054Z
Learning: In Appwrite's database module, the event label for deleting a table index intentionally uses 'update' (e.g., 'databases.[databaseId].tables.[tableId].indexes.[indexId].update') instead of 'delete', to match legacy behavior as seen in previous controller implementations.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Indexes/Delete.php
📚 Learning: 2025-05-10T04:12:13.037Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Indexes/Delete.phpapp/controllers/api/migrations.phpsrc/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Indexes/Get.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: The Utopia\Queue\Broker\Pool class (aliased as BrokerPool) implements both Publisher and Consumer interfaces, making it directly compatible with constructors expecting Publisher parameters like Func::__construct(protected Publisher $publisher). No wrapping is needed.
Applied to files:
app/cli.php
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: In the Utopia Queue system, the Pool class implements both Publisher and Consumer interfaces, so BrokerPool (Pool) can be passed directly to constructors expecting Publisher parameters like Func::__construct(protected Publisher $publisher).
Applied to files:
app/cli.php
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: In Appwrite's codebase, Utopia\Queue\Broker\Pool (aliased as BrokerPool) is a readonly class that implements both Publisher and Consumer interfaces, allowing it to be used directly in constructors expecting either interface type. This is extensively used throughout the codebase for queue operations.
Applied to files:
app/cli.php
🧬 Code graph analysis (32)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Polygon/Create.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
createAttribute(295-473)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Datetime/Create.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Create.php (1)
action(76-90)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Enum/Create.php (1)
action(80-109)
src/Appwrite/Migration/Migration.php (1)
app/realtime.php (1)
getProjectDB(75-121)
src/Appwrite/Platform/Modules/Console/Http/Resources/Get.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
action(138-512)src/Appwrite/Platform/Modules/Sites/Http/Deployments/Template/Create.php (1)
action(84-215)
app/controllers/shared/api.php (3)
tests/e2e/Services/Databases/Legacy/Permissions/DatabasesPermissionsGuestTest.php (1)
getAuthorization(22-31)tests/unit/Auth/AuthTest.php (1)
getAuthorization(18-27)tests/unit/Messaging/MessagingChannelsTest.php (1)
getAuthorization(41-49)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Line/Create.php (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Create.php (1)
action(76-90)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Point/Create.php (1)
action(77-89)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
createAttribute(295-473)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/XList.php (1)
action(71-149)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php (1)
action(80-165)
app/controllers/api/users.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php (1)
action(80-178)src/Appwrite/Utopia/Response.php (1)
Response(158-987)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Delete.php (2)
src/Appwrite/Databases/TransactionState.php (1)
getDocument(41-82)src/Appwrite/Auth/Auth.php (4)
Auth(18-515)isAppUser(441-448)getRoles(456-502)isPrivilegedUser(421-432)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Email/Create.php (7)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Create.php (1)
action(76-90)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Datetime/Create.php (1)
action(77-101)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Enum/Create.php (1)
action(80-109)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/IP/Create.php (1)
action(77-101)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Integer/Create.php (1)
action(81-117)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Relationship/Create.php (1)
action(89-162)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/String/Create.php (1)
action(84-146)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Enum/Create.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Create.php (1)
action(76-90)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Relationship/Create.php (1)
action(89-162)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Create.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Datetime/Create.php (1)
action(77-101)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Enum/Create.php (1)
action(80-109)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
action(138-512)src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Create.php (1)
action(62-88)src/Appwrite/Databases/TransactionState.php (2)
TransactionState(19-744)getDocument(41-82)
app/cli.php (2)
src/Appwrite/Utopia/Response.php (1)
setAuthorization(983-986)src/Appwrite/Utopia/Request.php (1)
setAuthorization(239-242)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Get.php (4)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
action(138-512)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Delete.php (1)
action(89-235)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php (1)
action(80-178)src/Appwrite/Databases/TransactionState.php (2)
TransactionState(19-744)getDocument(41-82)
app/controllers/api/migrations.php (2)
src/Appwrite/Auth/Auth.php (4)
Auth(18-515)isAppUser(441-448)getRoles(456-502)isPrivilegedUser(421-432)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php (1)
action(80-178)
app/controllers/api/storage.php (4)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php (1)
action(80-178)src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Operations/Create.php (1)
action(71-254)src/Appwrite/Utopia/Response.php (2)
Response(158-987)output(745-827)src/Appwrite/Auth/Auth.php (4)
Auth(18-515)isAppUser(441-448)getRoles(456-502)isPrivilegedUser(421-432)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Point/Create.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
createAttribute(295-473)
app/worker.php (3)
src/Appwrite/Utopia/Response.php (1)
setAuthorization(983-986)src/Appwrite/Utopia/Request.php (1)
setAuthorization(239-242)src/Appwrite/Auth/Auth.php (1)
getRoles(456-502)
app/init/resources.php (2)
src/Appwrite/Utopia/Request.php (2)
Request(13-243)setAuthorization(239-242)src/Appwrite/Utopia/Response.php (1)
setAuthorization(983-986)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
action(138-512)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Delete.php (1)
action(89-235)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Get.php (1)
action(76-139)
app/controllers/api/teams.php (3)
src/Appwrite/Utopia/Response.php (1)
Response(158-987)src/Appwrite/Auth/Auth.php (4)
isPrivilegedUser(421-432)Auth(18-515)getRoles(456-502)isAppUser(441-448)src/Appwrite/Utopia/Request.php (1)
Request(13-243)
app/controllers/api/messaging.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php (1)
action(80-178)src/Appwrite/Utopia/Response.php (1)
Response(158-987)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php (4)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
action(138-512)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Delete.php (1)
action(89-235)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Get.php (1)
action(76-139)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php (1)
action(97-383)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Integer/Create.php (4)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Create.php (1)
action(76-90)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Datetime/Create.php (1)
action(77-101)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Enum/Create.php (1)
action(80-109)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Float/Create.php (1)
action(81-115)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Float/Create.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Create.php (1)
action(76-90)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Integer/Create.php (1)
action(81-117)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
src/Appwrite/Utopia/Response.php (1)
Response(158-987)
app/controllers/api/account.php (3)
src/Appwrite/Auth/Auth.php (1)
Auth(18-515)src/Appwrite/Utopia/Response.php (1)
Response(158-987)src/Appwrite/Utopia/Request.php (1)
getUserAgent(209-222)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/IP/Create.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Create.php (1)
action(76-90)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Email/Create.php (1)
action(77-100)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
src/Appwrite/Utopia/Request.php (1)
Request(13-243)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php (6)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
action(138-512)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Delete.php (1)
action(89-235)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Get.php (1)
action(76-139)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php (1)
action(93-359)src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php (1)
action(106-397)src/Appwrite/Databases/TransactionState.php (2)
TransactionState(19-744)getDocument(41-82)
app/controllers/api/vcs.php (3)
src/Appwrite/Utopia/Request.php (1)
Request(13-243)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/XList.php (1)
action(74-129)src/Appwrite/Utopia/Response.php (1)
Response(158-987)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Email/Create.php
77-77: Avoid unused parameters such as '$authorization'. (undefined)
(UnusedFormalParameter)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php
106-106: Avoid unused parameters such as '$user'. (undefined)
(UnusedFormalParameter)
src/Appwrite/Platform/Modules/Compute/Base.php
145-145: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php
97-97: Avoid unused parameters such as '$requestTimestamp'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Benchmark
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V1)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Email/Update.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Enum/Update.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Float/Update.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Integer/Update.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Line/Update.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/Relationship/Update.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Columns/URL/Update.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Indexes/Create.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Bulk/Delete.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Bulk/Upsert.php
Show resolved
Hide resolved
…transaction management. Updated method signatures and resource initialization to include authorization, ensuring consistent handling across the application.
…ppwrite into refactor-auth-single-instance
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 (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Polygon/Update.php (1)
5-20: Fix import ordering to resolve pipeline failure.The
Authorizationimport (line 8) is incorrectly placed amongAppwrite\SDK\*imports. PSR-12 requires imports to be grouped by vendor namespace (Appwrite before Utopia), then ordered alphabetically within each group.Apply this diff to fix the import order:
use Appwrite\Event\Event; use Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Attributes\Action; use Appwrite\SDK\AuthType; -use Utopia\Database\Validator\Authorization; use Appwrite\SDK\ContentType; use Appwrite\SDK\Deprecated; use Appwrite\SDK\Method; use Appwrite\SDK\Response as SDKResponse; use Appwrite\Utopia\Response as UtopiaResponse; use Utopia\Database\Database; +use Utopia\Database\Validator\Authorization; use Utopia\Database\Validator\Key; use Utopia\Database\Validator\Spatial; use Utopia\Database\Validator\UID; use Utopia\Swoole\Response as SwooleResponse; use Utopia\Validator\Boolean; use Utopia\Validator\Nullable;src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (2)
499-499: Critical syntax error: Double opening parentheses.Line 499 contains a syntax error with double opening parentheses.
Apply this diff to fix the syntax error:
- if ($attribute->getAttribute(('type') !== $type)) { + if ($attribute->getAttribute('type') !== $type)) {
503-503: Critical syntax error: Double opening parentheses.Line 503 contains a syntax error with double opening parentheses.
Apply this diff to fix the syntax error:
- if ($attribute->getAttribute('type') === Database::VAR_STRING && $attribute->getAttribute(('filter') !== $filter)) { + if ($attribute->getAttribute('type') === Database::VAR_STRING && $attribute->getAttribute('filter') !== $filter)) {
🧹 Nitpick comments (1)
src/Appwrite/Databases/TransactionState.php (1)
23-24: Remove redundant docblock.The docblock on line 24 is both misplaced (appears after the property declaration) and redundant. With typed property syntax on line 23, the type annotation is already explicit.
Apply this diff to remove the redundant docblock:
private Database $dbForProject; private Authorization $authorization; -/** @var Authorization $authorization */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/init/resources.php(28 hunks)src/Appwrite/Databases/TransactionState.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php(5 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Update.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Datetime/Update.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Email/Update.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Enum/Update.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Float/Update.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/IP/Update.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Integer/Update.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Line/Update.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Point/Update.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Polygon/Update.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Relationship/Update.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/String/Update.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/URL/Update.php(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-10T04:12:13.037Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Polygon/Update.php
📚 Learning: 2025-10-02T10:16:48.075Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10023
File: src/Appwrite/Databases/TransactionState.php:174-219
Timestamp: 2025-10-02T10:16:48.075Z
Learning: In `src/Appwrite/Databases/TransactionState.php`, the `listDocuments` method intentionally applies queries only to the initial database fetch of committed documents. Transaction state changes (creates, updates, deletes) are then overlaid on top of the filtered committed set without re-evaluating the queries against the final merged result. This is the intended behavior for transaction-aware document listing.
Applied to files:
src/Appwrite/Databases/TransactionState.php
🧬 Code graph analysis (15)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/String/Update.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
updateAttribute(475-682)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Integer/Update.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Update.php (1)
action(76-94)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
updateAttribute(475-682)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Update.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Datetime/Update.php (1)
action(77-95)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
updateAttribute(475-682)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Float/Update.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Update.php (1)
action(76-94)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
updateAttribute(475-682)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Relationship/Update.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
updateAttribute(475-682)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Datetime/Update.php (11)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Update.php (1)
action(76-94)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Email/Update.php (1)
action(77-96)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Enum/Update.php (1)
action(79-99)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Float/Update.php (1)
action(79-105)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/IP/Update.php (1)
action(77-96)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Integer/Update.php (1)
action(79-105)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Relationship/Update.php (1)
action(78-111)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/String/Update.php (1)
action(80-110)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/URL/Update.php (1)
action(77-106)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Datetime/Create.php (1)
action(77-101)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
updateAttribute(475-682)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/IP/Update.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Update.php (1)
action(76-94)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
updateAttribute(475-682)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Polygon/Update.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Point/Update.php (1)
action(77-95)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
updateAttribute(475-682)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Enum/Update.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Update.php (1)
action(76-94)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
updateAttribute(475-682)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Line/Update.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Point/Update.php (1)
action(77-95)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
updateAttribute(475-682)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Point/Update.php (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Update.php (1)
action(76-94)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Point/Create.php (1)
action(77-89)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
updateAttribute(475-682)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Email/Update.php (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/String/Update.php (1)
action(80-110)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Email/Create.php (1)
action(77-100)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
updateAttribute(475-682)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/URL/Update.php (2)
src/Appwrite/Event/Event.php (1)
Event(10-654)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (1)
updateAttribute(475-682)
app/init/resources.php (3)
src/Appwrite/Utopia/Request.php (2)
Request(13-243)setAuthorization(239-242)src/Appwrite/Databases/TransactionState.php (2)
getDocument(44-85)TransactionState(20-747)src/Appwrite/Utopia/Response.php (1)
setAuthorization(983-986)
src/Appwrite/Databases/TransactionState.php (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php (1)
__construct(40-78)src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php (1)
__construct(43-81)src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Operations/Create.php (1)
__construct(38-69)
🪛 GitHub Actions: Linter
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Polygon/Update.php
[error] 1-1: PSR 12: 'ordered_imports' check failed. Enforce import order in this file.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Line/Update.php
[error] 1-1: PSR 12: 'ordered_imports' check failed. Enforce import order in this file.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: E2E Shared Mode Service Test (FunctionsSchedule, Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Console, Shared V1)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Shared Mode Service Test (Functions, Shared V1)
- GitHub Check: E2E Shared Mode Service Test (GraphQL, Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Console, Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Databases/Legacy, Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Databases/TablesDB, Shared V1)
- GitHub Check: E2E Shared Mode Service Test (Functions, Shared V2)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V1)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V2)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: scan
🔇 Additional comments (42)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Integer/Update.php (1)
14-14: LGTM! Clean Authorization threading.The refactoring correctly propagates per-request Authorization through the integer attribute update flow:
- Import added for type hint
- DI injection wired up
- Parameter added to action signature
- Authorization passed to parent
updateAttributemethodThe implementation is consistent with the pattern across other attribute update handlers (e.g., Boolean/Update.php) and aligns with the PR's objective to replace static Authorization calls with per-request instances.
Also applies to: 75-75, 79-79, 87-87
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Datetime/Update.php (1)
14-14: LGTM! Authorization refactoring applied consistently.The changes correctly thread the per-request Authorization instance through the Datetime attribute update flow, matching the identical pattern applied across all other attribute Update operations (Boolean, Email, Enum, Float, IP, Integer, Relationship, String, URL). The import, DI injection, method signature, and parameter passing are all properly implemented.
Also applies to: 73-73, 77-77, 85-85
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Enum/Update.php (1)
14-14: LGTM! Authorization refactoring implemented correctly.The changes properly thread the
Authorizationinstance through this endpoint:
- Import added
- DI injection configured
- Method signature updated with the new parameter
- Authorization propagated to
updateAttributeThis implementation is consistent with the pattern established in similar attribute update classes (e.g.,
Boolean/Update.php).Also applies to: 75-75, 79-79, 87-87
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Float/Update.php (1)
14-14: LGTM! Authorization refactoring correctly implemented.The per-request Authorization instance is properly:
- Imported (line 14)
- Injected via DI (line 75)
- Added to the method signature (line 79)
- Passed through to
updateAttribute(line 87)This matches the established pattern from similar attribute update endpoints (e.g., Boolean/Update.php) and correctly threads the authorization context through the request lifecycle.
Also applies to: 75-75, 79-79, 87-87
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Point/Update.php (4)
14-14: LGTM!The Authorization import is correctly added to support the type hint in the action method signature.
73-73: LGTM!The Authorization injection follows the established DI pattern consistently with other attribute handlers.
77-77: LGTM!The action method signature is correctly extended with the Authorization parameter, matching the pattern in Boolean/Update.php and other attribute handlers.
79-90: LGTM!The Authorization context is correctly passed to updateAttribute, consistent with the method signature in Action.php and the broader refactoring pattern.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Polygon/Update.php (1)
73-73: LGTM! Authorization refactoring correctly implemented.The changes properly thread the per-request
Authorizationinstance through the dependency injection chain and into theupdateAttributecall. The implementation is consistent with the pattern used in sibling files (e.g.,Point/Update.php) and correctly aligns with the parentAction::updateAttributemethod signature.Also applies to: 77-77, 85-85
src/Appwrite/Databases/TransactionState.php (2)
25-28: LGTM!The constructor correctly accepts and stores the Authorization instance, enabling per-request authorization context throughout the TransactionState class.
347-356: LGTM!The replacement of static
Authorization::skip()calls with instance method$this->authorization->skip()is correct and aligns with the PR's refactoring objectives. Usingskip()for internal transaction document and log retrieval is appropriate.src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Relationship/Update.php (4)
14-14: LGTM! Authorization import added correctly.The import statement is necessary for the Authorization dependency injection and type hints used in the action method.
74-74: LGTM! Authorization dependency injection configured correctly.The authorization resource is properly registered for injection following Utopia framework patterns.
86-87: LGTM! Authorization parameter added correctly to action method.The Authorization parameter is properly typed and positioned in the method signature, enabling per-request authorization context as intended by the refactoring.
95-95: LGTM! Authorization correctly propagated to updateAttribute.The authorization instance is properly passed to the updateAttribute method, completing the authorization context propagation chain for this endpoint.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Email/Update.php (1)
15-15: LGTM! Authorization refactoring correctly applied.All changes follow the established pattern for threading per-request Authorization throughout the codebase:
- Import added for type hint
- Authorization injected via DI
- Method signature updated to receive Authorization parameter
- Authorization correctly passed to
updateAttributeas expected by the parent Action classThe implementation is consistent with related attribute endpoints (String/Update, Email/Create).
Also applies to: 73-73, 77-77, 85-85
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/String/Update.php (1)
14-14: LGTM! Authorization injection implemented correctly.The changes properly integrate instance-based Authorization throughout the request flow:
- Import statement added correctly
- Dependency injection registered in constructor chain
- Method signature extended to receive Authorization parameter
- Authorization correctly propagated to
updateAttributeusing named parametersThis aligns with the broader architectural shift from static Authorization calls to per-request instances as described in the PR objectives.
Also applies to: 76-76, 90-91, 99-99
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/IP/Update.php (2)
14-14: LGTM: Authorization import added correctly.The import statement properly brings in the Authorization class needed for the per-request authorization refactoring.
73-73: LGTM: Authorization injection and propagation implemented correctly.The changes consistently thread the Authorization instance through the action:
- DI injection at line 73
- Method signature update at line 77
- Parameter propagation to
updateAttribute()at line 85This matches the pattern shown in the Boolean attribute update handler and aligns with the PR's objective of enabling per-request authorization context throughout the codebase.
Also applies to: 77-77, 85-85
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Action.php (6)
295-295: LGTM: Authorization parameter added to method signature.The addition of the
Authorization $authorizationparameter aligns with the PR's goal of threading per-request authorization instances throughout the codebase.
313-313: LGTM: Static call replaced with instance method.The change from
Authorization::skip(...)to$authorization->skip(...)correctly uses the injected authorization instance.
374-376: LGTM: Static call replaced with instance method.The authorization skip call correctly uses the injected instance for bypassing authorization checks when checking for existing spatial data.
475-475: LGTM: Authorization parameter added to method signature.The
updateAttributemethod signature is correctly updated to accept the Authorization instance, maintaining consistency withcreateAttribute.
477-477: LGTM: Static call replaced with instance method.Correctly uses the injected authorization instance to bypass checks when loading the database document.
617-625: LGTM: Static call replaced with instance method.The authorization skip wrapper correctly uses the injected instance for the updateAttribute database operation.
app/init/resources.php (13)
383-385: LGTM: Authorization resource properly registered.The new
authorizationresource creates a fresh Authorization instance per request, which is the core of this refactoring effort.
155-227: LGTM: Authorization injected into platforms resource.The platforms resource correctly receives and uses the Authorization instance for rule lookups (lines 203, 205), replacing static calls.
229-340: LGTM: Authorization injected into user resource.The user resource correctly receives Authorization and uses
$authorization->setDefaultStatus(true)(line 238) instead of the static call, properly initializing per-request authorization context.
342-356: LGTM: Authorization injected into project resource.The project resource correctly uses
$authorization->skip(...)(line 353) to bypass authorization checks when loading project documents.
387-424: LGTM: Authorization injected and set on dbForProject.The dbForProject resource correctly receives Authorization, passes it through to the Database instance via
->setAuthorization($authorization)(line 403), ensuring the database honors the per-request authorization context.
426-440: LGTM: Authorization injected and set on dbForPlatform.Correctly propagates Authorization to the platform database instance.
442-494: LGTM: Authorization threaded through getProjectDB factory.The factory closure correctly captures and propagates Authorization to all dynamically created Database instances (lines 459, 488), ensuring per-tenant databases honor the authorization context.
496-522: LGTM: Authorization threaded through getLogsDB factory.Correctly propagates Authorization to the logs database instance (line 509).
716-801: LGTM: Authorization injected into schema resource.The schema resource correctly uses Authorization to bypass checks when loading attributes for GraphQL schema generation (line 727).
847-888: LGTM: Authorization used in devKey resource.Correctly replaces static skip calls with instance methods (lines 866, 883) for updating development key access metadata.
890-925: LGTM: Authorization used in team resource.Correctly uses the authorization instance for loading team documents (lines 900, 909, 918).
963-1024: LGTM: Authorization used in resourceToken resource.Correctly uses authorization instance for loading and updating resource tokens (lines 980, 1009).
1057-1059: LGTM: Authorization passed to TransactionState.The TransactionState constructor correctly receives the Authorization instance, enabling transaction state methods to bypass authorization checks when reading transaction documents (as shown in the relevant code snippets).
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Boolean/Update.php (1)
14-14: LGTM: Authorization properly integrated.The changes correctly inject Authorization as a dependency, add it to the action method signature, and propagate it to the
updateAttributemethod. This follows the consistent pattern across all attribute update actions.Also applies to: 72-72, 76-76, 84-84
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/URL/Update.php (1)
14-14: LGTM: Authorization properly integrated.The changes correctly inject Authorization as a dependency, add it to the action method signature, and propagate it to the
updateAttributemethod. The refactoring is consistent with the pattern used across other attribute update actions.Also applies to: 73-73, 86-87, 95-95
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Line/Update.php (2)
73-77: LGTM! Authorization injection follows the established pattern.The DI injection and method signature changes correctly implement the per-request Authorization pattern consistent with the PR objectives and match the approach used in other attribute update handlers (e.g., Point/Update.php).
79-90: LGTM! Authorization correctly propagated to updateAttribute().The Authorization instance is properly passed through to the
updateAttribute()method, enabling authorization-aware update logic for line attributes. The implementation matches the pattern established in other attribute handlers.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Line/Update.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Line/Update.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Polygon/Update.php(2 hunks)src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/Action.php(1 hunks)src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/XList.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Polygon/Update.php
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Attributes/Line/Update.php
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-04-17T08:07:54.565Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9661
File: app/controllers/api/migrations.php:332-337
Timestamp: 2025-04-17T08:07:54.565Z
Learning: In the Appwrite codebase, error handling intentionally uses the same error codes for different failure conditions (like STORAGE_BUCKET_NOT_FOUND for both non-existent buckets and authorization failures) as a security measure to prevent information disclosure about what specifically went wrong.
Applied to files:
src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/Action.php
📚 Learning: 2025-05-10T04:12:13.037Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.
Applied to files:
src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/Action.php
🧬 Code graph analysis (2)
src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/Action.php (1)
src/Appwrite/Auth/Auth.php (4)
Auth(18-515)isAppUser(441-448)getRoles(456-502)isPrivilegedUser(421-432)
src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/XList.php (1)
src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/Action.php (1)
getFileAndBucket(14-44)
🔇 Additional comments (1)
src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Buckets/Files/XList.php (1)
16-16: LGTM! Authorization instance properly threaded through the module.The changes correctly integrate the Authorization instance into this endpoint:
- Import added for the Authorization validator
- DI injection configured to provide the authorization instance
- Method signature extended to receive the authorization parameter
- Helper method call updated to pass the authorization instance
The parameter order matches the
getFileAndBucketsignature from the parent Action class, and all changes are consistent with the broader refactoring pattern.Also applies to: 61-61, 65-65, 67-67
| if (!$authorization->isValid(new Input(Database::PERMISSION_READ, $bucket->getRead()))) { | ||
| throw new Exception(Exception::USER_UNAUTHORIZED, $authorization->getDescription()); | ||
| } | ||
|
|
||
| $fileSecurity = $bucket->getAttribute('fileSecurity', false); | ||
| if ($fileSecurity) { | ||
| $file = $dbForProject->getDocument('bucket_' . $bucket->getSequence(), $fileId); | ||
| } else { | ||
| $file = Authorization::skip(fn () => $dbForProject->getDocument('bucket_' . $bucket->getSequence(), $fileId)); | ||
| $file = $authorization->skip(fn () => $dbForProject->getDocument('bucket_' . $bucket->getSequence(), $fileId)); | ||
| } |
There was a problem hiding this comment.
Don't block file-level permissions when file security is enabled
With fileSecurity enabled, bucket-level read permissions are intentionally optional; access can be granted solely by file-level permissions. This new unconditional bucket read check will reject legitimate clients whose buckets intentionally leave $bucket->getRead() empty while relying on per-file ACLs. Please gate the bucket-level check behind a !$fileSecurity condition (or otherwise allow file-level fallbacks), so file-secured buckets continue to work. (appwrite.io)
- if (!$authorization->isValid(new Input(Database::PERMISSION_READ, $bucket->getRead()))) {
+ if (!$fileSecurity && !$authorization->isValid(new Input(Database::PERMISSION_READ, $bucket->getRead()))) {
throw new Exception(Exception::USER_UNAUTHORIZED, $authorization->getDescription());
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
This is intentional, users only need permissions at either collection or document level, if they have collection, we skip row level, because we know they're already permitted.
There was a problem hiding this comment.
@abnegate, thank you for clarifying! I see now that the bucket-level permission check was already present in the original code (using new Authorization(Database::PERMISSION_READ) validator), and this PR is simply refactoring it to use the injected authorization instance. The behavior itself isn't changing—just the mechanism for performing the check. I appreciate you confirming that requiring bucket-level read permissions as the primary gate is the intended design, with file-level permissions acting as an additional layer when fileSecurity is enabled.
🧠 Learnings used
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9661
File: app/controllers/api/migrations.php:332-337
Timestamp: 2025-04-17T08:07:54.565Z
Learning: In the Appwrite codebase, error handling intentionally uses the same error codes for different failure conditions (like STORAGE_BUCKET_NOT_FOUND for both non-existent buckets and authorization failures) as a security measure to prevent information disclosure about what specifically went wrong.
… improving consistency in authorization handling. Update Action class to remove unnecessary authorization skip, and include authorization in Email/Create class constructor for better dependency management.
… class references, ensuring consistent usage of the Authorization class across various modules. Update method signatures to include Authorization as a parameter where necessary, and remove unnecessary injections. Additionally, correct the documentation in CHANGES.md for clarity.
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/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.php (1)
78-86: Add missing->inject('authorization')before->callback().The
actionmethod signature on line 89 includes anAuthorization $authorizationparameter, but the constructor doesn't declare->inject('authorization')in the dependency injection chain. This will cause a runtime error when the DI container attempts to resolve parameters for the callback.Apply this diff to add the missing injection:
->inject('queueForWebhooks') ->inject('plan') +->inject('authorization') ->callback($this->action(...));
♻️ Duplicate comments (1)
src/Appwrite/Utopia/Response.php (1)
812-812: Null-safe operator still missing despite previous fix.This line was previously identified as requiring the null-safe operator (
?->) to prevent fatal errors when$this->authorizationis null. The past review comment indicates this was addressed in commit 9503507, but the current code still uses$this->authorization->getRoles()without the null-safe operator, suggesting a regression.Apply this diff to fix the null-pointer dereference:
- $roles = $this->authorization->getRoles() ?? []; + $roles = $this->authorization?->getRoles() ?? [];
🧹 Nitpick comments (2)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
145-145: Authorization parameter correctly added.The addition of the
Authorization $authorizationparameter aligns well with the PR's refactoring objective.Note: The
$requestparameter appears unused in this method (and also inredeployVcsFunction). If it's not required for API consistency or future use, consider removing it to reduce noise.src/Appwrite/Utopia/Response.php (1)
983-986: Consider returning$thisfor method chaining consistency.The setter doesn't support method chaining, unlike other setters in this class (e.g.,
setModel()at line 648). While not critical, returning$thiswould maintain consistency with the fluent interface pattern used elsewhere in the Response class.Apply this diff to enable method chaining:
- public function setAuthorization(Authorization $authorization): void + public function setAuthorization(Authorization $authorization): self { $this->authorization = $authorization; + return $this; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/controllers/api/users.php(2 hunks)src/Appwrite/Platform/Modules/Compute/Base.php(4 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Indexes/Create.php(2 hunks)src/Appwrite/Utopia/Response.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Indexes/Create.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T10:32:38.142Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10297
File: app/controllers/api/users.php:649-657
Timestamp: 2025-08-11T10:32:38.142Z
Learning: In Appwrite's utopia-php/database (version 0.77.* and later), the database layer internally handles filter separation for count operations. When using `$dbForProject->count()` within a `skipFilters` closure, there's no need to manually separate filter queries using `Query::groupByType($queries)['filters']` as the database handles this automatically.
Applied to files:
app/controllers/api/users.php
🧬 Code graph analysis (5)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
src/Appwrite/Utopia/Request.php (1)
Request(13-243)
src/Appwrite/Utopia/Response.php (2)
src/Appwrite/Auth/Auth.php (1)
getRoles(456-502)src/Appwrite/Utopia/Request.php (1)
setAuthorization(239-242)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.php (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php (1)
action(87-206)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php (1)
action(97-383)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php (1)
action(90-232)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.php (1)
action(89-209)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Delete.php (1)
action(89-235)
app/controllers/api/users.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php (1)
action(80-178)src/Appwrite/Utopia/Response.php (1)
Response(158-987)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Compute/Base.php
145-145: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.php
89-89: Avoid unused parameters such as '$authorization'. (undefined)
(UnusedFormalParameter)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php
87-87: Avoid unused parameters such as '$authorization'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: scan
🔇 Additional comments (3)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
241-261: Authorization skip calls correctly refactored.All three occurrences of
Authorization::skip()have been properly updated to use the injected$authorizationinstance. The refactoring maintains the same observable behavior while aligning with the PR's architectural changes.Also applies to: 267-287, 304-324
app/controllers/api/users.php (1)
2626-2637: LGTM! Authorization refactoring correctly implemented.The changes properly convert static
Authorization::skip()to instance-based authorization:
- Dependency injection added with
->inject('authorization')- Parameter added to action signature:
Authorization $authorization- Instance method call:
$authorization->skip(...)The refactoring is consistent with the pattern shown in other modules.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.php (1)
89-209: Let me verify if authorization is enforced at the routing level and check for any design documentation in the bulk Upsert file:The authorization parameter is intentionally unused—remove the review suggestion.
Bulk operations can only be performed via server-side SDKs to prevent abuse and protect against unexpected costs, ensuring only trusted server environments can perform large-scale data operations. In the codebase, a comment in
Databases/Transactions/Operations/Create.php:157explicitly states: "Bulk operations skip permission validation entirely (API key/admin only, already checked above)."Server APIs are constrained by an API key's scope, ignoring user permissions. The
$authorizationparameter is received but intentionally not used in theaction()method because authorization is enforced at the routing/middleware layer before the method is invoked. This is the documented security pattern for Appwrite bulk operations—not an oversight requiring refactoring.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php
Outdated
Show resolved
Hide resolved
…tion class across multiple controllers and functions. Update method signatures to include Authorization as a parameter, ensuring cleaner and more maintainable code. This change enhances the overall structure and readability of the authorization logic.
…ode and improve maintainability.
…odify Upsert action method to remove unused Authorization parameter and enhance document validation. This improves code clarity and maintainability.
…thorization handling in relationship updates.
…orization handling for string column updates.
…ints for utopia-php/audit and utopia-php/database, ensuring compatibility with the latest releases. Adjusted versioning for utopia-php/migration and appwrite/sdk-generator to their respective latest versions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
composer.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: scan
| if (!$authorization->isValid(new Input(Database::PERMISSION_READ, $bucket->getRead()))) { | ||
| throw new Exception(Exception::USER_UNAUTHORIZED, $authorization->getDescription()); | ||
| } | ||
|
|
||
| $fileSecurity = $bucket->getAttribute('fileSecurity', false); | ||
| if ($fileSecurity) { | ||
| $file = $dbForProject->getDocument('bucket_' . $bucket->getSequence(), $fileId); | ||
| } else { | ||
| $file = Authorization::skip(fn () => $dbForProject->getDocument('bucket_' . $bucket->getSequence(), $fileId)); | ||
| $file = $authorization->skip(fn () => $dbForProject->getDocument('bucket_' . $bucket->getSequence(), $fileId)); | ||
| } |
There was a problem hiding this comment.
This is intentional, users only need permissions at either collection or document level, if they have collection, we skip row level, because we know they're already permitted.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Sites/Http/Deployments/Create.php (1)
344-363: Authorization skip correctly refactored; consider extracting duplicate rule creation logic.The static call is properly replaced with the instance-based call. The use of
skip()is appropriate for system-level operations.Note: This rule creation logic is nearly identical to lines 279-298. Consider extracting a helper method to reduce duplication.
Example refactoring:
private function createDeploymentRule( Database $dbForPlatform, Document $project, Document $site, Document $deployment, string $domain, string $ruleId ): Document { return $dbForPlatform->createDocument('rules', new Document([ '$id' => $ruleId, 'projectId' => $project->getId(), 'projectInternalId' => $project->getSequence(), 'domain' => $domain, 'type' => 'deployment', 'trigger' => 'deployment', 'deploymentId' => $deployment->isEmpty() ? '' : $deployment->getId(), 'deploymentInternalId' => $deployment->isEmpty() ? '' : $deployment->getSequence(), 'deploymentResourceType' => 'site', 'deploymentResourceId' => $site->getId(), 'deploymentResourceInternalId' => $site->getSequence(), 'status' => 'verified', 'certificateId' => '', 'search' => implode(' ', [$ruleId, $domain]), 'owner' => 'Appwrite', 'region' => $project->getAttribute('region') ])); }Then use:
$authorization->skip(fn() => $this->createDeploymentRule(...));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)src/Appwrite/Platform/Modules/Sites/Http/Deployments/Create.php(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V1)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V2)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V1)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: scan
🔇 Additional comments (3)
src/Appwrite/Platform/Modules/Sites/Http/Deployments/Create.php (3)
89-89: LGTM! Authorization dependency properly injected.The authorization instance is correctly added to the dependency injection chain, enabling per-request authorization context.
109-110: LGTM! Method signature updated correctly.The authorization parameter is properly added to receive the injected instance.
279-298: LGTM! Authorization skip correctly refactored.The static call is properly replaced with the instance-based call. Using
skip()is appropriate here since creating platform-level deployment rules is a system operation that should bypass user-level authorization checks.
No description provided.