Closed Bug 1346061 Opened 7 years ago Closed 7 years ago

Register for push notifications from FxA before account is verified

Categories

(Firefox for Android Graveyard :: Firefox Accounts, enhancement, P1)

Product:
Firefox for Android Graveyard
Component:
Firefox Accounts
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: Grisha, Assigned: eoger)

References

Details

Attachments

(3 files)

Bug 1346061 part 1 - Expose getSessionToken() on State.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1346061 part 3 - Trigger a sync/email-verification-check on empty FxA push message.
59 bytes, text/x-review-board-request
Grisha
: review+
nalexander
: review+
Details
Bug 1346061 part 2 - Register device on FxA on Engaged state.
59 bytes, text/x-review-board-request
Grisha
: review+
nalexander
: review+
Details 
Implementation details to come.

Doing this will unblock a lot of good UX:
- push-driven "verified" state
- start sync as soon as account/login is verified
- send tabs will start working sooner
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: -- → P1
Attachment #8849271 - Flags: review?(nalexander)
Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review124114

This looks like a good first cut, but I think there's an error in where the state machine is plumbed.  I expect you'll need to keep at least some of the `Married` logic in place, if not all of it.  If you need both parts of the functionality in both states, extract a helper and invoke both.  Also, be sure that you don't accidentally re-register unnecessarily if you get from `Engaged` through to `Married` in one shot.  (I expect your version and timestamp checking will ensure this just works, but there's asynchronous code in play...)

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountPushHandler.java:66
(Diff revision 1)
>      }
>  
> +    private static void handleVerification(Context context) {
> +        AndroidFxAccount fxAccount = AndroidFxAccount.fromContext(context);
> +        if (fxAccount == null) {
> +            Log.e(LOG_TAG, "The account does not exist anymore");

nit: the _Android_ account does not exist anymore.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountPushHandler.java:69
(Diff revision 1)
> +        AndroidFxAccount fxAccount = AndroidFxAccount.fromContext(context);
> +        if (fxAccount == null) {
> +            Log.e(LOG_TAG, "The account does not exist anymore");
> +            return;
> +        }
> +        fxAccount.requestImmediateSync(null, null); // This will trigger verification check and a sync

nit: full stop at the end.  (We always use full sentence comments in this code base.)

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:617
(Diff revision 1)
>    }
>  
>    public byte[] getSessionToken() throws InvalidFxAState {
>      State state = getState();
>      StateLabel stateLabel = state.getStateLabel();
>      if (stateLabel == StateLabel.Cohabiting || stateLabel == StateLabel.Married) {

If grisha agrees, let's expose `getSessionToken` on all states, and return `null` if it's not available.  You'll need to ensure consumers do the right thing when they get `null`.  This'll prevent some of this casting.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:623
(Diff revision 1)
>        TokensAndKeysState tokensAndKeysState = (TokensAndKeysState) state;
>        return tokensAndKeysState.getSessionToken();
> +    } else if (stateLabel == StateLabel.Engaged) {
> +      return ((Engaged) state).getSessionToken();
>      }
> -    throw new InvalidFxAState("Cannot get sessionToken: not in a TokensAndKeysState state");
> +    throw new InvalidFxAState("Cannot get sessionToken: not in a state with a session token");

Maybe include the `stateLabel` in this message to aid debugging.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:530
(Diff revision 1)
>          private boolean shouldRequestToken(final BackoffHandler tokenBackoffHandler, final Bundle extras) {
>            return shouldPerformSync(tokenBackoffHandler, "token", extras);
>          }
>  
>          @Override
> +        public void handleEngaged(Engaged state) {

This changes the semantics in a way I don't think you intend.  Referring to https://people-mozilla.org/~nalexander/FxA_client_states.pdf (which I believe is still correct for Android; for iOS, we split some of the states in fairly obvious ways) you'll see that you expect to get to `Married` every sync, but you expect to get to `Engaged` only when you've _lost_ the `sessionToken` in some way and -- generally by having the remote password change underneath you -- and recovered it -- generally by having the user sign in again on this device.

So, as written, you'll not execute this code as frequently as you were before, which I think is not what you wanted, since you're handling your period registration renewal here.
Attachment #8849271 - Flags: review?(nalexander) → review-
Thank you Nick, I addressed your comments.
Specifically, I am now doing the device registration in two places (Married and Engaged), better safe than sorry.
I also added a simple lock mechanism to avoid making 2 concurrent requests.
Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review124672

Sorry to make you loop on this again, but there's a simpler, more robust way to achieve this.  You might need to make the executor service static or longer lived.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:63
(Diff revision 2)
>    // devices after we update what we send on device registration.
>    public static final Integer DEVICE_REGISTRATION_VERSION = 2;
>  
>    private static FxAccountDeviceRegistrator instance;
>    private final WeakReference<Context> context;
> +  private final ExpiringLock lock = new ExpiringLock(LOCK_EXPIRE_MS);

This assumes that there is only one `FxAccountDeviceRegistrator`, which it appears is the case since you have a singleton pattern in place.  I'd still consider making the lock static in order to be really clear about the expectations here.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:185
(Diff revision 2)
>        Log.i(LOG_TAG, "Attempting registration for an existing device");
>        Logger.pii(LOG_TAG, "Device ID: " + deviceId);
>        device = FxAccountDevice.forUpdate(deviceId, clientName, pushCallback, pushPublicKey, pushAuthKey);
>      }
>  
>      ExecutorService executor = Executors.newSingleThreadExecutor(); // Not called often, it's okay to spawn another thread

This lock/unlock is fragile.  You already have a much more robust way to make your requests sequentially and ensure they're non-overlapping, the executor here.  (See https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executors.html#newSingleThreadExecutor() for notes.)

So just always submit a new task, but make your task check to see if it's still needed (timestamp, version, whatever).  Then if you succeed in `Engaged` you just don't do anything in `Married`; if you fail in `Engaged`, you'll retry in `Married`, but that's okay I think.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:217
(Diff revision 2)
>                  && error.apiErrorNumber == FxAccountRemoteError.INVALID_AUTHENTICATION_TOKEN) {
>            handleTokenError(error, fxAccountClient, fxAccount);
>          } else {
>            logErrorAndResetDeviceRegistrationVersionAndTimestamp(error, fxAccount);
>          }
> +        lock.unlock();

I'd feel more confident if you had a `finally` block that unlocked.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:352
(Diff revision 2)
> +    public ExpiringLock(long timeout) {
> +      this.timeout = timeout;
> +    }
> +
> +    private synchronized boolean tryGetLock() {
> +      long now = new Date().getTime();

We generally use `System.currentTimeMillis()` for such timestamps.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/login/Doghouse.java:27
(Diff revision 2)
>  
>    @Override
>    public Action getNeededAction() {
>      return Action.NeedsUpgrade;
>    }
> +

Extract this `sessionToken` bit to a Pre: patch, please.
Attachment #8849271 - Flags: review?(nalexander) → review-
From my conversation on IRC with nalexander:

I think using the Executor is a good idea, however response callbacks are executed in a different thread [0][1].
My problem with this is that if we post a new device registration while another one is already in flight, the Executor will first call the new-device-registration Runnable before executing the Runnable callbacks (setting DEVICE_REGISTRATION_VERSION, used to determine if we start a registration or not..).

We came up with 3 solutions:

(1) I proposed to allow the FxAccountClient20 constructor to accept a null Executor, and execute the callbacks in the current thread instead of posting a new Runnable. We would still maybe have multiple concurrent Push registrations, but the device registration would be completely sequential.

(2) Nick proposed we could use a PriorityBlockingQueue inside the ExecutorService, and have the callbacks have a higher priority than the registration start.

(3) We could also peek at the previous stages of the FxA state machine (stateLabelsSeen), and don't trigger a device registration on Married when we already went through Engaged.


[0]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountClient20.java#270-286
[1]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java#201-203
Flags: needinfo?(nalexander)
(In reply to Edouard Oger [:eoger] from comment #6)
> From my conversation on IRC with nalexander:
> 
> I think using the Executor is a good idea, however response callbacks are
> executed in a different thread [0][1].
> My problem with this is that if we post a new device registration while
> another one is already in flight, the Executor will first call the
> new-device-registration Runnable before executing the Runnable callbacks
> (setting DEVICE_REGISTRATION_VERSION, used to determine if we start a
> registration or not..).
> 
> We came up with 3 solutions:
> 
> (1) I proposed to allow the FxAccountClient20 constructor to accept a null
> Executor, and execute the callbacks in the current thread instead of posting
> a new Runnable. We would still maybe have multiple concurrent Push
> registrations, but the device registration would be completely sequential.
> 
> (2) Nick proposed we could use a PriorityBlockingQueue inside the
> ExecutorService, and have the callbacks have a higher priority than the
> registration start.
> 
> (3) We could also peek at the previous stages of the FxA state machine
> (stateLabelsSeen), and don't trigger a device registration on Married when
> we already went through Engaged.

I prefer (3), 'cuz it's simple and uses "local" information (rather than a "global" lock).

I'd be happy with (1) as well, although this opens you up to races when updating the FxA information locally (which you should have already protected against).  If you prefer this, use https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/Utils.java#568 rather than allowing the ExecutorService to be null.
Flags: needinfo?(nalexander)
Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review125036

Apologies for a late review. Some quick thoughts below, and i'll jot some more notes soon.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountPushHandler.java:41
(Diff revision 2)
>              }
>          }
>          if (message == null) {
>              // An empty body means we should check the verification state of the account (FxA sends this
>              // when the account email is verified for example).
> -            // TODO: We're only registering the push endpoint when we are in the Married state, that's why we're skipping the message :(
> +            handleVerification(context);

If you're going to split the patch up for readability, this bit would make a good "Part 2".

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountPushHandler.java:48
(Diff revision 2)
>          }
>          try {
>              String command = message.getString("command");
>              JSONObject data = message.getJSONObject("data");
>              switch (command) {
>                  case COMMAND_DEVICE_DISCONNECTED:

From the handler implementation, this command seems to be "your device disconnected", telling us to remove our account.


There's a "deviceConnected" event as well - I'm not sure if that might include "your device connected", and what "connected" means - verified? or just "logged-in"?:
https://github.com/mozilla/fxa-auth-server/blob/master/docs/pushpayloads.schema.json#L14

Is that something you might want to handle similarly to "empty push message", requesting a verification (that is, requesting a sync)? Either as Part 2 or as a follow-up.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/login/Doghouse.java:27
(Diff revision 2)
>  
>    @Override
>    public Action getNeededAction() {
>      return Action.NeedsUpgrade;
>    }
> +

+1

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:546
(Diff revision 2)
>          private boolean shouldRequestToken(final BackoffHandler tokenBackoffHandler, final Bundle extras) {
>            return shouldPerformSync(tokenBackoffHandler, "token", extras);
>          }
>  
>          @Override
> +        public void handleEngaged(Engaged state) {

If you're unverified, you will end up in an Engaged state as you pump through the state machine, correct?

The `handleFinal` handler will then call either `handleMarried` or `handleNotMarried`. Couldn't you instead of performing registration in `handleEngaged` do it as a a side-effect of the `handleFinal` IF you are in an Engaged state at the end?

Unless I'm missing something this should eliminate the "we're going to try and register twice in a row" case.
Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review125118

Going to clear the flag for now, and wait for a simpler version of this which does registration at the end of the state machine transitions (unless there's a great reason we somehow can't do that).
Attachment #8849271 - Flags: review?(gkruglov) → review-
Comment on attachment 8850607 [details]
Bug 1346061 part 2 - Register device on FxA on Engaged state.

https://reviewboard.mozilla.org/r/123142/#review125578

I think this is fine. I'll try it out to see what the new experience feels like.

::: commit-message-cd130:1
(Diff revision 1)
> +Bug 1346061 part 3 - Register device on FxA on Engaged state. r?nalexander, grisha

Complete nit, but I would have made this part 2, and your part 2 would have been part 3.
Attachment #8850607 - Flags: review?(gkruglov) → review+
Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review125584

Why do you still need this?
Attachment #8849271 - Flags: review?(gkruglov)
Comment on attachment 8850606 [details]
Bug 1346061 part 3 - Trigger a sync/email-verification-check on empty FxA push message.

https://reviewboard.mozilla.org/r/123140/#review125588

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountPushHandler.java:40
(Diff revision 1)
>                  return;
>              }
>          }
>          if (message == null) {
>              // An empty body means we should check the verification state of the account (FxA sends this
>              // when the account email is verified for example).

Can you please link to server docs, API docs, code, or anything else that definitely describes this?
Attachment #8850606 - Flags: review?(gkruglov) → review+
> Why do you still need this?

We're still getting a session token on Engaged state, see nalexander's comment 2
This seems to work pretty well in local testing! The beginnings of Push driven sync :-)
Comment on attachment 8850606 [details]
Bug 1346061 part 3 - Trigger a sync/email-verification-check on empty FxA push message.

https://reviewboard.mozilla.org/r/123140/#review125652

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountPushHandler.java:70
(Diff revision 2)
> +        AndroidFxAccount fxAccount = AndroidFxAccount.fromContext(context);
> +        if (fxAccount == null) {
> +            Log.e(LOG_TAG, "The Android account does not exist anymore");
> +            return;
> +        }
> +        // This will trigger an email verification check and a sync.

I'd add a Log.i statemenet here, something like "Received 'accountVerified' push event, requesting immediate sync".

For completeness as you're looking at logs.
Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review125654

Is there a good reason we would ever try to obtain a sessionToken from sessions which are not one of Married, Engaged and, I guess, Cohabitating? Trying to get a sessionToken when you're in Separated or Doghouse state seems like a programming error to me, something is either off with our state machine flow or with how we're treating its final state. So I suggest you still throw in those getters, or throw by default in `getSessionToken` method in State base class.

::: commit-message-05bfa:1
(Diff revision 4)
> +Bug 1346061 part 1 - Expose getSessionToken() on State. r?nalexander

Describe the "why" as well as the "what". Specifically, say that we'll need access to sessionToken while we're in the Engaged state in order to perform device registration, and so we're exposing it here as a pre-requisite for further work.
Attachment #8849271 - Flags: review+
Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review125656

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/login/State.java:76
(Diff revision 4)
>    public abstract void execute(ExecuteDelegate delegate);
>  
>    public abstract Action getNeededAction();
> +
> +  @Nullable
> +  public abstract byte[] getSessionToken();

Perhaps this should throw. See my other comment.
Thanks Grisha, I made the method throw by default
Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review126514

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/login/MigratedFromSync11.java:34
(Diff revision 5)
>    }
> +
> +  @Nullable
> +  @Override
> +  public byte[] getSessionToken() {
> +    return null;

Shouldn't this throw, like the other place?

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:195
(Diff revision 5)
>          return;
>        }
>        final AndroidFxAccount fxAccount = new AndroidFxAccount(context, account);
>        final ExtendedJSONObject payload = createNotifyDevicesPayload();
>  
> -      final byte[] sessionToken;
> +      final byte[] sessionToken = fxAccount.getState().getSessionToken();

Why don't you need to catch exceptions here?

Are you assuming that the state is `Married`, since you're syncing?  This should be true, but there may be races in the system -- things can happen (like debug actions in the FxAccountStatusActivity that force the account into different states) while a Sync is ongoing!

Or are you assuming that unhandled exceptions will be caught outside if this method?

`IllegalStateException` is unchecked so the compiler won't tell you statically that you're not catching it.
Attachment #8849271 - Flags: review?(nalexander) → review-
Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review126514

I'm pretty sure you've introduced new uncaught exception flows in this patch.  You'd be safer to just re-use/re-purpose the illegal state exception, which is checked and would reveal many of these things statically.

If I'm mis-reading or missing things, please comment (or add code comments!) explaining why you don't need to catch these unchecked exceptions.
Comment on attachment 8850607 [details]
Bug 1346061 part 2 - Register device on FxA on Engaged state.

https://reviewboard.mozilla.org/r/123142/#review126518

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java:549
(Diff revision 3)
>          @Override
>          public void handleNotMarried(State notMarried) {
>            Logger.info(LOG_TAG, "handleNotMarried: in " + notMarried.getStateLabel());
>            schedulePolicy.onHandleFinal(notMarried.getNeededAction());
>            syncDelegate.handleCannotSync(notMarried);
> +          if (notMarried.getStateLabel() == StateLabel.Engaged) {

This might be more pleasant if you handled it before branching on the `StateLabel`, around https://dxr.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/FxAccountAuthenticator.java#172.

If you do that, the `null` session token might make more sense than raising an exception.  I leave this choice to you (and possibly Grisha, since he suggested the exception).
Attachment #8850607 - Flags: review?(nalexander) → review+
Comment on attachment 8850606 [details]
Bug 1346061 part 3 - Trigger a sync/email-verification-check on empty FxA push message.

https://reviewboard.mozilla.org/r/123140/#review126520

lgtm.
Attachment #8850606 - Flags: review?(nalexander) → review+
Comment on attachment 8849271 [details]
Bug 1346061 part 1 - Expose getSessionToken() on State.

https://reviewboard.mozilla.org/r/122088/#review126848

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/login/State.java:17
(Diff revisions 5 - 7)
>    public static final long CURRENT_VERSION = 3L;
>  
> +  public class NotASessionTokenState extends Exception {
> +
> +    private static final long serialVersionUID = 8628129091996684799L;
> +    

nit: whitespace.
Attachment #8849271 - Flags: review?(nalexander) → review+
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d14c27bb279
part 1 - Expose getSessionToken() on State. r=Grisha,nalexander
https://hg.mozilla.org/integration/autoland/rev/5602cad1c4a1
part 2 - Register device on FxA on Engaged state. r=Grisha,nalexander
https://hg.mozilla.org/integration/autoland/rev/9b0e58ec80cb
part 3 - Trigger a sync/email-verification-check on empty FxA push message. r=Grisha,nalexander
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: