Closed Bug 1221097 Opened 9 years ago Closed 8 years ago

[meta] Firefox Accounts device registration

Categories

(Firefox :: Sync, defect)

Product:
Firefox
Component:
Sync
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: stomlinson, Unassigned)

References

Details

(Whiteboard: [fxa][fxa-waffle-ignore])

Attachments

(1 file, 6 obsolete files)

user.js I am using to test.
1.36 KB, application/x-javascript
Details 
This is a bug to track the implementation of "device registration" where Firefox registers itself with Firefox Accounts, with the goal of allowing users to see all of their connect Firefox devices.

For more information see:

* https://github.com/mozilla/fxa/blob/phil/fxa-45-requirements/features/FxA-45-device-registration-api/README.md
* https://github.com/mozilla/fxa/blob/rfk/devices-view-requirements/features/FxA-16-devices-view/README.md
* https://github.com/mozilla/fxa-content-server/issues/3250
* https://github.com/mozilla/fxa-content-server/issues/3252
Assignee: nobody → stomlinson
Depends on: 1221101
Depends on: 1221105
Depends on: 1221107
Depends on: 1221111
Depends on: 1221115
Depends on: 1221125
Depends on: 1221128
Depends on: 1221130
Depends on: 1221135
Depends on: 1222013
Comment on attachment 8686247 [details] [diff] [review]
Register existing and new FxA users with the Devices API. Handle local device name change.

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

::: services/fxaccounts/FxAccounts.jsm
@@ +353,5 @@
>    initialize() {
>      this.currentTimer = null;
>      this.currentAccountState = this.newAccountState();
> +
> +    // XXX the only way I can get this listener to be called is if I comment 

:markh, perhaps you know why this would happen? In the linked to URL, a call is made to `Svc.Prefs.get("client.GUID")` from within a `nsPrefs:changed` observer. The call to `Svc.Prefs.get` prevents the observer registered here from being called.

@@ +518,5 @@
>        // really something we should commit to? Why not let the write happen in
>        // the background? Already does for updateAccountData ;)
>        return currentAccountState.promiseInitialized.then(() => {
>          Services.telemetry.getHistogramById("FXA_CONFIGURED").add(1);
> +        this.registerDevice(existingDeviceId);

markh: you were right, moving all the logic from "browser/base/content/browser-fxaccounts.js" to here made everything much simpler.

::: services/fxaccounts/FxAccountsClient.jsm
@@ +359,5 @@
>        pushPublicKey: devicePushPublicKey,
>        type: deviceType
>      };
>  
> +    log.error("registering a device with info" + JSON.stringify(body));

This is a mock response until the FxA auth server portion is ready.

@@ +400,5 @@
>        id: deviceId,
>        name: deviceName
>      };
>  
> +    log.error("updating a device with info" + JSON.stringify(body));

This is a mock response until the FxA auth server portion is ready.
Attachment #8686247 - Flags: feedback?(markh)
Phil, I'm "ni"ing you so that you know where to look if I disappear suddenly.
Flags: needinfo?(pbooth)
Comment on attachment 8686247 [details] [diff] [review]
Register existing and new FxA users with the Devices API. Handle local device name change.

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

This is going to be quite finicky to get right and I'm not sure I've actually helped, but I think we are on the right path.

::: services/fxaccounts/FxAccounts.jsm
@@ +353,5 @@
>    initialize() {
>      this.currentTimer = null;
>      this.currentAccountState = this.newAccountState();
> +
> +    // XXX the only way I can get this listener to be called is if I comment 

hmm - that makes no sense to me. The code in clients.js seems be be assuming that the client.GUID pref already exists at that point, but I can't see how it would prevent a completely unrelated observer from firing.

Note also that the setting of this pref is, best I can tell, only done by the Sync preference pane - Sync doesn't seem to support the device name being updated by a different device, so it isn't going to change as part of a Sync. So we could consider having Sync prefs make a different call (ie, directly into FxA instead of directly into Sync) so then *this* code becomes the only thing to set that pref. But - IIUC our device UI *will* be allowing the name of a different device to be changed, so Sync is probably going to need to grow support for this device name changing as part of a sync, which would confuse that observer in any case.

@@ +365,2 @@
>    },
> +  

nit: there's quite a few trailing spaces introduced by this patch

@@ +501,5 @@
>     */
>    setSignedInUser: function setSignedInUser(credentials) {
>      log.debug("setSignedInUser - aborting any existing flows");
> +    let existingDeviceId;
> +    this.getSignedInUser().then(currentUserData => {

It looks like you want a "return" here, and the abortExistingFlow() should happen first.

This is also pointing out a bit of a failing in the existing API - setSignedInUser() is used both for a "new" login and for reauthentication, which sucks in some ways, as currently we throw the entire login away - this is the first example of us wanting to carry some data across when reauthenticating, but I wonder if it will be the last.  IOW, there's almost a case for a new API "updateSignedInUser()" - but I'm not sure it's worth trying to fix that here.

I guess you could argue you *are* almost doing that here (although I'd expect we'd check against uid rather than email?), so maybe we could take that further - if the uids do match, don't create the new accountState or abort the existing flows? In that scenario we could probably skip the entire device registration stuff - if we are updating an existing login we must have previously migrated the device stuff - this code isn't called at normal startup, so migration is going to need to happen elsewhere.

IOW:
* If existing account matches, then device must have been previously migrated or registered.
* If existing account does not match, we unconditionally make a new registration.

I'm still unclear on where that migration should live though...

@@ +518,5 @@
>        // really something we should commit to? Why not let the write happen in
>        // the background? Already does for updateAccountData ;)
>        return currentAccountState.promiseInitialized.then(() => {
>          Services.telemetry.getHistogramById("FXA_CONFIGURED").add(1);
> +        this.registerDevice(existingDeviceId);

As written, the this.registerDevice() call could result in a rejected promise, so it needs a .catch() that logs.

Also, note that the user may be unverified at this point, so the device endpoint will need to accept unverified users. I wonder if it makes more sense to not make that assumption?

So off the top of my head I'm thinking:

* loadAndPoll could be the vector for migration - if the user is already verified then do the migration. I think we'd want to do this in the background and not return failure anywhere (other than logs) - if the network is down we'd just try and migrate next startup, which should be fine.

* If the user is not verified we are going to end up in getKeys(), so maybe this function could perform the device registration before fetching the actual keys. getKeys() could maybe be renamed - it's getting encryption keys, so it's basically already doing per-device setup (ie, it could be argued it is the device that needs the keys.) This device registration would need to cope with being called multiple times (ie, we shouldn't assume it will be called exactly once per login) but that sounds reasonable.

FxAccounts is a bit messy around some of this, so feel free to propose refactoring that might make it easier (but if you do refactor, please do that as a separate patch with the device-specific changes on top of it.

@@ +846,1 @@
>      } catch(e) {

note Preferences.jsm will return undefined instead of throwing, so we can drop the catch. (Preferences.jsm is a bit pointless IMO - it's another layer of abstract complexity that doesn't offer much value - I don't object to it being used, but also wouldn't object to not using it ;)

@@ +1362,5 @@
>        }
>      ).catch(err => Promise.reject(this._errorToErrorClass(err)));
>    },
> +
> +  registerDeviceIfNotRegistered: function () {

we typically use the new shorthand for function declarations (ie registerDeviceIfNotRegistered() {...}, for new code

@@ +1366,5 @@
> +  registerDeviceIfNotRegistered: function () {
> +    log.debug("registerDeviceIfNotRegistered");
> +    return this.getSignedInUser()
> +      .then(signedInUser => {
> +        if (! signedInUser) {

nit: please remove the space after the various "!"

@@ +1378,5 @@
> +        }
> +
> +        return this.registerDevice();
> +      }).catch(error => {
> +        log.error('getSignedInUser error: ' + String(error));

as mentioned elsewhere, passing error as the second param will cause the log module to do the right thing (ie, correctly format it and include the stack)

@@ +1396,5 @@
> +        log.error("no signed in user");
> +        return;
> +      }
> +
> +      // TODO - should there be a default device name?

while there should ways be a device name when you are migrating, there isn't going to be that pref for the new-signin flow. Sync's util.js has getDefaultDeviceName() which you could import and use without forcing Sync to initialize.

::: services/fxaccounts/FxAccountsClient.jsm
@@ +400,5 @@
>        id: deviceId,
>        name: deviceName
>      };
>  
> +    log.error("updating a device with info" + JSON.stringify(body));

FTR, doing |log.error("blah blah", body);| would cause body to be automagically JSON.stringified, allowing you to avoid the explicit stringify (and even preventing the stringify being done at all if logging config means the message is discarded) If that second param is an exception object you also get a stack etc too.
Attachment #8686247 - Flags: feedback?(markh) → feedback+
> But - IIUC our device UI *will* be allowing the name of a different device to be changed, so Sync is
> probably going to need to grow support for this device name changing as part of a sync, which
> would confuse that observer in any case.

I don't think we except a device to *discover* that its name has been changed through sync; it would discover the change through the devices API, and push the change into the sync clients record itself.
(In reply to Ryan Kelly [:rfkelly] from comment #7)
> I don't think we except a device to *discover* that its name has been
> changed through sync; it would discover the change through the devices API,
> and push the change into the sync clients record itself.

Right - so IIUC this implies periodically hitting the device endpoint for no reason other than to get this kind of change back - which raises the issue of when exactly we do that. It *might* make sense to integrate that with the clients engine (ie, have FxA just to the initial registration but allow all updates to happen as the clients engine syncs.) So the clients engine's sync basically looks something like:

* If I have a device attribute change locally (eg, name change) push that to the device endpoint.
* Query the device endpoints to see if there are remote changes to apply locally.
* See if the storage record for my device needs to be pushed to client storage.
* Query the storage for new "commands" targeting this device.

(if it wasn't for "commands" I think we could move towards deprecating the client storage record completely - push might make that possible.

If we go that far though, I guess I'd start to wonder why initial registration and legacy migration can't also be done directly the the engine. It does mean the device stuff is tied to Sync, but it's tied to the clients engine which can't be disabled. IOW, the clients engine is an implementation detail and can be something that always syncs even if the user has de-selected all Sync engines (and something we'd probably need to do for "send tab to device" to be sane regardless)
> * Query the storage for new "commands" targeting this device.
> (if it wasn't for "commands" I think we could move towards deprecating the
> client storage record completely - push might make that possible.

I am right now working on a planning document that says "replace sync client commands functionality with a purpose-build service/protocol"...
Whiteboard: [fxa]
This is the 2nd (still unfinished) patch. I am uploading this now because I'm unsure whether I'll be able to continue.
Attachment #8686247 - Attachment is obsolete: true

        Attachment #8686246 -
        Attachment is obsolete: true

    
    

    
  


  
philbooth, this is the user.js I am using to test. I put this into mozilla-central/obj-x86_64-apple-darwin14.5.0/tmp/scratch_user/user.js, you may need to place it elsewhere depending on which user you use when building.

Notice I am using context=fx_desktop_v2. I expect Vlad's fx_desktop_v2 patch to have landed by time we land our patches. Using _v2 also allows a user to delete the current device, then sign back in and have the browser "Do the right thing."

    
  
Depends on: 1227527

    
    

    
  
I'm marking this [fxa-waffle-ignore] since I don't think we want to track the breakdown bug in our waffleboard
Whiteboard: [fxa] → [fxa][fxa-waffle-ignore]
Depends on: 1250782
Depends on: 1250783

    
  
Summary: Breakdown: Firefox Accounts device registration → [meta] Firefox Accounts device registration

    
    

    
  
Shane, is this breakdown complete?

    
    

    
  
I defer to Phil Booth who did the actual implementation.

    
  
Assignee: stomlinson → pbooth

    
    

    
  
> Shane, is this breakdown complete?

Apologies if I'm being dense here. By "complete", do you mean "all of the dependent bugs are done" or do you mean "all of the potential work is covered by dependent bugs"?
Flags: needinfo?(pbooth) → needinfo?(ckarlof)

    
    

    
  
> By "complete", do you mean "all of the work is done" or do you mean "all of the potential work for this feature is covered by dependent bugs"?

If the former, the answer is no because the following are unresolved:

  * Bug 1221105 - Query FxA devices API for current device name on startup
  * Bug 1221125 - Listen for device name change notifications from open Firefox Accounts tabs
  * Bug 1250782 - Register Fennec Firefox Accounts using device registration fxa-auth-server API
  * Bug 1250783 - FxA device registration for iOS
  * Bug 1238895 - Firefox Accounts device registration for B2G

If the latter, I _think_ the answer is yes. But I might be wrong.
Flags: needinfo?(ckarlof)

    
  
Assignee: pbooth → nobody

    
    

    
  
Are any of the patches here still relevant? It would probably be clearer to mark them obsolete if not (and I guess move them to a different bug if they are?)

    
  

        Attachment #8689085 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8689569 [details] [diff] [review]
Define error codes for FxA Devices API

># HG changeset patch
># User Phil Booth <pmbooth@gmail.com>
>
>Bug 1222013 - Define error codes for FxA Devices API
>
>---
> services/fxaccounts/FxAccountsCommon.js | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
>diff --git a/services/fxaccounts/FxAccountsCommon.js b/services/fxaccounts/FxAccountsCommon.js
>index 9fe947a..7924641 100644
>--- a/services/fxaccounts/FxAccountsCommon.js
>+++ b/services/fxaccounts/FxAccountsCommon.js
>@@ -124,6 +124,10 @@ exports.ERRNO_INCORRECT_LOGIN_METHOD         = 117;
> exports.ERRNO_INCORRECT_KEY_RETRIEVAL_METHOD = 118;
> exports.ERRNO_INCORRECT_API_VERSION          = 119;
> exports.ERRNO_INCORRECT_EMAIL_CASE           = 120;
>+exports.ERRNO_ACCOUNT_LOCKED                 = 121;
>+exports.ERRNO_ACCOUNT_UNLOCKED               = 122;
>+exports.ERRNO_UNKNOWN_DEVICE                 = 123;
>+exports.ERRNO_DEVICE_SESSION_CONFLICT        = 124;
> exports.ERRNO_SERVICE_TEMP_UNAVAILABLE       = 201;
> exports.ERRNO_PARSE                          = 997;
> exports.ERRNO_NETWORK                        = 998;
>@@ -150,7 +154,10 @@ exports.ERRNO_INVALID_CONTENT_TYPE           = 113 + exports.OAUTH_SERVER_ERRNO_
> // Errors.
> exports.ERROR_ACCOUNT_ALREADY_EXISTS         = "ACCOUNT_ALREADY_EXISTS";
> exports.ERROR_ACCOUNT_DOES_NOT_EXIST         = "ACCOUNT_DOES_NOT_EXIST ";
>+exports.ERROR_ACCOUNT_LOCKED                 = "ACCOUNT_LOCKED";
>+exports.ERROR_ACCOUNT_UNLOCKED               = "ACCOUNT_UNLOCKED";
> exports.ERROR_ALREADY_SIGNED_IN_USER         = "ALREADY_SIGNED_IN_USER";
>+exports.ERROR_DEVICE_SESSION_CONFLICT        = "DEVICE_SESSION_CONFLICT";
> exports.ERROR_ENDPOINT_NO_LONGER_SUPPORTED   = "ENDPOINT_NO_LONGER_SUPPORTED";
> exports.ERROR_INCORRECT_API_VERSION          = "INCORRECT_API_VERSION";
> exports.ERROR_INCORRECT_EMAIL_CASE           = "INCORRECT_EMAIL_CASE";
>@@ -184,6 +191,7 @@ exports.ERROR_UI_REQUEST                     = "UI_REQUEST";
> exports.ERROR_PARSE                          = "PARSE_ERROR";
> exports.ERROR_NETWORK                        = "NETWORK_ERROR";
> exports.ERROR_UNKNOWN                        = "UNKNOWN_ERROR";
>+exports.ERROR_UNKNOWN_DEVICE                 = "UNKNOWN_DEVICE";
> exports.ERROR_UNVERIFIED_ACCOUNT             = "UNVERIFIED_ACCOUNT";
> 
> // OAuth errors.
>@@ -267,6 +275,10 @@ SERVER_ERRNO_TO_ERROR[ERRNO_INCORRECT_LOGIN_METHOD]         = ERROR_INCORRECT_LO
> SERVER_ERRNO_TO_ERROR[ERRNO_INCORRECT_KEY_RETRIEVAL_METHOD] = ERROR_INCORRECT_KEY_RETRIEVAL_METHOD;
> SERVER_ERRNO_TO_ERROR[ERRNO_INCORRECT_API_VERSION]          = ERROR_INCORRECT_API_VERSION;
> SERVER_ERRNO_TO_ERROR[ERRNO_INCORRECT_EMAIL_CASE]           = ERROR_INCORRECT_EMAIL_CASE;
>+SERVER_ERRNO_TO_ERROR[ERRNO_ACCOUNT_LOCKED]                 = ERROR_ACCOUNT_LOCKED;
>+SERVER_ERRNO_TO_ERROR[ERRNO_ACCOUNT_UNLOCKED]               = ERROR_ACCOUNT_UNLOCKED;
>+SERVER_ERRNO_TO_ERROR[ERRNO_UNKNOWN_DEVICE]                 = ERROR_UNKNOWN_DEVICE;
>+SERVER_ERRNO_TO_ERROR[ERRNO_DEVICE_SESSION_CONFLICT]        = ERROR_DEVICE_SESSION_CONFLICT;
> SERVER_ERRNO_TO_ERROR[ERRNO_SERVICE_TEMP_UNAVAILABLE]       = ERROR_SERVICE_TEMP_UNAVAILABLE;
> SERVER_ERRNO_TO_ERROR[ERRNO_UNKNOWN_ERROR]                  = ERROR_UNKNOWN;
> SERVER_ERRNO_TO_ERROR[ERRNO_NETWORK]                        = ERROR_NETWORK;
>@@ -290,7 +302,10 @@ SERVER_ERRNO_TO_ERROR[ERRNO_INVALID_CONTENT_TYPE]           = ERROR_INVALID_CONT
> // Map internal errors to more generic error classes for consumers
> ERROR_TO_GENERAL_ERROR_CLASS[ERROR_ACCOUNT_ALREADY_EXISTS]         = ERROR_AUTH_ERROR;
> ERROR_TO_GENERAL_ERROR_CLASS[ERROR_ACCOUNT_DOES_NOT_EXIST]         = ERROR_AUTH_ERROR;
>+ERROR_TO_GENERAL_ERROR_CLASS[ERROR_ACCOUNT_LOCKED]                 = ERROR_AUTH_ERROR;
>+ERROR_TO_GENERAL_ERROR_CLASS[ERROR_ACCOUNT_UNLOCKED]               = ERROR_AUTH_ERROR;
> ERROR_TO_GENERAL_ERROR_CLASS[ERROR_ALREADY_SIGNED_IN_USER]         = ERROR_AUTH_ERROR;
>+ERROR_TO_GENERAL_ERROR_CLASS[ERROR_DEVICE_SESSION_CONFLICT]        = ERROR_AUTH_ERROR;
> ERROR_TO_GENERAL_ERROR_CLASS[ERROR_ENDPOINT_NO_LONGER_SUPPORTED]   = ERROR_AUTH_ERROR;
> ERROR_TO_GENERAL_ERROR_CLASS[ERROR_INCORRECT_API_VERSION]          = ERROR_AUTH_ERROR;
> ERROR_TO_GENERAL_ERROR_CLASS[ERROR_INCORRECT_EMAIL_CASE]           = ERROR_AUTH_ERROR;
>@@ -314,6 +329,7 @@ ERROR_TO_GENERAL_ERROR_CLASS[ERROR_NO_SILENT_REFRESH_AUTH]         = ERROR_AUTH_
> ERROR_TO_GENERAL_ERROR_CLASS[ERROR_NOT_VALID_JSON_BODY]            = ERROR_AUTH_ERROR;
> ERROR_TO_GENERAL_ERROR_CLASS[ERROR_PERMISSION_DENIED]              = ERROR_AUTH_ERROR;
> ERROR_TO_GENERAL_ERROR_CLASS[ERROR_REQUEST_BODY_TOO_LARGE]         = ERROR_AUTH_ERROR;
>+ERROR_TO_GENERAL_ERROR_CLASS[ERROR_UNKNOWN_DEVICE]                 = ERROR_AUTH_ERROR;
> ERROR_TO_GENERAL_ERROR_CLASS[ERROR_UNVERIFIED_ACCOUNT]             = ERROR_AUTH_ERROR;
> ERROR_TO_GENERAL_ERROR_CLASS[ERROR_UI_ERROR]                       = ERROR_AUTH_ERROR;
> ERROR_TO_GENERAL_ERROR_CLASS[ERROR_UI_REQUEST]                     = ERROR_AUTH_ERROR;
>

        Attachment #8689569 -
        Attachment is obsolete: true

    
  

        Attachment #8689577 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Phil Booth from comment #19)
> > Shane, is this breakdown complete?
> 
> Apologies if I'm being dense here. By "complete", do you mean "all of the
> dependent bugs are done" or do you mean "all of the potential work is
> covered by dependent bugs"?

Yeah, I meant "is all the potential (known) work covered by dependent bugs". Kit changed this to a meta-bug instead of a breakdown bug. A breakdown bug in Firefox is sometimes used to just capture the work to breakdown a larger piece of work. 

Thanks, I'm good now!

    
    

    
  
This has landed! \o/
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: