Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add configuration parameter to allow redaction of content from push m… #2301

Merged
merged 4 commits into from Jun 24, 2017

Conversation

Projects
None yet
4 participants
Contributor

cjdelisle commented Jun 23, 2017

Currently almost all messages sent by anyone on any matrix homeserver are also sent to matrix.org in order to be forwarded then to google and/or apple to be used for push notifications.

This pull request introduces a configuration parameter which allows redaction of the content of these messages if the operator of the homeserver would rather additional privacy in the case that (for example) everyone in the room is on the same homeserver and they would rather not have the message content go to matrix.org and to google and apple.

Disclosure: I did not test this, I don't have a reasonable means to carry out tests on Synapse. Please let me know if I made some typo.

Related:

Can one of the admins verify this patch?

Can one of the admins verify this patch?

Can one of the admins verify this patch?

cjdelisle added some commits Jun 23, 2017

Owner

erikjohnston commented Jun 23, 2017

@matrixbot ok to test

Contributor

cjdelisle commented Jun 23, 2017 edited

Are these SyTest failures something I should be paying attention to ? I don't see how this code could interact with that. Are they known to be flakey tests ?

synapse/config/push.py
+ def default_config(self, config_dir_path, server_name, **kwargs):
+ return """
+ # Control how push messages are sent to google/apple to notifications.
+ # Normally every message is posted to a push server hosted by matrix.org
@ara4n

ara4n Jun 23, 2017

Owner

every pushed message

synapse/config/push.py
+ # which is registered with google and apple in order to allow push
+ # notifications to be sent to mobile devices.
+ # Setting redact_content to true will make the push messages contain no
+ # message content which will provide increased privacy.
@ara4n

ara4n Jun 23, 2017

Owner

It would be good to add that this is a temporary measure - in future we expect both GCM/FCM & APNS/PushKit to simply wake up the app in order to display the local notification.

@cjdelisle

cjdelisle Jun 23, 2017

Contributor

Proposed content:
https://cryptpad.fr/code/#/1/edit/EpHG3FrIQwq18pjm9wW3hA/S-VUuh3I-AOHwfmuZKfGgD5U/
I'm aiming for it to be meaningful in an "executive summary" way but still true.
With your approval I'll push another commit.

Owner

ara4n commented Jun 23, 2017

lgtm other than comments, but @erikjohnston should take a 2nd look

Owner

erikjohnston commented Jun 23, 2017

Yeah, those failures look unrelated. I think flakey tests have crept in. Again.

Owner

erikjohnston commented Jun 23, 2017

This LGTM, though I'm tempted to change the name to something other than "redacted", e.g. include_contents: false, as redacted has a special meaning already.

Owner

erikjohnston commented Jun 23, 2017

If you could just sign off too, that would be grand (https://github.com/matrix-org/synapse/blob/master/CONTRIBUTING.rst#sign-off). A simple comment here is fine.

Contributor

cjdelisle commented Jun 23, 2017

Signed-off-by: Caleb James DE LISLE <calebjamesdelisle@xwiki.com>
Contributor

cjdelisle commented Jun 24, 2017

As there was no feedback on the proposal made in the pad, I've pushed a different default content. It is longer but unfortunately I find the description to be a matter of: concise, correct and meaningful, pick 2.

Owner

erikjohnston commented Jun 24, 2017

LGTM, thanks for this!

glares at the flakey test

@erikjohnston erikjohnston merged commit ff13c5e into matrix-org:develop Jun 24, 2017

3 of 4 checks passed

Sytest Dendron (Merged PR) Build finished.
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Merged PR) Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment