Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
Jul 1, 2022

Conversation

N-Pex
Copy link
Contributor

@N-Pex N-Pex commented Jun 28, 2022

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

@N-Pex N-Pex requested a review from a team as a code owner June 28, 2022 14:30
Signed-off-by: N-Pex <npexdev@gmail.com>
@N-Pex
Copy link
Contributor Author

N-Pex commented Jun 28, 2022

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 squahtx linked an issue Jun 28, 2022 that may be closed by this pull request
@squahtx
Copy link
Contributor

squahtx commented Jun 28, 2022

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
Copy link
Contributor Author

N-Pex commented Jun 28, 2022

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

@babolivier babolivier self-assigned this Jun 29, 2022
@babolivier
Copy link
Contributor

Related: #308

@babolivier babolivier closed this Jun 29, 2022
@babolivier
Copy link
Contributor

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

@babolivier babolivier reopened this Jun 29, 2022
@babolivier
Copy link
Contributor

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, #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 #308? (if not I'll have a look next week)

@babolivier babolivier closed this Jun 29, 2022
@babolivier
Copy link
Contributor

Gosh darn it Github.

@babolivier babolivier reopened this Jun 29, 2022
@N-Pex
Copy link
Contributor Author

N-Pex commented Jun 30, 2022

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
Copy link
Contributor Author

N-Pex commented Jun 30, 2022

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

@babolivier babolivier requested review from babolivier and removed request for a team June 30, 2022 12:55
@N-Pex
Copy link
Contributor Author

N-Pex commented Jun 30, 2022

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

Copy link
Contributor

@babolivier babolivier left a comment

Choose a reason for hiding this comment

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

Thanks for your work! I've noted a few things that I'd like to see improved. I'd also like to see a test for this (you can use the tests here for inspiration).

Feel free to drop by #sygnal:matrix.org if you need help with this too! :)

Comment on lines 197 to 201
push_type = self.get_config("push_type", str)
if not push_type:
self.push_type = None
else:
self.push_type = ApnsPushTypeHeader(push_type)
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand it, aioapns expects an aioapns.PushType (which is an Enum) here. So I think something we should do is: first check whether the value of push_type is a member of this enum, and then use the matching value within the enum for self.push_type.

I think this is best done by declaring a constant which is a dictionary between a string value and its matching enum member (because annoyingly there isn't a way to list all members of an enum in Python without doing some voodoo with its internals). So something like this:

APNS_PUSH_TYPES = {
    "alert": PushType.ALERT,
    "background": PushType.BACKGROUND,
    ...
}

(you can see the list of members of PushType here: https://github.com/Fatal1ty/aioapns/blob/26c2cdc3aacb529f4de1065bf3d1cff2137f13bc/aioapns/common.py#L9-L15 - unfortunately aioapns doesn't seem to have docs so we need to refer to the code)

Then you can do something like:

push_type = self.get_config("push_type", str)
if not push_type:
    self.push_type = None
else:
    if push_type not in APNS_PUSH_TYPES.keys():
        raise PushkinSetupException(f"Invalid value for push_type: {push_type}")

    self.push_type = APNS_PUSH_TYPES[push_type]

Which a) ensures we use a supported value, b) removes the need for an internal class in favour of one provided by aioapns.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
changelog.d/309.feature Outdated Show resolved Hide resolved
N-Pex and others added 4 commits July 1, 2022 12:23
Co-authored-by: Brendan Abolivier <github@brendanabolivier.com>
Co-authored-by: Brendan Abolivier <github@brendanabolivier.com>
Co-authored-by: Brendan Abolivier <github@brendanabolivier.com>
@N-Pex
Copy link
Contributor Author

N-Pex commented Jul 1, 2022

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

Copy link
Contributor

@babolivier babolivier left a comment

Choose a reason for hiding this comment

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

Fantastic, thanks a lot for your contribution! :)

@babolivier babolivier merged commit 2a33681 into matrix-org:main Jul 1, 2022
@N-Pex
Copy link
Contributor Author

N-Pex commented Jul 1, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for on device notification filtering
3 participants