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

Track invites and allow cancelling invites #447

Closed
gmaclennan opened this issue Jan 29, 2024 · 8 comments · Fixed by #525
Closed

Track invites and allow cancelling invites #447

gmaclennan opened this issue Jan 29, 2024 · 8 comments · Fixed by #525
Assignees
Labels
mvp Requirement for MVP project invite

Comments

@gmaclennan
Copy link
Member

A peer might have sent an invite and then want to expire it manually, or change the invite (e.g. re-invite) with different role positions. This would require the receiver to:

A) Override a previously received invite with the new one
B) Include some kind of identifier in the response to the the invitor know which invite is being responded to

@gmaclennan gmaclennan changed the title Track invites and allow manual expiration of invites Track invites and allow cancelling invites Feb 8, 2024
@gmaclennan
Copy link
Member Author

After reviewing the implementation of invites in the front-end, it seems like we need a way to cancel invites.

If a user invites another device, they are currently left waiting for a response, and need a way to cancel if they do not receive a reply within an expected time-frame. The cancellation should ensure that when a response is received, it is ignored (currently whenever a response to an invite is received, the invited device is automatically added to the project).

The cancellation should also send a wire message to the invitee, so that they can see that their invite has been cancelled (and this should result in the project key to be deleted, and the project instance to be deleted if it has not already been).

This points to perhaps the need for an additional step in the invite flow, of the invitee receiving a final message back about whether they were actually added to a project.

@EvanHahn EvanHahn added the mvp Requirement for MVP label Feb 8, 2024
@EvanHahn
Copy link
Contributor

EvanHahn commented Feb 8, 2024

Here's a proposal for how we'd do this.

tl;dr

Currently, two messages are sent:

  1. Inviter sends an invite message, which has lots of info
  2. Invitee sends ACCEPT, REJECT, or ALREADY
  3. Inviter adds them to the project locally

I propose that three messages be sent:

  1. Inviter sends an invite message, which has limited info
  2. Invitee sends ACCEPT, REJECT, or ALREADY
  3. Inviter responds with project details (project key, encryption keys, etc) if they receive an ACCEPT, and add them to the project

Made a little diagram:

sequenceDiagram
  Inviter->>Invitee: Invite (limited info)
  Invitee->>Inviter: Accept
  Inviter->>Invitee: Project details (key, encryption keys, etc)
Loading

Goals

Here are the goals of the invite system (I think):

  • Users can invite multiple peers (though this may be hidden in the frontend)
  • Users can receive multiple invites from other peers, possibly conflicting
  • Users can respond to invites by accepting, rejecting, or saying they're already in the project
  • Users can cancel outbound invites
  • Users can re-invite peers for any reason
  • Invites are culled and do not grow indefinitely
  • Outbound invites can have a TTL (e.g., a timeout)
  • Messages may be lost in transit
  • Devices may have bugs (e.g., accepting invites twice) or be malicious
  • New: when the inviter cancels the invite, the invitee should know (and not join the project)
  • New: when the invitee accepts, it should be confirmed by the inviter before joining (without UI)
  • New: the invitee shouldn't get the project key until they accept

Non-goals:

  • Invites are persisted across app restarts (it's okay if they're only in memory)
  • Handle conflicting roles, e.g., two invites for the same project with different roles (this is important but out of scope)

Proposed user experience

Happy path:

  1. Inviter taps "invite this peer" in the UI. It is shown as pending.
  2. Invitee receives the invite and accepts. It is shown as pending.
  3. Inviter sees that their invite is no longer pending.
  4. Invitee sees that their invite is no longer pending.

Happy path where two people invite the same person:

  1. Inviter A taps "invite this peer" in the UI. It is shown as pending.
  2. Inviter B taps "invite this peer" in the UI. It is shown as pending.
  3. Invitee receives the Inviter A's invite and accepts. It is shown as pending. (Inviter B's invite is never shown.)
  4. Inviter A sees that their invite is no longer pending.
  5. Invitee sees that their invite is no longer pending.
  6. Inviter B sees that their invite is no longer pending. (This step may happen earlier.)

Unhappy path where invite is canceled:

  1. Inviter taps "invite this peer" in the UI. It is shown as pending.
  2. Inviter cancels the invite, removing it from the UI.
  3. Invitee receives the invite and accepts. It is shown as pending.
  4. Invitee sees an error message.

Unhappy path where invite is rejected:

  1. Inviter taps "invite this peer" in the UI. It is shown as pending.
  2. Invitee receives the invite and rejects. It is instantly removed from their UI.
  3. Inviter sees that their invite was rejected. The invite is removed from the UI.

Unhappy path where the user is already part of the project:

  1. Inviter taps "invite this peer" in the UI. It is shown as pending.
  2. Invitee sees that their invite is no longer pending; it appears like a success.

Unhappy path where connection is lost before invite can be sent:

  1. Inviter taps "invite this peer" in the UI. It is shown as pending.
  2. After a timeout, the inviter sees an error message that their invite failed. The invite is removed from the UI.

Unhappy path where connection is lost before invite can be sent:

  1. Inviter taps "invite this peer" in the UI. It is shown as pending.
  2. Invitee receives the invite and accepts. It is shown as pending.
  3. After a timeout, the invitee sees an error message that the invite could not be accepted. They can try again. If they accept, tries to connect again and may succeed. If they reject, the invite is instantly removed from the UI.
  4. After a timeout, the inviter sees an error message that their invite failed. The invite is removed from the UI.

Proposed front-end code

  1. Inviter front-end code calls project.$member.invite(deviceId, { roleId, abortSignal }).

    This will resolve with a decision (accepted, rejected, already a member, or unrecognized). It will reject if any of the arguments are invalid, if there is a pending invite for this device, if the device ID is not found, if a network connection occurs, or if the invite has been locally canceled.

    Here's a sketch:

    export const ReviewAndInvite: NativeNavigationComponent<"ReviewAndInvite"> = ({
      /* ... */
    }) => {
      // ...
    
      const inviteAbortControllerRef = React.useRef<null | AbortController>(null);
    
      const sendInvite = React.useCallback(() => {
        const abortController = new AbortController();
        inviteAbortControllerRef.current = abortController;
    
        return project.$member.invite(deviceId, {
          roleId: role,
          signal: abortController.signal,
        });
      }, [deviceId, role]);
    
      const onSendInvite = () => {
        setInviteStatus("waiting");
      };
    
      const onCancelInvite = () => {
        inviteAbortControllerRef.current?.abort();
      };
    
      const shouldSendInvite = inviteStatus === "waiting";
      React.useEffect(() => {
        if (!shouldSendInvite) return;
    
        let isCanceled = false;
    
        (async () => {
          let success = false;
    
          try {
            switch (await sendInvite()) {
              case "ACCEPT":
              case "ALREADY":
                success = true;
                break;
              default:
                break;
            }
          } catch (err) {
            // No-op.
          }
    
          if (isCanceled) return;
    
          if (success) {
            queryClient.invalidateQueries({ queryKey: ["projectMembers"] });
            navigation.navigate("InviteAccepted", { ...route.params });
          } else {
            openSheet();
          }
        })();
    
        return () => {
          isCanceled = true;
          inviteAbortControllerRef.current?.abort();
        };
      }, [shouldSendInvite, sendInvite]);
    
      return (
        <React.Fragment>
          {inviteStatus === "reviewing"
            ? (
              <ReviewInvitation
                sendInvite={onSendInvite}
                // ...
              />
            )
            : <WaitingForInviteAccept cancelInvite={onCancelInvite} />}
          {/* ... */}
        </React.Fragment>
      );
    };
  2. Invitee front-end code calls manager.invite.accept(projectId) or manager.invite.reject(projectId).

    .accept() will resolve once you're able to join the project. .reject() should resolve immediately.

Proposed back-end code

High-level overview:

sequenceDiagram
  Inviter->>Invitee: Invite (limited info)
  Invitee->>Inviter: Accept
  Inviter->>Invitee: Project details (key, encryption keys, etc)
Loading

Here's a sketch of the inviter code:

export class MemberApi {
  // ...

  async invite(
    deviceId: string,
    { roleId, signal }: { roleId: string; signal: AbortSignal },
  ): Promise<InviteResponse_Decision> {
    // Set up our abort signals.
    const timeoutSignal = AbortSignal.timeout(60_000);
    const abortSignal = AbortSignal.any([signal, timeoutSignal]);
    const handleAborted = () => {
      if (abortSignal.aborted) {
        throw new ProjectInviteError(
          (abortSignal.reason?.name === "TimeoutError") ? "TIMEOUT" : "ABORTED",
        );
      }
    };

    // Was this aborted before we even started?
    handleAborted();

    // Find the peer.
    let peer: Peer;
    try {
      peer = await this.#getPeerByDeviceId(deviceId);
    } catch (err) {
      handleAborted();
      console.error("Couldn't find peer to invite", err);
      throw new ProjectInviteError("PEER_NOT_FOUND");
    }

    // Create a cleanup function.
    const cleanUp = () => {
      peer.pendingInvites.delete(this.#projectId);
    };

    // Do we already have a pending invite for this device?
    if (peer.pendingInvites.has(this.#projectId)) {
      throw new ProjectInviteError("DUPLICATE_INVITE");
    }

    // Make sure we clean up if we abort.
    once(abortSignal, "abort", cleanUp);

    // Add the pending invite.
    //
    // Do this BEFORE we send anything because it's possible (though unlikely) that we'll
    // get a response before we believe it's sent.
    const deferred = Promise.withResolvers<InviteResponse_Decision>();
    peer.pendingInvites.set(this.#projectId, deferred);

    // Send the invite.
    peer.sendInvite({ roleId });

    // Wait for their decision. Make sure we clean up.
    let decision: InviteResponse_Decision;
    try {
      decision = await deferred.promise;
    } finally {
      peer.pendingInvites.delete(this.#projectId);
    }
    handleAborted();

    // Respond to the decision. At this point, we can't cancel.
    switch (decision) {
      case "ACCEPT":
        await peer.sendProjectInfo({ projectKey, encryptionKeys });
        addPeerToProject();
        return decision;
      case "ALREADY":
      case "REJECT":
        return decision;
      case "UNRECOGNIZED":
        console.warn("Got an unrecognized decision.");
        return decision;
      default:
        throw new ShouldBeImpossibleError(decision);
    }
  }

  // ...
}

When the invitee receives the invite message, UI should be shown.

When the invitee receives the project info message, they should join the project without user intervention.

@gmaclennan
Copy link
Member Author

gmaclennan commented Feb 9, 2024

Great, thanks for this, really helpful to have this written up.

Suggestions for goals:

  • Users can invite multiple peers (though this may be hidden in the frontend)

The key thing we are hiding from the front-end is inviting multiple peers at the same time. The UI is currently designed for a serial invite process: invite peer A -> invite accepted -> invite peer B -> invite accepted -> invite peerC etc. The reason for this is to avoid extra complexity in the user experience (handling pending invites managing them) at the expense of inconvenience when inviting multiple people (most common in a training environment). We want to gather more feedback on partners about this when we role it out. But yes, the backend should be able to handle multiple active invites to the same project being "pending" at the same time, but we should not allow it (e.g. throw when trying to send an invite when there is already a pending invite).

  • Users can receive multiple invites from other peers, possibly conflicting

Yes, we can't do anything to avoid this. Different devices could independently decide to invite a peer on the network.

  • Users can respond to invites by accepting, rejecting, or saying they're already in the project

Strictly, users only respond "accept" or "reject". "already" is automatic and without user intervention.

  • Invites are culled and do not grow indefinitely

What we want to do, at this stage, is avoid the user concept (in the UX) that there can be more than one pending invite and it is "in the background". The user experience we currently want is that waiting for an invite response is an active process that involves waiting on a screen, and in-person prompting of the invited device to respond.

  • Outbound invites can have a TTL (e.g., a timeout)

I'm wondering if we should always have this, and show it to the device receiving the invite (e.g. a countdown timer on the invite). I think probably out-of-scope for this initial implementation (the UX part), but I think we should add TTL functionality.

  • Messages may be lost in transit
  • Devices may have bugs (e.g., accepting invites twice) or be malicious
  • New: when the inviter cancels the invite, the invitee should know (and not join the project)
  • New: when the invitee accepts, it should be confirmed by the inviter before joining (without UI)
  • New: the invitee shouldn't get the project key until they accept

Non-goals:

  • Invites are persisted across app restarts (it's okay if they're only in memory)

I think we should auto-cancel invites when the app is closed / put into the background. I think we should also auto-cancel invites when the connection to the peer is lost.

  • Handle conflicting roles, e.g., two invites for the same project with different roles (this is important but out of scope)

What is in scope (and is currently what happens) is that the invitee only sees the first invite they received, and that is the only one they respond to. All other invites (with or without conflicting roles) receive either "reject" or "already" as the response.

Happy path where two people invite the same person:

  1. Inviter A taps "invite this peer" in the UI. It is shown as pending.
  2. Inviter B taps "invite this peer" in the UI. It is shown as pending.
  3. Invitee receives the Inviter A's invite and accepts. It is shown as pending. (Inviter B's invite is never shown.)
  4. Inviter A sees that their invite is no longer pending.

Inviter A sees that their invite is accepted

  1. Invitee sees that their invite is no longer pending.

Invitee sees that their response was accepted, and joins the project.

  1. Inviter B sees that their invite is no longer pending. (This step may happen earlier.)

Inviter B sees that the invitee is already a member of the project (does not happen until invitee responds to A)

Unhappy path where invite is canceled:

  1. Inviter taps "invite this peer" in the UI. It is shown as pending.
  2. Inviter cancels the invite, removing it from the UI.
  3. Invitee receives the invite and accepts. It is shown as pending.
  4. Invitee sees an error message.

I think the invitee needs to know about the cancellation immediately, otherwise the invitor would not be able to re-send the invite (because as a duplicate invite to the same project, it would be ignored by the invitee).

  1. Inviter taps "invite this peer" in the UI. It is shown as pending.
  2. Invitee receives the invite.
  3. Inviter cancels the invite, removing it from the UI.
  4. Invitee sees that the invite has been cancelled, before they are able to accept or reject it.

Unhappy path where the user is already part of the project:

  1. Inviter taps "invite this peer" in the UI. It is shown as pending.
  2. Invitee sees that their invite is no longer pending; it appears like a success.

The invitee should not see anything in this path. I think this maybe meant to say "Inviter" in (2). However the inviter should see that the response is "already part of the project" as distinct from "accepted".

Unhappy path where connection is lost before invite can be sent:

Ideally we detect connection loss and immediately cancel the invite.

Unhappy path where connection is lost before invite can be sent:

  1. Inviter taps "invite this peer" in the UI. It is shown as pending.
  2. Invitee receives the invite and accepts. It is shown as pending.
  3. After a timeout, the invitee sees an error message that the invite could not be accepted. They can try again. If they accept, tries to connect again and may succeed. If they reject, the invite is instantly removed from the UI.

Not sure about this one, e.g. the re-try.

Proposed front-end code

  1. Inviter front-end code calls project.$member.invite(deviceId, { roleId, abortSignal }).

Just a note that we cannot pass an abort signal across the IPC channel. We can create this method on the front-end by overriding the $member.invite() method in @mapeo/ipc (like we do for getProject, but we'll need to implement this via event emitters across IPC.

This will resolve with a decision (accepted, rejected, already a member, or unrecognized).

I think maybe unrecognized should be a rejection? I guess it doesn't matter too much, but that seems more like an "error" to me.

Additional thoughts

  1. I think we need a "invite cancelled" message, that is sent to the invitee immediately when the inviter cancels the invite, and is also sent in response to an invitee responding to a cancelled invite.
  2. I think we should add a (unique, unguessable) inviteId, so that we ensure that the sequence of messages is about the same invite, e.g. if the invitor cancels the first invite, deciding to re-invite with a different role, but for some reason the invitee does not learn of the cancellation, and thinks they are responding to the first invite.
  3. I think the switch to only send the sensitive project information upon accept, which allows an invite to be cancelled without sharing any sensitive information, is really valuable.

@EvanHahn
Copy link
Contributor

The invitee should not see anything in this path. I think this maybe meant to say "Inviter" in (2). However the inviter should see that the response is "already part of the project" as distinct from "accepted".

Good catch, that was a typo.

Just a note that we cannot pass an abort signal across the IPC channel.

I didn't know this—you probably saved me at least hour of confusion!

I think maybe unrecognized should be a rejection? I guess it doesn't matter too much, but that seems more like an "error" to me.

I agree that it doesn't matter much. When I implement this, I'll do whatever's easiest (unless I divine some strong reason why one option is preferable).

1. I think we need a "invite cancelled" message, that is sent to the invitee immediately when the inviter cancels the invite, and is also sent in response to an invitee responding to a cancelled invite.

👍️

2. I think we should add a (unique, unguessable) inviteId, so that we ensure that the sequence of messages is about the same invite, e.g. if the invitor cancels the first invite, deciding to re-invite with a different role, but for some reason the invitee does not learn of the cancellation, and thinks they are responding to the first invite.

👍️

I'm wondering if we should always have this, and show it to the device receiving the invite (e.g. a countdown timer on the invite). I think probably out-of-scope for this initial implementation (the UX part), but I think we should add TTL functionality.

[...]

I think we should auto-cancel invites when the app is closed / put into the background. I think we should also auto-cancel invites when the connection to the peer is lost.

I'll file two separate post-MVP issues for these...does that sound reasonable?

@EvanHahn
Copy link
Contributor

Started working on this. The overall happy path, I think:

  1. The inviter sends an Invite, which contains a random invite ID, the project's public ID, and some metadata.
  2. The invitee sends an InviteResponse, with a decision of ACCEPT, REJECT, or ALREADY.
  3. The inviter sends a ShareProjectDetails, which has the sensitive stuff.

I think these are the protos:

message Invite {
  bytes inviteId = 1;
  string projectPublicId = 2;
  string projectName = 3;
  optional string roleName = 4;
  optional string roleDescription = 5;
  optional string invitorName = 6;
}

message InviteCancel {
  bytes inviteId = 1;
}

message InviteResponse {
  enum Decision {
    REJECT = 0;
    ACCEPT = 1;
    ALREADY = 2;
  }
  bytes responseToInviteId = 1;
  Decision decision = 2;
}

message ShareProjectDetails {
  bytes inviteId = 1;
  bytes projectKey = 2;
  EncryptionKeys encryptionKeys = 3;
}

Here's the happy path:

sequenceDiagram
  Inviter->>Invitee: Invite
  Invitee->>Inviter: InviteResponse with ACCEPT
  Inviter->>Invitee: ShareProjectDetails
Loading

Still working on this so things might change!

@EvanHahn
Copy link
Contributor

Spoke with Erik and we decided to have an explicit "cancel invite" method instead of an abort controller. Should be conceptually the same, but a small deviation from the current public API.

@gmaclennan
Copy link
Member Author

Looks good! quick thoughts:

  • ShareProjectDetails -> JoinProjectDetails or ProjectJoinDetails? idk...
  • feels a bit weird to have inviteId as bytes and publicProjectId as a string, but might be easier I guess than converting projectPublicId back-and-forth to bytes/string.

@EvanHahn
Copy link
Contributor

#484 does most of the work here. Most significantly, invite cancellations are missing.

Here's what I think that entails...

This assumes that #484 doesn't change drastically.

We'd add a new RPC message:

message InviteCancel {
  bytes inviteId = 1;
}

We'd add a method on Peer for sending invite cancels:

/**
 * @param {string} deviceId
 * @param {InviteCancel} inviteCancel
 * @returns {Promise<void>}
 */
async sendInviteCancel(deviceId, inviteCancel) {
  await this.#waitForPendingConnections()
  const peer = await this.#getPeerByDeviceId(deviceId)
  peer.sendInviteCancel(inviteCancel)
}

We'd add the ability to send cancelations in LocalPeers (probably also requires updating MESSAGE_TYPES):

/** @param {InviteCancel} inviteCancel */
sendInviteCancel(inviteCancel) {
  this.#assertConnected()
  const buf = Buffer.from(InviteCancel.encode(inviteCancel).finish())
  const messageType = MESSAGE_TYPES.InviteCancel
  this.#channel.messages[messageType].send(buf)
  this.#log('sent invite cancel %h', inviteCancel.inviteId)
}

Then, in #handleMessage, we'd add a case for receiving invite cancelations (notice the new invite-cancel event):

case 'InviteCancel': {
  const inviteCancel = InviteCancel.decode(value)
  if (!isInviteCancelValid(inviteCancel)) {
    this.#l.log('Received invalid invite cancel')
    return
  }
  const peerId = keyToId(protomux.stream.remotePublicKey)
  this.emit('invite-cancel', peerId, inviteCancel)
  this.#l.log(
    'Invite cancel from %S for %h',
    peerId,
    inviteCancel.inviteId
  )
  break
}

// ...

/**
 * @param {InviteCancel} inviteCancel
 * @returns {boolean}
 */
function isInviteCancelValid(inviteCancel) {
  return isInviteIdValid(inviteCancel.inviteId)
}

Finally, we would need to:

  • Add an API for canceling invites
  • Cancel invites when the invitor's timeout is hit
  • Handle invite cancelations on the invitee's side

EvanHahn added a commit that referenced this issue Mar 13, 2024
See [#447][0] for details.

[0]: #447

Co-authored-by: Gregor MacLennan <gmaclennan@digital-democracy.org>
EvanHahn added a commit that referenced this issue Apr 10, 2024
This PR adds invite cancelations. Notable changes:

- `InviteCancel` RPC messages are now sent and received.

- `project.$member.invite()` takes an `AbortSignal`, used for
  cancelations. It replaces the timeout option.

- The `invite-removed` event now has a "reason" parameter which the
  frontend can use.

Closes #447.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mvp Requirement for MVP project invite
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants