ChatSecure / ChatSecure-iOS

ChatSecure is a free and open source encrypted chat client for iOS that supports OTR and OMEMO encryption over XMPP.
https://chatsecure.org
Other
3.13k stars 1.02k forks source link

Push Notifications are broken.. sometimes. #637

Closed lazyadmin111 closed 7 years ago

lazyadmin111 commented 7 years ago

Dear Developers,

I installed your nice app at some friends, and I am using it with a own xmpp server (prosody 0.10) + Push-module (cloud_notify). Some of my friends a while back also got some push notifications, so it seemed to work somehow a while ago. But now there aren't any push notifications any more. I wonder now if the push server is down (as my prosody logs seem to indicate, as they say something of "not found"), or if is rather my fault, e.g. because he the push app server doesn't like my self-signed certificate. But if the later, why did it work then sometimes a while ago?

Any helpful comment highly appreciated :-)

greets

davepb16 commented 7 years ago

@silberzwiebel

I'm happy to do that. Busy this week but will hopefully have some time next week to do it.

We should open a separate thread for it so as to not fill this one up.

Also, waiting for @chrisballinger to release a new version of Chatsecure-ios that will give information on the XMPP server capabilities and PUSH notification issues.

My email is in my profile if you need to contact me.

tmolitor-stud-tu commented 7 years ago

@davepb16 The problem with prosody 0.10 is this: #712 Current versions of Chatsecure don't provide an id on iqs and prosody 0.10 is stricter in not allowing such iqs.

tmolitor-stud-tu commented 7 years ago

@chrisballinger I solved the id problem (see #712) by manually reissuing the enable stanza via sendxmpp commandline client (and specifying an id this time). Later I also disabled the id check in prosody entirely.

Now my server sends out push messages as expected but nothing happens on the client :(

Interestingly I can send push messages via https://push.chatsecure.org/api/v1/messages/ using the token registered with my xmpp server and those push messages are displayed and can be used to wake up chatsecure by opening them (this doesn't happen automatically).

But pushes send from my xmpp server don't seem to make it to the device. This is an example of the push stanza (anonymized, if you need the full stanza just ask):

<iq type="set" to="pubsub.chatsecure.org" from="user@example.org" id="pubsub.chatsecure.org&lt;DEADBEEF-DEAD-BEEF-DEAD-BEEFDEADBEEF">
    <pubsub xmlns="http://jabber.org/protocol/pubsub">
    <publish node="DEADBEEF-DEAD-BEEF-DEAD-BEEFDEADBEEF">
        <item>
            <notification xmlns="urn:xmpp:push:0">
                <x xmlns="jabber:x:data" type="form">
                    <field type="hidden" var="FORM_TYPE">
                        <value>urn:xmpp:push:summary</value>
                    </field>
                    <field type="text-single" var="message-count">
                        <value>1</value>
                    </field>
                    <field type="text-single" var="pending-subscription-count"/>
                    <field type="jid-single" var="last-message-sender"/>
                    <field type="text-single" var="last-message-body"/>
                </x>
            </notification>
        </item>
    </publish>
    <publish-options>
        <x xmlns="jabber:x:data">
            <field var="FORM_TYPE">
                <value>http://jabber.org/protocol/pubsub#publish-options</value>
            </field>
            <field var="token">
                <value>anonymized_token_value</value>
            </field>
            <field var="endpoint">
                <value>https://push.chatsecure.org/api/v1/messages/</value>
            </field>
        </x>
    </publish-options>
</pubsub>
</iq>

Your sever's answer: <iq from="pubsub.chatsecure.org" to="user@example.org" id="pubsub.chatsecure.org&lt;DEADBEEF-DEAD-BEEF-DEAD-BEEFDEADBEEF" type="result"/>

If you need some more info, I'd be glad to help.

tmolitor-stud-tu commented 7 years ago

@chrisballinger @iNPUTmice Wouldn't it be possible to extend XEP-0357 to allow the server to specify a priority for every push message? This way the server could trigger low priority (silent) pushes for typing notifications and other thanzas containing no message body and high priority pushes for message stanzas having a body.

Wouldn't that solve most of the issues described here: https://github.com/ChatSecure/ChatSecure-iOS/issues/637#issuecomment-279450445

tmolitor-stud-tu commented 7 years ago

@chrisballinger Could you contact me via xmpp at tmolitor@datenknoten.me to discuss those XEP changes further? I already had a longer discussion with Holger about this issue and we had several ideas to improve the situation that I want to discuss with you. I don't have much experience regarding iOS and your experience here is invaluable to sort out those ideas that actually could work.

chrisballinger commented 7 years ago

@tmolitor-stud-tu We already forward pushes for typing notifications. All notifications are silent and just trigger the app to wake up for ~30 seconds, any notifications you see are generated locally on the device. Btw, please don't send last-message-sender and last-message-body because we don't want that information and purposefully discard these values

I have a new beta that will be sent out soon that will fix the iq issue, among other things. Would you mind sending me an email so I can invite you to the next TestFlight release?

iNPUTmice commented 7 years ago

@tmolitor-stud-tu Why send a push notification at all for unimportant things like Chat States? Anything that will trigger a notification on the user's phone should be pushed with the highest possible priority and anything else just shouldn't be pushed at all. It should probably be tied to CSI. If CSI thinks it's worthy to be send, send a push. If not don't. I can't think of an example where I would prefer a low priority push over no push at all. But maybe I'm missing something?

tmolitor-stud-tu commented 7 years ago

@iNPUTmice I think you are missing iq stanzas. Those should not be displayed as notification to the user but wake up the device so it can respond to them.

tmolitor-stud-tu commented 7 years ago

@chrisballinger do you need my private email address or my apple id to do this?

weiss commented 7 years ago

anything else just shouldn't be pushed at all.

If we want to maintain a server-side session as long as the client is reachable we must be able to ping the client (e.g. after server restarts or after some idle timeout). The server must also be able to flush the CSI queue. If there's no way to make "silent notifications" reliable on iOS, I think we must give up the idea of keeping an online session for iOS clients.

weiss commented 7 years ago

All notifications are silent and just trigger the app to wake up for ~30 seconds

I thought the point of this issue was that silent APNS notifications don't seem to be reliable? Is your plan to stick to them nevertheless?

chrisballinger commented 7 years ago

@iNPUTmice Sending a notification for chat state change helps because it has the side effect of waking up the other client before you've finished writing your message. There is no concept of priority for our pushes in the current design, nor can there be. They are all "low priority" background pushes without any content that simply wake up the device for ~30 seconds. We will never include plaintext content in our pushes.

Your server can choose to not send pushes for unimportant events like chat states, but the usability is slightly worse because of the slight lag between push -> client awake.

@tmolitor-stud-tu Our pubsub server will trigger a push whenever you send it a valid push iq. It's up to your server to decide when to trigger these.

Any email will do, you connect your Apple ID later during the signup process.

tmolitor-stud-tu commented 7 years ago

@iNPUTmice @weiss Wouldn't be muc invites also a thing for which you want silent pushes instead of no pushes at all?

@chrisballinger As far as I understand the phone displays the text sent with the APNS notification and to not disclose the message to Apple we have to use silent notifications (which aren't reliable). Sending high priority pushes with a placeholder text is problematic and those messages would have to be replaced by the actual message content received via XMPP after the device/app wakes up.

iNPUTmice commented 7 years ago

@iNPUTmice I think you are missing iq stanzas. Those should not be displayed as notification to the user but wake up the device so it can respond to them.

Well IQ+push is kinda uncharted territory for now. My guess is that the server might either a) try to answer them (caps caching) or b) send an automated error response for like really stupid unnecessary things some bad behaving clients might send (Thinking of client to client ping for example.) or c) try to deliver them as soon as possible. Think jingle file transfer or voice for example. Even with IQs in mind I have a hard time coming up with a use case for 'hey here is this push but answer that when ever'. Most IQ are fairly time sensitive as the sending side probably has timeouts in place as well.

@weiss I was just responding to a adding priorities to XEP-0357. Even if the situation on iOS is that we can't reliably deliver notifications I'm not sure how adding priorities to the XEP changes that situation.

tmolitor-stud-tu commented 7 years ago

@chrisballinger No, I don't want you to send the message content as plaintext through apples servers. That would be very bad!

iNPUTmice commented 7 years ago

@iNPUTmice @weiss Wouldn't be muc invites also a thing for which you want silent pushes instead of no pushes at all?

I would prefer a high priority push for that. Because only after receiving the invite I can join the MUC, catch up with MAM and actually check for messages that may or may not trigger notifications.

Or if your client goes the do not auto accept invites route (fine as well) you might want to trigger a notification asking the user to accept the invitation.

Edit: Sorry @tmolitor-stud-tu I'm not trying stop you from adding that to the XEP. If you think it is necessary or desirable you should probably talk to the author. I don't need it personally (for Conversations) but that doesn't mean it can't be in the XEP. But you mentioned me so I thought I'd share my opinion.

chrisballinger commented 7 years ago

@weiss Silent notifications are throttled by the OS and won't work if you have low power enabled, you've force closed the app, or you've restarted the device. From my experience, they work well enough for me to receive messages most of the time.

In the future we can (ab)use iOS 10 mutable notifications, as they are more reliable than content-available, but it requires a 1:1 mapping between a push and content to display. If you fail to fetch content, it must display some sort of default message to the user. This would require sending additional data down the pipe so we know if a push corresponds to a typing notification, or filtering out typing notifications altogether. Snapchat sends pushes for typing notifications, so maybe it's not terrible to show those.

Adding iOS 10 mutable notifications isn't easy, as it requires a MAJOR refactor of the app to allow for a multi-process design. See UNNotificationServiceExtension. Since I plan to start working on a macOS port over the summer, there will be a lot of refactoring during this period to allow for multi process sandboxing.

tmolitor-stud-tu commented 7 years ago

@iNPUTmice On iOS silent pushes aren't reliable and high priority pushes always display a notification containing the text specified by the push server. With the current API it isn't possible for iOS developers to have reliable pushes AND auto accept invites to check MAM etc.

Having a way in the XEP to distinguish pushes which contain actual messages instead of some invite or iq or something else, would be good. This way high priority pushes could be sent for those messages and silent ones for everything else.

This way message could still be delivered even when the device is in power saving mode or the app was force closed.

iNPUTmice commented 7 years ago

@tmolitor-stud-tu so just to clarify you want to send a high priority push messages for things that will definitively trigger a notification and low priority push messages for something that might trigger notifications?

tmolitor-stud-tu commented 7 years ago

@iNPUTmice Exactly :) Otherwise (e.g. when everything is pushed as high priority) there could be useless notifications floating around irritating users and spamming the lockscreen.

tmolitor-stud-tu commented 7 years ago

@iNPUTmice Ideally there would be some sort of api to specify which stanzas the client wants as high priority push and which ones as low priority....maybe along with an identifier to further identify certain stanzas. This could be used by the push server to display different messages to the user in the high priority case like "New Message", "Incoming MUC invite" etc.

None of this is neccessary for Conversations as GCM silent pushes are 100% reliable...but in the iOS case these are neccessary to deliver a good user experience.

weiss commented 7 years ago

In the future we can (ab)use iOS 10 mutable notifications

Couldn't you use "normal" (non-silent) high priority notifications already, just that this wouldn't allow you to edit the notification text? I mean, couldn't you send a generic "new message" text without refactoring ChatSecure and depending on iOS 10?

From my experience, and from various user comments I've seen in this issue and elsewhere, the current solution does feel quite unreliable in practice. Maybe users would be happy to live with generic "new message" notifications if this makes things reliable? (This is also what the Matrix client "Riot" seems to be doing when E2EE is enabled, by the way. Not sure how other messengers solve the issue on iOS ...)

iNPUTmice commented 7 years ago

@tmolitor-stud-tu I'd design it the other way round. Instead the client telling the server for what types it wants a HIGH or LOW priority I'd let the server tell the app server more about the type and let the app server decide. So a server to app server push would contain a type=readable-message, or type=invite or something like that.

chrisballinger commented 7 years ago

We can send a high priority push that also fetches in the background, but it must be accompanied by text/sound/badge and this content cannot be modified at runtime. That allows for a push that says something generic like "Incoming Message", which then does some background stuff, and then generates local notifications as it does currently.

The notification’s priority. Provide one of the following values:

10 The push message is sent immediately. The push notification must trigger an alert, sound, or badge on the device. It is an error to use this priority for a push that contains only the content-available key. 5 The push message is sent at a time that conserves power on the device receiving it.

tmolitor-stud-tu commented 7 years ago

@iNPUTmice Well, this would also be possible. But we need an exhaustive list of types to help the app server decide what message it will show for high priority pushes.

Having those pre-defined types is not as flexible and future proof as having the client specify which stanzas it wants and assigning a type to them.

But defining an api for those "stanza filtering" in the XEP is a big change whereas your proposal is a bit smaller and easier to implement on the xmpp server side. While making the app server side not much harder to implement.

I'd go with your proposal as far as I'm concerned :)

tmolitor-stud-tu commented 7 years ago

@chrisballinger Is it already possible to wake up the app by using https://push.chatsecure.org/api/v1/messages/ directly (with a generic alert text like "New Message")?

If yes, I could hack my server implementation to use this api endpoint directly for high priority messages without you changing the app at all.

Would be a HUGHE improvement for me!

PS: I sent you an email containing my mailaddress for the beta btw.

afriedmanGlacier commented 7 years ago

I'm late to the party, but we are doing what @weiss and @tmolitor-stud-tu are suggesting in sending a generic "New Message." We decided this was a better solution for us in that we always want to receive the push notification and we weren't getting them all as silent pushes. Using eJabber and a modified version of @weiss mod_push along with ChatSecure-Push-Server and RubDub. mod_push is changed so that we aren't getting things like typing notifications and non-messages. Changed the push notification in CS Push Server to be non-silent "New Message." I'm not sure our solution is the best for everyone, but it works well for our needs and we get all of our pushes. The thing I'm not sure about is how our current version plays with MUC because we haven't been using it yet. Just in case that helps anyone :)

chrisballinger commented 7 years ago

@tmolitor-stud-tu We can change the behavior of our pubsub module and push API server to allow for this without needing to modify the clients at all.

I think that some types should be defined so that my pubsub node translate or filter the requests before passing them off to our push API server. Some ideas and how they could map to hard coded localizable strings:

tmolitor-stud-tu commented 7 years ago

@chrisballinger This would be really awesome! :) I'll create a list of stanza types tomorrow.

tmolitor-stud-tu commented 7 years ago

@chrisballinger @iNPUTmice @weiss I would propose the following notification types:

Any thoughts on this?

weiss commented 7 years ago

I would propose the following notification types:

I think I'd avoid predefining notification types.

One idea might be to let clients add an include_payload (or whatever) attribute to the <enable/> request, which is none by default but can be set to full or stripped instead. If it's set to full, the published <notification/>s would include a <payload/> child which holds the complete stanza. With stripped, it would strip all stanza contents except for the stanza's name, its type attribute (if any), and the direct child elements with their xmlns attributes (if any). Or something like that.

Maybe the (underspecified) urn:xmpp:push:summary thing could then be removed from the XEP.

Anyway, these things should probably be discussed on the standards list instead.

chrisballinger commented 7 years ago

@weiss I don't think including the full stanza is a good idea for privacy reasons because the pubsub server is usually run by the app developers, not your XMPP server operator. You can then end up leaking content and metadata in two places instead of (mostly) just one. I 100% do not support including full stanzas, and I refuse to implement any functionality that relies on parsing them. I don't even feel comfortable with the current summary functionality that allows people to include the sender and message body (I also ignore this content).

Allowing servers to send our pubsub bridge unwanted full stanzas is dangerous and creates a central point of failure. In my opinion it defeats the whole point of having a decentralized network. Servers operators might think it's required to include this data, or modules might have bad defaults. The ONLY functionality we need is a tap on the shoulder to wake the client up, and then do a normal connection.

tmolitor-stud-tu commented 7 years ago

@chrisballinger Holger proposed this setting to be entirely configurable by the client. Servers sending full stanzas when not configured to do so would violate the standard and servers can do this already, despite of any XEP changes.

There are some valid scenarios for inclusion of full stanzas. For example a server monitoring pager used by a company which operates the XMPP server AND the app server as well as their own app. This doesn't even mean that the app server does send out the whole unencrypted stanza through GCM or APNS.

However Holger's proposal mainly was to use stripped stanzas instead of hardcoded stanza types. This way no content would be leaked, but the xep and implementation thereof would be much more flexible:

With stripped, it would strip all stanza contents except for the stanza's name, its type attribute (if any), and the direct child elements with their xmlns attributes (if any).

And last but not least Holger proposed to make the current behaviour to not send any (useful) metadata at all the default:

One idea might be to let clients add an include_payload (or whatever) attribute to the request, which is none by default but can be set to full or stripped instead.

I like his proposal and I think it is the right way to go (and removing the underspecified urn:xmpp:push:summary thing was something I thought about as well).

weiss commented 7 years ago

The current revision of the XEP specifies how to include message body contents with the push notification. It doesn't specify under what circumstances the server might or should do that. This is something I'd definitely remove from the XEP.

As @tmolitor-stud-tu said, what I suggested was making this configurable per client instead, which IMO is totally harmless. Just don't let ChatSecure enable this behavior. I suggested having the option because I can imagine non-APNS scenarios where including full stanzas makes sense. E.g., an Ubuntu Phone app cannot reconnect (to fetch stanzas) while in the background, and you might have a trusted app server there (you're not forced to use central infrastructure like on iOS or Android). Or you might be using XEP-0357 in a totally different context, such as IoT.

However, as @tmolitor-stud-tu also pointed out, my main suggestion was the option to include stripped stanzas with notifications (instead of specifying a predefined "notification type"). Examples:

<!-- A chat message: -->
<message type='chat'><body/></message>

<!-- A PEP/PubSub event: -->
<message type='headline'><event xmlns='http://jabber.org/protocol/pubsub#event'></message>

<!-- A Jingle session initiation: -->
<iq type='set'><jingle xmlns='urn:xmpp:jingle:1'/></iq>

The ONLY functionality we need is a tap on the shoulder to wake the client up, and then do a normal connection.

Initially I was really hoping this would be true on all relevant mobile platforms (which might not include things like Ubuntu Phone), but this discussion was motivated by the fact that silent notifications don't seem to be reliable on iOS, no? :-/

chrisballinger commented 7 years ago

As @tmolitor-stud-tu said, what I suggested was making this configurable per client instead, which IMO is totally harmless. Just don't let ChatSecure enable this behavior.

I see, basically allow clients to tell the server how to behave, and not leave it up to the settings on the server. My nightmare scenario was that all of a sudden I get a bunch of leaky full stanzas because of poorly configured servers. There are valid cases where, if you own the whole infrastructure, you don't mind that stuff is leaking... but I don't want those cases to ever be considered default behavior. I just don't like that I can't really control what is sent to pubsub.chatsecure.org, so I want to minimize that as much as possible.

My hesitation with stripped stanzas is how do we know what is "safe" to include? If a client sends a more complex stanza than the strip algorithm can't handle, how do we ensure that the result doesn't leak unexpected content? Even if we can agree on a stripping algorithm, it will still leak more metadata to me than I want compared to boiling it down to just message and invite types. It also adds complexity to each 357 pubsub implementation to figure out how to parse different kinds of stanzas.

tmolitor-stud-tu commented 7 years ago

@chris I don't think that stripping stanzas would be overly complex. I guess an implementation in prosody would take about 10-15 lines of code.

Yes, this is a bit more metadata than just invite or message, but in my opinion the stripped stanzas contain nothing sensitive. Only having direct child tag names and namespaces as well as tag name and namespace of the stanza itself really isn't much metadata.

weiss commented 7 years ago

If a client sends a more complex stanza than the strip algorithm can't handle, how do we ensure that the result doesn't leak unexpected content?

Well, my ad-hoc suggestion above was to "strip all stanza contents except for the stanza's name, its type attribute (if any), and the direct child elements with their xmlns attributes (if any)." This assumes the element names and type/xmlns values won't contain sensitive data. But yes, this does leak some details about the type of data your users are receiving. (Though to me this seems less sensitive than the user JIDs currently leaked to your app server as per the XEP.)

it will still leak more metadata to me than I want compared to boiling it down to just message and invite types.

Yes, if we're reasonably confident that message and invite will be "enough for anybody"™ then there's no point in my suggestion. My fear is that other clients might have push requirements we failed to anticipate while predefining notification types.

chrisballinger commented 7 years ago

@weiss

(Though to me this seems less sensitive than the user JIDs currently leaked to your app server as per the XEP.)

I also would like that to not be leaked as well, because it's not needed in our case. Perhaps that could be addressed in future revision and made configurable by the client as well.

My fear is that other clients might have push requirements we failed to anticipate while predefining notification types.

I agree, perhaps we could still boil it down to a few simple types but offer an escape hatch w/ stripped or full stanzas.

tmolitor-stud-tu commented 7 years ago

@gerg5c42g542g2c54g52c That won't be possible, sorry...

chrisballinger commented 7 years ago

@gerg5c42g542g2c54g52c What wrong with ejabberd's mod_push? The issue with Prosody 0.10 and ejabberd 17.01 is resolved in v4.0.4, which should be available some time early next week.

weiss commented 7 years ago

It would be nice if it was possible to fix push by solely client-side changes as many servers don't use prosody, but use old ejabberd version with pre-alpha mod_push.

I don't think many servers use the "pre-alpha" module you have in mind (I'm not aware of a single public server that does).

So as I understand this, push on iOS won't work on any ejabberd servers in a reasonably close future

Depends on your definition of "reasonable". A proper mod_push module will be shipped with one of the next ejabberd releases.

chrisballinger commented 7 years ago

Alright, I'm going to close this issue because it's gone way beyond its original scope and now that v4.0.4 is out there are additional resources for end users to self-debug their issues. Please open separate issues for new feature discussion.