Merge pull request #9789 from matrix-org/rav/edited_events

Ensure that events are correctly updated when they are edited.

Author: richvdh

cypress/e2e/crypto/crypto.spec.ts

@@ -14,17 +14,17 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import { MatrixClient, Room } from "matrix-js-sdk/src/matrix";
-
+import type { ISendEventResponse, MatrixClient, Room } from "matrix-js-sdk/src/matrix";
 import type { VerificationRequest } from "matrix-js-sdk/src/crypto/verification/request/VerificationRequest";
 import type { ISasEvent } from "matrix-js-sdk/src/crypto/verification/SAS";
+import type { CypressBot } from "../../support/bot";
 import { SynapseInstance } from "../../plugins/synapsedocker";
 import Chainable = Cypress.Chainable;
 
 type EmojiMapping = [emoji: string, name: string];
 interface CryptoTestContext extends Mocha.Context {
     synapse: SynapseInstance;
-    bob: MatrixClient;
+    bob: CypressBot;
 }
 
 const waitForVerificationRequest = (cli: MatrixClient): Promise<VerificationRequest> => {
@@ -197,7 +197,7 @@ describe("Cryptography", function () {
         cy.bootstrapCrossSigning();
         autoJoin(this.bob);
 
-        /* we need to have a room with the other user present, so we can open the verification panel */
+        // we need to have a room with the other user present, so we can open the verification panel
         let roomId: string;
         cy.createRoom({ name: "TestRoom", invite: [this.bob.getUserId()] }).then((_room1Id) => {
             roomId = _room1Id;
@@ -210,4 +210,85 @@ describe("Cryptography", function () {
 
         verify.call(this);
     });
+
+    it("should show the correct shield on edited e2e events", function (this: CryptoTestContext) {
+        cy.bootstrapCrossSigning();
+
+        // bob has a second, not cross-signed, device
+        cy.loginBot(this.synapse, this.bob.getUserId(), this.bob.__cypress_password, {}).as("bobSecondDevice");
+
+        autoJoin(this.bob);
+
+        // first create the room, so that we can open the verification panel
+        cy.createRoom({ name: "TestRoom", invite: [this.bob.getUserId()] })
+            .as("testRoomId")
+            .then((roomId) => {
+                cy.log(`Created test room ${roomId}`);
+                cy.visit(`/#/room/${roomId}`);
+
+                // enable encryption
+                cy.getClient().then((cli) => {
+                    cli.sendStateEvent(roomId, "m.room.encryption", { algorithm: "m.megolm.v1.aes-sha2" });
+                });
+
+                // wait for Bob to join the room, otherwise our attempt to open his user details may race
+                // with his join.
+                cy.contains(".mx_TextualEvent", "Bob joined the room").should("exist");
+            });
+
+        verify.call(this);
+
+        cy.get<string>("@testRoomId").then((roomId) => {
+            // bob sends a valid event
+            cy.wrap(this.bob.sendTextMessage(roomId, "Hoo!")).as("testEvent");
+
+            // the message should appear, decrypted, with no warning
+            cy.contains(".mx_EventTile_body", "Hoo!")
+                .closest(".mx_EventTile")
+                .should("have.class", "mx_EventTile_verified")
+                .should("not.have.descendants", ".mx_EventTile_e2eIcon_warning");
+
+            // bob sends an edit to the first message with his unverified device
+            cy.get<MatrixClient>("@bobSecondDevice").then((bobSecondDevice) => {
+                cy.get<ISendEventResponse>("@testEvent").then((testEvent) => {
+                    bobSecondDevice.sendMessage(roomId, {
+                        "m.new_content": {
+                            msgtype: "m.text",
+                            body: "Haa!",
+                        },
+                        "m.relates_to": {
+                            rel_type: "m.replace",
+                            event_id: testEvent.event_id,
+                        },
+                    });
+                });
+            });
+
+            // the edit should have a warning
+            cy.contains(".mx_EventTile_body", "Haa!")
+                .closest(".mx_EventTile")
+                .within(() => {
+                    cy.get(".mx_EventTile_e2eIcon_warning").should("exist");
+                });
+
+            // a second edit from the verified device should be ok
+            cy.get<ISendEventResponse>("@testEvent").then((testEvent) => {
+                this.bob.sendMessage(roomId, {
+                    "m.new_content": {
+                        msgtype: "m.text",
+                        body: "Hee!",
+                    },
+                    "m.relates_to": {
+                        rel_type: "m.replace",
+                        event_id: testEvent.event_id,
+                    },
+                });
+            });
+
+            cy.contains(".mx_EventTile_body", "Hee!")
+                .closest(".mx_EventTile")
+                .should("have.class", "mx_EventTile_verified")
+                .should("not.have.descendants", ".mx_EventTile_e2eIcon_warning");
+        });
+    });
 });

cypress/support/bot.ts

@@ -51,6 +51,10 @@ const defaultCreateBotOptions = {
     bootstrapCrossSigning: true,
 } as CreateBotOpts;
 
+export interface CypressBot extends MatrixClient {
+    __cypress_password: string;
+}
+
 declare global {
     // eslint-disable-next-line @typescript-eslint/no-namespace
     namespace Cypress {
@@ -60,7 +64,7 @@ declare global {
              * @param synapse the instance on which to register the bot user
              * @param opts create bot options
              */
-            getBot(synapse: SynapseInstance, opts: CreateBotOpts): Chainable<MatrixClient>;
+            getBot(synapse: SynapseInstance, opts: CreateBotOpts): Chainable<CypressBot>;
             /**
              * Returns a new Bot instance logged in as an existing user
              * @param synapse the instance on which to register the bot user
@@ -156,14 +160,20 @@ function setupBotClient(
     });
 }
 
-Cypress.Commands.add("getBot", (synapse: SynapseInstance, opts: CreateBotOpts): Chainable<MatrixClient> => {
+Cypress.Commands.add("getBot", (synapse: SynapseInstance, opts: CreateBotOpts): Chainable<CypressBot> => {
     opts = Object.assign({}, defaultCreateBotOptions, opts);
     const username = Cypress._.uniqueId(opts.userIdPrefix);
     const password = Cypress._.uniqueId("password_");
-    return cy.registerUser(synapse, username, password, opts.displayName).then((credentials) => {
-        cy.log(`Registered bot user ${username} with displayname ${opts.displayName}`);
-        return setupBotClient(synapse, credentials, opts);
-    });
+    return cy
+        .registerUser(synapse, username, password, opts.displayName)
+        .then((credentials) => {
+            cy.log(`Registered bot user ${username} with displayname ${opts.displayName}`);
+            return setupBotClient(synapse, credentials, opts);
+        })
+        .then((client): Chainable<CypressBot> => {
+            Object.assign(client, { __cypress_password: password });
+            return cy.wrap(client as CypressBot);
+        });
 });
 
 Cypress.Commands.add(

src/components/views/rooms/EventTile.tsx

@@ -232,7 +232,7 @@ interface IState {
     // Whether the action bar is focused.
     actionBarFocused: boolean;
     // Whether the event's sender has been verified.
-    verified: string;
+    verified: string | null;
     // The Relations model from the JS SDK for reactions to `mxEvent`
     reactions?: Relations | null | undefined;
 
@@ -278,7 +278,8 @@ export class UnwrappedEventTile extends React.Component<EventTileProps, IState>
         this.state = {
             // Whether the action bar is focused.
             actionBarFocused: false,
-            // Whether the event's sender has been verified.
+            // Whether the event's sender has been verified. `null` if no attempt has yet been made to verify
+            // (including if the event is not encrypted).
             verified: null,
             // The Relations model from the JS SDK for reactions to `mxEvent`
             reactions: this.getReactions(),
@@ -371,6 +372,7 @@ export class UnwrappedEventTile extends React.Component<EventTileProps, IState>
             client.on(CryptoEvent.DeviceVerificationChanged, this.onDeviceVerificationChanged);
             client.on(CryptoEvent.UserTrustStatusChanged, this.onUserVerificationChanged);
             this.props.mxEvent.on(MatrixEventEvent.Decrypted, this.onDecrypted);
+            this.props.mxEvent.on(MatrixEventEvent.Replaced, this.onReplaced);
             DecryptionFailureTracker.instance.addVisibleEvent(this.props.mxEvent);
             if (this.props.showReactions) {
                 this.props.mxEvent.on(MatrixEventEvent.RelationsCreated, this.onReactionsCreated);
@@ -395,7 +397,7 @@ export class UnwrappedEventTile extends React.Component<EventTileProps, IState>
         const room = client.getRoom(this.props.mxEvent.getRoomId());
         room?.on(ThreadEvent.New, this.onNewThread);
 
-        this.verifyEvent(this.props.mxEvent);
+        this.verifyEvent();
     }
 
     private get supportsThreadNotifications(): boolean {
@@ -461,6 +463,7 @@ export class UnwrappedEventTile extends React.Component<EventTileProps, IState>
         }
         this.isListeningForReceipts = false;
         this.props.mxEvent.removeListener(MatrixEventEvent.Decrypted, this.onDecrypted);
+        this.props.mxEvent.removeListener(MatrixEventEvent.Replaced, this.onReplaced);
         if (this.props.showReactions) {
             this.props.mxEvent.removeListener(MatrixEventEvent.RelationsCreated, this.onReactionsCreated);
         }
@@ -470,15 +473,19 @@ export class UnwrappedEventTile extends React.Component<EventTileProps, IState>
         this.threadState?.off(NotificationStateEvents.Update, this.onThreadStateUpdate);
     }
 
-    public componentDidUpdate(prevProps: Readonly<EventTileProps>) {
+    public componentDidUpdate(prevProps: Readonly<EventTileProps>, prevState: Readonly<IState>) {
+        // If the verification state changed, the height might have changed
+        if (prevState.verified !== this.state.verified && this.props.onHeightChanged) {
+            this.props.onHeightChanged();
+        }
         // If we're not listening for receipts and expect to be, register a listener.
         if (!this.isListeningForReceipts && (this.shouldShowSentReceipt || this.shouldShowSendingReceipt)) {
             MatrixClientPeg.get().on(RoomEvent.Receipt, this.onRoomReceipt);
             this.isListeningForReceipts = true;
         }
         // re-check the sender verification as outgoing events progress through the send process.
         if (prevProps.eventSendStatus !== this.props.eventSendStatus) {
-            this.verifyEvent(this.props.mxEvent);
+            this.verifyEvent();
         }
     }
 
@@ -586,26 +593,36 @@ export class UnwrappedEventTile extends React.Component<EventTileProps, IState>
      */
     private onDecrypted = () => {
         // we need to re-verify the sending device.
-        // (we call onHeightChanged in verifyEvent to handle the case where decryption
-        // has caused a change in size of the event tile)
-        this.verifyEvent(this.props.mxEvent);
-        this.forceUpdate();
+        this.verifyEvent();
+        // decryption might, of course, trigger a height change, so call onHeightChanged after the re-render
+        this.forceUpdate(this.props.onHeightChanged);
     };
 
     private onDeviceVerificationChanged = (userId: string, device: string): void => {
         if (userId === this.props.mxEvent.getSender()) {
-            this.verifyEvent(this.props.mxEvent);
+            this.verifyEvent();
         }
     };
 
     private onUserVerificationChanged = (userId: string, _trustStatus: UserTrustLevel): void => {
         if (userId === this.props.mxEvent.getSender()) {
-            this.verifyEvent(this.props.mxEvent);
+            this.verifyEvent();
         }
     };
 
-    private async verifyEvent(mxEvent: MatrixEvent): Promise<void> {
+    /** called when the event is edited after we show it. */
+    private onReplaced = () => {
+        // re-verify the event if it is replaced (the edit may not be verified)
+        this.verifyEvent();
+    };
+
+    private verifyEvent(): void {
+        // if the event was edited, show the verification info for the edit, not
+        // the original
+        const mxEvent = this.props.mxEvent.replacingEvent() ?? this.props.mxEvent;
+
         if (!mxEvent.isEncrypted() || mxEvent.isRedacted()) {
+            this.setState({ verified: null });
             return;
         }
 
@@ -615,66 +632,36 @@ export class UnwrappedEventTile extends React.Component<EventTileProps, IState>
 
         if (encryptionInfo.mismatchedSender) {
             // something definitely wrong is going on here
-            this.setState(
-                {
-                    verified: E2EState.Warning,
-                },
-                this.props.onHeightChanged,
-            ); // Decryption may have caused a change in size
+            this.setState({ verified: E2EState.Warning });
             return;
         }
 
         if (!userTrust.isCrossSigningVerified()) {
             // If the message is unauthenticated, then display a grey
             // shield, otherwise if the user isn't cross-signed then
             // nothing's needed
-            this.setState(
-                {
-                    verified: encryptionInfo.authenticated ? E2EState.Normal : E2EState.Unauthenticated,
-                },
-                this.props.onHeightChanged,
-            ); // Decryption may have caused a change in size
+            this.setState({ verified: encryptionInfo.authenticated ? E2EState.Normal : E2EState.Unauthenticated });
             return;
         }
 
         const eventSenderTrust =
             encryptionInfo.sender && MatrixClientPeg.get().checkDeviceTrust(senderId, encryptionInfo.sender.deviceId);
         if (!eventSenderTrust) {
-            this.setState(
-                {
-                    verified: E2EState.Unknown,
-                },
-                this.props.onHeightChanged,
-            ); // Decryption may have caused a change in size
+            this.setState({ verified: E2EState.Unknown });
             return;
         }
 
         if (!eventSenderTrust.isVerified()) {
-            this.setState(
-                {
-                    verified: E2EState.Warning,
-                },
-                this.props.onHeightChanged,
-            ); // Decryption may have caused a change in size
+            this.setState({ verified: E2EState.Warning });
             return;
         }
 
         if (!encryptionInfo.authenticated) {
-            this.setState(
-                {
-                    verified: E2EState.Unauthenticated,
-                },
-                this.props.onHeightChanged,
-            ); // Decryption may have caused a change in size
+            this.setState({ verified: E2EState.Unauthenticated });
             return;
         }
 
-        this.setState(
-            {
-                verified: E2EState.Verified,
-            },
-            this.props.onHeightChanged,
-        ); // Decryption may have caused a change in size
+        this.setState({ verified: E2EState.Verified });
     }
 
     private propsEqual(objA: EventTileProps, objB: EventTileProps): boolean {
@@ -768,7 +755,9 @@ export class UnwrappedEventTile extends React.Component<EventTileProps, IState>
     };
 
     private renderE2EPadlock() {
-        const ev = this.props.mxEvent;
+        // if the event was edited, show the verification info for the edit, not
+        // the original
+        const ev = this.props.mxEvent.replacingEvent() ?? this.props.mxEvent;
 
         // no icon for local rooms
         if (isLocalRoom(ev.getRoomId()!)) return;