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.

Sign up for GitHub

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

feat(crypto): Support verification violation composer banner #29067

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
+209 −46
Draft

feat(crypto): Support verification violation composer banner #29067

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions res/css/views/rooms/_UserIdentityWarning.pcss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,14 @@ Please see LICENSE files in the repository root for full details.
margin-left: var(--cpd-space-6x);
flex-grow: 1;
}
.mx_UserIdentityWarning_main.critical {
color: var(--cpd-color-text-critical-primary);
}
}
}
.mx_UserIdentityWarning.critical {
background: linear-gradient(180deg, var(--cpd-color-red-100) 0%, var(--cpd-color-theme-bg) 100%);
}

.mx_MessageComposer.mx_MessageComposer--compact > .mx_UserIdentityWarning {
margin-left: calc(-25px + var(--RoomView_MessageList-padding));
Expand Down
164 changes: 119 additions & 45 deletions src/components/views/rooms/UserIdentityWarning.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,23 @@
key: string;
}

type ViolationType = "PinViolation" | "VerificationViolation";

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

function mapToViolationType(verificationStatus: UserVerificationStatus): ViolationType | null {
if (verificationStatus.wasCrossSigningVerified() && !verificationStatus.isCrossSigningVerified()) {
return "VerificationViolation";
} else if (verificationStatus.needsUserApproval) {
return "PinViolation";
}
return null;
}

/**
Expand All @@ -46,6 +57,11 @@
Completed,
}

type ViolationPrompt = {
member: RoomMember;
type: ViolationType;
};

/**
* Displays a banner warning when there is an issue with a user's identity.
*
Expand All @@ -58,13 +74,13 @@

// The current room member that we are prompting the user to approve.
// `undefined` means we are not currently showing a prompt.
const [currentPrompt, setCurrentPrompt] = useState<RoomMember | undefined>(undefined);
const [currentPrompt, setCurrentPrompt] = useState<ViolationPrompt | 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());
const membersNeedingApprovalRef = useRef<Map<string, ViolationPrompt>>(new Map());
// For each user, we assign a sequence number to each verification status
// that we get, or fetch.
//
Expand Down Expand Up @@ -100,7 +116,8 @@
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 (currentPrompt && membersNeedingApproval.get(currentPrompt.member.userId)?.type === currentPrompt.type)
return currentPrompt;

if (membersNeedingApproval.size === 0) {
if (currentPrompt) {
Expand All @@ -113,7 +130,10 @@
// 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]!);
logger.debug(`UserIdentityWarning: now warning about user ${selection?.userId}`);
logger.debug(`UserIdentityWarning: selection is ${JSON.stringify(selection)}`);
logger.debug(
`UserIdentityWarning: now warning about user ${selection?.member.userId} for a ${selection?.type} violation`,
);
return selection;
});
}, []);
Expand All @@ -123,15 +143,24 @@
// member of the room. If they are not a member, this function will do
// nothing.
const addMemberNeedingApproval = useCallback(
(userId: string, member?: RoomMember): void => {
(userId: string, violation?: ViolationType, member?: RoomMember): void => {
logger.debug(`UserIdentityWarning: add member ${userId} for violation ${violation}`);

if (userId === cli.getUserId()) {
// We always skip our own user, because we can't pin our own identity.
return;
}
if (violation === undefined) return;

// Member might be already resolved depending on the context. When called after a
// CryptoEvent.UserTrustStatusChanged event emitted it will not yet be resolved.
member = member ?? room.getMember(userId) ?? undefined;
if (!member) return;
if (!member) {
logger.debug(`UserIdentityWarning: user ${userId} not found in room members, ignoring violation`);
return;
}

membersNeedingApprovalRef.current.set(userId, member);
membersNeedingApprovalRef.current.set(userId, { member, type: violation });
// 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
Expand Down Expand Up @@ -159,12 +188,12 @@
const userId = member.userId;
const sequenceNum = incrementVerificationStatusSequence(userId);
promises.push(
userNeedsApproval(crypto!, userId).then((needsApproval) => {
if (needsApproval) {
userNeedsApproval(crypto!, userId).then((type) => {
if (type != null) {
// Only actually update the list if we have the most
// recent value.
if (verificationStatusSequences.get(userId) === sequenceNum) {
addMemberNeedingApproval(userId, member);
addMemberNeedingApproval(userId, type, member);
}
}
}),
Expand Down Expand Up @@ -231,8 +260,10 @@

incrementVerificationStatusSequence(userId);

if (verificationStatus.needsUserApproval) {
addMemberNeedingApproval(userId);
const violation = mapToViolationType(verificationStatus);

if (violation) {
addMemberNeedingApproval(userId, violation);
} else {
removeMemberNeedingApproval(userId);
}
Expand Down Expand Up @@ -296,39 +327,82 @@
if (!crypto || !currentPrompt) return null;

const confirmIdentity = async (): Promise<void> => {
await crypto.pinCurrentUserIdentity(currentPrompt.userId);
if (currentPrompt.type === "VerificationViolation") {
await crypto.withdrawVerificationRequirement(currentPrompt.member.userId);

Check failure on line 331 in src/components/views/rooms/UserIdentityWarning.tsx

View workflow job for this annotation

GitHub Actions / Typescript Syntax Check 

Property 'withdrawVerificationRequirement' does not exist on type 'CryptoApi'.
} else if (currentPrompt.type === "PinViolation") {
await crypto.pinCurrentUserIdentity(currentPrompt.member.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>
if (currentPrompt.type === "VerificationViolation") {
return (
<div className="mx_UserIdentityWarning critical">
<Separator />
<div className="mx_UserIdentityWarning_row">
<MemberAvatar member={currentPrompt.member} title={currentPrompt.member.userId} size="30px" />
<span className="mx_UserIdentityWarning_main critical">
{currentPrompt.member.rawDisplayName === currentPrompt.member.userId
? _t(
"encryption|verified_identity_changed_no_displayname",
{ userId: currentPrompt.member.userId },
{
a: substituteATag,
b: substituteBTag,
},
)
: _t(
"encryption|verified_identity_changed",
{
displayName: currentPrompt.member.rawDisplayName,
userId: currentPrompt.member.userId,
},
{
a: substituteATag,
b: substituteBTag,
},
)}
</span>
<Button kind="secondary" size="sm" onClick={confirmIdentity}>
{_t("encryption|withdraw_verification_action")}
</Button>
</div>
</div>
</div>
);
);
} else {
return (
<div className="mx_UserIdentityWarning">
<Separator />
<div className="mx_UserIdentityWarning_row">
<MemberAvatar member={currentPrompt.member} title={currentPrompt.member.userId} size="30px" />
<span className="mx_UserIdentityWarning_main">
{currentPrompt.member.rawDisplayName === currentPrompt.member.userId
? _t(
"encryption|pinned_identity_changed_no_displayname",
{ userId: currentPrompt.member.userId },
{
a: substituteATag,
b: substituteBTag,
},
)
: _t(
"encryption|pinned_identity_changed",
{
displayName: currentPrompt.member.rawDisplayName,
userId: currentPrompt.member.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 {
Expand Down
5 changes: 4 additions & 1 deletion src/i18n/strings/en_EN.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1020,8 +1020,11 @@
"waiting_other_user": "Waiting for %(displayName)s to verify…"
},
"verification_requested_toast_title": "Verification requested",
"verified_identity_changed": "%(displayName)s's (<b>%(userId)s</b>) verified identity has changed. <a>Learn more</a>",
"verified_identity_changed_no_displayname": "<b>%(userId)s</b>'s verified identity has changed. <a>Learn more</a>",
"verify_toast_description": "Other users may not trust it",
"verify_toast_title": "Verify this session"
"verify_toast_title": "Verify this session",
"withdraw_verification_action": "Withdraw verification"
},
"error": {
"admin_contact": "Please <a>contact your service administrator</a> to continue using this service.",
Expand Down
80 changes: 80 additions & 0 deletions test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@
jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([
mockRoomMember("@alice:example.org", "Alice"),
]);
// jest.spyOn(room, "getMember").mockReturnValue(
// mockRoomMember("@alice:example.org", "Alice")
// );
const crypto = client.getCrypto()!;
jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue(
new UserVerificationStatus(false, false, false, true),
Expand All @@ -109,6 +112,49 @@
await waitFor(() => expect(crypto.pinCurrentUserIdentity).toHaveBeenCalledWith("@alice:example.org"));
});

// This tests the basic functionality of the component. If we have a room
// member whose identity is in verification violation, we should display a warning. When
// the "Withdraw verification" button gets pressed, it should call `withdrawVerification`.
it("displays a warning when a user's identity is in verification violation", async () => {
jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([
mockRoomMember("@alice:example.org", "Alice"),
]);
const crypto = client.getCrypto()!;
jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue(
new UserVerificationStatus(false, true, false, true),
);
crypto.withdrawVerificationRequirement = jest.fn();

Check failure on line 126 in test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx

View workflow job for this annotation

GitHub Actions / Typescript Syntax Check 

Property 'withdrawVerificationRequirement' does not exist on type 'CryptoApi'.
renderComponent(client, room);

await waitFor(() =>
expect(getWarningByText("Alice's (@alice:example.org) verified identity has changed.")).toBeInTheDocument(),
);

expect(
screen.getByRole("button", {
name: "Withdraw verification",
}),
).toBeInTheDocument();
await userEvent.click(screen.getByRole("button")!);
await waitFor(() => expect(crypto.withdrawVerificationRequirement).toHaveBeenCalledWith("@alice:example.org"));

Check failure on line 139 in test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx

View workflow job for this annotation

GitHub Actions / Typescript Syntax Check 

Property 'withdrawVerificationRequirement' does not exist on type 'CryptoApi'.
});

it("Should not display a warning if the user was verified and is still verified", async () => {
jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([
mockRoomMember("@alice:example.org", "Alice"),
]);
const crypto = client.getCrypto()!;
jest.spyOn(crypto, "getUserVerificationStatus").mockResolvedValue(
new UserVerificationStatus(true, true, false, false),
);

renderComponent(client, room);
await sleep(10); // give it some time to finish initialising

expect(() => getWarningByText("Alice's (@alice:example.org) identity appears to have changed.")).toThrow();
expect(() => getWarningByText("Alice's (@alice:example.org) verified identity has changed.")).toThrow();
});

// We don't display warnings in non-encrypted rooms, but if encryption is
// enabled, then we should display a warning if there are any users whose
// identity need accepting.
Expand Down Expand Up @@ -472,6 +518,40 @@
);
});

it("displays the next user when the verification requirement is withdrawn", async () => {
jest.spyOn(room, "getEncryptionTargetMembers").mockResolvedValue([
mockRoomMember("@alice:example.org", "Alice"),
mockRoomMember("@bob:example.org"),
]);
const crypto = client.getCrypto()!;
jest.spyOn(crypto, "getUserVerificationStatus").mockImplementation(async (userId) => {
if (userId == "@alice:example.org") {
return new UserVerificationStatus(false, true, false, true);
} else {
return new UserVerificationStatus(false, false, false, true);
}
});

renderComponent(client, room);
// We should warn about Alice's identity first.
await waitFor(() =>
expect(getWarningByText("Alice's (@alice:example.org) verified identity has changed.")).toBeInTheDocument(),
);

// Simulate Alice's new identity having been approved, so now we warn
// about Bob's identity.
act(() => {
client.emit(
CryptoEvent.UserTrustStatusChanged,
"@alice:example.org",
new UserVerificationStatus(false, false, false, false),
);
});
await waitFor(() =>
expect(getWarningByText("@bob:example.org's identity appears to have changed.")).toBeInTheDocument(),
);
});

// If we get an update for a user's verification status while we're fetching
// that user's verification status, we should display based on the updated
// value.
Expand Down
Loading