Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display a warning when an unverified user's identity changes #28211

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
1 change: 1 addition & 0 deletions res/css/_components.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,7 @@
@import "./views/rooms/_ThirdPartyMemberInfo.pcss";
@import "./views/rooms/_ThreadSummary.pcss";
@import "./views/rooms/_TopUnreadMessagesBar.pcss";
@import "./views/rooms/_UserIdentityWarning.pcss";
@import "./views/rooms/_VoiceRecordComposerTile.pcss";
@import "./views/rooms/_WhoIsTypingTile.pcss";
@import "./views/rooms/wysiwyg_composer/_EditWysiwygComposer.pcss";
Expand Down
28 changes: 28 additions & 0 deletions res/css/views/rooms/_UserIdentityWarning.pcss
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
Copyright 2024 New Vector Ltd.

SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
Please see LICENSE files in the repository root for full details.
*/

.mx_UserIdentityWarning {
/* 42px is the padding-left of .mx_MessageComposer_wrapper in res/css/views/rooms/_MessageComposer.pcss */
margin-left: calc(-42px + var(--RoomView_MessageList-padding));
uhoreg marked this conversation as resolved.
Show resolved Hide resolved

.mx_UserIdentityWarning_row {
display: flex;
align-items: center;

.mx_BaseAvatar {
margin-left: var(--cpd-space-2x);
}
.mx_UserIdentityWarning_main {
margin-left: var(--cpd-space-6x);
flex-grow: 1;
}
}
}

.mx_MessageComposer.mx_MessageComposer--compact > .mx_UserIdentityWarning {
margin-left: calc(-25px + var(--RoomView_MessageList-padding));
uhoreg marked this conversation as resolved.
Show resolved Hide resolved
}
2 changes: 2 additions & 0 deletions src/components/views/rooms/MessageComposer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import E2EIcon from "./E2EIcon";
import SettingsStore from "../../../settings/SettingsStore";
import { aboveLeftOf, MenuProps } from "../../structures/ContextMenu";
import ReplyPreview from "./ReplyPreview";
import { UserIdentityWarning } from "./UserIdentityWarning";
import { UPDATE_EVENT } from "../../../stores/AsyncStore";
import VoiceRecordComposerTile from "./VoiceRecordComposerTile";
import { VoiceRecordingStore } from "../../../stores/VoiceRecordingStore";
Expand Down Expand Up @@ -675,6 +676,7 @@ export class MessageComposer extends React.Component<IProps, IState> {
<div className={classes} ref={this.ref} role="region" aria-label={_t("a11y|message_composer")}>
{recordingTooltip}
<div className="mx_MessageComposer_wrapper">
<UserIdentityWarning room={this.props.room} key={this.props.room.roomId} />
<ReplyPreview
replyToEvent={this.props.replyToEvent}
permalinkCreator={this.props.permalinkCreator}
Expand Down
337 changes: 337 additions & 0 deletions src/components/views/rooms/UserIdentityWarning.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,337 @@
/*
Copyright 2024 New Vector Ltd.

SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
Please see LICENSE files in the repository root for full details.
*/

import React, { useCallback, useRef, useState } from "react";
import { EventType, KnownMembership, MatrixEvent, Room, RoomStateEvent, RoomMember } from "matrix-js-sdk/src/matrix";
import { CryptoApi, CryptoEvent, UserVerificationStatus } from "matrix-js-sdk/src/crypto-api";
import { logger } from "matrix-js-sdk/src/logger";
import { Button, Separator } from "@vector-im/compound-web";

import { _t } from "../../../languageHandler";
import MemberAvatar from "../avatars/MemberAvatar";
import { useMatrixClientContext } from "../../../contexts/MatrixClientContext";
import { useTypedEventEmitter } from "../../../hooks/useEventEmitter";

interface UserIdentityWarningProps {
/**
* The current room being viewed.
*/
room: Room;
/**
* The ID of the room being viewed. This is used to ensure that the
* component's state and references are cleared when the room changes.
*/
key: string;
}

/**
* Does the given user's identity need to be approved?
*/
async function userNeedsApproval(crypto: CryptoApi, userId: string): Promise<boolean> {
const verificationStatus = await crypto.getUserVerificationStatus(userId);
return verificationStatus.needsUserApproval;
}

/**
* Whether the component is uninitialised, is in the process of initialising, or
* has completed initialising.
*/
enum InitialisationStatus {
Uninitialised,
Initialising,
Completed,
}

/**
* Displays a banner warning when there is an issue with a user's identity.
*
* Warns when an unverified user's identity has changed, and gives the user a
* button to acknowledge the change.
*/
export const UserIdentityWarning: React.FC<UserIdentityWarningProps> = ({ room }) => {
const cli = useMatrixClientContext();
const crypto = cli.getCrypto();

// The current room member that we are prompting the user to approve.
richvdh marked this conversation as resolved.
Show resolved Hide resolved
// `undefined` means we are not currently showing a prompt.
const [currentPrompt, setCurrentPrompt] = useState<RoomMember | undefined>(undefined);

// Whether or not we've already initialised the component by loading the
// room membership.
const initialisedRef = useRef<InitialisationStatus>(InitialisationStatus.Uninitialised);
// Which room members need their identity approved.
const membersNeedingApprovalRef = useRef<Map<string, RoomMember>>(new Map());
// Whether we got a verification status update while we were fetching a
// user's verification status.
//
// We set the entry for a user to `false` when we start fetching a user's
// verification status, and remove the user's entry when we are done
// fetching. When we receive a verification status update, if the entry for
// the user is `false`, we set it to `true`. After we have finished
// fetching the user's verification status, if the entry for the user is
// `true`, rather than `false`, we know that we got an update, and so we
// discard the value that we fetched. We always use the value from the
// update and consider it as the most up-to-date version. If the fetched
// value is more up-to-date, then we should be getting a new update soon
// with the newer value, so it will fix itself in the end.
const gotVerificationStatusUpdateRef = useRef<Map<string, boolean>>(new Map());

// Update the current prompt. Select a new user if needed, or hide the
// warning if we don't have anyone to warn about.
const updateCurrentPrompt = useCallback((): undefined => {
const membersNeedingApproval = membersNeedingApprovalRef.current;
// We have to do this in a callback to `setCurrentPrompt`
// because this function could have been called after an
// `await`, and the `currentPrompt` that this function would
// have may be outdated.
setCurrentPrompt((currentPrompt) => {
// If we're already displaying a warning, and that user still needs
// approval, continue showing that user.
if (currentPrompt && membersNeedingApproval.has(currentPrompt.userId)) return currentPrompt;

if (membersNeedingApproval.size === 0) {
return undefined;
}

// We pick the user with the smallest user ID.
const keys = Array.from(membersNeedingApproval.keys()).sort((a, b) => a.localeCompare(b));
const selection = membersNeedingApproval.get(keys[0]!);
return selection;
});
}, []);

// Add a user to the membersNeedingApproval map, and update the current
// prompt if necessary. The user will only be added if they are actually a
// member of the room. If they are not a member, this function will do
// nothing.
const addMemberNeedingApproval = useCallback(
(userId: string, member?: RoomMember): void => {
if (userId === cli.getUserId()) {
// We always skip our own user, because we can't pin our own identity.
return;
}
member = member ?? room.getMember(userId) ?? undefined;
if (!member) return;

membersNeedingApprovalRef.current.set(userId, member);
// We only select the prompt if we are done initialising,
// because we will select the prompt after we're done
// initialising, and we want to start by displaying a warning
// for the user with the smallest ID.
if (initialisedRef.current === InitialisationStatus.Completed) {
updateCurrentPrompt();
}
},
[cli, room, updateCurrentPrompt],
);

// Check if the user's identity needs approval, and if so, add them to the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Check if the user's identity needs approval, and if so, add them to the
// For each user in the list, check if their identity needs approval, and if so, add them to the

// membersNeedingApproval map and update the prompt if needed. They will
// only be added if they are a member of the room.
const addMembersWhoNeedApproval = useCallback(
async (members: RoomMember[]): Promise<void> => {
const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current;

const promises: Promise<void>[] = [];

for (const member of members) {
const userId = member.userId;
if (gotVerificationStatusUpdate.has(userId)) {
// We're already checking their verification status, so we don't
// need to do anything here.
continue;
}
gotVerificationStatusUpdate.set(userId, false);
promises.push(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of interest, was the switch to doing this in parallel meant to fix something, or just an optimisation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an optimisation. I changed the function to take a list of users so that it would update gotVerificationStatusUpdate for all the users at once, so that if we get an update for, say, Alice, but we haven't started fetching her update status, we won't try to overwrite the update with a fetched value. Then I figured it was silly that we were waiting for one to complete before querying the next.

userNeedsApproval(crypto!, userId).then((needsApproval) => {
if (needsApproval) {
// Only actually update the list if we haven't received a
// `UserTrustStatusChanged` for this user in the meantime.
if (gotVerificationStatusUpdate.get(userId) === false) {
addMemberNeedingApproval(userId, member);
}
}
gotVerificationStatusUpdate.delete(userId);
}),
);
}

await Promise.all(promises);
},
[crypto, addMemberNeedingApproval],
);

// Remove a user from the membersNeedingApproval map, and update the current
// prompt if necessary.
const removeMemberNeedingApproval = useCallback(
(userId: string): void => {
membersNeedingApprovalRef.current.delete(userId);
updateCurrentPrompt();
},
[updateCurrentPrompt],
);

// Initialise the component. Get the room members, check which ones need
// their identity approved, and pick one to display.
const loadMembers = useCallback(async (): Promise<void> => {
if (!crypto || initialisedRef.current !== InitialisationStatus.Uninitialised) {
return;
}
// If encryption is not enabled in the room, we don't need to do
// anything. If encryption gets enabled later, we will retry, via
// onRoomStateEvent.
if (!(await crypto.isEncryptionEnabledInRoom(room.roomId))) {
return;
}
initialisedRef.current = InitialisationStatus.Initialising;

const members = await room.getEncryptionTargetMembers();
await addMembersWhoNeedApproval(members);

updateCurrentPrompt();
initialisedRef.current = InitialisationStatus.Completed;
}, [crypto, room, addMembersWhoNeedApproval, updateCurrentPrompt]);

loadMembers().catch((e) => {
logger.error("Error initialising UserIdentityWarning:", e);
});

// When a user's verification status changes, we check if they need to be
// added/removed from the set of members needing approval.
const onUserVerificationStatusChanged = useCallback(
(userId: string, verificationStatus: UserVerificationStatus): void => {
const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current;

// If we haven't started initialising, that means that we're in a
// room where we don't need to display any warnings.
if (initialisedRef.current === InitialisationStatus.Uninitialised) {
return;
}

if (gotVerificationStatusUpdate.has(userId)) {
// There is an ongoing call to `addMembersWhoNeedApproval`. Flag
// to it that we've got a more up-to-date result so it should
// discard its result.
gotVerificationStatusUpdate.set(userId, true);
uhoreg marked this conversation as resolved.
Show resolved Hide resolved
}

if (verificationStatus.needsUserApproval) {
addMemberNeedingApproval(userId);
} else {
removeMemberNeedingApproval(userId);
}
},
[addMemberNeedingApproval, removeMemberNeedingApproval],
);
useTypedEventEmitter(cli, CryptoEvent.UserTrustStatusChanged, onUserVerificationStatusChanged);

// We watch for encryption events (since we only display warnings in
// encrypted rooms), and for membership changes (since we only display
// warnings for users in the room).
const onRoomStateEvent = useCallback(
async (event: MatrixEvent): Promise<void> => {
if (!crypto || event.getRoomId() !== room.roomId) {
return;
}

const eventType = event.getType();
if (eventType === EventType.RoomEncryption && event.getStateKey() === "") {
// Room is now encrypted, so we can initialise the component.
return loadMembers().catch((e) => {
logger.error("Error initialising UserIdentityWarning:", e);
});
} else if (eventType !== EventType.RoomMember) {
return;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// We're processing an m.room.member event

if (initialisedRef.current === InitialisationStatus.Uninitialised) {
return;
}
richvdh marked this conversation as resolved.
Show resolved Hide resolved

const userId = event.getStateKey();

if (!userId) return;

if (
event.getContent().membership === KnownMembership.Join ||
(event.getContent().membership === KnownMembership.Invite && room.shouldEncryptForInvitedMembers())
) {
// Someone's membership changed and we will now encrypt to them. If
// their identity needs approval, show a warning.
const member = room.getMember(userId);
if (member) {
await addMembersWhoNeedApproval([member]).catch((e) => {
logger.error("Error adding member in UserIdentityWarning:", e);
});
}
} else {
// Someone's membership changed and we no longer encrypt to them.
// If we're showing a warning about them, we don't need to any more.
removeMemberNeedingApproval(userId);
const gotVerificationStatusUpdate = gotVerificationStatusUpdateRef.current;
if (gotVerificationStatusUpdate.has(userId)) {
// There is an ongoing call to `addMembersWhoNeedApproval`.
// Indicate that we've received a verification status update
// to prevent it from trying to add the user as needing
// verification.
gotVerificationStatusUpdate.set(userId, true);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okayyy, but now don't we have a problem for a join-leave-join scenario?

I can't help but feel we need a proper queue of things to process :/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I figured out a way to solve it while minimising the changes.

}
},
[crypto, room, addMembersWhoNeedApproval, removeMemberNeedingApproval, loadMembers],
);
useTypedEventEmitter(cli, RoomStateEvent.Events, onRoomStateEvent);

if (!crypto || !currentPrompt) return null;

const confirmIdentity = async (): Promise<void> => {
await crypto.pinCurrentUserIdentity(currentPrompt.userId);
};

return (
<div className="mx_UserIdentityWarning">
<Separator />
<div className="mx_UserIdentityWarning_row">
<MemberAvatar member={currentPrompt} title={currentPrompt.userId} size="30px" />
<span className="mx_UserIdentityWarning_main">
{currentPrompt.rawDisplayName === currentPrompt.userId
? _t(
"encryption|pinned_identity_changed_no_displayname",
{ userId: currentPrompt.userId },
{
a: substituteATag,
b: substituteBTag,
},
)
: _t(
"encryption|pinned_identity_changed",
{ displayName: currentPrompt.rawDisplayName, userId: currentPrompt.userId },
{
a: substituteATag,
b: substituteBTag,
},
)}
</span>
<Button kind="primary" size="sm" onClick={confirmIdentity}>
{_t("action|ok")}
</Button>
</div>
</div>
);
};

function substituteATag(sub: string): React.ReactNode {
return (
<a href="https://element.io/help#encryption18" target="_blank" rel="noreferrer noopener">
{sub}
</a>
);
}

function substituteBTag(sub: string): React.ReactNode {
return <b>{sub}</b>;
}
2 changes: 2 additions & 0 deletions src/i18n/strings/en_EN.json
Original file line number Diff line number Diff line change
Expand Up @@ -904,6 +904,8 @@
"warning": "If you didn't set the new recovery method, an attacker may be trying to access your account. Change your account password and set a new recovery method immediately in Settings."
},
"not_supported": "<not supported>",
"pinned_identity_changed": "%(displayName)s's (<b>%(userId)s</b>) identity appears to have changed. <a>Learn more</a>",
"pinned_identity_changed_no_displayname": "<b>%(userId)s</b>'s identity appears to have changed. <a>Learn more</a>",
"recovery_method_removed": {
"description_1": "This session has detected that your Security Phrase and key for Secure Messages have been removed.",
"description_2": "If you did this accidentally, you can setup Secure Messages on this session which will re-encrypt this session's message history with a new recovery method.",
Expand Down
Loading
Loading