Add JWT authentication module and static analysis integration#3
Add JWT authentication module and static analysis integration#3phongphongg wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new authentication module with JSON-based login security infrastructure, adds an API response framework to the common module, integrates compile-time null-checking via Error Prone and NullAway, reorganizes module component scanning, and establishes consistent package-level null-safety annotations across modules. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SecurityFilterChain
participant JsonLoginAuthenticationFilter
participant AuthenticationManager
participant UserDetailsService
participant JsonAuthSuccessHandler
participant JsonAuthFailureHandler
participant Response
Client->>SecurityFilterChain: POST /api/v1/auth/login (JSON)
SecurityFilterChain->>JsonLoginAuthenticationFilter: intercept request
JsonLoginAuthenticationFilter->>JsonLoginAuthenticationFilter: parse JSON (username, password)
JsonLoginAuthenticationFilter->>JsonLoginAuthenticationFilter: create UsernamePasswordAuthenticationToken
JsonLoginAuthenticationFilter->>AuthenticationManager: authenticate(token)
alt Authentication Success
AuthenticationManager->>UserDetailsService: loadUserByUsername(username)
UserDetailsService-->>AuthenticationManager: return UserDetails
AuthenticationManager-->>JsonLoginAuthenticationFilter: authenticated token
JsonLoginAuthenticationFilter->>JsonAuthSuccessHandler: onAuthenticationSuccess()
JsonAuthSuccessHandler->>Response: HTTP 200 + JSON {success: true, username}
else Authentication Failure
AuthenticationManager-->>JsonLoginAuthenticationFilter: throw AuthenticationException
JsonLoginAuthenticationFilter->>JsonAuthFailureHandler: onAuthenticationFailure(exception)
JsonAuthFailureHandler->>Response: HTTP 401 + JSON {success: false, error}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
phongphongg
force-pushed
the
feat/authentication
branch
2 times, most recently
from
da20aa8 to
d782581
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
phongphongg
force-pushed
the
feat/authentication
branch
9 times, most recently
from
07ef2c5 to
235ded8
Compare
|
/gemini summary |
Summary of ChangesThis pull request significantly enhances the project's security posture by introducing a dedicated authentication module built on Spring Security and JSON Web Tokens. Concurrently, it elevates code quality standards and developer experience through the integration of advanced static analysis tools and refined build configurations. The changes lay a robust foundation for secure API interactions and improved code maintainability. Highlights
Changelog
Ignored Files
Activity
|
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
modules/console/src/main/resources/application-test.yaml (1)
1-6: Same JWT secret management considerations as inapplication-dev.yaml.This test profile repeats the same
workastra.jwtblock. The guidance about treatingsecretas a non-production placeholder and sourcing real secrets from env/secret management (see comment onapplication-dev.yaml) applies here as well.
🧹 Nitpick comments (10)
modules/console/src/main/java/com/workastra/console/package-info.java (1)
1-4: Package-level @NullMarked usage is fine; consider documenting package semanticsThe null-safety annotation and import are correct and idiomatic for package-info. To improve clarity for future readers, consider adding a brief Javadoc on the package describing its role and the “non-null by default” contract enforced by
@NullMarked.modules/authentication/src/main/java/com/workastra/authentication/controller/package-info.java (1)
1-4: Null-safety default for controllers looks goodUsing
@NullMarkedat the controller package is appropriate and consistent. As a minor improvement, you could add a package Javadoc clarifying that controller code is “non-null by default” and how to handle nullable inputs/outputs (e.g., explicitly annotate them).modules/worker/src/main/java/com/workastra/worker/package-info.java (1)
1-4: Worker package null-default is correctly definedThe package-level
@NullMarkedand import are correct and align with the rest of the codebase. Optionally, add a short package Javadoc describing the worker module’s responsibility and the non-null-by-default convention.modules/authentication/src/main/java/com/workastra/authentication/configuration/package-info.java (1)
1-4: Configuration package null-safety annotation is consistent and correct
@NullMarkedat the configuration package level is appropriate and consistent with other modules. For extra clarity, consider adding package-level Javadoc summarizing the configuration role and stating that all declarations are non-null by default unless explicitly annotated otherwise.modules/console/src/main/java/com/workastra/console/WorkastraConsoleApplication.java (1)
6-6: Be explicit about component scan scope instead of scanning the entirecom.workastratree.Switching to
scanBasePackages = "com.workastra"will pick up beans from every submodule on the classpath (including worker or future apps), which can lead to unintended controllers/configs being loaded into the console context. Consider narrowing this to only the modules you actually want in the console app, e.g.:-@SpringBootApplication(scanBasePackages = "com.workastra") +@SpringBootApplication( + scanBasePackages = { + "com.workastra.console", + "com.workastra.common", + "com.workastra.authentication" + } +)This aligns better with least surprise and avoids accidental cross-wiring between applications.
modules/console/build.gradle.kts (1)
8-12: Simplify annotation processor wiring if you don’t need processors on the compile classpath.The new dependency on
:modules:authenticationand thespring-boot-configuration-processorlook good. ThecompileOnly { extendsFrom(configurations.annotationProcessor.get()) }block is only necessary if you specifically want all annotation processors also on the compile classpath (common for Lombok, not required for Spring Boot’s configuration-processor).If you’re only using
spring-boot-configuration-processor, you can simplify to:-configurations { - compileOnly { - extendsFrom(configurations.annotationProcessor.get()) - } -} - dependencies { implementation(project(":modules:common")) implementation(project(":modules:authentication")) implementation("org.springframework.boot:spring-boot-starter-actuator") implementation("org.springframework.boot:spring-boot-starter-webmvc") annotationProcessor("org.springframework.boot:spring-boot-configuration-processor") ... }This keeps the compile classpath narrower and avoids leaking processors into places they’re not needed.
Also applies to: 16-16, 19-19
gradle/libs.versions.toml (1)
6-7: Static analysis additions look good; centralize versions and confirm JDK compatibility.Adding
error-prone-core,nullaway, and thenet.ltgt.errorproneplugin is a solid move. Two suggestions:
Centralize versions for easier upgrades
Consider moving these version numbers into the[versions]section and referencing them viaversion.reffor consistency withspring-boot:[versions] spring-boot = "4.0.0" error-prone = "2.44.0" nullaway = "0.12.12" [libraries] error-prone-core = { module = "com.google.errorprone:error_prone_core", version.ref = "error-prone" } nullaway = { module = "com.uber.nullaway:nullaway", version.ref = "nullaway" } [plugins] net-ltgt-errorprone = { id = "net.ltgt.errorprone", version = "4.3.0" }Verify compatibility with your JDK level
Please double-check that these specific versions support the Java version you’re building with (especially if you’re on a recent JDK).Also applies to: 13-13
modules/console/src/main/resources/application-dev.yaml (1)
1-6: Avoid shipping a concrete default JWT secret, even in dev configs.The
secrethere is clearly a placeholder, which is good, but to reduce the risk of someone promoting this config unchanged to a higher environment, consider:
- Removing the default secret value and requiring it via env/secret manager, or
- Keeping the placeholder but adding an explicit comment or validation that fails fast if the placeholder value is used at runtime.
This keeps JWT key management safer and makes misconfigurations more obvious.
modules/common/build.gradle.kts (1)
6-9: Reconsider leakingspring-boot-starter-webmvcviaapifromcommonDeclaring
spring-boot-starter-webmvcas anapidependency in thecommonmodule means every module depending oncommonis now transitively tied to MVC/web. For a “common” library this is usually undesirable and increases coupling.If only web-oriented modules need MVC, prefer:
-dependencies { - implementation(platform(libs.spring.boot.bom)) - api("org.springframework.boot:spring-boot-starter-webmvc") -} +dependencies { + implementation(platform(libs.spring.boot.bom)) + implementation("org.springframework.boot:spring-boot-starter-webmvc") +}or move MVC-specific code to a dedicated web module.
modules/authentication/src/main/java/com/workastra/authentication/controller/JwtTokenService.java (1)
1-2: MoveJwtTokenServiceout of thecontrollerpackageThis is a pure service component (
@Service) with no web concerns, but it lives incom.workastra.authentication.controller. Mixing services into controller packages hurts layering and discoverability.Consider moving it to something like
com.workastra.authentication.service(and updating imports) to keep controllers thin and services grouped.Also applies to: 16-17
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.github/workflows/workastra_server.yaml(1 hunks).vscode/launch.json(1 hunks)build.gradle.kts(3 hunks)config/checkstyle/checkstyle.xml(1 hunks)gradle/libs.versions.toml(1 hunks)modules/authentication/build.gradle.kts(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/configuration/JwtConfiguration.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/configuration/package-info.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/controller/AuthController.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/controller/JwtTokenService.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/controller/package-info.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/package-info.java(1 hunks)modules/common/build.gradle.kts(1 hunks)modules/console/build.gradle.kts(1 hunks)modules/console/src/main/java/com/workastra/console/WorkastraConsoleApplication.java(1 hunks)modules/console/src/main/java/com/workastra/console/package-info.java(1 hunks)modules/console/src/main/resources/META-INF/additional-spring-configuration-metadata.json(1 hunks)modules/console/src/main/resources/application-dev.yaml(1 hunks)modules/console/src/main/resources/application-test.yaml(1 hunks)modules/console/src/main/resources/application.yaml(1 hunks)modules/worker/src/main/java/com/workastra/worker/package-info.java(1 hunks)settings.gradle.kts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
**/src/main/java/**/*.java: You are acting as a Senior Java Engineer and Software Architect.
Review with a direct, practical, no-nonsense tone.You must enforce modern Java (Java 25) best practices and discourage legacy or outdated patterns.
Always prefer:
- modern language features when appropriate (var, records, switch expressions, pattern matching, sealed types, etc.)
- the java.time API over java.util.Date/Calendar
- java.util.concurrent and modern concurrency utilities over legacy synchronized collections
- NIO and modern I/O APIs where suitable
- streams and functional APIs where they clearly improve readability and safety (but avoid over-complicating simple logic)
You must explicitly:
- Call out usage of old/legacy or deprecated APIs, raw types, Vector, Hashtable, synchronized collections, old I/O where NIO is better, and other outdated patterns.
- Highlight any code that is not aligned with current Java 25 idioms.
- Enforce
importhygiene: avoid star imports, unused imports, and messy import sections.Handle NullPointerException (NPE) risks very carefully:
- Identify every place where NPE can occur (parameters, return values, map lookups, collection elements, optional values, chained calls, etc.).
- Recommend explicit null checks, Objects.requireNonNull, Optional, or nullability annotations where appropriate.
- Call out dangerous chaining on potentially null values.
- Prefer designs that minimize nullable state and unexpected nulls.
You must review naming quality:
- Check that class, method, variable, and field names are clear, descriptive, and follow Java conventions (CamelCase for classes, lowerCamelCase for fields/variables/methods, UPPER_SNAKE_CASE for constants).
- Flag vague, misleading, or overly generic names (e.g. data, obj, tmp, doStuff, handle, manager, util).
- Suggest better, more meaningful names that reflect the intent and domain.
- Prefer names that express behavior and domain concepts over implementation details.
Enforce...
Files:
modules/console/src/main/java/com/workastra/console/package-info.javamodules/authentication/src/main/java/com/workastra/authentication/controller/JwtTokenService.javamodules/worker/src/main/java/com/workastra/worker/package-info.javamodules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.javamodules/authentication/src/main/java/com/workastra/authentication/configuration/JwtConfiguration.javamodules/authentication/src/main/java/com/workastra/authentication/controller/AuthController.javamodules/console/src/main/java/com/workastra/console/WorkastraConsoleApplication.javamodules/authentication/src/main/java/com/workastra/authentication/configuration/package-info.javamodules/authentication/src/main/java/com/workastra/authentication/controller/package-info.javamodules/authentication/src/main/java/com/workastra/authentication/package-info.java
🧠 Learnings (1)
📚 Learning: 2025-11-23T02:13:19.027Z
Learnt from: phongphongg
Repo: workastra/server PR: 2
File: gradle/libs.versions.toml:1-10
Timestamp: 2025-11-23T02:13:19.027Z
Learning: Spring Boot 4.0.0 is compatible with org.graalvm.buildtools.native plugin version 0.11.3, which is the recommended version for Spring Boot 4 native image builds.
Applied to files:
modules/console/build.gradle.ktsgradle/libs.versions.toml
🧬 Code graph analysis (1)
modules/console/src/main/java/com/workastra/console/WorkastraConsoleApplication.java (1)
modules/worker/src/main/java/com/workastra/worker/WorkastraWorkerApplication.java (1)
SpringBootApplication(6-12)
🔇 Additional comments (7)
config/checkstyle/checkstyle.xml (1)
143-147: Verify strict @bean access control aligns with project architecture.The XPath correctly flags @bean methods with any modifier beyond the annotation (public, protected, private, static, etc.). However, this enforces package-private access for ALL @bean methods, which deviates from standard Spring Boot patterns where @bean methods are typically public within @configuration classes.
Confirm:
- Is forbidding public @bean methods intentional?
- Will existing code need significant refactoring?
- Does this apply project-wide or only to specific authentication/security modules?
.vscode/launch.json (1)
10-11: Perfect! I now have all the information needed. The verification reveals that:
Java version: The project explicitly targets Java 25 (
JavaLanguageVersion.of(25)), which fully supportsUseCompactObjectHeaders(available since Java 17 via JEP 400).application-dev.yaml: The file definitively exists at
modules/console/src/main/resources/application-dev.yaml.Spring profile configuration: The dev profile is properly configured with no conflicts detected.
The original review comment raises concerns that are not supported by the actual codebase.
The launch configuration is correct. Java 25 fully supports UseCompactObjectHeaders (available since Java 17), and application-dev.yaml exists at modules/console/src/main/resources/application-dev.yaml.
The dev profile is properly configured and the environment variable setting is appropriate for local development.
Likely an incorrect or invalid review comment.
modules/console/src/main/resources/application.yaml (1)
9-12: Confirm expected type forspring.mvc.apiversion.use.path-segment.If this property is boolean-like, prefer
true/falseover1/0. Please double-check the library’s expected type and adjust if needed.settings.gradle.kts (1)
4-8: Authentication module inclusion looks correct.Including
"modules:authentication"alongsidecommon,console, andworkeris consistent and should wire the new module into the build as expected.build.gradle.kts (1)
1-62: Error Prone / NullAway integration looks consistentThe
net.ltgt.errorproneplugin,errorproneconfiguration, andJavaCompileoptions are wired correctly for subprojects with sensible defaults (generated-code exclusion, NullAway OnlyNullMarked/JSpecify options). No changes needed here.modules/authentication/build.gradle.kts (1)
1-13: Authentication module Gradle setup is reasonableUsing
java-librarywith the Spring BOM and security-related starters (plus tests) is appropriate for this module. No changes needed based on this diff.modules/authentication/src/main/java/com/workastra/authentication/controller/AuthController.java (1)
18-21: I'll help you verify and rewrite this review comment. Let me start by examining the AuthController implementation and related security configuration.
<function_calls>
Find and read the AuthController file
fd -t f "AuthController.java" --exec wc -l {} ;
</function_calls>
<function_calls>
Read the AuthController file
fd -t f "AuthController.java" --exec cat -n {} ;
</function_calls>
|
|
||
| - name: Run checks | ||
| run: ./gradlew --no-daemon clean check | ||
| run: ./gradlew --no-daemon clean check -x test |
There was a problem hiding this comment.
Redundant clean commands defeat Gradle caching and slow down CI/CD.
Each test step runs clean, which wipes the build cache and forces a full rebuild. Since the check step (line 49) already builds the project, the subsequent test steps should reuse that build state instead of cleaning and rebuilding from scratch. This is especially costly for native compilation.
Apply this diff to remove redundant cleans:
- name: Run checks
- run: ./gradlew --no-daemon clean check -x test
+ run: ./gradlew --no-daemon check -x test
- name: Run JVM tests
env:
SPRING_PROFILES_ACTIVE: test
- run: ./gradlew --no-daemon clean test
+ run: ./gradlew --no-daemon test
- name: Run native tests
env:
SPRING_PROFILES_ACTIVE: test
- run: ./gradlew --no-daemon clean nativeTest
+ run: ./gradlew --no-daemon nativeTestAlso applies to: 54-54, 59-59
🤖 Prompt for AI Agents
In .github/workflows/workastra_server.yaml around lines 49, 54 and 59, the run
commands include a redundant clean that defeats Gradle caching and forces full
rebuilds; remove the literal "clean" token from those run lines so they read
e.g. "./gradlew --no-daemon check -x test" (or the equivalent test invocation
without "clean"), preserving other flags like --no-daemon and -x test as
appropriate so subsequent steps reuse the previously built state.
| @Bean | ||
| SecretKey jwtSecretKey(JwtConfiguration properties) { | ||
| Assert.notNull(properties, "JWT properties must not be null"); | ||
| Assert.notNull(properties.secret(), "JWT secret must not be null"); | ||
| return new SecretKeySpec(properties.secret().getBytes(StandardCharsets.UTF_8), "HmacSHA256"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the JwtConfiguration and related files
find . -type f -name "*JwtConfiguration*" -o -name "*JwtTokenService*" | head -20Repository: workastra/server
Length of output: 176
🏁 Script executed:
#!/bin/bash
# List the authentication module structure
find modules/authentication -type f -name "*.java" | grep -E "(Config|Jwt)" | head -30Repository: workastra/server
Length of output: 357
🏁 Script executed:
#!/bin/bash
# Read JwtConfiguration to see what properties exist
cat -n modules/authentication/src/main/java/com/workastra/authentication/configuration/JwtConfiguration.javaRepository: workastra/server
Length of output: 394
🏁 Script executed:
#!/bin/bash
# Read JwtTokenService to see how issuer/ttl are used
cat -n modules/authentication/src/main/java/com/workastra/authentication/controller/JwtTokenService.javaRepository: workastra/server
Length of output: 2216
🏁 Script executed:
#!/bin/bash
# Read full AuthenticationModuleConfig to see context
cat -n modules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.javaRepository: workastra/server
Length of output: 4442
🏁 Script executed:
#!/bin/bash
# Search for any usage of jwtConfiguration.ttl()
rg -n "\.ttl\(\)" modules/authentication/Repository: workastra/server
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for "jwtConfiguration" usage across the codebase
rg -n "jwtConfiguration" modules/authentication/ -A 2 -B 2Repository: workastra/server
Length of output: 1921
Perfect. I now have enough information to verify and rewrite the review comment. Here are my findings:
Key findings:
issuerIS used: Line 38 of JwtTokenService callsthis.jwtConfiguration.issuer()in the JWT claims builder. If null, this will cause an issue.ttlis NOT used: The search returned no matches. The token expiration is hardcoded to 1 hour (line 29 of JwtTokenService), not using thettlproperty from the config.- Validation gap confirmed: The
jwtSecretKey()bean method only validatessecret, notissuer, creating a risk of runtime failures if the issuer is missing from configuration.
The original review comment is partially correct but includes an incorrect suggestion about validating ttl since that property is never actually used. I need to correct this.
Validate the required JWT issuer property, not unused fields
jwtSecretKey(JwtConfiguration properties) only asserts properties and properties.secret() are non-null. However, issuer is required for correct token generation (used in JwtTokenService line 38) and will cause issues if missing. Fail fast at startup by adding:
Assert.notNull(properties, "JWT properties must not be null");
Assert.notNull(properties.secret(), "JWT secret must not be null");
+ Assert.hasText(properties.issuer(), "JWT issuer must not be null or blank");
return new SecretKeySpec(properties.secret().getBytes(StandardCharsets.UTF_8), "HmacSHA256");Note: Do not validate ttl—it is not used in token generation and would be unnecessary validation on dead config.
🤖 Prompt for AI Agents
In
modules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.java
around lines 33 to 38, the jwtSecretKey bean currently validates only properties
and properties.secret(), but JwtTokenService uses properties.issuer() and will
fail at runtime if issuer is missing; update the bean to also assert that
properties.issuer() is not null (e.g., Assert.notNull(properties.issuer(), "JWT
issuer must not be null")) so the application fails fast on startup when issuer
is absent; do not add validation for ttl since it is not used.
| @Bean | ||
| SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { | ||
| http | ||
| .csrf((csrf) -> csrf.spa()) | ||
| .authorizeHttpRequests((auth) -> | ||
| auth | ||
| .requestMatchers(HttpMethod.GET, "/actuator/health") | ||
| .permitAll() | ||
| .requestMatchers("/api/v1/csrf") | ||
| .permitAll() | ||
| .requestMatchers("/api/v1/login") | ||
| .permitAll() | ||
| .requestMatchers("/api/**") | ||
| .authenticated() | ||
| .anyRequest() | ||
| .denyAll() | ||
| ) | ||
| .sessionManagement((session) -> session.sessionCreationPolicy(SessionCreationPolicy.IF_REQUIRED)) | ||
| .formLogin((form) -> form.disable()); | ||
|
|
||
| return http.build(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the authentication module configuration file
find . -type f -name "AuthenticationModuleConfig.java" | head -5Repository: workastra/server
Length of output: 159
🏁 Script executed:
#!/bin/bash
# Read the full AuthenticationModuleConfig file
wc -l ./modules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.javaRepository: workastra/server
Length of output: 162
🏁 Script executed:
#!/bin/bash
# Read the full file to understand complete JWT and session configuration
cat -n ./modules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.javaRepository: workastra/server
Length of output: 4442
🏁 Script executed:
#!/bin/bash
# Find AuthController to see how authentication tokens are handled
find . -type f -name "AuthController.java" | head -5Repository: workastra/server
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Read AuthController to understand token creation and session handling
cat -n ./modules/authentication/src/main/java/com/workastra/authentication/controller/AuthController.javaRepository: workastra/server
Length of output: 2368
🏁 Script executed:
#!/bin/bash
# Find JwtTokenService
find . -type f -name "JwtTokenService.java" | head -5Repository: workastra/server
Length of output: 159
🏁 Script executed:
#!/bin/bash
# Read JwtTokenService
cat -n ./modules/authentication/src/main/java/com/workastra/authentication/controller/JwtTokenService.javaRepository: workastra/server
Length of output: 2216
🏁 Script executed:
#!/bin/bash
# Search for oauth2ResourceServer configuration anywhere in the project
rg -n "oauth2ResourceServer" --type javaRepository: workastra/server
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for JWT filters, authentication filters, or any filter implementations
rg -n "Filter|filter" --type java | grep -i jwtRepository: workastra/server
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for other Spring Security configuration classes
find . -type f -name "*Config.java" -o -name "*Security*.java" | grep -v test | grep -v targetRepository: workastra/server
Length of output: 159
🏁 Script executed:
#!/bin/bash
# Search for README or documentation about authentication
find . -type f \( -name "README*" -o -name "*.md" \) | xargs grep -l -i "auth\|jwt\|token" 2>/dev/null | head -5Repository: workastra/server
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for test files related to authentication or JWT
find . -type f -name "*Test.java" -o -name "*Tests.java" | xargs grep -l "jwt\|token\|login" -i 2>/dev/null | head -10Repository: workastra/server
Length of output: 42
Configure SecurityFilterChain to use JWT resource server authentication for stateless API requests
The SecurityFilterChain config issues a JwtDecoder bean (lines 45–48) but never wires it into the security chain. Instead:
SessionCreationPolicy.IF_REQUIRED(line 67) allows sessions to be created on-demand- No
oauth2ResourceServer().jwt(...)call to validate JWT tokens on incoming requests AuthControllerexplicitly callshttpRequest.getSession(true)(line 39) after login, forcing session creation
Result: JWT tokens are generated and returned to clients, but /api/** requests are authenticated via session, not JWT validation. The JwtDecoder is unused.
To fix, choose one approach:
Option 1 (Recommended for REST APIs): Switch to stateless JWT authentication:
- .sessionManagement((session) -> session.sessionCreationPolicy(SessionCreationPolicy.IF_REQUIRED))
- .formLogin((form) -> form.disable());
+ .sessionManagement((session) -> session.sessionCreationPolicy(SessionCreationPolicy.STATELESS))
+ .formLogin((form) -> form.disable())
+ .oauth2ResourceServer(oauth2 -> oauth2.jwt(Customizer.withDefaults()));Add import: org.springframework.security.config.Customizer
Remove the explicit session creation from AuthController (line 39): httpRequest.getSession(true);
Option 2: If session-based auth is intentional, remove the unused JwtDecoder, JwtEncoder, and JwtTokenService beans and simplify AuthController.
The current hybrid setup is inconsistent and confusing.
📝 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.
| @Bean | |
| SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { | |
| http | |
| .csrf((csrf) -> csrf.spa()) | |
| .authorizeHttpRequests((auth) -> | |
| auth | |
| .requestMatchers(HttpMethod.GET, "/actuator/health") | |
| .permitAll() | |
| .requestMatchers("/api/v1/csrf") | |
| .permitAll() | |
| .requestMatchers("/api/v1/login") | |
| .permitAll() | |
| .requestMatchers("/api/**") | |
| .authenticated() | |
| .anyRequest() | |
| .denyAll() | |
| ) | |
| .sessionManagement((session) -> session.sessionCreationPolicy(SessionCreationPolicy.IF_REQUIRED)) | |
| .formLogin((form) -> form.disable()); | |
| return http.build(); | |
| } | |
| @Bean | |
| SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { | |
| http | |
| .csrf((csrf) -> csrf.spa()) | |
| .authorizeHttpRequests((auth) -> | |
| auth | |
| .requestMatchers(HttpMethod.GET, "/actuator/health") | |
| .permitAll() | |
| .requestMatchers("/api/v1/csrf") | |
| .permitAll() | |
| .requestMatchers("/api/v1/login") | |
| .permitAll() | |
| .requestMatchers("/api/**") | |
| .authenticated() | |
| .anyRequest() | |
| .denyAll() | |
| ) | |
| .sessionManagement((session) -> session.sessionCreationPolicy(SessionCreationPolicy.STATELESS)) | |
| .formLogin((form) -> form.disable()) | |
| .oauth2ResourceServer(oauth2 -> oauth2.jwt(Customizer.withDefaults())); | |
| return http.build(); | |
| } |
🤖 Prompt for AI Agents
In
modules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.java
around lines 50 to 71, the SecurityFilterChain config issues a JwtDecoder bean
but never wires it into the chain and still allows sessions; change the chain to
use oauth2ResourceServer().jwt(...) (e.g., oauth2ResourceServer((o) ->
o.jwt(Customizer.withDefaults()))) and set sessionManagement to
SessionCreationPolicy.STATELESS to enforce JWT-based stateless auth; add the
import org.springframework.security.config.Customizer; then remove the explicit
session creation call in AuthController (httpRequest.getSession(true)) so
requests authenticate via JWT; alternatively, if you prefer session-based auth,
delete the JwtEncoder/JwtDecoder/JwtTokenService beans and adjust the controller
and security policy to consistently use sessions.
| @ConfigurationProperties(prefix = "workastra.jwt") | ||
| public record JwtConfiguration(String secret, String issuer, Duration ttl) {} |
There was a problem hiding this comment.
ttl property is defined but ignored by token generation
JwtConfiguration exposes a Duration ttl, but JwtTokenService.generateToken currently hard-codes Duration.ofHours(1) for expiry. This makes workastra.jwt.ttl a dead configuration knob and will confuse anyone trying to tune token lifetime.
Either:
- use
jwtConfiguration.ttl()when computingexpiresAt, or - drop
ttlfrom this record if you don’t plan to support configurable TTL.
I recommend wiring ttl into JwtTokenService so configuration is the single source of truth.
🤖 Prompt for AI Agents
In
modules/authentication/src/main/java/com/workastra/authentication/configuration/JwtConfiguration.java
around lines 6 to 7, the ttl Duration field is declared but not used; update
JwtTokenService.generateToken to derive token expiry from jwtConfiguration.ttl()
rather than hard-coding Duration.ofHours(1) (or, alternatively, remove ttl from
the record if you intentionally want a fixed TTL). Inject or pass
JwtConfiguration into JwtTokenService and replace the hard-coded expiry
calculation with one that uses jwtConfiguration.ttl(), ensuring null/default
handling if ttl is absent.
| @PostMapping(path = "/api/v{version}/login", version = "1") | ||
| public ResponseEntity<TokenResponse> postMethodName(@RequestBody LoginRequest request) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if the repository exists and find the AuthController file
find . -type f -name "AuthController.java" 2>/dev/null | head -5Repository: workastra/server
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Search for Spring Framework documentation or usage patterns
# Check what version of Spring is used in the project
find . -type f \( -name "pom.xml" -o -name "build.gradle" -o -name "build.gradle.kts" \) 2>/dev/null | head -3Repository: workastra/server
Length of output: 162
🏁 Script executed:
#!/bin/bash
# Read the AuthController file to see the exact lines
cat -n ./modules/authentication/src/main/java/com/workastra/authentication/controller/AuthController.java | head -50Repository: workastra/server
Length of output: 2368
🏁 Script executed:
#!/bin/bash
# Check for build.gradle.kts in authentication module and root
find ./modules/authentication -type f -name "build.gradle.kts" 2>/dev/null
ls -la ./build.gradle.kts 2>/dev/null || echo "No root build.gradle.kts"Repository: workastra/server
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Check Spring version and dependencies in build.gradle.kts
cat ./modules/authentication/build.gradle.ktsRepository: workastra/server
Length of output: 593
🌐 Web query:
Spring Framework @PostMapping annotation supported attributes
💡 Result:
@PostMapping supports the same attributes as a composed @RequestMapping. The elements are:
- name
- value
- path
- params
- headers
- consumes
- produces
Each is an alias for the corresponding RequestMapping attribute. [1]
Sources:
[1] Spring Framework @PostMapping Javadoc (elements: name, value, path, params, headers, consumes, produces).
🏁 Script executed:
#!/bin/bash
# Search for error handling (ControllerAdvice, ExceptionHandler)
find ./modules/authentication -type f -name "*.java" | xargs grep -l "ControllerAdvice\|ExceptionHandler" 2>/dev/nullRepository: workastra/server
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check for validation annotations on LoginRequest or similar patterns
rg -n "@NotNull|@NotBlank|@Valid" ./modules/authentication/src/main/java/com/workastra/authentication/controller/ 2>/dev/null | head -20Repository: workastra/server
Length of output: 42
Based on my verification, the original review comment is correct and identifies a critical issue. The version attribute on @PostMapping is definitively unsupported—Spring's @PostMapping only supports: name, value, path, params, headers, consumes, and produces.
I've also identified additional issues beyond the original comment:
-
Line 39 creates an HTTP session (
httpRequest.getSession(true)) while line 41 returns a JWT token. This is an architectural contradiction—JWT is stateless, so session creation should be removed. -
Missing error handling: There's no try-catch or
ControllerAdviceto handle authentication failures. TheauthenticationManager.authenticate()call will throw exceptions that are unhandled. -
Missing validation:
LoginRequestfields (username,password) have no validation annotations.
Fix @PostMapping attributes, method name, remove conflicting session creation, and add error handling
@PostMapping does not have a version attribute; the current usage won't compile. Additionally, creating an HTTP session (line 39) contradicts JWT token usage (line 41)—JWT is stateless, so remove the session creation. Add error handling for authentication failures.
- @PostMapping(path = "/api/v{version}/login", version = "1")
- public ResponseEntity<TokenResponse> postMethodName(@RequestBody LoginRequest request) {
+ @PostMapping("/api/v1/login")
+ public ResponseEntity<TokenResponse> login(@RequestBody LoginRequest request) {
Authentication authentication = authenticationManager.authenticate(
new UsernamePasswordAuthenticationToken(request.username(), request.password())
);
SecurityContextHolder.getContext().setAuthentication(authentication);
- HttpServletRequest httpRequest =
- ((ServletRequestAttributes) RequestContextHolder.currentRequestAttributes()).getRequest();
- httpRequest.getSession(true);
String token = jwtTokenService.generateToken(authentication);
return ResponseEntity.ok(new TokenResponse(token));Remove the session creation (lines 37–39) since JWT is stateless. Also add a ControllerAdvice handler for BadCredentialsException or similar to provide meaningful error responses instead of letting exceptions propagate.
📝 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.
| @PostMapping(path = "/api/v{version}/login", version = "1") | |
| public ResponseEntity<TokenResponse> postMethodName(@RequestBody LoginRequest request) { | |
| @PostMapping("/api/v1/login") | |
| public ResponseEntity<TokenResponse> login(@RequestBody LoginRequest request) { | |
| Authentication authentication = authenticationManager.authenticate( | |
| new UsernamePasswordAuthenticationToken(request.username(), request.password()) | |
| ); | |
| SecurityContextHolder.getContext().setAuthentication(authentication); | |
| String token = jwtTokenService.generateToken(authentication); | |
| return ResponseEntity.ok(new TokenResponse(token)); | |
| } |
🤖 Prompt for AI Agents
In
modules/authentication/src/main/java/com/workastra/authentication/controller/AuthController.java
around lines 30–41, update the @PostMapping to remove the unsupported version
attribute (use path/value like "/api/v1/login" or param-based versioning),
rename the handler from postMethodName to a meaningful name such as login,
remove the httpRequest.getSession(true) session creation (JWT is stateless),
wrap authenticationManager.authenticate(...) in a try-catch to return an
appropriate ResponseEntity on authentication failure (or rethrow a custom
exception handled by ControllerAdvice), and add validation annotations to
LoginRequest fields (e.g., @NotBlank) so incoming payloads are validated.
| public String generateToken(Authentication authentication) { | ||
| Instant now = Instant.now(); | ||
| Instant expiresAt = now.plus(Duration.ofHours(1)); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and read JwtTokenService.java
fd -t f "JwtTokenService.java" --exec cat -n {} \;Repository: workastra/server
Length of output: 2216
🏁 Script executed:
#!/bin/bash
# Find and read JwtConfiguration.java
fd -t f "JwtConfiguration.java" --exec cat -n {} \;Repository: workastra/server
Length of output: 394
Replace hard-coded 1-hour token expiry with configured TTL
Line 29 currently bypasses the configured TTL:
Instant expiresAt = now.plus(Duration.ofHours(1));The JwtConfiguration record exposes ttl() as a field from application configuration (workastra.jwt.ttl), but this is not being used. The hard-coded duration makes the configuration property ineffective.
Use the configured TTL instead:
- Instant expiresAt = now.plus(Duration.ofHours(1));
+ Instant expiresAt = now.plus(jwtConfiguration.ttl());This keeps token expiry fully configurable and removes the magic constant.
🤖 Prompt for AI Agents
In
modules/authentication/src/main/java/com/workastra/authentication/controller/JwtTokenService.java
around lines 27 to 30, the token expiry is hard-coded to one hour; replace that
with the configured TTL from JwtConfiguration. Use the injected
JwtConfiguration.ttl() value (convert to a Duration if the record stores a
numeric TTL) when computing expiresAt (e.g., now.plus(jwtConfiguration.ttl()) or
now.plus(Duration.ofSeconds(jwtConfiguration.ttl())) depending on type),
ensuring the service uses the application property instead of the magic
constant.
| @NullMarked | ||
| package com.workastra.authentication; | ||
|
|
||
| import org.jspecify.annotations.NullMarked; |
There was a problem hiding this comment.
Import must come before package declaration.
The import is placed after the package declaration. In package-info.java, imports must come first, then the annotated package statement. This code won't compile because @NullMarked is used before it's imported.
Apply this diff:
-@NullMarked
-package com.workastra.authentication;
-
import org.jspecify.annotations.NullMarked;
+
+@NullMarked
+package com.workastra.authentication;📝 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.
| @NullMarked | |
| package com.workastra.authentication; | |
| import org.jspecify.annotations.NullMarked; | |
| import org.jspecify.annotations.NullMarked; | |
| @NullMarked | |
| package com.workastra.authentication; |
🤖 Prompt for AI Agents
In
modules/authentication/src/main/java/com/workastra/authentication/package-info.java
around lines 1 to 4, the @NullMarked annotation is applied before the import
statement, causing a compile error; move the import for
org.jspecify.annotations.NullMarked above the package declaration so imports
come first, then the package annotation and declaration, ensuring the file has
the import line(s) first followed by @NullMarked and package
com.workastra.authentication; no other changes required.
| { | ||
| "properties": [ | ||
| { | ||
| "name": "workastra.jwt.issuer", | ||
| "type": "java.lang.String", | ||
| "description": "The issuer claim value for JWT tokens" | ||
| }, | ||
| { | ||
| "name": "workastra.jwt.secret", | ||
| "type": "java.lang.String", | ||
| "description": "The secret key used to sign and verify JWT tokens" | ||
| }, | ||
| { | ||
| "name": "workastra.jwt.ttl", | ||
| "type": "java.lang.Integer", | ||
| "description": "The JWT token expiration time (in seconds or duration format)" | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for JwtConfiguration class
fd -e java -e kt | xargs rg -l "class JwtConfiguration" --type java --type kotlinRepository: workastra/server
Length of output: 44
🏁 Script executed:
# Search for the JWT properties defined in the codebase
rg -n "workastra\.jwt\." --type java --type kotlin -A 2 -B 2Repository: workastra/server
Length of output: 42
🏁 Script executed:
# Check for Spring Boot configuration processor setup in authentication module
fd -name "pom.xml" -o -name "build.gradle" | xargs rg -l "spring-boot-configuration-processor"Repository: workastra/server
Length of output: 340
🏁 Script executed:
# Check what's in the console module's metadata file and related structure
fd "additional-spring-configuration-metadata.json"Repository: workastra/server
Length of output: 149
🏁 Script executed:
# List the modules directory structure
ls -la modules/Repository: workastra/server
Length of output: 397
🏁 Script executed:
# Find all Java/Kotlin files with proper fd syntax
fd --type f -e java | head -20Repository: workastra/server
Length of output: 1193
🏁 Script executed:
# Search for JWT-related class definitions with simpler pattern
rg "class.*Jwt" --type java --type kotlinRepository: workastra/server
Length of output: 188
🏁 Script executed:
# Check the console build.gradle.kts to understand dependencies and processor config
cat modules/console/build.gradle.ktsRepository: workastra/server
Length of output: 1206
🏁 Script executed:
# View the JwtConfiguration class
cat modules/authentication/src/main/java/com/workastra/authentication/configuration/JwtConfiguration.javaRepository: workastra/server
Length of output: 345
🏁 Script executed:
# Check authentication module's build.gradle.kts
cat modules/authentication/build.gradle.ktsRepository: workastra/server
Length of output: 593
🏁 Script executed:
# Also check if there's any metadata file in authentication module
fd "additional-spring-configuration-metadata.json" modules/authentication/Repository: workastra/server
Length of output: 42
Perfect! I now have all the information needed to verify and rewrite the review comment.
Fix critical type mismatch in JWT metadata and configure annotation processor in the authentication module.
The metadata contains two significant issues:
-
Critical:
workastra.jwt.ttltype is incorrect
The metadata declarestype: "java.lang.Integer", but the actualJwtConfigurationrecord definesttlasjava.time.Duration:@ConfigurationProperties(prefix = "workastra.jwt") public record JwtConfiguration(String secret, String issuer, Duration ttl) {}
The metadata must be corrected to:
{ "name": "workastra.jwt.ttl", "type": "java.time.Duration", "description": "The JWT token expiration time" } -
Processor configuration in wrong module
Thespring-boot-configuration-processoris currently in the console module'sbuild.gradle.kts, butJwtConfigurationlives in the authentication module. The processor should be added to the authentication module so metadata is automatically generated at build time rather than maintained manually in console. This prevents drift between the actual properties and their metadata declarations.
🤖 Prompt for AI Agents
In
modules/console/src/main/resources/META-INF/additional-spring-configuration-metadata.json
lines 1-19: update the JWT metadata and move the processor - the TTL property is
incorrectly typed as java.lang.Integer but JwtConfiguration.ttl is
java.time.Duration so change the metadata entry type to java.time.Duration and
simplify its description to reflect duration semantics, and remove/manual
maintenance by enabling the spring-boot-configuration-processor in the
authentication module (where JwtConfiguration is defined) as an
annotationProcessor dependency so the configuration metadata is generated
automatically at build time rather than kept in the console module.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
modules/common/src/main/java/com/workastra/common/api/Pagination.java (1)
3-8: Clarify pagination semantics (0- vs 1-based and invariants).The record is fine structurally, but it’s unclear whether
page/totalPagesare 0- or 1-based and whether negative values are allowed. Consider documenting this in Javadoc and/or validating these fields at creation time to avoid subtle off‑by‑one bugs downstream.modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAccessDeniedHandler.java (1)
21-39: Add explicit UTF‑8 encoding (and optionally logging) to the 403 response.Handler logic is correct, but the response relies on the container’s default charset. For JSON APIs, it’s safer to set UTF‑8 explicitly and (optionally) log the denial for diagnostics.
-import java.io.IOException; - -import org.springframework.http.MediaType; +import java.io.IOException; +import java.nio.charset.StandardCharsets; + +import org.springframework.http.MediaType; @@ - response.setStatus(HttpServletResponse.SC_FORBIDDEN); - response.setContentType(MediaType.APPLICATION_JSON_VALUE); + response.setStatus(HttpServletResponse.SC_FORBIDDEN); + response.setContentType(MediaType.APPLICATION_JSON_VALUE); + response.setCharacterEncoding(StandardCharsets.UTF_8.name()); response.getWriter().write(this.mapper.writeValueAsString(ApiResponse.error( "FORBIDDEN", "You do not have permission to access this resource.", Meta.fromCurrentRequest())));You can also consider logging the
accessDeniedExceptionat WARN for traceability.modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthenticationEntryPoint.java (1)
21-42: Add UTF‑8 charset and consider extracting shared error-response helper.The 401 response is correct and consistent with the other handlers, but again there’s no explicit charset and the error-writing pattern is duplicated across handlers.
You can harden and de-duplicate this as follows:
-import java.io.IOException; - -import org.springframework.http.MediaType; +import java.io.IOException; +import java.nio.charset.StandardCharsets; + +import org.springframework.http.MediaType; @@ response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); response.setContentType(MediaType.APPLICATION_JSON_VALUE); + response.setCharacterEncoding(StandardCharsets.UTF_8.name()); @@ - ApiResponse<Void> body = ApiResponse.error( + ApiResponse<Void> body = ApiResponse.error( "UNAUTHORIZED", "Authentication is required to access this resource.", Meta.fromCurrentRequest() );Longer term, consider a small utility (or base class) that takes
(response, status, code, message)and writes theApiResponse.error(...)JSON so JsonAuthenticationEntryPoint and JsonAccessDeniedHandler don’t duplicate this boilerplate.modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthSuccessHandler.java (1)
10-41: Align success payload with global ApiResponse and verify ObjectMapper importTwo things to tighten up here:
Response shape consistency
Other endpoints and handlers useApiResponseas a standard envelope, while this handler returns a bare{success, username}map. That forces clients to special‑case login responses. Consider something like:record LoginSuccess(String username) {} // ... var body = ApiResponse.ok(new LoginSuccess(authentication.getName())); response.getWriter().write(objectMapper.writeValueAsString(body));This keeps the JSON schema uniform and leverages
Meta.fromCurrentRequest().Charset + ObjectMapper package
- You only set
Content-Typewithout an explicit charset; for JSON APIs it’s safer to set UTF‑8 explicitly (or include;charset=UTF-8in the content type) so the writer and Jackson agree on encoding.- The import is
tools.jackson.databind.ObjectMapper, which is non‑standard; normally this iscom.fasterxml.jackson.databind.ObjectMapper. Iftools.jacksonisn’t an intentionally shaded package in this project, switch to the standard Jackson package or fix the dependency so this compiles.modules/authentication/src/main/java/com/workastra/authentication/infrastructure/filter/package-info.java (1)
1-9: Add@NullMarkedto this package to enforce non‑null by defaultOther modules are using JSpecify’s
@NullMarkedat the package level. This package is missing it, so all the filters here default back to “unknown nullability”.Add:
@NullMarked package com.workastra.authentication.infrastructure.filter; import org.jspecify.annotations.NullMarked;to make non‑null the default and keep null‑safety consistent across the module.
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/SecurityConfig.java (1)
19-86: This configuration is session‑based; confirm that’s intentional and verify Jackson wiringThe filter chain is internally consistent but a couple of points are worth double‑checking:
Session‑based vs JWT‑based auth
- You use
HttpSessionSecurityContextRepositoryandSessionCreationPolicy.IF_REQUIRED, which means authenticated sessions are stored in the HTTP session and reused via JSESSIONID cookies.- There is no
oauth2ResourceServer().jwt(...)or equivalent JWT validation configured on incoming requests.That’s perfectly valid for an SPA using cookie‑based sessions, but it’s not a stateless JWT setup. Given the PR title references JWT, confirm whether the intent is:
- Stateful sessions (current code): then this is fine, but make sure docs/expectations reflect this and remove any unused JWT pieces elsewhere; or
- Stateless JWT: then you should switch to
SessionCreationPolicy.STATELESS, dropHttpSessionSecurityContextRepository, and wireoauth2ResourceServer(oauth2 -> oauth2.jwt(...))(plus the correspondingJwtDecoder/JwtEncoderconfiguration).Don’t mix both approaches; pick one and keep it consistent.
ObjectMapper import
As in other files, the import istools.jackson.databind.ObjectMapper. If that isn’t an intentionally shaded package in your build, you’ll want to switch this and all related imports tocom.fasterxml.jackson.databind.ObjectMapperand depend on the standard Jackson artifact.modules/common/src/main/java/com/workastra/common/api/Meta.java (1)
15-73: TightenMetatyping and clean up commented pagination resolverTwo small improvements here:
Prefer
InstantoverStringfortimestamp
You already useInstant.now(). Keeping the record field asInstantand letting Jackson serialize to ISO‑8601 preserves type safety and avoids stringly‑typed time handling:
- String timestamp, // ISO-8601
- Instant timestamp, // serialized as ISO-8601 by Jackson
and in `fromCurrentRequest()`: ```diff - String timestamp = Instant.now().toString(); + Instant timestamp = Instant.now();
- Remove or complete the commented
resolvePagination
The commented‑outresolvePaginationmethod is dead code and adds noise. Either implement it fully and wire it intofromCurrentRequest()or delete it until you actually need pagination extraction.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
AGENTS.md(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/controller/AuthController.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/PasswordEncoderConfig.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/SecurityConfig.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/package-info.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/filter/JsonLoginAuthenticationFilter.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/filter/package-info.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAccessDeniedHandler.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthFailureHandler.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthSuccessHandler.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthenticationEntryPoint.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/package-info.java(1 hunks)modules/common/src/main/java/com/workastra/common/api/ApiResponse.java(1 hunks)modules/common/src/main/java/com/workastra/common/api/ErrorItem.java(1 hunks)modules/common/src/main/java/com/workastra/common/api/Meta.java(1 hunks)modules/common/src/main/java/com/workastra/common/api/Pagination.java(1 hunks)modules/common/src/main/java/com/workastra/common/api/package-info.java(1 hunks)modules/console/src/main/resources/application-dev.yaml(1 hunks)modules/console/src/main/resources/application-test.yaml(1 hunks)modules/console/src/main/resources/application.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- AGENTS.md
- modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/package-info.java
- modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/package-info.java
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/console/src/main/resources/application-dev.yaml
- modules/console/src/main/resources/application.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
**/src/main/java/**/*.java: You are acting as a Senior Java Engineer and Software Architect.
Review with a direct, practical, no-nonsense tone.You must enforce modern Java (Java 25) best practices and discourage legacy or outdated patterns.
Always prefer:
- modern language features when appropriate (var, records, switch expressions, pattern matching, sealed types, etc.)
- the java.time API over java.util.Date/Calendar
- java.util.concurrent and modern concurrency utilities over legacy synchronized collections
- NIO and modern I/O APIs where suitable
- streams and functional APIs where they clearly improve readability and safety (but avoid over-complicating simple logic)
You must explicitly:
- Call out usage of old/legacy or deprecated APIs, raw types, Vector, Hashtable, synchronized collections, old I/O where NIO is better, and other outdated patterns.
- Highlight any code that is not aligned with current Java 25 idioms.
- Enforce
importhygiene: avoid star imports, unused imports, and messy import sections.Handle NullPointerException (NPE) risks very carefully:
- Identify every place where NPE can occur (parameters, return values, map lookups, collection elements, optional values, chained calls, etc.).
- Recommend explicit null checks, Objects.requireNonNull, Optional, or nullability annotations where appropriate.
- Call out dangerous chaining on potentially null values.
- Prefer designs that minimize nullable state and unexpected nulls.
You must review naming quality:
- Check that class, method, variable, and field names are clear, descriptive, and follow Java conventions (CamelCase for classes, lowerCamelCase for fields/variables/methods, UPPER_SNAKE_CASE for constants).
- Flag vague, misleading, or overly generic names (e.g. data, obj, tmp, doStuff, handle, manager, util).
- Suggest better, more meaningful names that reflect the intent and domain.
- Prefer names that express behavior and domain concepts over implementation details.
Enforce...
Files:
modules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/SecurityConfig.javamodules/common/src/main/java/com/workastra/common/api/Pagination.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/PasswordEncoderConfig.javamodules/common/src/main/java/com/workastra/common/api/package-info.javamodules/common/src/main/java/com/workastra/common/api/ApiResponse.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/filter/package-info.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthFailureHandler.javamodules/common/src/main/java/com/workastra/common/api/Meta.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/filter/JsonLoginAuthenticationFilter.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAccessDeniedHandler.javamodules/authentication/src/main/java/com/workastra/authentication/controller/AuthController.javamodules/common/src/main/java/com/workastra/common/api/ErrorItem.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthSuccessHandler.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthenticationEntryPoint.java
🧬 Code graph analysis (3)
modules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.java (4)
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/PasswordEncoderConfig.java (1)
Configuration(21-60)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/SecurityConfig.java (1)
Configuration(26-87)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAccessDeniedHandler.java (1)
JsonAccessDeniedHandler(21-40)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthenticationEntryPoint.java (1)
JsonAuthenticationEntryPoint(21-44)
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/SecurityConfig.java (5)
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/PasswordEncoderConfig.java (1)
Configuration(21-60)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/filter/JsonLoginAuthenticationFilter.java (1)
JsonLoginAuthenticationFilter(34-64)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAccessDeniedHandler.java (1)
JsonAccessDeniedHandler(21-40)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthFailureHandler.java (1)
JsonAuthFailureHandler(19-43)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthSuccessHandler.java (1)
JsonAuthSuccessHandler(19-42)
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/PasswordEncoderConfig.java (2)
modules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.java (1)
Configuration(23-43)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/SecurityConfig.java (1)
Configuration(26-87)
🪛 GitHub Actions: Linter & Formatter
modules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.java
[error] 1-1: Code is not formatted properly. Please run Prettier locally and commit the changes.
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/SecurityConfig.java
[error] 1-1: Code is not formatted properly. Please run Prettier locally and commit the changes.
modules/common/src/main/java/com/workastra/common/api/Pagination.java
[error] 1-1: Code is not formatted properly. Please run Prettier locally and commit the changes.
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/PasswordEncoderConfig.java
[error] 1-1: Code is not formatted properly. Please run Prettier locally and commit the changes.
modules/common/src/main/java/com/workastra/common/api/ApiResponse.java
[error] 1-1: Code is not formatted properly. Please run Prettier locally and commit the changes.
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthFailureHandler.java
[error] 1-1: Code is not formatted properly. Please run Prettier locally and commit the changes.
modules/common/src/main/java/com/workastra/common/api/Meta.java
[error] 1-1: Code is not formatted properly. Please run Prettier locally and commit the changes.
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/filter/JsonLoginAuthenticationFilter.java
[error] 1-1: Code is not formatted properly. Please run Prettier locally and commit the changes.
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAccessDeniedHandler.java
[error] 1-1: Code is not formatted properly. Please run Prettier locally and commit the changes.
modules/authentication/src/main/java/com/workastra/authentication/controller/AuthController.java
[error] 1-1: Code is not formatted properly. Please run Prettier locally and commit the changes.
modules/common/src/main/java/com/workastra/common/api/ErrorItem.java
[error] 1-1: Code is not formatted properly. Please run Prettier locally and commit the changes.
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthSuccessHandler.java
[error] 1-1: Code is not formatted properly. Please run Prettier locally and commit the changes.
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthenticationEntryPoint.java
[error] 1-1: Code is not formatted properly. Please run Prettier locally and commit the changes.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Check
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (6)
modules/console/src/main/resources/application-test.yaml (1)
1-1: Configuration file is empty — clarify intent.The test profile configuration file contains only the YAML document start marker. Given that CI activates the test profile for both JVM and native tests, and the PR introduces JWT authentication, determine whether this should remain empty (tests inherit production-like config) or provide test-specific overrides (e.g., authentication bypass, test database config, logging levels).
If intentional, add a comment explaining why. If config is needed, populate it.
modules/common/src/main/java/com/workastra/common/api/ErrorItem.java (1)
3-7: ErrorItem record is fine; fix CI formatting.Record shape (field, code, message) is straightforward and idiomatic; no correctness issues. CI is failing with a Prettier “code is not formatted properly” error for this file, so run the project’s formatter (Prettier/Gradle task) before merging.
modules/common/src/main/java/com/workastra/common/api/package-info.java (1)
1-4: Package-level @NullMarked usage looks good.Package annotation + import ordering is correct and gives a clear non-null-by-default contract for this API package.
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthFailureHandler.java (1)
19-42: I'll verify the concerns raised in this review comment. Let me start by examining the codebase for the patterns mentioned.
<function_calls>
#!/bin/bashFind other authentication/security handlers to verify ApiResponse pattern
fd -t f "Handler.java$" modules/authentication/src --exec grep -l "onAuthenticationFailure|onAuthenticationSuccess" {} ;
</function_calls><function_calls>
#!/bin/bashSearch for ApiResponse usage in authentication module
rg -n "ApiResponse." modules/authentication/src --type java -C 2
</function_calls><function_calls>
#!/bin/bashFind ApiResponse and Meta classes
fd -t f "ApiResponse.java" . && fd -t f "Meta.java" .
</function_calls><function_calls>
#!/bin/bashCheck for other handlers in security-related packages
find . -path "*/handler/*Handler.java" -type f | grep -i "auth|security" | head -20
</function_calls><function_calls>
#!/bin/bashSearch for other implementations of AuthenticationFailureHandler or similar
rg -n "implements.*Handler|extends.*Handler" modules/authentication/src --type java -A 1 | head -40
</function_calls>modules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.java (1)
23-42: Handler bean wiring looks fineBean definitions for
JsonAuthenticationEntryPointandJsonAccessDeniedHandlerare straightforward and correctly match the constructor injection inSecurityConfig. No issues from my side here.modules/common/src/main/java/com/workastra/common/api/ApiResponse.java (1)
33-81: ApiResponse design is solidGeneric record shape, nullability annotations, and factory methods are all reasonable and consistent with the rest of the module. I don’t see issues that need changing right now.
| public class AuthController { | ||
| @GetMapping(path = "/api/v{version}/csrf", version = "1") | ||
| public ApiResponse<CsrfToken> getCsrf(CsrfToken csrfToken) { | ||
| return ApiResponse.ok(csrfToken); | ||
| } | ||
|
|
||
| @GetMapping(path = "/api/v{version}/auth/me", version = "1") | ||
| public ApiResponse<Authentication> me(Authentication authentication) { | ||
| return ApiResponse.ok(authentication); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List all Java files to understand the repository structure
git ls-files "*.java" | head -50Repository: workastra/server
Length of output: 2381
🏁 Script executed:
#!/bin/bash
# Read the AuthController file
cat -n modules/authentication/src/main/java/com/workastra/authentication/controller/AuthController.javaRepository: workastra/server
Length of output: 971
🏁 Script executed:
#!/bin/bash
# Check ApiResponse implementation
cat -n modules/common/src/main/java/com/workastra/common/api/ApiResponse.javaRepository: workastra/server
Length of output: 3366
🏁 Script executed:
#!/bin/bash
# Check SecurityConfig to understand path patterns
cat -n modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/SecurityConfig.javaRepository: workastra/server
Length of output: 4920
🌐 Web query:
Spring @GetMapping valid attributes parameters
💡 Result:
@GetMapping is a composed variant of @RequestMapping for HTTP GET. It supports the same mapping attributes as @RequestMapping (except you don't need to set method). Main attributes:
- value / path — URI(s) to map (aliases). [1]
- params — request parameter conditions (presence, absence, exact value, e.g. "id", "!id", "type=full"). [2]
- headers — request header conditions (e.g. "X-My-Header=abc"). [2]
- consumes — media types the handler can consume (matches Content-Type). [1]
- produces — media types the handler can produce (matches Accept). [1]
- name — logical name for the mapping (used for URI building / naming). [1]
Notes:
- You can supply arrays for value/path, params, headers, consumes, produces. [1]
- params/headers support negation and value checks (e.g. "flag", "!flag", "q=1"). [2]
Sources: Spring Framework RequestMapping javadoc and reference documentation. [1][2]
References
[1] Spring RequestMapping javadoc. (docs.spring.io)
[2] Spring reference — Mapping requests (params/headers examples). (docs.spring.io)
Citations:
- 1: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/bind/annotation/RequestMapping.html?utm_source=openai
- 2: https://docs.spring.io/spring-framework/reference/web/webmvc/mvc-controller/ann-requestmapping.html?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Check for test files or build output
find . -type f \( -name "*Test.java" -o -name "*Tests.java" \) | grep -i auth | head -10Repository: workastra/server
Length of output: 42
🌐 Web query:
Spring Security Authentication CsrfToken Jackson serialization JSON
💡 Result:
Short answer
- Use Spring Security's Jackson modules (SecurityJackson2Modules.getModules(...)) and register them with your ObjectMapper so CsrfToken and Authentication-related types serialize/deserialize correctly. [1][2]
Example (Java)
- Register modules:
ObjectMapper mapper = new ObjectMapper();
ClassLoader loader = getClass().getClassLoader();
mapper.registerModules(SecurityJackson2Modules.getModules(loader));
// then serialize e.g. mapper.writeValueAsString(csrfTokenOrSecurityContext);
Notes / gotchas
- DefaultCsrfToken implements Serializable and is supported by the web Jackson modules, so fields headerName/parameterName/token will be handled. [2]
- Spring Security historically enabled Jackson typing for some modules; be careful when enabling default typing or deserializing untrusted JSON — use the modules approach and Jackson allowlist/mixins rather than blanket default typing to avoid remote code execution risks. [1][3]
- When storing sessions as JSON (e.g., Redis via Spring Session) you may need to handle Jackson allowlist/mixins for certain value types (some issues reported around allowlist entries like java.lang.Long inside security-related objects). If you hit deserialization allowlist errors, add explicit mixins or mappings rather than re-enabling unsafe default typing. [4]
References
- Spring Security Jackson integration (register SecurityJackson2Modules). [1]
- API / DefaultCsrfToken and web Jackson modules. [2]
- Spring Security blog on serialization/typing security considerations. [3]
- GitHub issue: allowlist/deserialization edge case when persisting sessions as JSON. [4]
Fix invalid @GetMapping attributes and avoid exposing Authentication directly
There are critical issues here:
-
Invalid
versionattribute on@GetMapping(compile‑time error)
@GetMappingdoesn't have aversionattribute; supported attributes arepath/value,params,headers,consumes,produces, andname. This will not compile. The{version}path variable is also unused. SecurityConfig explicitly configures/api/v1/...paths (lines 68–70), not parameterized version paths. Fix by hard-coding v1:
- @GetMapping(path = "/api/v{version}/csrf", version = "1")
- @GetMapping(path = "/api/v1/csrf")
public ApiResponse getCsrf(CsrfToken csrfToken) {
return ApiResponse.ok(csrfToken);
}
- @GetMapping(path = "/api/v{version}/auth/me", version = "1")
- @GetMapping(path = "/api/v1/auth/me")
public ApiResponse me(Authentication authentication) {
return ApiResponse.ok(authentication);
}
2. **Leaking `Authentication` and `CsrfToken` over the wire**
Returning raw Spring Security types couples your API schema to framework internals and creates serialization fragility. `CsrfToken` and `Authentication` require special Jackson modules (SecurityJackson2Modules) to serialize correctly; without them, serialization will fail. Even if configured, exposing these types is poor API design—clients shouldn't depend on Spring Security internals.
Use focused DTOs instead:
```java
public record AuthMeResponse(String username, List<String> roles) {}
public record CsrfTokenResponse(String token, String headerName, String parameterName) {}
@GetMapping(path = "/api/v1/auth/me")
public ApiResponse<AuthMeResponse> me(Authentication authentication) {
var principal = authentication.getPrincipal();
// Map to username + roles
}
@GetMapping(path = "/api/v1/csrf")
public ApiResponse<CsrfTokenResponse> getCsrf(CsrfToken csrfToken) {
return ApiResponse.ok(new CsrfTokenResponse(csrfToken.getToken(), csrfToken.getHeaderName(), csrfToken.getParameterName()));
}
🤖 Prompt for AI Agents
In
modules/authentication/src/main/java/com/workastra/authentication/controller/AuthController.java
around lines 13-22, remove the invalid version attribute from the @GetMapping
annotations and hard-code the v1 paths (e.g. "/api/v1/csrf" and
"/api/v1/auth/me"); stop returning Spring Security types directly — create DTOs
(e.g. CsrfTokenResponse with token, headerName, parameterName and AuthMeResponse
with username and roles) and map CsrfToken and Authentication to those DTOs (use
csrfToken.getToken(), getHeaderName(), getParameterName(); for authentication
derive username from authentication.getName() or principal and collect roles
from authentication.getAuthorities()), update method signatures to return
ApiResponse<...> of the DTOs, and add necessary imports for the new DTOs and any
collection utilities.
| @Configuration | ||
| public class PasswordEncoderConfig { | ||
|
|
||
| /** | ||
| * Provides the BCryptPasswordEncoder bean for password encryption. | ||
| * BCrypt automatically handles salt generation and secure hashing. | ||
| */ | ||
| @Bean | ||
| PasswordEncoder passwordEncoder() { | ||
| return new BCryptPasswordEncoder(); | ||
| } | ||
|
|
||
| /** | ||
| * Provides the AuthenticationManager bean from the AuthenticationConfiguration. | ||
| * This manager delegates to the configured authentication providers. | ||
| */ | ||
| @Bean | ||
| AuthenticationManager authenticationManager(AuthenticationConfiguration configuration) throws Exception { | ||
| return configuration.getAuthenticationManager(); | ||
| } | ||
|
|
||
| /** | ||
| * Provides an in-memory UserDetailsService with a default admin user. | ||
| * In production, this should be replaced with a persistent user repository | ||
| * connected to a database. | ||
| * | ||
| * Default user: | ||
| * - username: "admin" | ||
| * - password: "admin" (bcrypt encoded) | ||
| * - roles: "USER" | ||
| */ | ||
| @Bean | ||
| UserDetailsService userDetailsService(PasswordEncoder passwordEncoder) { | ||
| return new InMemoryUserDetailsManager( | ||
| User.withUsername("admin") | ||
| .password(passwordEncoder.encode("admin")) | ||
| .roles("USER") | ||
| .build()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current file and surrounding context for @Profile annotations
cat -n modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/PasswordEncoderConfig.javaRepository: workastra/server
Length of output: 2818
🏁 Script executed:
#!/bin/bash
# Search for all @Profile annotations in the authentication module
rg -n '@Profile' modules/authentication/Repository: workastra/server
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for other UserDetailsService bean definitions across the codebase
rg -n 'UserDetailsService' --type java -A 3 -B 1Repository: workastra/server
Length of output: 3401
🏁 Script executed:
#!/bin/bash
# Check for Spring profile configurations in properties/yml files
fd -e 'yml' -o -e 'yaml' -o -e 'properties' | xargs grep -l 'spring.profiles' 2>/dev/null || echo "No profile configs found"Repository: workastra/server
Length of output: 218
🏁 Script executed:
#!/bin/bash
# Check the project structure to understand the context (pom.xml or build.gradle for hints)
find . -maxdepth 2 -name 'pom.xml' -o -name 'build.gradle' | head -5Repository: workastra/server
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for application configuration files
fd -type f \( -name 'application*.yml' -o -name 'application*.yaml' -o -name 'application*.properties' \)Repository: workastra/server
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Search for any @Profile annotations in the entire codebase
rg -n '@Profile' --type javaRepository: workastra/server
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if there are test configurations in src/test
find . -path '*/src/test/java/*' -name '*Config*.java' | head -20Repository: workastra/server
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look for conditional bean definitions or alternative UserDetailsService implementations
rg -n 'InMemoryUserDetailsManager' --type java -B 5 -A 10Repository: workastra/server
Length of output: 4545
Gate the in-memory admin/admin user to non-production environments.
The hard-coded admin user with password admin is not currently gated to any Spring profile and will be active in all environments (including production) if this configuration is deployed. While the code comment says "for demo purposes," there is no @Profile annotation enforcing this restriction.
Recommended fixes:
- Add
@Profile("dev")to theuserDetailsService()bean method (lines 52–59), or - Add
@Profile("!prod")to gate it out of production, or - Move the bean to a separate dev-only configuration class and gate that class with
@Profile("dev").
The PasswordEncoder and AuthenticationManager beans are fine as-is; only the in-memory user needs gating.
| /** | ||
| * JsonLoginAuthenticationFilter is a custom authentication filter that processes | ||
| * login requests containing username and password in JSON format. It extends | ||
| * the UsernamePasswordAuthenticationFilter to handle authentication via a JSON | ||
| * payload instead of traditional form data. | ||
| * | ||
| * This filter expects a POST request with a JSON body that includes the following | ||
| * fields: | ||
| * - username: The username of the user attempting to authenticate. | ||
| * - password: The password of the user attempting to authenticate. | ||
| * | ||
| * If the request method is not POST, an AuthenticationServiceException is thrown. | ||
| * Upon successful authentication, it returns an Authentication object; otherwise, | ||
| * it returns null in case of an IOException during the reading of the request input. | ||
| */ | ||
| public class JsonLoginAuthenticationFilter extends UsernamePasswordAuthenticationFilter { | ||
|
|
||
| record LoginRequest(String username, String password) { | ||
| } | ||
|
|
||
| private final ObjectMapper objectMapper; | ||
|
|
||
| public JsonLoginAuthenticationFilter(AuthenticationManager authenticationManager, ObjectMapper objectMapper) { | ||
| super(authenticationManager); | ||
| this.objectMapper = objectMapper; | ||
| } | ||
|
|
||
| @Override | ||
| public Authentication attemptAuthentication( | ||
| HttpServletRequest request, | ||
| HttpServletResponse response | ||
| ) throws AuthenticationException { | ||
| if (!request.getMethod().equals(HttpMethod.POST.name())) { | ||
| throw new AuthenticationServiceException("Authentication method not supported: " + request.getMethod()); | ||
| } | ||
|
|
||
| try { | ||
| var loginRequest = this.objectMapper.readValue(request.getInputStream(), LoginRequest.class); | ||
| var authentication = UsernamePasswordAuthenticationToken.unauthenticated(loginRequest.username(), loginRequest.password()); | ||
| setDetails(request, authentication); | ||
| return getAuthenticationManager().authenticate(authentication); | ||
| } catch (IOException e) { | ||
| throw new BadCredentialsException("Invalid login request", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the file and examine its actual content
fd -t f "JsonLoginAuthenticationFilter.java" --exec cat -n {} \;Repository: workastra/server
Length of output: 3387
🏁 Script executed:
#!/bin/bash
# Check if the file exists and get its size
fd -t f "JsonLoginAuthenticationFilter.java"Repository: workastra/server
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Search for related authentication filter patterns and validation logic
rg -n "UsernamePasswordAuthenticationFilter" -A 3 -B 3 --type javaRepository: workastra/server
Length of output: 6305
🏁 Script executed:
#!/bin/bash
# Check for existing null/blank credential validation patterns
rg -n "username.*blank\|password.*blank\|isEmpty\|isBlank" --type java -C 2 | head -100Repository: workastra/server
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check what happens when ObjectMapper deserializes missing fields into records
rg -n "objectMapper.*readValue" --type java -C 3Repository: workastra/server
Length of output: 1333
🏁 Script executed:
#!/bin/bash
# Check if UsernamePasswordAuthenticationToken constructor validates null values
rg -n "unauthenticated.*null\|requireNonNull" --type java -C 3 | head -50Repository: workastra/server
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for test files to understand expected behavior with null/empty credentials
fd -t f "JsonLoginAuthenticationFilter" | grep -i testRepository: workastra/server
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check for any tests or validation related to credentials
rg -n "LoginRequest\|JsonLoginAuthenticationFilter" --type java -A 5 -B 2 | head -150Repository: workastra/server
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check Spring Security's UsernamePasswordAuthenticationToken to see if it validates
# by looking at related authentication code in the repo
rg -n "new UsernamePasswordAuthenticationToken\|\.unauthenticated" --type java -C 2 | head -100Repository: workastra/server
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Verify Jackson's default behavior with missing fields in records
# by checking if there are any Jackson configurations that might handle nulls differently
rg -n "ObjectMapper\|DeserializationFeature\|JsonProperty" --type java -C 2 | head -80Repository: workastra/server
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check AuthenticationManager usage and downstream handling
rg -n "authenticate\(" --type java -C 2 | grep -A 2 -B 2 "null" | head -50Repository: workastra/server
Length of output: 42
Fix Javadoc mismatch and validate null/blank credentials before authentication.
Two issues confirmed:
-
Javadoc inaccuracy (lines 31–32):
The comment claims the method "returns null in case of an IOException," but line 61 throwsBadCredentialsExceptioninstead. Update the Javadoc to reflect the actual behavior. -
Missing null/blank credential validation:
When the JSON body omitsusernameorpassword, Jackson deserializes them asnull. The code passes these directly toUsernamePasswordAuthenticationToken.unauthenticated(...)without validation, allowing invalid credentials to propagate downstream. Validate and reject empty/blank credentials upfront with aBadCredentialsException.
Suggested fix:
- * If the request method is not POST, an AuthenticationServiceException is thrown.
- * Upon successful authentication, it returns an Authentication object; otherwise,
- * it returns null in case of an IOException during the reading of the request input.
+ * If the request method is not POST, an AuthenticationServiceException is thrown.
+ * Upon successful authentication, it returns an Authentication object; otherwise,
+ * it throws a BadCredentialsException if the request cannot be read or parsed.
@@
try {
var loginRequest = this.objectMapper.readValue(request.getInputStream(), LoginRequest.class);
+ if (loginRequest.username() == null || loginRequest.username().isBlank()
+ || loginRequest.password() == null || loginRequest.password().isBlank()) {
+ throw new BadCredentialsException("Username or password is empty");
+ }
var authentication = UsernamePasswordAuthenticationToken.unauthenticated(loginRequest.username(), loginRequest.password());
setDetails(request, authentication);
return getAuthenticationManager().authenticate(authentication);
} catch (IOException e) {
throw new BadCredentialsException("Invalid login request", e);
}🤖 Prompt for AI Agents
In
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/filter/JsonLoginAuthenticationFilter.java
around lines 19 to 63, update the Javadoc to remove the incorrect claim that an
IOException causes a null return and state that a BadCredentialsException is
thrown on malformed/invalid JSON; after deserializing LoginRequest, validate
that username and password are non-null and non-blank (trim both) and if either
is missing/blank throw a BadCredentialsException with a clear message before
creating the UsernamePasswordAuthenticationToken so empty credentials are
rejected early.
52e713f to
20634ab
Compare
…handlers - Introduced a new `authentication` module for handling user authentication and authorization. - Implemented JWT-based authentication with configurable issuer, secret, and expiration time. - Added custom security handlers for JSON responses on authentication success, failure, and access denial. - Updated the console module to include the new authentication module as a dependency. - Enhanced API response structure with standardized error handling and metadata. - Configured Spring Security to use custom filters for JSON login requests. - Added necessary configuration files and logging settings for development and testing environments.
phongphongg
force-pushed
the
feat/authentication
branch
from
20634ab to
47a2699
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (2)
modules/console/src/main/resources/META-INF/additional-spring-configuration-metadata.json (1)
14-16: Correct the type mismatch:workastra.jwt.ttlshould bejava.time.Duration, notjava.lang.Integer.The metadata declares
java.lang.Integer, butJwtConfigurationdefinesttlasjava.time.Duration. This type mismatch will cause Spring configuration binding to fail at runtime.Apply this fix:
{ "name": "workastra.jwt.ttl", - "type": "java.lang.Integer", - "description": "The JWT token expiration time (in seconds or duration format)" + "type": "java.time.Duration", + "description": "The JWT token expiration time" }Additionally, move
spring-boot-configuration-processorto the authentication module's build configuration whereJwtConfigurationis defined. This allows metadata to be generated automatically at build time rather than manually maintained in the console module, preventing metadata drift.modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/PasswordEncoderConfig.java (1)
52-57: Gate in-memory admin user to non-production profile (duplicate).Hardcoded
admin/adminwill be active in all environments including production. Add@Profile("dev")or@Profile("!prod")to this bean method.
🧹 Nitpick comments (9)
modules/console/src/main/java/com/workastra/console/WorkastraConsoleApplication.java (1)
6-6: Use explicit package list for component scanning clarity.Console's gradle explicitly depends on
modules:authentication, so scanning authentication beans is intentional and correct. However, scanning the entire"com.workastra"hierarchy is imprecise and creates maintenance risk:
- Any future module added under
com.workastrawill be automatically scanned without explicit review- It differs from Worker's explicit approach (
"com.workastra.worker","com.workastra.common")- Explicit lists make dependencies and intentions clear
Replace with:
-@SpringBootApplication(scanBasePackages = "com.workastra") +@SpringBootApplication(scanBasePackages = { "com.workastra.console", "com.workastra.common", "com.workastra.authentication" })modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthFailureHandler.java (1)
33-33: Consider a dedicated response record for consistency and type safety.Using
Map.of()works, but a record DTO aligns better with modern Java practices and ensures a consistent error response structure across handlers.public record AuthErrorResponse(boolean success, String error) {}Then:
-var body = Map.of("success", false, "error", ex.getMessage()); +var body = new AuthErrorResponse(false, "Authentication failed");modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthSuccessHandler.java (2)
30-30: Redundant status code.
HttpServletResponse.SC_OK(200) is already the default status. This line is unnecessary.Apply this diff:
-response.setStatus(HttpServletResponse.SC_OK); response.setContentType(MediaType.APPLICATION_JSON_VALUE);
33-35: Prefer arecordfor the response body.Using
Map.ofwith hardcoded keys is not reusable or type-safe. Define a response record for clarity and maintainability, consistent with modern Java 25 practices and the project's ApiResponse pattern.Define a record at the top of the file or in a separate DTO package:
public record AuthSuccessResponse(boolean success, String username) {}Then update the method:
-var body = Map.of("success", true, "username", authentication.getName()); - -response.getWriter().write(this.objectMapper.writeValueAsString(body)); +var username = authentication.getName(); +var body = new AuthSuccessResponse(true, username != null ? username : "unknown"); +response.getWriter().write(this.objectMapper.writeValueAsString(body));build.gradle.kts (1)
1-5: libsCatalog access is functional but unnecessarily verbose.Consider using the direct
libsreference pattern instead of manually accessing VersionCatalogsExtension. Most Gradle 8.0+ projects simply uselibs.somethingdirectly in the build script without this boilerplate.-val libsCatalog = extensions - .getByType(VersionCatalogsExtension::class.java) - .named("libs") +// Remove boilerplate; use direct libs reference in lines 59-60 and 60 belowThen update lines 59-60:
-add("errorprone", libsCatalog.findLibrary("error-prone-core").get()) -add("errorprone", libsCatalog.findLibrary("nullaway").get()) +add("errorprone", libs.errorpronecore) +add("errorprone", libs.nullaway)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/PasswordEncoderConfig.java (1)
21-22: Class name doesn't reflect its scope.
PasswordEncoderConfigdefinesPasswordEncoder,AuthenticationManager, andUserDetailsService. Consider renaming toAuthenticationBeansConfigorSecurityBeansConfigto accurately describe its responsibility.modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/SecurityConfig.java (1)
48-87: Inject handlers as beans and ensure security config is covered by tests.Two points here:
- Handler construction in config
You’re constructingJsonAuthSuccessHandlerandJsonAuthFailureHandlerdirectly insecurityFilterChain. That couples the filter to concrete implementations and makes replacement/testing harder. Prefer injecting them as beans:- SecurityFilterChain securityFilterChain( - HttpSecurity http, - AuthenticationManager authenticationManager, - ObjectMapper objectMapper - ) throws Exception { - var jsonLoginAuthenticationFilter = new JsonLoginAuthenticationFilter(authenticationManager, objectMapper); - jsonLoginAuthenticationFilter.setFilterProcessesUrl("/api/v1/auth/login"); - jsonLoginAuthenticationFilter.setAuthenticationSuccessHandler(new JsonAuthSuccessHandler(objectMapper)); - jsonLoginAuthenticationFilter.setAuthenticationFailureHandler(new JsonAuthFailureHandler(objectMapper)); + SecurityFilterChain securityFilterChain( + HttpSecurity http, + AuthenticationManager authenticationManager, + ObjectMapper objectMapper, + JsonAuthSuccessHandler successHandler, + JsonAuthFailureHandler failureHandler + ) throws Exception { + var jsonLoginAuthenticationFilter = new JsonLoginAuthenticationFilter(authenticationManager, objectMapper); + jsonLoginAuthenticationFilter.setFilterProcessesUrl("/api/v1/auth/login"); + jsonLoginAuthenticationFilter.setAuthenticationSuccessHandler(successHandler); + jsonLoginAuthenticationFilter.setAuthenticationFailureHandler(failureHandler);
Tests for the filter chain
This configuration is security‑critical (public vs. protected endpoints, JSON 401/403 behavior). If you don’t already have them, add tests using Spring Security’s test support (e.g., MockMvc/WebTestClient) to assert:
/actuator/health,/api/v1/csrf,/api/v1/auth/loginarepermitAll./api/**requires authentication and returns JSON 401/403 via your handlers.- Non‑API URLs are
denyAll.modules/common/src/main/java/com/workastra/common/api/Meta.java (1)
17-71: HardencurrentRequest()and clean up the commented pagination block.Two things to tighten up here:
Avoid
ClassCastExceptionin non-servlet contexts
currentRequest()blindly castsRequestContextHolder.getRequestAttributes()toServletRequestAttributes. In any non‑servlet context this will blow up with aClassCastExceptioninstead of your clearIllegalStateException.Prefer an explicit type check:
- private static HttpServletRequest currentRequest() { - ServletRequestAttributes attr = (ServletRequestAttributes) RequestContextHolder.getRequestAttributes(); - - if (attr == null) { - throw new IllegalStateException( - "No current HTTP request — Meta.fromCurrentRequest() called outside web context" - ); - } - - return attr.getRequest(); - } + private static HttpServletRequest currentRequest() { + var attributes = RequestContextHolder.getRequestAttributes(); + if (!(attributes instanceof ServletRequestAttributes attr)) { + throw new IllegalStateException( + "No current HTTP request — Meta.fromCurrentRequest() called outside web context" + ); + } + return attr.getRequest(); + }
- Commented-out
resolvePagination
The large commented method block at the bottom will age badly. Either implement pagination support now or replace it with a short// TODOpointing to a ticket, and remove the commented code.modules/common/src/main/java/com/workastra/common/api/ApiResponse.java (1)
31-55: Makeerrorfactories symmetric withok()and add tests for response shape.Right now
ok()/ok(data)computeMetaviaMeta.fromCurrentRequest(), buterror(...)requires callers to supplyMeta. That’s an odd asymmetry and leaksMetaconstruction details into every error path.Consider adding convenience overloads that mirror
ok()and centralizeMetacreation:public static <T> ApiResponse<T> ok() { return new ApiResponse<>(true, "OK", null, null, null, Meta.fromCurrentRequest()); } public static <T> ApiResponse<T> ok(T data) { return new ApiResponse<>(true, "OK", null, data, null, Meta.fromCurrentRequest()); } public static <T> ApiResponse<T> error(String code, String message, @Nullable List<ErrorItem> errors, Meta meta) { return new ApiResponse<>(false, code, message, null, errors, meta); } - public static <T> ApiResponse<T> error(String code, String message, Meta meta) { - return error(code, message, null, meta); - } + public static <T> ApiResponse<T> error(String code, String message, Meta meta) { + return error(code, message, null, meta); + } + + public static <T> ApiResponse<T> error(String code, String message) { + return error(code, message, null, Meta.fromCurrentRequest()); + }Also, per the project guidelines, make sure you have unit tests that:
- Verify JSON serialization omits
nullfields (@JsonInclude.NON_NULL).- Cover
ok(),ok(data), anderror(...)overloads, including meta population fromMeta.fromCurrentRequest().
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.github/workflows/workastra_server.yaml(1 hunks).vscode/launch.json(1 hunks)build.gradle.kts(3 hunks)config/checkstyle/checkstyle.xml(1 hunks)gradle/libs.versions.toml(1 hunks)modules/authentication/build.gradle.kts(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/controller/AuthController.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/controller/package-info.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/PasswordEncoderConfig.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/SecurityConfig.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/package-info.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/filter/JsonLoginAuthenticationFilter.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/filter/package-info.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAccessDeniedHandler.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthFailureHandler.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthSuccessHandler.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthenticationEntryPoint.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/package-info.java(1 hunks)modules/authentication/src/main/java/com/workastra/authentication/package-info.java(1 hunks)modules/common/build.gradle.kts(1 hunks)modules/common/src/main/java/com/workastra/common/api/ApiResponse.java(1 hunks)modules/common/src/main/java/com/workastra/common/api/ErrorItem.java(1 hunks)modules/common/src/main/java/com/workastra/common/api/Meta.java(1 hunks)modules/common/src/main/java/com/workastra/common/api/Pagination.java(1 hunks)modules/common/src/main/java/com/workastra/common/api/package-info.java(1 hunks)modules/console/build.gradle.kts(1 hunks)modules/console/src/main/java/com/workastra/console/WorkastraConsoleApplication.java(1 hunks)modules/console/src/main/java/com/workastra/console/package-info.java(1 hunks)modules/console/src/main/resources/META-INF/additional-spring-configuration-metadata.json(1 hunks)modules/console/src/main/resources/application-dev.yaml(1 hunks)modules/console/src/main/resources/application-test.yaml(1 hunks)modules/console/src/main/resources/application.yaml(1 hunks)modules/worker/src/main/java/com/workastra/worker/package-info.java(1 hunks)settings.gradle.kts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/package-info.java
🚧 Files skipped from review as they are similar to previous changes (16)
- config/checkstyle/checkstyle.xml
- modules/console/src/main/resources/application-test.yaml
- modules/authentication/src/main/java/com/workastra/authentication/controller/AuthController.java
- .github/workflows/workastra_server.yaml
- modules/console/src/main/resources/application.yaml
- modules/console/src/main/java/com/workastra/console/package-info.java
- modules/common/build.gradle.kts
- modules/worker/src/main/java/com/workastra/worker/package-info.java
- modules/console/build.gradle.kts
- modules/common/src/main/java/com/workastra/common/api/Pagination.java
- modules/common/src/main/java/com/workastra/common/api/ErrorItem.java
- modules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.java
- modules/authentication/build.gradle.kts
- modules/authentication/src/main/java/com/workastra/authentication/infrastructure/filter/JsonLoginAuthenticationFilter.java
- modules/authentication/src/main/java/com/workastra/authentication/package-info.java
- modules/authentication/src/main/java/com/workastra/authentication/infrastructure/filter/package-info.java
🧰 Additional context used
📓 Path-based instructions (3)
**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
**/*.java: Prioritize using modern Java 25 language features and APIs; consult official Java 25 release documentation
All generated code must be fully compliant with the project's checkstyle.xml; continuously review and propose refactoring for modularity and scalability
Create comprehensive Unit Tests for any new or modified code; flag insufficient test coverage and generate necessary tests
Use explicit error handling with specific exception types and Optional over broad catch(Exception e); all log statements must be structured using SLF4J/Log4j 2 with contextual information
Write code compatible with Native Image constraints (GraalVM); minimize runtime reflection and proxies
Proactively identify and replace all deprecated or outdated APIs, libraries, and code constructs with modern Java 25 alternatives
Operate under the Non-Nullable by Default principle; all type usages (parameters, returns, fields) are non-nullable unless explicitly annotated
Use the @nullable annotation exclusively to mark any type permitted to hold a null value; never use @nonnull annotation
Nullability must specify both the container and the content (e.g., List<@nullable String> for nullable content, @nullable List for nullable container)
Audit all existing legacy code and mark any variables, method parameters, or return types that accept or return null with @nullable annotation
Files:
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/PasswordEncoderConfig.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthSuccessHandler.javamodules/common/src/main/java/com/workastra/common/api/Meta.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthenticationEntryPoint.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/package-info.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAccessDeniedHandler.javamodules/authentication/src/main/java/com/workastra/authentication/controller/package-info.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/SecurityConfig.javamodules/console/src/main/java/com/workastra/console/WorkastraConsoleApplication.javamodules/common/src/main/java/com/workastra/common/api/package-info.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthFailureHandler.javamodules/common/src/main/java/com/workastra/common/api/ApiResponse.java
**/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
**/src/main/java/**/*.java: You are acting as a Senior Java Engineer and Software Architect.
Review with a direct, practical, no-nonsense tone.You must enforce modern Java (Java 25) best practices and discourage legacy or outdated patterns.
Always prefer:
- modern language features when appropriate (var, records, switch expressions, pattern matching, sealed types, etc.)
- the java.time API over java.util.Date/Calendar
- java.util.concurrent and modern concurrency utilities over legacy synchronized collections
- NIO and modern I/O APIs where suitable
- streams and functional APIs where they clearly improve readability and safety (but avoid over-complicating simple logic)
You must explicitly:
- Call out usage of old/legacy or deprecated APIs, raw types, Vector, Hashtable, synchronized collections, old I/O where NIO is better, and other outdated patterns.
- Highlight any code that is not aligned with current Java 25 idioms.
- Enforce
importhygiene: avoid star imports, unused imports, and messy import sections.Handle NullPointerException (NPE) risks very carefully:
- Identify every place where NPE can occur (parameters, return values, map lookups, collection elements, optional values, chained calls, etc.).
- Recommend explicit null checks, Objects.requireNonNull, Optional, or nullability annotations where appropriate.
- Call out dangerous chaining on potentially null values.
- Prefer designs that minimize nullable state and unexpected nulls.
You must review naming quality:
- Check that class, method, variable, and field names are clear, descriptive, and follow Java conventions (CamelCase for classes, lowerCamelCase for fields/variables/methods, UPPER_SNAKE_CASE for constants).
- Flag vague, misleading, or overly generic names (e.g. data, obj, tmp, doStuff, handle, manager, util).
- Suggest better, more meaningful names that reflect the intent and domain.
- Prefer names that express behavior and domain concepts over implementation details.
Enforce...
Files:
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/PasswordEncoderConfig.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthSuccessHandler.javamodules/common/src/main/java/com/workastra/common/api/Meta.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthenticationEntryPoint.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/package-info.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAccessDeniedHandler.javamodules/authentication/src/main/java/com/workastra/authentication/controller/package-info.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/SecurityConfig.javamodules/console/src/main/java/com/workastra/console/WorkastraConsoleApplication.javamodules/common/src/main/java/com/workastra/common/api/package-info.javamodules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthFailureHandler.javamodules/common/src/main/java/com/workastra/common/api/ApiResponse.java
**/package-info.java
📄 CodeRabbit inference engine (AGENTS.md)
For every package, create a package-info.java file applying the @NullMarked annotation at package scope
Files:
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/package-info.javamodules/authentication/src/main/java/com/workastra/authentication/controller/package-info.javamodules/common/src/main/java/com/workastra/common/api/package-info.java
🧠 Learnings (9)
📚 Learning: 2025-11-23T02:13:19.046Z
Learnt from: phongphongg
Repo: workastra/server PR: 2
File: gradle/libs.versions.toml:1-10
Timestamp: 2025-11-23T02:13:19.046Z
Learning: Spring Boot 4.0.0 is compatible with org.graalvm.buildtools.native plugin version 0.11.3, which is the recommended version for Spring Boot 4 native image builds.
Applied to files:
gradle/libs.versions.toml
📚 Learning: 2025-11-30T03:19:40.814Z
Learnt from: CR
Repo: workastra/server PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-30T03:19:40.814Z
Learning: Applies to **/package-info.java : For every package, create a package-info.java file applying the NullMarked annotation at package scope
Applied to files:
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/package-info.javamodules/authentication/src/main/java/com/workastra/authentication/controller/package-info.javamodules/common/src/main/java/com/workastra/common/api/package-info.java
📚 Learning: 2025-11-30T03:19:40.814Z
Learnt from: CR
Repo: workastra/server PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-30T03:19:40.814Z
Learning: Applies to **/*.java : Audit all existing legacy code and mark any variables, method parameters, or return types that accept or return null with Nullable annotation
Applied to files:
modules/authentication/src/main/java/com/workastra/authentication/controller/package-info.javamodules/common/src/main/java/com/workastra/common/api/package-info.java
📚 Learning: 2025-11-30T03:19:40.814Z
Learnt from: CR
Repo: workastra/server PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-30T03:19:40.814Z
Learning: Applies to **/*.java : Use the Nullable annotation exclusively to mark any type permitted to hold a null value; never use NonNull annotation
Applied to files:
modules/authentication/src/main/java/com/workastra/authentication/controller/package-info.javamodules/common/src/main/java/com/workastra/common/api/package-info.java
📚 Learning: 2025-11-30T03:19:40.814Z
Learnt from: CR
Repo: workastra/server PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-30T03:19:40.814Z
Learning: Applies to **/*.java : Operate under the Non-Nullable by Default principle; all type usages (parameters, returns, fields) are non-nullable unless explicitly annotated
Applied to files:
modules/authentication/src/main/java/com/workastra/authentication/controller/package-info.javabuild.gradle.ktsmodules/common/src/main/java/com/workastra/common/api/package-info.java
📚 Learning: 2025-11-30T03:19:40.814Z
Learnt from: CR
Repo: workastra/server PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-30T03:19:40.814Z
Learning: Applies to **/*.java : Nullability must specify both the container and the content (e.g., List<Nullable String> for nullable content, Nullable List<String> for nullable container)
Applied to files:
modules/authentication/src/main/java/com/workastra/authentication/controller/package-info.javamodules/common/src/main/java/com/workastra/common/api/package-info.java
📚 Learning: 2025-11-30T03:19:40.814Z
Learnt from: CR
Repo: workastra/server PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-30T03:19:40.814Z
Learning: Applies to **/*.java : All generated code must be fully compliant with the project's checkstyle.xml; continuously review and propose refactoring for modularity and scalability
Applied to files:
build.gradle.kts
📚 Learning: 2025-11-30T03:19:40.814Z
Learnt from: CR
Repo: workastra/server PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-30T03:19:40.814Z
Learning: Applies to **/*.java : Use explicit error handling with specific exception types and Optional over broad catch(Exception e); all log statements must be structured using SLF4J/Log4j 2 with contextual information
Applied to files:
build.gradle.kts
📚 Learning: 2025-11-30T03:19:40.814Z
Learnt from: CR
Repo: workastra/server PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-30T03:19:40.814Z
Learning: Applies to **/*.java : Proactively identify and replace all deprecated or outdated APIs, libraries, and code constructs with modern Java 25 alternatives
Applied to files:
build.gradle.kts
🧬 Code graph analysis (3)
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/PasswordEncoderConfig.java (2)
modules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.java (1)
Configuration(21-41)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/SecurityConfig.java (1)
Configuration(24-88)
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/SecurityConfig.java (7)
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/filter/JsonLoginAuthenticationFilter.java (1)
JsonLoginAuthenticationFilter(31-61)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAccessDeniedHandler.java (1)
JsonAccessDeniedHandler(18-46)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthFailureHandler.java (1)
JsonAuthFailureHandler(16-37)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthSuccessHandler.java (1)
JsonAuthSuccessHandler(16-37)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthenticationEntryPoint.java (1)
JsonAuthenticationEntryPoint(18-43)modules/authentication/src/main/java/com/workastra/authentication/AuthenticationModuleConfig.java (1)
Configuration(21-41)modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/PasswordEncoderConfig.java (1)
Configuration(21-58)
modules/console/src/main/java/com/workastra/console/WorkastraConsoleApplication.java (1)
modules/worker/src/main/java/com/workastra/worker/WorkastraWorkerApplication.java (1)
SpringBootApplication(6-12)
🔇 Additional comments (11)
modules/console/src/main/resources/application-dev.yaml (1)
1-6: Configuration change is appropriate.Setting Spring Security logging to DEBUG in the dev profile makes sense for troubleshooting JWT authentication logic. YAML syntax is valid, and there's no production impact.
.vscode/launch.json (2)
10-10: Verify Java version compatibility for-XX:+UseCompactObjectHeaders.This JVM argument requires Java 15+. Ensure all developers on the team use a compatible version, or document the requirement. Otherwise, developers on older JDKs will encounter errors at launch.
11-11: Verify thedevSpring profile is defined.Confirm that a
devprofile exists in your Spring configuration (e.g.,application-dev.ymlorapplication-dev.properties). If missing, Spring will warn at startup. If authentication modules (referenced in the PR) require profile-specific configuration, ensure thedevprofile includes all necessary beans and properties.settings.gradle.kts (1)
3-8: LGTM!The module inclusion is syntactically correct and the ordering (common first, then authentication) is logical given the dependency structure described in the PR summary.
build.gradle.kts (3)
14-14: LGTM!Plugin application pattern is correct: applied at root with
apply trueand then to each subproject. This ensures Error Prone checks run across all modules consistently.Also applies to: 34-34
6-7: Compatibility verified: Versions are appropriate for Java 25.Error Prone must be run on JDK 21 or newer, and NullAway JSpecify mode requires a recent javac version, with Java 25 recommended. Error Prone 2.44.0 and NullAway 0.12.12 are both recent and compatible with Java 25. The net-ltgt-errorprone plugin version 4.3.0 is standard for modern Gradle builds. NullAway requires Error Prone version 2.14.0 or higher, so the 2.44.0 selection is well above the minimum.
Note on Lombok compatibility: Lombok modifies the in-memory AST causing numerous incompatibilities with Error Prone and NullAway, and is not particularly recommended for use together. If the project uses Lombok, verify compatibility.
If the project uses Lombok, confirm that NullAway is configured with the required
add(lombok.Generated)=trueLombok option, or consider suppressing specific warnings with@SuppressWarnings("NullAway")on Lombok-generated code.Also applies to: 14-14
58-75: Error Prone and NullAway configuration aligns well with project's nullability standards.The setup is sound and appropriately matches the project's adoption of JSpecify annotations. The
OnlyNullMarked+JSpecifyModecombination correctly enables gradual nullness checking on already-annotated code, and the error-level enforcement is suitable given the codebase already uses@Nullableand@NullMarkedannotations across modules. TheexcludedPathsregex.*/build/generated/.*follows standard conventions and is appropriate for future code generation scenarios. No additional verification needed for Error Prone checks, as the current NullAway-focused configuration aligns with the project's Non-Nullable by Default principle.modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAccessDeniedHandler.java (2)
26-45: Implementation looks correct.Status 403, JSON content type, and structured error response via
ApiResponse.errorwith request metadata. Properly delegates serialization to the injected mapper.
12-12: Jackson 3.x namespace is correct for this project—no action needed.Spring Boot 4.0.0 (configured in
gradle/libs.versions.toml) requires Jackson 3.x, which uses thetools.jackson.databindnamespace. The import is intentional and compatible with the project's Java 25 setup and Spring configuration. No verification or correction is necessary.modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/PasswordEncoderConfig.java (1)
28-40: Password encoder and authentication manager beans look good.BCrypt with default settings is appropriate. Standard pattern for exposing
AuthenticationManager.modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthenticationEntryPoint.java (1)
26-42: Implementation is correct.Proper 401 response with JSON content type and structured
ApiResponse.errorbody. Clean variable usage for the response body.
| error-prone-core = { module = "com.google.errorprone:error_prone_core", version = "2.44.0" } | ||
| nullaway = { module = "com.uber.nullaway:nullaway", version = "0.12.12" } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Error Prone 2.44.0 Java 25 compatibility
💡 Result:
Short answer: Error Prone 2.44.0 has no explicit “Java 25” support note — its release notes do not mention JDK 25 — and Error Prone's recent releases raised the minimum runtime to JDK 21. In practice you can often run Error Prone on newer JDKs (like JDK 25) but you may need the same extra jdk.compiler exports and JVM flags used for recent JDKs (e.g. --add-exports for javac internals); if you hit problems consider upgrading to a later Error Prone (2.45+), which calls out improved compatibility with newer JDK EA builds. [1][2][3]
Sources
- Error Prone v2.44.0 release notes. [1]
- Error Prone note that minimum runtime is JDK 21 (v2.43.0 release). [1]
- Notes about required jdk.compiler exports for running Error Prone on newer JDKs. [2]
If you want, I can: (a) show the exact --add-exports flags to try for JDK 25, or (b) check whether a specific Error Prone submodule/plug-in you use is reported working on JDK 25.
🌐 Web query:
NullAway 0.12.12 Java 25 compatibility
💡 Result:
Short answer: Yes — NullAway 0.12.12 is built/tested for JDK 25 and is compatible with Java 25. The project added JDK‑25 support in the 0.12.11/0.12.12 cycle and 0.12.12 fixes a performance regression from 0.12.11, so you should upgrade if you saw that issue. [1][2][3]
Notes:
- If you enable JSpecify mode, NullAway requires a javac that supports type‑use annotations in bytecode (javac ≥ 22) or you must pass -XDaddTypeAnnotationsToSymbol=true on compatible JDK 21 builds — see the JSpecify support docs. [2]
Sources:
- NullAway 0.12.12 release (notes). [1]
- NullAway JSpecify support / supported JDK versions. [2]
- NullAway 0.12.11 release (JDK 25 test/compile entries). [3]
🌐 Web query:
net-ltgt-errorprone plugin latest version 2025
💡 Result:
The latest net.ltgt.errorprone Gradle plugin release is version 4.3.0 (published June 26, 2025). Use it like:
plugins {
id("net.ltgt.errorprone") version "4.3.0"
}
Sources: Gradle Plugin Portal [1], GitHub repo [2].
Error Prone 2.44.0 requires Java 25 compatibility checks or upgrade.
NullAway 0.12.12 is confirmed compatible with Java 25 (added in the 0.12.11/0.12.12 cycle), and net-ltgt-errorprone 4.3.0 is the current latest version as of June 2025. However, error-prone-core 2.44.0 has no explicit Java 25 support documented in its release notes. While Error Prone can run on newer JDKs, Java 25 targets may require additional JVM flags (e.g., --add-exports for javac internals). Consider either:
- Adding necessary JVM configuration flags for Java 25 compatibility, or
- Upgrading to error-prone-core 2.45+ for improved Java 25 early-access build compatibility.
🤖 Prompt for AI Agents
In gradle/libs.versions.toml around lines 6-7, error-prone-core is pinned to
2.44.0 which may lack Java 25 compatibility; update the error-prone-core entry
to 2.45.0 (or the latest 2.45+ release) to pick up Java 25 compatibility fixes,
and if upgrading is not possible add the required JVM/javac flags for Java 25
compatibility (e.g., appropriate --add-exports/--add-opens flags) in the
build.gradle/gradle.properties so Error Prone can run on JDK 25.
| @NullMarked | ||
| package com.workastra.authentication.controller; | ||
|
|
||
| import org.jspecify.annotations.NullMarked; |
There was a problem hiding this comment.
Invalid Java syntax: import after package declaration.
Same issue as common/api/package-info.java. Imports must precede the annotation and package declaration.
Apply this diff:
-@NullMarked
-package com.workastra.authentication.controller;
-
import org.jspecify.annotations.NullMarked;
+
+@NullMarked
+package com.workastra.authentication.controller;📝 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.
| @NullMarked | |
| package com.workastra.authentication.controller; | |
| import org.jspecify.annotations.NullMarked; | |
| import org.jspecify.annotations.NullMarked; | |
| @NullMarked | |
| package com.workastra.authentication.controller; |
🤖 Prompt for AI Agents
In
modules/authentication/src/main/java/com/workastra/authentication/controller/package-info.java
lines 1-4, the import is placed after the package declaration which is invalid;
move the import statement before the annotation and package declaration so the
file reads: first the import of org.jspecify.annotations.NullMarked, then the
@NullMarked annotation, then the package com.workastra.authentication.controller
declaration.
| /** | ||
| * Infrastructure layer configuration for authentication and security. | ||
| * | ||
| * This package contains Spring configuration classes that set up security beans, | ||
| * filters, and the HTTP security filter chain. | ||
| * | ||
| * - SecurityConfig: Configures the HTTP security filter chain | ||
| * - PasswordEncoderConfig: Defines password encoding and authentication manager beans | ||
| */ | ||
| package com.workastra.authentication.infrastructure.config; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add @NullMarked to enforce non-null-by-default for this package.
This package-info documents the package but does not apply @NullMarked, so you lose the intended non-null-by-default semantics and NullAway/jspecify checks for these configs.
Add the annotation and import:
/**
* Infrastructure layer configuration for authentication and security.
*
* This package contains Spring configuration classes that set up security beans,
* filters, and the HTTP security filter chain.
*
* - SecurityConfig: Configures the HTTP security filter chain
* - PasswordEncoderConfig: Defines password encoding and authentication manager beans
*/
-package com.workastra.authentication.infrastructure.config;
+@org.jspecify.annotations.NullMarked
+package com.workastra.authentication.infrastructure.config;
+
+import org.jspecify.annotations.NullMarked;Based on learnings, this package must be annotated with @NullMarked.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/package-info.java
lines 1-10, the package lacks the @NullMarked annotation so types are not
non-null-by-default; add the jspecify NullMarked import and annotate the package
declaration with @NullMarked (i.e., add the appropriate import line for
org.jspecify.annotations.NullMarked and place @NullMarked immediately above the
package statement) so NullAway/jspecify and non-null-by-default semantics are
enforced.
| import com.workastra.authentication.infrastructure.filter.JsonLoginAuthenticationFilter; | ||
| import com.workastra.authentication.infrastructure.handler.JsonAccessDeniedHandler; | ||
| import com.workastra.authentication.infrastructure.handler.JsonAuthFailureHandler; | ||
| import com.workastra.authentication.infrastructure.handler.JsonAuthSuccessHandler; | ||
| import com.workastra.authentication.infrastructure.handler.JsonAuthenticationEntryPoint; | ||
| import org.springframework.context.annotation.Bean; | ||
| import org.springframework.context.annotation.Configuration; | ||
| import org.springframework.http.HttpMethod; | ||
| import org.springframework.security.authentication.AuthenticationManager; | ||
| import org.springframework.security.config.annotation.web.builders.HttpSecurity; | ||
| import org.springframework.security.config.http.SessionCreationPolicy; | ||
| import org.springframework.security.web.SecurityFilterChain; | ||
| import org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter; | ||
| import org.springframework.security.web.context.HttpSessionSecurityContextRepository; | ||
| import tools.jackson.databind.ObjectMapper; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the SecurityConfig.java file and read it completely
find . -name "SecurityConfig.java" -path "*/authentication/*" | head -5Repository: workastra/server
Length of output: 169
🏁 Script executed:
# Check for ObjectMapper usage in the codebase
rg "ObjectMapper" --type java -B 2 -A 2 | head -50Repository: workastra/server
Length of output: 6342
🏁 Script executed:
# Check build configuration for Jackson dependency
fd "pom.xml|build.gradle" | xargs cat 2>/dev/null | grep -i jackson | head -20Repository: workastra/server
Length of output: 42
🏁 Script executed:
cat -n ./modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/SecurityConfig.javaRepository: workastra/server
Length of output: 4881
🏁 Script executed:
# Look for pom.xml in authentication module
find ./modules/authentication -name "pom.xml" -exec cat {} \;Repository: workastra/server
Length of output: 42
🏁 Script executed:
# Check if tools.jackson is used consistently throughout codebase
rg "import tools\.jackson" --type java | wc -lRepository: workastra/server
Length of output: 61
🏁 Script executed:
# Also check if com.fasterxml.jackson is used anywhere
rg "import com\.fasterxml\.jackson" --type java | head -10Repository: workastra/server
Length of output: 300
🏁 Script executed:
# Find pom.xml files in the project
find . -name "pom.xml" -type fRepository: workastra/server
Length of output: 42
🏁 Script executed:
# Check root pom.xml for Jackson dependency
cat ./pom.xml | grep -A 5 -B 5 "jackson" 2>/dev/null || echo "Not found in root pom.xml"Repository: workastra/server
Length of output: 144
🏁 Script executed:
# Check if tools.jackson package exists in any dependency or if it's just a typo
rg "package tools\.jackson" --type javaRepository: workastra/server
Length of output: 42
🏁 Script executed:
# Double-check: search for any build files and Jackson version
find . -type f \( -name "*.gradle" -o -name "*.properties" \) | head -20Repository: workastra/server
Length of output: 122
🏁 Script executed:
# Verify there's no tools.jackson in Maven Central or any vendor
# Also check if Jackson is actually available as a dependency anywhere
rg "jackson" modules/ --type toml --type properties --type xml 2>/dev/null | head -20Repository: workastra/server
Length of output: 42
Fix incorrect ObjectMapper import.
Line 17 imports tools.jackson.databind.ObjectMapper, which is not a valid Jackson package. The correct import is com.fasterxml.jackson.databind.ObjectMapper. This will cause a compilation or runtime error.
Update to:
-import tools.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectMapper;📝 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.
| import com.workastra.authentication.infrastructure.filter.JsonLoginAuthenticationFilter; | |
| import com.workastra.authentication.infrastructure.handler.JsonAccessDeniedHandler; | |
| import com.workastra.authentication.infrastructure.handler.JsonAuthFailureHandler; | |
| import com.workastra.authentication.infrastructure.handler.JsonAuthSuccessHandler; | |
| import com.workastra.authentication.infrastructure.handler.JsonAuthenticationEntryPoint; | |
| import org.springframework.context.annotation.Bean; | |
| import org.springframework.context.annotation.Configuration; | |
| import org.springframework.http.HttpMethod; | |
| import org.springframework.security.authentication.AuthenticationManager; | |
| import org.springframework.security.config.annotation.web.builders.HttpSecurity; | |
| import org.springframework.security.config.http.SessionCreationPolicy; | |
| import org.springframework.security.web.SecurityFilterChain; | |
| import org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter; | |
| import org.springframework.security.web.context.HttpSessionSecurityContextRepository; | |
| import tools.jackson.databind.ObjectMapper; | |
| import com.workastra.authentication.infrastructure.filter.JsonLoginAuthenticationFilter; | |
| import com.workastra.authentication.infrastructure.handler.JsonAccessDeniedHandler; | |
| import com.workastra.authentication.infrastructure.handler.JsonAuthFailureHandler; | |
| import com.workastra.authentication.infrastructure.handler.JsonAuthSuccessHandler; | |
| import com.workastra.authentication.infrastructure.handler.JsonAuthenticationEntryPoint; | |
| import org.springframework.context.annotation.Bean; | |
| import org.springframework.context.annotation.Configuration; | |
| import org.springframework.http.HttpMethod; | |
| import org.springframework.security.authentication.AuthenticationManager; | |
| import org.springframework.security.config.annotation.web.builders.HttpSecurity; | |
| import org.springframework.security.config.http.SessionCreationPolicy; | |
| import org.springframework.security.web.SecurityFilterChain; | |
| import org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter; | |
| import org.springframework.security.web.context.HttpSessionSecurityContextRepository; | |
| import com.fasterxml.jackson.databind.ObjectMapper; |
🤖 Prompt for AI Agents
In
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/config/SecurityConfig.java
lines 3-18, the ObjectMapper import is incorrect
(tools.jackson.databind.ObjectMapper); replace it with the correct Jackson
package import com.fasterxml.jackson.databind.ObjectMapper and remove the
invalid import so the class compiles; update any IDE auto-imports or organize
imports to ensure the correct package is used.
| public JsonAccessDeniedHandler(ObjectMapper mapper) { | ||
| this.mapper = mapper; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add null validation for constructor parameter.
Per coding guidelines, enforce non-nullable by default with explicit validation. If mapper is null, fail fast.
+import java.util.Objects;
+
public JsonAccessDeniedHandler(ObjectMapper mapper) {
- this.mapper = mapper;
+ this.mapper = Objects.requireNonNull(mapper, "mapper must not be null");
}🤖 Prompt for AI Agents
In
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAccessDeniedHandler.java
around lines 22 to 24, the constructor accepts an ObjectMapper without
validation; add a null check to fail fast if mapper is null (e.g., call
Objects.requireNonNull(mapper, "mapper must not be null") or throw new
IllegalArgumentException("mapper must not be null")) so the class enforces
non-nullable constructor parameters and avoids NPEs later.
| var body = Map.of("success", false, "error", ex.getMessage()); | ||
|
|
||
| response.getWriter().write(this.objectMapper.writeValueAsString(body)); |
There was a problem hiding this comment.
ex.getMessage() can return null; also exposing raw exception messages is a security risk.
Two issues here:
- NPE risk:
AuthenticationException.getMessage()can returnnull, which will be serialized as"error": null. Handle this explicitly. - Security: Raw exception messages may leak internal details (stack traces, class names, DB info). Use a generic user-facing message.
-var body = Map.of("success", false, "error", ex.getMessage());
+var errorMessage = ex.getMessage() != null ? ex.getMessage() : "Authentication failed";
+var body = Map.of("success", false, "error", errorMessage);For better security, consider always returning a generic message like "Authentication failed" and logging the actual exception server-side for debugging.
🤖 Prompt for AI Agents
In
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthFailureHandler.java
around lines 33 to 35, the code puts ex.getMessage() into the response body
which can be null and exposes internal details; replace the response "error"
value with a fixed, generic message like "Authentication failed" (never use
ex.getMessage() in client-facing output) and instead log the actual exception
server-side (e.g. logger.warn/error("Authentication failure", ex)) so debugging
info is retained without leaking internals.
| import org.springframework.http.MediaType; | ||
| import org.springframework.security.core.Authentication; | ||
| import org.springframework.security.web.authentication.AuthenticationSuccessHandler; | ||
| import tools.jackson.databind.ObjectMapper; |
There was a problem hiding this comment.
Wrong package for ObjectMapper.
Line 10 imports tools.jackson.databind.ObjectMapper, but the standard Jackson library uses com.fasterxml.jackson.databind.ObjectMapper. This will either fail to compile or reference the wrong class.
Apply this diff:
-import tools.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectMapper;📝 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.
| import tools.jackson.databind.ObjectMapper; | |
| import com.fasterxml.jackson.databind.ObjectMapper; |
🤖 Prompt for AI Agents
In
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthSuccessHandler.java
around line 10, the import uses tools.jackson.databind.ObjectMapper which is
incorrect; replace it with com.fasterxml.jackson.databind.ObjectMapper (i.e.,
change the import statement to the standard Jackson package) so the class
resolves to the correct Jackson ObjectMapper and the code compiles.
| public JsonAuthSuccessHandler(ObjectMapper objectMapper) { | ||
| this.objectMapper = objectMapper; | ||
| } |
There was a problem hiding this comment.
Missing null check on constructor parameter.
The objectMapper parameter must be non-null (per Non-Nullable by Default principle), but there's no defensive check. Add Objects.requireNonNull to fail fast if null is passed.
Apply this diff:
+import java.util.Objects;
+
public JsonAuthSuccessHandler(ObjectMapper objectMapper) {
- this.objectMapper = objectMapper;
+ this.objectMapper = Objects.requireNonNull(objectMapper, "objectMapper must not be null");
}📝 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.
| public JsonAuthSuccessHandler(ObjectMapper objectMapper) { | |
| this.objectMapper = objectMapper; | |
| } | |
| import java.util.Objects; | |
| public JsonAuthSuccessHandler(ObjectMapper objectMapper) { | |
| this.objectMapper = Objects.requireNonNull(objectMapper, "objectMapper must not be null"); | |
| } |
🤖 Prompt for AI Agents
In
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthSuccessHandler.java
around lines 20 to 22, the constructor does not validate the objectMapper
parameter; add a defensive null check using Objects.requireNonNull(objectMapper,
"objectMapper must not be null") before assigning to this.objectMapper, and
ensure java.util.Objects is imported if not already present so the class fails
fast when a null is passed.
| response.setStatus(HttpServletResponse.SC_OK); | ||
| response.setContentType(MediaType.APPLICATION_JSON_VALUE); | ||
|
|
||
| var body = Map.of("success", true, "username", authentication.getName()); |
There was a problem hiding this comment.
Critical NPE risk: authentication.getName() can return null.
Map.of throws NPE if any value is null, but Authentication.getName() can return null. This will crash the success handler.
Apply this diff to provide a fallback:
-var body = Map.of("success", true, "username", authentication.getName());
+var username = authentication.getName();
+var body = Map.of(
+ "success", true,
+ "username", username != null ? username : "unknown"
+);Or use Map.ofEntries with explicit null handling, or mark the upstream flow to ensure getName() is never null.
🤖 Prompt for AI Agents
In
modules/authentication/src/main/java/com/workastra/authentication/infrastructure/handler/JsonAuthSuccessHandler.java
around line 33, Map.of("success", true, "username", authentication.getName())
can throw an NPE when authentication.getName() is null; fix it by computing a
safe username first (e.g., use
Optional.ofNullable(authentication.getName()).orElse(""), or a sensible default
like "unknown") and then pass that non-null username into Map.of (or switch to
Map.ofEntries with Map.entry(...) using the safe value) so no null values are
handed to Map.of.
| @NullMarked | ||
| package com.workastra.common.api; | ||
|
|
||
| import org.jspecify.annotations.NullMarked; |
There was a problem hiding this comment.
Invalid Java syntax: import after package declaration.
In package-info.java, imports must precede the annotation and package declaration. This file will not compile.
Apply this diff:
-@NullMarked
-package com.workastra.common.api;
-
import org.jspecify.annotations.NullMarked;
+
+@NullMarked
+package com.workastra.common.api;🤖 Prompt for AI Agents
In modules/common/src/main/java/com/workastra/common/api/package-info.java lines
1-4, the import is placed after the package/annotation which is invalid Java;
move the import statement to the top of the file (before any annotations or the
package declaration), then place the @NullMarked annotation immediately above
the package declaration so the file reads: import
org.jspecify.annotations.NullMarked; followed by @NullMarked and then the
package com.workastra.common.api; ensuring correct ordering for valid Java
syntax.
2 participants
@coderabbitai
Summary by CodeRabbit
Release Notes
New Features
Infrastructure
✏️ Tip: You can customize this high-level summary in your review settings.