JanssenProject / JanssenProject/jans

fix(jans-fido2): metrics aggregation counts every entry row as an attempt, inflating failure counts

Open
#14,649 1 comment 1 reaction 1 assignee Claimed by @imran-ishaq View on GitHub
comp-jans-fido2 enhancement kind-bug kind-feature
Dominant language
Java
Stars
648
Forks
174
Avg merge
1d 18h
Merged PRs (30d)
110

Description

**Is your feature request related to a problem? Please describe.**

`Fido2MetricsService.calculateAggregation` counts **every** row for an operation type as an attempt, regardless of status, then derives failures by subtraction:

```java
long registrationAttempts = entries.stream()
.filter(e -> Fido2MetricsConstants.REGISTRATION.equals(e.getOperationType()))
.count(); // Fido2MetricsService.java:608-610
long registrationFailures = registrationAttempts - registrationSuccesses; // :617
```

(identical logic for authentication at `:628-637`)

A single successful ceremony writes **two** rows — `ATTEMPT` at `/options` and `SUCCESS` at `/result`. So every success is counted as one attempt, one success, *and* one phantom failure. The persisted `jansFido2MetricsAggregation` rows are wrong as a result.

Measured against a live sample (27 Jul 2026), where the entry table holds 31 registration `ATTEMPT` + 7 registration `SUCCESS`, and 19 authentication `ATTEMPT` + 4 authentication `SUCCESS`:

| Metric | Stored in aggregation | Actual |
|---|---|---|
| registrationAttempts | 38 | 31 |
| registrationFailures | 31 | 0 recorded (24 never completed) |
| registrationSuccessRate | 0.184 | 0.226 |
| authenticationAttempts | 23 | 19 |
| authenticationFailures | 19 | 0 recorded (15 never completed) |
| authenticationSuccessRate | 0.231 | 0.211 |

This is also internally inconsistent: two other methods in the same class already do it correctly — `getErrorAnalysis` tallies `ATTEMPT` / `SUCCESS` / `FAILURE` as distinct statuses (`:494-507`), and `getAttestationRejectionAnalysis` filters attempts on `ATTEMPT` status (`:574-577`). Only `calculateAggregation` does not.

**Describe the solution you'd like**

In `calculateAggregation`, for both registration and authentication:

1. Count attempts as rows with status `ATTEMPT` only, matching `getAttestationRejectionAnalysis`.
2. Count failures as actual `FAILURE` rows rather than deriving them by subtraction.
3. Report ceremonies that started but never completed as their own figure (e.g. `abandoned` / drop-off) instead of folding them into `failures`.
4. Keep success rate as `successes / attempts` — it becomes correct automatically once (1) lands.

Existing `jansFido2MetricsAggregation` rows were computed with the old arithmetic and will stay wrong; recomputation or a note in the release notes is needed.

**Describe alternatives you've considered**

- *Keep subtraction but exclude `SUCCESS`/`FAILURE` rows from the attempt count.* Fixes the double-count, but still labels never-completed ceremonies as failures — misleading in exactly the way the sample above shows.
- *Stop writing the `ATTEMPT` row and only record terminal outcomes.* Would make subtraction valid, but loses drop-off measurement entirely, which is the more useful signal.
- *Fix only at the read/API layer and leave the stored aggregation as-is.* Rejected — the wrong values are persisted, so any direct DB consumer still gets bad data.

**Additional context**

Minor related observation, no action assumed: `deviceTypes` groups on `jansFido2MetricsAuthenticatorType`, which is only populated on `SUCCESS`/`FAILURE` rows (`ATTEMPT` rows pass `null`). In the sample it sums to 11, matching the 11 successes — so it counts completed ceremonies, not attempts. Worth confirming that's intended.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.