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 sandbox URL to ApnsHttp2 dispatcher #392

Merged
merged 1 commit into from
Mar 8, 2018

Conversation

brianlittmann
Copy link
Contributor

Was getting URI::InvalidURIError, bad URI(is not URI?) when trying to send Apns2 notifications with app.environment set to sandbox.

Was getting `URI::InvalidURIError, bad URI(is not URI?)` when trying to send Apns2 notifications with `app.environment` set to `sandbox`.
@aried3r
Copy link
Member

aried3r commented Dec 7, 2017

Hey and thanks for your PR.

I don't use APNS myself, but it seems the endpoints are just development and production. https://developer.apple.com/library/content/documentation/NetworkingInternet/Conceptual/RemoteNotificationsPG/CommunicatingwithAPNs.html

According to node-apn/node-apn#528 this got changed at some point, sandbox being removed.

Do you think this change is necessary considering the docs?

@brianlittmann
Copy link
Contributor Author

Do you think this change is necessary considering the docs?

I would say so, at least in considering consistency within the codebase. See this (albeit old) comment and the current validation for the APNs app model.

@gregblake
Copy link
Contributor

gregblake commented Feb 23, 2018

Our team recently encountered the same issue. We were getting URI::InvalidURIError, bad URI(is not URI?) when trying to send Apns2 notifications with app.environment set to sandbox. In other words, notifications worked fine in our application, unless the app.environment was sandbox.

This PR was very helpful in figuring out the cause of this issue, and how to fix it. I wanted to provide feedback on how we resolved it in our application. Hopefully this will help others that may run across the same thing. The short answer is that we only needed to make changes to how data was being stored.

As @aried3r mentioned, it seems that Apple changed the hostname of their APNS server. What was once sandbox is now development. This is what caused the issue. From what I can tell, Apple introduced this change with APNS2.

As @brianlittmann said, the validations require the environment to be one of the following: development, production, or sandbox. This is what enabled our fix, which is described below.

We were able to resolve the issue by taking the following steps:

  1. Our Rails application has a model that contains our APNS certs. Some of the records in this table had an environment of production, and others had an environment of sandbox. We changed sandbox to development. We did not change the records that had an environment of production, because those apps were working fine, and were unaffected by Apples change to their host name. We only had to change the "sandbox" certs, so that they used Apple's updated hostname of "development".
  2. Our Rails application has another model named MobileDevice. This model has a string attribute named apns_environment. Prior to this change, the iOS devices would set the apns_environment to either sandbox or production. We made changes to the iOS app so that it now sets the apns_environment to either development or production. In the cases that we were using the sandbox name before, now we are using the name development, so it matches the changes Apple made to their hostname.

Once we made the two changes described above, the app.environment matched one of these 2 urls, and notifications worked for both development and production environments.

@aried3r
Copy link
Member

aried3r commented Feb 26, 2018

@gregblake, thanks for the long and elaborate answer! What do you suggest we do? Personally I'd rather document this properly somewhere and leave it as is. Or even better, document it and leave a deprecation warning, but I doubt people will see it if it runs as a daemon.

@ileitch
Copy link
Member

ileitch commented Mar 8, 2018

Merging this. There's no harm in a duplicate environment name, but there is harm in sandbox causing an error.

@ileitch ileitch merged commit d6e81df into rpush:master Mar 8, 2018
Adrian1707 pushed a commit to Adrian1707/rpush that referenced this pull request Apr 24, 2024
Add sandbox URL to ApnsHttp2 dispatcher
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.

4 participants