From 04b5b587a1902b7d8587d318a739d87b86199667 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Wed, 17 Apr 2024 13:36:01 +0100 Subject: [PATCH] Expected UTDs: report a different Posthog code (#12389) * `DecryptionFailureTracker`: stronger typing Use `DecryptionFailureCode` rather than string * `DecryptionFailureTracker`: remove use of `DecryptionError` The second argument to `MatrixEventEvent.Decrypted` callbacks is deprecatedf, and we can get the info we need direct from the event. This means that we no longer need to reference the internal `DecryptionError` class in the js-sdk. * `DecryptionFailureTracker`: use a different Posthog code for historical UTDs * Update for new UTD error codes --- .eslintrc.js | 1 - src/DecryptionFailureTracker.ts | 60 +++++------ src/components/structures/MatrixChat.tsx | 3 +- test/DecryptionFailureTracker-test.ts | 123 +++++++++-------------- yarn.lock | 31 +----- 5 files changed, 84 insertions(+), 134 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 14afc41c07..caeeca403d 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -108,7 +108,6 @@ module.exports = { "!matrix-js-sdk/src/extensible_events_v1/PollEndEvent", "!matrix-js-sdk/src/extensible_events_v1/InvalidEventError", "!matrix-js-sdk/src/crypto", - "!matrix-js-sdk/src/crypto/algorithms", "!matrix-js-sdk/src/crypto/aes", "!matrix-js-sdk/src/crypto/olmlib", "!matrix-js-sdk/src/crypto/crypto", diff --git a/src/DecryptionFailureTracker.ts b/src/DecryptionFailureTracker.ts index f9afec0daa..9914704550 100644 --- a/src/DecryptionFailureTracker.ts +++ b/src/DecryptionFailureTracker.ts @@ -14,9 +14,9 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { DecryptionError } from "matrix-js-sdk/src/crypto/algorithms"; import { MatrixEvent } from "matrix-js-sdk/src/matrix"; import { Error as ErrorEvent } from "@matrix-org/analytics-events/types/typescript/Error"; +import { DecryptionFailureCode } from "matrix-js-sdk/src/crypto-api"; import { PosthogAnalytics } from "./PosthogAnalytics"; @@ -25,17 +25,15 @@ export class DecryptionFailure { public constructor( public readonly failedEventId: string, - public readonly errorCode: string, + public readonly errorCode: DecryptionFailureCode, ) { this.ts = Date.now(); } } -type ErrorCode = "OlmKeysNotSentError" | "OlmIndexError" | "UnknownError" | "OlmUnspecifiedError"; - +type ErrorCode = ErrorEvent["name"]; type TrackingFn = (count: number, trackedErrCode: ErrorCode, rawError: string) => void; - -export type ErrCodeMapFn = (errcode: string) => ErrorCode; +export type ErrCodeMapFn = (errcode: DecryptionFailureCode) => ErrorCode; export class DecryptionFailureTracker { private static internalInstance = new DecryptionFailureTracker( @@ -52,12 +50,14 @@ export class DecryptionFailureTracker { (errorCode) => { // Map JS-SDK error codes to tracker codes for aggregation switch (errorCode) { - case "MEGOLM_UNKNOWN_INBOUND_SESSION_ID": + case DecryptionFailureCode.MEGOLM_UNKNOWN_INBOUND_SESSION_ID: return "OlmKeysNotSentError"; - case "OLM_UNKNOWN_MESSAGE_INDEX": + case DecryptionFailureCode.OLM_UNKNOWN_MESSAGE_INDEX: return "OlmIndexError"; - case undefined: - return "OlmUnspecifiedError"; + case DecryptionFailureCode.HISTORICAL_MESSAGE_NO_KEY_BACKUP: + case DecryptionFailureCode.HISTORICAL_MESSAGE_BACKUP_UNCONFIGURED: + case DecryptionFailureCode.HISTORICAL_MESSAGE_WORKING_BACKUP: + return "HistoricalMessage"; default: return "UnknownError"; } @@ -76,11 +76,11 @@ export class DecryptionFailureTracker { // accumulated in `failureCounts`. public visibleFailures: Map = new Map(); - // A histogram of the number of failures that will be tracked at the next tracking - // interval, split by failure error code. - public failureCounts: Record = { - // [errorCode]: 42 - }; + /** + * A histogram of the number of failures that will be tracked at the next tracking + * interval, split by failure error code. + */ + private failureCounts: Map = new Map(); // Event IDs of failures that were tracked previously public trackedEvents: Set = new Set(); @@ -108,10 +108,10 @@ export class DecryptionFailureTracker { * * @param {function} fn The tracking function, which will be called when failures * are tracked. The function should have a signature `(count, trackedErrorCode) => {...}`, - * where `count` is the number of failures and `errorCode` matches the `.code` of - * provided DecryptionError errors (by default, unless `errorCodeMapFn` is specified. - * @param {function?} errorCodeMapFn The function used to map error codes to the - * trackedErrorCode. If not provided, the `.code` of errors will be used. + * where `count` is the number of failures and `errorCode` matches the output of `errorCodeMapFn`. + * + * @param {function} errorCodeMapFn The function used to map decryption failure reason codes to the + * `trackedErrorCode`. */ private constructor( private readonly fn: TrackingFn, @@ -138,13 +138,15 @@ export class DecryptionFailureTracker { // localStorage.setItem('mx-decryption-failure-event-ids', JSON.stringify([...this.trackedEvents])); // } - public eventDecrypted(e: MatrixEvent, err: DecryptionError): void { - // for now we only track megolm decrytion failures + public eventDecrypted(e: MatrixEvent): void { + // for now we only track megolm decryption failures if (e.getWireContent().algorithm != "m.megolm.v1.aes-sha2") { return; } - if (err) { - this.addDecryptionFailure(new DecryptionFailure(e.getId()!, err.code)); + + const errCode = e.decryptionFailureReason; + if (errCode !== null) { + this.addDecryptionFailure(new DecryptionFailure(e.getId()!, errCode)); } else { // Could be an event in the failures, remove it this.removeDecryptionFailuresForEvent(e); @@ -205,7 +207,7 @@ export class DecryptionFailureTracker { this.failures = new Map(); this.visibleEvents = new Set(); this.visibleFailures = new Map(); - this.failureCounts = {}; + this.failureCounts = new Map(); } /** @@ -236,7 +238,7 @@ export class DecryptionFailureTracker { private aggregateFailures(failures: Set): void { for (const failure of failures) { const errorCode = failure.errorCode; - this.failureCounts[errorCode] = (this.failureCounts[errorCode] || 0) + 1; + this.failureCounts.set(errorCode, (this.failureCounts.get(errorCode) ?? 0) + 1); } } @@ -245,12 +247,12 @@ export class DecryptionFailureTracker { * function with the number of failures that should be tracked. */ public trackFailures(): void { - for (const errorCode of Object.keys(this.failureCounts)) { - if (this.failureCounts[errorCode] > 0) { + for (const [errorCode, count] of this.failureCounts.entries()) { + if (count > 0) { const trackedErrorCode = this.errorCodeMapFn(errorCode); - this.fn(this.failureCounts[errorCode], trackedErrorCode, errorCode); - this.failureCounts[errorCode] = 0; + this.fn(count, trackedErrorCode, errorCode); + this.failureCounts.set(errorCode, 0); } } } diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx index 94b6de613c..3aae34e311 100644 --- a/src/components/structures/MatrixChat.tsx +++ b/src/components/structures/MatrixChat.tsx @@ -32,7 +32,6 @@ import { defer, IDeferred, QueryDict } from "matrix-js-sdk/src/utils"; import { logger } from "matrix-js-sdk/src/logger"; import { throttle } from "lodash"; import { CryptoEvent } from "matrix-js-sdk/src/crypto"; -import { DecryptionError } from "matrix-js-sdk/src/crypto/algorithms"; import { IKeyBackupInfo } from "matrix-js-sdk/src/crypto/keybackup"; import { TooltipProvider } from "@vector-im/compound-web"; @@ -1598,7 +1597,7 @@ export default class MatrixChat extends React.PureComponent { // When logging out, stop tracking failures and destroy state cli.on(HttpApiEvent.SessionLoggedOut, () => dft.stop()); - cli.on(MatrixEventEvent.Decrypted, (e, err) => dft.eventDecrypted(e, err as DecryptionError)); + cli.on(MatrixEventEvent.Decrypted, (e) => dft.eventDecrypted(e)); cli.on(ClientEvent.Room, (room) => { if (cli.isCryptoEnabled()) { diff --git a/test/DecryptionFailureTracker-test.ts b/test/DecryptionFailureTracker-test.ts index 553d4f4d74..7a0bf65f81 100644 --- a/test/DecryptionFailureTracker-test.ts +++ b/test/DecryptionFailureTracker-test.ts @@ -19,21 +19,11 @@ import { DecryptionFailureCode } from "matrix-js-sdk/src/crypto-api"; import { DecryptionFailureTracker } from "../src/DecryptionFailureTracker"; -class MockDecryptionError extends Error { - public readonly code: string; - - constructor(code?: string) { - super(); - - this.code = code || "MOCK_DECRYPTION_ERROR"; - } -} - -async function createFailedDecryptionEvent() { +async function createFailedDecryptionEvent(code?: DecryptionFailureCode) { return await mkDecryptionFailureMatrixEvent({ roomId: "!room:id", sender: "@alice:example.com", - code: DecryptionFailureCode.UNKNOWN_ERROR, + code: code ?? DecryptionFailureCode.UNKNOWN_ERROR, msg: ":(", }); } @@ -50,9 +40,7 @@ describe("DecryptionFailureTracker", function () { ); tracker.addVisibleEvent(failedDecryptionEvent); - - const err = new MockDecryptionError(); - tracker.eventDecrypted(failedDecryptionEvent, err); + tracker.eventDecrypted(failedDecryptionEvent); // Pretend "now" is Infinity tracker.checkFailures(Infinity); @@ -65,7 +53,9 @@ describe("DecryptionFailureTracker", function () { }); it("tracks a failed decryption with expected raw error for a visible event", async function () { - const failedDecryptionEvent = await createFailedDecryptionEvent(); + const failedDecryptionEvent = await createFailedDecryptionEvent( + DecryptionFailureCode.OLM_UNKNOWN_MESSAGE_INDEX, + ); let count = 0; let reportedRawCode = ""; @@ -79,9 +69,7 @@ describe("DecryptionFailureTracker", function () { ); tracker.addVisibleEvent(failedDecryptionEvent); - - const err = new MockDecryptionError("INBOUND_SESSION_MISMATCH_ROOM_ID"); - tracker.eventDecrypted(failedDecryptionEvent, err); + tracker.eventDecrypted(failedDecryptionEvent); // Pretend "now" is Infinity tracker.checkFailures(Infinity); @@ -93,7 +81,7 @@ describe("DecryptionFailureTracker", function () { expect(count).not.toBe(0); // Should add the rawCode to the event context - expect(reportedRawCode).toBe("INBOUND_SESSION_MISMATCH_ROOM_ID"); + expect(reportedRawCode).toBe("OLM_UNKNOWN_MESSAGE_INDEX"); }); it("tracks a failed decryption for an event that becomes visible later", async function () { @@ -106,9 +94,7 @@ describe("DecryptionFailureTracker", function () { () => "UnknownError", ); - const err = new MockDecryptionError(); - tracker.eventDecrypted(failedDecryptionEvent, err); - + tracker.eventDecrypted(failedDecryptionEvent); tracker.addVisibleEvent(failedDecryptionEvent); // Pretend "now" is Infinity @@ -131,8 +117,7 @@ describe("DecryptionFailureTracker", function () { () => "UnknownError", ); - const err = new MockDecryptionError(); - tracker.eventDecrypted(failedDecryptionEvent, err); + tracker.eventDecrypted(failedDecryptionEvent); // Pretend "now" is Infinity tracker.checkFailures(Infinity); @@ -156,9 +141,7 @@ describe("DecryptionFailureTracker", function () { ); tracker.addVisibleEvent(decryptedEvent); - - const err = new MockDecryptionError(); - tracker.eventDecrypted(decryptedEvent, err); + tracker.eventDecrypted(decryptedEvent); // Indicate successful decryption. await decryptExistingEvent(decryptedEvent, { @@ -188,15 +171,14 @@ describe("DecryptionFailureTracker", function () { () => "UnknownError", ); - const err = new MockDecryptionError(); - tracker.eventDecrypted(decryptedEvent, err); + tracker.eventDecrypted(decryptedEvent); // Indicate successful decryption. await decryptExistingEvent(decryptedEvent, { plainType: "m.room.message", plainContent: { body: "success" }, }); - tracker.eventDecrypted(decryptedEvent, null); + tracker.eventDecrypted(decryptedEvent); tracker.addVisibleEvent(decryptedEvent); @@ -222,16 +204,15 @@ describe("DecryptionFailureTracker", function () { tracker.addVisibleEvent(decryptedEvent); // Arbitrary number of failed decryptions for both events - const err = new MockDecryptionError(); - tracker.eventDecrypted(decryptedEvent, err); - tracker.eventDecrypted(decryptedEvent, err); - tracker.eventDecrypted(decryptedEvent, err); - tracker.eventDecrypted(decryptedEvent, err); - tracker.eventDecrypted(decryptedEvent, err); - tracker.eventDecrypted(decryptedEvent2, err); - tracker.eventDecrypted(decryptedEvent2, err); + tracker.eventDecrypted(decryptedEvent); + tracker.eventDecrypted(decryptedEvent); + tracker.eventDecrypted(decryptedEvent); + tracker.eventDecrypted(decryptedEvent); + tracker.eventDecrypted(decryptedEvent); + tracker.eventDecrypted(decryptedEvent2); + tracker.eventDecrypted(decryptedEvent2); tracker.addVisibleEvent(decryptedEvent2); - tracker.eventDecrypted(decryptedEvent2, err); + tracker.eventDecrypted(decryptedEvent2); // Pretend "now" is Infinity tracker.checkFailures(Infinity); @@ -259,8 +240,7 @@ describe("DecryptionFailureTracker", function () { tracker.addVisibleEvent(decryptedEvent); // Indicate decryption - const err = new MockDecryptionError(); - tracker.eventDecrypted(decryptedEvent, err); + tracker.eventDecrypted(decryptedEvent); // Pretend "now" is Infinity tracker.checkFailures(Infinity); @@ -268,7 +248,7 @@ describe("DecryptionFailureTracker", function () { tracker.trackFailures(); // Indicate a second decryption, after having tracked the failure - tracker.eventDecrypted(decryptedEvent, err); + tracker.eventDecrypted(decryptedEvent); tracker.trackFailures(); @@ -292,8 +272,7 @@ describe("DecryptionFailureTracker", function () { tracker.addVisibleEvent(decryptedEvent); // Indicate decryption - const err = new MockDecryptionError(); - tracker.eventDecrypted(decryptedEvent, err); + tracker.eventDecrypted(decryptedEvent); // Pretend "now" is Infinity // NB: This saves to localStorage specific to DFT @@ -312,7 +291,7 @@ describe("DecryptionFailureTracker", function () { //secondTracker.loadTrackedEvents(); - secondTracker.eventDecrypted(decryptedEvent, err); + secondTracker.eventDecrypted(decryptedEvent); secondTracker.checkFailures(Infinity); secondTracker.trackFailures(); @@ -326,25 +305,27 @@ describe("DecryptionFailureTracker", function () { // @ts-ignore access to private constructor const tracker = new DecryptionFailureTracker( (total: number, errorCode: string) => (counts[errorCode] = (counts[errorCode] || 0) + total), - (error: string) => (error === "UnknownError" ? "UnknownError" : "OlmKeysNotSentError"), + (error: DecryptionFailureCode) => + error === DecryptionFailureCode.UNKNOWN_ERROR ? "UnknownError" : "OlmKeysNotSentError", ); - const decryptedEvent1 = await createFailedDecryptionEvent(); - const decryptedEvent2 = await createFailedDecryptionEvent(); - const decryptedEvent3 = await createFailedDecryptionEvent(); - - const error1 = new MockDecryptionError("UnknownError"); - const error2 = new MockDecryptionError("OlmKeysNotSentError"); + const decryptedEvent1 = await createFailedDecryptionEvent(DecryptionFailureCode.UNKNOWN_ERROR); + const decryptedEvent2 = await createFailedDecryptionEvent( + DecryptionFailureCode.MEGOLM_UNKNOWN_INBOUND_SESSION_ID, + ); + const decryptedEvent3 = await createFailedDecryptionEvent( + DecryptionFailureCode.MEGOLM_UNKNOWN_INBOUND_SESSION_ID, + ); tracker.addVisibleEvent(decryptedEvent1); tracker.addVisibleEvent(decryptedEvent2); tracker.addVisibleEvent(decryptedEvent3); // One failure of ERROR_CODE_1, and effectively two for ERROR_CODE_2 - tracker.eventDecrypted(decryptedEvent1, error1); - tracker.eventDecrypted(decryptedEvent2, error2); - tracker.eventDecrypted(decryptedEvent2, error2); - tracker.eventDecrypted(decryptedEvent3, error2); + tracker.eventDecrypted(decryptedEvent1); + tracker.eventDecrypted(decryptedEvent2); + tracker.eventDecrypted(decryptedEvent2); + tracker.eventDecrypted(decryptedEvent3); // Pretend "now" is Infinity tracker.checkFailures(Infinity); @@ -364,21 +345,19 @@ describe("DecryptionFailureTracker", function () { (_errorCode: string) => "OlmUnspecifiedError", ); - const decryptedEvent1 = await createFailedDecryptionEvent(); - const decryptedEvent2 = await createFailedDecryptionEvent(); - const decryptedEvent3 = await createFailedDecryptionEvent(); - - const error1 = new MockDecryptionError("ERROR_CODE_1"); - const error2 = new MockDecryptionError("ERROR_CODE_2"); - const error3 = new MockDecryptionError("ERROR_CODE_3"); + const decryptedEvent1 = await createFailedDecryptionEvent( + DecryptionFailureCode.MEGOLM_UNKNOWN_INBOUND_SESSION_ID, + ); + const decryptedEvent2 = await createFailedDecryptionEvent(DecryptionFailureCode.OLM_UNKNOWN_MESSAGE_INDEX); + const decryptedEvent3 = await createFailedDecryptionEvent(DecryptionFailureCode.UNKNOWN_ERROR); tracker.addVisibleEvent(decryptedEvent1); tracker.addVisibleEvent(decryptedEvent2); tracker.addVisibleEvent(decryptedEvent3); - tracker.eventDecrypted(decryptedEvent1, error1); - tracker.eventDecrypted(decryptedEvent2, error2); - tracker.eventDecrypted(decryptedEvent3, error3); + tracker.eventDecrypted(decryptedEvent1); + tracker.eventDecrypted(decryptedEvent2); + tracker.eventDecrypted(decryptedEvent3); // Pretend "now" is Infinity tracker.checkFailures(Infinity); @@ -397,13 +376,9 @@ describe("DecryptionFailureTracker", function () { (errorCode: string) => Array.from(errorCode).reverse().join(""), ); - const decryptedEvent = await createFailedDecryptionEvent(); - - const error = new MockDecryptionError("ERROR_CODE_1"); - + const decryptedEvent = await createFailedDecryptionEvent(DecryptionFailureCode.OLM_UNKNOWN_MESSAGE_INDEX); tracker.addVisibleEvent(decryptedEvent); - - tracker.eventDecrypted(decryptedEvent, error); + tracker.eventDecrypted(decryptedEvent); // Pretend "now" is Infinity tracker.checkFailures(Infinity); @@ -411,6 +386,6 @@ describe("DecryptionFailureTracker", function () { tracker.trackFailures(); // should track remapped error code - expect(counts["1_EDOC_RORRE"]).toBe(1); + expect(counts["XEDNI_EGASSEM_NWONKNU_MLO"]).toBe(1); }); }); diff --git a/yarn.lock b/yarn.lock index e593b98002..9262df287b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8652,16 +8652,7 @@ string-length@^4.0.1: char-regex "^1.0.2" strip-ansi "^6.0.0" -"string-width-cjs@npm:string-width@^4.2.0": - version "4.2.3" - resolved "https://registry.yarnpkg.com/string-width/-/string-width-4.2.3.tgz#269c7117d27b05ad2e536830a8ec895ef9c6d010" - integrity sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g== - dependencies: - emoji-regex "^8.0.0" - is-fullwidth-code-point "^3.0.0" - strip-ansi "^6.0.1" - -string-width@^4.1.0, string-width@^4.2.0, string-width@^4.2.3: +"string-width-cjs@npm:string-width@^4.2.0", string-width@^4.1.0, string-width@^4.2.0, string-width@^4.2.3: version "4.2.3" resolved "https://registry.yarnpkg.com/string-width/-/string-width-4.2.3.tgz#269c7117d27b05ad2e536830a8ec895ef9c6d010" integrity sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g== @@ -8755,14 +8746,7 @@ string_decoder@~1.1.1: dependencies: safe-buffer "~5.1.0" -"strip-ansi-cjs@npm:strip-ansi@^6.0.1": - version "6.0.1" - resolved "https://registry.yarnpkg.com/strip-ansi/-/strip-ansi-6.0.1.tgz#9e26c63d30f53443e9489495b2105d37b67a85d9" - integrity sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A== - dependencies: - ansi-regex "^5.0.1" - -strip-ansi@^6.0.0, strip-ansi@^6.0.1: +"strip-ansi-cjs@npm:strip-ansi@^6.0.1", strip-ansi@^6.0.0, strip-ansi@^6.0.1: version "6.0.1" resolved "https://registry.yarnpkg.com/strip-ansi/-/strip-ansi-6.0.1.tgz#9e26c63d30f53443e9489495b2105d37b67a85d9" integrity sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A== @@ -9555,7 +9539,7 @@ which@^2.0.1: dependencies: isexe "^2.0.0" -"wrap-ansi-cjs@npm:wrap-ansi@^7.0.0": +"wrap-ansi-cjs@npm:wrap-ansi@^7.0.0", wrap-ansi@^7.0.0: version "7.0.0" resolved "https://registry.yarnpkg.com/wrap-ansi/-/wrap-ansi-7.0.0.tgz#67e145cff510a6a6984bdf1152911d69d2eb9e43" integrity sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q== @@ -9573,15 +9557,6 @@ wrap-ansi@^6.2.0: string-width "^4.1.0" strip-ansi "^6.0.0" -wrap-ansi@^7.0.0: - version "7.0.0" - resolved "https://registry.yarnpkg.com/wrap-ansi/-/wrap-ansi-7.0.0.tgz#67e145cff510a6a6984bdf1152911d69d2eb9e43" - integrity sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q== - dependencies: - ansi-styles "^4.0.0" - string-width "^4.1.0" - strip-ansi "^6.0.0" - wrap-ansi@^8.1.0: version "8.1.0" resolved "https://registry.yarnpkg.com/wrap-ansi/-/wrap-ansi-8.1.0.tgz#56dc22368ee570face1b49819975d9b9a5ead214"