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

Reject device display names that are too long #6882

Merged
merged 2 commits into from
Feb 10, 2020

Conversation

clokep
Copy link
Member

@clokep clokep commented Feb 10, 2020

This rejects device display name updates that are too long by raising a SynapseError from update device endpoint.

This defines a maximum length of 100 characters, but that is arbitrary. If someone has a better suggestion, please let me know!

@clokep clokep requested a review from a team February 10, 2020 12:58
@clokep
Copy link
Member Author

clokep commented Feb 10, 2020

I'm seeing a SyTest fail:

Test 312 /upgrade moves aliases to the new room

This doesn't feel related to this PR and seems to be failing on develop as well (see https://buildkite.com/matrix-dot-org/synapse/builds/7019).

@ara4n
Copy link
Member

ara4n commented Feb 10, 2020

matrix-org/sytest#801 should have fixed that

@clokep
Copy link
Member Author

clokep commented Feb 10, 2020

👍 I see that those were merged into develop on SyTest. Kicked off the builds again. Thanks @ara4n!

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm, though a regression test in tests.handlers.test_device would have won bonus points...

@clokep clokep force-pushed the clokep/limit-device-display-names branch from 95b37d3 to 520a579 Compare February 10, 2020 14:10
@clokep
Copy link
Member Author

clokep commented Feb 10, 2020

Thanks for the review @richvdh! I added a unit test, although now it seems more builds are failing for unrelated reasons.

@clokep clokep force-pushed the clokep/limit-device-display-names branch from 520a579 to 3e10e10 Compare February 10, 2020 16:48
@clokep clokep requested a review from richvdh February 10, 2020 17:06
@clokep
Copy link
Member Author

clokep commented Feb 10, 2020

@richvdh Would appreciate a quick check over the unit test to see if it is a reasonable test. Builds are green again too! Thanks!

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

yup looks great, thank you!

@clokep clokep merged commit a92e703 into develop Feb 10, 2020
@clokep clokep deleted the clokep/limit-device-display-names branch February 10, 2020 21:35
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit 'a92e703ab':
  Reject device display names that are too long (#6882)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants