Skip to content
This repository has been archived by the owner. It is now read-only.

Spurious 'login' message when navigating to /settings with an unverified account #3078

Closed
ncalexan opened this issue Sep 17, 2015 · 6 comments
Closed
Assignees

Comments

@ncalexan
Copy link
Member

@ncalexan ncalexan commented Sep 17, 2015

I see a spurious WebChannel 'login' message when I load accounts.firefox.com/settings?context=fx_fennec_v1&service=sync&entrypoint=preferences. The 'login' message is malformed; it doesn't include keys. Is this intended? Do clients just need to ignore it?

I witnessed this while developing Firefox for iOS as well.

@shane-tomlinson can you comment or re-direct?

The log below also shows the 'login' message appearing before 'loaded', which is counter-intuitive. (Of course, there's no spec, per se.)

D GeckoFxAccounts(26989) FxAccountsWebChannel message received, command: fxaccounts:login
W GeckoAccounts(26989) Got exception updating Firefox Account from JSON; ignoring.
W GeckoAccounts(26989) org.mozilla.gecko.util.NativeJSObject$InvalidPropertyException: Property does not exist
W GeckoAccounts(26989) at org.mozilla.gecko.util.NativeJSObject.getString(Native Method)
W GeckoAccounts(26989) at org.mozilla.gecko.AccountsHelper.handleMessage(AccountsHelper.java:150)
W GeckoAccounts(26989) at org.mozilla.gecko.EventDispatcher.dispatchEvent(EventDispatcher.java:223)
W GeckoAccounts(26989) at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:2275)
W GeckoAccounts(26989) at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
W GeckoAccounts(26989) at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:436)
E GeckoFxAccounts(26989) Could not update Firefox Account from JSON: org.mozilla.gecko.util.NativeJSObject$InvalidPropertyException: Property does not exist
D GeckoFxAccounts(26989) FxAccountsWebChannel message received, command: fxaccounts:loaded
D GeckoFxAccounts(26989) Got LOADED message!

@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Oct 9, 2015

Ahha, I have tracked this down. This occurs with the following STR:

  1. Apply the diff from below to the content server.
  2. Create a browser profile the uses the local FxA stack.
  3. Open http://127.0.0.1:3030/signup?context=fx_fennec_v1&service=sync&entrypoint=preferences
  4. Sign up for a new account, but do not verify.
  5. Open the browser console.
  6. Open http://127.0.0.1:3030/settings?context=fx_fennec_v1&service=sync&entrypoint=preferences
  7. Look in the browser developer console, a login message is sent with several undefined values.
{
  customizeSync: undefined, 
  declinedSyncEngines: undefined, 
  email: "set117@yahoo.com", 
  keyFetchToken: undefined, 
  sessionToken: "XXXXXX", 
  uid: "XXXXX", 
  unwrapBKey: undefined, 
  verified: false, 
  verifiedCanLinkAccount: false
}
diff --git a/app/scripts/models/auth_brokers/fx-sync.js b/app/scripts/models/auth_brokers/fx-sync.js
index e444111..5b4cb48 100644
--- a/app/scripts/models/auth_brokers/fx-sync.js
+++ b/app/scripts/models/auth_brokers/fx-sync.js
@@ -187,7 +187,12 @@ define([
     },

     _notifyRelierOfLogin: function (account) {
-      return this.send(this.getCommand('LOGIN'), this._getLoginData(account));
+      var loginData = this._getLoginData(account);
+
+      console.dir(loginData);
+      console.trace();
+
+      return this.send(this.getCommand('LOGIN'), loginData);
     },

     _getLoginData: function (account) {
@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Oct 9, 2015

Proposed solution - if the browser has already been notified of the login event for the current user, don't send another login event unless the user must sign in again. A second display of the confirm screen should not cause the login event to be sent again.

@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Oct 9, 2015

Another, simpler, proposed solution. Saving whether the relier has been notified of login is more challenging than I expected. A simpler workaround is to not send the login message if unwrapBKey or keyFetchToken are undefined. These values are not persisted to localStorage by FxA. If the user loads FxA and is sent to the /confirm screen without these values, we can assume the user is no longer in the original tab and the message has already been sent.

@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Oct 9, 2015

Yet another solution, only send the login message in the new afterSignUp auth broker method. This makes no assumptions about anything and is the most elegant of the three proposals.

shane-tomlinson pushed a commit that referenced this issue Oct 9, 2015
If the user signs up but does not verify their account,
then visit `/` or `/settings`, they are redirected to `/confirm`
which attempts to notify the browser of login.

Niether `unwrapBKey` and `keyFetchToken` are persisted to disk.

If `/confirm` is shown in a tab other than the original tab,
`notifyBrowserOfLogin` is called with an account without these
fields. The browser can't do anything without this information,
so don't actually send the message.

fixes #3078
@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Oct 9, 2015

Yet another solution, only send the login message in the new afterSignUp auth broker method. This makes no assumptions about anything and is the most elegant of the three proposals.

That doesn't work with "choose what to sync". Solution 2 it is.

shane-tomlinson pushed a commit that referenced this issue Oct 9, 2015
If the user signs up but does not verify their account,
then visit `/` or `/settings`, they are redirected to `/confirm`
which attempts to notify the browser of login.

Neither `unwrapBKey` and `keyFetchToken` are persisted to disk.

If `/confirm` is shown in a tab other than the original tab,
`notifyBrowserOfLogin` is called with an account without these
fields. The browser can't do anything without this information,
so don't actually send the message.

fixes #3078
@ncalexan
Copy link
Member Author

@ncalexan ncalexan commented Oct 9, 2015

On Fri, Oct 9, 2015 at 6:37 AM, Shane Tomlinson notifications@github.com
wrote:

Another, simpler, proposed solution. Saving whether the relier has been
notified of login is more challenging than I expected. A simpler workaround
is to not send the login message if unwrapBKey or keyFetchToken are
undefined. These values are not persisted to localStorage by FxA. If the
user loads FxA and is sent to the /confirm screen without these values,
we can assume the user is no longer in the original tab and the message has
already been sent.

This is consistent with Firefox for iOS, which just drops these incomplete
messages:

https://github.com/mozilla/firefox-ios/blob/abba4c52950f216bdee8b98bad04a930334e8afa/Client/Frontend/Settings/SettingsTableViewController.swift#L143
https://github.com/mozilla/firefox-ios/blob/367dec62b2db133600351e20951e8f37c8a4798a/Client/Frontend/Browser/BrowserViewController.swift#L2031

In fact, that check should be lifted into the FxA code -- but that can be
follow-up :)

shane-tomlinson pushed a commit that referenced this issue Oct 12, 2015
If the user signs up but does not verify their account,
then visit `/` or `/settings`, they are redirected to `/confirm`
which attempts to notify the browser of login.

Neither `unwrapBKey` and `keyFetchToken` are persisted to disk.

If `/confirm` is shown in a tab other than the original tab,
`notifyBrowserOfLogin` is called with an account without these
fields. The browser can't do anything without this information,
so don't actually send the message.

fixes #3078
shane-tomlinson pushed a commit that referenced this issue Oct 12, 2015
If the user signs up but does not verify their account,
then visit `/` or `/settings`, they are redirected to `/confirm`
which attempts to notify the browser of login.

Neither `unwrapBKey` and `keyFetchToken` are persisted to disk.

If `/confirm` is shown in a tab other than the original tab,
`notifyBrowserOfLogin` is called with an account without these
fields. The browser can't do anything without this information,
so don't actually send the message.

fixes #3078
shane-tomlinson pushed a commit that referenced this issue Oct 12, 2015
If the user signs up but does not verify their account,
then visit `/` or `/settings`, they are redirected to `/confirm`
which attempts to notify the browser of login.

Neither `unwrapBKey` and `keyFetchToken` are persisted to disk.

If `/confirm` is shown in a tab other than the original tab,
`notifyBrowserOfLogin` is called with an account without these
fields. The browser can't do anything without this information,
so don't actually send the message.

fixes #3078
shane-tomlinson pushed a commit that referenced this issue Oct 13, 2015
If the user signs up but does not verify their account,
then visit `/` or `/settings`, they are redirected to `/confirm`
which attempts to notify the browser of login.

Neither `unwrapBKey` and `keyFetchToken` are persisted to disk.

If `/confirm` is shown in a tab other than the original tab,
`notifyBrowserOfLogin` is called with an account without these
fields. The browser can't do anything without this information,
so don't actually send the message.

fixes #3078
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants