fix(jans-fido2): measure user-adoption metrics against the right population

jans-fido2/server/src/main/java/io/jans/fido2/service/metric/Fido2MetricsService.java

@@ -14,6 +14,7 @@
 import io.jans.fido2.model.trust.AttestationTrustDiagnostic;
 import io.jans.as.common.service.common.ApplicationFactory;
 import io.jans.orm.PersistenceEntryManager;
+import io.jans.orm.model.SearchScope;
 import io.jans.orm.search.filter.Filter;
 import jakarta.enterprise.context.ApplicationScoped;
 import jakarta.inject.Inject;
@@ -64,6 +65,9 @@ public class Fido2MetricsService {
     private static final String METRICS_ENTRY_BASE_DN = "ou=fido2-metrics,o=jans";
     private static final String METRICS_AGGREGATION_BASE_DN = "ou=fido2-aggregations,o=jans";
 
+    /** Page size for the paged prior-adopters lookup in {@link #getUsersRegisteredBefore}. */
+    private static final int PRIOR_ADOPTERS_CHUNK_SIZE = 1000;
+
     // ========== METRICS ENTRY OPERATIONS ==========
 
     /**
@@ -360,41 +364,97 @@ public void cleanupOldData(int retentionDays) {
     // ========== ANALYTICS AND REPORTING ==========
 
     /**
-     * Get user adoption metrics
+     * Get user adoption metrics.
+     * <p>
+     * "New" and "returning" are decided against every successful registration on record, not just the
+     * rows inside {@code [startTime, endTime]}: a user only counts as new the first time their
+     * registration succeeds, and as returning if they had already registered before the window began.
+     * {@code adoptionRate} is newUsers against the cumulative population of everyone who has ever
+     * registered as of {@code endTime} — a self-contained figure bounded by how long metrics entries
+     * are retained, not a rate against the full identity directory.
      */
     public Map<String, Object> getUserAdoptionMetrics(LocalDateTime startTime, LocalDateTime endTime) {
         List<Fido2MetricsEntry> entries = getMetricsEntries(startTime, endTime);
-        
+
         Map<String, Object> metrics = new HashMap<>();
-        
-        // Total unique users
+
+        // Every user with any activity in this window, regardless of operation or outcome
         Set<String> uniqueUsers = entries.stream()
             .map(Fido2MetricsEntry::getUserId)
             .filter(Objects::nonNull)
             .collect(Collectors.toSet());
         metrics.put(Fido2MetricsConstants.TOTAL_UNIQUE_USERS, uniqueUsers.size());
 
-        // New users (first registration)
-        Set<String> newUsers = entries.stream()
-            .filter(e -> Fido2MetricsConstants.REGISTRATION.equals(e.getOperationType()) && Fido2MetricsConstants.SUCCESS.equals(e.getStatus()))
+        // Users already known to have registered successfully before this window began
+        Set<String> priorAdopters = getUsersRegisteredBefore(startTime);
+
+        // Registration successes recorded inside this window
+        Set<String> registeredInWindow = entries.stream()
+            .filter(e -> Fido2MetricsConstants.REGISTRATION.equals(e.getOperationType())
+                    && Fido2MetricsConstants.SUCCESS.equals(e.getStatus()))
             .map(Fido2MetricsEntry::getUserId)
             .filter(Objects::nonNull)
             .collect(Collectors.toSet());
+
+        // New users: this is the first time their registration ever succeeded
+        Set<String> newUsers = new HashSet<>(registeredInWindow);
+        newUsers.removeAll(priorAdopters);
         metrics.put(Fido2MetricsConstants.NEW_USERS, newUsers.size());
 
-        // Returning users
+        // Returning users: active this window, and already an adopter before it began.
+        // Computed directly against priorAdopters rather than as uniqueUsers minus newUsers, so a user
+        // who registers a second passkey and signs in within the same window is still counted here.
         Set<String> returningUsers = new HashSet<>(uniqueUsers);
-        returningUsers.removeAll(newUsers);
+        returningUsers.retainAll(priorAdopters);
         metrics.put(Fido2MetricsConstants.RETURNING_USERS, returningUsers.size());
 
-        // Adoption rate
-        if (!uniqueUsers.isEmpty()) {
-            metrics.put(Fido2MetricsConstants.ADOPTION_RATE, (double) newUsers.size() / uniqueUsers.size());
+        // Adoption rate: new users against the cumulative population of everyone who has ever
+        // registered as of endTime (priorAdopters and newUsers are disjoint by construction).
+        long cumulativeAdopters = (long) priorAdopters.size() + newUsers.size();
+        if (cumulativeAdopters > 0) {
+            metrics.put(Fido2MetricsConstants.ADOPTION_RATE, (double) newUsers.size() / cumulativeAdopters);
+        } else {
+            metrics.put(Fido2MetricsConstants.ADOPTION_RATE, null);
         }
 
         return metrics;
     }
 
+    /**
+     * Distinct users whose registration succeeded at any point before {@code beforeTime}, searched
+     * directly against the metrics store rather than derived from the {@code [startTime, endTime]}
+     * window. Bounded by the metrics retention policy: a user whose only prior registration entry has
+     * already been cleaned up by {@link #cleanupOldData} will not appear here, and will be reported as
+     * new again.
+     */
+    private Set<String> getUsersRegisteredBefore(LocalDateTime beforeTime) {
+        try {
+            // Strictly before beforeTime, so a registration timestamped exactly at the window's start
+            // is not counted both as a prior adopter and as part of this window.
+            Date exclusiveUpperBound = new Date(convertToDate(beforeTime).getTime() - 1);
+
+            Filter filter = Filter.createANDFilter(
+                Filter.createEqualityFilter("jansFido2MetricsOperationType", Fido2MetricsConstants.REGISTRATION),
+                Filter.createEqualityFilter("jansFido2MetricsStatus", Fido2MetricsConstants.SUCCESS),
+                Filter.createLessOrEqualFilter(Fido2MetricsConstants.JANS_TIMESTAMP, exclusiveUpperBound)
+            );
+
+            // Paged retrieval: the unpaged findEntries(filter) overload issues a single search that a
+            // persistence backend enforcing a result-size limit can reject outright, which would
+            // misclassify every in-window registration as new. Paging in PRIOR_ADOPTERS_CHUNK_SIZE
+            // batches keeps this working past that limit.
+            return persistenceEntryManager.findEntries(METRICS_ENTRY_BASE_DN, Fido2MetricsEntry.class, filter,
+                    SearchScope.SUB, null, 0, 0, PRIOR_ADOPTERS_CHUNK_SIZE)
+                .stream()
+                .map(Fido2MetricsEntry::getUserId)
+                .filter(Objects::nonNull)
+                .collect(Collectors.toSet());
+        } catch (Exception e) {
+            log.error("Failed to retrieve prior registrations before {}: {}", beforeTime, e.getMessage(), e);
+            return Collections.emptySet();
+        }
+    }
+
     /**
      * Whether this entry records a ceremony a user was actually present for.
      * <p>

jans-fido2/server/src/test/java/io/jans/fido2/service/metric/Fido2MetricsServiceTest.java

@@ -12,6 +12,7 @@
 import io.jans.fido2.model.metric.Fido2MetricsData;
 import io.jans.fido2.model.metric.Fido2MetricsEntry;
 import io.jans.orm.PersistenceEntryManager;
+import io.jans.orm.model.SearchScope;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
@@ -32,7 +33,9 @@
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.argThat;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.timeout;
@@ -373,6 +376,115 @@ void aggregation_countsDeviceTypesPerCompletedCeremony() {
         assertEquals(Map.of("PLATFORM", 1L), metrics.get(Fido2MetricsConstants.DEVICE_TYPES));
     }
 
+    /**
+     * A user who already registered before the window and only signs in during it must be reported as
+     * returning, never as new — the prior definition only ever looked at rows inside the window, so an
+     * established user with no registration activity here fell out of both buckets.
+     */
+    @Test
+    void getUserAdoptionMetrics_ifUserRegisteredBeforeWindowAndSignsInDuringIt_isReturningNotNew() {
+        Fido2MetricsEntry signIn = statusEntry(Fido2MetricsConstants.AUTHENTICATION, Fido2MetricsConstants.SUCCESS);
+        signIn.setUserId("user-1");
+        stubAdoptionQueries(List.of(signIn), List.of("user-1"));
+
+        Map<String, Object> metrics = adoption();
+
+        assertEquals(1, metrics.get(Fido2MetricsConstants.TOTAL_UNIQUE_USERS));
+        assertEquals(0, metrics.get(Fido2MetricsConstants.NEW_USERS));
+        assertEquals(1, metrics.get(Fido2MetricsConstants.RETURNING_USERS));
+    }
+
+    /**
+     * The previous formula measured adoption against uniqueUsers active in the window, which shrinks
+     * as new registrations taper off — so it reported near-zero adoption exactly when adoption
+     * finished. A user with no prior registration record is new the first time they register,
+     * regardless of what else happens in the window.
+     */
+    @Test
+    void getUserAdoptionMetrics_ifUserHasNoPriorRegistration_firstSuccessIsNew() {
+        Fido2MetricsEntry registration = statusEntry(Fido2MetricsConstants.REGISTRATION, Fido2MetricsConstants.SUCCESS);
+        registration.setUserId("user-2");
+        stubAdoptionQueries(List.of(registration), List.of());
+
+        Map<String, Object> metrics = adoption();
+
+        assertEquals(1, metrics.get(Fido2MetricsConstants.NEW_USERS));
+        assertEquals(0, metrics.get(Fido2MetricsConstants.RETURNING_USERS));
+        assertEquals(1.0, (Double) metrics.get(Fido2MetricsConstants.ADOPTION_RATE), 0.0001);
+    }
+
+    /**
+     * Enrolling a second passkey must not make an already-adopted user look new again — the old
+     * "newUsers" filter only checked REGISTRATION + SUCCESS inside the window, with no check for a
+     * prior registration, so this changed meaning with the date picker.
+     */
+    @Test
+    void getUserAdoptionMetrics_ifUserEnrolsSecondPasskey_isNotCountedAsNewAgain() {
+        Fido2MetricsEntry secondRegistration = statusEntry(Fido2MetricsConstants.REGISTRATION, Fido2MetricsConstants.SUCCESS);
+        secondRegistration.setUserId("user-3");
+        stubAdoptionQueries(List.of(secondRegistration), List.of("user-3"));
+
+        Map<String, Object> metrics = adoption();
+
+        assertEquals(0, metrics.get(Fido2MetricsConstants.NEW_USERS));
+        assertEquals(1, metrics.get(Fido2MetricsConstants.RETURNING_USERS));
+    }
+
+    /**
+     * adoptionRate is newUsers against everyone who has ever registered as of the end of the window
+     * (prior adopters plus this window's new ones) rather than against uniqueUsers active in the
+     * window, so it keeps meaning "share of all adopters that are new" instead of falling toward zero
+     * as adoption succeeds.
+     */
+    @Test
+    void getUserAdoptionMetrics_adoptionRateIsAgainstCumulativeAdoptersNotWindowActivity() {
+        Fido2MetricsEntry newUser = statusEntry(Fido2MetricsConstants.REGISTRATION, Fido2MetricsConstants.SUCCESS);
+        newUser.setUserId("user-new");
+        stubAdoptionQueries(List.of(newUser), List.of("user-old-1", "user-old-2", "user-old-3"));
+
+        Map<String, Object> metrics = adoption();
+
+        // 1 new user out of 4 cumulative adopters (3 prior + this 1 new)
+        assertEquals(0.25, (Double) metrics.get(Fido2MetricsConstants.ADOPTION_RATE), 0.0001);
+    }
+
+    /**
+     * With nobody ever having registered, there is no population to measure adoption against — the
+     * rate must be left unknown rather than published as a misleading 0.0.
+     */
+    @Test
+    void getUserAdoptionMetrics_ifNoOneHasEverRegistered_adoptionRateIsNull() {
+        stubAdoptionQueries(List.of(), List.of());
+
+        assertNull(adoption().get(Fido2MetricsConstants.ADOPTION_RATE));
+    }
+
+    private Map<String, Object> adoption() {
+        return fido2MetricsService.getUserAdoptionMetrics(LocalDateTime.now().minusDays(1), LocalDateTime.now());
+    }
+
+    /**
+     * getUserAdoptionMetrics issues two distinct queries against the same store: one for activity
+     * inside the window, and one for registrations that succeeded before it began. The window query
+     * carries no status filter, so that is what distinguishes the two for stubbing purposes.
+     */
+    private void stubAdoptionQueries(List<Fido2MetricsEntry> windowEntries, List<String> priorAdopterUserIds) {
+        when(persistenceEntryManager.findEntries(any(String.class), eq(Fido2MetricsEntry.class),
+                argThat(filter -> filter == null || !filter.toString().contains("jansFido2MetricsStatus"))))
+                .thenReturn(windowEntries);
+
+        List<Fido2MetricsEntry> priorEntries = priorAdopterUserIds.stream().map(userId -> {
+            Fido2MetricsEntry entry = statusEntry(Fido2MetricsConstants.REGISTRATION, Fido2MetricsConstants.SUCCESS);
+            entry.setUserId(userId);
+            return entry;
+        }).collect(java.util.stream.Collectors.toList());
+
+        when(persistenceEntryManager.findEntries(any(String.class), eq(Fido2MetricsEntry.class),
+                argThat(filter -> filter != null && filter.toString().contains("jansFido2MetricsStatus")),
+                any(SearchScope.class), any(), anyInt(), anyInt(), anyInt()))
+                .thenReturn(priorEntries);
+    }
+
     private Map<String, Object> performance() {
         return fido2MetricsService.getPerformanceMetrics(LocalDateTime.now().minusDays(1), LocalDateTime.now());
     }