Prevent login if user is disabled #3509
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new authentication exception hierarchy and refactor existing exceptions accordingly. Authentication logic is updated to handle disabled users, including immediate session invalidation upon user disablement. Repositories are updated to return user status with password hash, and related methods are renamed. A new event handler ensures disabled users are logged out. Tests are added to verify disabled user behavior. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (12)
✨ 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
CodeRabbit Configuration File (
|
![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/core/authentication/handlers/disabling-user-logs-them-out.handler.ts (1)
1-13: Well-implemented event handler with minor suggestion.The handler correctly listens for
UserUpdatedEventand triggers logout when a user is disabled. The implementation follows NestJS patterns properly.Consider adding error handling around the
logoutByUsercall to prevent the handler from failing silently if the logout operation encounters issues:async handle({ input, updated: user }: UserUpdatedEvent) { if (input.status === 'Disabled') { - await this.auth.logoutByUser(user.id); + try { + await this.auth.logoutByUser(user.id); + } catch (error) { + // Log error but don't rethrow to avoid preventing user status update + console.error('Failed to logout disabled user:', error); + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/common/exceptions/unauthenticated.exception.ts(1 hunks)src/core/authentication/authentication.gel.repository.ts(2 hunks)src/core/authentication/authentication.module.ts(2 hunks)src/core/authentication/authentication.repository.ts(3 hunks)src/core/authentication/authentication.service.ts(4 hunks)src/core/authentication/handlers/disabling-user-logs-them-out.handler.ts(1 hunks)test/authentication.e2e-spec.ts(2 hunks)test/utility/login.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
test/utility/login.ts (1)
src/graphql.ts (1)
graphql(6-18)
src/core/authentication/authentication.gel.repository.ts (1)
src/common/id-field.ts (1)
ID(24-25)
src/common/exceptions/unauthenticated.exception.ts (1)
src/common/exceptions/exception.ts (1)
ClientException(16-16)
src/core/authentication/authentication.repository.ts (2)
src/core/database/query/matching.ts (1)
ACTIVE(33-33)src/common/id-field.ts (1)
ID(24-25)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: Unit
- GitHub Check: lint
- GitHub Check: Generate (base)
- GitHub Check: Generate (head)
- GitHub Check: Clean
- GitHub Check: Analyze (javascript)
🔇 Additional comments (18)
test/utility/login.ts (1)
11-11: LGTM! Good refactoring for test reusability.Exporting
LoginDocenables its reuse in other test files, which is exactly what's needed for the new authentication tests.src/core/authentication/authentication.module.ts (2)
9-9: LGTM! Proper import for the new event handler.The import correctly references the new event handler that will manage disabled user logout.
43-43: LGTM! Correct provider registration.The handler is properly registered as a provider, enabling it to listen for user update events and trigger logout when users are disabled.
test/authentication.e2e-spec.ts (2)
12-14: LGTM! Proper imports for the new test functionality.The imports correctly reference the utility functions needed for the disabled user test case.
122-159: Excellent test coverage for disabled user behavior.This test comprehensively verifies the disabled user feature:
- Confirms user is initially logged in after registration
- Disables the user and verifies immediate logout
- Confirms login prevention with proper error message and codes
The test flow matches the expected behavior and provides good coverage for both the logout trigger and login prevention aspects.
src/core/authentication/authentication.service.ts (6)
5-5: LGTM! Proper import for the new exception hierarchy.The import correctly adds the new
AuthenticationExceptionbase class.
77-77: Good enhancement to fetch comprehensive user information.Changing from
getPasswordHashByEmailtogetInfoForLoginallows the service to access both password hash and user status in a single repository call, which is more efficient.
79-79: LGTM! Updated to use the new info structure.The password verification correctly uses the
passwordHashfrom the new info object structure.
82-84: Excellent security implementation for disabled user check.The logic correctly:
- Verifies the password first (preventing information disclosure)
- Only then checks if the user is disabled
- Throws a specific exception for disabled users
This approach prevents timing attacks and maintains proper security practices.
105-107: Well-implemented programmatic logout method.The
logoutByUsermethod provides the necessary functionality for the event handler to log out users when they're disabled. The implementation delegates to the repository appropriately.
164-168: Proper exception implementation for disabled users.The
UserDisabledExceptioncorrectly extendsAuthenticationExceptionand provides a clear, specific error message. This allows clients to handle disabled user scenarios appropriately.src/core/authentication/authentication.gel.repository.ts (2)
67-78: LGTM! Method rename and enhanced return data align with disabled user requirements.The method rename from
getPasswordHashtogetInfoForLoginaccurately reflects its expanded functionality. The query now correctly returns bothpasswordHashandstatusfrom theAuth.Identitytable, which enables the authentication service to check user status during login.
284-296: Well-implemented session deactivation for disabled users.The new
deactivateAllSessionsmethod correctly nullifies the user field on all sessions for a given user, effectively logging them out. The implementation properly uses the typedID<'User'>parameter and follows the established query pattern.src/common/exceptions/unauthenticated.exception.ts (2)
4-9: Excellent exception hierarchy design.The new abstract
AuthenticationExceptionbase class provides a clean foundation for all authentication-related exceptions with consistent HTTP 401 status. This design supports the addition of specific authentication exceptions likeUserDisabledExceptionwhile maintaining consistency.
11-18: Clean refactoring maintains backward compatibility.The
UnauthenticatedExceptionrefactoring to extendAuthenticationExceptionmaintains its existing behavior while fitting into the new exception hierarchy. The specific documentation clearly distinguishes this from other authentication exceptions.src/core/authentication/authentication.repository.ts (3)
5-5: Appropriate import for enhanced user status handling.The
UserStatusimport supports the expandedgetInfoForLoginmethod functionality to return user status alongside password hash.
101-129: Robust query implementation for user info retrieval.The updated
getInfoForLoginmethod correctly implements the complex Cypher query to match email, user, password, and status nodes with active relationships. The structured query builder pattern improves maintainability compared to raw Cypher, and the return type properly reflects bothpasswordHashandstatusfields.
363-374: Effective session deactivation implementation.The
deactivateAllSessionsmethod correctly matches the user and their active token relationships, then deactivates them by settingactive: false. This aligns with the Cypher database pattern and supports the requirement to log out disabled users.
CarsonF
force-pushed
the
disabled-user-no-login
branch
from
cd16a3a to
930dba2
Compare
No description provided.