feat(jans-config-api): FIDO2 metrics endpoints #13448
Open
Conversation
Signed-off-by: pujavs <pujas.works@gmail.com>
…ns-config-api-13226
…ns-config-api-13226
Signed-off-by: pujavs <pujas.works@gmail.com>
…ns-config-api-13226
Signed-off-by: pujavs <pujas.works@gmail.com>
Signed-off-by: pujavs <pujas.works@gmail.com>
…ns-config-api-13226
…ns-config-api-13226
Signed-off-by: pujavs <pujas.works@gmail.com>
…ns-config-api-13226
Signed-off-by: pujavs <pujas.works@gmail.com>
…ns-config-api-13226
Signed-off-by: pujavs <pujas.works@gmail.com>
Signed-off-by: pujavs <pujas.works@gmail.com>
…ns-config-api-13226
Signed-off-by: pujavs <pujas.works@gmail.com>
…ns-config-api-13226
Signed-off-by: pujavs <pujas.works@gmail.com>
…ns-config-api-13226
Signed-off-by: pujavs <pujas.works@gmail.com>
…ns-config-api-13226
Signed-off-by: pujavs <pujas.works@gmail.com>
Signed-off-by: pujavs <pujas.works@gmail.com>
…ns-config-api-13226
Signed-off-by: pujavs <pujas.works@gmail.com>
…ns-config-api-13226
Signed-off-by: pujavs <pujas.works@gmail.com>
Signed-off-by: pujavs <pujas.works@gmail.com>
…ns-config-api-13226
…ns-config-api-13226
Signed-off-by: pujavs <pujas.works@gmail.com>
…ns-config-api-13226
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java (6)
104-133:⚠️ Potential issue | 🟠 MajorDon't rewrite validation and upstream HTTP errors to 500.
validateDate()and the service call can already raise HTTP errors, but this catch-all converts everything intothrowInternalServerException(ex). That masks bad-request validation failures and propagated 4xx/5xx responses here, and the same catch pattern repeats across the rest of the resource.In JAX-RS, if a resource catches a `WebApplicationException` and converts it into a generic 500 response, does that replace the original HTTP status code returned to the client?Suggested fix
- } catch (Exception ex) { + } catch (WebApplicationException ex) { + throw ex; + } catch (Exception ex) { logger.error(ERR_MSG, ex); throwInternalServerException(ex); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 104 - 133, The current catch-all in Fido2MetricsResource around validateDate/parseDate/getFido2MetricsEntries swallows and converts any WebApplicationException (and other HTTP errors) into a 500 via throwInternalServerException; change the catch block to rethrow WebApplicationException instances unchanged (e.g., if (ex instanceof WebApplicationException) throw (WebApplicationException) ex;), and only log+wrap non-WebApplicationException errors with throwInternalServerException; reference validateDate, parseDate, fido2MetricsService.getFido2MetricsEntries and throwInternalServerException to locate the code to update.
405-410:⚠️ Potential issue | 🟡 MinorEither implement paging here or drop it from the contract.
limitandstartIndexare accepted, documented, and logged, but the handler still calls the service without them. The same no-op paging pattern repeats across the JSON summary/analytics/comparison endpoints, so clients always receive the full payload.Also applies to: 439-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 405 - 410, The handlers (starting with getFido2MetricsAggregationSummary) declare and log paging params limit and startIndex but never apply them when calling the service; either wire these params through to the service call (e.g., add limit and startIndex arguments to the service method invocation and ensure the service uses them to page results) or remove the paging params, docs and logs from the method signature to match the contract. Apply the same fix to the other aggregation endpoints that accept paging (the JSON/analytics/comparison aggregation handlers) so the API either supports paging end-to-end or does not expose no-op paging parameters.
95-96:⚠️ Potential issue | 🟠 MajorRemove
@NotNullfrom optional date filters.These params are documented and handled as optional, but
@NotNullwill reject requests that omit them before theStringUtils.isNotBlank()checks run. The same annotation pattern is copied across the other date-filtered metrics endpoints.In Jakarta REST with Bean Validation enabled, does annotating an optional `@QueryParam String` with `@NotNull` cause requests that omit that query parameter to fail validation before the resource method executes?Also applies to: 114-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 95 - 96, The method parameters startDate and endDate in Fido2MetricsResource are annotated with `@NotNull` which causes Bean Validation to reject requests that omit these optional query params before the method logic (StringUtils.isNotBlank checks) runs; remove the `@NotNull` annotation from the `@QueryParam` parameters (startDate, endDate) in the affected method and remove the same `@NotNull` usage from the other date-filtered metric endpoints (the other parameter list around lines 114-122) so the parameters remain optional and are validated/handled inside the method body.
175-179:⚠️ Potential issue | 🟠 MajorRedact
userIdin the info log.This writes caller-supplied user identifiers into standard logs. If you need correlation, log a redacted/hash form or move the detailed value to debug.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 175 - 179, The info log in Fido2MetricsResource (logger.info call building "Fido2MetricsEntry for user search param ...") emits the raw caller-supplied userId; change it to avoid logging the full identifier by replacing the plain userId with a redacted or hashed form (e.g., mask characters or compute a one-way hash) or move the full value to a debug-only log; update the escapeLog(userId) usage in that logger.info invocation so it logs e.g. escapeLog(redactOrHash(userId)) and, if needed, add a separate logger.debug that outputs the original escaped userId for troubleshooting.
1027-1042:⚠️ Potential issue | 🟠 MajorThe paged-result helpers still treat
startIndexas 0-based and misreport page size.The API says
startIndexis 1-based, but these helpers validate and slice with the raw value.startIndex=1skips the first row, single-item/empty results can still fail during pagination, andentriesCountis set totoIndexinstead of the number of returned entries.Suggested fix
- getStartIndex(identityProviderList, startIndex); - int toIndex = (startIndex + limit <= listSize) ? startIndex + limit : listSize; + int fromIndex = getStartIndex(identityProviderList, startIndex); + int toIndex = Math.min(fromIndex + limit, listSize); logger.info("Final startIndex:{}, limit:{}, toIndex:{}", startIndex, limit, toIndex); // Extract paginated data List<Fido2MetricsEntry> sublist = (identityProviderList == null) ? null - : identityProviderList.subList(startIndex, toIndex); + : identityProviderList.subList(fromIndex, toIndex); fido2MetricsEntryPagedResult = new Fido2MetricsEntryPagedResult(); fido2MetricsEntryPagedResult.setStart(startIndex); - fido2MetricsEntryPagedResult.setEntriesCount(toIndex); + fido2MetricsEntryPagedResult.setEntriesCount(sublist == null ? 0 : sublist.size());private int getStartIndex(List<?> entriesList, int startIndex) { + if (startIndex < 1) { + throwBadRequestException("Page start index must be >= 1"); + } + int zeroBasedStart = startIndex - 1; if (logger.isDebugEnabled()) { logger.debug("Get startIndex entriesList:{}, startIndex:{}", entriesList, escapeLog(startIndex)); } if (entriesList != null && !entriesList.isEmpty()) { logger.debug("Get startIndex entriesList.size():{}", entriesList.size()); try { - entriesList.get(startIndex); + entriesList.get(zeroBasedStart); } catch (IndexOutOfBoundsException ioe) { logger.error("Error while getting data startIndex:{}", startIndex, ioe); throwBadRequestException("Page start index incorrect, total entries:{" + entriesList.size() + "}, but provided:{" + startIndex + "} "); } } - return startIndex; + return zeroBasedStart; }Also applies to: 1056-1071, 1080-1095
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 1027 - 1042, The pagination treats startIndex as 0-based and miscomputes counts; update Fido2MetricsResource to convert the incoming 1-based startIndex to a 0-based offset before slicing and bounds-checking (e.g., startIndex = getStartIndex(identityProviderList, startIndex) or have getStartIndex return the adjusted zero-based index), compute toIndex = Math.min(startIndex + limit, listSize) using that zero-based start, guard for null/empty identityProviderList so sublist uses safe indices, and set entriesCount to the actual number of returned entries (sublist == null ? 0 : sublist.size()) instead of toIndex; apply the same fixes to the other similar blocks around the 1056-1071 and 1080-1095 sections.
1010-1018:⚠️ Potential issue | 🟠 Major
parseDate()still rejects the advertised date-only formats.
LocalDateTime.parseneeds time fields, so theISO_LOCAL_DATEanddd-MM-yyyybranches will throw for inputs like2026-01-31or31-01-2026. That breaks the documented contract for every endpoint using this helper.Can `LocalDateTime.parse` in Java parse a date-only string such as `"2026-01-31"` with `DateTimeFormatter.ISO_LOCAL_DATE`, or `"31-01-2026"` with `DateTimeFormatter.ofPattern("dd-MM-yyyy")`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 1010 - 1018, parseDate() currently calls LocalDateTime.parse with date-only formatters which throws for inputs like "2026-01-31"; instead detect and parse date-only strings with LocalDate.parse (using DateTimeFormatter.ISO_LOCAL_DATE and DateTimeFormatter.ofPattern(METRICS_DATE_FORMAT)) and then convert to a LocalDateTime (e.g., .atStartOfDay()) before returning; update the ISO_LOCAL_DATE and METRICS_DATE_FORMAT branches in parseDate() to use LocalDate.parse(...).atStartOfDay() to accept advertised date-only inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jans-config-api/plugins/docs/fido2-plugin-swagger.yaml`:
- Around line 292-298: The path parameter aggregationType is marked required but
has a default of "" which is semantically incorrect; remove the default or
replace it with an explicit enum of allowed values (e.g., HOURLY, DAILY, WEEKLY,
MONTHLY) to document valid inputs and keep required: true. Update every
occurrence of the aggregationType path parameter in the spec (the
aggregationType parameter definitions you added) to remove default: "" and, if
appropriate, add a schema.enum listing the permitted aggregation values so
clients and generators validate correctly.
- Around line 2136-2147: The OAuth2 clientCredentials scopes block in the
Swagger YAML is missing the scope "https://jans.io/oauth/config/fido2.admin"
which is referenced by Constants.java and used via
`@SecurityRequirement/`@ProtectedApi on Fido2ConfigResource,
Fido2RegistrationResource, and Fido2MetricsResource; regenerate or update the
Swagger components.securitySchemes.oauth2.flows.clientCredentials.scopes to
include the "https://jans.io/oauth/config/fido2.admin" entry (with a brief
description like "Administer fido2 config") so the YAML matches the Java
annotations and constants.
---
Duplicate comments:
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java`:
- Around line 104-133: The current catch-all in Fido2MetricsResource around
validateDate/parseDate/getFido2MetricsEntries swallows and converts any
WebApplicationException (and other HTTP errors) into a 500 via
throwInternalServerException; change the catch block to rethrow
WebApplicationException instances unchanged (e.g., if (ex instanceof
WebApplicationException) throw (WebApplicationException) ex;), and only log+wrap
non-WebApplicationException errors with throwInternalServerException; reference
validateDate, parseDate, fido2MetricsService.getFido2MetricsEntries and
throwInternalServerException to locate the code to update.
- Around line 405-410: The handlers (starting with
getFido2MetricsAggregationSummary) declare and log paging params limit and
startIndex but never apply them when calling the service; either wire these
params through to the service call (e.g., add limit and startIndex arguments to
the service method invocation and ensure the service uses them to page results)
or remove the paging params, docs and logs from the method signature to match
the contract. Apply the same fix to the other aggregation endpoints that accept
paging (the JSON/analytics/comparison aggregation handlers) so the API either
supports paging end-to-end or does not expose no-op paging parameters.
- Around line 95-96: The method parameters startDate and endDate in
Fido2MetricsResource are annotated with `@NotNull` which causes Bean Validation to
reject requests that omit these optional query params before the method logic
(StringUtils.isNotBlank checks) runs; remove the `@NotNull` annotation from the
`@QueryParam` parameters (startDate, endDate) in the affected method and remove
the same `@NotNull` usage from the other date-filtered metric endpoints (the other
parameter list around lines 114-122) so the parameters remain optional and are
validated/handled inside the method body.
- Around line 175-179: The info log in Fido2MetricsResource (logger.info call
building "Fido2MetricsEntry for user search param ...") emits the raw
caller-supplied userId; change it to avoid logging the full identifier by
replacing the plain userId with a redacted or hashed form (e.g., mask characters
or compute a one-way hash) or move the full value to a debug-only log; update
the escapeLog(userId) usage in that logger.info invocation so it logs e.g.
escapeLog(redactOrHash(userId)) and, if needed, add a separate logger.debug that
outputs the original escaped userId for troubleshooting.
- Around line 1027-1042: The pagination treats startIndex as 0-based and
miscomputes counts; update Fido2MetricsResource to convert the incoming 1-based
startIndex to a 0-based offset before slicing and bounds-checking (e.g.,
startIndex = getStartIndex(identityProviderList, startIndex) or have
getStartIndex return the adjusted zero-based index), compute toIndex =
Math.min(startIndex + limit, listSize) using that zero-based start, guard for
null/empty identityProviderList so sublist uses safe indices, and set
entriesCount to the actual number of returned entries (sublist == null ? 0 :
sublist.size()) instead of toIndex; apply the same fixes to the other similar
blocks around the 1056-1071 and 1080-1095 sections.
- Around line 1010-1018: parseDate() currently calls LocalDateTime.parse with
date-only formatters which throws for inputs like "2026-01-31"; instead detect
and parse date-only strings with LocalDate.parse (using
DateTimeFormatter.ISO_LOCAL_DATE and
DateTimeFormatter.ofPattern(METRICS_DATE_FORMAT)) and then convert to a
LocalDateTime (e.g., .atStartOfDay()) before returning; update the
ISO_LOCAL_DATE and METRICS_DATE_FORMAT branches in parseDate() to use
LocalDate.parse(...).atStartOfDay() to accept advertised date-only inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4bc8f5d-9f14-43c4-9c8d-50872d92429a
📒 Files selected for processing (3)
jans-config-api/docs/jans-config-api-swagger.yamljans-config-api/plugins/docs/fido2-plugin-swagger.yamljans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java
Signed-off-by: pujavs <pujas.works@gmail.com>
…ns-config-api-13226
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
jans-config-api/plugins/docs/fido2-plugin-swagger.yaml (1)
2083-2094:⚠️ Potential issue | 🟠 MajorDeclare the
config/fido2.adminscope in the OAuth2 scheme.These operations use
https://jans.io/oauth/config/fido2.adminin theirsecurityblocks, butclientCredentials.scopesnever declares it here. That leaves the checked-in OpenAPI contract incomplete for generators and consumers.🧾 Proposed fix
scopes: https://jans.io/oauth/config/fido2.readonly: View fido2 config related information https://jans.io/oauth/config/fido2.write: Manage fido2 config related information https://jans.io/oauth/config/fido2.delete: Delete fido2 config related information + https://jans.io/oauth/config/fido2.admin: Administer fido2 config related + information https://jans.io/oauth/fido2.readonly: View fido2 related information https://jans.io/oauth/fido2.write: Manage fido2 related information https://jans.io/oauth/fido2.delete: Delete fido2 related information🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/docs/fido2-plugin-swagger.yaml` around lines 2083 - 2094, The OAuth2 clientCredentials.scopes block is missing the admin scope used by operations; add the scope "https://jans.io/oauth/config/fido2.admin" to the same scopes list (clientCredentials.scopes) alongside the existing config/fido2.readonly, config/fido2.write, and config/fido2.delete entries and give it a short description such as "Administer fido2 config related information" so the OpenAPI contract includes the admin permission referenced in security blocks.jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java (5)
175-179:⚠️ Potential issue | 🟠 MajorDo not log raw
userIdvalues at info level.This writes caller-supplied identifiers into the standard request log. Redact it, hash it, or move it to debug.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 175 - 179, In Fido2MetricsResource, stop emitting raw userId at info level: update the logger.info call that currently includes escapeLog(userId) to either remove userId from the info message or replace it with a non-reversible identifier (e.g., a hashed or redacted value produced by a new helper like hashOrRedactUserId(userId)), and adjust the log format arguments accordingly; if full userId is still needed for troubleshooting, move the unredacted value to logger.debug with appropriate escaping instead.
95-96:⚠️ Potential issue | 🟠 MajorRemove
@NotNullfrom the optional date filters.Every one of these handlers treats
start_date/end_dateas optional in the method body, but@NotNullmakes omission fail before the handler runs. If omitted dates are intentional, drop the annotation here and update the generated contract so these query params are no longer marked required.🧾 Representative fix
- `@Parameter`(description = "Start date/time for " - + " entries report. Accepted format dd-MM-yyyy or ISO-8601 date-time (e.g. yyyy-MM-ddTHH:mm:ssZ).", schema = `@Schema`(type = "string")) `@QueryParam`(value = "start_date") `@NotNull` String startDate, - `@Parameter`(description = "End date/time for the log entries. Accepted format dd-MM-yyyy or ISO-8601 date-time (e.g. yyyy-MM-ddTHH:mm:ssZ).", schema = `@Schema`(type = "string")) `@QueryParam`(value = "end_date") `@NotNull` String endDate) + `@Parameter`(description = "Start date/time for " + + " entries report. Accepted format dd-MM-yyyy or ISO-8601 date-time (e.g. yyyy-MM-ddTHH:mm:ssZ).", schema = `@Schema`(type = "string")) `@QueryParam`(value = "start_date") String startDate, + `@Parameter`(description = "End date/time for the log entries. Accepted format dd-MM-yyyy or ISO-8601 date-time (e.g. yyyy-MM-ddTHH:mm:ssZ).", schema = `@Schema`(type = "string")) `@QueryParam`(value = "end_date") String endDate)In Jakarta REST / Bean Validation, does annotating a `@QueryParam` with `@NotNull` cause requests that omit that query parameter to fail validation before the resource method executes?Also applies to: 171-172, 250-251, 330-331, 409-410, 483-484, 553-554, 623-624, 693-694, 764-765
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 95 - 96, Remove the `@NotNull` annotations from the `@QueryParam` parameters named start_date / end_date in Fido2MetricsResource so the optional date filters are not validated as required; specifically edit the method signatures in class Fido2MetricsResource to delete `@NotNull` on the startDate and endDate parameters (and the other repeated occurrences noted in the review) and regenerate/update the OpenAPI contract so those query params are no longer marked required in the generated API spec.
67-69:⚠️ Potential issue | 🟡 MinorThe code uses
startIndexas 0-based, not 1-based.The default is
0, and the paginator passesstartIndexstraight intosubList(startIndex, toIndex). A client following the documented 1-based contract will page incorrectly. Please align the docs/Javadocs (and the checked-in Swagger) with the actual behavior, or convert to 0-based before slicing.Also applies to: 92-93, 1021-1042
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 67 - 69, Javadoc and Swagger state startIndex is 1-based but the implementation in Fido2MetricsResource passes startIndex directly into the paginator/subList, which is 0-based; either update the Javadoc/Swagger to document startIndex as 0-based everywhere or convert the incoming startIndex to 0-based before slicing (e.g., in the method handling the request in Fido2MetricsResource decrement startIndex by 1 when startIndex>0) and adjust any bounds checks accordingly; ensure the same change is applied to the other affected occurrences noted (around the paginator/subList usage and the related method signatures).
104-134:⚠️ Potential issue | 🟠 MajorDon't convert validation failures into 500s.
validateDate()callsthrowBadRequestException(...), but this blanketcatch (Exception)rewrites that intothrowInternalServerException(ex). Invalid client input will therefore surface as 500 instead of 400. The same catch-all pattern repeats throughout the class.🧾 Proposed fix
- } catch (Exception ex) { + } catch (WebApplicationException ex) { + throw ex; + } catch (Exception ex) { logger.error(ERR_MSG, ex); throwInternalServerException(ex); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 104 - 134, The current broad catch (Exception) in methods like the block around validateDate(...) swallows validation errors (thrown by validateDate/throwBadRequestException) and converts them to 500s; change the catch to rethrow client errors and only translate unexpected exceptions to 500s — e.g., replace catch (Exception ex) with logic that if ex is an instance of BadRequestException or WebApplicationException (or whatever type throwBadRequestException produces) then rethrow it, otherwise log and call throwInternalServerException(ex); apply the same pattern to the other catch-all handlers in this class (including references to validateDate, parseDate, throwBadRequestException, and throwInternalServerException).
991-1018:⚠️ Potential issue | 🔴 CriticalDate-only inputs never parse in
parseDate().
LocalDateTime.parse(..., ISO_LOCAL_DATE)and thedd-MM-yyyybranch both require a time component, so the two advertised date-only formats are rejected for every endpoint that uses this helper.🧾 Proposed fix
+import java.time.LocalDate; import java.time.LocalDateTime; import java.time.ZonedDateTime; @@ - try { - return LocalDateTime.parse(trimmed, DateTimeFormatter.ISO_LOCAL_DATE); + try { + return LocalDate.parse(trimmed, DateTimeFormatter.ISO_LOCAL_DATE).atStartOfDay(); } catch (DateTimeParseException ignored) { // continue } // Fallback to legacy dd-MM-yyyy - return LocalDateTime.parse(trimmed, DateTimeFormatter.ofPattern(METRICS_DATE_FORMAT)); + return LocalDate.parse(trimmed, fallbackFormatter).atStartOfDay();In Java, can `LocalDateTime.parse("2026-02-13", DateTimeFormatter.ISO_LOCAL_DATE)` or `LocalDateTime.parse("13-02-2026", DateTimeFormatter.ofPattern("dd-MM-yyyy"))` succeed, or do those formats need to be parsed as `LocalDate` and then converted with `atStartOfDay()`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 991 - 1018, The parseDate method currently calls LocalDateTime.parse with ISO_LOCAL_DATE and the METRICS_DATE_FORMAT which both require time parts and thus fail for date-only strings; update parseDate (the method parseDate in Fido2MetricsResource) so that when a date-only parse is intended (ISO_LOCAL_DATE or the dd-MM-yyyy legacy format referenced by METRICS_DATE_FORMAT) you call LocalDate.parse(...) and then convert to LocalDateTime via atStartOfDay() before returning; keep the existing ZonedDateTime/ISO_OFFSET_DATE_TIME and ISO_LOCAL_DATE_TIME branches unchanged and only switch the date-only branches to parse-as-LocalDate then atStartOfDay().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jans-config-api/docs/jans-config-api-swagger.yaml`:
- Around line 18351-18352: The Swagger spec documents a "persisted" property
that is transient in the model (SessionId.java -> persisted marked `@Transient`
and transient) and therefore not serialized by Jackson; either remove the
persisted property from the OpenAPI spec (jans-config-api-swagger.yaml) to match
runtime behavior, or update the Jackson configuration in
ObjectMapperContextResolver to enable serialization of transient fields (so
transient fields are included) and regenerate the Swagger if this field must be
part of the API contract; reference SessionId.java -> persisted and
ObjectMapperContextResolver when making the change so the model, serializer, and
spec remain consistent.
In `@jans-config-api/plugins/docs/fido2-plugin-swagger.yaml`:
- Around line 832-841: The response example for the 200 response references the
schema Fido2MetricsEntryPagedResult but uses an empty string value (""), which
is invalid JSON for that schema; update the example under "Response example" to
be a JSON object that matches the Fido2MetricsEntryPagedResult schema (or remove
the example so the validator uses the schema), ensuring keys/types align with
the components/schemas/Fido2MetricsEntryPagedResult definition so strict OpenAPI
validators and code generators accept it.
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java`:
- Around line 1021-1045: The paging logic in getFido2MetricsEntryPagedResult
allows calls to identityProviderList.subList(startIndex,toIndex) when the list
is null/empty or startIndex is out of bounds; ensure startIndex is validated and
clamped before calling subList by (1) checking identityProviderList != null &&
!identityProviderList.isEmpty(), (2) computing a safeStart = Math.max(0,
Math.min(startIndex, listSize)) (or invoking getStartIndex and using its
returned/clamped value), (3) computing toIndex = Math.max(safeStart,
Math.min(safeStart + limit, listSize)), and (4) only calling
subList(safeStart,toIndex) when safeStart < toIndex; apply the same bounds
validation pattern to the other similar blocks noted (the other paged-result
methods around the later ranges).
---
Duplicate comments:
In `@jans-config-api/plugins/docs/fido2-plugin-swagger.yaml`:
- Around line 2083-2094: The OAuth2 clientCredentials.scopes block is missing
the admin scope used by operations; add the scope
"https://jans.io/oauth/config/fido2.admin" to the same scopes list
(clientCredentials.scopes) alongside the existing config/fido2.readonly,
config/fido2.write, and config/fido2.delete entries and give it a short
description such as "Administer fido2 config related information" so the OpenAPI
contract includes the admin permission referenced in security blocks.
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java`:
- Around line 175-179: In Fido2MetricsResource, stop emitting raw userId at info
level: update the logger.info call that currently includes escapeLog(userId) to
either remove userId from the info message or replace it with a non-reversible
identifier (e.g., a hashed or redacted value produced by a new helper like
hashOrRedactUserId(userId)), and adjust the log format arguments accordingly; if
full userId is still needed for troubleshooting, move the unredacted value to
logger.debug with appropriate escaping instead.
- Around line 95-96: Remove the `@NotNull` annotations from the `@QueryParam`
parameters named start_date / end_date in Fido2MetricsResource so the optional
date filters are not validated as required; specifically edit the method
signatures in class Fido2MetricsResource to delete `@NotNull` on the startDate and
endDate parameters (and the other repeated occurrences noted in the review) and
regenerate/update the OpenAPI contract so those query params are no longer
marked required in the generated API spec.
- Around line 67-69: Javadoc and Swagger state startIndex is 1-based but the
implementation in Fido2MetricsResource passes startIndex directly into the
paginator/subList, which is 0-based; either update the Javadoc/Swagger to
document startIndex as 0-based everywhere or convert the incoming startIndex to
0-based before slicing (e.g., in the method handling the request in
Fido2MetricsResource decrement startIndex by 1 when startIndex>0) and adjust any
bounds checks accordingly; ensure the same change is applied to the other
affected occurrences noted (around the paginator/subList usage and the related
method signatures).
- Around line 104-134: The current broad catch (Exception) in methods like the
block around validateDate(...) swallows validation errors (thrown by
validateDate/throwBadRequestException) and converts them to 500s; change the
catch to rethrow client errors and only translate unexpected exceptions to 500s
— e.g., replace catch (Exception ex) with logic that if ex is an instance of
BadRequestException or WebApplicationException (or whatever type
throwBadRequestException produces) then rethrow it, otherwise log and call
throwInternalServerException(ex); apply the same pattern to the other catch-all
handlers in this class (including references to validateDate, parseDate,
throwBadRequestException, and throwInternalServerException).
- Around line 991-1018: The parseDate method currently calls LocalDateTime.parse
with ISO_LOCAL_DATE and the METRICS_DATE_FORMAT which both require time parts
and thus fail for date-only strings; update parseDate (the method parseDate in
Fido2MetricsResource) so that when a date-only parse is intended (ISO_LOCAL_DATE
or the dd-MM-yyyy legacy format referenced by METRICS_DATE_FORMAT) you call
LocalDate.parse(...) and then convert to LocalDateTime via atStartOfDay() before
returning; keep the existing ZonedDateTime/ISO_OFFSET_DATE_TIME and
ISO_LOCAL_DATE_TIME branches unchanged and only switch the date-only branches to
parse-as-LocalDate then atStartOfDay().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 74750e75-c9c5-482e-97ee-4603f2efc1a6
📒 Files selected for processing (3)
jans-config-api/docs/jans-config-api-swagger.yamljans-config-api/plugins/docs/fido2-plugin-swagger.yamljans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java
...ins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java
Show resolved
Hide resolved
Signed-off-by: pujavs <pujas.works@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
jans-config-api/server/src/main/resources/example/fido2/metrics/fido2-analytics-trends-aggregation-type.json (1)
4-4:⚠️ Potential issue | 🟠 MajorAvoid encoding "no data" as 0% success rate.
This is the same issue flagged in the previous review:
avgAuthenticationSuccessRate: 0is misleading when Lines 24-26 show zero authentication attempts. A value of0implies "all attempts failed," not "no data available." Usenullor omit the field when the denominator is zero to avoid skewing dashboards and trend calculations during idle periods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/server/src/main/resources/example/fido2/metrics/fido2-analytics-trends-aggregation-type.json` at line 4, The JSON metric field "avgAuthenticationSuccessRate" is currently set to 0 which incorrectly implies a 0% success rate when there were no authentication attempts; update the generator/serialization so that when the authentication attempt denominator is zero it either sets "avgAuthenticationSuccessRate" to null or omits the field entirely instead of writing 0 — locate where "avgAuthenticationSuccessRate" is produced in the metrics aggregation logic and add a check for zero attempts (or NaN) to return null/skip the property.jans-config-api/server/src/main/resources/example/fido2/metrics/fido2-analytics-performance-metrics.json (1)
2-4:⚠️ Potential issue | 🟡 MinorUse a realistic duration spread in this fixture.
registrationAvgDuration,registrationMaxDuration, andregistrationMinDurationare all identical here, so the example reads like a single-sample edge case instead of representative performance data.Suggested fix
{ - "registrationAvgDuration": 471, - "registrationMaxDuration": 471, - "registrationMinDuration": 471 + "registrationAvgDuration": 450, + "registrationMaxDuration": 850, + "registrationMinDuration": 120 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/server/src/main/resources/example/fido2/metrics/fido2-analytics-performance-metrics.json` around lines 2 - 4, The three duration fields registrationAvgDuration, registrationMaxDuration, and registrationMinDuration currently have identical values; update them to represent a realistic spread (registrationMinDuration < registrationAvgDuration < registrationMaxDuration) — e.g., pick a lower min (e.g., ~200–350), a higher max (e.g., ~600–900), and set avg to a plausible midpoint (e.g., ~400–600) so the fixture reflects typical performance variance; adjust only the values of registrationMinDuration, registrationAvgDuration, and registrationMaxDuration to maintain internal consistency.jans-config-api/docs/jans-config-api-swagger.yaml (2)
18535-18536: 🧹 Nitpick | 🔵 TrivialAdd description for
accessTokenproperty in source model.The
accessTokenboolean property inTokenAttributeslacks a description. API consumers won't know whattruevsfalsesignifies. Per repository conventions, add a@Schema(description="...")annotation in the upstream model class (likely in jans-auth component) so the description propagates to this auto-generated Swagger.Based on learnings: "When reviewing components like
jans-auth,jans-fido2,jans-lockin the JanssenProject/jans repository, raise issues about missing property descriptions in API model classes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/docs/jans-config-api-swagger.yaml` around lines 18535 - 18536, The TokenAttributes model's boolean property accessToken needs a descriptive annotation: locate the TokenAttributes class in the jans-auth model sources and add a `@Schema`(description="...") on the accessToken field explaining what true/false means (e.g., whether the token represents an access token or whether access token issuance is enabled), so the description propagates into the generated Swagger (ensure you update the exact field named accessToken in TokenAttributes).
16632-16633: 🧹 Nitpick | 🔵 TrivialConsider adding description for
fapiproperty in source model.The
fapiboolean property lacks a description. Based on the AI summary, this indicates "support for Financial-grade API (FAPI) standards." If this property is within the scope of jans-config-api (not a dependency object), adding a@Schema(description="...")annotation in the source model class would clarify its purpose for API consumers.Since this Swagger file is auto-generated, any description should be added in the upstream source component.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/docs/jans-config-api-swagger.yaml` around lines 16632 - 16633, The OpenAPI `fapi` boolean property lacks a description in the source model; add a `@Schema`(description="Support for Financial-grade API (FAPI) standards - when true, enables FAPI-related behaviors") (or similar clear text) to the field definition in the source model class that declares the `fapi` property, then re-run the Swagger/OpenAPI generation so the docs include the description; target the field named `fapi` in the model rather than editing the generated yaml directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@jans-config-api/server/src/main/resources/example/fido2/metrics/fido2-analytics-trends-aggregation-type.json`:
- Line 10: The "growthRate" field currently set to 0 is ambiguous given only one
entry in the "dataPoints" array; update the JSON so that when there are
insufficient historical data points to compute a trend (i.e., length of
"dataPoints" <= 1) the "growthRate" value is set to null (or the field omitted)
instead of 0 — locate the "growthRate" key in the
fido2-analytics-trends-aggregation-type.json example and change its value to
null (or remove the key) so consumers can distinguish "no change" from "cannot
compute".
---
Duplicate comments:
In `@jans-config-api/docs/jans-config-api-swagger.yaml`:
- Around line 18535-18536: The TokenAttributes model's boolean property
accessToken needs a descriptive annotation: locate the TokenAttributes class in
the jans-auth model sources and add a `@Schema`(description="...") on the
accessToken field explaining what true/false means (e.g., whether the token
represents an access token or whether access token issuance is enabled), so the
description propagates into the generated Swagger (ensure you update the exact
field named accessToken in TokenAttributes).
- Around line 16632-16633: The OpenAPI `fapi` boolean property lacks a
description in the source model; add a `@Schema`(description="Support for
Financial-grade API (FAPI) standards - when true, enables FAPI-related
behaviors") (or similar clear text) to the field definition in the source model
class that declares the `fapi` property, then re-run the Swagger/OpenAPI
generation so the docs include the description; target the field named `fapi` in
the model rather than editing the generated yaml directly.
In
`@jans-config-api/server/src/main/resources/example/fido2/metrics/fido2-analytics-performance-metrics.json`:
- Around line 2-4: The three duration fields registrationAvgDuration,
registrationMaxDuration, and registrationMinDuration currently have identical
values; update them to represent a realistic spread (registrationMinDuration <
registrationAvgDuration < registrationMaxDuration) — e.g., pick a lower min
(e.g., ~200–350), a higher max (e.g., ~600–900), and set avg to a plausible
midpoint (e.g., ~400–600) so the fixture reflects typical performance variance;
adjust only the values of registrationMinDuration, registrationAvgDuration, and
registrationMaxDuration to maintain internal consistency.
In
`@jans-config-api/server/src/main/resources/example/fido2/metrics/fido2-analytics-trends-aggregation-type.json`:
- Line 4: The JSON metric field "avgAuthenticationSuccessRate" is currently set
to 0 which incorrectly implies a 0% success rate when there were no
authentication attempts; update the generator/serialization so that when the
authentication attempt denominator is zero it either sets
"avgAuthenticationSuccessRate" to null or omits the field entirely instead of
writing 0 — locate where "avgAuthenticationSuccessRate" is produced in the
metrics aggregation logic and add a check for zero attempts (or NaN) to return
null/skip the property.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f4488d07-9290-4feb-949c-091c84b7d09c
📒 Files selected for processing (17)
jans-config-api/docs/jans-config-api-swagger.yamljans-config-api/plugins/docs/fido2-plugin-swagger.yamljans-config-api/plugins/docs/user-mgt-plugin-swagger.yamljans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.javajans-config-api/server/src/main/resources/example/fido2/metrics/fido2-analytics-performance-metrics.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-analytics-trends-aggregation-type.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-adoption.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-aggregation-summary.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-aggregations-type.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-analytics-devices.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-analytics-errors.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-by-operation-type.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-by-user.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-configuration.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-data.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-health-check.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-period-over-period-comparison.json
...server/src/main/resources/example/fido2/metrics/fido2-analytics-trends-aggregation-type.json
Show resolved
Hide resolved
Contributor
Author
|
@coderabbitai request you to please review again |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (21)
jans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-analytics-errors.json (1)
1-6:⚠️ Potential issue | 🟠 MajorAdd the missing
completionRateanddropOffRatefields.This example JSON is incomplete. According to the previous review, the
MetricsErrorAnalyticsresponse schema requirescompletionRateanddropOffRatefields that are always populated by the FIDO2 metrics service but are absent from this example.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-analytics-errors.json` around lines 1 - 6, The example JSON for the MetricsErrorAnalytics response is missing the required numeric fields completionRate and dropOffRate; update the example object that currently contains errorCategories, failureRate, successRate, and topErrors by adding completionRate and dropOffRate (as numeric values consistent with failureRate/successRate) so the example matches the MetricsErrorAnalytics schema (referencing the MetricsErrorAnalytics response and the fields completionRate and dropOffRate).jans-config-api/shared/src/main/java/io/jans/configapi/core/rest/BaseResource.java (1)
294-304:⚠️ Potential issue | 🟠 MajorEscape the caller-supplied fragment before building the regex.
patternis still appended verbatim into a regex, so inputs like[or*.jsoncan either fail when compiled or change the match semantics. Quote the user fragment before wrapping it with^.*/.*$.🔧 Proposed fix
import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.util.regex.Pattern; ... public String getRegexPattern(String pattern) { String searchPattern = DEFAULT_SEARCH_PATTERN; if (StringUtils.isNotBlank(pattern)) { StringBuilder stringBuilder = new StringBuilder(); stringBuilder.append("^.*"); - stringBuilder.append(pattern); + stringBuilder.append(Pattern.quote(pattern)); stringBuilder.append(".*$"); searchPattern = stringBuilder.toString(); } return searchPattern; }#!/bin/bash set -euo pipefail echo "== Current implementation ==" fd "BaseResource.java" --type f --exec sed -n '294,304p' {} echo echo "== Call sites of getRegexPattern(...) ==" rg -nP --type java -C2 '\bgetRegexPattern\s*\(' echo echo "== Generic regex metacharacter demonstration ==" python3 <<'PY' import re samples = ["[", "*.json", "a+b"] for raw in samples: expr = f"^.*{raw}.*$" print(f"\ninput={raw!r} regex={expr!r}") try: re.compile(expr) print("compiled: yes (but metacharacters alter semantics)") except re.error as e: print(f"compiled: no -> {e}") PYExpected result: the method body shows raw concatenation, and the demonstration highlights that unescaped metacharacters either fail compilation or stop behaving as literal text.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/shared/src/main/java/io/jans/configapi/core/rest/BaseResource.java` around lines 294 - 304, The getRegexPattern method appends the caller-supplied pattern verbatim into a regex, which can break or change semantics for metacharacters; fix it by quoting the user fragment with java.util.regex.Pattern.quote(...) (i.e., call Pattern.quote(pattern) before wrapping with "^.*" and ".*$"), keep the existing blank-check logic, and add the necessary import for java.util.regex.Pattern so compiled regexes treat the fragment as a literal.jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java (1)
24-24:⚠️ Potential issue | 🟠 MajorSecurity: Do not log bearer tokens.
The
accessTokenis logged at ERROR level. Test logs are typically retained in CI systems, which would expose credentials. This applies to lines 24, 39, and 54.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java` at line 24, The test currently logs the raw bearer token via log.error (see the log.error(...) call referencing accessToken in Fido2MetricsTest.java and similar calls at lines 39 and 54); remove the raw accessToken from all test log messages and either omit it entirely or replace it with a safe/masked representation (e.g., show only last N chars or a fixed placeholder) in the log statements that reference accessToken so that no full bearer token is ever written to test logs.jans-config-api/plugins/docs/fido2-plugin-swagger.yaml (2)
1978-1989:⚠️ Potential issue | 🟠 MajorDeclare
https://jans.io/oauth/config/fido2.adminin the OAuth2 scope registry.The new operations reference this scope in their
securityblocks, butcomponents.securitySchemes.oauth2.flows.clientCredentials.scopesstill omits it. That leaves the document inconsistent and trips strict validators/generators.Suggested fix
scopes: https://jans.io/oauth/config/fido2.readonly: View fido2 config related information https://jans.io/oauth/config/fido2.write: Manage fido2 config related information https://jans.io/oauth/config/fido2.delete: Delete fido2 config related information + https://jans.io/oauth/config/fido2.admin: Administer fido2 config related + information https://jans.io/oauth/fido2.readonly: View fido2 related information https://jans.io/oauth/fido2.write: Manage fido2 related information https://jans.io/oauth/fido2.delete: Delete fido2 related information https://jans.io/oauth/fido2-metrics.readonly: View fido2 metrics related information🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/docs/fido2-plugin-swagger.yaml` around lines 1978 - 1989, Add the missing OAuth2 scope declaration "https://jans.io/oauth/config/fido2.admin" to components.securitySchemes.oauth2.flows.clientCredentials.scopes so the registry matches the scopes referenced by the FIDO2 operation security blocks; update the scopes list in the YAML under the clientCredentials flow to include that exact scope string and a short description like "Administer fido2 config related information" so strict validators/generators no longer flag the discrepancy.
255-261:⚠️ Potential issue | 🟡 MinorMake
startIndexdocs match the documented default.These parameters still say "1-based" while
default: 0describes zero-based pagination. Pick one convention; as written, the generated docs are misleading.Also applies to: 431-437, 524-530, 668-674
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/docs/fido2-plugin-swagger.yaml` around lines 255 - 261, The parameter docs for startIndex are inconsistent: the description says "1-based" while the schema default is 0; change the description to match the default (e.g., "The 0-based index of the first query result") or change the default to 1 if you intend 1-based pagination; update every occurrence of the startIndex parameter in this file (the startIndex parameter definitions at the shown diff and the other occurrences referenced around lines 431-437, 524-530, 668-674) so description and default follow the same convention.jans-config-api/plugins/fido2-plugin/src/test/resources/testng.xml (1)
18-22:⚠️ Potential issue | 🟡 MinorGive the metrics suite block a unique TestNG name.
This second
<test>still reusesname="Fido2", so reports and name-based filtering cannot distinguish it fromFido2ResourceTest.Suggested fix
- <test name="Fido2" enabled="true"> + <test name="Fido2Metrics" enabled="true"> <classes> <class name="io.jans.configapi.plugin.fido2.test.Fido2MetricsTest" /> </classes> </test>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/test/resources/testng.xml` around lines 18 - 22, The second TestNG <test> entry reuses name="Fido2" which conflicts with the other suite; update the name attribute to a unique value (e.g., "Fido2Metrics" or similar) for the test that runs io.jans.configapi.plugin.fido2.test.Fido2MetricsTest so reports and name-based filtering can distinguish it from the Fido2ResourceTest suite.jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java (2)
170-180:⚠️ Potential issue | 🟠 MajorKeep
readList()typed as aListand preserveJsonProcessingException.This overload returns
List<T>but accepts anyCollectionclass, and it converts mapper failures intoRuntimeException. That weakens the contract for callers that already declareJsonProcessingException.Suggested fix
- public static <T> List<T> readList(String str, Class<T> type) { + public static <T> List<T> readList(String str, Class<T> type) throws JsonProcessingException { return readList(str, ArrayList.class, type); } - public static <T> List<T> readList(String str, Class<? extends Collection> type, Class<T> elementType) { + public static <T> List<T> readList(String str, Class<? extends List> type, Class<T> elementType) + throws JsonProcessingException { final ObjectMapper mapper = new ObjectMapper(); - try { - return mapper.readValue(str, mapper.getTypeFactory().constructCollectionType(type, elementType)); - } catch (IOException e) { - throw new RuntimeException(e); - } + return mapper.readValue(str, mapper.getTypeFactory().constructCollectionType(type, elementType)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java` around lines 170 - 180, The readList overload should keep returning List<T> and preserve JsonProcessingException instead of wrapping IOExceptions in RuntimeException: change the signature of readList(String, Class<? extends Collection>, Class<T>) to declare throws JsonProcessingException (or let JsonProcessingException propagate) and remove the catch that throws new RuntimeException(e); ensure the first overload still delegates to this method using ArrayList.class and returns List<T>; use mapper.readValue(str, mapper.getTypeFactory().constructCollectionType(type, elementType)) and cast/return it as List<T> so callers keep the List<T> return type and can handle JsonProcessingException themselves (identify methods: readList(String, Class<T>), readList(String, Class<? extends Collection>, Class<T>), mapper.readValue, constructCollectionType).
164-167:⚠️ Potential issue | 🟠 Major
readListValue()ignoresclazzand loses the element type.
new TypeReference<List<T>>() {}cannot retainThere, so Jackson will deserialize elements as generic maps instead ofclazz.Suggested fix
public static <T> List<T> readListValue(String content, Class<T> clazz) throws JsonProcessingException { ObjectMapper mapper = new ObjectMapper(); - return mapper.readValue(content, new TypeReference<List<T>>() { - }); + return mapper.readValue(content, + mapper.getTypeFactory().constructCollectionType(List.class, clazz)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java` around lines 164 - 167, readListValue currently ignores the provided clazz and deserializes elements as generic maps because TypeReference<List<T>> loses T at runtime; fix it by building a concrete JavaType for a List of the given element class using the ObjectMapper's TypeFactory (e.g., constructCollectionType/ListType) and pass that JavaType into mapper.readValue so elements are deserialized as clazz; update the method readListValue and the ObjectMapper usage to use mapper.getTypeFactory().constructCollectionType(List.class, clazz) (or equivalent) when calling readValue.jans-config-api/docs/jans-config-api-swagger.yaml (2)
1855-1862:⚠️ Potential issue | 🟡 MinorUse concrete sample values for these date parameters.
The parenthetical
yyyy-MM-ddTHH:mm:ssZis still a format token, not an example, so callers still have to infer the accepted shape. Publish concrete samples such as31-12-2025and2025-12-31T23:59:59Zin the upstream endpoint annotations and regenerate the spec.Based on learnings,
jans-config-api/docs/jans-config-api-swagger.yamlis auto-generated from upstream source components, so this should be corrected in the owning endpoint annotations rather than in the generated YAML.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/docs/jans-config-api-swagger.yaml` around lines 1855 - 1862, Update the upstream endpoint annotations that generate the swagger descriptions for the query parameters start_date and end_date so their descriptions include concrete example values instead of format tokens; replace the parenthetical "yyyy-MM-ddTHH:mm:ssZ" with explicit samples (e.g. "31-12-2025" and "2025-12-31T23:59:59Z") in the annotation for both start_date and end_date, then regenerate the OpenAPI spec so the generated jans-config-api-swagger.yaml contains those concrete examples.
16632-16633:⚠️ Potential issue | 🟡 MinorDocument what
fapi=truemeans in the owning model.Adding
fapias a bare boolean leaves the contract ambiguous for consumers. Please add a property-level description on the source model field/getter and regenerate the Swagger so callers know which behavior this flag enables.Based on learnings, missing property descriptions in this generated Swagger should be added in the upstream model classes so they propagate into
jans-config-api/docs/jans-config-api-swagger.yaml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/docs/jans-config-api-swagger.yaml` around lines 16632 - 16633, The generated Swagger exposes a bare boolean property "fapi" (in jans-config-api-swagger.yaml) without any description; update the upstream model class that declares the fapi field/getter (the owning POJO/DTO) to add a clear `@Schema` or Javadoc/property annotation describing what fapi=true enables (behavior, effects, defaults), then re-run the OpenAPI/Swagger generation to regenerate jans-config-api-swagger.yaml so the property-level description is included for consumers.jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/ApiApplication.java (1)
22-32:⚠️ Potential issue | 🟠 MajorDeclare
FIDO2_ADMIN_ACCESSin the OAuth2 scheme.
Fido2MetricsResourceadvertisesConstants.FIDO2_ADMIN_ACCESS, but Line 22-Line 32 never registers that scope in@SecurityScheme. Generated OpenAPI docs and clients will miss a valid authorization option for the metrics endpoints.Suggested fix
`@OAuthScope`(name = Constants.FIDO2_DELETE_ACCESS, description = "Delete fido2 related information"), `@OAuthScope`(name = Constants.FIDO2_METRICS_READ_ACCESS, description = "View fido2 metrics related information"), + `@OAuthScope`(name = Constants.FIDO2_ADMIN_ACCESS, description = "Admin access to fido2 resources"), `@OAuthScope`(name = ApiAccessConstants.SUPER_ADMIN_READ_ACCESS, description = "Super admin for viewing application resource information"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/ApiApplication.java` around lines 22 - 32, The OAuth2 SecurityScheme on ApiApplication (the `@SecurityScheme` annotation) is missing the Constants.FIDO2_ADMIN_ACCESS scope referenced by Fido2MetricsResource; add an `@OAuthScope` entry using Constants.FIDO2_ADMIN_ACCESS (with an appropriate description like "Admin access for fido2 operations/metrics") into the flows = `@OAuthFlows`(... `@OAuthFlow`(... scopes = { ... }) ) list so the generated OpenAPI docs include that authorization option.jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java (5)
134-137:⚠️ Potential issue | 🟠 MajorPreserve bad-request and upstream HTTP statuses.
Line 134 rewrites every
WebApplicationExceptionfromvalidateDate()orFido2MetricsServiceinto a 500. CatchWebApplicationExceptionseparately and rethrow it unchanged; the same catch-all is repeated throughout this resource.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 134 - 137, The catch-all in Fido2MetricsResource (e.g., in the block catching Exception around calls to validateDate() and Fido2MetricsService) currently converts every WebApplicationException into a 500; change the error handling to first catch javax.ws.rs.WebApplicationException (or JAX-RS WebApplicationException) and rethrow it unchanged, then catch Exception for logging and call throwInternalServerException(ex); ensure you reference the existing validateDate(), Fido2MetricsService calls, logger.error(ERR_MSG, ex) and throwInternalServerException(ex) so the new ordering preserves upstream HTTP statuses for client/upstream errors.
97-99:⚠️ Potential issue | 🟠 MajorDo not require optional
start_date/end_dateparameters.Line 98 and Line 99 are documented and handled as optional, but
@NotNullcauses Bean Validation to reject omitted query params before the handler runs. The same annotation pattern is repeated across the other metrics handlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 97 - 99, The query parameters startDate and endDate in Fido2MetricsResource are annotated with `@NotNull` which makes them mandatory even though they should be optional; remove the `@NotNull` annotation from the method parameters (startDate, endDate) in Fido2MetricsResource and the same pattern in the other metrics handlers so Bean Validation won't reject missing query params, and if your codebase uses nullability annotations add `@Nullable` (or leave unannotated) and ensure the existing handler logic checks for null/empty values and applies defaults as needed.
181-185:⚠️ Potential issue | 🟠 MajorRemove raw
userIdfrom info logs.Line 183-Line 185 writes caller-supplied user identifiers into standard logs. Redact/hash it or move it to debug to avoid PII leakage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 181 - 185, The info log in Fido2MetricsResource (the block that builds "Fido2MetricsEntry for user search param") currently emits caller-supplied userId and may leak PII; change it so userId is not logged in cleartext at info level—either remove it from the info message or replace it with a non-reversible identifier (e.g., a truncated/hashed value such as SHA-256 hex or a stable prefix) and keep the full userId only under debug (logger.debug) if needed; update the logging call that currently uses escapeLog(userId) accordingly and ensure any hashing/obfuscation uses a utility available in the class or a new helper so tests and other callers (e.g., the method in Fido2MetricsResource) reference the hashed/redacted symbol instead of the raw userId.
1012-1040:⚠️ Potential issue | 🟠 MajorFix date-only parsing in
parseDate().Line 1033 and Line 1039 parse date-only strings into
LocalDateTime, which throws for values like2026-03-12and12-03-2026. That breaks formats the API explicitly advertises.Suggested fix
- // Try ISO date only (e.g. 2024-02-13) + // Try ISO date only (e.g. 2024-02-13) try { - return LocalDateTime.parse(trimmed, DateTimeFormatter.ISO_LOCAL_DATE); + return java.time.LocalDate.parse(trimmed, DateTimeFormatter.ISO_LOCAL_DATE).atStartOfDay(); } catch (DateTimeParseException ignored) { // continue } // Fallback to legacy dd-MM-yyyy - return LocalDateTime.parse(trimmed, DateTimeFormatter.ofPattern(METRICS_DATE_FORMAT)); + return java.time.LocalDate.parse(trimmed, fallbackFormatter).atStartOfDay();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 1012 - 1040, The parseDate method currently calls LocalDateTime.parse for date-only inputs, causing DateTimeParseException for values like "2026-03-12" and "12-03-2026"; update parseDate so that for the ISO date-only branch you parse into a LocalDate (LocalDate.parse(trimmed, DateTimeFormatter.ISO_LOCAL_DATE)) and return date.atStartOfDay(), and likewise for the legacy METRICS_DATE_FORMAT branch parse into LocalDate using DateTimeFormatter.ofPattern(METRICS_DATE_FORMAT) and return atStartOfDay(); keep the existing ZonedDateTime and ISO_LOCAL_DATE_TIME branches unchanged.
1042-1120:⚠️ Potential issue | 🟠 MajorConvert the public 1-based index before calling
subList().Line 1051 and Line 1082 validate
startIndexas if it were 0-based and then slice with it directly. With the documented 1-based contract,startIndex=1skips the first item, and empty results with the default index can still throw.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 1042 - 1120, The code treats the documented 1-based startIndex as 0-based when slicing; convert the incoming 1-based startIndex to a 0-based index before validating and calling subList in both getFido2MetricsEntryPagedResult and getFido2MetricsAggregationPagedResult: create a new variable (e.g. zeroBasedStart = Math.max(0, startIndex - 1)), call getStartIndex(entriesList, zeroBasedStart), use zeroBasedStart for toIndex and subList calculations, and use the original startIndex when populating the paged result's setStart if the API contract requires returning the 1-based value; update getStartIndex signature/usages only if necessary to accept the zeroBasedStart.jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java (3)
449-453:⚠️ Potential issue | 🟠 MajorSend
periodsunder its documented query key.Line 451 stores the comparison window as
startTime, so the upstream comparison endpoint never receives theperiodsparameter.Suggested fix
// Query Parameter Map<String, String> data = new HashMap<>(); - data.put("startTime", String.valueOf(periods)); + data.put("periods", String.valueOf(periods));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java` around lines 449 - 453, The code in Fido2MetricsService is sending the comparison window under the wrong query key ("startTime") so the upstream comparison endpoint doesn't receive the periods value; change the Map key for the variable periods to the documented query parameter name (use "periods" instead of "startTime") before calling getMetricsData(this.getPeriodOverPeriodComparisonUrl() + "/" + aggregationType, headers, data), ensuring the Map<String,String> data contains data.put("periods", String.valueOf(periods)).
493-518:⚠️ Potential issue | 🔴 CriticalRead the paged wrapper before deserializing
entries.Line 507 treats the whole response object as
List<T>, but the metrics response is a paged wrapper. This either fails deserialization or discardsstart/entriesCount/totalEntriesCount, so the REST layer cannot report accurate paging metadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java` around lines 493 - 518, getFido2MetricsPagedResult is deserializing the entire response as List<T>, which ignores the paged wrapper; change it to first deserialize the JSON into the PagedResult wrapper (or a JsonNode), extract paging fields (start, entriesCount, totalEntriesCount) and then deserialize the "entries" sub-node into List<T> using Jackson.readList(..., type); populate a new PagedResult<T> with the extracted paging metadata and the deserialized entries, handling nulls/empty lists safely so the REST layer returns correct paging info.
162-163:⚠️ Potential issue | 🟠 MajorStop building forwarded URLs with
//.
getFido2UserMetricsEntriesUrl()already ends with/, so Line 162 produces/entries/user//{userId}. The same concatenation pattern at Line 198, Line 233, Line 267, Line 299, Line 427, and Line 453 can 404 on servers that do not normalize duplicate separators.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java` around lines 162 - 163, The code is producing duplicate slashes when building forwarded URLs (e.g., in the call to getFido2MetricsPagedResult using getFido2UserMetricsEntriesUrl() + "/" + userId). Fix by normalizing concatenation: check if getFido2UserMetricsEntriesUrl() endsWith("/") and append userId without adding an extra "/" (or otherwise ensure a single separator), and apply the same fix to every similar concatenation that appends "/" + userId or other path segments before calling getFido2MetricsPagedResult; update the call sites that use getFido2UserMetricsEntriesUrl(), getFido2MetricsEntriesUrl() (and any similar URL-returning methods) so they conditionally add the slash or use a small helper to join URL segments safely.jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/util/Fido2Util.java (2)
115-124:⚠️ Potential issue | 🟡 MinorUse the requested envelope key.
Line 123 always extracts
"response", so any caller that passes a differentnodeNamegets the wrong subtree.Suggested fix
- if (StringUtils.isNotBlank(nodeName) && jsonNode != null && jsonNode.get(nodeName) != null) { - jsonNode = jsonNode.get("response"); + if (StringUtils.isNotBlank(nodeName) && jsonNode != null && jsonNode.get(nodeName) != null) { + jsonNode = jsonNode.get(nodeName); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/util/Fido2Util.java` around lines 115 - 124, The method getResponseJsonNode currently always extracts the "response" child regardless of the requested key; update it so that after parsing (in getResponseJsonNode) you use the nodeName parameter (not the hard-coded "response") when calling jsonNode.get(...), and return that child when present; ensure you still handle blank jsonSring and null jsonNode cases and use the existing variable names (jsonSring, nodeName) so callers receive the correct subtree.
71-112:⚠️ Potential issue | 🟠 MajorSurface failed upstream GETs instead of returning
null.
ConfigHttpService.executeGet()can returnnullon I/O failure, and Line 75 plus Line 93 treat that the same as “no data”. That makes transport and upstream HTTP failures look like empty metrics responses instead of 4xx/5xx errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/util/Fido2Util.java` around lines 71 - 112, getResponseJsonNode and getResponseEntityString currently treat a null HttpServiceResponse (and null inner HttpResponse/entity) as “no data” and return null, which hides upstream transport failures; change both methods so that when serviceResponse is null (or when serviceResponse.getHttpResponse() is null) you throw a WebApplicationException (e.g., Bad Gateway/502) containing a descriptive message (and any available error text) instead of returning null, and likewise ensure getResponseEntityString throws a WebApplicationException with the upstream status code when httpResponse is present but non-200; update thrown messages to include the response body or cause to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java`:
- Line 31: The test contains copy-paste errors in log messages: replace the
incorrect method names in the log.info calls so they match the test methods
being exercised — change the message "getFido2Configuration()" to
"getFido2MetricsEntry()" for the log in the getFido2MetricsEntry test, change
"getFido2MetricsEntry()" to "getFido2UserMetrics()" for the log in the
getFido2UserMetrics test, and change "getFido2Configuration()" to
"getFido2MetricsConfig()" for the log in the getFido2MetricsConfig test (update
the log.info calls in Fido2MetricsTest.java accordingly so the string literals
reflect the correct method names).
In
`@jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2ResourceTest.java`:
- Line 24: The test currently logs the bearer token via log.error in
Fido2ResourceTest (the call that prints accessToken, issuer, fido2ConfigUrl);
remove the accessToken from that log (or replace it with a masked value like
"<REDACTED>" or only log its presence/length) so no live token is written to
test/CI logs, and update the log statement in Fido2ResourceTest.java accordingly
while keeping issuer and fido2ConfigUrl logging intact.
In
`@jans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-aggregation-summary.json`:
- Around line 1-8: The JSON currently uses 0 for avgAuthenticationSuccessRate
(and potentially other avg* fields) even when their denominators are zero;
update the logic that produces avgAuthenticationSuccessRate and
avgRegistrationSuccessRate so that when totalAuthentications or
totalRegistrations (respectively) is 0 the average value is null rather than 0,
otherwise compute and emit the numeric rate; locate and modify the code or
template that sets the JSON keys avgAuthenticationSuccessRate and
avgRegistrationSuccessRate to apply this conditional null-on-zero rule.
---
Duplicate comments:
In `@jans-config-api/docs/jans-config-api-swagger.yaml`:
- Around line 1855-1862: Update the upstream endpoint annotations that generate
the swagger descriptions for the query parameters start_date and end_date so
their descriptions include concrete example values instead of format tokens;
replace the parenthetical "yyyy-MM-ddTHH:mm:ssZ" with explicit samples (e.g.
"31-12-2025" and "2025-12-31T23:59:59Z") in the annotation for both start_date
and end_date, then regenerate the OpenAPI spec so the generated
jans-config-api-swagger.yaml contains those concrete examples.
- Around line 16632-16633: The generated Swagger exposes a bare boolean property
"fapi" (in jans-config-api-swagger.yaml) without any description; update the
upstream model class that declares the fapi field/getter (the owning POJO/DTO)
to add a clear `@Schema` or Javadoc/property annotation describing what fapi=true
enables (behavior, effects, defaults), then re-run the OpenAPI/Swagger
generation to regenerate jans-config-api-swagger.yaml so the property-level
description is included for consumers.
In `@jans-config-api/plugins/docs/fido2-plugin-swagger.yaml`:
- Around line 1978-1989: Add the missing OAuth2 scope declaration
"https://jans.io/oauth/config/fido2.admin" to
components.securitySchemes.oauth2.flows.clientCredentials.scopes so the registry
matches the scopes referenced by the FIDO2 operation security blocks; update the
scopes list in the YAML under the clientCredentials flow to include that exact
scope string and a short description like "Administer fido2 config related
information" so strict validators/generators no longer flag the discrepancy.
- Around line 255-261: The parameter docs for startIndex are inconsistent: the
description says "1-based" while the schema default is 0; change the description
to match the default (e.g., "The 0-based index of the first query result") or
change the default to 1 if you intend 1-based pagination; update every
occurrence of the startIndex parameter in this file (the startIndex parameter
definitions at the shown diff and the other occurrences referenced around lines
431-437, 524-530, 668-674) so description and default follow the same
convention.
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/ApiApplication.java`:
- Around line 22-32: The OAuth2 SecurityScheme on ApiApplication (the
`@SecurityScheme` annotation) is missing the Constants.FIDO2_ADMIN_ACCESS scope
referenced by Fido2MetricsResource; add an `@OAuthScope` entry using
Constants.FIDO2_ADMIN_ACCESS (with an appropriate description like "Admin access
for fido2 operations/metrics") into the flows = `@OAuthFlows`(... `@OAuthFlow`(...
scopes = { ... }) ) list so the generated OpenAPI docs include that
authorization option.
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java`:
- Around line 134-137: The catch-all in Fido2MetricsResource (e.g., in the block
catching Exception around calls to validateDate() and Fido2MetricsService)
currently converts every WebApplicationException into a 500; change the error
handling to first catch javax.ws.rs.WebApplicationException (or JAX-RS
WebApplicationException) and rethrow it unchanged, then catch Exception for
logging and call throwInternalServerException(ex); ensure you reference the
existing validateDate(), Fido2MetricsService calls, logger.error(ERR_MSG, ex)
and throwInternalServerException(ex) so the new ordering preserves upstream HTTP
statuses for client/upstream errors.
- Around line 97-99: The query parameters startDate and endDate in
Fido2MetricsResource are annotated with `@NotNull` which makes them mandatory even
though they should be optional; remove the `@NotNull` annotation from the method
parameters (startDate, endDate) in Fido2MetricsResource and the same pattern in
the other metrics handlers so Bean Validation won't reject missing query params,
and if your codebase uses nullability annotations add `@Nullable` (or leave
unannotated) and ensure the existing handler logic checks for null/empty values
and applies defaults as needed.
- Around line 181-185: The info log in Fido2MetricsResource (the block that
builds "Fido2MetricsEntry for user search param") currently emits
caller-supplied userId and may leak PII; change it so userId is not logged in
cleartext at info level—either remove it from the info message or replace it
with a non-reversible identifier (e.g., a truncated/hashed value such as SHA-256
hex or a stable prefix) and keep the full userId only under debug (logger.debug)
if needed; update the logging call that currently uses escapeLog(userId)
accordingly and ensure any hashing/obfuscation uses a utility available in the
class or a new helper so tests and other callers (e.g., the method in
Fido2MetricsResource) reference the hashed/redacted symbol instead of the raw
userId.
- Around line 1012-1040: The parseDate method currently calls
LocalDateTime.parse for date-only inputs, causing DateTimeParseException for
values like "2026-03-12" and "12-03-2026"; update parseDate so that for the ISO
date-only branch you parse into a LocalDate (LocalDate.parse(trimmed,
DateTimeFormatter.ISO_LOCAL_DATE)) and return date.atStartOfDay(), and likewise
for the legacy METRICS_DATE_FORMAT branch parse into LocalDate using
DateTimeFormatter.ofPattern(METRICS_DATE_FORMAT) and return atStartOfDay(); keep
the existing ZonedDateTime and ISO_LOCAL_DATE_TIME branches unchanged.
- Around line 1042-1120: The code treats the documented 1-based startIndex as
0-based when slicing; convert the incoming 1-based startIndex to a 0-based index
before validating and calling subList in both getFido2MetricsEntryPagedResult
and getFido2MetricsAggregationPagedResult: create a new variable (e.g.
zeroBasedStart = Math.max(0, startIndex - 1)), call getStartIndex(entriesList,
zeroBasedStart), use zeroBasedStart for toIndex and subList calculations, and
use the original startIndex when populating the paged result's setStart if the
API contract requires returning the 1-based value; update getStartIndex
signature/usages only if necessary to accept the zeroBasedStart.
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java`:
- Around line 449-453: The code in Fido2MetricsService is sending the comparison
window under the wrong query key ("startTime") so the upstream comparison
endpoint doesn't receive the periods value; change the Map key for the variable
periods to the documented query parameter name (use "periods" instead of
"startTime") before calling
getMetricsData(this.getPeriodOverPeriodComparisonUrl() + "/" + aggregationType,
headers, data), ensuring the Map<String,String> data contains
data.put("periods", String.valueOf(periods)).
- Around line 493-518: getFido2MetricsPagedResult is deserializing the entire
response as List<T>, which ignores the paged wrapper; change it to first
deserialize the JSON into the PagedResult wrapper (or a JsonNode), extract
paging fields (start, entriesCount, totalEntriesCount) and then deserialize the
"entries" sub-node into List<T> using Jackson.readList(..., type); populate a
new PagedResult<T> with the extracted paging metadata and the deserialized
entries, handling nulls/empty lists safely so the REST layer returns correct
paging info.
- Around line 162-163: The code is producing duplicate slashes when building
forwarded URLs (e.g., in the call to getFido2MetricsPagedResult using
getFido2UserMetricsEntriesUrl() + "/" + userId). Fix by normalizing
concatenation: check if getFido2UserMetricsEntriesUrl() endsWith("/") and append
userId without adding an extra "/" (or otherwise ensure a single separator), and
apply the same fix to every similar concatenation that appends "/" + userId or
other path segments before calling getFido2MetricsPagedResult; update the call
sites that use getFido2UserMetricsEntriesUrl(), getFido2MetricsEntriesUrl() (and
any similar URL-returning methods) so they conditionally add the slash or use a
small helper to join URL segments safely.
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/util/Fido2Util.java`:
- Around line 115-124: The method getResponseJsonNode currently always extracts
the "response" child regardless of the requested key; update it so that after
parsing (in getResponseJsonNode) you use the nodeName parameter (not the
hard-coded "response") when calling jsonNode.get(...), and return that child
when present; ensure you still handle blank jsonSring and null jsonNode cases
and use the existing variable names (jsonSring, nodeName) so callers receive the
correct subtree.
- Around line 71-112: getResponseJsonNode and getResponseEntityString currently
treat a null HttpServiceResponse (and null inner HttpResponse/entity) as “no
data” and return null, which hides upstream transport failures; change both
methods so that when serviceResponse is null (or when
serviceResponse.getHttpResponse() is null) you throw a WebApplicationException
(e.g., Bad Gateway/502) containing a descriptive message (and any available
error text) instead of returning null, and likewise ensure
getResponseEntityString throws a WebApplicationException with the upstream
status code when httpResponse is present but non-200; update thrown messages to
include the response body or cause to aid debugging.
In
`@jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java`:
- Line 24: The test currently logs the raw bearer token via log.error (see the
log.error(...) call referencing accessToken in Fido2MetricsTest.java and similar
calls at lines 39 and 54); remove the raw accessToken from all test log messages
and either omit it entirely or replace it with a safe/masked representation
(e.g., show only last N chars or a fixed placeholder) in the log statements that
reference accessToken so that no full bearer token is ever written to test logs.
In `@jans-config-api/plugins/fido2-plugin/src/test/resources/testng.xml`:
- Around line 18-22: The second TestNG <test> entry reuses name="Fido2" which
conflicts with the other suite; update the name attribute to a unique value
(e.g., "Fido2Metrics" or similar) for the test that runs
io.jans.configapi.plugin.fido2.test.Fido2MetricsTest so reports and name-based
filtering can distinguish it from the Fido2ResourceTest suite.
In
`@jans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-analytics-errors.json`:
- Around line 1-6: The example JSON for the MetricsErrorAnalytics response is
missing the required numeric fields completionRate and dropOffRate; update the
example object that currently contains errorCategories, failureRate,
successRate, and topErrors by adding completionRate and dropOffRate (as numeric
values consistent with failureRate/successRate) so the example matches the
MetricsErrorAnalytics schema (referencing the MetricsErrorAnalytics response and
the fields completionRate and dropOffRate).
In
`@jans-config-api/shared/src/main/java/io/jans/configapi/core/rest/BaseResource.java`:
- Around line 294-304: The getRegexPattern method appends the caller-supplied
pattern verbatim into a regex, which can break or change semantics for
metacharacters; fix it by quoting the user fragment with
java.util.regex.Pattern.quote(...) (i.e., call Pattern.quote(pattern) before
wrapping with "^.*" and ".*$"), keep the existing blank-check logic, and add the
necessary import for java.util.regex.Pattern so compiled regexes treat the
fragment as a literal.
In
`@jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java`:
- Around line 170-180: The readList overload should keep returning List<T> and
preserve JsonProcessingException instead of wrapping IOExceptions in
RuntimeException: change the signature of readList(String, Class<? extends
Collection>, Class<T>) to declare throws JsonProcessingException (or let
JsonProcessingException propagate) and remove the catch that throws new
RuntimeException(e); ensure the first overload still delegates to this method
using ArrayList.class and returns List<T>; use mapper.readValue(str,
mapper.getTypeFactory().constructCollectionType(type, elementType)) and
cast/return it as List<T> so callers keep the List<T> return type and can handle
JsonProcessingException themselves (identify methods: readList(String,
Class<T>), readList(String, Class<? extends Collection>, Class<T>),
mapper.readValue, constructCollectionType).
- Around line 164-167: readListValue currently ignores the provided clazz and
deserializes elements as generic maps because TypeReference<List<T>> loses T at
runtime; fix it by building a concrete JavaType for a List of the given element
class using the ObjectMapper's TypeFactory (e.g.,
constructCollectionType/ListType) and pass that JavaType into mapper.readValue
so elements are deserialized as clazz; update the method readListValue and the
ObjectMapper usage to use
mapper.getTypeFactory().constructCollectionType(List.class, clazz) (or
equivalent) when calling readValue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15b3c1e2-447b-4415-af86-127ca3340fbd
📒 Files selected for processing (33)
jans-config-api/docs/jans-config-api-swagger.yamljans-config-api/plugins/docs/fido2-plugin-swagger.yamljans-config-api/plugins/docs/scim-plugin-swagger.yamljans-config-api/plugins/docs/user-mgt-plugin-swagger.yamljans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/ApiApplication.javajans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.javajans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.javajans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/util/ClientFactory.javajans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/util/Constants.javajans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/util/Fido2Util.javajans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.javajans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2ResourceTest.javajans-config-api/plugins/fido2-plugin/src/test/resources/test.propertiesjans-config-api/plugins/fido2-plugin/src/test/resources/testng.xmljans-config-api/profiles/default/config-api-test.propertiesjans-config-api/profiles/jenkins-config-api.gluu.org/test.propertiesjans-config-api/profiles/local/test.propertiesjans-config-api/server/src/main/resources/config-api-rs-protect.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-analytics-performance-metrics.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-analytics-trends-aggregation-type.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-adoption.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-aggregation-summary.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-aggregations-type.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-analytics-devices.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-analytics-errors.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-by-operation-type.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-by-user.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-configuration.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-data.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-health-check.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-period-over-period-comparison.jsonjans-config-api/shared/src/main/java/io/jans/configapi/core/rest/BaseResource.javajans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java
...plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java
Outdated
Show resolved
Hide resolved
...lugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2ResourceTest.java
Show resolved
Hide resolved
...g-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-aggregation-summary.json
Show resolved
Hide resolved
Signed-off-by: pujavs <pujas.works@gmail.com>
Signed-off-by: pujavs <pujas.works@gmail.com>
Signed-off-by: pujavs <pujas.works@gmail.com>
Signed-off-by: pujavs <pujas.works@gmail.com>
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (9)
jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java (1)
169-180:⚠️ Potential issue | 🟠 MajorPreserve
JsonProcessingExceptioninreadList()instead of wrapping it.Line 178 converts Jackson parse failures into
RuntimeException, which weakens the contract for callers that already declarethrows JsonProcessingException. In particular,jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java:494-516expectsJackson.readList(...)failures to stay typed, so malformed upstream metrics payloads now surface as generic 500s instead of the declared parse error.Proposed fix
- public static <T> List<T> readList(String str, Class<T> type) { + public static <T> List<T> readList(String str, Class<T> type) throws JsonProcessingException { return readList(str, ArrayList.class, type); } - public static <T> List<T> readList(String str, Class<? extends List> type, Class<T> elementType) { + public static <T> List<T> readList(String str, Class<? extends List> type, Class<T> elementType) + throws JsonProcessingException { final ObjectMapper mapper = new ObjectMapper(); - try { - return mapper.readValue(str, - mapper.getTypeFactory().constructCollectionType(type.asSubclass(Collection.class), elementType)); - } catch (IOException e) { - throw new RuntimeException(e); - } + return mapper.readValue(str, + mapper.getTypeFactory().constructCollectionType(type.asSubclass(Collection.class), elementType)); }#!/bin/bash set -euo pipefail echo "== Jackson.readList implementation ==" sed -n '169,180p' jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java echo echo "== Fido2MetricsService caller and declared exception contract ==" sed -n '494,516p' jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java` around lines 169 - 180, The readList(...) methods in Jackson currently catch IOException and wrap it in RuntimeException, losing JsonProcessingException typing expected by callers like Fido2MetricsService; update both overloads (readList(String, Class<T>) and readList(String, Class<? extends List>, Class<T>)) to declare "throws JsonProcessingException" and change the catch logic to rethrow JsonProcessingException (e.g., catch JsonProcessingException and throw it) while keeping a broader catch for other IOExceptions (wrap or convert those), so callers that declare throws JsonProcessingException continue to receive the typed exception.jans-config-api/docs/jans-config-api-swagger.yaml (2)
18350-18351:⚠️ Potential issue | 🟠 Major
persistedshould not be documented unless the owning model actually exposes it.Lines 18350-18351 add
persistedto the schema, butjans-auth-server/common/src/main/java/io/jans/as/common/model/session/SessionId.java:93-101marks that member@Hidden,@Transient, andtransient. That creates a spec/runtime mismatch unless the owning component explicitly serializes it or stops hiding it.#!/bin/bash # Verify that SessionId.persisted is hidden/transient in the source model # while still present in the generated OpenAPI contract. sed -n '90,102p' jans-auth-server/common/src/main/java/io/jans/as/common/model/session/SessionId.java rg -n -C2 '^ {8}persisted:$|^ {10}type: boolean$' jans-config-api/docs/jans-config-api-swagger.yamlBased on learnings,
jans-config-api/docs/jans-config-api-swagger.yamlis auto-generated from upstream source components, so the fix belongs in the owning model/generator path rather than in this generated YAML.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/docs/jans-config-api-swagger.yaml` around lines 18350 - 18351, The OpenAPI schema incorrectly documents the SessionId.persisted property even though the owning model (SessionId class) marks it `@Hidden`, `@Transient` and transient; update the source model or generator so the property is not emitted into the generated spec: either remove/annotate persisted for serialization (undo transient/Hidden) if it should be exposed, or change the generator/model-to-OpenAPI mapping to skip fields annotated with `@Hidden/`@Transient (affecting SessionId.persisted) so the swagger.yaml no longer contains the persisted boolean.
1855-1862:⚠️ Potential issue | 🟡 MinorUse concrete sample values for these date parameters.
Lines 1855-1862 still show format tokens (
dd-MM-yyyy,yyyy-MM-ddTHH:mm:ssZ) instead of actual examples, so callers do not see a concrete valid value to send. Please publish sample values like31-12-2025and2025-12-31T23:59:59Zin the upstream endpoint annotations that generate this YAML.Based on learnings,
jans-config-api/docs/jans-config-api-swagger.yamlis auto-generated from upstream source components, so this should be corrected in the owning endpoint annotations rather than in the generated YAML.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/docs/jans-config-api-swagger.yaml` around lines 1855 - 1862, The generated Swagger shows format tokens rather than concrete examples for the query parameters start_date and end_date; update the upstream endpoint parameter annotations (the controller/handler parameter annotations that generate these OpenAPI params for start_date and end_date) to include concrete example values and/or an example field instead of format tokens — e.g. example: "31-12-2025" for the dd-MM-yyyy form and example: "2025-12-31T23:59:59Z" for the ISO-8601 form — so the generated jans-config-api-swagger.yaml emits those sample values for the start_date and end_date parameters.jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java (2)
449-453:⚠️ Potential issue | 🟠 MajorQuery parameter key
startTimeshould beperiods.The upstream endpoint expects
periodsparameter, but this method sends it understartTime, so the comparison window is never received.Proposed fix
// Query Parameter Map<String, String> data = new HashMap<>(); - data.put("startTime", String.valueOf(periods)); + data.put("periods", String.valueOf(periods));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java` around lines 449 - 453, In Fido2MetricsService, the request builds query params using the wrong key "startTime" so the downstream comparison endpoint never receives the window; update the Map in the method that calls getMetricsData() (the block returning getMetricsData(this.getPeriodOverPeriodComparisonUrl() + "/" + aggregationType, headers, data)) to use data.put("periods", String.valueOf(periods)) instead of "startTime", ensuring the periods parameter name matches the upstream API.
52-54:⚠️ Potential issue | 🟠 MajorURL helper methods end with trailing slashes causing double-slash URLs downstream.
All URL helper methods return paths ending with
/. When callers concatenate"/" + variable, this produces URLs like/entries/user//userIdwhich will 404 on servers that don't normalize duplicate separators.Affected call sites: lines 162, 198, 233, 267, 299, 427, 453.
Proposed fix - remove trailing slashes from helpers
public String getFido2UserMetricsEntriesUrl() { - return getFido2MetricsUrl() + "/entries/user/"; + return getFido2MetricsUrl() + "/entries/user"; } public String getMetricsEntriesByOperationUrl() { - return getFido2MetricsUrl() + "/entries/operation/"; + return getFido2MetricsUrl() + "/entries/operation"; } public String getMetricsAggregationsUrl() { - return getFido2MetricsUrl() + "/aggregations/"; + return getFido2MetricsUrl() + "/aggregations"; } public String getMetricsAnalyticsUrl() { - return getFido2MetricsUrl() + "/analytics/"; + return getFido2MetricsUrl() + "/analytics"; } public String getMetricsAnalyticsPerformanceUrl() { - return getFido2MetricsUrl() + "/analytics/performance/"; + return getFido2MetricsUrl() + "/analytics/performance"; } public String getDeviceAnalyticsUrl() { - return getFido2MetricsUrl() + "/analytics/devices/"; + return getFido2MetricsUrl() + "/analytics/devices"; } public String getTrendAnalysisUrl() { - return getFido2MetricsUrl() + "/analytics/trends/"; + return getFido2MetricsUrl() + "/analytics/trends"; } public String getPeriodOverPeriodComparisonUrl() { - return getFido2MetricsUrl() + "/analytics/comparison/"; + return getFido2MetricsUrl() + "/analytics/comparison"; } public String getMetricsConfigUrl() { - return getFido2MetricsUrl() + "/config/"; + return getFido2MetricsUrl() + "/config"; } public String getMetricsHealthUrl() { - return getFido2MetricsUrl() + "/health/"; + return getFido2MetricsUrl() + "/health"; }Also applies to: 56-58, 60-62, 64-66, 68-70, 72-74, 76-78, 80-82, 84-86, 88-90, 92-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java` around lines 52 - 54, The helper methods (e.g., getFido2UserMetricsEntriesUrl, and the other getFido2* URL helpers in Fido2MetricsService) currently return paths with a trailing "/" which causes double slashes when callers append "/" + variable; remove the trailing slash from these returned strings (e.g., return getFido2MetricsUrl() + "/entries/user" instead of "/entries/user/") so callers that concatenate "/" + id produce a single separator, and update all similar helpers listed in the review to follow the same pattern; alternatively, normalize by documenting/using a URI join strategy, but the minimal fix is to strip trailing slashes in the helper return values.jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java (4)
1031-1039:⚠️ Potential issue | 🟠 Major
parseDate()cannot parse date-only formats intoLocalDateTime.
LocalDateTime.parse()withISO_LOCAL_DATEordd-MM-yyyypattern will throwDateTimeParseExceptionbecause these formatters produceLocalDate, notLocalDateTime. Parse toLocalDatefirst, then convert viaatStartOfDay().Proposed fix
// Try ISO date only (e.g. 2024-02-13) try { - return LocalDateTime.parse(trimmed, DateTimeFormatter.ISO_LOCAL_DATE); + return java.time.LocalDate.parse(trimmed, DateTimeFormatter.ISO_LOCAL_DATE).atStartOfDay(); } catch (DateTimeParseException ignored) { // continue } // Fallback to legacy dd-MM-yyyy - return LocalDateTime.parse(trimmed, DateTimeFormatter.ofPattern(METRICS_DATE_FORMAT)); + return java.time.LocalDate.parse(trimmed, DateTimeFormatter.ofPattern(METRICS_DATE_FORMAT)).atStartOfDay();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 1031 - 1039, The parseDate() method currently calls LocalDateTime.parse() with DateTimeFormatter.ISO_LOCAL_DATE and DateTimeFormatter.ofPattern(METRICS_DATE_FORMAT), which produce LocalDate and therefore fail; change the logic to first parse trimmed into a LocalDate when using ISO_LOCAL_DATE or the dd-MM-yyyy pattern (METRICS_DATE_FORMAT) and then convert it to LocalDateTime via atStartOfDay(), returning that LocalDateTime; keep existing try/catch flow for DateTimeParseException and only fall back to the legacy pattern if ISO parse fails, and ensure the final return type is LocalDateTime.
134-137:⚠️ Potential issue | 🟠 MajorPreserve HTTP status codes from
WebApplicationException.The catch-all
catch (Exception ex)rewrites 400s fromvalidateDate()and 4xx/5xx from upstream service calls into 500s. Re-throwWebApplicationExceptionto preserve the intended status.Proposed fix
- } catch (Exception ex) { + } catch (WebApplicationException wae) { + throw wae; + } catch (Exception ex) { logger.error(ERR_MSG, ex); throwInternalServerException(ex); }Add the import:
import jakarta.ws.rs.WebApplicationException;Also applies to: 215-218, 298-301, 380-383, 457-460, 528-531, 599-602, 669-672, 741-744, 815-818, 868-871, 912-915, 955-958
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 134 - 137, In Fido2MetricsResource update the generic Exception handlers to preserve HTTP status codes by re-throwing any caught jakarta.ws.rs.WebApplicationException instead of wrapping it as a 500: import jakarta.ws.rs.WebApplicationException and change the catch (Exception ex) blocks (e.g., the ones in class Fido2MetricsResource around the logger.error(ERR_MSG, ex); throwInternalServerException(ex); sites) to if (ex instanceof WebApplicationException) throw (WebApplicationException) ex; else log and call throwInternalServerException(ex); apply this same change to all similar catch blocks listed in the review.
97-100:⚠️ Potential issue | 🟠 MajorRemove
@NotNullfrom optional date query parameters.The date parameters are documented as optional and the code checks
StringUtils.isNotBlank()before use. However,@NotNulltriggers Bean Validation rejection before the method executes, preventing the intended optional behavior. This pattern appears across all endpoints with date parameters.Proposed fix for getFido2MetricsEntry (apply similarly to other endpoints)
- `@Parameter`(description = "Start date/time for " - + " entries report. Accepted format dd-MM-yyyy or ISO-8601 date-time like yyyy-MM-ddTHH:mm:ssZ, for example, 31-12-2025 and 2025-12-31T23:59:59Z.", schema = `@Schema`(type = "string")) `@QueryParam`(value = "start_date") `@NotNull` String startDate, - `@Parameter`(description = "End date/time for the log entries. Accepted format dd-MM-yyyy or ISO-8601 date-time like yyyy-MM-ddTHH:mm:ssZ, for example, 31-12-2025 and 2025-12-31T23:59:59Z.", schema = `@Schema`(type = "string")) `@QueryParam`(value = "end_date") `@NotNull` String endDate) + `@Parameter`(description = "Start date/time for " + + " entries report. Accepted format dd-MM-yyyy or ISO-8601 date-time like yyyy-MM-ddTHH:mm:ssZ, for example, 31-12-2025 and 2025-12-31T23:59:59Z.", schema = `@Schema`(type = "string")) `@QueryParam`(value = "start_date") String startDate, + `@Parameter`(description = "End date/time for the log entries. Accepted format dd-MM-yyyy or ISO-8601 date-time like yyyy-MM-ddTHH:mm:ssZ, for example, 31-12-2025 and 2025-12-31T23:59:59Z.", schema = `@Schema`(type = "string")) `@QueryParam`(value = "end_date") String endDate)Also applies to: 176-178, 259-260, 342-343, 422-423, 495-496, 566-567, 637-638, 708-709, 780-781
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 97 - 100, The query parameters startDate and endDate in Fido2MetricsResource methods (e.g., getFido2MetricsEntry) are marked `@NotNull` but are intended to be optional and are already guarded with StringUtils.isNotBlank(); remove the `@NotNull` annotation from the startDate and endDate `@QueryParam` parameters (and any other date query params in the same class) so Bean Validation does not reject null/empty values before the method runs, leaving the existing runtime checks in place to handle blank values.
1105-1120:⚠️ Potential issue | 🟠 Major
validateStartIndexdoes not guard empty lists with non-zerostartIndex.When the entries list is null or empty, validation is skipped. If
startIndex > 0, the subsequentsubList(startIndex, toIndex)call throwsIndexOutOfBoundsExceptionbecausetoIndexwill be 0 (clamped to list size) andfromIndex > toIndex.Proposed fix
private void validateStartIndex(List<?> entriesList, int startIndex) { if (logger.isDebugEnabled()) { logger.debug("Validate startIndex entriesList:{}, startIndex:{}", entriesList, escapeLog(startIndex)); } - if (entriesList != null && !entriesList.isEmpty()) { - logger.debug("Validate startIndex entriesList.size():{}", entriesList.size()); - try { - entriesList.get(startIndex); - } catch (IndexOutOfBoundsException ioe) { - logger.error("Error while getting data startIndex:{}", startIndex, ioe); - throwBadRequestException("Page start index incorrect, total entries:{" + entriesList.size() - + "}, but provided:{" + startIndex + "} "); - } + int listSize = (entriesList == null) ? 0 : entriesList.size(); + if (startIndex < 0 || startIndex > listSize) { + throwBadRequestException("Page start index incorrect, total entries:{" + listSize + + "}, but provided:{" + startIndex + "} "); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 1105 - 1120, The validateStartIndex method currently skips validation when entriesList is null or empty, which allows a later subList(fromIndex,toIndex) to fail when startIndex > 0; update validateStartIndex (method name: validateStartIndex) to explicitly guard this case: if startIndex > 0 and (entriesList == null || entriesList.isEmpty()) then throwBadRequestException with a message indicating total entries:0 and the provided startIndex, otherwise proceed with the existing bounds check (including the existing entriesList.get(startIndex) try/catch) so empty lists with non-zero startIndex are rejected early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@jans-auth-server/common/src/main/java/io/jans/as/common/model/session/SessionId.java`:
- Line 14: The SessionId class is exposing the internal boolean persisted in the
OpenAPI schema despite `@Hidden`; replace the Swagger `@Hidden` usage with Jackson
exclusion by importing com.fasterxml.jackson.annotation.JsonIgnore and annotate
the persisted field (and its getPersisted/setPersisted methods if present) with
`@JsonIgnore` so the property is omitted from JSON serialization and therefore
from the generated OpenAPI docs; update SessionId (the persisted field and its
accessor methods) accordingly or alternatively return a DTO that omits persisted
from REST endpoints like getSessionById.
In `@jans-config-api/plugins/docs/fido2-plugin-swagger.yaml`:
- Around line 900-907: The OpenAPI parameter "periods" currently sets required:
true while also providing default: 2, which is contradictory; update the
"periods" parameter definition to be consistent by either removing required:
true if you want the default 2 to be applied when omitted, or removing default:
2 if you require clients to always supply "periods" (adjust the schema under the
parameter named "periods" accordingly).
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/ApiApplication.java`:
- Line 20: The OpenAPI definition in ApiApplication declares only "Fido2 -
Configuration" and "Fido2 - Registration" tags while Fido2MetricsResource uses
"Fido2 - Metrics"; update the `@OpenAPIDefinition.tags` on the ApiApplication
class to include a Tag entry for "Fido2 - Metrics" so the generated docs group
the metrics endpoints consistently with Fido2MetricsResource.
In
`@jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java`:
- Around line 51-64: Update the incorrect log statements in the test method
getFido2MetricsConfig: change the log message that currently says
"getFido2Configuration()" to "getFido2MetricsConfig()" and update the
placeholder name from "fido2UserMetrics:{}" to "fido2MetricsConfig:{}" so the
debug log correctly references the method and the actual variable
fido2MetricsConfig used in the method.
---
Duplicate comments:
In `@jans-config-api/docs/jans-config-api-swagger.yaml`:
- Around line 18350-18351: The OpenAPI schema incorrectly documents the
SessionId.persisted property even though the owning model (SessionId class)
marks it `@Hidden`, `@Transient` and transient; update the source model or generator
so the property is not emitted into the generated spec: either remove/annotate
persisted for serialization (undo transient/Hidden) if it should be exposed, or
change the generator/model-to-OpenAPI mapping to skip fields annotated with
`@Hidden/`@Transient (affecting SessionId.persisted) so the swagger.yaml no longer
contains the persisted boolean.
- Around line 1855-1862: The generated Swagger shows format tokens rather than
concrete examples for the query parameters start_date and end_date; update the
upstream endpoint parameter annotations (the controller/handler parameter
annotations that generate these OpenAPI params for start_date and end_date) to
include concrete example values and/or an example field instead of format tokens
— e.g. example: "31-12-2025" for the dd-MM-yyyy form and example:
"2025-12-31T23:59:59Z" for the ISO-8601 form — so the generated
jans-config-api-swagger.yaml emits those sample values for the start_date and
end_date parameters.
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java`:
- Around line 1031-1039: The parseDate() method currently calls
LocalDateTime.parse() with DateTimeFormatter.ISO_LOCAL_DATE and
DateTimeFormatter.ofPattern(METRICS_DATE_FORMAT), which produce LocalDate and
therefore fail; change the logic to first parse trimmed into a LocalDate when
using ISO_LOCAL_DATE or the dd-MM-yyyy pattern (METRICS_DATE_FORMAT) and then
convert it to LocalDateTime via atStartOfDay(), returning that LocalDateTime;
keep existing try/catch flow for DateTimeParseException and only fall back to
the legacy pattern if ISO parse fails, and ensure the final return type is
LocalDateTime.
- Around line 134-137: In Fido2MetricsResource update the generic Exception
handlers to preserve HTTP status codes by re-throwing any caught
jakarta.ws.rs.WebApplicationException instead of wrapping it as a 500: import
jakarta.ws.rs.WebApplicationException and change the catch (Exception ex) blocks
(e.g., the ones in class Fido2MetricsResource around the logger.error(ERR_MSG,
ex); throwInternalServerException(ex); sites) to if (ex instanceof
WebApplicationException) throw (WebApplicationException) ex; else log and call
throwInternalServerException(ex); apply this same change to all similar catch
blocks listed in the review.
- Around line 97-100: The query parameters startDate and endDate in
Fido2MetricsResource methods (e.g., getFido2MetricsEntry) are marked `@NotNull`
but are intended to be optional and are already guarded with
StringUtils.isNotBlank(); remove the `@NotNull` annotation from the startDate and
endDate `@QueryParam` parameters (and any other date query params in the same
class) so Bean Validation does not reject null/empty values before the method
runs, leaving the existing runtime checks in place to handle blank values.
- Around line 1105-1120: The validateStartIndex method currently skips
validation when entriesList is null or empty, which allows a later
subList(fromIndex,toIndex) to fail when startIndex > 0; update
validateStartIndex (method name: validateStartIndex) to explicitly guard this
case: if startIndex > 0 and (entriesList == null || entriesList.isEmpty()) then
throwBadRequestException with a message indicating total entries:0 and the
provided startIndex, otherwise proceed with the existing bounds check (including
the existing entriesList.get(startIndex) try/catch) so empty lists with non-zero
startIndex are rejected early.
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java`:
- Around line 449-453: In Fido2MetricsService, the request builds query params
using the wrong key "startTime" so the downstream comparison endpoint never
receives the window; update the Map in the method that calls getMetricsData()
(the block returning getMetricsData(this.getPeriodOverPeriodComparisonUrl() +
"/" + aggregationType, headers, data)) to use data.put("periods",
String.valueOf(periods)) instead of "startTime", ensuring the periods parameter
name matches the upstream API.
- Around line 52-54: The helper methods (e.g., getFido2UserMetricsEntriesUrl,
and the other getFido2* URL helpers in Fido2MetricsService) currently return
paths with a trailing "/" which causes double slashes when callers append "/" +
variable; remove the trailing slash from these returned strings (e.g., return
getFido2MetricsUrl() + "/entries/user" instead of "/entries/user/") so callers
that concatenate "/" + id produce a single separator, and update all similar
helpers listed in the review to follow the same pattern; alternatively,
normalize by documenting/using a URI join strategy, but the minimal fix is to
strip trailing slashes in the helper return values.
In
`@jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java`:
- Around line 169-180: The readList(...) methods in Jackson currently catch
IOException and wrap it in RuntimeException, losing JsonProcessingException
typing expected by callers like Fido2MetricsService; update both overloads
(readList(String, Class<T>) and readList(String, Class<? extends List>,
Class<T>)) to declare "throws JsonProcessingException" and change the catch
logic to rethrow JsonProcessingException (e.g., catch JsonProcessingException
and throw it) while keeping a broader catch for other IOExceptions (wrap or
convert those), so callers that declare throws JsonProcessingException continue
to receive the typed exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a2b93b3-ecf0-4dab-9299-311e34624780
📒 Files selected for processing (11)
jans-auth-server/common/src/main/java/io/jans/as/common/model/session/SessionId.javajans-config-api/docs/jans-config-api-swagger.yamljans-config-api/plugins/docs/fido2-plugin-swagger.yamljans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/ApiApplication.javajans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.javajans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.javajans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.javajans-config-api/server/src/main/java/io/jans/configapi/security/client/AuthClientFactory.javajans-config-api/server/src/main/java/io/jans/configapi/util/AuthUtil.javajans-config-api/shared/src/main/java/io/jans/configapi/core/rest/BaseResource.javajans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java
💤 Files with no reviewable changes (1)
- jans-config-api/server/src/main/java/io/jans/configapi/util/AuthUtil.java
jans-auth-server/common/src/main/java/io/jans/as/common/model/session/SessionId.java
Outdated
Show resolved
Hide resolved
...i/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/ApiApplication.java
Outdated
Show resolved
Hide resolved
...plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java
Show resolved
Hide resolved
Signed-off-by: pujavs <pujas.works@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
jans-config-api/plugins/docs/fido2-plugin-swagger.yaml (2)
900-907:⚠️ Potential issue | 🟡 MinorDrop either
required: trueordefault: 2forperiods.A required query parameter never falls back to its default, so this definition is contradictory and generators may ignore one side or the other. Keep the default only if the server applies
2when the client omits the parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/docs/fido2-plugin-swagger.yaml` around lines 900 - 907, The OpenAPI parameter definition for the query parameter named "periods" is contradictory because it is marked required: true while also providing a default: 2; remove one of these to resolve the conflict. If the server must accept requests without the client sending "periods" and should use 2 in that case, remove required: true and keep default: 2; if the parameter must always be provided by clients, remove the default: 2 and keep required: true. Update the parameter block for "periods" accordingly so generators and clients behave consistently.
261-267:⚠️ Potential issue | 🟡 MinorMake the new
startIndexcontract internally consistent.These parameter blocks still describe
startIndexas 1-based while defaulting it to0. Clients now get conflicting pagination semantics; either describe it as 0-based or change the default to1.Also applies to: 441-447, 537-542, 683-688
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/docs/fido2-plugin-swagger.yaml` around lines 261 - 267, The startIndex parameter blocks are inconsistent (description says 1-based while default is 0); update the description for the startIndex query parameter to read "The 0-based index of the first query result" and keep the schema default as 0 for the startIndex parameter entries (apply the same change to all startIndex parameter blocks in the file so they are consistent).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jans-config-api/plugins/docs/fido2-plugin-swagger.yaml`:
- Around line 107-116: The example blocks currently use block-scalar strings
(value: |) which serialize the JSON as a string and conflict with the declared
JsonNode object schemas; update each example (e.g., the "Response example" under
the examples section) by replacing the block-scalar string payload with a native
YAML mapping/object that mirrors the JSON structure and types (numeric fields as
numbers, booleans as booleans, objects/arrays as YAML maps/lists) so the example
value is a real object rather than a string; apply the same replacement to all
other occurrences noted (lines referenced in the review) so OpenAPI validators
and generators see correctly-typed example values.
In
`@jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java`:
- Line 18: Remove the unused import
com.fasterxml.jackson.core.type.TypeReference from the Jackson.java file — it’s
no longer referenced after refactoring readListValue to use
objectMapper.getTypeFactory().constructCollectionType; simply delete the import
line to clean up the class and avoid the unused-import warning.
---
Duplicate comments:
In `@jans-config-api/plugins/docs/fido2-plugin-swagger.yaml`:
- Around line 900-907: The OpenAPI parameter definition for the query parameter
named "periods" is contradictory because it is marked required: true while also
providing a default: 2; remove one of these to resolve the conflict. If the
server must accept requests without the client sending "periods" and should use
2 in that case, remove required: true and keep default: 2; if the parameter must
always be provided by clients, remove the default: 2 and keep required: true.
Update the parameter block for "periods" accordingly so generators and clients
behave consistently.
- Around line 261-267: The startIndex parameter blocks are inconsistent
(description says 1-based while default is 0); update the description for the
startIndex query parameter to read "The 0-based index of the first query result"
and keep the schema default as 0 for the startIndex parameter entries (apply the
same change to all startIndex parameter blocks in the file so they are
consistent).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a2aaa94-d2ea-4745-a231-e6e842da559d
📒 Files selected for processing (4)
jans-config-api/docs/jans-config-api-swagger.yamljans-config-api/plugins/docs/fido2-plugin-swagger.yamljans-config-api/plugins/docs/user-mgt-plugin-swagger.yamljans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java
jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java
Outdated
Show resolved
Hide resolved
Signed-off-by: pujavs <pujas.works@gmail.com>
Signed-off-by: pujavs <pujas.works@gmail.com>
Contributor
Author
|
@coderabbitai request you to please review again |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
jans-config-api/docs/jans-config-api-swagger.yaml (1)
1855-1862:⚠️ Potential issue | 🟡 MinorUse concrete sample values in date parameter descriptions.
Line 1855 and Line 1861 still present format tokens (
dd-MM-yyyy,yyyy-MM-ddTHH:mm:ssZ) as examples, which remains ambiguous for API consumers. Please publish concrete examples in the upstream endpoint annotations (then regenerate this YAML), e.g.,31-12-2025and2025-12-31T23:59:59Z.Based on learnings,
jans-config-api/docs/jans-config-api-swagger.yamlis auto-generated, so this should be fixed in source annotations and regenerated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/docs/jans-config-api-swagger.yaml` around lines 1855 - 1862, Update the source API endpoint annotations that generate the start_date and end_date query parameter docs so they use concrete example values instead of format tokens: replace the placeholders `dd-MM-yyyy` and `yyyy-MM-ddTHH:mm:ssZ` for the parameters named start_date and end_date with explicit examples (e.g., start_date: "31-12-2025" and start_date/end_date ISO example: "2025-12-31T23:59:59Z") in the annotation strings, then regenerate the swagger YAML so jans-config-api/docs/jans-config-api-swagger.yaml reflects those concrete examples.jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java (5)
97-100:⚠️ Potential issue | 🟠 MajorRemove
@NotNullfrom optional date query params.
start_date/end_dateare handled as optional in method logic (StringUtils.isNotBlank) but@NotNullmakes them required before handler execution.Suggested fix
- `@QueryParam`(value = "start_date") `@NotNull` String startDate, - `@QueryParam`(value = "end_date") `@NotNull` String endDate) + `@QueryParam`(value = "start_date") String startDate, + `@QueryParam`(value = "end_date") String endDate)Also applies to: 176-178, 258-260, 342-343, 422-423, 495-496, 566-567, 637-638, 708-709, 780-781
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 97 - 100, The request parameters startDate and endDate in Fido2MetricsResource are annotated with `@NotNull` but treated as optional (checked via StringUtils.isNotBlank), so remove the `@NotNull` annotation from the start_date/end_date `@QueryParam` declarations across the class (e.g., in the method signatures that declare startDate and endDate in Fido2MetricsResource) and for the other duplicate occurrences noted (the additional method parameter declarations referenced in the comment). Ensure only the `@QueryParam/`@Parameter annotations remain so the parameters are truly optional at runtime.
1031-1039:⚠️ Potential issue | 🟠 Major
parseDate()cannot parse date-only strings intoLocalDateTime.
ISO_LOCAL_DATEanddd-MM-yyyyare date-only formats; parsing them directly asLocalDateTimethrowsDateTimeParseException.Suggested fix
+import java.time.LocalDate; @@ - // Try ISO date only (e.g. 2024-02-13) + // Try ISO date only (e.g. 2024-02-13) try { - return LocalDateTime.parse(trimmed, DateTimeFormatter.ISO_LOCAL_DATE); + return LocalDate.parse(trimmed, DateTimeFormatter.ISO_LOCAL_DATE).atStartOfDay(); } catch (DateTimeParseException ignored) { // continue } // Fallback to legacy dd-MM-yyyy - return LocalDateTime.parse(trimmed, DateTimeFormatter.ofPattern(METRICS_DATE_FORMAT)); + return LocalDate.parse(trimmed, fallbackFormatter).atStartOfDay();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 1031 - 1039, The parseDate() logic attempts to parse date-only strings with DateTimeFormatter.ISO_LOCAL_DATE and the dd-MM-yyyy pattern directly into LocalDateTime, which fails; update the ISO_LOCAL_DATE and METRICS_DATE_FORMAT branches to first parse into LocalDate (e.g., LocalDate.parse(trimmed, ...)) and then convert to LocalDateTime by calling atStartOfDay() before returning, keeping the method return type LocalDateTime and retaining the final fallback LocalDateTime.parse usage for full date-time strings.
181-185:⚠️ Potential issue | 🟠 MajorAvoid logging raw
userIdat info level.This logs caller-provided identifier data into standard logs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 181 - 185, The info-level log in Fido2MetricsResource (logger.info call that currently includes userId) is logging a caller-provided identifier; remove or redact the raw userId from that info log to avoid sensitive data exposure—locate the logger.info in Fido2MetricsResource (the "Fido2MetricsEntry for user search param ..." statement) and either omit userId from the parameter list or replace it with a masked/hashed value (or move full userId logging to a debug-level log after applying a redaction function) while keeping other fields and using escapeLog as needed.
1048-1058:⚠️ Potential issue | 🟠 MajorPaging bounds are still unsafe and can throw on empty/out-of-range inputs.
validateStartIndex()skips empty lists, andsubList(startIndex, toIndex)can still execute with invalid bounds (e.g., empty list + non-zero startIndex). Also, validation viaentriesList.get(startIndex)rejectsstartIndex == size, which should be valid for an empty page window.Suggested fix
- validateStartIndex(identityProviderList, startIndex); - int toIndex = (startIndex + limit <= listSize) ? startIndex + limit : listSize; + startIndex = validateStartIndex(identityProviderList, startIndex); + int safeLimit = Math.max(0, limit); + int toIndex = Math.min(startIndex + safeLimit, listSize); @@ - List<Fido2MetricsEntry> sublist = (identityProviderList == null) ? null - : identityProviderList.subList(startIndex, toIndex); + List<Fido2MetricsEntry> sublist = (identityProviderList == null || startIndex >= toIndex) + ? Collections.emptyList() + : identityProviderList.subList(startIndex, toIndex); @@ - private void validateStartIndex(List<?> entriesList, int startIndex) { + private int validateStartIndex(List<?> entriesList, int startIndex) { @@ - if (entriesList != null && !entriesList.isEmpty()) { - logger.debug("Validate startIndex entriesList.size():{}", entriesList.size()); - try { - entriesList.get(startIndex); - } catch (IndexOutOfBoundsException ioe) { - logger.error("Error while getting data startIndex:{}", startIndex, ioe); - throwBadRequestException("Page start index incorrect, total entries:{" + entriesList.size() - + "}, but provided:{" + startIndex + "} "); - } + int size = entriesList == null ? 0 : entriesList.size(); + if (startIndex < 0 || startIndex > size) { + throwBadRequestException("Page start index incorrect, total entries:{" + size + + "}, but provided:{" + startIndex + "} "); } + return startIndex; }Also applies to: 1079-1089, 1105-1118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 1048 - 1058, The paging code can still throw IndexOutOfBounds for empty or boundary cases; update validateStartIndex and the pagination logic in Fido2MetricsResource so it treats a null/empty identityProviderList as returning an empty page and allows startIndex == listSize (empty page) but rejects startIndex > listSize, compute toIndex = Math.min(startIndex + limit, listSize) after that check, and only call identityProviderList.subList(startIndex, toIndex) when identityProviderList is non-null and startIndex <= toIndex; reference validateStartIndex(...), identityProviderList, startIndex, limit, toIndex, and the subList call to locate and fix the logic (apply same fix for the other blocks around the class).
134-137:⚠️ Potential issue | 🟠 MajorPreserve upstream/client HTTP statuses instead of rewriting all errors to 500.
The catch-all
Exceptionpath masksWebApplicationException(e.g., validation 400 and upstream 4xx/5xx) and always returns 500.Suggested fix
- } catch (Exception ex) { + } catch (WebApplicationException ex) { + throw ex; + } catch (Exception ex) { logger.error(ERR_MSG, ex); throwInternalServerException(ex); }Also applies to: 215-218, 298-301, 380-383, 457-460, 528-531, 599-602, 670-672, 741-744, 815-818, 868-871, 912-915, 955-958
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 134 - 137, The generic catch blocks in Fido2MetricsResource currently convert all exceptions to a 500 via throwInternalServerException; update each catch (Exception ex) block to first check if ex is an instance of javax.ws.rs.WebApplicationException and, if so, rethrow it (throw (WebApplicationException) ex) so the original HTTP status and response are preserved; otherwise log the error as before and call throwInternalServerException(ex). Apply this change to all catch blocks in the Fido2MetricsResource class (the blocks using logger.error(ERR_MSG, ex) and throwInternalServerException(ex)).jans-config-api/plugins/docs/fido2-plugin-swagger.yaml (1)
262-268:⚠️ Potential issue | 🟡 MinorMake
startIndexsemantics consistent (description vs default).These parameters are documented as 1-based while default remains
0. Align by either changing description to 0-based or default to1.Also applies to: 442-448, 537-543, 683-689
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/docs/fido2-plugin-swagger.yaml` around lines 262 - 268, The startIndex query parameter is documented as 1-based but has a default of 0; make the semantics consistent by updating the schema default to 1 for every parameter block named startIndex (replace schema.default: 0 with schema.default: 1) so the description ("The 1-based index of the first query result") matches the default; apply the same fix to all other startIndex parameter occurrences in the file (all blocks with name: startIndex).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java`:
- Around line 49-50: The PARAM log template in Fido2MetricsResource currently
has four placeholders but the logging calls only pass two values; either update
the PARAM constant to match the actual arguments (e.g., change to "Search param
- limit:{}, startIndex:{}") or update each logging call to supply the missing
startDate and endDate values; locate the PARAM constant and all uses in
Fido2MetricsResource (the logging statements around where metrics queries are
built — the same spots noted in the review) and make the template and arguments
consistent so the number of {} placeholders matches the number of provided
values.
In
`@jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java`:
- Around line 21-64: Add tests to cover the newly added metrics handlers and
non-200 scenarios in addition to the existing happy-paths: extend the test class
by creating new parameterized test methods (mirroring getFido2MetricsEntry,
getFido2UserMetrics, getFido2MetricsConfig) that call each new endpoint handler
added in the PR, and add negative-case tests that simulate and assert
401/403/404/500 responses; reuse the same Builder flow
(getResteasyService().getClientBuilder(...), set AUTHORIZATION and CONTENT_TYPE
headers, call request.get()) but assert non-OK Status codes and validate error
payloads or headers, and add tests for empty/invalid input returns where
applicable to ensure parity and regression safety.
- Around line 30-33: The Response returned by request.get() in the
Fido2MetricsTest tests (e.g., the getFido2MetricsEntry() test where you call
Response response = request.get()) isn’t closed; wrap the request.get() call in
a try-with-resources (try (Response response = request.get()) { ... }) in
getFido2MetricsEntry() and the two other test methods that call request.get() so
the jakarta.ws.rs.core.Response is automatically closed and avoids test-time
resource leaks.
In
`@jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java`:
- Around line 159-177: The three deserialization helpers (readStringValue,
readListValue, readList) currently instantiate new ObjectMapper() without
configuration; update them to use the same configured mapper as the rest of the
class (i.e., an ObjectMapper with
DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES disabled or the existing
configured/shared mapper used by applyPatch/getObject) so unknown properties are
ignored during reads; modify the mapper instantiation in readStringValue,
readListValue, readList (and the overloaded readList with elementType) to
obtain/configure the mapper consistently rather than calling new ObjectMapper()
directly.
---
Duplicate comments:
In `@jans-config-api/docs/jans-config-api-swagger.yaml`:
- Around line 1855-1862: Update the source API endpoint annotations that
generate the start_date and end_date query parameter docs so they use concrete
example values instead of format tokens: replace the placeholders `dd-MM-yyyy`
and `yyyy-MM-ddTHH:mm:ssZ` for the parameters named start_date and end_date with
explicit examples (e.g., start_date: "31-12-2025" and start_date/end_date ISO
example: "2025-12-31T23:59:59Z") in the annotation strings, then regenerate the
swagger YAML so jans-config-api/docs/jans-config-api-swagger.yaml reflects those
concrete examples.
In `@jans-config-api/plugins/docs/fido2-plugin-swagger.yaml`:
- Around line 262-268: The startIndex query parameter is documented as 1-based
but has a default of 0; make the semantics consistent by updating the schema
default to 1 for every parameter block named startIndex (replace schema.default:
0 with schema.default: 1) so the description ("The 1-based index of the first
query result") matches the default; apply the same fix to all other startIndex
parameter occurrences in the file (all blocks with name: startIndex).
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java`:
- Around line 97-100: The request parameters startDate and endDate in
Fido2MetricsResource are annotated with `@NotNull` but treated as optional
(checked via StringUtils.isNotBlank), so remove the `@NotNull` annotation from the
start_date/end_date `@QueryParam` declarations across the class (e.g., in the
method signatures that declare startDate and endDate in Fido2MetricsResource)
and for the other duplicate occurrences noted (the additional method parameter
declarations referenced in the comment). Ensure only the `@QueryParam/`@Parameter
annotations remain so the parameters are truly optional at runtime.
- Around line 1031-1039: The parseDate() logic attempts to parse date-only
strings with DateTimeFormatter.ISO_LOCAL_DATE and the dd-MM-yyyy pattern
directly into LocalDateTime, which fails; update the ISO_LOCAL_DATE and
METRICS_DATE_FORMAT branches to first parse into LocalDate (e.g.,
LocalDate.parse(trimmed, ...)) and then convert to LocalDateTime by calling
atStartOfDay() before returning, keeping the method return type LocalDateTime
and retaining the final fallback LocalDateTime.parse usage for full date-time
strings.
- Around line 181-185: The info-level log in Fido2MetricsResource (logger.info
call that currently includes userId) is logging a caller-provided identifier;
remove or redact the raw userId from that info log to avoid sensitive data
exposure—locate the logger.info in Fido2MetricsResource (the "Fido2MetricsEntry
for user search param ..." statement) and either omit userId from the parameter
list or replace it with a masked/hashed value (or move full userId logging to a
debug-level log after applying a redaction function) while keeping other fields
and using escapeLog as needed.
- Around line 1048-1058: The paging code can still throw IndexOutOfBounds for
empty or boundary cases; update validateStartIndex and the pagination logic in
Fido2MetricsResource so it treats a null/empty identityProviderList as returning
an empty page and allows startIndex == listSize (empty page) but rejects
startIndex > listSize, compute toIndex = Math.min(startIndex + limit, listSize)
after that check, and only call identityProviderList.subList(startIndex,
toIndex) when identityProviderList is non-null and startIndex <= toIndex;
reference validateStartIndex(...), identityProviderList, startIndex, limit,
toIndex, and the subList call to locate and fix the logic (apply same fix for
the other blocks around the class).
- Around line 134-137: The generic catch blocks in Fido2MetricsResource
currently convert all exceptions to a 500 via throwInternalServerException;
update each catch (Exception ex) block to first check if ex is an instance of
javax.ws.rs.WebApplicationException and, if so, rethrow it (throw
(WebApplicationException) ex) so the original HTTP status and response are
preserved; otherwise log the error as before and call
throwInternalServerException(ex). Apply this change to all catch blocks in the
Fido2MetricsResource class (the blocks using logger.error(ERR_MSG, ex) and
throwInternalServerException(ex)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0170b9db-ae20-4248-a930-5a3b07bc7c13
📒 Files selected for processing (7)
jans-auth-server/common/src/main/java/io/jans/as/common/model/session/SessionId.javajans-config-api/docs/jans-config-api-swagger.yamljans-config-api/plugins/docs/fido2-plugin-swagger.yamljans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/ApiApplication.javajans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.javajans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.javajans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java
Comment on lines
+49
to
+50
| private static final String PARAM = "Search param - limit:{}, startIndex:{}, startDate:{}, endDate:{}"; | ||
| private static final String ERR_MSG = "Exception while getting metric data is - "; |
Contributor
There was a problem hiding this comment.
Fix log template/argument mismatch for PARAM.
PARAM expects 4 placeholders, but these calls provide only 2 values, which produces misleading logs.
Also applies to: 499-501, 570-572, 641-643, 712-714
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java`
around lines 49 - 50, The PARAM log template in Fido2MetricsResource currently
has four placeholders but the logging calls only pass two values; either update
the PARAM constant to match the actual arguments (e.g., change to "Search param
- limit:{}, startIndex:{}") or update each logging call to supply the missing
startDate and endDate values; locate the PARAM constant and all uses in
Fido2MetricsResource (the logging statements around where metrics queries are
built — the same spots noted in the review) and make the template and arguments
consistent so the number of {} placeholders matches the number of provided
values.
Comment on lines
+21
to
+64
| @Parameters({ "test.issuer", "fido2MetricsUrl" , "fido2MetricsEntries"}) | ||
| @Test | ||
| public void getFido2MetricsEntry(final String issuer, final String fido2MetricsUrl, final String fido2MetricsEntries) { | ||
| log.debug(" getFido2MetricsEntry() - issuer:{}, fido2MetricsUrl:{}, fido2MetricsEntries:{}", issuer, fido2MetricsUrl, fido2MetricsEntries); | ||
|
|
||
| Builder request = getResteasyService().getClientBuilder(issuer + fido2MetricsUrl + fido2MetricsEntries); | ||
| request.header(AUTHORIZATION, AUTHORIZATION_TYPE + " " + accessToken); | ||
| request.header(CONTENT_TYPE, MediaType.APPLICATION_JSON); | ||
|
|
||
| Response response = request.get(); | ||
| log.info("getFido2MetricsEntry() - response:{}, response.getStatus():{}", response, response.getStatus()); | ||
| assertEquals(response.getStatus(), Status.OK.getStatusCode()); | ||
|
|
||
| } | ||
|
|
||
| @Parameters({ "test.issuer", "fido2MetricsUrl" , "fido2UserMetrics"}) | ||
| @Test | ||
| public void getFido2UserMetrics(final String issuer, final String fido2MetricsUrl, final String fido2UserMetrics) { | ||
| log.debug("\n\n getFido2UserMetrics() - issuer:{}, fido2MetricsUrl:{}, fido2UserMetrics:{}", issuer, fido2MetricsUrl, fido2UserMetrics); | ||
|
|
||
| Builder request = getResteasyService().getClientBuilder(issuer + fido2MetricsUrl + fido2UserMetrics); | ||
| request.header(AUTHORIZATION, AUTHORIZATION_TYPE + " " + accessToken); | ||
| request.header(CONTENT_TYPE, MediaType.APPLICATION_JSON); | ||
|
|
||
| Response response = request.get(); | ||
| log.info("getFido2UserMetrics() - response:{}, response.getStatus():{}", response, response.getStatus()); | ||
| assertEquals(response.getStatus(), Status.OK.getStatusCode()); | ||
|
|
||
| } | ||
|
|
||
| @Parameters({ "test.issuer", "fido2MetricsUrl" , "fido2MetricsConfig"}) | ||
| @Test | ||
| public void getFido2MetricsConfig(final String issuer, final String fido2MetricsUrl, final String fido2MetricsConfig) { | ||
| log.debug("getFido2MetricsConfig() - issuer:{}, fido2MetricsUrl:{}, fido2MetricsConfig:{}", issuer, fido2MetricsUrl, fido2MetricsConfig); | ||
|
|
||
| Builder request = getResteasyService().getClientBuilder(issuer + fido2MetricsUrl + fido2MetricsConfig); | ||
| request.header(AUTHORIZATION, AUTHORIZATION_TYPE + " " + accessToken); | ||
| request.header(CONTENT_TYPE, MediaType.APPLICATION_JSON); | ||
|
|
||
| Response response = request.get(); | ||
| log.info("getFido2MetricsConfig() - response:{}, response.getStatus():{}", response, response.getStatus()); | ||
| assertEquals(response.getStatus(), Status.OK.getStatusCode()); | ||
|
|
||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Expand tests for the newly added metrics endpoints and non-200 scenarios.
Current coverage validates only 3 happy-path endpoints; this PR adds many additional handlers that should be exercised for parity and regression safety.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java`
around lines 21 - 64, Add tests to cover the newly added metrics handlers and
non-200 scenarios in addition to the existing happy-paths: extend the test class
by creating new parameterized test methods (mirroring getFido2MetricsEntry,
getFido2UserMetrics, getFido2MetricsConfig) that call each new endpoint handler
added in the PR, and add negative-case tests that simulate and assert
401/403/404/500 responses; reuse the same Builder flow
(getResteasyService().getClientBuilder(...), set AUTHORIZATION and CONTENT_TYPE
headers, call request.get()) but assert non-OK Status codes and validate error
payloads or headers, and add tests for empty/invalid input returns where
applicable to ensure parity and regression safety.
Comment on lines
+30
to
+33
| Response response = request.get(); | ||
| log.info("getFido2MetricsEntry() - response:{}, response.getStatus():{}", response, response.getStatus()); | ||
| assertEquals(response.getStatus(), Status.OK.getStatusCode()); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and check the file structure
find . -path "*/jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java" -type fRepository: JanssenProject/jans
Length of output: 173
🏁 Script executed:
# Check the file content around the mentioned lines
if [ -f "jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java" ]; then
echo "=== File exists, checking line count ==="
wc -l "jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java"
echo ""
echo "=== Lines 1-70 with content ==="
head -70 "jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java" | cat -n
fiRepository: JanssenProject/jans
Length of output: 3915
🏁 Script executed:
# Check imports to see what Response class is being used
rg "import.*Response" "jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java"Repository: JanssenProject/jans
Length of output: 141
🏁 Script executed:
# Check if there are other tests in the codebase that show how Response is handled
rg -A 2 "Response response = request\.(get|post|put|delete)" \
"jans-config-api/plugins/fido2-plugin/src/test" \
-cRepository: JanssenProject/jans
Length of output: 285
🏁 Script executed:
# Let's look for patterns of Response handling across the test suite
rg "try.*Response" "jans-config-api/plugins/fido2-plugin/src/test" | head -20Repository: JanssenProject/jans
Length of output: 45
🏁 Script executed:
# Check base test class for any resource management patterns
find "jans-config-api/plugins/fido2-plugin/src/test" -name "*BaseTest*" -type fRepository: JanssenProject/jans
Length of output: 163
🏁 Script executed:
# Let's also check if Response.close() is called anywhere in tests
rg "response\.close\(\)" "jans-config-api/plugins/fido2-plugin/src/test"Repository: JanssenProject/jans
Length of output: 45
🏁 Script executed:
# Check the base test class
cat "jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/Fido2BaseTest.java" | head -100Repository: JanssenProject/jans
Length of output: 1176
🏁 Script executed:
# Search for how Response is typically used in other test files
rg "Response response" "jans-config-api/plugins/fido2-plugin/src/test" -B 2 -A 5 | head -80Repository: JanssenProject/jans
Length of output: 4872
🌐 Web query:
jakarta.ws.rs.core.Response AutoCloseable close resource management
💡 Result:
jakarta.ws.rs.core.Response is AutoCloseable, so you should manage it like any other closeable resource—typically with try-with-resources—to ensure the underlying entity input stream / HTTP connection gets released back to the client runtime/pool. The JAX-RS Response.close() contract also makes further entity access illegal after closing. [1]
Recommended pattern (client side)
try (Response r = target.request().get()) { // Response is AutoCloseable
if (r.getStatusInfo().getFamily() == Family.SUCCESSFUL) {
String body = r.readEntity(String.class); // usually consumes + closes entity stream
}
} // r.close() called automaticallyTry-with-resources applies because the resource implements AutoCloseable. [2]
When is close() especially important?
- If you don’t fully consume the entity stream, you should still close the
Responseto avoid leaking the connection/resources. Many guides explicitly state thatResponse.close()must be used to release the connection when the invocation returns aResponse. [3] - If you call
readEntity(InputStream.class)(or otherwise take ownership of a stream), you must close the stream/response yourself; otherwise the connection may remain held. (By contrast, for non-stream entity types,readEntity(...)typically consumes the stream and “automatically closes” the unconsumed original entity data stream.) [1]
Server side note
On the server, when you return a Response from a resource method, the JAX-RS runtime handles writing the entity; you generally don’t manually close that Response instance—this guidance is mainly about client-side Response objects.
Sources: Jakarta Response Javadoc (close/readEntity behavior) [1]; Java try-with-resources / AutoCloseable [2]; Red Hat JAX-RS client guidance on Response.close() releasing the connection [3].
Close Response objects to avoid test-time resource leaks.
All three test methods in this file obtain Response objects via request.get() without releasing them. Since jakarta.ws.rs.core.Response implements AutoCloseable, use try-with-resources to ensure proper resource cleanup.
Suggested fix (applies to all 3 test methods)
- Response response = request.get();
- log.info("getFido2MetricsEntry() - response:{}, response.getStatus():{}", response, response.getStatus());
- assertEquals(response.getStatus(), Status.OK.getStatusCode());
+ try (Response response = request.get()) {
+ log.info("getFido2MetricsEntry() - response:{}, response.getStatus():{}", response, response.getStatus());
+ assertEquals(response.getStatus(), Status.OK.getStatusCode());
+ }Also applies to: lines 45–48, 60–63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java`
around lines 30 - 33, The Response returned by request.get() in the
Fido2MetricsTest tests (e.g., the getFido2MetricsEntry() test where you call
Response response = request.get()) isn’t closed; wrap the request.get() call in
a try-with-resources (try (Response response = request.get()) { ... }) in
getFido2MetricsEntry() and the two other test methods that call request.get() so
the jakarta.ws.rs.core.Response is automatically closed and avoids test-time
resource leaks.
Comment on lines
+159
to
+177
| public static <T> T readStringValue(String content, Class<T> clazz) throws JsonProcessingException { | ||
| ObjectMapper mapper = new ObjectMapper(); | ||
| return mapper.readValue(content, clazz); | ||
| } | ||
|
|
||
| public static <T> List<T> readListValue(String content, Class<T> clazz) throws JsonProcessingException { | ||
| ObjectMapper mapper = new ObjectMapper(); | ||
| return mapper.readValue(content, mapper.getTypeFactory().constructCollectionType(List.class, clazz)); | ||
| } | ||
|
|
||
| public static <T> List<T> readList(String str, Class<T> type) throws JsonProcessingException{ | ||
| return readList(str, ArrayList.class, type); | ||
| } | ||
|
|
||
| public static <T> List<T> readList(String str, Class<? extends List> type, Class<T> elementType) | ||
| throws JsonProcessingException { | ||
| final ObjectMapper mapper = new ObjectMapper(); | ||
| return mapper.readValue(str, | ||
| mapper.getTypeFactory().constructCollectionType(type.asSubclass(Collection.class), elementType)); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "Jackson.java" | grep -E "jans-config-api/shared"Repository: JanssenProject/jans
Length of output: 142
🏁 Script executed:
cat -n jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.javaRepository: JanssenProject/jans
Length of output: 10346
🏁 Script executed:
# Also search for JacksonUtils to understand the newMapper() method
find . -type f -name "JacksonUtils.java"Repository: JanssenProject/jans
Length of output: 45
🏁 Script executed:
# Search for usages of the reviewed methods
rg -i "readStringValue|readListValue|readList" --type java -B 2 -A 2 | head -100Repository: JanssenProject/jans
Length of output: 6756
🏁 Script executed:
# Also check if there are any test cases or comments about unknown properties handling
rg -i "unknown.*propert|fail_on_unknown" --type java -B 2 -A 2 | head -100Repository: JanssenProject/jans
Length of output: 5379
Use the configured mapper in these deserialization helpers to handle payloads with unknown properties.
Methods readStringValue, readListValue, and readList use new ObjectMapper() without configuration, unlike similar methods in this class (applyPatch and getObject) that configure FAIL_ON_UNKNOWN_PROPERTIES=false. The codebase consistently disables this feature across deserialization operations to tolerate upstream schema evolution. These methods should follow the same pattern to prevent parse failures.
Suggested fix
+ private static ObjectMapper readMapper() {
+ return JacksonUtils.newMapper().configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
+ }
+
public static <T> T readStringValue(String content, Class<T> clazz) throws JsonProcessingException {
- ObjectMapper mapper = new ObjectMapper();
+ ObjectMapper mapper = readMapper();
return mapper.readValue(content, clazz);
}
public static <T> List<T> readListValue(String content, Class<T> clazz) throws JsonProcessingException {
- ObjectMapper mapper = new ObjectMapper();
+ ObjectMapper mapper = readMapper();
return mapper.readValue(content, mapper.getTypeFactory().constructCollectionType(List.class, clazz));
}
public static <T> List<T> readList(String str, Class<? extends List> type, Class<T> elementType)
throws JsonProcessingException {
- final ObjectMapper mapper = new ObjectMapper();
+ final ObjectMapper mapper = readMapper();
return mapper.readValue(str,
mapper.getTypeFactory().constructCollectionType(type.asSubclass(Collection.class), elementType));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java`
around lines 159 - 177, The three deserialization helpers (readStringValue,
readListValue, readList) currently instantiate new ObjectMapper() without
configuration; update them to use the same configured mapper as the rest of the
class (i.e., an ObjectMapper with
DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES disabled or the existing
configured/shared mapper used by applyPatch/getObject) so unknown properties are
ignored during reads; modify the mapper instantiation in readStringValue,
readListValue, readList (and the overloaded readList with elementType) to
obtain/configure the mapper consistently rather than calling new ObjectMapper()
directly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (14)
jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2ResourceTest.java (1)
24-24:⚠️ Potential issue | 🟠 MajorRemove bearer token from logs (security leak).
Line 24 logs
accessTokenin plaintext and aterrorlevel. This can leak live credentials through CI/test logs. Remove or redact the token and lower log level for routine tracing.Suggested fix
- log.error("\n\n getFido2Configuration() - accessToken:{}, issuer:{}, fido2ConfigUrl:{}", accessToken, issuer, fido2ConfigUrl); + log.info("\n\n getFido2Configuration() - issuer:{}, fido2ConfigUrl:{}", issuer, fido2ConfigUrl);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2ResourceTest.java` at line 24, The test currently logs the raw bearer token at error level in Fido2ResourceTest (see the log.error call in getFido2Configuration() test); remove the plaintext accessToken from the message (or replace it with a redacted placeholder like "<REDACTED_TOKEN>") and lower the severity from error to a less noisy level (debug or info) so credentials are not exposed in CI logs; update the log statement in Fido2ResourceTest.java accordingly while keeping issuer and fido2ConfigUrl as needed.jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java (1)
159-177:⚠️ Potential issue | 🟠 MajorUse the same configured mapper in new deserialization helpers.
Line 160, Line 165, and Line 175 create raw
ObjectMapperinstances, while this class already uses tolerant deserialization (FAIL_ON_UNKNOWN_PROPERTIES=false) elsewhere. This inconsistency can break reads on forward-compatible payloads.🔧 Proposed refactor
+ private static ObjectMapper readMapper() { + return JacksonUtils.newMapper() + .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); + } + public static <T> T readStringValue(String content, Class<T> clazz) throws JsonProcessingException { - ObjectMapper mapper = new ObjectMapper(); + ObjectMapper mapper = readMapper(); return mapper.readValue(content, clazz); } public static <T> List<T> readListValue(String content, Class<T> clazz) throws JsonProcessingException { - ObjectMapper mapper = new ObjectMapper(); + ObjectMapper mapper = readMapper(); return mapper.readValue(content, mapper.getTypeFactory().constructCollectionType(List.class, clazz)); } @@ public static <T> List<T> readList(String str, Class<? extends List> type, Class<T> elementType) throws JsonProcessingException { - final ObjectMapper mapper = new ObjectMapper(); + final ObjectMapper mapper = readMapper(); return mapper.readValue(str, mapper.getTypeFactory().constructCollectionType(type.asSubclass(Collection.class), elementType)); }#!/bin/bash set -euo pipefail echo "Verify mapper construction in Jackson helper methods:" fd 'Jackson.java' | rg 'jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java' | while read -r f; do sed -n '150,190p' "$f" done echo echo "Find all direct ObjectMapper constructions in this utility:" rg -n --type java 'new ObjectMapper\(' -C2 echo echo "Find call sites that depend on these helpers:" rg -n --type java 'Jackson\.readStringValue\(|Jackson\.readListValue\(|Jackson\.readList\(' -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java` around lines 159 - 177, The three helpers readStringValue, readListValue and both readList overloads currently instantiate raw ObjectMapper() instances which bypass the class's configured tolerant mapper; replace each new ObjectMapper() with the shared/configured mapper used elsewhere in this Jackson utility (e.g., the static mapper instance or a getMapper() method), and ensure the same TypeFactory calls (mapper.getTypeFactory().constructCollectionType(...)) and deserialization features (FAIL_ON_UNKNOWN_PROPERTIES=false) are applied so all deserialization uses the consistent, tolerant configuration.jans-config-api/plugins/fido2-plugin/src/test/resources/testng.xml (1)
18-22:⚠️ Potential issue | 🟡 MinorRename duplicate TestNG test block for clarity.
Both
<test>elements are namedFido2(lines 12 and 18), which creates ambiguity in TestNG reports and name-based filtering. Rename this block to match its test class.🔧 Suggested fix
- <test name="Fido2" enabled="true"> + <test name="Fido2Metrics" enabled="true"> <classes> <class name="io.jans.configapi.plugin.fido2.test.Fido2MetricsTest" /> </classes> </test>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/test/resources/testng.xml` around lines 18 - 22, Rename the duplicate TestNG <test> block named "Fido2" to a unique, descriptive name matching its test class; update the test element that contains the class io.jans.configapi.plugin.fido2.test.Fido2MetricsTest so its name attribute is e.g. "Fido2MetricsTest" (or similar unique identifier) to avoid ambiguity in TestNG reports and name-based filtering.jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java (6)
183-185:⚠️ Potential issue | 🟠 MajorAvoid logging raw
userIdat info level.Line 183-185 records caller user identifiers in standard logs; prefer redaction/hash or debug-only logging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 183 - 185, The info-level log in Fido2MetricsResource that prints "Fido2MetricsEntry for user search param..." currently includes the raw userId variable; change this to avoid logging identifiable user IDs at info level by either substituting a redacted or hashed value for userId (e.g., mask or hash before passing to the logger) or by moving the full userId logging to debug-only; update the logging call that passes escapeLog(userId) (alongside escapeLog(limit), escapeLog(startIndex), escapeLog(startDate), escapeLog(endDate)) to use the redacted/hashed value or conditionally log the raw userId at debug level instead.
49-50:⚠️ Potential issue | 🟡 MinorFix
PARAMplaceholder/argument mismatch.Line 49 defines four placeholders, but these calls pass only two values.
Also applies to: 499-501, 570-572, 641-643, 712-714
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 49 - 50, The PARAM log template in Fido2MetricsResource currently has four placeholders but calling sites supply only two arguments; update the PARAM constant (and any duplicate constants at the other occurrences) to match the actual arguments — e.g., change PARAM to "Search param - limit:{}, startIndex:{}" (or alternatively update the calling sites to pass startDate and endDate as the 3rd and 4th args) so the placeholder/argument counts match; keep ERR_MSG unchanged. Ensure you update the PARAM constant used by methods that log search parameters so no placeholder mismatch remains.
1051-1053:⚠️ Potential issue | 🟠 MajorPagination still applies
startIndexwith wrong index base.
startIndexis documented 1-based, but subList/validation treat it as 0-based, producing off-by-one failures and incorrect page windows.♻️ Suggested change
- validateStartIndex(identityProviderList, startIndex); - int toIndex = (startIndex + limit <= listSize) ? startIndex + limit : listSize; + int zeroBasedStart = validateStartIndex(identityProviderList, startIndex); + int toIndex = Math.min(zeroBasedStart + limit, listSize); @@ - List<Fido2MetricsEntry> sublist = (identityProviderList == null) ? null - : identityProviderList.subList(startIndex, toIndex); + List<Fido2MetricsEntry> sublist = (identityProviderList == null) ? null + : identityProviderList.subList(zeroBasedStart, toIndex); @@ - fido2MetricsEntryPagedResult.setStart(startIndex); + fido2MetricsEntryPagedResult.setStart(startIndex); @@ - private void validateStartIndex(List<?> entriesList, int startIndex) { + private int validateStartIndex(List<?> entriesList, int startIndex) { @@ - if (entriesList != null && !entriesList.isEmpty()) { - logger.debug("Validate startIndex entriesList.size():{}", entriesList.size()); - try { - entriesList.get(startIndex); - } catch (IndexOutOfBoundsException ioe) { - logger.error("Error while getting data startIndex:{}", startIndex, ioe); - throwBadRequestException("Page start index incorrect, total entries:{" + entriesList.size() - + "}, but provided:{" + startIndex + "} "); - } + int listSize = entriesList == null ? 0 : entriesList.size(); + if (startIndex < 1 || startIndex > Math.max(1, listSize)) { + throwBadRequestException("Page start index incorrect, total entries:{" + listSize + + "}, but provided:{" + startIndex + "} "); } + return startIndex - 1; }Also applies to: 1057-1057, 1082-1084, 1088-1088, 1110-1117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 1051 - 1053, The pagination logic treats the documented 1-based startIndex as 0-based when validating and slicing lists (see validateStartIndex, calculation of toIndex, and subList usages), causing off-by-one windows; update all places (including the blocks around validateStartIndex, toIndex calculation and any list.subList(...) calls at the mentioned locations) to convert the incoming 1-based startIndex to a 0-based internal index (e.g., internalIndex = startIndex - 1) before validation and slicing, ensure boundary checks use list size correctly, and log/return values consistently using the external 1-based startIndex where required.
98-99:⚠️ Potential issue | 🟠 MajorRemove
@NotNullfrom optionalstart_date/end_datequery params.Line 98/99 (and repeats) conflict with the documented optional behavior and the existing blank checks.
♻️ Suggested change
-@QueryParam(value = "start_date") `@NotNull` String startDate, -@QueryParam(value = "end_date") `@NotNull` String endDate +@QueryParam(value = "start_date") String startDate, +@QueryParam(value = "end_date") String endDateApply across all metrics handlers.
#!/bin/bash rg -n "@NotNull String startDate|@NotNull String endDate" jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.javaAlso applies to: 177-178, 259-260, 342-343, 422-423, 495-496, 566-567, 637-638, 708-709, 780-781
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 98 - 99, The `@NotNull` annotation is wrongly applied to optional query parameters start_date and end_date in Fido2MetricsResource; remove `@NotNull` from the method parameter declarations (the String startDate and String endDate parameters) in each metrics handler method in class Fido2MetricsResource so the parameters can be optional and rely on existing blank/null checks; apply the same change to every duplicate occurrence listed (all handlers around the reported lines) so the annotations match the documented optional behavior.
134-137:⚠️ Potential issue | 🟠 MajorDo not collapse client/upstream HTTP errors into 500.
Line 134 catches all exceptions and rewrites them to internal-server errors, which can mask valid 4xx/5xx propagation.
♻️ Suggested change
- } catch (Exception ex) { + } catch (WebApplicationException wae) { + throw wae; + } catch (Exception ex) { logger.error(ERR_MSG, ex); throwInternalServerException(ex); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 134 - 137, The catch-all in Fido2MetricsResource (catch (Exception ex)) collapses upstream/client HTTP errors into 500; change the error handling to preserve and propagate upstream status: catch specific exceptions (e.g., WebApplicationException, ProcessingException or the client-specific exception type), and if ex is a WebApplicationException (or contains an upstream Response), rethrow it or return Response.status(((WebApplicationException)ex).getResponse().getStatus()).entity(...). For other unexpected exceptions keep logger.error(ERR_MSG, ex) and throwInternalServerException(ex); update the catch block around the code that calls the HTTP client so client-originated HTTP errors are propagated instead of always mapping to 500.
1031-1034:⚠️ Potential issue | 🟠 MajorDate-only parsing branch is incorrect for
LocalDateTime.Line 1033 and Line 1039 parse date-only values into
LocalDateTime, which causesDateTimeParseExceptionfor validdd-MM-yyyy/yyyy-MM-ddinputs.♻️ Suggested change
+import java.time.LocalDate; @@ - try { - return LocalDateTime.parse(trimmed, DateTimeFormatter.ISO_LOCAL_DATE); + try { + return LocalDate.parse(trimmed, DateTimeFormatter.ISO_LOCAL_DATE).atStartOfDay(); } catch (DateTimeParseException ignored) { // continue } @@ - return LocalDateTime.parse(trimmed, DateTimeFormatter.ofPattern(METRICS_DATE_FORMAT)); + return LocalDate.parse(trimmed, DateTimeFormatter.ofPattern(METRICS_DATE_FORMAT)).atStartOfDay();#!/bin/bash rg -n "LocalDateTime.parse\\(trimmed, DateTimeFormatter.ISO_LOCAL_DATE\\)|LocalDateTime.parse\\(trimmed, DateTimeFormatter.ofPattern\\(METRICS_DATE_FORMAT\\)\\)" jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.javaAlso applies to: 1038-1039
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java` around lines 1031 - 1034, The code is incorrectly calling LocalDateTime.parse(...) for date-only strings; in Fido2MetricsResource replace LocalDateTime.parse(trimmed, DateTimeFormatter.ISO_LOCAL_DATE) and LocalDateTime.parse(trimmed, DateTimeFormatter.ofPattern(METRICS_DATE_FORMAT)) with LocalDate.parse(...).atStartOfDay() (e.g., LocalDate.parse(trimmed, DateTimeFormatter.ISO_LOCAL_DATE).atStartOfDay()) so date-only inputs produce a LocalDateTime at start of day; adjust exception handling blocks around these calls accordingly to catch DateTimeParseException from LocalDate.parse.jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java (1)
30-33:⚠️ Potential issue | 🟡 MinorClose JAX-RS
Responseobjects in tests.Each
request.get()result should be closed; otherwise test runs can leak client resources.♻️ Suggested change
- Response response = request.get(); - log.info("getFido2MetricsEntry() - response:{}, response.getStatus():{}", response, response.getStatus()); - assertEquals(response.getStatus(), Status.OK.getStatusCode()); + try (Response response = request.get()) { + log.info("getFido2MetricsEntry() - response:{}, response.getStatus():{}", response, response.getStatus()); + assertEquals(response.getStatus(), Status.OK.getStatusCode()); + }Apply the same pattern to Line 45 and Line 60 blocks.
#!/bin/bash rg -n "Response response = request.get\\(\\);" jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java rg -n "try \\(Response response = request.get\\(\\)\\)" jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.javaAlso applies to: 45-47, 60-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java` around lines 30 - 33, Tests in Fido2MetricsTest create JAX-RS Response objects with Response response = request.get() and fail to close them, leaking client resources; update each occurrence (the three request.get() calls in class Fido2MetricsTest) to use try-with-resources—e.g., replace "Response response = request.get(); ... assertEquals(...)" with "try (Response response = request.get()) { log.info(...); assertEquals(...); }" so responses are automatically closed (apply same change to the blocks around the getFido2MetricsEntry checks and the other two request.get() usages).jans-config-api/plugins/fido2-plugin/src/test/resources/test.properties (1)
21-22:⚠️ Potential issue | 🟠 MajorParameterize metrics test dates and user id instead of hardcoding.
Line 21 and Line 22 lock tests to one historical day and one specific user UUID, which makes the suite environment-dependent and flaky.
♻️ Suggested change
-fido2MetricsEntries=/entries?start_date=2026-02-20T00:00:00Z&end_date=2026-02-20T23:59:59Z -fido2UserMetrics=/entries/user/e1f32d71-2954-4248-a7c5-a2d68ce39f8d?start_date=2026-02-20T00:00:00Z&end_date=2026-02-20T23:59:59Z +fido2MetricsStartDate=${fido2MetricsStartDate} +fido2MetricsEndDate=${fido2MetricsEndDate} +fido2TestUserId=${fido2TestUserId} +fido2MetricsEntries=/entries?start_date=${fido2MetricsStartDate}&end_date=${fido2MetricsEndDate} +fido2UserMetrics=/entries/user/${fido2TestUserId}?start_date=${fido2MetricsStartDate}&end_date=${fido2MetricsEndDate}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/test/resources/test.properties` around lines 21 - 22, The test properties hardcode a single date range and UUID in fido2MetricsEntries and fido2UserMetrics which makes tests brittle; change these values to parameterized placeholders (e.g., ${TEST_START_DATE}, ${TEST_END_DATE}, ${TEST_USER_ID}) or reference environment/system properties and update the test setup to inject those variables at runtime (ensure the code that reads fido2MetricsEntries and fido2UserMetrics resolves the placeholders or reads env vars so tests can supply different dates and a user id per run).jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java (3)
450-453:⚠️ Potential issue | 🟠 MajorUse
periodsquery parameter, notstartTime.Line 451 forwards the comparison window under the wrong key, so upstream won’t receive the intended parameter.
🔧 Suggested fix
- data.put("startTime", String.valueOf(periods)); + data.put("periods", String.valueOf(periods));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java` around lines 450 - 453, Replace the incorrect query parameter key "startTime" with "periods" when building the data map in Fido2MetricsService before calling getMetricsData; specifically, update the Map<String, String> data population so it uses data.put("periods", String.valueOf(periods)) (located where getPeriodOverPeriodComparisonUrl() + "/" + aggregationType is invoked) so the upstream receives the intended periods parameter.
162-163:⚠️ Potential issue | 🟠 MajorFix duplicated
/in forwarded metrics URLs.Line 162 and similar call sites append
"/" + segmenteven though helper URLs already end with/, producing//in path segments.🔧 Suggested fix
- return getFido2MetricsPagedResult(Fido2MetricsEntry.class, this.getFido2UserMetricsEntriesUrl() + "/" + userId, + return getFido2MetricsPagedResult(Fido2MetricsEntry.class, this.getFido2UserMetricsEntriesUrl() + userId, headers, data); - return getFido2MetricsPagedResult(Fido2MetricsEntry.class, - this.getMetricsEntriesByOperationUrl() + "/" + operationType, headers, data); + return getFido2MetricsPagedResult(Fido2MetricsEntry.class, + this.getMetricsEntriesByOperationUrl() + operationType, headers, data); - return getFido2MetricsPagedResult(Fido2MetricsAggregation.class, - this.getMetricsAggregationsUrl() + "/" + aggregationType, headers, data); + return getFido2MetricsPagedResult(Fido2MetricsAggregation.class, + this.getMetricsAggregationsUrl() + aggregationType, headers, data); - return getMetricsData(this.getMetricsAggregationsUrl() + "/" + aggregationType + "/summary", headers, data); + return getMetricsData(this.getMetricsAggregationsUrl() + aggregationType + "/summary", headers, data); - return getMetricsData(this.getMetricsAnalyticsUrl() + "/adoption", headers, data); + return getMetricsData(this.getMetricsAnalyticsUrl() + "adoption", headers, data); - return getMetricsData(this.getTrendAnalysisUrl() + "/" + aggregationType, headers, data); + return getMetricsData(this.getTrendAnalysisUrl() + aggregationType, headers, data); - return getMetricsData(this.getPeriodOverPeriodComparisonUrl() + "/" + aggregationType, headers, data); + return getMetricsData(this.getPeriodOverPeriodComparisonUrl() + aggregationType, headers, data);Also applies to: 198-199, 233-234, 267-267, 299-299, 427-427, 453-453
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java` around lines 162 - 163, The forwarded metrics URLs are being built with duplicated slashes because callers append "/" + segment to helper methods that already return a trailing slash (e.g., change calls to getFido2MetricsPagedResult in Fido2MetricsService that use this.getFido2UserMetricsEntriesUrl() + "/" + userId); instead of concatenating an extra "/" remove the manual "/" so you pass this.getFido2UserMetricsEntriesUrl() + userId (and likewise for other call sites listed: getFido2UserMetricsEntriesUrl, getFido2DeviceMetricsEntriesUrl, getFido2RpMetricsEntriesUrl, getFido2BackupCodeMetricsEntriesUrl, getFido2CredentialsMetricsEntriesUrl, getFido2AuthenticationMetricsEntriesUrl) into getFido2MetricsPagedResult to avoid producing "//" in the path.
503-511:⚠️ Potential issue | 🔴 CriticalPreserve paged-response shape instead of treating payload as a raw list.
Current parsing reads the full payload as
List<T>and then forcestotalEntriesCount = list.size(). This drops upstream paging metadata and can misparse wrapper responses.🔧 Suggested fix
- if (jsonNode != null) { - fido2MetricsList = Jackson.readList(jsonNode.toPrettyString(), type); - log.debug("Fido2MetricsEntry Two fido2MetricsList:{}", fido2MetricsList); - } + if (jsonNode != null) { + JsonNode entriesNode = jsonNode.has("entries") ? jsonNode.get("entries") : jsonNode; + fido2MetricsList = Jackson.readList(entriesNode.toPrettyString(), type); + log.debug("Fido2MetricsEntry Two fido2MetricsList:{}", fido2MetricsList); + } pagedResultEntry = new PagedResult<>(); pagedResultEntry.setEntries(fido2MetricsList); - pagedResultEntry.setTotalEntriesCount( - (fido2MetricsList == null || fido2MetricsList.size() <= 0) ? 0 : fido2MetricsList.size()); + int totalEntriesCount = (jsonNode != null && jsonNode.has("totalEntriesCount")) + ? jsonNode.get("totalEntriesCount").asInt() + : ((fido2MetricsList == null || fido2MetricsList.isEmpty()) ? 0 : fido2MetricsList.size()); + pagedResultEntry.setTotalEntriesCount(totalEntriesCount);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java` around lines 503 - 511, The code in Fido2MetricsService currently uses Jackson.readList(jsonNode.toPrettyString(), type) to populate fido2MetricsList and then overwrites paging by setting pagedResultEntry.setEntries(fido2MetricsList) and setTotalEntriesCount(fido2MetricsList.size()), which discards any upstream paging metadata; instead, deserialize the jsonNode into a PagedResult<T> (or a wrapper type) using Jackson (e.g., readValue into PagedResult.class with the proper generic type), then set pagedResultEntry.setEntries(pagedResult.getEntries()) and pagedResultEntry.setTotalEntriesCount(pagedResult.getTotalEntriesCount()), guarding for nulls so you preserve the original paging metadata rather than forcing list.size(); update references around Jackson.readList, fido2MetricsList, pagedResultEntry, and any type variable used for the element type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java`:
- Around line 111-127: Consolidate the duplicated header and time-range map
construction in Fido2MetricsService by extracting two helpers (e.g., a private
method buildHeaders(String token) that returns the Map<String,String> with
CONTENT_TYPE and optional AUTHORIZATION, and a private method
buildTimeRangeMap(Instant startTime, Instant endTime) that returns the
Map<String,String> with startTime/endTime when non-null) and replace the
repeated blocks in all methods that currently build those maps (the blocks
around lines referenced in the review). Ensure callers use these helpers where
headers or query data are constructed so the logic is centralized and DRY.
- Around line 109-110: The logs in Fido2MetricsService (e.g., the log call shown
and the similar calls across the class) print bearer tokens/headers in
cleartext; remove sensitive values from INFO logs by not including the
token/headers or by replacing them with a masked value (e.g., "<REDACTED>" or
only logging "Authorization header present"). Update each logging call (the one
shown and the others at the listed locations) to omit the raw token/header, or
if you need the value for diagnostics change the log level to DEBUG and mask the
token before logging; ensure the changes are applied to the methods in
Fido2MetricsService so no bearer tokens/authorization headers are emitted at
INFO level.
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/util/ClientFactory.java`:
- Around line 7-23: The ClientFactory currently creates a JAX-RS Client (field
client) eagerly and never closes it; update ClientFactory to manage lifecycle by
either (A) implementing Closeable/AutoCloseable and adding a public close() that
calls client.close(), then register a JVM shutdown hook in the private
constructor to call instance().close(), or (B) convert ClientFactory into a
CDI-managed singleton (`@ApplicationScoped`) and add a `@PreDestroy` method (e.g.,
destroy() or close()) that calls client.close(); ensure getClientBuilder(String
url) and instance() keep working and reference the same client.
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/util/Fido2Util.java`:
- Around line 91-92: The log in Fido2Util that currently calls logger.info(...)
with the HTTP entity and status code should be lowered to DEBUG to avoid
exposing potentially sensitive response contents; update the call in Fido2Util
(the logger invocation referencing entity and
httpResponse.getStatusLine().getStatusCode()) to logger.debug(...) and, if
available, wrap it with a logger.isDebugEnabled() check to avoid unnecessary
string construction when debug is off.
---
Duplicate comments:
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.java`:
- Around line 183-185: The info-level log in Fido2MetricsResource that prints
"Fido2MetricsEntry for user search param..." currently includes the raw userId
variable; change this to avoid logging identifiable user IDs at info level by
either substituting a redacted or hashed value for userId (e.g., mask or hash
before passing to the logger) or by moving the full userId logging to
debug-only; update the logging call that passes escapeLog(userId) (alongside
escapeLog(limit), escapeLog(startIndex), escapeLog(startDate),
escapeLog(endDate)) to use the redacted/hashed value or conditionally log the
raw userId at debug level instead.
- Around line 49-50: The PARAM log template in Fido2MetricsResource currently
has four placeholders but calling sites supply only two arguments; update the
PARAM constant (and any duplicate constants at the other occurrences) to match
the actual arguments — e.g., change PARAM to "Search param - limit:{},
startIndex:{}" (or alternatively update the calling sites to pass startDate and
endDate as the 3rd and 4th args) so the placeholder/argument counts match; keep
ERR_MSG unchanged. Ensure you update the PARAM constant used by methods that log
search parameters so no placeholder mismatch remains.
- Around line 1051-1053: The pagination logic treats the documented 1-based
startIndex as 0-based when validating and slicing lists (see validateStartIndex,
calculation of toIndex, and subList usages), causing off-by-one windows; update
all places (including the blocks around validateStartIndex, toIndex calculation
and any list.subList(...) calls at the mentioned locations) to convert the
incoming 1-based startIndex to a 0-based internal index (e.g., internalIndex =
startIndex - 1) before validation and slicing, ensure boundary checks use list
size correctly, and log/return values consistently using the external 1-based
startIndex where required.
- Around line 98-99: The `@NotNull` annotation is wrongly applied to optional
query parameters start_date and end_date in Fido2MetricsResource; remove
`@NotNull` from the method parameter declarations (the String startDate and String
endDate parameters) in each metrics handler method in class Fido2MetricsResource
so the parameters can be optional and rely on existing blank/null checks; apply
the same change to every duplicate occurrence listed (all handlers around the
reported lines) so the annotations match the documented optional behavior.
- Around line 134-137: The catch-all in Fido2MetricsResource (catch (Exception
ex)) collapses upstream/client HTTP errors into 500; change the error handling
to preserve and propagate upstream status: catch specific exceptions (e.g.,
WebApplicationException, ProcessingException or the client-specific exception
type), and if ex is a WebApplicationException (or contains an upstream
Response), rethrow it or return
Response.status(((WebApplicationException)ex).getResponse().getStatus()).entity(...).
For other unexpected exceptions keep logger.error(ERR_MSG, ex) and
throwInternalServerException(ex); update the catch block around the code that
calls the HTTP client so client-originated HTTP errors are propagated instead of
always mapping to 500.
- Around line 1031-1034: The code is incorrectly calling
LocalDateTime.parse(...) for date-only strings; in Fido2MetricsResource replace
LocalDateTime.parse(trimmed, DateTimeFormatter.ISO_LOCAL_DATE) and
LocalDateTime.parse(trimmed, DateTimeFormatter.ofPattern(METRICS_DATE_FORMAT))
with LocalDate.parse(...).atStartOfDay() (e.g., LocalDate.parse(trimmed,
DateTimeFormatter.ISO_LOCAL_DATE).atStartOfDay()) so date-only inputs produce a
LocalDateTime at start of day; adjust exception handling blocks around these
calls accordingly to catch DateTimeParseException from LocalDate.parse.
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java`:
- Around line 450-453: Replace the incorrect query parameter key "startTime"
with "periods" when building the data map in Fido2MetricsService before calling
getMetricsData; specifically, update the Map<String, String> data population so
it uses data.put("periods", String.valueOf(periods)) (located where
getPeriodOverPeriodComparisonUrl() + "/" + aggregationType is invoked) so the
upstream receives the intended periods parameter.
- Around line 162-163: The forwarded metrics URLs are being built with
duplicated slashes because callers append "/" + segment to helper methods that
already return a trailing slash (e.g., change calls to
getFido2MetricsPagedResult in Fido2MetricsService that use
this.getFido2UserMetricsEntriesUrl() + "/" + userId); instead of concatenating
an extra "/" remove the manual "/" so you pass
this.getFido2UserMetricsEntriesUrl() + userId (and likewise for other call sites
listed: getFido2UserMetricsEntriesUrl, getFido2DeviceMetricsEntriesUrl,
getFido2RpMetricsEntriesUrl, getFido2BackupCodeMetricsEntriesUrl,
getFido2CredentialsMetricsEntriesUrl, getFido2AuthenticationMetricsEntriesUrl)
into getFido2MetricsPagedResult to avoid producing "//" in the path.
- Around line 503-511: The code in Fido2MetricsService currently uses
Jackson.readList(jsonNode.toPrettyString(), type) to populate fido2MetricsList
and then overwrites paging by setting
pagedResultEntry.setEntries(fido2MetricsList) and
setTotalEntriesCount(fido2MetricsList.size()), which discards any upstream
paging metadata; instead, deserialize the jsonNode into a PagedResult<T> (or a
wrapper type) using Jackson (e.g., readValue into PagedResult.class with the
proper generic type), then set
pagedResultEntry.setEntries(pagedResult.getEntries()) and
pagedResultEntry.setTotalEntriesCount(pagedResult.getTotalEntriesCount()),
guarding for nulls so you preserve the original paging metadata rather than
forcing list.size(); update references around Jackson.readList,
fido2MetricsList, pagedResultEntry, and any type variable used for the element
type.
In
`@jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.java`:
- Around line 30-33: Tests in Fido2MetricsTest create JAX-RS Response objects
with Response response = request.get() and fail to close them, leaking client
resources; update each occurrence (the three request.get() calls in class
Fido2MetricsTest) to use try-with-resources—e.g., replace "Response response =
request.get(); ... assertEquals(...)" with "try (Response response =
request.get()) { log.info(...); assertEquals(...); }" so responses are
automatically closed (apply same change to the blocks around the
getFido2MetricsEntry checks and the other two request.get() usages).
In
`@jans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2ResourceTest.java`:
- Line 24: The test currently logs the raw bearer token at error level in
Fido2ResourceTest (see the log.error call in getFido2Configuration() test);
remove the plaintext accessToken from the message (or replace it with a redacted
placeholder like "<REDACTED_TOKEN>") and lower the severity from error to a less
noisy level (debug or info) so credentials are not exposed in CI logs; update
the log statement in Fido2ResourceTest.java accordingly while keeping issuer and
fido2ConfigUrl as needed.
In `@jans-config-api/plugins/fido2-plugin/src/test/resources/test.properties`:
- Around line 21-22: The test properties hardcode a single date range and UUID
in fido2MetricsEntries and fido2UserMetrics which makes tests brittle; change
these values to parameterized placeholders (e.g., ${TEST_START_DATE},
${TEST_END_DATE}, ${TEST_USER_ID}) or reference environment/system properties
and update the test setup to inject those variables at runtime (ensure the code
that reads fido2MetricsEntries and fido2UserMetrics resolves the placeholders or
reads env vars so tests can supply different dates and a user id per run).
In `@jans-config-api/plugins/fido2-plugin/src/test/resources/testng.xml`:
- Around line 18-22: Rename the duplicate TestNG <test> block named "Fido2" to a
unique, descriptive name matching its test class; update the test element that
contains the class io.jans.configapi.plugin.fido2.test.Fido2MetricsTest so its
name attribute is e.g. "Fido2MetricsTest" (or similar unique identifier) to
avoid ambiguity in TestNG reports and name-based filtering.
In
`@jans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java`:
- Around line 159-177: The three helpers readStringValue, readListValue and both
readList overloads currently instantiate raw ObjectMapper() instances which
bypass the class's configured tolerant mapper; replace each new ObjectMapper()
with the shared/configured mapper used elsewhere in this Jackson utility (e.g.,
the static mapper instance or a getMapper() method), and ensure the same
TypeFactory calls (mapper.getTypeFactory().constructCollectionType(...)) and
deserialization features (FAIL_ON_UNKNOWN_PROPERTIES=false) are applied so all
deserialization uses the consistent, tolerant configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6c7aa076-05bc-455e-be44-eedbcf8ca8eb
📒 Files selected for processing (35)
jans-auth-server/common/src/main/java/io/jans/as/common/model/session/SessionId.javajans-config-api/docs/jans-config-api-swagger.yamljans-config-api/plugins/docs/fido2-plugin-swagger.yamljans-config-api/plugins/docs/scim-plugin-swagger.yamljans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/ApiApplication.javajans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/rest/Fido2MetricsResource.javajans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.javajans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/util/ClientFactory.javajans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/util/Constants.javajans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/util/Fido2Util.javajans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2MetricsTest.javajans-config-api/plugins/fido2-plugin/src/test/java/io/jans/configapi/plugin/fido2/test/Fido2ResourceTest.javajans-config-api/plugins/fido2-plugin/src/test/resources/test.propertiesjans-config-api/plugins/fido2-plugin/src/test/resources/testng.xmljans-config-api/profiles/default/config-api-test.propertiesjans-config-api/profiles/jenkins-config-api.gluu.org/test.propertiesjans-config-api/profiles/local/test.propertiesjans-config-api/server/src/main/java/io/jans/configapi/security/client/AuthClientFactory.javajans-config-api/server/src/main/java/io/jans/configapi/util/AuthUtil.javajans-config-api/server/src/main/resources/config-api-rs-protect.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-analytics-performance-metrics.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-analytics-trends-aggregation-type.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-adoption.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-aggregation-summary.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-aggregations-type.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-analytics-devices.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-analytics-errors.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-by-operation-type.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-by-user.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-configuration.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-data.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-metrics-health-check.jsonjans-config-api/server/src/main/resources/example/fido2/metrics/fido2-period-over-period-comparison.jsonjans-config-api/shared/src/main/java/io/jans/configapi/core/rest/BaseResource.javajans-config-api/shared/src/main/java/io/jans/configapi/core/util/Jackson.java
💤 Files with no reviewable changes (1)
- jans-config-api/server/src/main/java/io/jans/configapi/util/AuthUtil.java
Comment on lines
+109
to
+110
| log.info("Get Fido2 metrics entries with token:{}, startTime:{}, endTime:{}", token, startTime, endTime); | ||
|
|
Contributor
There was a problem hiding this comment.
Do not log bearer tokens or auth headers at INFO level.
token (and headers) are logged in plain text across these methods. This leaks credentials into logs and audit pipelines.
🔧 Suggested fix
- log.info("Get Fido2 metrics entries with token:{}, startTime:{}, endTime:{}", token, startTime, endTime);
+ log.info("Get Fido2 metrics entries with startTime:{}, endTime:{}", startTime, endTime);
- log.info("Fido2MetricsEntryList Data: type:{}, url:{}, headers:{}, data:{}", type, url, headers, data);
+ log.info("Fido2MetricsEntryList Data: type:{}, url:{}, dataKeys:{}", type, url,
+ data == null ? null : data.keySet());
- log.info("Fido2Metrics Data: url:{}, headers:{}, data:{}", url, headers, data);
+ log.info("Fido2Metrics Data: url:{}, dataKeys:{}", url, data == null ? null : data.keySet());Also applies to: 142-143, 177-178, 213-214, 247-248, 279-280, 312-313, 344-345, 375-376, 407-408, 439-440, 462-463, 481-482, 496-497, 521-522
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java`
around lines 109 - 110, The logs in Fido2MetricsService (e.g., the log call
shown and the similar calls across the class) print bearer tokens/headers in
cleartext; remove sensitive values from INFO logs by not including the
token/headers or by replacing them with a masked value (e.g., "<REDACTED>" or
only logging "Authorization header present"). Update each logging call (the one
shown and the others at the listed locations) to omit the raw token/header, or
if you need the value for diagnostics change the log level to DEBUG and mask the
token before logging; ensure the changes are applied to the methods in
Fido2MetricsService so no bearer tokens/authorization headers are emitted at
INFO level.
Comment on lines
+111
to
+127
| // Request headers | ||
| Map<String, String> headers = new HashMap<>(); | ||
| headers.put(CONTENT_TYPE, MediaType.APPLICATION_JSON); | ||
| if (StringUtils.isNotBlank(token)) { | ||
| headers.put(AUTHORIZATION, token); | ||
| } | ||
|
|
||
| // Query Parameter | ||
| Map<String, String> data = new HashMap<>(); | ||
| if (startTime != null) { | ||
| data.put("startTime", startTime.toString()); | ||
| } | ||
|
|
||
| if (endTime != null) { | ||
| data.put("endTime", endTime.toString()); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consolidate duplicated request-header and time-range map construction.
The same header/query-map logic is repeated across nearly every method, increasing drift risk.
♻️ Refactor sketch
+ private Map<String, String> buildHeaders(String token) {
+ Map<String, String> headers = new HashMap<>();
+ headers.put(CONTENT_TYPE, MediaType.APPLICATION_JSON);
+ if (StringUtils.isNotBlank(token)) {
+ headers.put(AUTHORIZATION, token);
+ }
+ return headers;
+ }
+
+ private Map<String, String> buildTimeRange(LocalDateTime startTime, LocalDateTime endTime) {
+ Map<String, String> data = new HashMap<>();
+ if (startTime != null) data.put("startTime", startTime.toString());
+ if (endTime != null) data.put("endTime", endTime.toString());
+ return data;
+ }
- Map<String, String> headers = new HashMap<>();
- headers.put(CONTENT_TYPE, MediaType.APPLICATION_JSON);
- if (StringUtils.isNotBlank(token)) {
- headers.put(AUTHORIZATION, token);
- }
- Map<String, String> data = new HashMap<>();
- if (startTime != null) {
- data.put("startTime", startTime.toString());
- }
- if (endTime != null) {
- data.put("endTime", endTime.toString());
- }
+ Map<String, String> headers = buildHeaders(token);
+ Map<String, String> data = buildTimeRange(startTime, endTime);Also applies to: 145-160, 180-197, 216-231, 250-265, 281-297, 314-330, 346-361, 377-392, 410-425, 442-447, 464-469, 483-488
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/service/Fido2MetricsService.java`
around lines 111 - 127, Consolidate the duplicated header and time-range map
construction in Fido2MetricsService by extracting two helpers (e.g., a private
method buildHeaders(String token) that returns the Map<String,String> with
CONTENT_TYPE and optional AUTHORIZATION, and a private method
buildTimeRangeMap(Instant startTime, Instant endTime) that returns the
Map<String,String> with startTime/endTime when non-null) and replace the
repeated blocks in all methods that currently build those maps (the blocks
around lines referenced in the review). Ensure callers use these helpers where
headers or query data are constructed so the logic is centralized and DRY.
Comment on lines
+7
to
+23
| public class ClientFactory { | ||
| private final static ClientFactory INSTANCE = new ClientFactory(); | ||
|
|
||
| private final Client client; | ||
|
|
||
| private ClientFactory() { | ||
| this.client = ClientBuilder.newClient(); | ||
| } | ||
|
|
||
| public static ClientFactory instance() { | ||
| return INSTANCE; | ||
| } | ||
|
|
||
| public Invocation.Builder getClientBuilder(String url) { | ||
| return client.target(url).request(); | ||
| } | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding lifecycle management for the JAX-RS Client.
The Client instance is created eagerly but never explicitly closed. While many JAX-RS implementations handle cleanup on JVM shutdown, explicit lifecycle management is a best practice for resource-intensive clients.
Consider either:
- Implementing
Closeablewith a shutdown hook, or - Converting to a CDI
@ApplicationScopedbean with@PreDestroycleanup
♻️ Alternative: CDI-managed singleton with lifecycle
-public class ClientFactory {
- private final static ClientFactory INSTANCE = new ClientFactory();
-
- private final Client client;
-
- private ClientFactory() {
- this.client = ClientBuilder.newClient();
- }
-
- public static ClientFactory instance() {
- return INSTANCE;
- }
+import jakarta.annotation.PreDestroy;
+import jakarta.enterprise.context.ApplicationScoped;
+
+@ApplicationScoped
+public class ClientFactory {
+
+ private final Client client = ClientBuilder.newClient();
+
+ `@PreDestroy`
+ public void close() {
+ if (client != null) {
+ client.close();
+ }
+ }
public Invocation.Builder getClientBuilder(String url) {
return client.target(url).request();
}
}Based on learnings: When using HTTP clients in Java services, use a reusable client at the service level, initialized and closed properly to prevent resource leaks.
📝 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.
Suggested change
| public class ClientFactory { | |
| private final static ClientFactory INSTANCE = new ClientFactory(); | |
| private final Client client; | |
| private ClientFactory() { | |
| this.client = ClientBuilder.newClient(); | |
| } | |
| public static ClientFactory instance() { | |
| return INSTANCE; | |
| } | |
| public Invocation.Builder getClientBuilder(String url) { | |
| return client.target(url).request(); | |
| } | |
| } | |
| import jakarta.annotation.PreDestroy; | |
| import jakarta.enterprise.context.ApplicationScoped; | |
| `@ApplicationScoped` | |
| public class ClientFactory { | |
| private final Client client = ClientBuilder.newClient(); | |
| `@PreDestroy` | |
| public void close() { | |
| if (client != null) { | |
| client.close(); | |
| } | |
| } | |
| public Invocation.Builder getClientBuilder(String url) { | |
| return client.target(url).request(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/util/ClientFactory.java`
around lines 7 - 23, The ClientFactory currently creates a JAX-RS Client (field
client) eagerly and never closes it; update ClientFactory to manage lifecycle by
either (A) implementing Closeable/AutoCloseable and adding a public close() that
calls client.close(), then register a JVM shutdown hook in the private
constructor to call instance().close(), or (B) convert ClientFactory into a
CDI-managed singleton (`@ApplicationScoped`) and add a `@PreDestroy` method (e.g.,
destroy() or close()) that calls client.close(); ensure getClientBuilder(String
url) and instance() keep working and reference the same client.
Comment on lines
+91
to
+92
| logger.info("entity:{}, httpResponse.getStatusLine().getStatusCode():{}", entity, | ||
| httpResponse.getStatusLine().getStatusCode()); |
Contributor
There was a problem hiding this comment.
Reduce log level for potentially sensitive HTTP response details.
Logging HTTP entity and status code at INFO level in production could expose sensitive response data. Consider using DEBUG level instead.
🔧 Suggested fix
- logger.info("entity:{}, httpResponse.getStatusLine().getStatusCode():{}", entity,
+ logger.debug("entity:{}, httpResponse.getStatusLine().getStatusCode():{}", entity,
httpResponse.getStatusLine().getStatusCode());📝 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.
Suggested change
| logger.info("entity:{}, httpResponse.getStatusLine().getStatusCode():{}", entity, | |
| httpResponse.getStatusLine().getStatusCode()); | |
| logger.debug("entity:{}, httpResponse.getStatusLine().getStatusCode():{}", entity, | |
| httpResponse.getStatusLine().getStatusCode()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@jans-config-api/plugins/fido2-plugin/src/main/java/io/jans/configapi/plugin/fido2/util/Fido2Util.java`
around lines 91 - 92, The log in Fido2Util that currently calls logger.info(...)
with the HTTP entity and status code should be lowered to DEBUG to avoid
exposing potentially sensitive response contents; update the call in Fido2Util
(the logger invocation referencing entity and
httpResponse.getStatusLine().getStatusCode()) to logger.debug(...) and, if
available, wrap it with a logger.isDebugEnabled() check to avoid unnecessary
string construction when debug is off.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prepare
Description
New endpoints to access FIDO2 metrics endpoints
Target issue
closes #13226
Implementation Details
FIDO2 plugin modified to add new metrics endpoints
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
New APIs / Schema
Improvements
Tests
Chores