Refactor authentication with RSA-based JWT and streamlined security context#106
Refactor authentication with RSA-based JWT and streamlined security context#106alexey-lapin merged 1 commit intomasterfrom
Conversation
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #106 +/- ##
============================================
- Coverage 67.35% 66.78% -0.57%
- Complexity 143 148 +5
============================================
Files 71 73 +2
Lines 582 566 -16
Branches 48 48
============================================
- Hits 392 378 -14
+ Misses 162 158 -4
- Partials 28 30 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the authentication mechanism to centralize authentication context handling. Instead of passing currentUsername parameters through commands and queries from controllers to handlers, the refactoring introduces direct usage of AuthenticationService in handlers to retrieve the authenticated user's information. Additionally, the JWT implementation is switched from HMAC-SHA256 symmetric signing to RSA asymmetric signing, and user IDs are now included in JWT claims for more efficient lookups.
Key changes:
- Removed
currentUsernamefields from all command and query DTOs, delegating authentication context retrieval to handlers - Enhanced
AuthenticationServicewith methods to retrieve current user ID and username from JWT claims - Migrated JWT signing from HMAC (HS256) to RSA (RS256) with dynamic key generation
Reviewed changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| SecurityConfig.java | Replaced secret-based HMAC JWT signing with RSA key generation and corresponding encoder/decoder beans |
| AuthenticationService.java | Added methods to retrieve current user ID/username with nullable and required variants |
| DefaultAuthenticationService.java | Implemented new authentication methods extracting claims from JWT principal |
| DefaultJwtService.java | Switched from MacAlgorithm.HS256 to SignatureAlgorithm.RS256 and added user ID claim |
| UserController.java | Removed AuthenticationService dependency and currentUsername parameter passing |
| ProfileController.java | Removed AuthenticationService dependency and currentUsername parameter passing |
| ArticleController.java | Removed AuthenticationService dependency and currentUsername parameter passing |
| GetCurrentUserHandler.java | Added AuthenticationService injection to retrieve current user by ID instead of username |
| UpdateUserHandler.java | Added AuthenticationService injection to retrieve current user by ID instead of username |
| GetProfileHandler.java | Simplified to use AuthenticationService for current user ID retrieval |
| GetFeedHandler.java | Refactored to use AuthenticationService instead of username-based lookup |
| GetArticlesHandler.java | Added AuthenticationService to retrieve current user ID for filtering |
| GetArticleHandler.java | Refactored to use AuthenticationService for current user context |
| GetCommentsHandler.java | Simplified current user retrieval using AuthenticationService |
| GetCommentHandler.java | Simplified current user retrieval using AuthenticationService |
| CreateArticleHandler.java | Replaced username-based user lookup with ID-based lookup via AuthenticationService |
| UpdateArticleHandler.java | Replaced username-based user lookup with ID-based authentication |
| DeleteArticleHandler.java | Refactored authorization check to use current user ID from AuthenticationService |
| FavoriteArticleHandler.java | Replaced username-based user lookup with ID-based authentication |
| UnfavoriteArticleHandler.java | Replaced username-based user lookup with ID-based authentication |
| AddCommentHandler.java | Refactored to use AuthenticationService for current user ID |
| DeleteCommentHandler.java | Refactored authorization check to use current user ID from AuthenticationService |
| FollowProfileHandler.java | Replaced username-based follower lookup with ID-based authentication |
| UnfollowProfileHandler.java | Replaced username-based follower lookup with ID-based authentication |
| SlugService.java | Improved variable naming from NONLATIN to NON_LATIN and noseparators to noSeparators |
| UnauthorizedException.java | Removed redundant super() call from no-arg constructor |
| package-info.java | Added @NullMarked annotation to service package |
| LoginUser.java | Added @notblank validation to email field |
| RegisterUser.java | Added @notblank validation to email field and improved formatting |
| UpdateArticle.java | Removed currentUsername field and adjusted builder configuration |
| AddComment.java | Removed currentUsername field and added @with annotation for slug |
| All command/query DTOs | Removed currentUsername/follower fields from various commands and queries |
| Bus files | Minor formatting improvements with additional blank lines |
| App.java | Minor formatting improvements with additional blank lines |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public RSAKey rsaKey() throws Exception { | ||
| return new RSAKeyGenerator(2048).generate(); | ||
| } |
There was a problem hiding this comment.
The RSA key is being generated dynamically on each application startup. This means existing JWT tokens will become invalid whenever the application restarts, forcing all users to re-authenticate. For production environments, the RSA key pair should be either loaded from a persistent store (file system, secrets manager) or remain static across deployments to maintain session continuity.
|
|
||
| private final AuthenticationService authenticationService; | ||
| private final ArticleRepository articleRepository; | ||
| private final UserRepository userRepository; |
There was a problem hiding this comment.
The UserRepository is no longer used in this handler after the refactoring but is still injected as a dependency. This dependency should be removed to keep the code clean.
|
|
||
| if (comment.authorId() != currentUser.id()) { | ||
| throw forbidden("comment [id=%s] is not owned by %s", comment.id(), command.getCurrentUsername()); | ||
| if (comment.authorId() != currentUserId) { |
There was a problem hiding this comment.
The comparison uses != for Long values. While this works when both are primitives, if comment.authorId() returns a Long object rather than a primitive long, this could produce incorrect results due to reference comparison rather than value comparison. Use .equals() for null-safe comparison: !Objects.equals(comment.authorId(), currentUserId).
| var user = userRepository.findById(currentUserId) | ||
| .orElseThrow(() -> notFound("user [name=%s] does not exist", | ||
| authenticationService.getCurrentUserName())); |
There was a problem hiding this comment.
The error message says the user doesn't exist by name, but the lookup is now done by ID. If a user ID from the JWT is not found in the database, it indicates a data inconsistency issue. The error message should be updated to reflect this, for example: "user [id=%d] does not exist" with the currentUserId parameter.
| public JwtEncoder jwtEncoder(RSAKey rsaKey) throws Exception { | ||
| return NimbusJwtEncoder.withKeyPair(rsaKey.toRSAPublicKey(), rsaKey.toRSAPrivateKey()).build(); |
There was a problem hiding this comment.
The JwtEncoder is being constructed with an incorrect method signature. The withKeyPair method does not exist in NimbusJwtEncoder. The correct approach is to use NimbusJwtEncoder with a JWKSource. You should create an ImmutableJWKSet containing the RSAKey and pass it to the NimbusJwtEncoder constructor.
| var uid = jwt.getClaimAsString("uid"); | ||
| if (uid == null) { | ||
| return null; | ||
| } | ||
| return Long.valueOf(uid); |
There was a problem hiding this comment.
The user ID claim is being stored as a string but should be stored as a long. When encoding the JWT, the claim is set with user.id() which is a Long, but when decoding it's retrieved as a string and parsed with Long.valueOf(). This inconsistency could lead to issues. Use jwt.getClaim("uid") with proper type handling, or store it consistently as a number type in the JWT claims.
| .orElseThrow(() -> badRequest("user [name=%s] does not exist", command.getCurrentUsername())); | ||
|
|
||
| if (!Objects.equals(article.authorId(), currentUser.id())) { | ||
| if (article.authorId() != currentUserId) { |
There was a problem hiding this comment.
The comparison uses != for Long values (primitive long vs Long object comparison). While this works for unboxed values, it could fail if either value is null or cause unexpected behavior with autoboxing. Since authorId() returns a long primitive and currentUserId is a Long, use .equals() for null-safe comparison or ensure both are primitives. The pattern should be: !Objects.equals(article.authorId(), currentUserId) or ensure currentUserId is unboxed.
|
|
||
| if (!Objects.equals(article.authorId(), currentUser.id())) { | ||
| throw forbidden("article [slug=%s] is not owned by %s", command.getSlug(), command.getCurrentUsername()); | ||
| if (article.authorId() != currentUserId) { |
There was a problem hiding this comment.
The comparison uses != for Long values. While this works when both are primitives, if article.authorId() returns a Long object rather than a primitive long, this could produce incorrect results due to reference comparison rather than value comparison. Use .equals() for null-safe comparison: !Objects.equals(article.authorId(), currentUserId).
| @Override | ||
| public String getToken(User user) { | ||
| JwsHeader header = JwsHeader.with(MacAlgorithm.HS256).build(); | ||
| // JwsHeader header = JwsHeader.with(MacAlgorithm.HS256).build(); |
There was a problem hiding this comment.
Commented-out code should be removed rather than left in the codebase. If this is kept for reference during development, it should be removed before merging to keep the code clean.
| var user = userRepository.findById(currentUserId) | ||
| .orElseThrow(() -> badRequest("user [name=%s] does not exist", | ||
| authenticationService.getCurrentUserName())); |
There was a problem hiding this comment.
The error message says the user doesn't exist by name, but the lookup is now done by ID. If a user ID from the JWT is not found in the database, it indicates a data inconsistency issue. The error message should be updated to reflect this, for example: "user [id=%d] does not exist" with the currentUserId parameter, or use a more generic unauthorized error message since this scenario suggests an invalid/stale token.
alexey-lapin
force-pushed
the
feature/refactor-auth
branch
from
a68b7f9 to
12ddfd5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 51 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alexey-lapin
changed the title
2 participants
No description provided.