Display a warning when an unverified user's identity changes #28211
Conversation
uhoreg
requested review from
a team and
richvdh
and removed request for
a team
|
|
uhoreg
force-pushed
the
unverified_user_identity_change_warning
branch
from
ada3de0
to
6fa98f8
Compare
There was a problem hiding this comment.
In a first time, we should move UserIdentifyWarning into a functional component and break down its behaviour into multiple hooks. Let me know if you need help to do it
There are differences with the figma design too.
|
I think that I've addressed all of @florianduros 's concerns. The test failure looks like possible flakiness, since it's testing components that I haven't touched. |
There was a problem hiding this comment.
It's looking better! Now we can move theses hooks and their logic into separate custom hooks in order to encapsulate their behaviour and make the component easier to maintain.
In test:
- use
jest.spyOnin test to handle the mocking of the client or the crypto api. - use https://testing-library.com/docs/queries/about/ to get the dom element. We want to favour the
rolequery in order to make it sure that we are also accessible - In case of async behaviour when we need to wait for a rerender,
waitForis doing for us the retry/waiting.
test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx
Outdated
Show resolved
Hide resolved
test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx
Outdated
Show resolved
Hide resolved
test/unit-tests/components/views/rooms/UserIdentityWarning-test.tsx
Outdated
Show resolved
Hide resolved
|
@florianduros ping |
|
I've addressed all of Florian's concerns. @richvdh can you give this a crypto team review? |
|
|
||
| if ( | ||
| event.getContent().membership === KnownMembership.Join || | ||
| (event.getContent().membership === KnownMembership.Join && room.shouldEncryptForInvitedMembers()) |
There was a problem hiding this comment.
| (event.getContent().membership === KnownMembership.Join && room.shouldEncryptForInvitedMembers()) | |
| (event.getContent().membership === KnownMembership.Invite && room.shouldEncryptForInvitedMembers()) |
I'd feel happier if we had a unit test that picked this up...
|
I managed to find another race condition while writing a test, so I need to investigate and fix it. |
| ) { | ||
| // Someone's membership changed and we will now encrypt to them. If | ||
| // their identity needs approval, show a warning. | ||
| await addMemberIfNeedsApproval(userId); |
There was a problem hiding this comment.
Why do we catch on the loadMembers() call at line 241, but not here? Any errors thrown here will get lost.
| 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. | ||
| await addMemberIfNeedsApproval(userId); | ||
| } 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); | ||
| } |
There was a problem hiding this comment.
Suppose we see a member join and leave in quick succession (potentially, in the same /sync response)
The join will trigger a call to addMemberIfNeedsApproval, which will start a call to userNeedsApproval.
The leave will run removeMemberNeedingApproval, which won't do much.
userNeedsApproval will complete, and the user may be added to the list of approvals needed. We now have a non-member in the list of approvals.
There was a problem hiding this comment.
Right. Similar thing if someone leaves while we're still processing the initial room membership. Will have to think about how to solve this. I think that setting the flag in gotVerificationStatusUpdate might fix the join-then-leave issue, but not the leave-while-initialising problem.
| } | ||
| gotVerificationStatusUpdate.set(userId, false); | ||
| if (await userNeedsApproval(crypto!, userId)) { | ||
| if (!membersNeedingApproval.has(userId) && gotVerificationStatusUpdate.get(userId) === false) { |
There was a problem hiding this comment.
| if (!membersNeedingApproval.has(userId) && gotVerificationStatusUpdate.get(userId) === false) { | |
| // Only actually update the list if we haven't received a `UserTrustStatusChanged` for this user in | |
| // the meantime. | |
| if (!membersNeedingApproval.has(userId) && gotVerificationStatusUpdate.get(userId) === false) { |
There was a problem hiding this comment.
Also, consider removing !membersNeedingApproval.has(userId) from this condition. AFAICT, addMemberNeedingApproval is a no-op if the user is already in the list, and removing the check would simplify this function.
| const selectCurrentPrompt = useCallback((): RoomMember | undefined => { | ||
| const membersNeedingApproval = membersNeedingApprovalRef.current; | ||
| 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; | ||
| }, []); |
There was a problem hiding this comment.
aside: I wondered if it would be good to have a single updateCurrentPrompt function which updates currentPrompt based on the current state of the list. Hence we'd have something like:
const updateCurrentPrompt = useCallback(() => {
// Some sort of comment about callbacks and await
setCurrentPrompt((currentPrompt) => {
// If we have an existing prompt, leave it in place unless the user has been removed from the list
if (currentPrompt && membersNeedingApproval.has(currentPrompt.userId)) {
return currentPrompt;
}
return selectCurrentPrompt(); // probably inline this
});
}, [setCurrentPrompt]);... and then use that in the three places we currently call setCurrentPrompt.
It feels cognitively simpler to me, and has the advantage that we don't need to worry about someone slipping in an async before a call to setCurrentPrompt and breaking everything.
| [cli, room, updateCurrentPrompt], | ||
| ); | ||
|
|
||
| // Check if the user's identity needs approval, and if so, add them to the |
There was a problem hiding this comment.
| // 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 |
| } else if (eventType !== EventType.RoomMember) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
| // We're processing an m.room.member event | |
| 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); | ||
| } |
There was a problem hiding this comment.
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 :/
There was a problem hiding this comment.
I think I figured out a way to solve it while minimising the changes.
| } | ||
| gotVerificationStatusUpdate.set(userId, false); | ||
| promises.push( |
There was a problem hiding this comment.
out of interest, was the switch to doing this in parallel meant to fix something, or just an optimisation?
There was a problem hiding this comment.
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.
Fixes element-hq/element-meta#2513
Checklist
public/exportedsymbols have accurate TSDoc documentation.