Improving the FeedbackService/APNService model usability

[mbargiel]

We just recently set up a cron job for the Feedback service cleanup command, and while doing so, we ran into a few usability issues that I thought be interesting to highlight. They're minor, but still worth fixing, I think. @stephenmuss I'd like to have your comments about those issues.

1. The FeedbackService class doesn't need to be a model

Feedback Service calls are always done wrt. to a specific APN service, so it doesn't strike me as necessary to have to configure a FeedbackService separately. Since FeedbackService._connect() uses the underlying APNService instance to define its certificate, it should likely do the same with the hostname/URL. In fact, only the port differs (and, obviously, the request and response paylods).

Note: if FeedbackService isn't a model, then the admin could include custom actions in the APNService change list/change form.

2. The call_feedback_service command shouldn't require a FeedbackService ID

I don't see why one would want to call the Apple feedback service for only a single APN Service when there are more than one configured. Why not do them all at a time? That way, it wouldn't be necessary to have a cron job with hard-coded database IDs or that needs to look them up first - which is exactly what the command should do in the first place, IMHO. (Besides, making the FeedbackService class not derive from Model reinforces this.)

3. The APNService class doesn't need to be a model either

I believe having to define APNService entries is kind of overdoing it. What really needs to be configured is a set of certificates, with a useful name (we use the app's AppID), and whether it is a sandbox certificate or a production certificate.

I don't believe Apple is likely to change their model anytime soon, so I would replace having to enter hostnames and ports with settings that have default values, e.g. IOS_NOTIFICATIONS_APN_PORT, IOS_NOTIFICATIONS_FEEDBACK_PORT, IOS_NOTIFICATIONS_HOSTNAME_PRODUCTION, IOS_NOTIFICATIONS_HOSTNAME_SANDBOX. Users would be free to override these values, for instance if they want to proxy the calls.

The only reason I see for keeping the hostname in the database entries is if different certificates for the same environment must be proxied differently... but is that really a valid use case?

I think the APNService model could be replaced with a Certificate model. The APNService class and FeedbackService classes would remain to abstract the connection- and communication-related details, using a Certificate instance that would be passed in the constructor, for instance.

[stephenmuss]

@mbargiel I generally agree with everything you've mentioned above.

I'd be happy to implement a lot of these changes for a major release update. I already anticipated releasing the next version as 0.2 so I'd be happy to bundle this type of makeover in that release.

Implementing a lot of these changes will give greater flexibility to developers using DIN.

In particular, I think replacing the APNService model with a Certificate model is a good idea. This was largely the reason for having APNService as a model in the first place and moving that into a certificate model sounds like a better idea to me.

I also agree that having to pass an id of a feedback service instance is kind of clunky. I'd be happy to address this as part of the changes you've discussed.

[kalvish21]

I also would have one more suggestion. Since the app is using PyOpenSSL anyway, why not just use the the *.p12 file for the cert/private key, as opposed to the text in PEM format? It can be saved locally somewhere for the app. As I understand it, its very easy to import the cert and the private key:

p12 = crypto.load_pkcs12(file(“push.p12", 'rb').read(), [password])
p12.get_privatekey() # Private key
p12.get_certificate() # Certificate

This would save a lot of hassle with the app the way it is...having to convert the P12 (which can be exported from the keychain) to two pem files.

[mbargiel]

Agreed. Stephen probably had a reason for doing it like it is right now
though, so I'd like his opinion on this.

Le dimanche 8 décembre 2013, kalvish21 a écrit :

I also would have one more suggestion. Since the app is using PyOpenSSL
anyway, why not jus the the *.p12 file for the cert/private key, as opposed
to the text in PEM format? As I understand it, its very easy to import the
cert and the private key:

p12 = crypto.load_pkcs12(file(“push.p12", 'rb').read(), [password])
p12.get_privatekey() # Privat key
p12.get_certificate() # Certificate

This would save a lot of hassle with the app the way it is...having to
convert the P12 (which can be exported from the keychain) to two pem files.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/33#issuecomment-30089193
.

[stephenmuss]

This probably came down to personal preference when I first implemented. I preferred to have my keys centrally in the database rather than having to copy them all to a server and storing them in settings files or in the database.

For me it is just as inconvenient to do this as it is to convert to pem and store in the model.

I'm happy to be persuaded otherwise but at the moment this would be a fairly breaking change and I don't see any major tangible benefits.

[mbargiel]

Well, I agree with @kalvish21. I see the benefit of uploading the .p12
directly in a FileField (or perhaps since they're so small, base64'd and
saved in an django-fields EncryptedTextField). The advantage I see is the
simplicity of simply uploading a .p12 file rather than having to use the
OpenSSL command-line tool (or some other OpenSSL tool) to manually extract
the data, then save it to the DB. In the end, it's the same, only a bit
more work (at least in my experience).

If really necessary, we could find a way to support both ways of uploading
the certificate data to the DB, perhaps with different forms and a proxy
model class... Food for thought.

On Mon, Dec 9, 2013 at 4:28 AM, Stephen Muss notifications@github.comwrote:

This probably came down to personal preference when I first implemented. I
preferred to have my keys centrally in the database rather than having to
copy them all to a server and storing them in settings files or in the
database.

For me it is just as inconvenient to do this as it is to convert to pem
and store in the model.

I'm happy to be persuaded otherwise but at the moment this would be a
fairly breaking change and I don't see any major tangible benefits.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/33#issuecomment-30117215
.