Closed Bug 1337244 Opened 7 years ago Closed 7 years ago

Notification for new device connected to this account is displayed on the device being connected

Categories

(Firefox :: Sync, defect, P1)

Product:
Firefox
Component:
Sync
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: markh, Assigned: eoger)

References

Details

Attachments

(1 file)

Bug 1337244 - Delete previous device registration when setting-up a new FxA user.
59 bytes, text/x-review-board-request
markh
: review+
Details 
STR:
* Delete the chrome://FxAccounts login so sync credentials are lost
* Try to sync - notice you are forced into a "needs reauth" state.
* Re-authenticate

Actual:
* A notification is displayed indicating this device has been connected to sync.

Expected:
* Only other devices see that notification.

It's not clear if this also happens on an initial connection, or only on reconnection. Logs show:

> 1486439531220	FirefoxAccounts	TRACE	observed topic=push-message, data=chrome://fxa-device-update, subject=[xpconnect wrapped nsISupports]

It's also not clear if it is expected that the push message goes to this new device - Ryan?
Flags: needinfo?(rfkelly)
> It's also not clear if it is expected that the push message goes to this new device - Ryan?

No, the intention is definitely that the device not receive a notification about itself, see "excludedDeviceIds" here:

  https://github.com/mozilla/fxa-auth-server/blob/master/lib/push.js#L178

But it's entirely possible this is not working correctly.  Alternately, I wonder if this flow puts Firefox in a kind of transient state, where it's registering a new device record but still has an old one active and listening for notifications.  IIRC the "needs reauth" results in a fresh device id being registered, yes?
Flags: needinfo?(rfkelly)
(In reply to Ryan Kelly [:rfkelly] from comment #1)
> But it's entirely possible this is not working correctly.  Alternately, I
> wonder if this flow puts Firefox in a kind of transient state, where it's
> registering a new device record but still has an old one active and
> listening for notifications.

Ah, yes, I suspect that's exactly the case - the logs imply we never unregister the old push endpoint. Also FTR, this doesn't seem to be 100% reproducible - I had to try a few times to make that work.
See Also: → 1337609
Priority: -- → P2
Assignee: nobody → eoger
Priority: P2 → P1
Comment on attachment 8837267 [details]
Bug 1337244 - Delete previous device registration when setting-up a new FxA user.

https://reviewboard.mozilla.org/r/112436/#review113972

This looks good, thanks. However, now we have this new unregister method, I think we should also ensure it is called in the "normal" logout flow (ie, when the user disconnects).

Also, please make sure you give it a thorough test, particularly, for "first signin", "need to reauthenticate" and "logout" cases that that the bug as described is fixed, and that the FxA device management console correctly shows the device as being removed in all cases, and that in no cases is that new "failed to unregister" log entry emitted.

Thanks!

::: services/fxaccounts/FxAccounts.jsm:1559
(Diff revision 1)
> +        return this.fxaPushService.unsubscribe().then(() => {
> +          if (signedInUser.sessionToken && signedInUser.deviceId) {
> +            return this.fxAccountsClient.signOutAndDestroyDevice(signedInUser.sessionToken, signedInUser.deviceId);
> +          }
> +        })
> +        .then(device =>

I don't think the |device| arg here is always correct - IIUC, the promise above could resolve with undefined when there's no session token or device ID. Given it's not used, just remove it.

::: services/fxaccounts/FxAccounts.jsm:1566
(Diff revision 1)
> +            deviceId: null,
> +            deviceRegistrationVersion: null
> +          })
> +        )
> +      })
> +      .catch(err => { log.debug("Error while deleting device registration", err); });

I think we should make this a log.error() - if we find that causes "expected" errors we can tweak it later to better handle them.
Comments addressed, I added the device/push subscription deletion to |_signOutServer| and updated tests.

Also did a bunch of manual testing with different scenarii and never got a duplicate or a log message saying the device deletion failed.
|deleteDeviceRegistration| is also an async function: I asked #jsapi and mconley about 2 weeks ago about async functions in production code and both said it should be OK (but I don't mind reverting to promises if you think it's not reliable enough).
Comment on attachment 8837267 [details]
Bug 1337244 - Delete previous device registration when setting-up a new FxA user.

https://reviewboard.mozilla.org/r/112436/#review114370

Thanks!

::: services/fxaccounts/FxAccounts.jsm:1548
(Diff revision 3)
>      }).catch(error => this._logErrorAndResetDeviceRegistrationVersion(error));
>    },
>  
> +  // Delete the Push Subscription and the device registration on the auth server.
> +  // Returns a promise that always resolves, never rejects.
> +  async deleteDeviceRegistration(sessionToken, deviceId) {

services/sync/tps/extensions/tps/resource/auth/fxaccounts.jsm also has a call to signOutAndDestroyDevice, which should probably also be changed to call this method. It's difficult to verify that will actually work at the moment, but I think you should just make that change anyway :)
Attachment #8837267 - Flags: review?(markh) → review+
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ac4dccad20f
Delete previous device registration when setting-up a new FxA user. r=markh
https://hg.mozilla.org/mozilla-central/rev/9ac4dccad20f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: