fix: Avoid a M_FORBIDDEN response when a user tries to erase their account and profile updates are disabled#19398
Conversation
… profile data is disabled in configuration Specifically, to check if this is a deactivation and that it is being set to `""` for an GDPR level erasure
|
|
| def test_set_my_name_if_disabled(self) -> None: | ||
| self.hs.config.registration.enable_set_displayname = False | ||
|
|
||
| @override_config({"enable_set_displayname": False}) | ||
| def test_set_displayname_if_disabled(self) -> None: |
There was a problem hiding this comment.
This(and the similar in this file) was a drive-by fix to not accidentally change the state of the homeserver configuration the other tests in this series may depend on. If the order of the tests in this series was reversed, at least one of the tests would not pass. No changes here were relevant for to the purpose of this work
There was a problem hiding this comment.
I can remove this if it is an obstacle
| if ( | ||
| not by_admin and not self.hs.config.registration.enable_set_displayname | ||
| ) and not (deactivation and new_displayname == ""): |
There was a problem hiding this comment.
new_displayname(and similarily below for new_avatar_url) an explicit "" is checked for, as that is what the DeactivateAccountHandler.deactivate_account() uses
| self.get_success( | ||
| profile_handler.set_displayname( | ||
| user_id, create_requester(user_id), "Kermit the Frog" | ||
| ) | ||
| ) | ||
| self.get_success( | ||
| profile_handler.set_avatar_url( | ||
| user_id, create_requester(user_id), "http://test/Kermit.jpg" | ||
| ) | ||
| ) |
There was a problem hiding this comment.
I chose to use profile data for this erasure test as the "smoke signal" that it was functioning, since it was relevant for the fix being included. There are rather large area of other things that could be checked for, and I did not see any explicit testing for comprehensive deletion of data
anoadragon453
left a comment
anoadragon453
left a comment
There was a problem hiding this comment.
Hi @jason-famedly, as I wrote on #19390 (review), as far as the Element Backend team is aware, Famedly has not yet signed the CCLA. However this is apparently currently in progress.
Holding off on review until that's resolved. Regardless, thank you for submitting your work upstream!
3 participants
Currently synapse returns
M_FORBIDDENwhen trying to use the account deactivation API, if the server admin disabled displayname changes. This is undesirable, since it prevents GDPR erasure without admin interaction. The admin API seems to work fine though. This also only seems to affect the deactivate API, when the erase flag is true.Relevant endpoint: https://spec.matrix.org/latest/client-server-api/#post_matrixclientv3accountdeactivate
This change only adds an additional check on the condition prior to the forbidden response, to verify that the request is a deactivation and that it is clearing the relevant profile data
There did not seem to be a basic test for this endpoint that checks the
eraseusage, so that was added as well as checking the above mentioned behavior.Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.