feat(jans-fido2): add controllers for metrics and resolve staging issues #12934
Conversation
Signed-off-by: imran <imranishaq7071@gmail.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughConvert metric timestamps to UTC-backed Date types, introduce a new Fido2Metrics REST controller, make schedulers cluster-aware and UTC-aligned, refactor metric and user persistence/mapping (username lookups, device/IP/User-Agent capture), add interval-based aggregation config, and remove a trend-analysis unit test. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
mo-auto
added
comp-jans-fido2
…ns-fido2-metrics-endpoints
Signed-off-by: imran <imranishaq7071@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsService.java (2)
63-71: Async execution uses common ForkJoinPool without bounds or timeout.Using
CompletableFuture.runAsync()without an explicit executor relies on the common ForkJoinPool. Under high metrics volume, this could:
- Exhaust pool threads, affecting other async operations in the JVM
- Provide no backpressure mechanism if persistence is slow
Consider using a dedicated bounded executor for metrics operations.
♻️ Proposed approach
// Add as class field private static final ExecutorService metricsExecutor = Executors.newFixedThreadPool(4, r -> { Thread t = new Thread(r, "fido2-metrics-worker"); t.setDaemon(true); return t; }); // Then use: CompletableFuture.runAsync(() -> { ... }, metricsExecutor);
337-339: Cleanup removes entries one-by-one, causing N+1 database operations.The current implementation fetches all entries, then removes them individually in a loop. For large retention cleanups, this is inefficient.
Consider using batch delete or a single delete-by-filter operation if the persistence layer supports it.
♻️ Proposed approach
// If PersistenceEntryManager supports batch removal: // persistenceEntryManager.removeByFilter(METRICS_ENTRY_BASE_DN, Fido2MetricsEntry.class, filter); // Or batch the removals: int batchSize = 100; for (int i = 0; i < entries.size(); i += batchSize) { List<Fido2MetricsEntry> batch = entries.subList(i, Math.min(i + batchSize, entries.size())); for (Fido2MetricsEntry entry : batch) { persistenceEntryManager.remove(entry); } }jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2UserMetrics.java (4)
96-102:transientfields with@JsonObjectannotation won't persist correctly.The
userSegmentsandbehaviorPatternsfields are marked astransient, which prevents Java serialization. However, they also have@JsonObjectannotation for ORM persistence. This combination may cause data loss during serialization/deserialization cycles.If these fields need ORM persistence, remove
transient. If they are meant to be computed/transient, remove@JsonObject.🔧 Suggested fix (if persistence is intended)
`@AttributeName`(name = "jansUserSegments") `@JsonObject` - private transient Map<String, Object> userSegments; + private Map<String, Object> userSegments; `@AttributeName`(name = "jansBehaviorPatterns") `@JsonObject` - private transient Map<String, Object> behaviorPatterns; + private Map<String, Object> behaviorPatterns;
354-380: Minor timestamp inconsistency: twonew Date()calls per increment.Each increment method creates two separate
Dateinstances forlastActivityDateandlastUpdated. While the time difference is negligible, using a single timestamp would be more consistent and slightly more efficient.♻️ Suggested improvement
public void incrementRegistrations(boolean success) { this.totalRegistrations++; if (success) { this.successfulRegistrations++; } else { this.failedRegistrations++; } - this.lastActivityDate = new Date(); - this.lastUpdated = new Date(); + Date now = new Date(); + this.lastActivityDate = now; + this.lastUpdated = now; }Apply similar pattern to
incrementAuthenticationsandincrementFallbackEvents.
473-487: Consider usingidfield for equality instead of mutable fields.The
equalsandhashCodemethods usefirstRegistrationDate(a mutableDate), which is risky if instances are stored in hash-based collections. For ORM entities, using the primary identifier (id) is typically safer and more predictable.♻️ Suggested fix
`@Override` public boolean equals(Object obj) { if (this == obj) return true; if (obj == null || getClass() != obj.getClass()) return false; Fido2UserMetrics that = (Fido2UserMetrics) obj; - return Objects.equals(userId, that.userId) && - Objects.equals(username, that.username) && - Objects.equals(firstRegistrationDate, that.firstRegistrationDate); + return Objects.equals(id, that.id); } `@Override` public int hashCode() { - return Objects.hash(userId, username, firstRegistrationDate); + return Objects.hash(id); }
33-114: Add property descriptions for API documentation.The fields lack descriptions that would be reflected in auto-generated Swagger specifications. Consider adding
@Schemaor similar annotations with descriptions for fields likeuserId,username,riskScore,engagementLevel, etc., to improve API documentation quality. Based on learnings, missing property descriptions should be raised in components likejans-fido2.jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2UserMetricsService.java (3)
69-96: Fire-and-forget async without error propagation or timeout.
CompletableFuture.runAsync()without a custom executor uses the common ForkJoinPool and has no timeout. If the persistence layer is slow or blocked, tasks accumulate. Also, exceptions are only logged but not propagated or monitored.Consider:
- Using a bounded executor to prevent resource exhaustion
- Adding timeouts via
orTimeout()orcompleteOnTimeout()- Monitoring failed futures for observability
♻️ Example with bounded executor and timeout
// At class level: private static final ExecutorService metricsExecutor = Executors.newFixedThreadPool( Runtime.getRuntime().availableProcessors(), r -> { Thread t = new Thread(r, "fido2-user-metrics"); t.setDaemon(true); return t; } ); // In method: CompletableFuture.runAsync(() -> { // ... existing logic }, metricsExecutor) .orTimeout(30, TimeUnit.SECONDS) .exceptionally(ex -> { log.error("User metrics update failed: {}", ex.getMessage()); return null; });
245-260: Simplify UTC date conversion ingetNewUsers.The conversion chain
ZonedDateTime -> LocalDateTime -> ZonedDateTime -> Instant -> Dateis unnecessarily complex.♻️ Simplified conversion
public List<Fido2UserMetrics> getNewUsers(int days) { try { - // Use UTC timezone to align with FIDO2 services - LocalDateTime cutoffLdt = ZonedDateTime.now(ZoneId.of("UTC")) - .toLocalDateTime() - .minusDays(days); - Date cutoffDate = Date.from(cutoffLdt.atZone(ZoneId.of("UTC")).toInstant()); + Date cutoffDate = Date.from( + ZonedDateTime.now(ZoneId.of("UTC")) + .minusDays(days) + .toInstant() + ); Filter filter = Filter.createGreaterOrEqualFilter("jansFirstRegistrationDate", cutoffDate);
463-488: Integer division loses precision in average duration calculation.The moving average calculation uses
longdivision which truncates fractional results, accumulating error over time. For metrics where precision matters, consider maintaining a running sum or using floating-point arithmetic.🔧 Alternative: Use double for intermediate calculation
} else { // Simple moving average - in production, you might want more sophisticated calculation long currentAvg = userMetrics.getAvgRegistrationDuration(); int count = userMetrics.getSuccessfulRegistrations() != null ? userMetrics.getSuccessfulRegistrations() : 1; - long newAvg = (currentAvg * (count - 1) + durationMs) / count; + long newAvg = Math.round((currentAvg * (count - 1.0) + durationMs) / count); userMetrics.setAvgRegistrationDuration(newAvg); }jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java (1)
166-175: Async metric recording uses unbounded common pool.Same pattern as in
Fido2UserMetricsService-CompletableFuture.runAsync()without a custom executor may lead to thread pool exhaustion under high load. Consider using a dedicated bounded executor for metrics operations.jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2AnalyticsService.java (1)
46-62: Consider extracting magic numbers to configuration.While some thresholds are loaded from
ResourceBundle, several magic numbers remain hardcoded (Lines 54-61):LOW_SUCCESS_RATE_THRESHOLD = 0.8,HIGH_SUCCESS_RATE_THRESHOLD = 0.95, authentication benchmark thresholds (Lines 631-639), etc.For consistency, consider moving all thresholds to the
fido2-metrics.propertiesconfiguration file.jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsAggregationScheduler.java (1)
293-313: Verify that aggregation methods implement idempotent persistence or database-level unique constraints.The aggregation methods (
createHourlyAggregation,createDailyAggregation, etc.) callpersistenceEntryManager.persist()without checking if an aggregation already exists for the period. If multiple nodes experience cluster coordination failures and all fall back to single-node behavior, they will attempt to create duplicate aggregation records.Unlike
Fido2UserMetricsService, which properly checks existence and usesmerge()for updates, the aggregation methods lack this upsert pattern. The exception handlers only log errors without preventing duplicates. Additionally,Fido2MetricsAggregationhas no visible@Uniqueor@UniqueConstraintannotations to enforce database-level uniqueness.Either implement existence checks before persistence (following the pattern in
Fido2UserMetricsServicelines 428–436) or add database constraints to the aggregation ID.
🤖 Fix all issues with AI agents
In
`@jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsAggregation.java`:
- Line 17: Remove the unused import of java.time.LocalDateTime from the
Fido2MetricsAggregation class; open the file containing class
Fido2MetricsAggregation and delete the import line referencing LocalDateTime
since the class now uses java.util.Date types and the LocalDateTime symbol is no
longer referenced.
- Around line 69-76: The Fido2MetricsAggregation(String aggregationType, String
period, Date startTime, Date endTime) constructor sets lastUpdated via new
Date() which uses system default timezone; change it to create a UTC-based Date
(e.g., assign lastUpdated = Date.from(Instant.now()) or
Date.from(ZonedDateTime.now(ZoneId.of("UTC")).toInstant())) so lastUpdated is
consistently UTC; update imports as needed and keep the assignment located in
the same constructor.
In
`@jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsData.java`:
- Around line 105-106: Fido2MetricsData currently sets timestamp as
LocalDateTime (ZonedDateTime.now(ZoneId.of("UTC")).toLocalDateTime()), while
Fido2MetricsAggregation uses java.util.Date; to make models consistent, change
Fido2MetricsData to store timestamps as Date (UTC) instead of LocalDateTime and
initialize it using Date.from(Instant.now()) or equivalent in the constructor;
update any getters/setters in Fido2MetricsData to use Date and ensure the
service layer method convertToDate() is no longer required for this conversion
(or adjust its usage) so both Fido2MetricsData and Fido2MetricsAggregation share
the same timestamp type.
In
`@jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsEntry.java`:
- Around line 90-92: The field additionalData in class Fido2MetricsEntry is
marked transient while annotated with `@JsonObject/`@AttributeName, so it will not
be persisted; remove the transient modifier (or otherwise make it non-transient)
on the additionalData field so the ORM can serialize it, ensuring the
`@JsonObject` annotation on additionalData is effective; update the declaration of
additionalData in Fido2MetricsEntry (matching the pattern used in
Fido2UserMetrics) and run tests to confirm persistence.
- Around line 38-98: Add an LDAP schema definition for the new Fido2 metrics
model: create an objectClass named jansFido2MetricsEntry and define all
jansFido2Metrics* attributes used by the Fido2MetricsEntry class (e.g.,
jansFido2MetricsId, jansFido2MetricsType, jansFido2MetricsTimestamp,
jansFido2MetricsUserId, jansFido2MetricsUsername, jansFido2MetricsOperationType,
jansFido2MetricsStatus, jansFido2MetricsDuration,
jansFido2MetricsAuthenticatorType, jansFido2MetricsDeviceInfo,
jansFido2MetricsErrorReason, jansFido2MetricsErrorCategory,
jansFido2MetricsFallbackMethod, jansFido2MetricsFallbackReason,
jansFido2MetricsUserAgent, jansFido2MetricsIpAddress, jansFido2MetricsSessionId,
jansFido2MetricsAdditionalData, jansFido2MetricsNodeId,
jansFido2MetricsApplicationType) in the LDAP LDIF schema (the same template
location used for other FIDO2 schemas); specify appropriate syntaxes (e.g.,
distinguishedName/string/Date/Integer/JSON where supported), single- vs
multi-valued constraints, and include the new jansFido2MetricsEntry objectClass
in the appropriate structural/auxiliary category so instances of the
Fido2MetricsEntry Java class can be created and validated by the directory.
In
`@jans-fido2/server/src/main/java/io/jans/fido2/service/app/AppInitializer.java`:
- Around line 164-204: Track non-fatal timer init failures by adding a boolean
or counter (e.g., failedTimerInits or hasTimerInitWarnings) in AppInitializer
and increment/set it inside each catch block for metricService.initTimer,
configurationFactory.initTimer, loggerService.initTimer, cleanerTimer.initTimer,
mds3UpdateTimer.initTimer, and customScriptManager.initTimer; after all
try/catch blocks, change the final success logging to check this flag/counter
and log either a normal success or a warning like "Completed with warnings: X
timer(s) failed to initialize" (include the count) so the final message reflects
partial failures.
- Around line 206-216: The null check on the injected CDI Instance
fido2MetricsAggregationSchedulerInstance is redundant because CDI always injects
the wrapper; remove the "fido2MetricsAggregationSchedulerInstance != null &&"
portion and only check isUnsatisfied() before calling
fido2MetricsAggregationSchedulerInstance.get().initTimer(), keeping the existing
try/catch and log messages (references:
fido2MetricsAggregationSchedulerInstance, isUnsatisfied(), get(), initTimer()).
In
`@jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsAggregationScheduler.java`:
- Around line 106-119: The warning currently logs a hardcoded failure reason
when scheduler.startPeriodicLockUpdates() returns null; replace the misleading
literal with a generic message that does not invent the cause. Update the
log.warn call (in Fido2MetricsAggregationScheduler around updateTask handling)
to something like "Cluster lock updates unavailable, running in single-node mode
for {}" using jobType, or otherwise surface a real error if
startPeriodicLockUpdates() is changed to return/throw the actual failure; remove
the hardcoded "ou=node is not configured..." text so the log reflects only known
facts.
- Around line 419-434: The initializeClusterEnvironment method currently catches
a generic Exception when attempting
CDI.current().select(Fido2ClusterNodeService.class).get(), which hides whether
the failure is simply UnsatisfiedResolutionException (no bean) or a real
configuration/runtime error; update the try/catch to catch
javax.enterprise.inject.UnsatisfiedResolutionException specifically to handle
the single-node case (set isClusterEnvironment=false, updateExecutor=null and
log that cluster service is absent), and add a separate catch for Exception (or
Throwable) to log the full error and rethrow or handle as a true failure so
configuration errors when obtaining clusterNodeService are visible; keep
references to clusterNodeService, updateExecutor, isClusterEnvironment and the
logging messages in Fido2MetricsAggregationScheduler.
In
`@jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsService.java`:
- Around line 323-329: The current Fido2MetricsService code performs an
unnecessary ZonedDateTime → LocalDateTime → ZonedDateTime → Date conversion for
cutoffDate; replace this with a direct Instant-based computation: compute an
Instant by subtracting retentionDays from Instant.now(Clock.systemUTC()) (or
Instant.now().minus(retentionDays, ChronoUnit.DAYS)) and then create cutoffDate
via Date.from(instant), updating references to cutoffDateTime/cutoffDate
accordingly to remove the redundant LocalDateTime conversion.
- Around line 647-653: The helper convertToDate(LocalDateTime dateTime)
currently assumes the input is UTC without stating or enforcing it; update the
method to either rename it to convertToDateUtc or change its signature to accept
an explicit ZoneId (e.g., convertToDate(LocalDateTime dateTime, ZoneId zone))
and use that zone when calling dateTime.atZone(zone).toInstant(); update all
callers in Fido2MetricsService to pass ZoneId.of("UTC") (or the appropriate
zone) and add a one-line Javadoc clarifying the required timezone behavior if
you choose the rename approach.
In
`@jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java`:
- Around line 470-512: The extractIpAddress method currently trusts
client-supplied proxy headers; update it so headers are only used when the
request came through a trusted proxy: add a configurable flag and trusted-proxy
list (e.g., via an injected AppConfiguration or MetricService constructor),
check request.getRemoteAddr() is in that trusted-proxy set (or
appConfiguration.isBehindTrustedProxy() + isTrustedProxy(remoteAddr)) before
reading X-Forwarded-For/other headers in extractIpAddress(HttpServletRequest
request), otherwise always return request.getRemoteAddr(); ensure the
trusted-proxy config is plumbed into MetricService and documented in the config
object so header-trusting is opt-in and configurable.
- Around line 521-563: getUserIdFromUsername currently issues a persistence
query on every call (using userPersistenceEntryManager) which can overload DB
under high throughput; change MetricService to first consult a threadsafe
username->userId cache (e.g., userIdCache in MetricService) with TTL and only
call the existing persistence lookup code when cache miss, storing resolved inum
in the cache (and caching negative/fallback results appropriately), and add an
optional configuration flag (e.g., enableUserLookup) so the lookup can be
disabled; refactor the persistence logic into a helper like
lookupUserIdFromPersistence(username) that contains the current
findEntries/filter/inum resolution so getUserIdFromUsername only handles cache,
TTL eviction, and optional config checks.
- Around line 599-612: Replace direct username logging in MetricService where
log.debug uses metricsData.getUsername() (the three occurrences after
updateUserRegistrationMetrics, updateUserAuthenticationMetrics, and
updateUserFallbackMetrics) with a privacy-safe identifier: either log the
userId/inum (e.g., request.getUserId() or metricsData.getUserId()) or a
deterministic hash/anonymized token of the username; ensure the same identifier
is used consistently across the three branches so logs remain correlatable but
free of raw PII, and update log messages to reflect the new field (e.g.,
"Updated user registration metrics for userId:{}" or "for userHash:{}").
In
`@jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java`:
- Around line 56-57: The ISO_FORMATTER currently set to
DateTimeFormatter.ISO_LOCAL_DATE_TIME in Fido2MetricsController rejects ISO-8601
strings with offsets; update the parsing/formatting to accept offset/zulu
timestamps by replacing or augmenting ISO_FORMATTER handling with an
offset-aware flow: attempt parsing with DateTimeFormatter.ISO_OFFSET_DATE_TIME
(or OffsetDateTime.parse) and convert the result to UTC (e.g., to Instant or
OffsetDateTime.withOffsetSameInstant(ZoneOffset.UTC) then to
LocalDateTime/desired format), and fallback to the existing ISO_LOCAL_DATE_TIME
parse if needed; ensure all places referencing ISO_FORMATTER or parsing
timestamps in Fido2MetricsController use this new accept-and-normalize logic and
update any API docs/comments to indicate offsets are supported.
- Around line 362-377: The controller returns a raw Map from the /config
endpoint in Fido2MetricsController which yields poor Swagger docs; create a new
DTO class (e.g., Fido2MetricsConfigResponse) with fields matching the map
entries (metricsEnabled, aggregationEnabled, retentionDays,
deviceInfoCollection, errorCategorization, performanceMetrics,
supportedAggregationTypes) and annotate each field with
`@Schema`(description="...") and proper types, then update the controller to
construct and return that DTO (instead of the Map and
dataMapperService.writeValueAsString(config)) so the endpoint method signature
exposes the concrete response type for OpenAPI generation.
- Around line 464-477: In processRequest (Fido2MetricsController.processRequest)
avoid including e.getMessage() in the client-facing
errorResponseFactory.unknownError; instead log the full exception with log.error
(already present) and pass a generic, non-sensitive message to unknownError
(e.g., "An unexpected error occurred while processing the metrics request") or a
sanitized summary; optionally generate and return an error id/UUID to the client
and include that id in the server log alongside the exception to correlate logs
(use that id in the unknownError payload) so internal details never leak.
- Around line 385-398: The getHealth method currently always returns status
"UP"; update it to perform a simple connectivity check via the metricsService
(e.g., call a lightweight method like metricsService.isHealthy() or
metricsService.ping()/metricsService.testConnection()) and catch any exceptions
from that call, setting "status" to "DOWN" and including an error message when
the check fails; retain the existing fields (metricsEnabled, aggregationEnabled,
timestamp) and ensure you still return the serialized map via
dataMapperService.writeValueAsString(health) while handling exceptions so
failures produce an informative JSON health response instead of always "UP".
- Around line 37-39: Fido2MetricsController currently exposes user-specific
endpoints like /entries/user/{userId} without auth; update
Fido2MetricsController to enforce authorization by adding an appropriate
security annotation (e.g., `@ProtectedApi` or `@RolesAllowed`("ROLE_ADMIN") or
similar used across the project) on the class or on the user-specific method(s)
that handle /entries/user/{userId}, or if security is handled outside the app,
add a clear comment/Javadoc on Fido2MetricsController stating that
authentication/authorization is enforced by the API gateway/OAuth interceptor
and specify the required role/scope so reviewers know the endpoint is protected.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsAggregation.javajans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsConstants.javajans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsData.javajans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsEntry.javajans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2UserMetrics.javajans-fido2/server/src/main/java/io/jans/fido2/service/app/AppInitializer.javajans-fido2/server/src/main/java/io/jans/fido2/service/app/ResteasyInitializer.javajans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2AnalyticsService.javajans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsAggregationScheduler.javajans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsService.javajans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2UserMetricsService.javajans-fido2/server/src/main/java/io/jans/fido2/service/shared/LoggerService.javajans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.javajans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.javajans-fido2/server/src/main/resources/fido2-metrics.propertiesjans-fido2/server/src/test/java/io/jans/fido2/service/metric/Fido2MetricsTrendAnalysisTest.java
💤 Files with no reviewable changes (2)
- jans-fido2/server/src/main/java/io/jans/fido2/service/shared/LoggerService.java
- jans-fido2/server/src/test/java/io/jans/fido2/service/metric/Fido2MetricsTrendAnalysisTest.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: pujavs
Repo: JanssenProject/jans PR: 12847
File: jans-config-api/docs/jans-config-api-swagger.yaml:15497-15498
Timestamp: 2025-12-17T13:07:08.750Z
Learning: When reviewing components like `jans-auth`, `jans-fido2`, `jans-lock` in the JanssenProject/jans repository, raise issues about missing property descriptions in API model classes, as these descriptions will be reflected in the auto-generated Swagger specifications.
🧬 Code graph analysis (3)
jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsService.java (1)
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsConstants.java (1)
Fido2MetricsConstants(15-121)
jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsAggregationScheduler.java (1)
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsConstants.java (1)
Fido2MetricsConstants(15-121)
jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java (1)
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/UserMetricsUpdateRequest.java (1)
UserMetricsUpdateRequest(17-141)
⏰ 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). (11)
- GitHub Check: sonar scan (jans-linux-setup)
- GitHub Check: sonar scan (jans-pycloudlib)
- GitHub Check: sonar scan (jans-core)
- GitHub Check: sonar scan (jans-auth-server)
- GitHub Check: sonar scan (jans-keycloak-integration)
- GitHub Check: sonar scan (jans-casa)
- GitHub Check: sonar scan (jans-link)
- GitHub Check: sonar scan (jans-config-api)
- GitHub Check: sonar scan (jans-fido2)
- GitHub Check: sonar scan (jans-cli-tui)
- GitHub Check: sonar scan (jans-scim)
🔇 Additional comments (22)
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsConstants.java (2)
74-74: LGTM!The renamed constant
jansFido2MetricsTimestampprovides a more specific and descriptive attribute name, distinguishing FIDO2 metrics timestamps from other timestamp fields in the system.
111-113: LGTM!The new query parameter constants support the REST API's time-based filtering requirements and align with the PR objectives.
jans-fido2/server/src/main/resources/fido2-metrics.properties (1)
1-32: Configuration structure is well-organized.The staggered execution times (5, 10, 15, 20 minutes past the hour) help prevent concurrent aggregation runs. The retention periods are reasonable.
Consider documenting the timezone assumption for cron expressions. Since the service uses UTC for timestamps, the cron jobs should ideally also execute in UTC context to ensure consistency. This may need to be configured at the scheduler level.
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsAggregation.java (1)
276-294: Defensive numeric conversion is appropriate.The updated
getMapMetric()correctly handles JSON deserialization edge cases where numeric values may be deserialized asIntegerinstead ofLong. The explicit conversion via((Number) val).longValue()is a robust solution.jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsService.java (4)
91-98: UTC conversion for persistence filters is correct.The
convertToDate()helper properly convertsLocalDateTimetoDateusing UTC zone, aligning with the broader UTC strategy in this PR.
296-302: Interval overlap logic is correctly implemented.The filter correctly identifies aggregations that overlap with the query range using the standard interval overlap condition:
aggregation.startTime <= queryEndTime AND aggregation.endTime >= queryStartTime.
693-698: Essential fields may include null values.The
setEssentialFieldsmethod setsuserId,username,operationType, andstatusunconditionally, but these fields may be null in the sourcemetricsData. If null values are problematic for persistence, consider adding null guards similar tosetOptionalFields.
743-761: Device info population logic is well-structured.The boolean accumulation pattern (
hasDeviceInfo |= setDeviceField(...)) correctly tracks whether any device field was set, avoiding persisting emptyDeviceInfoobjects.jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2UserMetricsService.java (2)
335-378: LGTM - UTC-based behavior pattern analysis.The method correctly uses UTC for temporal calculations and provides comprehensive user behavior analytics.
425-440: LGTM - Proper persist/merge distinction for user metrics.The method correctly differentiates between new entries (using
persist) and existing entries (usingmerge) based on DN presence.jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java (1)
349-408: LGTM - Robust metrics data creation with graceful degradation.The method implements defensive coding with multiple try-catch blocks, ensuring partial failures (device info extraction, node ID retrieval) don't prevent metric collection. Good use of fallback to minimal device info on extraction failure.
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsEntry.java (2)
105-111: LGTM - UTC timestamp initialization in constructor.The constructor correctly initializes the timestamp using UTC timezone, consistent with the broader UTC standardization in this PR.
33-34: Good addition:@JsonInclude(NON_NULL)optimizes JSON responses.This annotation reduces API response payload size by excluding null fields, which aligns with the PR objective of optimizing JSON responses.
jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2AnalyticsService.java (2)
92-98: LGTM - UTC-aligned report metadata.The report metadata now correctly uses UTC for the
generatedAttimestamp, ensuring consistency across the metrics system.
557-587: LGTM - Correct UTC conversion for seasonal pattern analysis.The method properly converts
DatetoLocalDateTimeusing UTC zone before extracting day-of-week information, maintaining consistency with the UTC-based timestamp approach across the PR.jans-fido2/server/src/main/java/io/jans/fido2/service/app/ResteasyInitializer.java (1)
37-37: LGTM!The
Fido2MetricsControllerregistration follows the existing pattern for exposing REST controllers and properly integrates the new metrics endpoints.jans-fido2/server/src/main/java/io/jans/fido2/service/app/AppInitializer.java (1)
107-108: LGTM: Proper CDI pattern for optional bean.Using
Instance<Fido2MetricsAggregationScheduler>allows lazy access and availability checking, which is appropriate for an optional feature component.jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsAggregationScheduler.java (4)
72-86: Acceptable defensive loading for optional configuration.The static block gracefully handles missing
fido2-metrics.propertiesfile with a fallback to defaults. This ensures the scheduler can operate even without a configuration file.
147-155: LGTM: Consistent UTC handling across all aggregation jobs.The UTC-based timestamp calculations are correct and consistent across hourly, daily, weekly, and monthly aggregations. This aligns with the broader FIDO2 services UTC convention mentioned in the AI summary.
371-412: Lock update mechanism is sound.The periodic lock update logic correctly handles both cluster and single-node modes. The 30-second refresh interval is reasonable for maintaining lock heartbeats.
Consider making the refresh interval configurable if cluster environments have varying latency requirements.
546-562: LGTM: Proper executor shutdown handling.The shutdown logic follows best practices: graceful shutdown with timeout, forced shutdown on timeout, and proper handling of
InterruptedExceptionby restoring the interrupt flag.jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java (1)
443-457: LGTM: Robust aggregation type validation.The validation correctly normalizes to uppercase before comparison and provides a clear error message listing valid options.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsAggregation.java
Outdated
Show resolved
Hide resolved
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsAggregation.java
Show resolved
Hide resolved
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsData.java
Show resolved
Hide resolved
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsEntry.java
Show resolved
Hide resolved
| @AttributeName(name = "jansFido2MetricsAdditionalData") | ||
| @JsonObject | ||
| private transient Map<String, Object> additionalData; |
There was a problem hiding this comment.
transient field with @JsonObject won't persist.
Same issue as in Fido2UserMetrics - the additionalData field is marked transient but has @JsonObject for ORM. This combination will prevent persistence of this field.
🔧 Suggested fix
`@AttributeName`(name = "jansFido2MetricsAdditionalData")
`@JsonObject`
- private transient Map<String, Object> additionalData;
+ private Map<String, Object> additionalData;📝 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.
| @AttributeName(name = "jansFido2MetricsAdditionalData") | |
| @JsonObject | |
| private transient Map<String, Object> additionalData; | |
| `@AttributeName`(name = "jansFido2MetricsAdditionalData") | |
| `@JsonObject` | |
| private Map<String, Object> additionalData; |
🤖 Prompt for AI Agents
In
`@jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsEntry.java`
around lines 90 - 92, The field additionalData in class Fido2MetricsEntry is
marked transient while annotated with `@JsonObject/`@AttributeName, so it will not
be persisted; remove the transient modifier (or otherwise make it non-transient)
on the additionalData field so the ORM can serialize it, ensuring the
`@JsonObject` annotation on additionalData is effective; update the declaration of
additionalData in Fido2MetricsEntry (matching the pattern used in
Fido2UserMetrics) and run tests to confirm persistence.
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java
Show resolved
Hide resolved
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java
Show resolved
Hide resolved
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java
Show resolved
Hide resolved
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java
Show resolved
Hide resolved
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java
Show resolved
Hide resolved
…ns-fido2-metrics-endpoints
Signed-off-by: imran <imranishaq7071@gmail.com>
Signed-off-by: imran <imranishaq7071@gmail.com>
…ns-fido2-metrics-endpoints
…ssenProject/jans into jans-fido2-metrics-endpoints
|
|
|
|
|
|
|
|
|
|
|
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsAggregation.java (1)
111-141: Defensive copies for mutableDatefields.
Dateis mutable; returning/storing references allows external mutation of entity state. Consider copying on get/set to preserve encapsulation. Please verify that no callers rely on mutating the returnedDateinstance.♻️ Suggested change
public Date getStartTime() { - return startTime; + return startTime == null ? null : new Date(startTime.getTime()); } public void setStartTime(Date startTime) { - this.startTime = startTime; + this.startTime = startTime == null ? null : new Date(startTime.getTime()); } public Date getEndTime() { - return endTime; + return endTime == null ? null : new Date(endTime.getTime()); } public void setEndTime(Date endTime) { - this.endTime = endTime; + this.endTime = endTime == null ? null : new Date(endTime.getTime()); } public Date getLastUpdated() { - return lastUpdated; + return lastUpdated == null ? null : new Date(lastUpdated.getTime()); } public void setLastUpdated(Date lastUpdated) { - this.lastUpdated = lastUpdated; + this.lastUpdated = lastUpdated == null ? null : new Date(lastUpdated.getTime()); }
🤖 Fix all issues with AI agents
In
`@jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsEntry.java`:
- Around line 38-105: Add OpenAPI `@Schema` annotations to the Fido2MetricsEntry
class and its publicly serialized fields so REST responses include descriptions
for Swagger; specifically annotate the class Fido2MetricsEntry and each field
such as id, metricType, timestamp, userId, username, operationType, status,
durationMs, authenticatorType, deviceInfo, errorReason, errorCategory,
fallbackMethod, fallbackReason, userAgent, ipAddress, sessionId, additionalData,
nodeId, and applicationType with `@Schema`(description="...") following the
project's pattern (see jans-scim/jans-lock usage) — include concise
human-readable descriptions for each field and mark required/nullable where
applicable. Ensure imports for io.swagger.v3.oas.annotations.media.Schema are
added and that transient/additionalData retains `@JsonObject` while also receiving
its `@Schema` description.
In `@jans-fido2/server/pom.xml`:
- Around line 115-121: The Surefire plugin dependency currently hardcodes
junit-jupiter-engine version "5.9.2"; change that dependency to reference the
managed property ${junit.jupiter.version} instead so it uses the BOM-managed
JUnit version (update the <dependency> entry for junit-jupiter-engine inside the
Surefire plugin to remove the literal version and use ${junit.jupiter.version}).
Ensure the artifactId junit-jupiter-engine remains unchanged and no other plugin
declarations are altered.
In
`@jans-fido2/server/src/main/java/io/jans/fido2/service/app/AppInitializer.java`:
- Around line 219-228: The current code always logs "=== FIDO2 Application
Initialization Completed Successfully ===" even if
eventApplicationInitialized.select(...).fire(new ApplicationInitializedEvent())
throws; change the logic so the completed-success message is only emitted when
the event firing succeeds (or set a boolean like eventFiredOk) and emit an
error/alternate status when it fails; specifically adjust the try/catch around
eventApplicationInitialized.select(ApplicationInitialized.Literal.APPLICATION).fire(...)
and move or gate the log.info("=== FIDO2 Application Initialization Completed
Successfully ===") behind the successful path (or log a failure status when
catching the exception) so observers’ failures are reflected in the final
status.
- Around line 107-108: The class AppInitializer currently injects the scheduler
using its fully-qualified name; add an import for
io.jans.fido2.service.metric.Fido2MetricsAggregationScheduler and change the
field declaration
Instance<io.jans.fido2.service.metric.Fido2MetricsAggregationScheduler>
fido2MetricsAggregationSchedulerInstance to use the simple type
Instance<Fido2MetricsAggregationScheduler>
fido2MetricsAggregationSchedulerInstance to match the style of other injected
dependencies and improve readability.
In
`@jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsAggregationScheduler.java`:
- Around line 106-134: The cluster branch must not proceed with aggregation if
we failed to start periodic lock updates; update the
Fido2MetricsAggregationScheduler method that calls shouldPerformAggregation()
and startPeriodicLockUpdates() so that when isClusterMode is true and
startPeriodicLockUpdates() returns null (updateTask == null) you log the warning
(as existing) and immediately return/abort scheduling instead of continuing to
invoke the async aggregation methods; specifically, after detecting updateTask
== null in the block that handles cluster mode (referencing isClusterMode,
startPeriodicLockUpdates(), updateTask and shouldPerformAggregation()), stop
further execution (return from the method or skip submitting
CompletableFuture.runAsync tasks) to avoid duplicate aggregation writes.
In
`@jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsService.java`:
- Around line 520-524: The conversion of startTime and endTime to java.util.Date
is duplicated; replace the inline conversions with calls to the existing
convertToDate method to ensure uniform UTC handling. In Fido2MetricsService,
where startDate and endDate are created from startTime and endTime, call
convertToDate(startTime) and convertToDate(endTime) instead of using
Date.from(...atZone(ZoneId.of("UTC")).toInstant()), then pass those Dates into
the Fido2MetricsAggregation constructor.
In
`@jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java`:
- Around line 86-106: The userIdCache is unbounded and will grow indefinitely;
add a bounded eviction and periodic cleanup: introduce a MAX_USER_ID_CACHE_SIZE
constant and replace the ConcurrentHashMap-based userIdCache with either a
thread-safe LRU map (e.g., a synchronized LinkedHashMap override that
removesEldestEntry when size > MAX_USER_ID_CACHE_SIZE) or use a small
Caffeine/Guava cache with maximumSize and
expireAfterWrite(USER_ID_CACHE_TTL_MS), keep the CacheEntry class and isExpired
logic only if you stick with manual cleanup, and add a ScheduledExecutorService
task that periodically scans/evicts expired entries; also ensure put operations
enforce trimming (remove eldest) so the cache never exceeds
MAX_USER_ID_CACHE_SIZE — apply the same change to the other username→userId
cache usage that mirrors userIdCache.
- Around line 626-642: The debug logs in MetricService that print the raw
username (referenced as variable username) should be changed to avoid logging
PII: when an inum (userId) is found (users.get(0).getAttribute("inum")), log
only the resolved inum and omit the username; when falling back to using the
username as identifier, log a generic message that the fallback is used without
emitting the raw username (or log a non-reversible identifier such as a hashed
value if diagnostic mapping is required). Also update the exception catch log to
include the exception message but not the username; reference the existing
log.debug calls around resolving userId/inum and the catch block in
MetricService to apply these changes.
In
`@jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java`:
- Around line 165-182: The validateAggregationType method should be renamed to
normalizeAggregationType and changed to return the uppercased, validated
aggregation type (throwing the same invalidRequest errors for null/empty or
invalid values); then update all call sites that currently call
validateAggregationType and then pass the original aggregationType (e.g., in
getAggregations and the other endpoints that call it) to instead use the
returned normalized value when calling metricsService methods and any downstream
consumers (replace usages that pass the original parameter with the returned
uppercased String from normalizeAggregationType).
- Around line 83-154: The endpoints getMetricsEntries, getMetricsEntriesByUser,
and getMetricsEntriesByOperation currently accept startTime/endTime but don't
validate ordering; after parsing with parseDateTime (using
Fido2MetricsConstants.PARAM_START_TIME / PARAM_END_TIME) add a check: if
start.isAfter(end) then throw errorResponseFactory.invalidRequest("startTime
must be before or equal to endTime") so the controller returns a 400 for
inverted ranges; apply the same check in all methods (and any similar endpoints)
immediately after parsing start and end.
♻️ Duplicate comments (5)
jans-fido2/server/src/main/java/io/jans/fido2/service/app/AppInitializer.java (1)
164-204: Final success message may be misleading when timer initializations fail.When any of these timer initializations fail (metricService, configurationFactory, loggerService, cleanerTimer, mds3UpdateTimer, customScriptManager), the error is logged but the application continues to line 228 and logs "Completed Successfully". This can mask startup issues in production.
Consider tracking timer initialization outcomes and reflecting them in the final status:
♻️ Suggested approach
+ int failedTimerCount = 0; + try { metricService.initTimer(); log.info("Metric service timer initialized"); } catch (Exception e) { log.error("Failed to initialize metric service timer: {}", e.getMessage(), e); + failedTimerCount++; } // ... similar for other timers ... - log.info("=== FIDO2 Application Initialization Completed Successfully ==="); + if (failedTimerCount > 0) { + log.warn("=== FIDO2 Application Initialization Completed with {} timer(s) failed ===", failedTimerCount); + } else { + log.info("=== FIDO2 Application Initialization Completed Successfully ==="); + }jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsEntry.java (1)
90-98:transientmay prevent persistence of additionalData.If Jans ORM filters
transientfields (common),additionalDatawill never persist despite@JsonObject. Please confirm ORM behavior or droptransient.🐛 Suggested fix (if persistence is required)
`@AttributeName`(name = "jansFido2MetricsAdditionalData") `@JsonObject` -private transient Map<String, Object> additionalData; +private Map<String, Object> additionalData;#!/bin/bash # Check whether Jans ORM skips transient fields during persistence rg -n --type=java -C3 'Modifier\.isTransient|isTransient\(' rg -n --type=java -C3 '@JsonObject'jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java (1)
492-551: Proxy header trust still allows spoofing.The TODOs are useful, but without a trusted-proxy check this remains vulnerable to client header spoofing. Please ensure proxy-header trust is configurable and only enabled behind trusted proxies.
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java (1)
369-390: Consider typed DTOs for config (and other map responses).Returning raw maps weakens OpenAPI docs and hides field descriptions. A dedicated DTO with
@Schemaannotations will generate better Swagger output. Based on learnings, please add property descriptions in source model classes rather than the generated Swagger YAML.jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsAggregationScheduler.java (1)
435-449: Cluster detection should not swallow unexpected exceptions.Catching
UnsatisfiedResolutionExceptionseparately will avoid masking real configuration errors.
jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsEntry.java
Show resolved
Hide resolved
jans-fido2/server/src/main/java/io/jans/fido2/service/app/AppInitializer.java
Outdated
Show resolved
Hide resolved
jans-fido2/server/src/main/java/io/jans/fido2/service/app/AppInitializer.java
Show resolved
Hide resolved
...ido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsAggregationScheduler.java
Outdated
Show resolved
Hide resolved
jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsService.java
Outdated
Show resolved
Hide resolved
jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java
Show resolved
Hide resolved
jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java
Outdated
Show resolved
Hide resolved
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java
Show resolved
Hide resolved
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java
Show resolved
Hide resolved
Signed-off-by: imran <imranishaq7071@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java`:
- Around line 517-593: The isValidIpAddress regex is too permissive and proxy
header trust is unchecked; update isValidIpAddress to use
InetAddress.getByName(...) for real validation (catch UnknownHostException and
return false) and preserve checks for localhost/loopback (::1/127.0.0.1), and
make extractIpAddress respect a configurable boolean (e.g., trustProxyHeaders)
so the proxyHeadersToTry loop only runs when that config flag is true (ensure
the new config is read where MetricService is constructed or injected).
- Around line 86-131: cleanupCache has a race when using a stream over the live
ConcurrentHashMap and the inner condition uses >= so the eviction block always
runs at max size; fix by taking a snapshot list of userIdCache.entrySet() (new
ArrayList<>(userIdCache.entrySet())), sort that snapshot by
CacheEntry.timestamp, compute removeCount = Math.max(0, userIdCache.size() -
USER_ID_CACHE_MAX_SIZE + 1000) only when userIdCache.size() >
USER_ID_CACHE_MAX_SIZE, then iterate the first removeCount entries and call
userIdCache.remove(key) for each (or collect keys and call removeAll) so
removals operate on a stable snapshot; optionally wrap the eviction block in a
synchronized(userIdCache) if you need stronger atomicity.
In
`@jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java`:
- Around line 598-611: checkServiceAvailability currently only checks that
metricsService != null; replace this with a lightweight functional check by
invoking a simple non-mutating method on the service (e.g., a
ping/health/count/fetchOne-like method such as metricsService.ping() or
metricsService.countMetrics()) to validate DB/connectivity and service
initialization, catch RuntimeException around that call, log the full exception
and return false on failure, and return true only when the call succeeds; update
references to checkServiceAvailability and metricsService accordingly.
♻️ Duplicate comments (4)
jans-fido2/server/src/main/java/io/jans/fido2/service/app/AppInitializer.java (2)
165-205: Timer initialization failures are silently absorbed.Each timer failure is logged but doesn't affect the final "Completed Successfully" status on line 229. While this is appropriate behavior (timers are non-critical), consider tracking failure counts to emit a more accurate completion message like "Completed with N timer initialization warnings".
220-229: Event firing failure doesn't prevent success status.If
eventApplicationInitialized.fire()throws, the error is logged but "Completed Successfully" is still emitted on line 229. This may be intentional to prevent a misbehaving observer from crashing the server, but plugins relying on this event could be in an inconsistent state.Consider either adjusting the final message to reflect partial success, or adding a comment documenting that this behavior is intentional.
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java (1)
386-401: Config endpoint returns useful information but could benefit from typed response.Based on learnings for this repository, API model classes should have property descriptions for better Swagger documentation. The
/configendpoint returns aMap<String, Object>which results in poor auto-generated documentation.Consider creating a
Fido2MetricsConfigResponseDTO with@Schemaannotations for each field.jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsAggregationScheduler.java (1)
447-462: Cluster environment detection catches generic Exception.The
initializeClusterEnvironmentmethod (lines 447-462) catchesExceptionwhen attempting CDI lookup. As noted in a previous review, this obscures whether the failure is due toUnsatisfiedResolutionException(expected for single-node) vs an actual configuration error.♻️ More precise exception handling
private void initializeClusterEnvironment() { // Try to detect cluster environment by checking if cluster service exists try { // Attempt to get cluster service via CDI lookup clusterNodeService = CDI.current().select(Fido2ClusterNodeService.class).get(); isClusterEnvironment = true; // Initialize update executor for cluster coordination updateExecutor = Executors.newScheduledThreadPool(1); log.info("FIDO2 metrics aggregation enabled in CLUSTER mode - distributed locking will be used"); - } catch (Exception e) { + } catch (jakarta.enterprise.inject.UnsatisfiedResolutionException e) { // Cluster service not available - single node deployment isClusterEnvironment = false; updateExecutor = null; log.info("FIDO2 metrics aggregation enabled in SINGLE-NODE mode - no cluster coordination needed"); + } catch (Exception e) { + // Unexpected error during cluster detection + isClusterEnvironment = false; + updateExecutor = null; + log.warn("Failed to initialize cluster environment, falling back to single-node mode: {}", e.getMessage()); } }
jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java
Show resolved
Hide resolved
jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java
Show resolved
Hide resolved
Signed-off-by: imran <imranishaq7071@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java (2)
351-373: Fallback events never update user-level metrics.
updateUserMetricsincludes aFALLBACKbranch butrecordPasskeyFallbacknever calls it, so per-user fallback analytics remain empty. Also setuserIdto keep identifiers consistent.🔧 Suggested fix
- metricsData.setUsername(username); + metricsData.setUsername(username); + metricsData.setUserId(getUserIdFromUsername(username)); metricsData.setFallbackMethod(fallbackMethod); metricsData.setFallbackReason(reason); ... storeFido2MetricsData(metricsData); + updateUserMetrics(metricsData);
307-331: User metrics are skipped when device-info collection is disabled.
updateUserMetricsis inside theisFido2DeviceInfoCollection()guard at line 310, so disabling device-info collection also disables per-user metrics entirely. However, user metrics only require optional device-info fields (with null-checks at line 733) and can work with just username, userId, status, authenticator type, duration, and request-derived fields (IP, user-agent) which are extracted independently of device-info collection. Decouple the calls: continue storing device-level metrics only when device-info is enabled, but allow user-metric updates to proceed when status is not ATTEMPT, regardless of device-info collection setting.
🤖 Fix all issues with AI agents
In
`@jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java`:
- Around line 129-158: The getMetricsEntriesByOperation method currently only
checks for non-empty operationType; normalize the input (e.g., trim and
toUpperCase) and validate it against the supported operation values (the
constants/enum used by your metrics layer such as REGISTRATION and
AUTHENTICATION or Fido2MetricsConstants/OperationType) before calling
metricsService.getMetricsEntriesByOperation; if the value is not one of the
allowed values, throw errorResponseFactory.invalidRequest("unsupported
operationType: ...") so callers receive a clear validation error instead of an
empty result.
- Around line 29-43: The class Javadoc for Fido2MetricsController contains an
incorrect GitHub issue reference (`#11923`); update the Javadoc comment on the
Fido2MetricsController class to reference the correct issue number (`#11922`) so
PR traceability is accurate, leaving the rest of the comment unchanged.
- Around line 63-71: Update the explanatory comment above the formatters to
accurately state that ISO_FORMATTER (DateTimeFormatter.ISO_LOCAL_DATE_TIME)
parses timestamps without offsets and is interpreted as UTC, while
ISO_OFFSET_FORMATTER (DateTimeFormatter.ISO_OFFSET_DATE_TIME) supports ISO‑8601
timestamps with timezone offsets; mention that ISO_OFFSET_FORMATTER can be used
to accept offsets and convert to UTC if desired and remove the misleading line
that implies offset support is only a consideration.
♻️ Duplicate comments (3)
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java (2)
374-402: Prefer DTOs over raw maps for Swagger quality.Returning
Map<String, Object>weakens generated API docs. Consider a dedicated response DTO with@Schemafield descriptions for/config(and other map-based endpoints).Based on learnings, please ensure model property descriptions are added in source components for Swagger generation.
594-621: Consider a lightweight service call for health checks.The current check doesn’t exercise the service or its dependencies. A minimal non-mutating call (if available) would make the health signal more reliable.
jans-fido2/server/src/main/java/io/jans/fido2/service/shared/MetricService.java (1)
527-622: Proxy-header trust is still unconditional.
The TODOs note the risk, but proxy headers are still trusted without a config gate or trusted-proxy validation. Please wire a config flag before shipping.
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java
Show resolved
Hide resolved
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java
Show resolved
Hide resolved
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java
Show resolved
Hide resolved
Signed-off-by: imran <imranishaq7071@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java`:
- Around line 649-652: In hasTimezoneIndicator(String dateTime) remove the
redundant dateTime.contains("+") inside the nested expression and simplify the
boolean logic so it checks for 'Z' or '+' or a '-' that appears after the date
portion; specifically keep the checks using the method name hasTimezoneIndicator
and adjust the condition to: return dateTime.contains("Z") ||
dateTime.contains("+") || (dateTime.contains("-") && dateTime.lastIndexOf('-') >
10).
♻️ Duplicate comments (1)
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java (1)
619-642: Health check performs configuration access but doesn't actually test service functionality.The
checkServiceAvailabilitymethod readsappConfiguration.isFido2MetricsEnabled()(line 629) but discards the result. The method always returnstrueifmetricsService != nulland no exception occurs during configuration access. This doesn't verify that the metrics service can actually perform its core functions (e.g., database connectivity).Consider invoking a lightweight service method to validate operational readiness:
♻️ More meaningful availability check
private boolean checkServiceAvailability() { try { if (metricsService == null) { return false; } - // Perform a lightweight check: verify service can access configuration - // This tests that the service bean is properly initialized and can access dependencies - // We don't query the database to keep the health check fast - boolean metricsEnabled = appConfiguration.isFido2MetricsEnabled(); - - // If metrics are enabled, verify the service can handle a simple operation - // This is a minimal check that exercises the service without heavy database operations - return true; + // Perform a lightweight database connectivity check + // Query a minimal time range to verify service can execute operations + LocalDateTime now = ZonedDateTime.now(ZoneId.of("UTC")).toLocalDateTime(); + metricsService.getMetricsEntries(now.minusSeconds(1), now); + return true; } catch (RuntimeException e) { log.warn("Health check detected service issue: {}", e.getMessage()); return false; - } catch (Exception e) { - // Catch any other exceptions (e.g., configuration access issues) - log.debug("Health check encountered unexpected error: {}", e.getMessage()); - return false; + } catch (Exception e) { + log.warn("Health check detected service issue: {}", e.getMessage()); + return false; } }
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java
Show resolved
Hide resolved
Signed-off-by: imran <imranishaq7071@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java`:
- Around line 619-642: The health-check method checkServiceAvailability
currently assigns metricsEnabled = appConfiguration.isFido2MetricsEnabled() but
never uses it; remove the dead variable or use it to influence the result.
Update checkServiceAvailability to either (a) remove the metricsEnabled line
entirely and simply return true when metricsService != null (keeping existing
lightweight behavior), or (b) incorporate the flag by returning metricsService
!= null && appConfiguration.isFido2MetricsEnabled() so the health check reflects
the configuration; adjust logging (log.warn/log.debug) only as needed if you
choose option (b). Ensure references to metricsService and
appConfiguration.isFido2MetricsEnabled() in method checkServiceAvailability are
consistent and no unused variables remain.
♻️ Duplicate comments (2)
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java (2)
44-46: LGTM on the documented security approach.The security note in the Javadoc (lines 33-38) appropriately documents that authentication/authorization should be enforced at the infrastructure level. This aligns with the existing review comment and is a valid approach for this project.
378-401: Consider response DTOs for better API documentation.This is a duplicate of the existing suggestion. Using
Map<String, Object>works functionally but produces poor OpenAPI/Swagger documentation. Based on learnings for this repository, API model classes should have property descriptions that are reflected in auto-generated Swagger specifications.
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java
Show resolved
Hide resolved
Signed-off-by: imran <imranishaq7071@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java`:
- Around line 516-553: The methods normalizeAggregationType and
normalizeOperationType should trim the input before converting to uppercase and
validating to allow harmless surrounding whitespace; replace direct use of
aggregationType.toUpperCase() and operationType.toUpperCase() with a trimmed
variable (e.g., String trimmed = aggregationType.trim() / operationType.trim())
then convert trimmed.toUpperCase() and validate that upper-case trimmed value
against Fido2MetricsConstants, returning the upper-case trimmed value and
keeping the existing null/empty checks that already use trim().
- Around line 106-125: The handler
Fido2MetricsController.getMetricsEntriesByUser currently checks userId for
null/empty but doesn't trim it before use; trim the incoming userId at the start
of the method (e.g., String trimmedUserId = userId == null ? null :
userId.trim()), use trimmedUserId for the null/empty check and pass
trimmedUserId into metricsService.getMetricsEntriesByUser so lookups don't fail
due to accidental whitespace, leaving parseDateTime and validateTimeRange calls
unchanged.
♻️ Duplicate comments (1)
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java (1)
378-401: Prefer typed DTOs for /config response.Returning
Map<String, Object>hurts Swagger docs; a dedicated DTO with@Schemafield descriptions would make the API contract explicit and discoverable. Based on learnings, please add property descriptions in source model classes so they surface in generated Swagger.
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java
Show resolved
Hide resolved
jans-fido2/server/src/main/java/io/jans/fido2/ws/rs/controller/Fido2MetricsController.java
Show resolved
Hide resolved
Signed-off-by: imran <imranishaq7071@gmail.com>
|
Description
Adds FIDO2 metrics system with real-time collection, automated aggregations, and 13 REST API endpoints for analytics (adoption, performance, devices, errors, trends). Includes cluster-safe scheduler and optimized JSON responses.
Target issue
Implements FIDO2 metrics REST API with collection, aggregation scheduler, and analytics endpoints for monitoring Passkey usage and performance
closes #11922
Implementation Details
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.