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

Multidevice FR Fixes #1138

Merged
merged 14 commits into from
May 28, 2020
Merged

Multidevice FR Fixes #1138

merged 14 commits into from
May 28, 2020

Conversation

vincentbavitz
Copy link

@vincentbavitz vincentbavitz commented May 21, 2020

Core Issues Fixed

FR Auto Accept From Secondary without Explicit from B

Issue

Three devices can get into a state where the primary device of one user remains in a deadlock where friend requests will never confirm.
The users can enter a state where one A1 is friends with B, and yet B never accepted ANY friend request from any of A's devices.

Desired Behaviour

When A2 messages B with a friend request, A1 should only auto-accept if B:

  1. Explicitly accepts the FR from A2
  2. Explicitly sends FR to A1
  3. Sends message back to A2 or A1.

Reproduction

fr-link

  • B opens the friend request from A2 in the Contacts dropdown. B does NOT explicitly accept the friend request
    image-20200515155127049
  • A2 syncs with A1.

Acceptance from B in Chat Yields Stalemate

Issue

Three devices can get into a stale mate where A1, being linked with A2, cannot establish a session with B after B has accepted a FR from A2 in chat.
When B accepts the FR from A2 in chat, (through clicking Accept, not through messaging), the conversation is marked as "Waiting for friend request approval" on B and A2, and is nonexistent for A1.

Desired Behaviour

When A2 messages B with a friend request, and B accepts,
A2 should first see FR approved, and sync with A1. A1 should Then

Reproduction

  • A1 links with A2

  • A2 sends FR to B

  • B now sees from A2:

    isFriend: false
    isPendingFriendRequest: true
    hasReceivedFriendRequest: true
    hasSentFriendRequest: false
    activeAt: undefined
    lastMessage: {status: undefined, text: undefined}
  • B accepts FR from A2 IN CHAT.
    This is a conversation with A1! Who has NOT sent a FR.
    image-20200518133657461
    Therefore B is trying to accept a non-existent FR.

Fixed.



Small Issues Fixed

FR From Secondary Showing in Conversations

Friend requests from secondary devices (A2) previously appeared in the Messages pane of the recipient (B). Fixed.

Can't Accept FR From Secondary in Contacts Dropdown

Friend requests from secondary devices (A2) were previously inactionable from the dropdown of the recipient (B). Accept / Decline had no effect as the actions pointed to the wrong conversation model.
image-20200518134821791
Fixed.

Can't Accept FR From Secondary in Chat

The issue with accepting FR from A2 in the dialog, is that the FR accept is going to A1. We need to ensure that this message is only shown for the device who sent the FR.
image-20200518133657461

@vincentbavitz vincentbavitz force-pushed the fr-fixes branch 4 times, most recently from c58336d to f634c3e Compare May 27, 2020 00:21
js/models/messages.js Outdated Show resolved Hide resolved
@neuroscr
Copy link

neuroscr commented May 27, 2020

Nitpick:

Really good documentation in the PR but reading the code there are few comments to see what changes map to what fix and/or comments WHY we want this new defined behavior. As you dove down into this mess, it really helps others to document what you learned in the code and also for yourself if you have to come back to the code again in 6+ months. If we had comments expressing the desired behavior you want, then it would be easier for us reviewers to confirm the code does what you want it to.

@vincentbavitz
Copy link
Author

Nitpick:

Really good documentation in the PR but reading the code there are few comments to see what changes map to what fix and/or comments WHY we want this new defined behavior. As you dove down into this mess, it really helps others to document what you learned in the code and also for yourself if you have to come back to the code again in 6+ months. If we had comments expressing the desired behavior you want, then it would be easier for us reviewers to confirm the code does what you want it to.

Sure -- will update after lunch 👍

package.json Outdated Show resolved Hide resolved
libtextsecure/message_receiver.js Outdated Show resolved Hide resolved
js/models/messages.js Show resolved Hide resolved
js/models/conversations.js Outdated Show resolved Hide resolved
Copy link

@Mikunj Mikunj left a comment

Choose a reason for hiding this comment

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

Looks good, had the similar comments to what Audric said.

@vincentbavitz vincentbavitz force-pushed the fr-fixes branch 3 times, most recently from 68df329 to 0d911ca Compare May 27, 2020 04:41
@vincentbavitz vincentbavitz merged commit 13f2890 into oxen-io:clearnet May 28, 2020
@vincentbavitz vincentbavitz changed the title [WIP] Multidevice FR Fixes Multidevice FR Fixes May 28, 2020
@@ -245,6 +246,8 @@
this.messageCollection.forEach(m => m.trigger('change'));
},
async acceptFriendRequest() {
// Friend request message conmfirmations (Accept / Decline) are always
Copy link

Choose a reason for hiding this comment

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

This comment isn't true from a protocol perspective. Friend request acceptance happens on a per-device basis (because friend request status is device based and not user based). Also, declining a message doesn't send anything.

}

profileName = conversation.getProfileName() || profileName;
conversation.onAcceptFriendRequest();
Copy link

Choose a reason for hiding this comment

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

conversation.onAcceptFriendRequest() already loops through all devices associated with the contact, right? So invoking this for each of the contact's devices means you're going to send the friend request accepted message twice to each device 🤔

Copy link

Choose a reason for hiding this comment

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

Uh it doesn't.

 async onAcceptFriendRequest(options = {}) {
      if (this.unlockTimer) {
        clearTimeout(this.unlockTimer);
      }
      if (this.hasReceivedFriendRequest()) {
        this.setFriendRequestStatus(FriendRequestStatusEnum.friends, options);

        await this.respondToAllFriendRequests({
          response: 'accepted',
          direction: 'incoming',
          status: ['pending', 'expired'],
        });
        window.libloki.api.sendBackgroundMessage(
          this.id,
          window.textsecure.OutgoingMessage.DebugMessageType
            .INCOMING_FR_ACCEPTED
        );
      }
    },

As seen from above it just sets its own friend request status and sends out a FR accept to the specific device.

Copy link

Choose a reason for hiding this comment

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

Oh, I was looking at conversations.js → acceptFriendRequest(). I didn't realize there's a third method with this name :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants