1215253 - autopush does not expose DELETE /register{/UAID}

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

Right now, there's no way to DELETE a User-Agent ID; and there's no way to DELETE a registered Channel ID.

I could be convinced that we should never remove an existing uaid, although it seems like a nice courtesy, and could aid testing.  We absolutely need a way to signal that an existing channel is no longer required, though: otherwise, I see no way to *stop* GCM messages reaching a device.

Comment 1

[Nick Alexander :nalexander [he/him]]

jrconlin: over to you to file in GH, re-categorize this ticket, and do whatever needs to be done.

Comment 2

[JR Conlin [:jrconlin,:jconlin]]

thanks!

https://github.com/mozilla-services/autopush/issues/183

Comment 3

[JR Conlin [:jrconlin,:jconlin]]

https://github.com/mozilla-services/autopush/issues/183

Comment 4

[Nick Alexander :nalexander [he/him]]

JR: I have rebased some things and improved error reporting at https://github.com/ncalexan/autopush/commit/41869776fc81abd299bb09064100bcfc59b05b21.

When I issue an HTTP DELETE against /register/uaid/chid, I get a 500, error message below.  Now, it's possible that my changes have impacted this, but I *really* doubt it.  Can you investigate?

{"EnvVersion": "2.0", "Severity": 3, "Timestamp": 1.446613416550708e+18, "Hostname": "chocho.local", "Fields": {"remote-ip": "127.0.0.1", "uaid_hash": "", "time": 1446613416.548894, "system": "-", "user-agent": "Firefox-Android-FxAccounts/45.0a1 (Fennec nalexander)", "channel_id": "", "task_uuid": "547f87fd-6f90-4a32-9981-089036964f1b", "router_key": "", "error": true, "message": "Unhandled Error\nTraceback (most recent call last):\n  File \"/Users/nalexander/Mozilla/autopush/lib/python2.7/site-packages/twisted/internet/base.py\", line 1201, in mainLoop\n    self.runUntilCurrent()\n  File \"/Users/nalexander/Mozilla/autopush/lib/python2.7/site-packages/twisted/internet/base.py\", line 797, in runUntilCurrent\n    f(*a, **kw)\n  File \"/Users/nalexander/Mozilla/autopush/lib/python2.7/site-packages/twisted/internet/defer.py\", line 383, in callback\n    self._startRunCallbacks(result)\n  File \"/Users/nalexander/Mozilla/autopush/lib/python2.7/site-packages/twisted/internet/defer.py\", line 491, in _startRunCallbacks\n    self._runCallbacks()\n--- <exception caught here> ---\n  File \"/Users/nalexander/Mozilla/autopush/lib/python2.7/site-packages/twisted/internet/defer.py\", line 578, in _runCallbacks\n    current.result = callback(current.result, *args, **kw)\n  File \"/Users/nalexander/Mozilla/autopush/autopush/db.py\", line 141, in wrapper\n    return func(self, *args, **kwargs)\nexceptions.TypeError: unregister_channel() takes exactly 3 arguments (4 given)\n"}, "Logger": "Autoendpoint-1.7.2", "Type": "twisted:log"}

Comment 5

[JR Conlin [:jrconlin,:jconlin]]

hrm, might be a merge issue (possibly on the differing branches). 

I've merged both into https://github.com/mozilla-services/autopush/pull/216
Which should solve what you're seeing. I'm going to run a few local tests tomorrow as well and make sure that you can call the delete functions without problems, but the preliminary tests i ran worked without incident.

Comment 6

[JR Conlin [:jrconlin,:jconlin]]

Merged to dev. To be released as part of 1.8.0

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to JR Conlin [:jrconlin,:jconlin] from comment #6)
> Merged to dev. To be released as part of 1.8.0

These DELETE endpoints don't function.  They don't write request bodies, produce 200s on success, and 404s when the given uaid (or chid) is not present.

Comment 8

[JR Conlin [:jrconlin,:jconlin]]

Ok, since there's not much documentation around this, let's make sure that I'm doing what you're expecting.

I presume that you're working from the WebPush API, where the service would return either a 404 for a successful delete or a 410 if the UAID or CHID is not valid, correct? I can fix the return message to do that. There's no mention of what should be in the response body. What would you like that to contain?

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to JR Conlin [:jrconlin,:jconlin] from comment #8)
> Ok, since there's not much documentation around this, let's make sure that
> I'm doing what you're expecting.
> 
> I presume that you're working from the WebPush API, where the service would
> return either a 404 for a successful delete or a 410 if the UAID or CHID is
> not valid, correct? I can fix the return message to do that. There's no
> mention of what should be in the response body. What would you like that to
> contain?

I wasn't, actually; although perhaps we should.  I'm surprised that they use 410 for unknown uaid/chid, but there you go.  Perhaps we should converge.

Comment 10

[Nick Alexander :nalexander [he/him]]

This is still busted, for UAID and CHIDs.  First, for UAIDs, We get a 500 if the registration isn't found:

WaitHelper :: D :: performWait called.
BaseResource :: D :: HTTP POST https://updates-autopush-dev.stage.mozaws.net/v1/gcm/829133274407/registration
BaseResource :: D :: Response: HTTP/1.1 200 OK
WaitHelper :: D :: Action done.
WaitHelper :: D :: performNotify called.
WaitHelper :: D :: Got result from queue: org.mozilla.gecko.background.testhelpers.WaitHelper$Result@60532a0a
WaitHelper :: D :: performWait called.
BaseResource :: D :: HTTP PUT https://updates-autopush-dev.stage.mozaws.net/v1/gcm/829133274407/registration/37498a391c4e4b56b636fcff857d45da
BaseResource :: D :: Added auth header.
BaseResource :: D :: Response: HTTP/1.1 200 OK
WaitHelper :: D :: Action done.
WaitHelper :: D :: performNotify called.
WaitHelper :: D :: Got result from queue: org.mozilla.gecko.background.testhelpers.WaitHelper$Result@7a371926
WaitHelper :: D :: performWait called.
BaseResource :: D :: HTTP DELETE https://updates-autopush-dev.stage.mozaws.net/v1/gcm/829133274407/registration/37498a391c4e4b56b636fcff857d45da
BaseResource :: D :: Added auth header.
BaseResource :: D :: Response: HTTP/1.1 500 Internal Server Error

Second, for CHIDs, we get a 200 even if the CHID isn't found:

WaitHelper :: D :: performWait called.
BaseResource :: D :: HTTP POST https://updates-autopush-dev.stage.mozaws.net/v1/gcm/829133274407/registration
BaseResource :: D :: Using protocols and cipher suites for Android API 21
BaseResource :: D :: Response: HTTP/1.1 200 OK
WaitHelper :: D :: Action done.
WaitHelper :: D :: performNotify called.
WaitHelper :: D :: Got result from queue: org.mozilla.gecko.background.testhelpers.WaitHelper$Result@52234cc4
WaitHelper :: D :: performWait called.
BaseResource :: D :: HTTP POST https://updates-autopush-dev.stage.mozaws.net/v1/gcm/829133274407/registration/9dd4b9e046f6442db4111920e3b257f1/subscription
BaseResource :: D :: Added auth header.
BaseResource :: D :: Response: HTTP/1.1 200 OK
WaitHelper :: D :: Action done.
WaitHelper :: D :: performNotify called.
WaitHelper :: D :: Got result from queue: org.mozilla.gecko.background.testhelpers.WaitHelper$Result@7e791283
WaitHelper :: D :: performWait called.
BaseResource :: D :: HTTP DELETE https://updates-autopush-dev.stage.mozaws.net/v1/gcm/829133274407/registration/9dd4b9e046f6442db4111920e3b257f1/subscription/a0c31b2aac43463c93b153564ed1bdc5
BaseResource :: D :: Added auth header.
BaseResource :: D :: Response: HTTP/1.1 200 OK

Comment 11

[JR Conlin [:jrconlin,:jconlin]]

Hmm... looks like a dynamodb interaction bug, thus why it wasn't being reproduced in testing. I'm able to reproduce this locally. 

Thanks!

Comment 12

[JR Conlin [:jrconlin,:jconlin]]

Patch in: 

https://github.com/mozilla-services/autopush/pull/281

pending review.

I've also created a smoke test script at: https://pastebin.mozilla.org/8854845 which might be useful to verify functions.

Comment 13

[Nick Alexander :nalexander [he/him]]

Looks like this is fixed.  I'll re-open if necessary.