-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix hiding devices names over federation #10015
Fix hiding devices names over federation #10015
Conversation
Signed-off-by: Aaron Raimist <aaron@raim.ist>
Signed-off-by: Aaron Raimist <aaron@raim.ist>
Signed-off-by: Aaron Raimist <aaron@raim.ist>
Did you want review of this, or is it still a WIP? |
It is still WIP but I think I'm going to need some help to finish it. I tested this patch locally using the demo federation script and it seemed to work. But then I installed it on my server and device names were still being shared so this PR must not have fixed it. I haven't been able to figure out how the device names are being shared. I asked in |
Shout if/when this is ready for review 🙂 |
Does the PR work now? I'm missing some context here. |
@squahtx context is that I still have not had time to take another look and this PR still probably does not fix 100% of the places where devices names can leak. However this PR is better than nothing since it definitely leaks without this PR and this feature is being turned on by default in 1.59.0rc1. Unless someone else is able to put in some time to figure out all of the places where device names can leak then I would say just merge this and try to get it in to v1.59.0 so the feature at the top of the release notes is at least a bit more reliable. Otherwise if this can't get in to v1.59.0, I would recommend postponing hiding device names over federation by default until someone has time to investigate it further. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation! I agree that it makes sense to land this fix as-is. I've filed #12750 to track the leakiness.
As part of future work, we could look into automated testing for the option. I also wonder if it would be more appropriate for the logic to sit in a higher layer than the data store.
LGTM
CI seems to be stuck from a year ago. Could you merge in the latest develop or a dummy commit to give it a kick? |
Signed-off-by: Aaron Raimist <aaron@raim.ist>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging in the latest develop branch has revealed a handful of test failures unfortunately.
We'll want to investigate and fix them before merging.
There is one failing sytest |
Hopefully gets matrix-org/synapse#10015 over the line.
I think this should pass (modulo complement flakes) with matrix-org/sytest#1307. |
Note that this does not look to solve #13114 (tested manually). |
* Allow HSes to omit device display names Hopefully gets matrix-org/synapse#10015 over the line. * Fix perl syntax?!?!
I'll update the changelog to reflect this. |
Follow up to #9945. Turns out, while that PR appears to work most of the time there is one more spot where device names can be sent over federation.
I also noticed all these blank
![](https://user-images.githubusercontent.com/5855073/118766850-9b6e9500-b842-11eb-97b0-76837b89b414.png)
org.matrix.opentracing_context
lines while I was debugging why my first PR wasn't working. I assume those don't need to be there unless opentracing is enabled so I also prevented those from being sent if they were blank.Pull Request Checklist
EventStore
toEventWorkerStore
.".code blocks
.