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

XEP-0357 Push not working due to removed subscription #646

Closed
afriedmanGlacier opened this issue Jan 9, 2017 · 10 comments
Closed

XEP-0357 Push not working due to removed subscription #646

afriedmanGlacier opened this issue Jan 9, 2017 · 10 comments
Labels
Milestone

Comments

@afriedmanGlacier
Copy link
Contributor

afriedmanGlacier commented Jan 9, 2017

Using eJabberd with the weiss version of mod_push, we have Push notifications working, but after a little while it stops working. One of the issues we noticed is that after some time of sleeping and the app being suspended (and we think after presence is unset), when the user re-opens the app, the client logs back in, and creates a new session (rather than resuming the old one). At this point the subscription to buddies is deleted, and it never resubscribes unless the client manually logs out and logs back in. Without the subscription, push notifications no longer work.

I think the client should re-subscribe after re-logging in?

@afriedmanGlacier
Copy link
Contributor Author

afriedmanGlacier commented Jan 9, 2017

I don't think this is the ideal solution, but in playing with code to fix this issue, we made the following changes in OTRXMPPManager. This seemed to mostly fix this particular problem (although I'm not sure yet if it creates other problems). Just adding this in case its of any use to those who know the code better.

In the declarations at the top, we added:
@Property (nonatomic) BOOL isEnabled;

We set isEnabled to YES at the end of the enablePushWithToken method after sending enable for the first time, so that we don't have to receive xmppCapabilities again when reenabling.

Similar to the code in the xmppCapabilities delegate method, we added the following:

(void) reenablePush {
    PushController *pushController = [OTRAppDelegate appDelegate].pushController;
    BOOL hasPushAccount = [pushController.pushStorage hasPushAccount];
    
    if (hasPushAccount && self.isEnabled) {
        ... do the same stuff that is in xmppCapabilities for push ...
                        [self enablePushWithToken:token endpoint:endpoint];
        ...
    }
}

The reenablePush method gets called at the end of the goOnline method, if(isEnabled)

@chrisballinger
Copy link
Member

This change fixes your issue? You should only really need to call enablePushWithToken once every ~30 days when the token expires. There won't be any negative side effects by doing this, but it isn't necessary.

There is no such thing as "subscription to buddies" as far as push messages are concerned. The push settings should never be reset by the server on a new session, so something else must be going on server side.

@afriedmanGlacier
Copy link
Contributor Author

Oops, thanks for your comment, for now we updated it to only resend the "enable" message without the token or endpoint. We are looking into the possibility of server side issues, and I will try to post again (or close the issue) if we find something on that side.

@chrisballinger
Copy link
Member

chrisballinger commented Jan 10, 2017

If you send "enable" without the token and endpoint it won't work. The token and endpoint are custom parameters that are required for ChatSecure to work with our servers. Another thing.. if you are running your own build of the app it won't work with push.chatsecure.org because of how APNS works. You'll need to run your own ChatSecure Push API server, although you can still use pubsub.chatsecure.org

You might want to re-read how the spec works: http://xmpp.org/extensions/xep-0357.html#enabling

edit: apologies for sounding snarky

@weiss
Copy link

weiss commented Jan 10, 2017

@chrisballinger

The push settings should never be reset by the server on a new session

This is an experimental XEP-0357 module which actually does reset the push settings on a new session. I'm currently rewriting the code and the new version won't do that.

However, I still believe it would be a good idea to re-<enable/> notifications whenever creating a new session. If you don't, there's no way for the server to associate the new session with the push-enabled client. So (as @iNPUTmice noted) the server will continue generating notifications despite that client being online, and won't be able to perform certain adjustments for push clients. Also, the admin might want to expire stale push subscriptions after some time period (which might be less than 30 days).

These things are underspecified in XEP-0357, but I think the business rules suggested by @iNPUTmice make sense, so I'd appreciate if we could either agree on those or discuss any necessary improvements on the standards list.

@chrisballinger
Copy link
Member

@weiss Oh okay, that would explain things. Yeah, because of how our code is designed at the moment, we don't re-enable on every session, but still do it somewhat frequently.

We don't have a good test setup on our end for testing ejabberd/mod_push so apologies for not testing that more thoroughly.

@afriedmanGlacier
Copy link
Contributor Author

@chrisballinger Sorry, I didn't specify that we are using ChatSecure-Push-Server and RubDub. Yeah, I noticed after I posted that sending the enable without the token/endpoint only worked until the session was recreated, but I wanted to test more things before replying more fully.

I ended up removing the change to my ChatSecure and making a change on the server-side for now (mod_push) until the released versions of those are on the same page.

Based on your conversation with @weiss, do you want to leave this issue open for testing with ejabberd and mod_push on your end, or would you rather I close it?

@iNPUTmice
Copy link

However, I still believe it would be a good idea to re- notifications whenever creating a new session. If you don't, there's no way for the server to associate the new session with the push-enabled client.

Quick reminder that sending <enable/> for push on every connect (or in every session rather) is a small change that would increase interop with the existing push module. (I can't asses if this is actually trivial in the ChatSecure code but if it is i'd love to see that for 4.0.1)

@chrisballinger chrisballinger added this to the 4.0.1 milestone Jan 18, 2017
@davidchiles
Copy link
Member

So I looked over how ChatSecure sends the enable push stanza.

Every time we receive the push capability and we have a ChatSecure push account with an active token we send the enable stanza.

@chrisballinger chrisballinger modified the milestones: 4.0.1, 4.0.2 Jan 24, 2017
@chrisballinger chrisballinger modified the milestones: 4.0.2, 4.0.3, 4.0.4 Feb 7, 2017
@chrisballinger
Copy link
Member

I'm gonna close this because there has been a lot of refactoring and a few releases since the last activity here.

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

No branches or pull requests

5 participants