1231549 - Set default profile endpoint server URI for Firefox Accounts not created via the web flow

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[Nick Alexander :nalexander [he/him]]

When we landed Bug 1171141, we didn't migrate Firefox Accounts forward by including a default or stage profile URI when appropriate.

This ticket tracks adding a default fallback to the profile server URI accessor around https://dxr.mozilla.org/mozilla-central/rev/319be5e7ce3061c7c16f24d750b6dacdbcac4c35/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java#301 -- see the example for OAuth just past that -- or migrating null URIs forward in place (in the upgrade receiver, I suppose).

Comment 1

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #0)
> When we landed Bug 1171141, we didn't migrate Firefox Accounts forward by
> including a default or stage profile URI when appropriate.

This means that all users with Firefox Accounts *created* before Fennec 44 are not displaying profile images throughout the App, even when they could.

Comment 2

[dlim]

(In reply to Nick Alexander :nalexander from comment #0)
> When we landed Bug 1171141, we didn't migrate Firefox Accounts forward by
> including a default or stage profile URI when appropriate.
> 
> This ticket tracks adding a default fallback to the profile server URI
> accessor around
> https://dxr.mozilla.org/mozilla-central/rev/
> 319be5e7ce3061c7c16f24d750b6dacdbcac4c35/mobile/android/services/src/main/
> java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java#301 -- see
> the example for OAuth just past that -- or migrating null URIs forward in
> place (in the upgrade receiver, I suppose).

I understood the first approach, but I didn't quite get the second one, do you mind expanding on that?

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to dlim from comment #2)
> 
> (In reply to Nick Alexander :nalexander from comment #0)
> > When we landed Bug 1171141, we didn't migrate Firefox Accounts forward by
> > including a default or stage profile URI when appropriate.
> > 
> > This ticket tracks adding a default fallback to the profile server URI
> > accessor around
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 319be5e7ce3061c7c16f24d750b6dacdbcac4c35/mobile/android/services/src/main/
> > java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java#301 -- see
> > the example for OAuth just past that -- or migrating null URIs forward in
> > place (in the upgrade receiver, I suppose).
> 
> I understood the first approach, but I didn't quite get the second one, do
> you mind expanding on that?

Each time the Android package is upgraded, we get a broadcast notification.  I can't recall what we currently do in there :)  See https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/receivers/FxAccountUpgradeReceiver.java.

We could use that upgrade receiver to check if we have a null profile server URI, do the fallback calculation, and update the state.  The advantage there is that we get "good data" into the account, rather than always working around the problem forever.  The dis-advantage -- upgrades can fail, can conflict, are less flexible to roll-back, etc.

Comment 4

[dlim]

Attachment: Set default profile endpoint server URI for Firefox Accounts not created via the web flow

Comment 5

[dlim]

(In reply to Nick Alexander :nalexander from comment #3)
> (In reply to dlim from comment #2)
> > 
> > (In reply to Nick Alexander :nalexander from comment #0)
> > > When we landed Bug 1171141, we didn't migrate Firefox Accounts forward by
> > > including a default or stage profile URI when appropriate.
> > > 
> > > This ticket tracks adding a default fallback to the profile server URI
> > > accessor around
> > > https://dxr.mozilla.org/mozilla-central/rev/
> > > 319be5e7ce3061c7c16f24d750b6dacdbcac4c35/mobile/android/services/src/main/
> > > java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java#301 -- see
> > > the example for OAuth just past that -- or migrating null URIs forward in
> > > place (in the upgrade receiver, I suppose).
> > 
> > I understood the first approach, but I didn't quite get the second one, do
> > you mind expanding on that?
> 
> Each time the Android package is upgraded, we get a broadcast notification. 
> I can't recall what we currently do in there :)  See
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/
> main/java/org/mozilla/gecko/fxa/receivers/FxAccountUpgradeReceiver.java.
> 
> We could use that upgrade receiver to check if we have a null profile server
> URI, do the fallback calculation, and update the state.  The advantage there
> is that we get "good data" into the account, rather than always working
> around the problem forever.  The dis-advantage -- upgrades can fail, can
> conflict, are less flexible to roll-back, etc.

It seems like there are more drawbacks that benefits of going with the second option?

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8722605 [details] [diff] [review]
Set default profile endpoint server URI for Firefox Accounts not created via the web flow

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

It's possible for users to have custom auth servers, in which case the default profile server is not appropriate.  So, if the profileURI is null, then check for stage and default auth servers and return the appropriate profile server, following the oauth server example.

Yes, this is a little ridiculous :)

Comment 7

[dlim]

Attachment: Set default profile endpoint server URI for Firefox Accounts not created via the web flow

Added :)

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8722640 [details] [diff] [review]
Set default profile endpoint server URI for Firefox Accounts not created via the web flow

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

Excellent.

You should be able to set the "checkin-needed" flag on the bug, which will have a sheriff see your patch, see my review, and land your code.  Good work!

Comment 9

[dlim]

(In reply to Nick Alexander :nalexander from comment #8)
> Comment on attachment 8722640 [details] [diff] [review]
> Set default profile endpoint server URI for Firefox Accounts not created via
> the web flow
> 
> Review of attachment 8722640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Excellent.
> 
> You should be able to set the "checkin-needed" flag on the bug, which will
> have a sheriff see your patch, see my review, and land your code.  Good work!

Thanks!  
However, I don't think I

Comment 10

[dlim]

(In reply to dlim from comment #9)
> (In reply to Nick Alexander :nalexander from comment #8)
> > Comment on attachment 8722640 [details] [diff] [review]
> > Set default profile endpoint server URI for Firefox Accounts not created via
> > the web flow
> > 
> > Review of attachment 8722640 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Excellent.
> > 
> > You should be able to set the "checkin-needed" flag on the bug, which will
> > have a sheriff see your patch, see my review, and land your code.  Good work!

Thanks!  
However, I don't think I have the ability to add keywords to a bug, it's not showing an edit button beside the 'keyword' field?

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0ed2152fd50b

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ed2152fd50b

Comment 13

[Nick Alexander :nalexander [he/him]]

mfinkle: no rush, but you might verify that your profile avatar begins to appear in new Nightly builds.  Thanks!

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Verified. It works now.