966004 - Allow DELETE /collection?ids= (i.e. "delete these zero items")

Status: VERIFIED WONTFIX

(Cloud Services Graveyard :: Server: Sync, defect)

Description

[Ryan Kelly [:rfkelly]]

I've observed FF trying to do this in the wild:

  1391118588255	Sync.Collection	DEBUG	mesg: DELETE fail 400 https://sync-1-us-east-1.sync.services.mozilla.com/1.1/9002/storage/passwords?ids=

The 400 error doesn't seem to have broken anything, but we may as well allow it if it's baked-in client behaviour.

Comment 1

[Ryan Kelly [:rfkelly]]

Attachment: sync15-delete-empty-ids.diff

simple fix, simple test

Comment 2

[Toby Elliott [:telliott]]

Comment on attachment 8368400 [details] [diff] [review]
sync15-delete-empty-ids.diff

Review of attachment 8368400 [details] [diff] [review]:
-----------------------------------------------------------------

I understand making sure we support not screwing up the client at this time, but not thrilled having such an ambiguous behavior floating around in the client. Is ids= a request to delete nothing or everything? I'd argue it's simply client error and should be treated as such in the long run.

Comment 3

[Ryan Kelly [:rfkelly]]

Well, maybe it's better for us to error out here than.  Adding Richard for client-side perspective - is is expected that we'll see DELETE requests with an empty "ids" parameter?

Comment 4

[Richard Newman [:rnewman]]

This could happen if the client is somehow under the belief that it should delete its Sync credentials from the server passwords collection, but it doesn't actually have any. That's pretty unlikely.

Other than that, from reading the code I can't see any reason why this would occur. Do you have more of a log?

As far as the client is concerned, if an ids= param is provided, it means to delete only those records -- it's not the same as DELETE /collection.

Conceivably this is related to the recent oddities around machines not syncing correctly with FxA auth, which I haven't investigated.

Comment 5

[Toby Elliott [:telliott]]

I agree that from a safety perspective, deleting everything in this situation is bad. 

The correct response here is definitely an error, but the open question here is what would happen if the client receives that error? If the resulting client behavior is messed up or unclear, we should accept it as a 200 no-op for now for safety.

Comment 6

[Ryan Kelly [:rfkelly]]

The sync log from Bug 966148 shows such a request: https://bug966148.bugzilla.mozilla.org/attachment.cgi?id=8368386

Comment 7

[Richard Newman [:rnewman]]

Betcha it's this:

  _syncFinish: function _syncFinish() {
    SyncEngine.prototype._syncFinish.call(this);

    // Delete the weave credentials from the server once
    if (!Svc.Prefs.get("deletePwd", false)) {
      try {
        let ids = Services.logins.findLogins({}, PWDMGR_HOST, "", "")
                          .map(function(info) {
          return info.QueryInterface(Components.interfaces.nsILoginMetaInfo).guid;
        });
        let coll = new Collection(this.engineURL, null, this.service);
        coll.ids = ids;
        let ret = coll.delete();
        this._log.debug("Delete result: " + ret);

        Svc.Prefs.set("deletePwd", true);


Betcha you didn't have services.sync.deletePwd, then that delete happened, then you did.

With FxA the old Sync PWDMGR_HOST passwords might not be set, so this'll do nothing.

Comment 8

[Ryan Kelly [:rfkelly]]

Yep, this sounds like the culprit.  It also sounds fairly benign and evidently the 400 error doesn't cause any harm since all we do is log the result.  Perhaps we're better off leaving this as an error from the server.

Comment 9

[Richard Newman [:rnewman]]

File a client bug for those guys to fix? We shouldn't even be uploading Weave credential records; this seems like a band-aid fix from years ago.

Comment 10

[Ryan Kelly [:rfkelly]]

OK, the client-side behavior has been fixed so let's just leave the server as-is.

Comment 11

[James Bonacci [:jbonacci]]

OK.