Remove code invalidated by deprecated config flag 'trust_identity_servers_for_password_resets'

[H-Shay]

I believe this code can be removed. The background update relies on checking self.config.registration.trusted_third_party_id_servers, which has been deprecated for two years and now removed, so the code will no longer do anything. I dug a little deeper and it appears this update was added when we were beginning to track which identity server a user's 3pid was bound on, to help make sure that any binds that occurred before we began tracking were copied over onto the new table along with the identity server that was assumed to make the bind.
This code was not changed when the concept of delegating to trusted 3pid servers was deprecated. So I suspect that it has not been run in awhile, and should be safe to remove.

[reivilibre]

(this appears to fall off the back of #11333)

[reivilibre]

Doing a ripgrep:

CHANGES.md
3803:- Fixed backwards compatibility logic of the first value of `trusted_third_party_id_servers` being used for `account_threepid_delegates.email`, which occurs when the former, deprecated option is set and the latter is not. ([\#7316](https://github.com/matrix-org/synapse/issues/7316))
4988:- Deprecate the `trusted_third_party_id_servers` option. ([\#5875](https://github.com/matrix-org/synapse/issues/5875))

These are OK for historical value.

docs/upgrade.md
1004:`trusted_third_party_id_servers` will be used as the
1261:    `trusted_third_party_id_servers`, you should remove it. It was added

Same here, though I think in retrospect you will want to add an entry to the upgrade documentation about it being removed.

synapse/storage/databases/main/registration.py
1816:        id_servers = set(self.config.registration.trusted_third_party_id_servers)

This appears to be what you're dealing with now.

tests/utils.py
122:        "trusted_third_party_id_servers": [],

Haven't looked at what this is, but chances are it could do with the test being removed if it's obsolete, or the test having that line of config removed.

docker/conf/homeserver.yaml
156:trusted_third_party_id_servers:

I think this means the Docker configuration needs to be updated.

[reivilibre]

We need to decide / communicate clearly that we're not going to support this migration anymore.
If I understand correctly, admins of ancient Synapses would have to upgrade to an earlier version of Synapse first and then to the latest in order to migrate their users across (if they rely on the trusted identity servers).

@richvdh feels like a good person to pick on: is it OK that we won't migrate users properly for homeservers upgrading from ancient Synapse to modern Synapse in one go?

(At the least, I think we need mention of this in the upgrade notes.)

[reivilibre]

If we're happy to remove this update, we should probably make it a no-op so that when a homeserver upgrades from an ancient version to today's, it doesn't fail on this non-existent job. That's because

synapse/synapse/storage/schema/main/delta/53/user_threepid_id.sql

Lines 28 to 29 in 7ffddd8

	INSERT INTO background_updates (update_name, progress_json) VALUES
	('user_threepids_grandfather', '{}');

is a schema delta which inserts an entry into the background job queue, but you've just removed the job to which it refers.

[clokep]

If it is pre 1.0.0 it might be OK to just remove?

[richvdh]

given register_noop_background_update exists to make the no-op a oneliner, I'd vote to do that.

[H-Shay]

Purely out of curiosity: in general, why use no-op background updates vs adding another schema delta to drop an irrelevant background update job?

[richvdh]

hrm, that's a great question!

I think either solution would be fine. But since we've already got no-ops, there's something to be said for consistency?

[reivilibre]

That's a valid point though; I didn't even think about using deletes in schema deltas for removed background jobs (rather than a few cases where we've deleted & re-inserted to prevent duplicates).
I personally don't like the no-ops as it feels like a monotonically increasing amount of cruft that we carry around in our storage code until the end of time, whereas at least the deltas feel more self-contained.
That said, hard to argue with consistency.

[H-Shay]

Great thanks for answering, all! I've changed it to a no-op, I agree that it is hard to argue with consistency.

[richvdh]

@richvdh feels like a good person to pick on: is it OK that we won't migrate users properly for homeservers upgrading from ancient Synapse to modern Synapse in one go?

Let me try to fully understand the ramifications:

• any user who registered a 3pid against a server running 0.99.3 or earlier (pre-Implement MSC1915 - 3PID unbind APIs #4982), will have had that 3pid implicitly bound on one or more of identity servers in trusted_third_party_id_servers.
• If that server never gets upgraded to anything in the range 0.99.4 - 1.47.1, but rather goes straight to 1.48 or so...
• and then the user chooses to unbind their 3pid...
• that 3pid will not be correctly unbound on the identity server.

Is that correct? If so, it seems such an unlikely edge-case that I'm struggling to care.

I'm also of the opinion that trying to explain it in the upgrade notes is far more likely to be confusing than helpful. Any server that is still running 0.99.3 has far bigger problems than whether their users' 3pids get correctly unbound.

[reivilibre]

I think I may have misunderstood the consequences; if it's just what you're saying then fine (and indeed, quite the edge case).

[reivilibre]

LGTM