SpectrumIM / spectrum2

Spectrum 2 IM transports
https://spectrum.im
409 stars 90 forks source link

Handling of friend request accept/deny #324

Closed himselfv closed 5 years ago

himselfv commented 5 years ago

I'm using spectrum2 (xmpp gateway)+purple_skypeweb and debugging a problem where friend request denials are ignored and the request simply reappears the next time I reconnect.

As far as I've managed to figure, the friend request accept/deny scheme in spectrum works like this:

  1. libpurple_backend registers purple callbacks accountRequestAuth and accountRemoveAuth. It stores auth requests with their accept_cb and deny_cb callbacks and sends <presence type=subscribe> on their behalf.

  2. The only way these callbacks are called are: accept_cb in any handleBuddyUpdatedRequest(), and deby_cb only in handleBuddyRemoved().

This raises a few questions:

  1. Does handleBuddyUpdatedRequest() really only get called after the subscription had been accepted? Can't it be called just because something about this buddy has changed, even though it's not yet accepted?

  2. Does handleBuddyRemoved() really happen when the frontend receives <presence type=unsubscribed> from the user?

The second thing doesn't seem to happen. User <presence> handling goes through UserManager::handlePresence(), UserManager::handleSubscription(), User::handleSubscription() to RosterManager::handleSubscription() where on Presence::Unsubscribed it simply sends back <presence type=unsubscribe> (why?) and sets buddy's subscription type to Buddy::Both (why? x2).

So... What I'm missing? Shouldn't there be a message to send from the frontend to the backend to tell it to accept or reject a subscription request on the backend network?

himselfv commented 5 years ago

Also, from what I can tell, each time a new contact requests subscription, XMPPRosterManager::sendRIE() runs sendBuddySubscribePresence() on every buddy in the server list, including the rejected ones, which linger probably since they got Buddy::Both back at rejection, though I'm not sure. Anyway, all rejected friend requests, ever, get redelivered!

vitalyster commented 5 years ago

Does handleBuddyUpdatedRequest() really only get called after the subscription had been accepted?

It's called on every presence change, not only subscription.

himselfv commented 5 years ago

For anyone having the same questions later, here's how it works:

Spectrum follows Pidgin model where

Spectrum keeps its own permanent list of buddies separate from the frontend (XMPP roster) if "remote roster" is disabled, and the backend (legacy network friends). Entries can only be:

It forwards all friend/unfriend requests to the backend but does not care about the results. Anyone you call a friend becomes a mutual friend in Spectrum's eyes. It produces the required XMPP notifications to keep your roster aligned with this model:

It does not remove the contact from your XMPP roster unless "remote roster" with necessary privileges is active (in which case it manages that part of your roster). You keep it unless you explicitly delete it.

The XMPP semantics are converged to:

  1. Any friendship action (add contact/ask for friend privileges/accept their request) => mutual friend => Backend :: Add Buddy => All friendship actions (add/accept/send request if needed).
  2. Any unfriending (remove contact/remove their friend privileges/reject their subscription/ask to remove your friend privileges) => no friend at all => Backend :: Remove Buddy => All unfriending actions (remove/reject)

So the backend is required to handle only two actions: remove buddy (handleBuddyRemoved) and add buddy (handleBuddyUpdatedRequest). The last one is called on any changes in a buddy's groups or a buddy's privacy lists. Since you cannot make these changes without having that buddy present, it's also used as an indicator of buddy addition.

vitalyster commented 5 years ago

It does not and cannot remove the contact from your XMPP roster

This is not true when remote_roster/privileges are active

himselfv commented 5 years ago

Ok, fixed.

himselfv commented 5 years ago

(The lingering rejected subscriptions issue should be fixed by https://github.com/SpectrumIM/spectrum2/pull/327 )