feat(metrics_server): do not invalidate legacy report types (#1532)

* Check if `countries` is an array.

* Use `VALID_REPORT` in `postConnectionMetrics` test.

* Make realistic legacy reports and silently drop them during validation.

* Add more explicit support for legacy report types.

* Make diff smaller.

* Update `@typescript-eslint/eslint-plugin` dependency.

* Revert some changes.

* Revert "Update `@typescript-eslint/eslint-plugin` dependency."

This reverts commit a2f3edff09.

* Update test variables to improve readability.

* Add back in `userId` tests.

* Remove check that at least 1 of `userId` or `countries` is set.

* Remove unneeded test.

src/metrics_server/connection_metrics.spec.ts

@@ -32,14 +32,22 @@ const VALID_USER_REPORT2: HourlyUserConnectionMetricsReport = {
 };
 
 /*
- * A user report to test legacy fields to ensure backwards compatibility with
- * older servers that may still send reports like this.
+ * Legacy access key user reports to ensure backwards compatibility with servers not
+ * synced past https://github.com/Jigsaw-Code/outline-server/pull/1529).
  */
-const LEGACY_USER_REPORT = {
+const LEGACY_PER_KEY_USER_REPORT: HourlyUserConnectionMetricsReport = {
   userId: 'foo',
+  bytesTransferred: 123,
+};
+
+/*
+ * Legacy multiple countries user reports to ensure backwards compatibility with servers
+ * not synced past https://github.com/Jigsaw-Code/outline-server/pull/1242.
+ */
+const LEGACY_PER_LOCATION_USER_REPORT: HourlyUserConnectionMetricsReport = {
+  userId: 'foobar',
   countries: ['US', 'UK'],
   bytesTransferred: 123,
-  tunnelTimeSec: 789,
 };
 
 const VALID_REPORT: HourlyConnectionMetricsReport = {
@@ -49,10 +57,17 @@ const VALID_REPORT: HourlyConnectionMetricsReport = {
   userReports: [
     structuredClone(VALID_USER_REPORT),
     structuredClone(VALID_USER_REPORT2),
-    structuredClone(LEGACY_USER_REPORT),
+    structuredClone(LEGACY_PER_LOCATION_USER_REPORT),
   ],
 };
 
+const LEGACY_REPORT: HourlyConnectionMetricsReport = {
+  serverId: 'legacy-id',
+  startUtcMs: 3,
+  endUtcMs: 4,
+  userReports: [structuredClone(LEGACY_PER_KEY_USER_REPORT)],
+};
+
 class FakeConnectionsTable implements InsertableTable<ConnectionRow> {
   public rows: ConnectionRow[] | undefined;
 
@@ -62,54 +77,46 @@ class FakeConnectionsTable implements InsertableTable<ConnectionRow> {
 }
 
 describe('postConnectionMetrics', () => {
-  it('correctly inserts feature metrics rows', async () => {
+  it('correctly inserts connection metrics rows', async () => {
     const table = new FakeConnectionsTable();
-    const userReports = [
-      {
-        countries: ['UK'],
-        bytesTransferred: 123,
-        tunnelTimeSec: 987,
-      },
-      {
-        countries: ['EC'],
-        bytesTransferred: 456,
-        tunnelTimeSec: 654,
-      },
-      {
-        countries: ['BR'],
-        bytesTransferred: 789,
-      },
-    ];
-    const report = {serverId: 'id', startUtcMs: 1, endUtcMs: 2, userReports};
-    await postConnectionMetrics(table, report);
+
+    await postConnectionMetrics(table, VALID_REPORT);
+
     const rows: ConnectionRow[] = [
       {
-        serverId: report.serverId,
-        startTimestamp: new Date(report.startUtcMs).toISOString(),
-        endTimestamp: new Date(report.endUtcMs).toISOString(),
-        bytesTransferred: userReports[0].bytesTransferred,
-        tunnelTimeSec: userReports[0].tunnelTimeSec,
-        countries: userReports[0].countries,
+        serverId: VALID_REPORT.serverId,
+        startTimestamp: new Date(VALID_REPORT.startUtcMs).toISOString(),
+        endTimestamp: new Date(VALID_REPORT.endUtcMs).toISOString(),
+        bytesTransferred: VALID_USER_REPORT.bytesTransferred,
+        tunnelTimeSec: VALID_USER_REPORT.tunnelTimeSec,
+        countries: VALID_USER_REPORT.countries,
       },
       {
-        serverId: report.serverId,
-        startTimestamp: new Date(report.startUtcMs).toISOString(),
-        endTimestamp: new Date(report.endUtcMs).toISOString(),
-        bytesTransferred: userReports[1].bytesTransferred,
-        tunnelTimeSec: userReports[1].tunnelTimeSec,
-        countries: userReports[1].countries,
+        serverId: VALID_REPORT.serverId,
+        startTimestamp: new Date(VALID_REPORT.startUtcMs).toISOString(),
+        endTimestamp: new Date(VALID_REPORT.endUtcMs).toISOString(),
+        bytesTransferred: VALID_USER_REPORT2.bytesTransferred,
+        tunnelTimeSec: VALID_USER_REPORT2.tunnelTimeSec,
+        countries: VALID_USER_REPORT2.countries,
       },
       {
-        serverId: report.serverId,
-        startTimestamp: new Date(report.startUtcMs).toISOString(),
-        endTimestamp: new Date(report.endUtcMs).toISOString(),
-        bytesTransferred: userReports[2].bytesTransferred,
-        tunnelTimeSec: undefined,
-        countries: userReports[2].countries,
+        serverId: VALID_REPORT.serverId,
+        startTimestamp: new Date(VALID_REPORT.startUtcMs).toISOString(),
+        endTimestamp: new Date(VALID_REPORT.endUtcMs).toISOString(),
+        bytesTransferred: LEGACY_PER_LOCATION_USER_REPORT.bytesTransferred,
+        tunnelTimeSec: LEGACY_PER_LOCATION_USER_REPORT.tunnelTimeSec,
+        countries: LEGACY_PER_LOCATION_USER_REPORT.countries,
       },
     ];
     expect(table.rows).toEqual(rows);
   });
+  it('correctly drops legacy connection metrics', async () => {
+    const table = new FakeConnectionsTable();
+
+    await postConnectionMetrics(table, LEGACY_REPORT);
+
+    expect(table.rows).toEqual([]);
+  });
 });
 
 describe('isValidConnectionMetricsReport', () => {
@@ -117,6 +124,10 @@ describe('isValidConnectionMetricsReport', () => {
     const report = structuredClone(VALID_REPORT);
     expect(isValidConnectionMetricsReport(report)).toBeTrue();
   });
+  it('returns true for legacy report', () => {
+    const report = structuredClone(LEGACY_REPORT);
+    expect(isValidConnectionMetricsReport(report)).toBeTrue();
+  });
   it('returns false for missing report', () => {
     expect(isValidConnectionMetricsReport(undefined)).toBeFalse();
   });
@@ -166,14 +177,6 @@ describe('isValidConnectionMetricsReport', () => {
     delete report['endUtcMs'];
     expect(isValidConnectionMetricsReport(report)).toBeFalse();
   });
-  it('returns false for missing user report field `countries`', () => {
-    const report = structuredClone(VALID_REPORT);
-    const userReport: Partial<HourlyUserConnectionMetricsReport> =
-      structuredClone(VALID_USER_REPORT);
-    delete userReport['countries'];
-    report.userReports[0] = userReport as HourlyUserConnectionMetricsReport;
-    expect(isValidConnectionMetricsReport(report)).toBeFalse();
-  });
   it('returns false for missing user report field `bytesTransferred`', () => {
     const report = structuredClone(VALID_REPORT);
     const userReport: Partial<HourlyUserConnectionMetricsReport> =
@@ -202,7 +205,17 @@ describe('isValidConnectionMetricsReport', () => {
     report.endUtcMs = '100' as unknown as number;
     expect(isValidConnectionMetricsReport(report)).toBeFalse();
   });
-  it('returns false for `countries` field type that is not a string', () => {
+  it('returns false for `userId` field type that is not a string', () => {
+    const report = structuredClone(VALID_REPORT);
+    report.userReports[0].userId = 1234 as unknown as string;
+    expect(isValidConnectionMetricsReport(report)).toBeFalse();
+  });
+  it('returns false for `countries` field type that is not an array', () => {
+    const report = structuredClone(VALID_REPORT);
+    report.userReports[0].countries = 'US' as unknown as string[];
+    expect(isValidConnectionMetricsReport(report)).toBeFalse();
+  });
+  it('returns false for `countries` arry items that are not strings', () => {
     const report = structuredClone(VALID_REPORT);
     report.userReports[0].countries = [1, 2, 3] as unknown as string[];
     expect(isValidConnectionMetricsReport(report)).toBeFalse();

src/metrics_server/connection_metrics.ts

@@ -14,7 +14,13 @@
 
 import {Table} from '@google-cloud/bigquery';
 import {InsertableTable} from './infrastructure/table';
-import {HourlyConnectionMetricsReport} from './model';
+import {
+  HourlyConnectionMetricsReport,
+  HourlyUserConnectionMetricsReport,
+  HourlyUserConnectionMetricsReportByLocation,
+} from './model';
+
+const TERABYTE = Math.pow(2, 40);
 
 export interface ConnectionRow {
   serverId: string;
@@ -45,18 +51,29 @@ function getConnectionRowsFromReport(report: HourlyConnectionMetricsReport): Con
   const endTimestampStr = new Date(report.endUtcMs).toISOString();
   const rows = [];
   for (const userReport of report.userReports) {
-    rows.push({
-      serverId: report.serverId,
-      startTimestamp: startTimestampStr,
-      endTimestamp: endTimestampStr,
-      bytesTransferred: userReport.bytesTransferred,
-      tunnelTimeSec: userReport.tunnelTimeSec || undefined,
-      countries: userReport.countries || [],
-    });
+    // User reports come in 2 flavors: "per location" and "per key". We no longer store the
+    // "per key" reports.
+    if (isPerLocationUserReport(userReport)) {
+      rows.push({
+        serverId: report.serverId,
+        startTimestamp: startTimestampStr,
+        endTimestamp: endTimestampStr,
+        bytesTransferred: userReport.bytesTransferred,
+        tunnelTimeSec: userReport.tunnelTimeSec || undefined,
+        countries: userReport.countries,
+      });
+    }
   }
   return rows;
 }
 
+function isPerLocationUserReport(
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  userReport: HourlyUserConnectionMetricsReport
+): userReport is HourlyUserConnectionMetricsReportByLocation {
+  return 'countries' in userReport;
+}
+
 // Returns true iff testObject contains a valid HourlyConnectionMetricsReport.
 export function isValidConnectionMetricsReport(
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -93,14 +110,21 @@ export function isValidConnectionMetricsReport(
     return false;
   }
 
-  const MIN_BYTES_TRANSFERRED = 0;
-  const MAX_BYTES_TRANSFERRED = 1 * Math.pow(2, 40); // 1 TB.
   for (const userReport of testObject.userReports) {
-    // Check that `bytesTransferred` is a number between min and max transfer limits
+    // Check that `userId` is a string.
+    if (userReport.userId && typeof userReport.userId !== 'string') {
+      return false;
+    }
+
+    // We used to set a limit of 1TB per access key, then per location. We later
+    // realized that a server may use a single key, or all the traffic may come
+    // from a single location.
+    // However, as we report hourly, it's unlikely we hit 1TB, so we keep the
+    // check for now to try and prevent malicious reports.
     if (
       typeof userReport.bytesTransferred !== 'number' ||
-      userReport.bytesTransferred < MIN_BYTES_TRANSFERRED ||
-      userReport.bytesTransferred > MAX_BYTES_TRANSFERRED
+      userReport.bytesTransferred < 0 ||
+      userReport.bytesTransferred > TERABYTE
     ) {
       return false;
     }
@@ -112,16 +136,16 @@ export function isValidConnectionMetricsReport(
       return false;
     }
 
-    // We require at least 1 country to be set
-    const countries = userReport.countries ?? [];
-    if (countries.length === 0) {
-      return false;
-    }
-    // Check that all `countries` are strings.
-    for (const country of countries) {
-      if (typeof country !== 'string') {
+    // Check that `countries` is an array of strings.
+    if (userReport.countries) {
+      if (!Array.isArray(userReport.countries)) {
         return false;
       }
+      for (const country of userReport.countries) {
+        if (typeof country !== 'string') {
+          return false;
+        }
+      }
     }
   }
 

src/metrics_server/model.ts

@@ -22,11 +22,17 @@ export interface HourlyConnectionMetricsReport {
 }
 
 export interface HourlyUserConnectionMetricsReport {
-  countries: string[];
+  userId?: string;
+  countries?: string[];
   bytesTransferred: number;
   tunnelTimeSec?: number;
 }
 
+export interface HourlyUserConnectionMetricsReportByLocation
+  extends Omit<HourlyUserConnectionMetricsReport, 'countries'> {
+  countries: string[];
+}
+
 export interface DailyFeatureMetricsReport {
   serverId: string;
   serverVersion: string;