From 9dce81e04407f104ee7779c66ffb91492bc74596 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 5 Aug 2022 14:57:33 -0600 Subject: [PATCH 1/3] Migrate the hidden read receipts flag to new "send read receipts" option For safety. --- .../e2e/settings/hidden-rr-migration.spec.ts | 90 +++++++++++++++++++ cypress/support/login.ts | 7 +- cypress/support/settings.ts | 15 ++-- src/Lifecycle.ts | 3 + src/settings/SettingsStore.ts | 38 ++++++++ .../handlers/DeviceSettingsHandler.ts | 6 +- 6 files changed, 150 insertions(+), 9 deletions(-) create mode 100644 cypress/e2e/settings/hidden-rr-migration.spec.ts diff --git a/cypress/e2e/settings/hidden-rr-migration.spec.ts b/cypress/e2e/settings/hidden-rr-migration.spec.ts new file mode 100644 index 00000000000..2b4134fbddd --- /dev/null +++ b/cypress/e2e/settings/hidden-rr-migration.spec.ts @@ -0,0 +1,90 @@ +/* +Copyright 2022 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +/// + +import { SynapseInstance } from "../../plugins/synapsedocker"; + +function seedLabs(synapse: SynapseInstance, labsVal: boolean | null) { + cy.initTestUser(synapse, "Sally", () => { + // seed labs flag + cy.window({ log: false }).then(win => { + if (typeof labsVal === "boolean") { + // stringify boolean + win.localStorage.setItem("mx_labs_feature_feature_hidden_read_receipts", `${labsVal}`); + } + }); + }); +} + +function testForVal(settingVal: boolean | null) { + const testRoomName = "READ RECEIPTS"; + cy.createRoom({ name: testRoomName }).as("roomId"); + cy.all([cy.get("@roomId")]).then(() => { + cy.viewRoomByName(testRoomName).then(() => { + // if we can see the room, then sync is working for us. It's time to see if the + // migration even ran. + + cy.getSettingValue("sendReadReceipts", null, true).should("satisfy", (val) => { + if (typeof settingVal === "boolean") { + return val === settingVal; + } else { + return !val; // falsy - we don't actually care if it's undefined, null, or a literal false + } + }); + }); + }); +} + +describe("Hidden Read Receipts Setting Migration", () => { + // We run this as a full-blown end-to-end test to ensure it works in an integration + // sense. If we unit tested it, we'd be testing that the code works but not that the + // migration actually runs. + // + // Here, we get to test that not only the code works but also that it gets run. Most + // of our interactions are with the JS console as we're honestly just checking that + // things got set correctly. + // + // For a security-sensitive feature like hidden read receipts, it's absolutely vital + // that we migrate the setting appropriately. + + let synapse: SynapseInstance; + + beforeEach(() => { + cy.startSynapse("default").then(data => { + synapse = data; + }); + }); + + afterEach(() => { + cy.stopSynapse(synapse); + }); + + it('should not migrate the lack of a labs flag', () => { + seedLabs(synapse, null); + testForVal(null); + }); + + it('should migrate labsHiddenRR=false as sendRR=true', () => { + seedLabs(synapse, false); + testForVal(true); + }); + + it('should migrate labsHiddenRR=true as sendRR=false', () => { + seedLabs(synapse, true); + testForVal(false); + }); +}); diff --git a/cypress/support/login.ts b/cypress/support/login.ts index da72ec69a46..1d14589580c 100644 --- a/cypress/support/login.ts +++ b/cypress/support/login.ts @@ -35,13 +35,14 @@ declare global { * Generates a test user and instantiates an Element session with that user. * @param synapse the synapse returned by startSynapse * @param displayName the displayName to give the test user + * @param prelaunchFn optional function to run before the app is visited */ - initTestUser(synapse: SynapseInstance, displayName: string): Chainable; + initTestUser(synapse: SynapseInstance, displayName: string, prelaunchFn?: () => void): Chainable; } } } -Cypress.Commands.add("initTestUser", (synapse: SynapseInstance, displayName: string): Chainable => { +Cypress.Commands.add("initTestUser", (synapse: SynapseInstance, displayName: string, prelaunchFn?: () => void): Chainable => { // XXX: work around Cypress not clearing IDB between tests cy.window({ log: false }).then(win => { win.indexedDB.databases().then(databases => { @@ -87,6 +88,8 @@ Cypress.Commands.add("initTestUser", (synapse: SynapseInstance, displayName: str win.localStorage.setItem("mx_local_settings", '{"language":"en"}'); }); + prelaunchFn?.(); + return cy.visit("/").then(() => { // wait for the app to load return cy.get(".mx_MatrixChat", { timeout: 15000 }); diff --git a/cypress/support/settings.ts b/cypress/support/settings.ts index a44f3f06d25..f3094f48206 100644 --- a/cypress/support/settings.ts +++ b/cypress/support/settings.ts @@ -96,7 +96,7 @@ declare global { * value. * @return {*} The value, or null if not found */ - getSettingValue(name: string, roomId?: string): Chainable; + getSettingValue(name: string, roomId?: string, excludeDefault?: boolean): Chainable; } } } @@ -116,11 +116,14 @@ Cypress.Commands.add("setSettingValue", ( }); }); -Cypress.Commands.add("getSettingValue", (name: string, roomId?: string): Chainable => { - return cy.getSettingsStore().then((store: typeof SettingsStore) => { - return store.getValue(name, roomId); - }); -}); +Cypress.Commands.add( + "getSettingValue", + (name: string, roomId?: string, excludeDefault?: boolean): Chainable => { + return cy.getSettingsStore().then((store: typeof SettingsStore) => { + return store.getValue(name, roomId, excludeDefault); + }); + }, +); Cypress.Commands.add("openUserMenu", (): Chainable> => { cy.get('[aria-label="User menu"]').click(); diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts index df9dec9f5ed..94b2e257067 100644 --- a/src/Lifecycle.ts +++ b/src/Lifecycle.ts @@ -826,6 +826,9 @@ async function startMatrixClient(startSyncing = true): Promise { await MatrixClientPeg.assign(); } + // Run the migrations after the MatrixClientPeg has been assigned + SettingsStore.runMigrations(); + // This needs to be started after crypto is set up DeviceListener.sharedInstance().start(); // Similarly, don't start sending presence updates until we've started diff --git a/src/settings/SettingsStore.ts b/src/settings/SettingsStore.ts index b6f63d4e9c6..f4e76bf05ba 100644 --- a/src/settings/SettingsStore.ts +++ b/src/settings/SettingsStore.ts @@ -35,6 +35,8 @@ import SettingsHandler from "./handlers/SettingsHandler"; import { SettingUpdatedPayload } from "../dispatcher/payloads/SettingUpdatedPayload"; import { Action } from "../dispatcher/actions"; import PlatformSettingsHandler from "./handlers/PlatformSettingsHandler"; +import dispatcher from "../dispatcher/dispatcher"; +import { ActionPayload } from "../dispatcher/payloads"; const defaultWatchManager = new WatchManager(); @@ -565,6 +567,42 @@ export default class SettingsStore { return null; } + /** + * Runs or queues any setting migrations needed. + */ + public static runMigrations() { + // Dev notes: to add your migration, just add a new `migrateMyFeature` function, call it, and + // add a comment to note when it can be removed. + + SettingsStore.migrateHiddenReadReceipts(); // Can be removed after October 2022. + } + + private static migrateHiddenReadReceipts() { + // We wait for the first sync to ensure that the user's existing account data has loaded, as otherwise + // getValue() for an account-level setting like sendReadReceipts will return `null`. + const disRef = dispatcher.register((payload: ActionPayload) => { + if (payload.action === "MatrixActions.sync") { + dispatcher.unregister(disRef); + + const rrVal = SettingsStore.getValue("sendReadReceipts", null, true); + if (typeof rrVal !== "boolean") { + // new setting isn't set - see if the labs flag was. We have to manually reach into the + // handler for this because it isn't a setting anymore (`getValue` will yell at us). + const handler = LEVEL_HANDLERS[SettingLevel.DEVICE] as DeviceSettingsHandler; + const labsVal = handler.readFeature("feature_hidden_read_receipts"); + if (typeof labsVal === "boolean") { + // Inverse of labs flag because negative->positive language switch in setting name + const newVal = !labsVal; + console.log(`Setting sendReadReceipts to ${newVal} because of previously-set labs flag`); + + // noinspection JSIgnoredPromiseFromCall + SettingsStore.setValue("sendReadReceipts", null, SettingLevel.ACCOUNT, newVal); + } + } + } + }); + } + /** * Debugging function for reading explicit setting values without going through the * complicated/biased functions in the SettingsStore. This will print information to diff --git a/src/settings/handlers/DeviceSettingsHandler.ts b/src/settings/handlers/DeviceSettingsHandler.ts index 138457d5a9f..4be80b764d0 100644 --- a/src/settings/handlers/DeviceSettingsHandler.ts +++ b/src/settings/handlers/DeviceSettingsHandler.ts @@ -114,12 +114,16 @@ export default class DeviceSettingsHandler extends AbstractLocalStorageSettingsH // Note: features intentionally don't use the same key as settings to avoid conflicts // and to be backwards compatible. - private readFeature(featureName: string): boolean | null { + // public for access to migrations - not exposed from the SettingsHandler interface + public readFeature(featureName: string): boolean | null { if (MatrixClientPeg.get() && MatrixClientPeg.get().isGuest()) { // Guests should not have any labs features enabled. return false; } + // XXX: This turns they key names into `mx_labs_feature_feature_x` (double feature). + // This is because all feature names start with `feature_` as a matter of policy. + // Oh well. return this.getBoolean("mx_labs_feature_" + featureName); } From b99470c40d8f5ed5789dfa639267deb885a92e6c Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 5 Aug 2022 15:08:52 -0600 Subject: [PATCH 2/3] Appease linter & ignore guests --- cypress/support/login.ts | 7 ++++++- cypress/support/settings.ts | 14 ++++++-------- src/settings/SettingsStore.ts | 3 +++ 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/cypress/support/login.ts b/cypress/support/login.ts index 1d14589580c..4cdfd6a84df 100644 --- a/cypress/support/login.ts +++ b/cypress/support/login.ts @@ -37,11 +37,16 @@ declare global { * @param displayName the displayName to give the test user * @param prelaunchFn optional function to run before the app is visited */ - initTestUser(synapse: SynapseInstance, displayName: string, prelaunchFn?: () => void): Chainable; + initTestUser( + synapse: SynapseInstance, + displayName: string, + prelaunchFn?: () => void, + ): Chainable; } } } +// eslint-disable-next-line max-len Cypress.Commands.add("initTestUser", (synapse: SynapseInstance, displayName: string, prelaunchFn?: () => void): Chainable => { // XXX: work around Cypress not clearing IDB between tests cy.window({ log: false }).then(win => { diff --git a/cypress/support/settings.ts b/cypress/support/settings.ts index f3094f48206..06ec815364b 100644 --- a/cypress/support/settings.ts +++ b/cypress/support/settings.ts @@ -116,14 +116,12 @@ Cypress.Commands.add("setSettingValue", ( }); }); -Cypress.Commands.add( - "getSettingValue", - (name: string, roomId?: string, excludeDefault?: boolean): Chainable => { - return cy.getSettingsStore().then((store: typeof SettingsStore) => { - return store.getValue(name, roomId, excludeDefault); - }); - }, -); +// eslint-disable-next-line max-len +Cypress.Commands.add("getSettingValue", (name: string, roomId?: string, excludeDefault?: boolean): Chainable => { + return cy.getSettingsStore().then((store: typeof SettingsStore) => { + return store.getValue(name, roomId, excludeDefault); + }); +}); Cypress.Commands.add("openUserMenu", (): Chainable> => { cy.get('[aria-label="User menu"]').click(); diff --git a/src/settings/SettingsStore.ts b/src/settings/SettingsStore.ts index f4e76bf05ba..7de8521b73c 100644 --- a/src/settings/SettingsStore.ts +++ b/src/settings/SettingsStore.ts @@ -37,6 +37,7 @@ import { Action } from "../dispatcher/actions"; import PlatformSettingsHandler from "./handlers/PlatformSettingsHandler"; import dispatcher from "../dispatcher/dispatcher"; import { ActionPayload } from "../dispatcher/payloads"; +import { MatrixClientPeg } from "../MatrixClientPeg"; const defaultWatchManager = new WatchManager(); @@ -578,6 +579,8 @@ export default class SettingsStore { } private static migrateHiddenReadReceipts() { + if (MatrixClientPeg.get().isGuest()) return; // not worth it + // We wait for the first sync to ensure that the user's existing account data has loaded, as otherwise // getValue() for an account-level setting like sendReadReceipts will return `null`. const disRef = dispatcher.register((payload: ActionPayload) => { From 1d231e87f6eefb84318bd367f0ec534b2ca93508 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 8 Aug 2022 10:27:16 -0600 Subject: [PATCH 3/3] `void` --- cypress/e2e/settings/hidden-rr-migration.spec.ts | 4 ++-- src/settings/SettingsStore.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cypress/e2e/settings/hidden-rr-migration.spec.ts b/cypress/e2e/settings/hidden-rr-migration.spec.ts index 2b4134fbddd..1d5419af339 100644 --- a/cypress/e2e/settings/hidden-rr-migration.spec.ts +++ b/cypress/e2e/settings/hidden-rr-migration.spec.ts @@ -18,7 +18,7 @@ limitations under the License. import { SynapseInstance } from "../../plugins/synapsedocker"; -function seedLabs(synapse: SynapseInstance, labsVal: boolean | null) { +function seedLabs(synapse: SynapseInstance, labsVal: boolean | null): void { cy.initTestUser(synapse, "Sally", () => { // seed labs flag cy.window({ log: false }).then(win => { @@ -30,7 +30,7 @@ function seedLabs(synapse: SynapseInstance, labsVal: boolean | null) { }); } -function testForVal(settingVal: boolean | null) { +function testForVal(settingVal: boolean | null): void { const testRoomName = "READ RECEIPTS"; cy.createRoom({ name: testRoomName }).as("roomId"); cy.all([cy.get("@roomId")]).then(() => { diff --git a/src/settings/SettingsStore.ts b/src/settings/SettingsStore.ts index 7de8521b73c..76b12b62bbe 100644 --- a/src/settings/SettingsStore.ts +++ b/src/settings/SettingsStore.ts @@ -571,14 +571,14 @@ export default class SettingsStore { /** * Runs or queues any setting migrations needed. */ - public static runMigrations() { + public static runMigrations(): void { // Dev notes: to add your migration, just add a new `migrateMyFeature` function, call it, and // add a comment to note when it can be removed. SettingsStore.migrateHiddenReadReceipts(); // Can be removed after October 2022. } - private static migrateHiddenReadReceipts() { + private static migrateHiddenReadReceipts(): void { if (MatrixClientPeg.get().isGuest()) return; // not worth it // We wait for the first sync to ensure that the user's existing account data has loaded, as otherwise