Added auth option to allow invalidating existing sessions#10010
Added auth option to allow invalidating existing sessions#10010abnegate merged 10 commits intoappwrite:1.8.xfrom
Conversation
WalkthroughA new project-level boolean attribute Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant DB
Client->>API: PATCH /v1/projects/:projectId/auth/session-invalidation (invalidateSessions)
API->>DB: Find project by projectId
API->>DB: Update project's auths['invalidateSessions']
API-->>Client: Return updated project document
sequenceDiagram
participant User
participant API
participant DB
participant Cache
User->>API: PATCH /v1/users/{userId}/password (new password)
API->>DB: Update user password and history
API->>DB: Check project.auths['invalidateSessions']
alt invalidateSessions is true
API->>DB: Identify current session
API->>DB: Delete all other user sessions except current
API->>Cache: Purge cached user document
end
API-->>User: Return updated user document
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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: 6
🔭 Outside diff range comments (1)
app/controllers/api/projects.php (1)
118-132:⚠️ Potential issueStore a strict boolean to avoid truthy/falsy surprises
$onPasswordChangecan arrive as the string'true'/'false','1'/'0', or a native bool depending on HTTP client & framework parsing.
Down-stream logic (users controller) expects a real boolean. Cast explicitly, mirroring the pattern used for$statuslater in the file:+$onPasswordChange = ($onPasswordChange === '1' || $onPasswordChange === 'true' || + $onPasswordChange === 1 || $onPasswordChange === true);Place this right before
$auths = [to guarantee we persist a strict bool and keep the stored schema consistent.
🧹 Nitpick comments (4)
tests/e2e/Scopes/ProjectCustom.php (1)
203-215: Unnecessaryarray_merge()call adds overhead & suppresses static analysis
array_merge()with a single argument is a no-op and flags on recent PHP-Stan/Psalm rules.
Just pass the headers array directly:- $response = $this->client->call(Client::METHOD_PATCH, '/projects/' . self::$project['$id'] . '/auth/password-change', array_merge([ + $response = $this->client->call( + Client::METHOD_PATCH, + '/projects/' . self::$project['$id'] . '/auth/password-change', + [ 'origin' => 'http://localhost', 'content-type' => 'application/json', 'cookie' => 'a_session_console=' . $this->getRoot()['session'], 'x-appwrite-project' => 'console', - ]), [ + ], + [ 'onPasswordChange' => $value, ] - ]); + );app/controllers/api/users.php (1)
1357-1361: Stalesessionsarray returned to callerAfter deleting the session documents,
$userstill contains the oldsessionslist, so the response shows sessions that no longer exist.
Consider refreshing the user document or purging/clearing thesessionsattribute before returning.tests/e2e/Services/Users/UsersBase.php (1)
1132-1139: Add explicit assertions on the sessions payloadThe test rightfully checks that no sessions remain, but it does so by assuming the response body contains a
sessionskey. To make intent clear and avoid silent shape changes in the API:$this->assertEquals(200, $sessions['headers']['status-code']); +$this->assertArrayHasKey('sessions', $sessions['body']); $this->assertIsArray($sessions['body']); $this->assertEmpty($sessions['body']['sessions']);Including the
assertArrayHasKeyprotects against accidental contract changes where the field is renamed or removed.app/controllers/api/projects.php (1)
97-97: Clarify parameter semantics & wordingThe label
'Auth option to allow invalidating existing sessions'is vague and doesn’t communicate when invalidation happens.
Consider a clearer name/description, e.g. “Invalidate all user sessions when a user’s password is changed.”Tagging will also improve autogenerated API docs and SDK code-gen.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/references/databases/update-auth-on-password-change.mdis excluded by!docs/references/**
📒 Files selected for processing (6)
app/controllers/api/projects.php(3 hunks)app/controllers/api/users.php(1 hunks)src/Appwrite/Utopia/Response/Model/Project.php(2 hunks)tests/e2e/Scopes/ProjectCustom.php(1 hunks)tests/e2e/Services/Projects/ProjectsConsoleClientTest.php(1 hunks)tests/e2e/Services/Users/UsersBase.php(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/e2e/Services/Account/AccountCustomClientTest.php (1)
483-495: Trim 5 s from test by ditching the per-iterationsleep(1)The one-second pause inside the loop slows the suite without adding functional value.
If you only need distinct timestamps,usleep(100_000)(0.1 s) is usually enough; otherwise the delay can be removed entirely.- $this->assertEquals(201, $response['headers']['status-code']); - sleep(1); + $this->assertEquals(201, $response['headers']['status-code']); + // Brief pause to avoid potential race conditions while keeping the test fast + usleep(100_000); // 0.1 s
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/account.php(1 hunks)tests/e2e/Services/Account/AccountCustomClientTest.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: scan-pr / osv-scan
- GitHub Check: Check if utopia-php/database changed
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Linter
- GitHub Check: scan
🔇 Additional comments (1)
app/controllers/api/account.php (1)
2846-2853: Should this honor the project-levelauthInvalidateSessionsflag?The PR description mentions a new per-project boolean to toggle session invalidation on password change.
The users controller (updatePasswordinusers.php) checks that flag, but this account endpoint invalidates unconditionally. If the intent is that self-service password changes always revoke other sessions, ignore this note. Otherwise, wrap the block in:if ($project->getAttribute('authInvalidateSessions', true)) { … }
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
tests/e2e/Services/Account/AccountCustomClientTest.php (2)
483-495: Address the session creation logic issueBased on the past review comment, creating multiple sessions when you already have an active one should fail according to the application's business rules. This test is creating 5 sessions successfully, which may not reflect the actual expected behavior.
526-561: Implement negative assertions for stronger session validationThe current test only verifies that the session count is 1 and checks for 404 responses when accessing invalidated sessions. However, it doesn't prove that the stale session cookies are actually unusable for API requests.
Consider capturing the session cookies before password change and then making authenticated requests with each stale cookie to assert they return 401 unauthorized status. This would provide stronger evidence that sessions are truly invalidated.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/account.php(1 hunks)tests/e2e/Services/Account/AccountCustomClientTest.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/api/account.php
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: Benchmark
🔇 Additional comments (2)
tests/e2e/Services/Account/AccountCustomClientTest.php (2)
564-578: Good implementation of disabled invalidation testingThis section properly addresses the past review comment about testing when
invalidateSessionsis false. The logic correctly creates multiple sessions and prepares to verify they remain valid after password update.
601-610: Good verification of session persistenceThis section correctly verifies that when session invalidation is disabled, all sessions remain accessible after password update. The logic properly iterates through all session IDs and confirms they return 200 status codes.
| foreach ($allSessions as $sessionId) { | ||
| if ($currentSessionId !== $sessionId) { | ||
| $response = $this->client->call(Client::METHOD_GET, '/account/sessions/'.$sessionId, array_merge([ | ||
| 'origin' => 'http://localhost', | ||
| 'content-type' => 'application/json', | ||
| 'x-appwrite-project' => $this->getProject()['$id'], | ||
| 'cookie' => 'a_session_' . $this->getProject()['$id'] . '=' . $session, | ||
| ])); | ||
|
|
||
| $this->assertEquals(404, $response['headers']['status-code']); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify the session invalidation logic
The test fetches all sessions after password update and then checks that non-current sessions return 404. However, there's a logical issue: if session invalidation is working correctly, the getAllSessions call should only return the current session, making the foreach loop unnecessary.
Consider asserting that only one session exists before iterating, or restructure to use the previously captured session IDs for validation:
+ $this->assertEquals(1, $response['body']['total']);
$allSessions = array_map(fn ($sessionDetails) => $sessionDetails['$id'], $response['body']['sessions']);📝 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.
| foreach ($allSessions as $sessionId) { | |
| if ($currentSessionId !== $sessionId) { | |
| $response = $this->client->call(Client::METHOD_GET, '/account/sessions/'.$sessionId, array_merge([ | |
| 'origin' => 'http://localhost', | |
| 'content-type' => 'application/json', | |
| 'x-appwrite-project' => $this->getProject()['$id'], | |
| 'cookie' => 'a_session_' . $this->getProject()['$id'] . '=' . $session, | |
| ])); | |
| $this->assertEquals(404, $response['headers']['status-code']); | |
| } | |
| } | |
| // After fetching all sessions, assert only the current session remains | |
| $this->assertEquals(1, $response['body']['total']); | |
| $allSessions = array_map(fn ($sessionDetails) => $sessionDetails['$id'], $response['body']['sessions']); | |
| foreach ($allSessions as $sessionId) { | |
| if ($currentSessionId !== $sessionId) { | |
| $response = $this->client->call(Client::METHOD_GET, '/account/sessions/'.$sessionId, \array_merge([ | |
| 'origin' => 'http://localhost', | |
| 'content-type' => 'application/json', | |
| 'x-appwrite-project' => $this->getProject()['$id'], | |
| 'cookie' => 'a_session_' . $this->getProject()['$id'] . '=' . $session, | |
| ])); | |
| $this->assertEquals(404, $response['headers']['status-code']); | |
| } | |
| } |
🤖 Prompt for AI Agents
In tests/e2e/Services/Account/AccountCustomClientTest.php around lines 635 to
646, the test incorrectly loops over all sessions after password update, but if
session invalidation works, only the current session should remain. Fix this by
first asserting that the list of sessions contains only one session (the current
one) before any iteration, or adjust the test to validate session IDs captured
before the password update to confirm invalidation, removing unnecessary looping
over all sessions.
| $response = $this->client->call(Client::METHOD_PATCH, '/account/password', array_merge([ | ||
| 'origin' => 'http://localhost', | ||
| 'content-type' => 'application/json', | ||
| 'x-appwrite-project' => $this->getProject()['$id'], | ||
| 'cookie' => 'a_session_' . $this->getProject()['$id'] . '=' . $session, | ||
| ]), [ | ||
| 'password' => $newPassword, | ||
| 'oldPassword' => $newPassword, | ||
| ]); |
There was a problem hiding this comment.
Another password inconsistency issue
Similar to the earlier issue, using the same value for both password and oldPassword parameters doesn't make logical sense for a password update operation.
- ]), [
- 'password' => $newPassword,
- 'oldPassword' => $newPassword,
- ]);
+ ]), [
+ 'password' => 'final-new-password',
+ 'oldPassword' => $newPassword,
+ ]);📝 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.
| $response = $this->client->call(Client::METHOD_PATCH, '/account/password', array_merge([ | |
| 'origin' => 'http://localhost', | |
| 'content-type' => 'application/json', | |
| 'x-appwrite-project' => $this->getProject()['$id'], | |
| 'cookie' => 'a_session_' . $this->getProject()['$id'] . '=' . $session, | |
| ]), [ | |
| 'password' => $newPassword, | |
| 'oldPassword' => $newPassword, | |
| ]); | |
| $response = $this->client->call(Client::METHOD_PATCH, '/account/password', array_merge([ | |
| 'origin' => 'http://localhost', | |
| 'content-type' => 'application/json', | |
| 'x-appwrite-project' => $this->getProject()['$id'], | |
| 'cookie' => 'a_session_' . $this->getProject()['$id'] . '=' . $session, | |
| ]), [ | |
| 'password' => 'final-new-password', | |
| 'oldPassword' => $newPassword, | |
| ]); |
🤖 Prompt for AI Agents
In tests/e2e/Services/Account/AccountCustomClientTest.php around lines 614 to
622, the password update call incorrectly uses the same value for both
'password' and 'oldPassword' parameters. To fix this, ensure 'oldPassword' holds
the current password value and 'password' holds the new password value to
correctly represent a password change operation.
| $response = $this->client->call(Client::METHOD_PATCH, '/account/password', array_merge([ | ||
| 'origin' => 'http://localhost', | ||
| 'content-type' => 'application/json', | ||
| 'x-appwrite-project' => $this->getProject()['$id'], | ||
| 'cookie' => 'a_session_' . $this->getProject()['$id'] . '=' . $session, | ||
| ]), [ | ||
| 'password' => $newPassword, | ||
| 'oldPassword' => $newPassword, | ||
| ]); |
There was a problem hiding this comment.
Inconsistent password usage in test
The test is updating the password using the same value for both password and oldPassword parameters ($newPassword). This seems incorrect - typically you would use the current password as oldPassword and a different value for the new password.
- ]), [
- 'password' => $newPassword,
- 'oldPassword' => $newPassword,
- ]);
+ ]), [
+ 'password' => 'another-new-password',
+ 'oldPassword' => $newPassword,
+ ]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/e2e/Services/Account/AccountCustomClientTest.php around lines 589 to
597, the test incorrectly uses the same value for both 'password' and
'oldPassword' parameters when calling the password update endpoint. To fix this,
set 'oldPassword' to the current password value and 'password' to the new
password value, ensuring the test correctly simulates changing from the old
password to a new one.
2 participants
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests