matrix-org / sygnal

Sygnal: reference Push Gateway for Matrix
Apache License 2.0
167 stars 148 forks source link

Add support for setting the apns-push-type header via device data #309

Closed N-Pex closed 2 years ago

N-Pex commented 2 years ago

Signed-off-by: N-Pex npexdev@gmail.com

N-Pex commented 2 years ago

In order to be able to support silencing push notifications client side, the apns-push-type header must be sent to APNS (please see note at the bottom of this page: https://developer.apple.com/documentation/bundleresources/entitlements/com_apple_developer_usernotifications_filtering)

This PR adds support for setting the header, via device data sent to Synapse in "/pushers/set" call (by putting the header field in there like this:)

...

NSDictionary *pushData = @{
        @"url": self.pushGatewayURL,
        @"format": @"event_id_only",
        @"default_payload": @{@"aps": @{@"mutable-content": @(1), @"alert": @{@"loc-key": locKey, @"loc-args": @[]}}},
        @"apns_push_type": @"alert"
    };

...

squahtx commented 2 years ago

There was a previous attempt at this feature in #305, which hardcoded the push type. We wanted to make the push type a per-pusher config option instead.

N-Pex commented 2 years ago

Oh, interesting. We came across this requirement quite a while back. I haven't come around to implementing it until now.

babolivier commented 2 years ago

Related: https://github.com/matrix-org/sygnal/issues/308

babolivier commented 2 years ago

Github why did you close this PR, I did not click that button.

babolivier commented 2 years ago

I don't think this is how we want to fix this issue. The device data is defined in the request to create the pusher, which format is defined in the spec: https://spec.matrix.org/latest/client-server-api/#post_matrixclientv3pushersset

I'm uncomfortable relying on a key that isn't described in the specification, in a structure that is. So if we were to implement it with device data we would need a companion MSC (Matrix Spec Change, a proposal to update the Matrix specification) to go with it. And since it's very specific to one platform I'm not sure how likely it would be to be accepted.

On the other hand, https://github.com/matrix-org/sygnal/issues/308 describes an alternative solution using the app configuration, which gets you basically the same result with a bit less trouble, and also has the added bonus of making the fix work on already existing pushers (whereas if we were using device data, existing clients would need to re-register their pushers). This is definitely a solution we'd be happier with.

@N-Pex would you be happy to change your PR so that it instead implements the solution described in https://github.com/matrix-org/sygnal/issues/308? (if not I'll have a look next week)

babolivier commented 2 years ago

Gosh darn it Github.

N-Pex commented 2 years ago

Thanks @babolivier ! Yeah, I think that makes a lot of sense! I'll have a stab at it, but if I fail (given my almost total lack of Python skills, haha) I might need you to chime in. I'll get back to you soon here =)

N-Pex commented 2 years ago

Ok, changes pushed! Not sure what to call the option, but I thought "push_type" would be proper?

N-Pex commented 2 years ago

Sorry about that, I guess I should lint this all, but my tooling is not properly setup.

N-Pex commented 2 years ago

Ok, I think I got most of it done (well, you wrote almost all the code, haha) =)

N-Pex commented 2 years ago

Thanks for the quick responses and the guidance, glad we could get this in!