Add imperso 8000 nation feature for user management#11533
Open
Conversation
- Introduced a new API endpoint to update user impersonator capability. - Enhanced user model to include impersonator attributes. - Updated database schema to support impersonation. - Implemented impersonation logic in the request handling to allow users with impersonator capability to act as other users. - Added relevant API documentation for impersonation headers. This feature allows users with the appropriate permissions to impersonate other users, enhancing flexibility in user management.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds user impersonation support: a new boolean Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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! |
Greptile SummaryThis PR introduces a user impersonation feature that allows users granted the
Confidence Score: 1/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant C as Client (Impersonator)
participant R as resources.php (user resource)
participant A as shared/api.php (init middleware)
participant E as Endpoint (e.g. GET /v1/users)
participant DB as Database
C->>R: Request + x-appwrite-impersonate-user-id: targetId
R->>DB: getDocument('users', impersonatorUserId) [already resolved as $user]
Note over R: $user.impersonator == true ✓
R->>DB: skip(getDocument('users', targetId))
DB-->>R: $targetUser
R->>R: $user = clone $targetUser<br/>$user->impersonatorUserId = impersonatorId
R-->>A: $user = targetUser (+ impersonatorUserId attr)
A->>A: Step 4: $scopes = roles['users']['scopes']
A->>A: ⚠️ impersonatorUserId set → append 'users.read' to $scopes
A->>A: Step 9: scope check — allowed ∩ $scopes
Note over A: 'users.read' now in $scopes regardless<br/>of target user's actual permissions
A->>E: authorized (with escalated scopes)
E->>DB: action runs as targetUser
Note over DB: accessedAt updated for targetUser<br/>Audit logs record targetUser — impersonatorId lost
Last reviewed commit: 8304a8e |
app/init/resources.php
Outdated
Comment on lines
+483
to
+486
| $impersonatorUserId = $user->getId(); | ||
| $user = clone $targetUser; | ||
| $user->setAttribute('impersonatorUserId', $impersonatorUserId); | ||
| } |
There was a problem hiding this comment.
Audit trail loses impersonator identity
After line 485, $user is the target user and impersonatorUserId lives only as a transient in-memory attribute (not a DB field). All downstream audit recording ($queueForAudits->setUser($user), $dbForProject->setMetadata('user', $user->getId()), and the accessedAt timestamp update in api.php) will attribute every action to the impersonated user's ID, with no persistent link to the actual impersonator.
This means:
- The impersonator's own
accessedAtis never bumped for impersonated requests. - The impersonated user's
accessedAtis updated as if they made the request, making access logs misleading. - Audit events record
userId = targetUser->getId()with no field recordingimpersonatorUserId.
The impersonatorUserId attribute should be passed to the audit queue explicitly so every action under impersonation can be traced back to its true actor.
🔄 PHP-Retry SummaryFlaky tests detected across commits: Commit
|
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testDatabaseStatsCollectionsAPI |
1 | 10.24s | Logs |
LegacyConsoleClientTest::testUniqueIndexDuplicate |
1 | 298ms | Logs |
LegacyConsoleClientTest::testUpdatePermissionsWithEmptyPayload |
1 | 240.30s | Logs |
TablesDBConsoleClientTest::testCreateIndexes |
1 | 241.48s | Logs |
TablesDBCustomServerTest::testAttributeResponseModels |
1 | 240.74s | Logs |
TablesDBCustomServerTest::testUniqueIndexDuplicate |
1 | 240.21s | Logs |
TablesDBTransactionsConsoleClientTest::testDeleteDocumentDuringTransaction |
1 | 241.04s | Logs |
Commit 29d9c13 - 9 flaky tests
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testDatabaseStatsCollectionsAPI |
1 | 10.28s | Logs |
LegacyCustomClientTest::testEnforceCollectionPermissions |
1 | 240.27s | Logs |
TablesDBConsoleClientTest::testCreateIndexes |
1 | 242.46s | Logs |
TablesDBConsoleClientTest::testListDocumentsCacheBustedByAttributeChange |
1 | 240.06s | Logs |
TablesDBCustomClientTest::testListDocumentsCacheBustedByAttributeChange |
1 | 240.43s | Logs |
TablesDBCustomClientTest::testUniqueIndexDuplicate |
1 | 279ms | Logs |
TablesDBCustomServerTest::testTimeout |
1 | 240.34s | Logs |
LegacyTransactionsCustomClientTest::testOperationsOnNonExistentDocuments |
1 | 240.70s | Logs |
TablesDBTransactionsCustomClientTest::testBulkDeleteMatchingCreatedDocuments |
1 | 240.37s | Logs |
Commit d8df5f1 - 2 flaky tests
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testDatabaseStatsCollectionsAPI |
1 | 10.22s | Logs |
FunctionsCustomServerTest::testCreateFunctionAndDeploymentFromTemplate |
1 | 50.53s | Logs |
Commit b85cf2f - 1 flaky test
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testDatabaseStatsCollectionsAPI |
1 | 10.33s | Logs |
Commit f6d38fe - 2 flaky tests
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testDatabaseStatsCollectionsAPI |
1 | 10.27s | Logs |
FunctionsScheduleTest::testCreateScheduledAtExecution |
1 | 126.42s | Logs |
Note: Flaky test results are tracked for the last 5 commits
✨ Benchmark results
⚡ Benchmark Comparison
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/shared/api.php`:
- Around line 341-345: The current logic appends a global 'users.read' scope
when $user->getAttribute('impersonatorUserId') is present, which escalates
impersonated sessions to project-wide user access; instead, stop adding a global
scope and grant a scoped permission limited to the impersonation target (e.g.,
create and add a per-user scope like "users.read:{targetUserId}" using
$user->getAttribute('id') or otherwise record the impersonation target in the
session/scopes), or remove adding any users.read scope and ensure downstream
authorization checks in users-related handlers validate that the active session
is only allowed to access the impersonated user's records via the impersonation
attribute.
In `@app/init/resources.php`:
- Around line 482-490: The code replaces the authenticated caller in $user with
the impersonated $targetUser; instead, preserve the original actor and introduce
an explicit effective user for impersonation. Do not reassign $user—store the
original caller id in $impersonatorUserId = $user->getId(); create an
$effectiveUser (or $actingUser) that is a clone of $targetUser and
setAttribute('impersonatorUserId', $impersonatorUserId) on that clone, then use
$effectiveUser when performing actions as the target while keeping $user for
session re-verification; also set DB metadata to record both actor and effective
IDs (e.g. keep $dbForProject->setMetadata('user', $user->getId()) and add
$dbForProject->setMetadata('effective_user', $effectiveUser->getId()), same for
$dbForPlatform) so auditing preserves the real actor.
- Around line 472-480: The code chooses $dbForPlatform when APP_MODE_ADMIN ===
$mode which causes admin (Console) requests against non-console projects to look
up impersonation targets in the wrong DB; change the $userDb assignment (the
ternary that sets $userDb) so it selects $dbForPlatform only when
$project->getId() === 'console' and otherwise uses $dbForProject (i.e., $userDb
= ($project->getId() === 'console') ? $dbForPlatform : $dbForProject) so the
impersonation lookups in the block that calls
$userDb->getAuthorization()->skip(...), $userDb->getDocument('users',
$impersonateUserId) and $userDb->findOne('users', ...) query the project users
collection for non-console projects even in admin mode.
In `@src/Appwrite/Platform/Tasks/Specs.php`:
- Around line 163-180: Update the descriptions for the ImpersonateUserId,
ImpersonateUserEmail, and ImpersonateUserPhone header specs (the array entries
keyed as 'ImpersonateUserId', 'ImpersonateUserEmail', 'ImpersonateUserPhone') to
explicitly state these headers only work on requests already authenticated as a
user who has the impersonator capability and cannot be used with X-Appwrite-Key
alone; apply the same wording change to the other identical header spec blocks
elsewhere in the file (the repeated Impersonate* entries) so all occurrences
clearly indicate they require prior user authentication with impersonator
capability.
In `@src/Appwrite/Utopia/Response/Model/Account.php`:
- Around line 119-124: The description for the 'impersonatorUserId' field in the
Account model is ambiguous; update the rule for 'impersonatorUserId' to
explicitly state whether it refers to the original actor (the user who performed
impersonation) or the impersonation target — e.g., if it's the original actor,
change the description to: "ID of the original actor who initiated impersonation
(the impersonator); present only when the session was created using
impersonation headers" — otherwise, rename the field to 'impersonatedUserId' and
update its description accordingly so it isn't redundant with $id; adjust the
'impersonatorUserId' rule in Account.php to reflect this clarified contract.
In `@src/Appwrite/Utopia/Response/Model/User.php`:
- Around line 149-154: The description for the schema rule added via
addRule('impersonatorUserId', ...) is misleading: change it to clearly state
this field holds the ID of the user performing the impersonation (the original
actor), not the impersonation target; update the 'description' value for
impersonatorUserId in the User model to something like "ID of the user
performing the impersonation (original actor); present only when the request
used impersonation headers" so it matches the runtime behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 09aa7318-817c-488c-949d-38140020e8cb
📒 Files selected for processing (8)
app/config/collections/common.phpapp/controllers/api/users.phpapp/controllers/shared/api.phpapp/init/resources.phpsrc/Appwrite/Platform/Tasks/Specs.phpsrc/Appwrite/Utopia/Database/Validator/Queries/Users.phpsrc/Appwrite/Utopia/Response/Model/Account.phpsrc/Appwrite/Utopia/Response/Model/User.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/Services/Users/UsersBase.php`:
- Around line 2635-2669: The test testListUsersFilterByImpersonator currently
only checks response shape and a single contains, so update it to actually
assert the impersonator filter: create a control user (e.g.,
$userWithoutImpersonator) with impersonator=false, call the PATCH endpoint only
for $userWithImpersonator (as already done), then for the GET with
Query::equal('impersonator',[true]) assert that every returned user in
$response['body']['users'] has 'impersonator'===true and that
$userWithImpersonator['body']['$id'] is present; for the GET with
Query::equal('impersonator',[false]) assert every returned user has
'impersonator'===false and that $userWithoutImpersonator['body']['$id'] is
present (or assert $userWithImpersonator['body']['$id'] is absent) to prove
inclusion/exclusion.
- Around line 2589-2630: The test testImpersonationUsersReadScope currently only
checks the happy path with the x-appwrite-impersonate-user-id header; add an
extra GET /users request using the same session (use $session['body']['secret']
as before) but without the 'x-appwrite-impersonate-user-id' header and assert it
returns 401 to ensure users.read is only available during active impersonation;
update the test around the existing $listHeaders/$users block in
UsersBase::testImpersonationUsersReadScope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a01810ee-8f97-4b1f-b32f-63d074321c1f
📒 Files selected for processing (1)
tests/e2e/Services/Users/UsersBase.php
Comment on lines
+2635
to
+2669
| public function testListUsersFilterByImpersonator(): void | ||
| { | ||
| $projectId = $this->getProject()['$id']; | ||
| $headers = array_merge([ | ||
| 'content-type' => 'application/json', | ||
| 'x-appwrite-project' => $projectId, | ||
| ], $this->getHeaders()); | ||
|
|
||
| $userWithImpersonator = $this->client->call(Client::METHOD_POST, '/users', $headers, [ | ||
| 'userId' => ID::unique(), | ||
| 'email' => 'filter-impersonator-true@appwrite.io', | ||
| 'password' => 'password', | ||
| 'name' => 'Has Impersonator', | ||
| ]); | ||
| $this->assertEquals(201, $userWithImpersonator['headers']['status-code']); | ||
| $this->client->call(Client::METHOD_PATCH, '/users/' . $userWithImpersonator['body']['$id'] . '/impersonator', $headers, ['impersonator' => true]); | ||
|
|
||
| $response = $this->client->call(Client::METHOD_GET, '/users', $headers, [ | ||
| 'queries' => [ | ||
| Query::equal('impersonator', [true])->toString(), | ||
| ], | ||
| ]); | ||
| $this->assertEquals(200, $response['headers']['status-code']); | ||
| $this->assertIsArray($response['body']['users']); | ||
| $userIds = array_column($response['body']['users'], '$id'); | ||
| $this->assertContains($userWithImpersonator['body']['$id'], $userIds); | ||
|
|
||
| $response = $this->client->call(Client::METHOD_GET, '/users', $headers, [ | ||
| 'queries' => [ | ||
| Query::equal('impersonator', [false])->toString(), | ||
| ], | ||
| ]); | ||
| $this->assertEquals(200, $response['headers']['status-code']); | ||
| $this->assertIsArray($response['body']['users']); | ||
| } |
Contributor
There was a problem hiding this comment.
Actually assert the impersonator filter, not just the response shape.
The true branch passes on an unfiltered list because it only checks assertContains, and the false branch never checks membership at all. Add a known false control user and assert inclusion/exclusion, or verify every returned row has the expected impersonator value.
Suggested assertions
$userWithImpersonator = $this->client->call(Client::METHOD_POST, '/users', $headers, [
'userId' => ID::unique(),
'email' => 'filter-impersonator-true@appwrite.io',
'password' => 'password',
'name' => 'Has Impersonator',
]);
$this->assertEquals(201, $userWithImpersonator['headers']['status-code']);
+
+ $controlUser = $this->client->call(Client::METHOD_POST, '/users', $headers, [
+ 'userId' => ID::unique(),
+ 'email' => 'filter-impersonator-false@appwrite.io',
+ 'password' => 'password',
+ 'name' => 'No Impersonator',
+ ]);
+ $this->assertEquals(201, $controlUser['headers']['status-code']);
+
$this->client->call(Client::METHOD_PATCH, '/users/' . $userWithImpersonator['body']['$id'] . '/impersonator', $headers, ['impersonator' => true]);
$response = $this->client->call(Client::METHOD_GET, '/users', $headers, [
'queries' => [
Query::equal('impersonator', [true])->toString(),
],
]);
$this->assertEquals(200, $response['headers']['status-code']);
$this->assertIsArray($response['body']['users']);
$userIds = array_column($response['body']['users'], '$id');
$this->assertContains($userWithImpersonator['body']['$id'], $userIds);
+ $this->assertNotContains($controlUser['body']['$id'], $userIds);
+ foreach ($response['body']['users'] as $user) {
+ $this->assertTrue($user['impersonator']);
+ }
$response = $this->client->call(Client::METHOD_GET, '/users', $headers, [
'queries' => [
Query::equal('impersonator', [false])->toString(),
],
]);
$this->assertEquals(200, $response['headers']['status-code']);
$this->assertIsArray($response['body']['users']);
+ $userIds = array_column($response['body']['users'], '$id');
+ $this->assertContains($controlUser['body']['$id'], $userIds);
+ $this->assertNotContains($userWithImpersonator['body']['$id'], $userIds);
+ foreach ($response['body']['users'] as $user) {
+ $this->assertFalse($user['impersonator']);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public function testListUsersFilterByImpersonator(): void | |
| { | |
| $projectId = $this->getProject()['$id']; | |
| $h 4D1C eaders = array_merge([ | |
| 'content-type' => 'application/json', | |
| 'x-appwrite-project' => $projectId, | |
| ], $this->getHeaders()); | |
| $userWithImpersonator = $this->client->call(Client::METHOD_POST, '/users', $headers, [ | |
| 'userId' => ID::unique(), | |
| 'email' => 'filter-impersonator-true@appwrite.io', | |
| 'password' => 'password', | |
| 'name' => 'Has Impersonator', | |
| ]); | |
| $this->assertEquals(201, $userWithImpersonator['headers']['status-code']); | |
| $this->client->call(Client::METHOD_PATCH, '/users/' . $userWithImpersonator['body']['$id'] . '/impersonator', $headers, ['impersonator' => true]); | |
| $response = $this->client->call(Client::METHOD_GET, '/users', $headers, [ | |
| 'queries' => [ | |
| Query::equal('impersonator', [true])->toString(), | |
| ], | |
| ]); | |
| $this->assertEquals(200, $response['headers']['status-code']); | |
| $this->assertIsArray($response['body']['users']); | |
| $userIds = array_column($response['body']['users'], '$id'); | |
| $this->assertContains($userWithImpersonator['body']['$id'], $userIds); | |
| $response = $this->client->call(Client::METHOD_GET, '/users', $headers, [ | |
| 'queries' => [ | |
| Query::equal('impersonator', [false])->toString(), | |
| ], | |
| ]); | |
| $this->assertEquals(200, $response['headers']['status-code']); | |
| $this->assertIsArray($response['body']['users']); | |
| } | |
| public function testListUsersFilterByImpersonator(): void | |
| { | |
| $projectId = $this->getProject()['$id']; | |
| $headers = array_merge([ | |
| 'content-type' => 'application/json', | |
| 'x-appwrite-project' => $projectId, | |
| ], $this->getHeaders()); | |
| $userWithImpersonator = $this->client->call(Client::METHOD_POST, '/users', $headers, [ | |
| BEA4 | 'userId' => ID::unique(), |
| 'email' => 'filter-impersonator-true@appwrite.io', | |
| 'password' => 'password', | |
| 'name' => 'Has Impersonator', | |
| ]); | |
| $this->assertEquals(201, $userWithImpersonator['headers']['status-code']); | |
| $controlUser = $this->client->call(Client::METHOD_POST, '/users', $headers, [ | |
| 'userId' => ID::unique(), | |
| 'email' => 'filter-impersonator-false@appwrite.io', | |
| 'password' => 'password', | |
| 'name' => 'No Impersonator', | |
| ]); | |
| $this->assertEquals(201, $controlUser['headers']['status-code']); | |
| $this->client->call(Client::METHOD_PATCH, '/users/' . $userWithImpersonator['body']['$id'] . '/impersonator', $headers, ['impersonator' => true]); | |
| $response = $this->client->call(Client::METHOD_GET, '/users', $headers, [ | |
| 'queries' => [ | |
| Query::equal('impersonator', [true])->toString(), | |
| ], | |
| ]); | |
| $this->assertEquals(200, $response['headers']['status-code']); | |
| $this->assertIsArray($response['body']['users']); | |
| $userIds = array_column($response['body']['users'], '$id'); | |
| $this->assertContains($userWithImpersonator['body']['$id'], $userIds); | |
| $this->assertNotContains($controlUser['body']['$id'], $userIds); | |
| foreach ($response['body']['users'] as $user) { | |
| $this->assertTrue($user['impersonator']); | |
| } | |
| $response = $this->client->call(Client::METHOD_GET, '/users', $headers, [ | |
| 'queries' => [ | |
| Query::equal('impersonator', [false])->toString(), | |
| ], | |
| ]); | |
| $this->assertEquals(200, $response['headers']['status-code']); | |
| $this->assertIsArray($response['body']['users']); | |
| $userIds = array_column($response['body']['users'], '$id'); | |
| $this->assertContains($controlUser['body']['$id'], $userIds); | |
| $this->assertNotContains($userWithImpersonator['body']['$id'], $userIds); | |
| foreach ($response['body']['users'] as $user) { | |
| $this->assertFalse($user['impersonator']); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/Services/Users/UsersBase.php` around lines 2635 - 2669, The test
testListUsersFilterByImpersonator currently only checks response shape and a
single contains, so update it to actually assert the impersonator filter: create
a control user (e.g., $userWithoutImpersonator) with impersonator=false, call
the PATCH endpoint only for $userWithImpersonator (as already done), then for
the GET with Query::equal('impersonator',[true]) assert that every returned user
in $response['body']['users'] has 'impersonator'===true and that
$userWithImpersonator['body']['$id'] is present; for the GET with
Query::equal('impersonator',[false]) assert every returned user has
'impersonator'===false and that $userWithoutImpersonator['body']['$id'] is
present (or assert $userWithImpersonator['body']['$id'] is absent) to prove
inclusion/exclusion.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (6)
demo/src/api.ts (2)
83-93: Consider usingelse ifor aswitchstatement for mutually exclusive impersonation types.Since
ImpersonationHeaderis a discriminated union where only one type can be active, the current structure evaluates all three conditions even after a match. While functionally correct, anelse ifchain orswitchstatement would be more semantically accurate and slightly more efficient.♻️ Optional refactor using switch
- if (impersonation.type === 'id') { - headers['x-appwrite-impersonate-user-id'] = impersonation.value; - } - - if (impersonation.type === 'email') { - headers['x-appwrite-impersonate-user-email'] = impersonation.value; - } - - if (impersonation.type === 'phone') { - headers['x-appwrite-impersonate-user-phone'] = impersonation.value; - } + switch (impersonation.type) { + case 'id': + headers['x-appwrite-impersonate-user-id'] = impersonation.value; + break; + case 'email': + headers['x-appwrite-impersonate-user-email'] = impersonation.value; + break; + case 'phone': + headers['x-appwrite-impersonate-user-phone'] = impersonation.value; + break; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/src/api.ts` around lines 83 - 93, The three separate if checks on impersonation.type should be made mutually exclusive to reflect the ImpersonationHeader discriminated union; update the logic that sets headers (the block referencing impersonation.type and keys 'x-appwrite-impersonate-user-id', 'x-appwrite-impersonate-user-email', 'x-appwrite-impersonate-user-phone') to use an else-if chain or a switch on impersonation.type so only one header is set per call, leaving the rest unchanged.
102-104: Type assertion on 204 response may cause runtime issues.Returning
{} as Tfor 204 responses can be unsafe ifTexpects specific properties. CurrentlydeleteCurrentSessionusesRecord<string, never>which is compatible, but if other callers expect different types, this could silently return an unexpected shape.♻️ Consider returning undefined or void for 204 responses
One approach is to change the return type signature to indicate that 204 responses return
undefined:-export const request = async <T>({ +export const request = async <T, AllowEmpty extends boolean = false>({ endpoint, projectId, path, method = 'GET', impersonation = { type: 'none' }, payload, -}: RequestOptions): Promise<T> => { +}: RequestOptions & { allowEmpty?: AllowEmpty }): Promise<AllowEmpty extends true ? T | undefined : T> => {Alternatively, for this demo app, the current approach is acceptable since only
deleteCurrentSessionhits this path and usesRecord<string, never>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/src/api.ts` around lines 102 - 104, The current 204 branch returns "{} as T" which can be unsafe; change the branch to return undefined (i.e. "return undefined") and update the method's generic return type to reflect that 204 yields no body (e.g. Promise<T | undefined> or Promise<void> as appropriate). Update callers such as deleteCurrentSession to accept the new undefined result (or keep deleteCurrentSession typed as Record<string, never> if you prefer to leave that specific caller unchanged). Ensure the central response handling function (the block with "if (response.status === 204)") and any exported API function signatures are updated consistently to avoid unsafe asserts.demo/src/App.tsx (2)
21-21:localStorageaccess can throw in restricted contexts.In some browsers (Safari private browsing, certain iframe sandboxes),
localStorageaccess throws. While acceptable for a demo app, consider wrapping in try-catch for robustness.♻️ Defensive localStorage access
-const getStoredValue = (key: string, fallback: string) => localStorage.getItem(key) ?? fallback; +const getStoredValue = (key: string, fallback: string) => { + try { + return localStorage.getItem(key) ?? fallback; + } catch { + return fallback; + } +};Similarly for the
useEffectsetters on lines 33-39:useEffect(() => { - localStorage.setItem(storageKeys.endpoint, endpoint); + try { + localStorage.setItem(storageKeys.endpoint, endpoint); + } catch { + // Storage unavailable (e.g., private browsing) + } }, [endpoint]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/src/App.tsx` at line 21, Wrap all direct localStorage accesses in try/catch to avoid thrown exceptions in restricted contexts: update getStoredValue to catch errors and return the fallback on any exception, and similarly wrap any localStorage.setItem/removeItem calls used inside the component's useEffect setters so they silently fail (and optionally log) instead of throwing; locate functions/places by the getStoredValue function and the useEffect blocks that call localStorage.setItem/removeItem and add try/catch around those calls, returning fallback or skipping the write on error.
96-110: Consider handling the case whentypeis'none'.The
setTargetfunction receivesImpersonationHeader['type']which includes'none', but there's no explicit handling for it. While currently no caller passes'none', adding explicit handling would make the function more defensive.♻️ Add explicit 'none' handling
const setTarget = (type: ImpersonationHeader['type'], user: User) => { + if (type === 'none') { + setImpersonation({ type: 'none' }); + return; + } + if (type === 'id') { setImpersonation({ type, value: user.$id }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/src/App.tsx` around lines 96 - 110, setTarget currently ignores the 'none' option from ImpersonationHeader['type']; add an explicit branch for when type === 'none' to clear the impersonation state via setImpersonation. Update the setTarget function to detect type === 'none' and call setImpersonation with the appropriate "cleared" value your state expects (e.g. null/undefined or an explicit { type: 'none', value: '' } shape) so the UI/state is defensively reset when 'none' is passed.demo/src/index.css (2)
69-76: Consider removing quotes aroundSFMono-Regular.Stylelint prefers unquoted font family names when they don't contain spaces or special characters that require quoting. The hyphen doesn't require quotes.
♻️ Remove unnecessary quotes
- font-family: 'SFMono-Regular', ui-monospace, monospace; + font-family: SFMono-Regular, ui-monospace, monospace;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/src/index.css` around lines 69 - 76, In the code block for the CSS selector "code" in demo/src/index.css, remove the unnecessary quotes around the font-family token 'SFMono-Regular' so the font-family declaration reads with an unquoted SFMono-Regular followed by the fallback list (ui-monospace, monospace); update the font-family line in the "code" rule accordingly to satisfy stylelint's preference for unquoted font names without spaces or special characters.
9-9: CSS keyword casing:optimizeLegibilityshould be lowercase.Per CSS specification, keyword values should be lowercase. Stylelint flags this as an error.
♻️ Fix keyword case
- text-rendering: optimizeLegibility; + text-rendering: optimizelegibility;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/src/index.css` at line 9, The text-rendering declaration in index.css uses the camelCase keyword "optimizeLegibility"; update the keyword to lowercase so the declaration reads text-rendering: optimizelegibility; to satisfy stylelint's keyword-case rule—modify the value for the text-rendering property in demo/src/index.css accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@demo/src/api.ts`:
- Around line 83-93: The three separate if checks on impersonation.type should
be made mutually exclusive to reflect the ImpersonationHeader discriminated
union; update the logic that sets headers (the block referencing
impersonation.type and keys 'x-appwrite-impersonate-user-id',
'x-appwrite-impersonate-user-email', 'x-appwrite-impersonate-user-phone') to use
an else-if chain or a switch on impersonation.type so only one header is set per
call, leaving the rest unchanged.
- Around line 102-104: The current 204 branch returns "{} as T" which can be
unsafe; change the branch to return undefined (i.e. "return undefined") and
update the method's generic return type to reflect that 204 yields no body (e.g.
Promise<T | undefined> or Promise<void> as appropriate). Update callers such as
deleteCurrentSession to accept the new undefined result (or keep
deleteCurrentSession typed as Record<string, never> if you prefer to leave that
specific caller unchanged). Ensure the central response handling function (the
block with "if (response.status === 204)") and any exported API function
signatures are updated consistently to avoid unsafe asserts.
In `@demo/src/App.tsx`:
- Line 21: Wrap all direct localStorage accesses in try/catch to avoid thrown
exceptions in restricted contexts: update getStoredValue to catch errors and
return the fallback on any exception, and similarly wrap any
localStorage.setItem/removeItem calls used inside the component's useEffect
setters so they silently fail (and optionally log) instead of throwing; locate
functions/places by the getStoredValue function and the useEffect blocks that
call localStorage.setItem/removeItem and add try/catch around those calls,
returning fallback or skipping the write on error.
- Around line 96-110: setTarget currently ignores the 'none' option from
ImpersonationHeader['type']; add an explicit branch for when type === 'none' to
clear the impersonation state via setImpersonation. Update the setTarget
function to detect type === 'none' and call setImpersonation with the
appropriate "cleared" value your state expects (e.g. null/undefined or an
explicit { type: 'none', value: '' } shape) so the UI/state is defensively reset
when 'none' is passed.
In `@demo/src/index.css`:
- Around line 69-76: In the code block for the CSS selector "code" in
demo/src/index.css, remove the unnecessary quotes around the font-family token
'SFMono-Regular' so the font-family declaration reads with an unquoted
SFMono-Regular followed by the fallback list (ui-monospace, monospace); update
the font-family line in the "code" rule accordingly to satisfy stylelint's
preference for unquoted font names without spaces or special characters.
- Line 9: The text-rendering declaration in index.css uses the camelCase keyword
"optimizeLegibility"; update the keyword to lowercase so the declaration reads
text-rendering: optimizelegibility; to satisfy stylelint's keyword-case
rule—modify the value for the text-rendering property in demo/src/index.css
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04a108c1-7f7e-48ab-832a-782a02f38dbb
⛔ Files ignored due to path filters (6)
demo/package-lock.jsonis excluded by!**/package-lock.jsondemo/public/favicon.svgis excluded by!**/*.svgdemo/public/icons.svgis excluded by!**/*.svgdemo/src/assets/hero.pngis excluded by!**/*.pngdemo/src/assets/react.svgis excluded by!**/*.svgdemo/src/assets/vite.svgis excluded by!**/*.svg
📒 Files selected for processing (19)
app/config/cors.phpapp/controllers/shared/api.phpdemo/.envdemo/.env.exampledemo/.gitignoredemo/README.mddemo/eslint.config.jsdemo/index.htmldemo/package.jsondemo/src/App.cssdemo/src/App.tsxdemo/src/api.tsdemo/src/index.cssdemo/src/main.tsxdemo/tsconfig.app.jsondemo/tsconfig.jsondemo/tsconfig.node.jsondemo/vite.config.tstests/e2e/Services/Users/UsersBase.php
✅ Files skipped from review due to trivial changes (3)
- demo/.env
- demo/.gitignore
- demo/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/shared/api.php
Made-with: Cursor # Conflicts: # app/controllers/shared/api.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add impersonation feature for user management
This feature allows users with the appropriate permissions to impersonate other users, enhancing flexibility in user management.