Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: rfkelly, Assigned: bnicholson)

Tracking

(Blocks 1 bug)

unspecified
Dependency tree / graph

Firefox Tracking Flags

(fxios-v6.0 fixed)

Details

(Whiteboard: [send-tab],[MobileAS],[device-manager])

Attachments

(1 attachment)

Reporter

Description

3 years ago
Firefox Accounts has a new(ish) device registration API, allowing devices to explicitly register themselves when they connect to an account, and provide metadata including a displayname and push notification URL.  Registered devices will appear in a "devices list" view in the FxA settings UI, and can receive push notifications of events associated with the account.

This bug is to discuss (and hopefully implement!) getting Firefox for iOS devices to register with this API.

The API calls are documented at [1], and the initial desktop integration was done in Bug 1227527.  We're working on using it to speed up account verification in Bug 1235607, and plan to build out more push-driven functionality this year.

Richard, do you have any thoughts on this API, and whether/how it can add value to the iOS device connection experience?

[1] https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#api-endpoints
Reporter

Updated

3 years ago
Blocks: 1221097
Reporter

Comment 1

3 years ago
Copying :markh's additional info from Bug 1250782:

"""
Note that to try and help us mitigate the pain of multiple device concepts, desktop now writes a |fxaDeviceId| to the client record, which is the FxA device GUID. The expectation is that this will (a) help later Sync clients to determine whether a client record came from an FxA-device-aware client (eg, to prefer the FxA device info over the Sync info), and (b) to allow Synced Tabs to indirect back to the FxA device (tab records have the Sync client ID rather than the FxA ID). So it probably makes sense for Fennec to do the same until we actually *do* kill it with fire.
"""
We definitely want it -- we want push, we want strong device identification, and all that other 21st-century jazz :)

We can do the same kind of Sync client record hack that desktop does, but I'll split that out into a separate bug -- handling the stuff from desktop can be done immediately.

I'll take a look at the API a little later.


Maria, can we get an Aha card for this? This blocks any push notification work that's already on the boards.
Flags: needinfo?(mpopova)
Depends on: 1252117
See Also: → 1250782
(In reply to Ryan Kelly [:rfkelly] from comment #0)

> Richard, do you have any thoughts on this API, and whether/how it can add
> value to the iOS device connection experience?

Comments on the API:

* The device descriptor should be at least as descriptive as <http://docs.services.mozilla.com/sync/objectformats.html#id2>.

That is, my iPhone should report formfactor="phone", device="iPhone 6S", not just "mobile". We're going to have TVs, tablets, phones, watches, laptops, desktops, and refrigerators here, not just phone-or-desktop.

* Presumably we don't bump createdAt if the device ID has already been seen.

* POST /device doesn't give much clarity around what happens if this session is already registered. It implies that if the device ID differs, it'll 400, and if it's the same it'll update metadata, but it'd be nice to be explicit.

* Similarly when I POST /account/create, and I specify a device bundle that's already associated with an existing device, what happens?

* GET /devices should have some kind of 204/delta response to avoid fetching the entire list every time.

Comment 4

3 years ago
Here is the Aha card: https://mozilla.aha.io/features/FIOS-285
Flags: needinfo?(mpopova)
Reporter

Comment 5

3 years ago
Thanks Richard, I've filed an auth-server bug to review the API based on your feedback: https://github.com/mozilla/fxa-auth-server/issues/1210

> some kind of 204/delta response

Do you mean "304" here?
Here's my progress (does not work) if someone wants to pick this up :
https://github.com/eoger/firefox-ios/commits/device-registration
Hey Edouard,

I skimmed over this and it looks pretty great! Are you looking to keep working on this or transfer it over to someone else potentially on the iOS team? Either way, it'd be great if you could open up a PR to the main repo to make it more visible and make it easier to diff/review the existing work.

Thanks!
Flags: needinfo?(edouard.oger)
I'm not sure I'll have enough time to work on this.
tlacroix contacted me today, maybe he'll carry the flame?
Flags: needinfo?(edouard.oger) → needinfo?(tlacroix)
Whiteboard: [send-tab]
Whiteboard: [send-tab] → [send-tab][MobileAS s1.1]
I looked into this for the last week and Nathanael is going to pick this up. The in-progress PR is https://github.com/mozilla/firefox-ios/pull/2025

To-do:

Save fxaDeviceId and deviceRegistrationVersion when the registerOrUpdateDevice succeeds.
- call Profile.setAccount(...) in the completion handler of registerOrUpdateDevice to save the account.

More error handling:
- recoverFromUnknownDevice()
- logErrorAndResetDeviceRegistrationVersion()
- handleTokenError()

Resources:
Android and Desktop have the function implemented, checkout: https://dxr.mozilla.org/
Firefox Accounts Server API is documented here: https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md



Future work:
Add UI to change device name. (registerOrUpdateDevice works, just the UI needs to be done)
Flags: needinfo?(tlacroix)

Updated

3 years ago
Assignee: nobody → nalcock

Comment 10

3 years ago
The only things left to do now are saving fxaDeviceId and deviceRegistrationVersion; and calling Profile.setAccount().
The working branch for this is currently: https://github.com/mozilla/firefox-ios/tree/pr/2025
Whiteboard: [send-tab][MobileAS s1.1] → [send-tab][MobileAS s1.2]
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [send-tab][MobileAS s1.2] → [send-tab],[MobileAS s1.2],[device-manager]
Assignee

Comment 11

3 years ago
I've been working on this as a precursor to bug 1287643, so assigning it to myself. I've been pushing cleanup/refactoring commits to the same branch at https://github.com/mozilla/firefox-ios/tree/pr/2025.
Assignee: nalcock → bnicholson
Assignee

Comment 12

3 years ago
I think this is ready for an initial review pass. Planning on adding some comments to the PR soon.
Comment on attachment 8782194 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/2056

I just went through the PR and dropped some comments/questions more about the code and some things I saw since I'm not 100 on the account services stuff. I'll defer to the :rnewman/:nalexander for this.
Attachment #8782194 - Flags: feedback+
Whiteboard: [send-tab],[MobileAS s1.2],[device-manager] → [send-tab],[MobileAS s1.3],[device-manager]
Priority: P1 → P2
Whiteboard: [send-tab],[MobileAS s1.3],[device-manager] → [send-tab],[MobileAS],[device-manager]
Assignee

Comment 14

3 years ago
Comment on attachment 8782194 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/2056

Updated with cleaned up error handling and other review comments address. To decrease the scope of this bug, I dropped the WIP code we had to add the fxaDeviceId to the Sync client records; we can that add that in a separate bug.

Flagging sleroux/rnewman in case either of you are up for reviewing this now. If not, I'll defer to nalexander once he's back from PTO!
Attachment #8782194 - Flags: review?(sleroux)
Attachment #8782194 - Flags: review?(rnewman)
Attachment #8782194 - Flags: feedback+
Assignee

Comment 15

3 years ago
(In reply to Brian Nicholson (:bnicholson) from comment #14)
> I dropped the WIP code we had to add the fxaDeviceId to the Sync
> client records; we can that add that in a separate bug.

Namely, bug 1252117.
Blocks: 1252117
No longer depends on: 1252117
Comment on attachment 8782194 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/2056

Skimmed over and left some comments - mostly just nits/code style stuff
Attachment #8782194 - Flags: feedback+
Priority: P2 → P1
Assignee

Comment 17

3 years ago
Comment on attachment 8782194 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/2056

Nick, it'd be great to get your eyes on this now.

One uncertainty I had working on this PR was where we should do the device registration handling. I see that you proposed including this as a post-marriage state at [1], though Richard suggested that it be separate [2].

Right now, the registration is handled when we actually access the account (in BVC [3] and SettingsTableViewController [4]). I don't think this is correct as these are triggered by UI events, and we don't want to tie device registration to UI as you mentioned in [1].

The dependencies are a bit tricky since FxAStateMachine/FirefoxAccount don't have a reference to Profile, which we need to persist the updated account. Perhaps Profile can implement a FirefoxAccountDelegate, attaching itself in the lazy account getter. We could then call the delegate from FirefoxAccount.advance() whenever the FxAState is married.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1250782#c104
[2] https://github.com/mozilla/firefox-ios/pull/2056#discussion_r75365437
[3] https://github.com/mozilla/firefox-ios/pull/2056/files#diff-6d1fb0cf203fcd3c299d4ba3f07f21deR2882
[4] https://github.com/mozilla/firefox-ios/pull/2056/files#diff-6209ecb7e871159425f66f16eed90d7dR464
Attachment #8782194 - Flags: review?(sleroux)
Attachment #8782194 - Flags: review?(rnewman)
Attachment #8782194 - Flags: review?(nalexander)
Comment on attachment 8782194 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/2056

The mechanics here look totally reasonable.

Conceptually, rnewman and I are aligned: device registration is an identity-attached service, like Sync or a Reading List or any other such service.  In practice, however, device management will be consumed by essentially all other services (to support Push) and is much closer to the auth service itself.  (I believe the FxA would have built it into the auth service directly if we had been ready to do so when the auth server was built; we only didn't due to scheduling and a desire to keep the auth server small and stable.)

That suggests that registration should be *parallel* to FxA state advancement.  That's a pain, since the account wants to own the device registration details as well.  Therefore, I suggest to have the state advancement also try to register, and just accept that some registrations may fail.  (We might have devices that are in the process of upgrading and fail to register for some time, but are still able to Sync, so one cannot assume that we always get a registration on any particular schedule anyway; that means we have to handle optional registrations, contrary to what sleroux suggested about making the registration required.)

We want to use the device registration to Push-instigate a Sync after the account is verified on a different device, which means that we want the registration to be in place *before* we are Married (which requires a verified account).  (I see we get this wrong on Android -- sorry for the bad advice earlier.)  So I would add a "general step" to the login state machine that uses the `sessionToken`, if it's available, to register the device before doing the usual state machine.  That is, we insert a "transient NeedsRegistration state" each time we advance the state machine.  This also conveniently allows to keep track of *when* we registered, and re-register every week or month, which is a generally good practice.

Does that make sense?
Attachment #8782194 - Flags: review?(nalexander) → review+
Assignee

Comment 19

3 years ago
Comment on attachment 8782194 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/2056

Updated to do the device registration step before the normal advancing. Having to set the FirefoxAccountDelegate to flush the profile after registration is a bit awkward, but I'm not sure of a better way to persist the device.
Attachment #8782194 - Flags: review+ → review?(nalexander)
Comment on attachment 8782194 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/2056

I'd take this as is, but do spend some time thinking about removing that delegate -- that's an unfortunate life-cycle coupling that we'd prefer to avoid.
Attachment #8782194 - Flags: review?(nalexander) → review+
Assignee

Comment 21

3 years ago
Rebased and merged: https://github.com/mozilla/firefox-ios/commit/80b8d9a16daed208ab37dabecd68307f0b444eb5

Thanks for all the reviews and feedback!
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Iteration: --- → 1.3
You need to log in before you can comment on or make changes to this bug.