Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes the logout functionality by enhancing security and improving the API design. The logout endpoint is changed from a GET to a POST method with proper authentication and authorization checks. The key improvement is that the logout endpoint now verifies that users can only revoke their own sessions, preventing unauthorized session termination. Additionally, the OAuth callback response now includes the session_id in the redirect URL, enabling the frontend to track and manage specific sessions.
- Changed logout from GET (query parameter) to POST (request body) with authentication middleware
- Added session ownership verification to prevent users from revoking other users' sessions
- Added
get_session_by_idmethod to the SessionRepository trait and implementation - Included session_id in OAuth callback redirect to frontend
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/services/src/auth/ports.rs | Added get_session_by_id method to SessionRepository trait for session lookup by ID |
| crates/database/src/repositories/session_repository.rs | Implemented get_session_by_id to retrieve sessions from the database by ID |
| crates/api/src/routes/oauth.rs | Changed logout from GET to POST, added authentication requirement, session ownership validation, and included session_id in callback redirect |
| crates/api/src/routes/mod.rs | Moved logout route to authenticated router with auth middleware |
| crates/api/src/error.rs | Added session_id_not_found error for when a session ID doesn't exist |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| ApiError::session_id_not_found() | ||
| })?; | ||
|
|
||
| // Verify that the session belongs to the authenticated user |
There was a problem hiding this comment.
The comment on line 253 is duplicated on line 268. Remove the duplicate comment on line 268 since the verification logic (lines 269-277) is adequately explained by the first comment.
| // Verify that the session belongs to the authenticated user |
| /// Session id not found | ||
| pub fn session_id_not_found() -> Self { | ||
| Self::not_found("Session id not found") | ||
| .with_details("The provided session id does not match any active session.") |
There was a problem hiding this comment.
Inconsistent naming: 'Session id' should be 'Session ID' (uppercase ID) to match the naming convention used in other error messages and documentation in the codebase.
| /// Session id not found | |
| pub fn session_id_not_found() -> Self { | |
| Self::not_found("Session id not found") | |
| .with_details("The provided session id does not match any active session.") | |
| /// Session ID not found | |
| pub fn session_id_not_found() -> Self { | |
| Self::not_found("Session ID not found") | |
| .with_details("The provided session ID does not match any active session.") |
Fixes nearai/private-chat#129