Add module API callbacks for adding and deleting local 3PID associations

[anoadragon453]

This PR adds a couple Synapse module API callbacks for adding and removing a third party identifier from your account: on_add_user_third_party_identifier and on_remove_user_third_party_identifier.

These callbacks are fired when a client add/removes a third-party ID to your account, not when a client bind/unbinds a third-party ID from an identity server. I wrote a little bit about how this can be confusing in #14955.

The on_add_user_third_party_identifier replaces and deprecates the on_threepid_bind callback. The rationale is described in the upgrade notes.

Recommended to review commit-by-commit. Supersedes #13227.

[anoadragon453]

I'm curious on whether people think this level of caution is necessary?

[reivilibre]

I find it unintuitive that we trigger the event prior to performing the action.
The only reason I'd expect this would be if we were offering the modules the ability to cancel the event — however this does not seem to be the case.

As it stands, I'd likely prefer to trigger the events after the action has been performed instead, to avoid this 'takeback' situation.

If we are intending to let modules block the association, then we should probably support that here and now.
In my experience with other systems, though, there are usually multiple phases that an event listener can register themselves to an event for: at a minimum, before and after. (with the event only being cancellable before). Maybe we make that explicit with two different API callbacks (or two different phases) — it sounds awkward but then having no way to subscribe to events that are definitely going to happen is also awkward.

[anoadragon453]

I did intend to leave the door open to allowing modules to block the action, and I'm not opposed to adding it now. I only avoided doing so to save time, and don't consider it a breaking change to do so.

Having module callbacks that allow for both before and after an action sound reasonable. In the use case this is intended for (automatically binding 3pids to a company's Sydent instance), subscribing to a post-action callback does sound best (and delivers the guarantees we want without having to do an extra callback in case of a database exception. The module would just accept that if they were going for the pre-action callback.

I think I'm going to update this PR to only include the post-action callback (and leave the name as-is, as it sounds post-action-y already...). A future PR is welcome to add a can_add_user_third_party_identifier or somesuch that allows for blocking.

[anoadragon453]

Done in a52fbcc.

[reivilibre]

I won't necessarily block this PR, but I would have to point out that I'm a bit reluctant to only support these callbacks for pre-add/pre-remove.

Some thoughts:

• the name should probably reflect that they run before — e.g. add the word before or pre to the names. I don't think this catch should be buried in the documentation.
• I would probably prefer to support two phases here.
• The 'on error, emit events to "undo" the prior events' seems fragile...
  • modules have no way of knowing when something is 'confirmed'
  • it's not really obvious from the docs. I don't think it's intuitive, either.
  • if we add support to these callbacks for blocking an action, what happens if a module blocks the take-back? E.g. if we fail to add a 3pid, trigger the '3pid removed' event and get told to block that, we actually can't — we never added the 3pid in the first place (and if we tried again, presumably the same error would happen).
    • If we tell module A that we are going to add a 3pid and module B blocks it, do we then need to go and tell module A we're removing it? (and pathologically: what do we do if they block each other? :p)
  • There's no testing for this case, which is definitely rectifiable but it feels like an awkward behaviour to test from a module author's point of view too.
  • Generally, conflating 'ask' and 'tell' in one callback seems to be perilous.

[reivilibre]

I find it unintuitive that we trigger the event prior to performing the action.
The only reason I'd expect this would be if we were offering the modules the ability to cancel the event — however this does not seem to be the case.

As it stands, I'd likely prefer to trigger the events after the action has been performed instead, to avoid this 'takeback' situation.

If we are intending to let modules block the association, then we should probably support that here and now.
In my experience with other systems, though, there are usually multiple phases that an event listener can register themselves to an event for: at a minimum, before and after. (with the event only being cancellable before). Maybe we make that explicit with two different API callbacks (or two different phases) — it sounds awkward but then having no way to subscribe to events that are definitely going to happen is also awkward.

[anoadragon453]

CI failure is a Complement workers failure of TestInboundFederationRejectsEventsWithRejectedAuthEvents. I'm considering this a flake due to:

• This change being unrelated to that test.
• The test passing on Complement monolith.

Merging manually.