Feat: CE support for Platform API #10988
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaces project-scoped identifiers with a resource model (resourceType, resourceInternalId, resourceId) across collections, controllers, DB filters, workers, metrics, and platform payloads. Adds organization- and account-scoped API key types and constants, plus new DB subquery filters for organization/account keys. Extends Key constructor and decode to accept team and user Documents and adds getTeamId/getUserId. Updates apiKey resource wiring and authentication to handle account-key headers, multi-entity key lookups (projects, teams, users), batched conditional key updates with cache purges, and makes Key/List models public. Splits scopes config into project/organization/account and updates many scope usages. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Appwrite/Auth/Key.php (1)
14-16: Consider using@return staticpattern instead of class-level template.The class-level
@template T of Keypattern is typically used for classes that are generic throughout (e.g., collection classes). For static factory methods likedecode(), the more idiomatic pattern is:/** * @return static */ public static function decode(...): staticThis clearly indicates the method returns an instance of the called class and is simpler for static analyzers to understand.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Auth/Key.php(2 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). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (1)
src/Appwrite/Auth/Key.php (1)
97-107:staticreturn type withnew Key()instantiation will fail for subclasses.The method declares a
staticreturn type (line 107) but instantiatesnew Key()throughout the implementation (lines 120, 157, 190). This creates a type mismatch when the method is inherited:
- When called as
Key::decode()→ works fine (staticresolves toKey)- When called as
SubClass::decode()(whereSubClass extends Key) → TypeError at runtime because PHP expects aSubClassinstance but receives aKeyinstanceTo support proper extensibility, replace all
new Key(...)instantiations withnew static(...):-return new Key( +return new static( $projectId, $type, $role, // ... remaining parameters );Note: This requires that all subclasses maintain constructor signature compatibility.
There was a problem hiding this comment.
Pull request overview
This PR attempts to make the Key class extendable by changing the decode() factory method's return type from Key to static and adding PHPDoc template annotations. However, the implementation is incomplete as the method still instantiates Key objects directly rather than using static, which prevents proper subclass support.
Key Changes
- Added PHPDoc
@template T of Keyannotation at class level - Changed
decode()return type fromKeytostatic - Removed
@paramannotations from thedecode()method's PHPDoc
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
✨ Benchmark results
⚡ Benchmark Comparison
|
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 (1)
src/Appwrite/Auth/Key.php (1)
137-150: Pre-existing issue: Undefined$payloadwhen JWT decode fails.If
$jwtObj->decode($secret)throws aJWTException, the code sets$expired = truebut$payloadis never assigned. Lines 143-150 then reference$payload, which will cause an undefined variable error.This is pre-existing code, not introduced by this PR, but worth noting since you're modifying this method.
🔎 Proposed fix
+ $payload = []; try { $payload = $jwtObj->decode($secret); } catch (JWTException) { $expired = true; }
♻️ Duplicate comments (3)
src/Appwrite/Auth/Key.php (3)
14-16: Template placement is incorrect for static factory pattern.For a static method like
decode(), the@templateannotation should be defined on the method itself, not at the class level. Class-level templates are bound to instance context and don't apply correctly to static methods.This was already flagged in a previous review. The correct approach for supporting subclass instantiation via a static factory would be:
/** * @template T of static * @return T */ public static function decode(...)
102-107: Incomplete implementation for extendability.The return type removal and
@return Tannotation suggest intent to support subclasses, but this won't work as expected because the method still instantiatesKeydirectly at lines 120, 157, and 190 rather than using late static binding withnew static(...).This was flagged in a previous review. To properly support inheritance, all
new Key(...)calls within this method need to becomenew static(...).
120-126: Usenew static()for inheritance support.If the goal is to make
Keyextendable, this should use late static binding so subclasses receive instances of themselves.🔎 Proposed fix
- $guestKey = new Key( + $guestKey = new static( $project->getId(), $type, User::ROLE_GUESTS, $roles[User::ROLE_GUESTS]['scopes'] ?? [], 'UNKNOWN' );The same change would be needed at lines 157 and 190.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Auth/Key.php(2 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). (20)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: Benchmark
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 (1)
app/config/collections/platform.php (1)
724-731: Critical: Index references renamed attribute.The index
_key_projectreferences attributeprojectInternalId(line 728), but this attribute was renamed toresourceIdat line 625. This mismatch will cause database errors.🔎 Proposed fix
'indexes' => [ [ '$id' => ID::custom('_key_project'), 'type' => Database::INDEX_KEY, - 'attributes' => ['projectInternalId'], + 'attributes' => ['resourceInternalId'], 'lengths' => [Database::LENGTH_KEY], 'orders' => [Database::ORDER_ASC], ],Note: You may also want to rename the index ID from
_key_projectto_key_resourcefor consistency, though this is optional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/config/collections/platform.php(1 hunks)
🔇 Additional comments (2)
app/config/collections/platform.php (2)
625-656: Consider consistency with the devKeys collection.The
keyscollection now uses generic resource terminology (resourceId,resourceInternalId,resourceType), but thedevKeyscollection (lines 746-841) still uses project-specific naming (projectInternalId,projectId).If the goal is to support multiple resource types (project, team, user) as indicated by the comment on line 647, should
devKeysalso be updated for consistency? Or isdevKeysintentionally limited to projects only?
625-656: The schema changes are appropriate for the internal keys collection and require no explicit migration script.The new fields (
resourceId,resourceInternalId,resourceType) are defined withrequired => false, making existing records compatible. Appwrite handles internal platform schema changes through application initialization, not explicit migration files. The field names are already extensively used throughout the codebase (particularly in general.php, Proxy rules, and Functions modules), confirming alignment between schema and code. Naming inconsistencies in platform.php are a documented working pattern.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/mock.php (1)
196-214: Mock endpoint correctly updated to resource-based key model.The key document structure properly uses the new resource-based fields (
resourceType,resourceInternalId,resourceId). The implementation correctly associates the key with the project as a resource.Note: The field ordering (
resourceTypefirst) differs fromapp/controllers/api/projects.php(whereresourceInternalIdcomes first). While this doesn't affect functionality, consistent ordering across the codebase improves readability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/config/collections/platform.php(1 hunks)app/controllers/api/projects.php(5 hunks)app/controllers/mock.php(1 hunks)app/init/database/filters.php(1 hunks)src/Appwrite/Platform/Workers/Deletes.php(1 hunks)src/Appwrite/Platform/Workers/StatsResources.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/config/collections/platform.php
🧰 Additional context used
🧠 Learnings (2)
📚 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:
src/Appwrite/Platform/Workers/StatsResources.phpapp/init/database/filters.php
📚 Learning: 2025-05-13T12:02:08.071Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
Applied to files:
app/controllers/api/projects.php
⏰ 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 (9)
src/Appwrite/Platform/Workers/Deletes.php (1)
567-571: Keys deletion query correctly updated to resource-based model.The query change from
projectInternalIdtoresourceInternalIdwithresourceType = 'projects'aligns with the broader refactoring to make keys resource-agnostic. This enables the Key model to be extended for other resource types in the future (e.g., teams, users) while maintaining backward compatibility for project-scoped keys.src/Appwrite/Platform/Workers/StatsResources.php (1)
113-116: Keys metric count correctly updated to resource-based filtering.The query update properly filters keys by
resourceInternalIdandresourceType = 'projects', aligning with the schema changes. This ensures accurate key counts for project-level metrics while enabling future extensibility for other resource types.app/init/database/filters.php (1)
137-143: subQueryKeys filter correctly updated for resource-based model.The filter properly uses
resourceInternalIdwith$document->getSequence()and adds theresourceType = 'projects'constraint. This ensures the subquery returns only project-scoped keys when populating thekeysattribute on project documents.app/controllers/api/projects.php (6)
1493-1518: Key creation correctly migrated to resource-based model.The key document structure properly uses
resourceInternalId,resourceId, andresourceTypeinstead of the previous project-specific fields. This enables keys to be associated with different resource types in the future while maintaining current project-scoped functionality.
1549-1553: listKeys query correctly updated.The query properly filters by
resourceInternalIdandresourceType = 'projects'to retrieve project-scoped keys.
1590-1594: getKey query correctly updated.The
findOnequery properly scopes to the specific key within the project using the new resource-based fields.
1635-1639: updateKey query correctly updated.The query correctly identifies the key using the resource-based filtering approach.
1687-1691: deleteKey query correctly updated.The query properly identifies the key to delete using the new resource-based fields.
1193-1211: Webhook creation remains project-scoped (intentional).The webhook document still uses
projectInternalIdandprojectId, which is intentional as this PR focuses on making only the Key model extendable for CE Platform API support. Webhooks remain project-specific.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/config/collections/platform.php (1)
725-731: Align index configuration with established patterns in the file.The index has been changed from a single-field index on
projectInternalIdto a composite index on['resourceType', 'resourceInternalId']. Thelengthsandordersarrays are optional, but this index uniquely specifies[Database::LENGTH_KEY]and[Database::ORDER_ASC](one element each) while other two-attribute indexes in the same file use empty arrays.For consistency with similar composite indexes elsewhere in the file (e.g.,
_key_region_accessed_atand_key_project_id_region), either:
- Set both to empty arrays:
'lengths' => []and'orders' => [], or- Add explicit values for both attributes:
'lengths' => [Database::LENGTH_KEY, Database::LENGTH_KEY]and'orders' => [Database::ORDER_ASC, Database::ORDER_ASC]Verify which approach aligns with your indexing strategy and apply consistently.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/config/collections/platform.php(2 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). (4)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Check if utopia-php/database changed
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (2)
app/config/collections/platform.php (2)
636-645: The fieldsresourceIdandresourceInternalIdare not newly added or renamed fromprojectId. The code consistently sets both fields with actual project data when creating keys, so the default value ofnullis appropriate for the schema definition. No breaking change or data migration issue exists here.Likely an incorrect or invalid review comment.
624-634: Reconsider required status for resource-identifying fields.The
resourceIdandresourceInternalIdfields are now optional (required: false). Since keys should always be linked to a resource, these fields should likely remain required to prevent orphaned keys with missing resource references.
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 (1)
src/Appwrite/Utopia/Response/Model/Key.php
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/controllers/shared/api.php (2)
302-331: Comment still mentions blocking API key + session, but the guard is removedThe step‑5 doc block says:
“Verify no user session exists simultaneously”
but the implementation now intentionally allows API keys even when a user session is present (which matches the PR description). To avoid confusion for future readers, consider updating or removing that bullet point.
331-395: Fix typo:Datetime::now()should beDateTime::now()on line 385Line 385 uses
Datetime::now()(lowercase 't'), but the imported class isUtopia\Database\DateTime(uppercase 'T'). This will fail at runtime since the unqualifiedDatetimeclass doesn't exist and PHP's built-inDateTimeclass has no staticnow()method. Line 372 in the same block correctly usesDateTime::now()with uppercase 'T'.if (!in_array($sdk, $sdks)) { $sdks[] = $sdk; $updates->setAttribute('sdks', $sdks); - $updates->setAttribute('accessedAt', Datetime::now()); + $updates->setAttribute('accessedAt', DateTime::now()); }
🧹 Nitpick comments (4)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (1)
2149-2185: Remove or utilize the unused variable.Variable
$sessionId1is assigned on line 2150 but never used, while$sessionId2is properly verified. Consider either:
- Removing
$sessionId1since it's not needed, or- Enhancing the test to verify the first session was actually deleted (stronger assertion)
🔎 Option 1: Remove unused variable
- $this->assertEquals(201, $response['headers']['status-code']); - $sessionId1 = $response['body']['$id']; + $this->assertEquals(201, $response['headers']['status-code']);🔎 Option 2: Verify first session was deleted (more thorough test)
$this->assertEquals(201, $response['headers']['status-code']); $sessionId1 = $response['body']['$id']; /** * create new session */ $response = $this->client->call(Client::METHOD_POST, '/account/sessions/email', array_merge([ 'origin' => 'http://localhost', 'content-type' => 'application/json', 'x-appwrite-project' => $id, ]), [ 'email' => $email, 'password' => $password, ]); $this->assertEquals(201, $response['headers']['status-code']); $sessionCookie = $response['headers']['set-cookie']; $sessionId2 = $response['body']['$id']; /** * List sessions */ $this->assertEventually(function () use ($id, $sessionCookie, $sessionId2, $sessionId1) { $response = $this->client->call(Client::METHOD_GET, '/account/sessions', [ 'origin' => 'http://localhost', 'content-type' => 'application/json', 'x-appwrite-project' => $id, 'Cookie' => $sessionCookie, ]); $this->assertEquals(200, $response['headers']['status-code']); $sessions = $response['body']['sessions']; $this->assertEquals(1, count($sessions)); $this->assertEquals($sessionId2, $sessions[0]['$id']); + $this->assertNotEquals($sessionId1, $sessions[0]['$id']); });Based on learnings, this aligns with the static analysis hint from PHPMD.
app/controllers/api/teams.php (1)
434-450: Using a cloned team document for sync + async deletes looks correctCloning the team before invoking
Deletes::deleteMembershipsand enqueueingDELETE_TYPE_TEAMS/DELETE_TYPE_DOCUMENTavoids any side effects on the original$teamwhile preserving$id/$sequencefor downstream cleanup. This wiring is consistent with the newDeletes::deleteTeamworker logic.If you touch this again, consider naming the variable
$teamClonefor readability.app/config/scopes/organization.php (1)
3-42: Organization scopes config is fine; wording could better reflect “organization”The scope keys and structure look consistent with other scope configs. The descriptions still talk about “project’s platforms/keys/webhooks”, which might be confusing for org-level keys; consider rephrasing to “organization’s projects/platforms/keys/webhooks” when you next touch this.
tests/unit/Auth/KeyTest.php (1)
13-33: Tests aligned with new decode signature; consider covering account/org keysThe updated
Key::decode(project: ..., team: new Document(), user: new Document(), key: $key)usage matches the new API and keeps the dynamic-key assertions valid. The TODO is a good reminder, but before stabilizing this change you likely want tests that:
- Assert
getUserId()/getTeamId()for account and organization keys.- Verify scopes/roles for those new key types.
Up to you whether to address this in this PR or a follow-up, but it would harden the new behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
app/config/collections.phpapp/config/collections/platform.phpapp/config/scopes/account.phpapp/config/scopes/organization.phpapp/config/scopes/project.phpapp/controllers/api/projects.phpapp/controllers/api/teams.phpapp/controllers/general.phpapp/controllers/mock.phpapp/controllers/shared/api.phpapp/init/configs.phpapp/init/constants.phpapp/init/database/filters.phpapp/init/resources.phpdocker-compose.ymlsrc/Appwrite/Auth/Key.phpsrc/Appwrite/Platform/Modules/Functions/Http/Functions/Create.phpsrc/Appwrite/Platform/Modules/Functions/Http/Functions/Update.phpsrc/Appwrite/Platform/Tasks/Screenshot.phpsrc/Appwrite/Platform/Workers/Deletes.phpsrc/Appwrite/Utopia/Response/Model/Key.phptests/e2e/Services/Projects/ProjectsConsoleClientTest.phptests/unit/Auth/KeyTest.php
✅ Files skipped from review due to trivial changes (1)
- docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Appwrite/Utopia/Response/Model/Key.php
- app/init/database/filters.php
- app/config/collections/platform.php
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-05-09T04:06:04.338Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Delete.php:88-95
Timestamp: 2025-05-09T04:06:04.338Z
Learning: It's acceptable to use DATABASE_TYPE_DELETE_COLLECTION event type for both collections and tables because workers process these events based on whether setCollection() or setTable() was called, not on the event type itself.
Applied to files:
app/init/constants.php
📚 Learning: 2025-05-13T12:02:08.071Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9752
File: app/config/collections/platform.php:1423-1423
Timestamp: 2025-05-13T12:02:08.071Z
Learning: In the Appwrite database schema, field names like 'resourceInternalId'/'resourceType' and 'deploymentInternalId'/'deploymentResourceType' are functionally equivalent and can coexist in the codebase without causing errors. The docker build and stack work fine with these naming inconsistencies.
Applied to files:
app/controllers/api/projects.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/init/resources.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/init/resources.php
🧬 Code graph analysis (8)
src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php (1)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)
app/controllers/api/teams.php (5)
src/Appwrite/Platform/Workers/Deletes.php (2)
Deletes(34-1414)deleteMemberships(443-461)app/realtime.php (1)
getProjectDB(80-132)src/Appwrite/Event/Build.php (1)
setType(91-96)src/Appwrite/Event/Func.php (1)
setType(107-112)src/Appwrite/Event/Migration.php (1)
setType(54-59)
src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php (2)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)src/Appwrite/Event/Event.php (1)
getParam(319-322)
tests/unit/Auth/KeyTest.php (1)
src/Appwrite/Auth/Key.php (2)
Key(14-294)decode(112-293)
app/controllers/api/projects.php (1)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)
app/controllers/shared/api.php (2)
src/Appwrite/Auth/Key.php (4)
getType(49-52)getProjectId(34-37)getUserId(39-42)getTeamId(44-47)src/Appwrite/Utopia/Request.php (1)
getHeader(199-203)
app/init/resources.php (2)
src/Appwrite/Utopia/Request.php (2)
getHeader(199-203)Request(14-240)src/Appwrite/Auth/Key.php (2)
Key(14-294)decode(112-293)
src/Appwrite/Platform/Workers/Deletes.php (1)
app/realtime.php (1)
getProjectDB(80-132)
🪛 PHPMD (2.15.0)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php
2150-2150: Avoid unused local variables such as '$sessionId1'. (undefined)
(UnusedLocalVariable)
⏰ 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). (5)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Check if utopia-php/database changed
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: CodeQL
- GitHub Check: scan
🔇 Additional comments (15)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (1)
2082-2088: LGTM! Good refactoring to make the test self-contained.The test is now independent and creates its own project rather than depending on data from
testCreateProject. This improves test isolation and maintainability.src/Appwrite/Platform/Tasks/Screenshot.php (1)
193-193: LGTM! Correct scope configuration source.The change from
'scopes'to'projectScopes'correctly aligns with the PR's introduction of separate scope configurations for project, organization, and account contexts.app/init/constants.php (1)
192-192: LGTM! Constants support new API key types.The new constants (
DELETE_TYPE_TEAMS,API_KEY_ORGANIZATION,API_KEY_ACCOUNT) follow existing naming conventions and properly extend the deletion types and API key types to support organization and account-scoped keys.Also applies to: 247-248
app/controllers/general.php (1)
1405-1405: LGTM! Defensive null check.Adding the
empty($route)guard prevents potential null pointer exceptions when the route is absent, ensuring the error view template is used in such cases. This aligns with the PR's broader route handling improvements.src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php (1)
86-86: LGTM! Consistent scope configuration usage.The change to
projectScopescorrectly aligns with the PR's introduction of separate scope configurations and is consistent with the corresponding change in the Create function endpoint.app/controllers/mock.php (1)
194-194: LGTM! Correct scope and resource model updates.Both changes correctly implement the PR's objectives:
- Line 194: Uses
projectScopesconfiguration, consistent with the new scope structure- Lines 203-205: Migrates to resource-based identifiers (
resourceType,resourceInternalId,resourceId), properly settingresourceTypeto'projects'Also applies to: 203-205
src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php (1)
90-90: LGTM! Consistent scope configuration usage.The change to
projectScopescorrectly aligns with the PR's introduction of separate scope configurations and maintains consistency with the Update function endpoint (line 86 in Update.php).app/config/collections.php (1)
29-30: The change fromarray_mergetoarray_merge_recursivehas no functional difference here because the merged arrays ($projectsand$common, or$platformand$common) have no overlapping collection keys. Since there are no shared keys to recursively merge, both functions produce identical results.Likely an incorrect or invalid review comment.
app/init/configs.php (1)
25-27: Scoped configs split by project/organization/account looks consistentLoading
projectScopes,organizationScopes, andaccountScopesfrom separate files is aligned with how roles and other configs are structured; no issues from this change alone.src/Appwrite/Platform/Workers/Deletes.php (1)
123-125: Team/user key cleanup in Deletes worker matches the new resource-based keys model
- Routing
DELETE_TYPE_TEAMStodeleteTeam()and deleting keys viaresourceInternalId+resourceTypefor projects/teams/users is consistent with the updated schema.- Gating key deletion on
$project->getId() === 'console'ensures you only touch the console/platform keys store, while still purging team/user cache for non-console projects.This looks correct given the new organization/account key design.
It’s still worth double-checking that:
- The
keyscollection actually stores console team/user keys withresourceType='teams'/'users'andresourceInternalId= team/user$sequence.- No legacy
projectInternalIdreferences remain that would leave orphaned keys after deletion.Also applies to: 569-575, 640-656, 677-684
src/Appwrite/Auth/Key.php (1)
112-139: The code is correct as written. Thesecretfield in the database stores the full key string including the type prefix (e.g.,account_ABC123, not justABC123), so callingfind(..., $key, ...)in all branches (STANDARD, ACCOUNT, ORGANIZATION) is the right approach. This is confirmed by the STANDARD key creation pattern (API_KEY_STANDARD . '_' . ...) and consistent usage inapp/controllers/shared/api.php, where the full header value is used for lookups across all key types. No change needed.Likely an incorrect or invalid review comment.
app/controllers/api/projects.php (4)
1549-1553: LGTM!The query correctly filters by both
resourceInternalIdandresourceTypeto ensure only project-scoped keys are returned. This is essential for the new multi-resource key model.
1590-1701: LGTM!Key retrieval, update, and delete operations consistently apply the
resourceInternalIdandresourceTypefilters alongside the key ID. This enforces proper access control, preventing keys from being accessed or modified across different resource types.
1724-1743: LGTM!JWT scope validation correctly updated to use
projectScopesconfig, maintaining consistency with the key endpoints.
1481-1517: Key creation properly migrated to resource-based identifiers.The migration from project-specific fields to resource-based identifiers (
resourceInternalId,resourceId,resourceType) is correctly implemented, enabling keys to be associated with different resource types. TheprojectScopesconfig key is properly defined and loaded fromapp/config/scopes/project.php, confirming that all scope references in the code are valid.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/controllers/shared/api.php (1)
333-352: Initialize$dbKeyto prevent undefined variable error.If all three conditions (
projectId,userId,teamIdare empty) fail to match,$dbKeyremains uninitialized, causing a PHP error on line 354. Initialize$dbKey = null;before the conditional chain.🔎 Proposed fix
if (\in_array($apiKey->getType(), [API_KEY_STANDARD, API_KEY_ORGANIZATION, API_KEY_ACCOUNT])) { + $dbKey = null; if (!empty($apiKey->getProjectId())) {
🧹 Nitpick comments (2)
tests/unit/Auth/KeyTest.php (1)
90-105: Remove unnecessary sleep operation.The key generated on line 91 with
timestamp: time() - 60andmaxAge: 1is already expired at the time of decoding (issued 60 seconds ago with 1 second validity). Thesleep(2)on line 92 is unnecessary.🔎 Proposed fix
// Decode expired dynamic key $expiredKey = static::generateKey($projectId, $usage, $scopes, maxAge: 1, timestamp: time() - 60); - \sleep(2); $decoded = Key::decode(src/Appwrite/Platform/Workers/StatsResources.php (1)
115-115: Consider using a constant for the 'projects' resource type.The hardcoded string
'projects'should ideally be replaced with a constant (e.g.,RESOURCE_TYPE_PROJECTS) for better maintainability and consistency across the codebase.🔎 Proposed refactor using a constant
If a constant like
RESOURCE_TYPE_PROJECTSexists or can be defined, replace the hardcoded string:$keys = $dbForPlatform->count('keys', [ Query::equal('resourceInternalId', [$project->getSequence()]), - Query::equal('resourceType', ['projects']), + Query::equal('resourceType', [RESOURCE_TYPE_PROJECTS]), ]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/config/scopes/account.phpapp/controllers/api/teams.phpapp/controllers/shared/api.phpdocker-compose.ymlsrc/Appwrite/Platform/Workers/StatsResources.phptests/unit/Auth/KeyTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- docker-compose.yml
- app/controllers/api/teams.php
- app/config/scopes/account.php
🧰 Additional context used
🧠 Learnings (2)
📚 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:
src/Appwrite/Platform/Workers/StatsResources.php
📚 Learning: 2025-08-29T15:29:23.250Z
Learnt from: ChiragAgg5k
Repo: appwrite/appwrite PR: 10408
File: app/controllers/general.php:0-0
Timestamp: 2025-08-29T15:29:23.250Z
Learning: In the Appwrite codebase, when SDK methods are marked as deprecated (isDeprecated() returns true), the getDeprecated() method is guaranteed to return valid deprecation metadata objects, not null. The deprecation checking logic first verifies isDeprecated() before accessing getDeprecated(), providing implicit null-safety.
Applied to files:
tests/unit/Auth/KeyTest.php
🧬 Code graph analysis (2)
tests/unit/Auth/KeyTest.php (1)
src/Appwrite/Auth/Key.php (16)
Key(14-291)decode(112-290)getProjectId(34-37)getTeamId(44-47)getUserId(39-42)getType(49-52)getRole(54-57)getScopes(59-62)getName(64-67)getDisabledMetrics(74-77)getHostnameOverride(80-83)isBannerDisabled(86-89)isProjectCheckDisabled(101-104)isPreviewAuthDisabled(91-94)isDeploymentStatusIgnored(96-99)isExpired(69-72)
app/controllers/shared/api.php (3)
src/Appwrite/Auth/Key.php (4)
getType(49-52)getProjectId(34-37)getUserId(39-42)getTeamId(44-47)src/Appwrite/Extend/Exception.php (2)
getType(427-430)Exception(7-472)src/Appwrite/Utopia/Request.php (1)
getHeader(199-203)
⏰ 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 (2)
app/controllers/shared/api.php (1)
358-393: LGTM: Efficient batched update pattern.The deferred, batched update approach correctly accumulates changes and performs the database write only when necessary, followed by cache invalidation. This reduces unnecessary database operations compared to immediate updates.
tests/unit/Auth/KeyTest.php (1)
17-342: LGTM: Comprehensive test coverage.The expanded test coverage thoroughly validates all key types (dynamic, standard, account, organization) across multiple scenarios including valid keys, invalid keys, expired keys, extra fields, and deprecated formats. This ensures robust validation of the Key decoding logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php (1)
1768-1774: Improved isolation for auth session-limit test; consider asserting final reset stateUsing
setupProjectand makingtestUpdateProjectAuthSessionsLimit()self-contained is a solid improvement and keeps this test independent from the@dependschain. After the final reset tolimit => 10, you currently only assert the 200 status; adding an assertion onauthSessionsLimitwould slightly strengthen coverage and mirror the earlier success case.Proposed assertion enhancement
$response = $this->client->call(Client::METHOD_PATCH, '/projects/' . $id . '/auth/max-sessions', array_merge([ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], ], $this->getHeaders()), [ 'limit' => 10, ]); - $this->assertEquals(200, $response['headers']['status-code']); + $this->assertEquals(200, $response['headers']['status-code']); + $this->assertEquals(10, $response['body']['authSessionsLimit']);Also applies to: 1883-1883
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/Services/Projects/ProjectsConsoleClientTest.php
⏰ 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 (Users)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Benchmark
- GitHub Check: scan
| // Verify no user session exists simultaneously | ||
| if (!$user->isEmpty()) { | ||
| throw new Exception(Exception::USER_API_KEY_AND_SESSION_SET); | ||
| } |
There was a problem hiding this comment.
Is this intentional? What happens now with session + key?
| 'type' => Database::INDEX_KEY, | ||
| 'attributes' => ['projectInternalId'], | ||
| 'attributes' => ['resourceType', 'resourceInternalId'], | ||
| 'lengths' => [Database::LENGTH_KEY], |
There was a problem hiding this comment.
Do we need another Database::LENGTH_KEY here?
| throw new Exception(Exception::GENERAL_SERVER_ERROR, 'Failed to remove team from DB'); | ||
| } | ||
|
|
||
| $clone = clone $team; |
There was a problem hiding this comment.
Why do we clone the team? Is it avoidable?
|
|
||
| $name = $key->getAttribute('name', 'UNKNOWN'); | ||
|
|
||
| $role = User::ROLE_USERS; |
There was a problem hiding this comment.
Is this correct? Aren't keys for ROLE_APPS?
loks0n
left a comment
loks0n
left a comment
There was a problem hiding this comment.
What stops an organization key from managing keys in projects it shouldn't touch?
What does this PR do?
Makes Key extendable
Test Plan
Current tests
Related PRs and Issues
Checklist