Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Remove code invalidated by deprecated config flag 'trust_identity_servers_for_password_resets' #11395

Merged
merged 8 commits into from
Nov 23, 2021
Merged

Remove code invalidated by deprecated config flag 'trust_identity_servers_for_password_resets' #11395

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9279383
remove background update code related to deprecated config flag
H-Shay Nov 18, 2021
20cbbd9
changelog entry
H-Shay Nov 18, 2021
cb5e336
update changelog
H-Shay Nov 19, 2021
4360c8f
Delete 11394.removal
reivilibre Nov 19, 2021
f32984a
add no-op background update and change newfragment so it will be cons…
H-Shay Nov 19, 2021
10f31a1
remove unused code
H-Shay Nov 22, 2021
1a70a23
merge:
H-Shay Nov 22, 2021
994d80d
Remove code associated with deprecated flag from legacy docker dynami…
H-Shay Nov 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/11395.removal
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove code related to deprecated 'trust_identity_servers_for_password_resets' config flag.
33 changes: 0 additions & 33 deletions synapse/storage/databases/main/registration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1727,10 +1727,6 @@ def __init__(
"refresh_tokens_device_index"
)

self.db_pool.updates.register_background_update_handler(
"user_threepids_grandfather", self._bg_user_threepids_grandfather
)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@reivilibre reivilibre Nov 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


self.db_pool.updates.register_background_update_handler(
"users_set_deactivated_flag", self._background_update_set_deactivated_flag
)
Expand Down Expand Up @@ -1805,35 +1801,6 @@ def _background_update_set_deactivated_flag_txn(txn):

return nb_processed

async def _bg_user_threepids_grandfather(self, progress, batch_size):
"""We now track which identity servers a user binds their 3PID to, so
we need to handle the case of existing bindings where we didn't track
this.

We do this by grandfathering in existing user threepids assuming that
they used one of the server configured trusted identity servers.
"""
id_servers = set(self.config.registration.trusted_third_party_id_servers)

def _bg_user_threepids_grandfather_txn(txn):
sql = """
INSERT INTO user_threepid_id_server
(user_id, medium, address, id_server)
SELECT user_id, medium, address, ?
FROM user_threepids
"""

txn.execute_batch(sql, [(id_server,) for id_server in id_servers])

if id_servers:
await self.db_pool.runInteraction(
"_bg_user_threepids_grandfather", _bg_user_threepids_grandfather_txn
)

await self.db_pool.updates._end_background_update("user_threepids_grandfather")

return 1

async def set_user_deactivated_status(
self, user_id: str, deactivated: bool
) -> None:
Expand Down