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

Fix power levels being incorrectly set in old and new rooms after a room upgrade #6633

Merged
merged 5 commits into from Jan 6, 2020

Conversation

@anoadragon453
Copy link
Member

anoadragon453 commented Jan 5, 2020

Fixes #6632

Sytests: matrix-org/sytest#777

Fixes a bug introduced in #6237 where in which a moderator in a room upgrades the room, the following oddities occur:

  • The Mod in the old room gets upgraded to an Admin (but only from the perspective of the server upgrading the room).
  • The Mod in the new room isn't being downgraded back to a Mod again, but is left as an Admin. (They need to be an Admin initially to send required room state, but should be downgraded back to their original power level immediately afterwards).

Turns out the problem was due to modifying initial_state, which was a dictionary representing the state of the old room. This modified object continues to persist however, and later on in _update_upgraded_room_pls, when we attempt to send a power level event to mute users in the old room, we end up grabbing initial_state again, which contains the Mod as an Admin instead. At this point, the Mod is set to an Admin in the old room.

Additionally, the way we downgrade the Mod back from an Admin to a Mod in the new room, is by simply applying the old room's power levels once again. But, since we're using the modified power levels object, which had the Mod as an Admin, the Mod remains an Admin in the new room.

Performing a deepcopy instead solves both of these problems.

@anoadragon453 anoadragon453 requested a review from matrix-org/synapse-core Jan 5, 2020
@anoadragon453 anoadragon453 self-assigned this Jan 5, 2020
power_levels["users"][requester.user.to_string()] = needed_power_level
# Perform a deepcopy of the dict in order to not modify initial_state, as its
# contents are preserved as the state for the old room later on
initial_state = copy.deepcopy(initial_state)

This comment has been minimized.

Copy link
@richvdh

richvdh Jan 5, 2020

Member

Do we really need to copy the entire state rather than just the power levels event?

(it might also be preferable just to copy the bits we know we want from the PL body)

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Jan 5, 2020

Author Member

Changed to only deepcopy the power levels state. Let me know what you think.

@babolivier

This comment has been minimized.

Copy link
Member

babolivier commented Jan 5, 2020

Can we also have a test case for this? This way we ensure the fix works as intended and doesn't break in the future.

@Mikaela

This comment has been minimized.

Copy link

Mikaela commented Jan 5, 2020

I am not sure this is a good idea, because this is currently the only method to get control of IRC rooms that are managed entirely by e.g. matrix-appservice-irc, matrix-org/matrix-appservice-irc#408.

Also if PL100 was required for upgrading rooms, only the appservice would be able to do that and I am not certain it's a good idea.

@tulir

This comment has been minimized.

Copy link
Contributor

tulir commented Jan 5, 2020

@Mikaela you can still manually upgrade rooms and set whatever power levels you like when creating the new room: https://gist.github.com/turt2live/a99c8e794d6115d4ddfaadb72aabf063

@anoadragon453

This comment has been minimized.

Copy link
Member Author

anoadragon453 commented Jan 5, 2020

Can we also have a test case for this? This way we ensure the fix works as intended and doesn't break in the future.

Added tests.

@richvdh
richvdh approved these changes Jan 6, 2020
Copy link
Member

richvdh left a comment

lgtm otherwise

power_levels["users"][requester.user.to_string()] = needed_power_level
# Perform a deepcopy in order to not modify the original power levels in a
# room, as its contents are preserved as the state for the old room later on
existing_power_levels = initial_state[(EventTypes.PowerLevels, "")]

This comment has been minimized.

Copy link
@richvdh

richvdh Jan 6, 2020

Member

we've already got this, in power_levels (line 359)

@richvdh richvdh merged commit 01c3c6c into develop Jan 6, 2020
22 checks passed
22 checks passed
buildkite/synapse Build #6326 passed (16 minutes, 48 seconds)
Details
buildkite/synapse/check-sample-config Passed (1 minute, 32 seconds)
Details
buildkite/synapse/check-style Passed (1 minute, 53 seconds)
Details
buildkite/synapse/isort Passed (17 seconds)
Details
buildkite/synapse/mypy Passed (55 seconds)
Details
buildkite/synapse/newspaper-newsfile Passed (15 seconds)
Details
buildkite/synapse/packaging Passed (19 seconds)
Details
buildkite/synapse/pipeline Passed (3 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-5 Passed (11 minutes)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite Passed (6 minutes, 52 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite-slash-old-deps Passed (8 minutes, 20 seconds)
Details
buildkite/synapse/python-3-dot-6-slash-sqlite Passed (7 minutes, 3 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-11 Passed (9 minutes, 22 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-9-dot-5 Passed (10 minutes, 55 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-sqlite Passed (6 minutes, 52 seconds)
Details
buildkite/synapse/synapse-port-db-slash-python-3-dot-5-slash-postgres-9-dot-5 Passed (1 minute, 35 seconds)
Details
buildkite/synapse/synapse-port-db-slash-python-3-dot-7-slash-postgres-11 Passed (1 minute, 37 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-monolith Passed (11 minutes, 21 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-workers Passed (14 minutes, 42 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-sqlite-slash-monolith Passed (9 minutes, 14 seconds)
Details
buildkite/synapse/sytest-python-3-dot-7-slash-postgres-11-slash-monolith Passed (9 minutes, 56 seconds)
Details
buildkite/synapse/sytest-python-3-dot-7-slash-postgres-11-slash-workers Passed (10 minutes, 38 seconds)
Details
@anoadragon453 anoadragon453 deleted the anoa/fix_room_upgrade_pls branch Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.