Verification causes logout on device

RESOLVED FIXED

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: _6a68, Assigned: spenrose)

Tracking

unspecified
x86
macOS
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Not sure quite what's up here, but if I go through the signup flow, then verify email from a desktop computer, the handset gets logged out, and *not* set to verified login state.

This is definitely just a nice to have, because I can easily log back in and see the verified state. Logging it without more detail as a placeholder for now
Note that, if I log back in after being logged out, the phone does show that the account is in verified state. Funky.
Assignee

Comment 2

5 years ago
Some shell output:
-------------------
A coding exception was thrown in a Promise resolution callback.

Full message: TypeError: currentState.whenKeysReadyDeferred is null
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
Full stack: FxAccountsInternal.prototype.getKeys/<@resource://gre/modules/FxAccounts.jsm:499:7
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:11
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:586:7
------------------
Assignee

Comment 3

5 years ago
Posted patch 983445-check_for_token.patch (obsolete) — Splinter Review
I simply checked for a null keyFetchToken in getKeys() before calling fetchAndUnwrapKeys(), and removed the corresponding test in the latter and its unit test. fetchAndUnwrapKeys() is only called in that one place.
Assignee: nobody → spenrose
Status: NEW → ASSIGNED
Attachment #8397342 - Flags: feedback?(jparsons)
Comment on attachment 8397342 [details] [diff] [review]
983445-check_for_token.patch

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

Looks good.  I don't think it's necessary to remove the logout protection in fetchAndUnwrap keys, but I leave it up to you to remove it if you prefer.

::: services/fxaccounts/FxAccounts.jsm
@@ +41,5 @@
>    "whenVerified"
>  ];
>  
> +let log = Log.repository.getLogger("FirefoxAccounts");
> +log.warn(' abcdefg\n');

The logger is exported by FxAccountsCommon.js already.  You can enable debugging by preffing 'identity.fxaccounts.loglevel' to 'DEBUG'

@@ -510,5 @@
> -      if (!keyFetchToken) {
> -        yield this.signOut();
> -        return null;
> -      }
> -

I'm sort of on the fence about removing this.  Having the |if (data.keyFetchToken)| guard in getKeys() makes perfect sense.  And I know there are no other callers of this function.  But removing the logout protection makes me feel that it's losing some future-proofing.
Attachment #8397342 - Flags: feedback?(jparsons) → feedback+
Assignee

Comment 5

5 years ago
Posted patch 983445-check_for_token.patch (obsolete) — Splinter Review
Great, thanks Jed. I replaced the null-token check and cleaned up the logging.
Attachment #8407142 - Flags: review?(jparsons)
Assignee

Comment 6

5 years ago
Posted patch 983445-check_for_token.patch (obsolete) — Splinter Review
Cleaned up, test restored and refocused, missing reject() handler restored.
Attachment #8397342 - Attachment is obsolete: true
Attachment #8407142 - Attachment is obsolete: true
Attachment #8407142 - Flags: review?(jparsons)
Attachment #8407183 - Flags: review?(jparsons)
Comment on attachment 8407183 [details] [diff] [review]
983445-check_for_token.patch

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

Looks good!  Thanks, Sam!

::: services/fxaccounts/FxAccounts.jsm
@@ +505,5 @@
>     */
>    getKeys: function() {
>      let currentState = this.currentAccountState;
> +    return currentState.getUserAccountData().then((userData) => {
> +      if (!userData) {

Thanks for cleaning up these variable names.  We had too many return values called 'data'.

@@ +514,4 @@
>        }
>        if (!currentState.whenKeysReadyDeferred) {
>          currentState.whenKeysReadyDeferred = Promise.defer();
> +        if (userData.keyFetchToken) {

Cool.  This guards the auto-logout from fetchAndUnwrapKeys, but fetchAndUnwrapKeys will still log you out if you call it without a keyfetch token for some reason.

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +321,5 @@
>      });
>    });
>  
>    fxa.setSignedInUser(user).then((user) => {
> +    fxa.internal.fetchAndUnwrapKeys();

Cool.
Attachment #8407183 - Flags: review?(jparsons) → review+
Assignee

Comment 8

5 years ago
Comment on attachment 8407183 [details] [diff] [review]
983445-check_for_token.patch

># HG changeset patch
># User Sam Penrose <spenrose@mozilla.com>
># Date 1397603116 25200
>#      Tue Apr 15 16:05:16 2014 -0700
># Node ID 60e8f32a386cefd1bd071d1f2e7c37b5c6352740
># Parent  088f4efdaba17f47384e17d4824e504feed8749d
>Bug 983445 -  Verification causes logout on device r=jedp
>
>diff --git a/services/fxaccounts/FxAccounts.jsm b/services/fxaccounts/FxAccounts.jsm
>--- a/services/fxaccounts/FxAccounts.jsm
>+++ b/services/fxaccounts/FxAccounts.jsm
>@@ -500,48 +500,53 @@ FxAccountsInternal.prototype = {
>    *          kA: An encryption key from the FxA server
>    *          kB: An encryption key derived from the user's FxA password
>    *          verified: email verification status
>    *        }
>    *        or null if no user is signed in
>    */
>   getKeys: function() {
>     let currentState = this.currentAccountState;
>-    return currentState.getUserAccountData().then((data) => {
>-      if (!data) {
>+    return currentState.getUserAccountData().then((userData) => {
>+      if (!userData) {
>         throw new Error("Can't get keys; User is not signed in");
>       }
>-      if (data.kA && data.kB) {
>-        return data;
>+      if (userData.kA && userData.kB) {
>+        return userData;
>       }
>       if (!currentState.whenKeysReadyDeferred) {
>         currentState.whenKeysReadyDeferred = Promise.defer();
>-        this.fetchAndUnwrapKeys(data.keyFetchToken).then(
>-          data => {
>-            if (!data.kA || !data.kB) {
>-              currentState.whenKeysReadyDeferred.reject(
>-                new Error("user data missing kA or kB")
>-              );
>-              return;
>+        if (userData.keyFetchToken) {
>+          this.fetchAndUnwrapKeys(userData.keyFetchToken).then(
>+            (dataWithKeys) => {
>+              if (!dataWithKeys.kA || !dataWithKeys.kB) {
>+                currentState.whenKeysReadyDeferred.reject(
>+                  new Error("user data missing kA or kB")
>+                );
>+                return;
>+              }
>+              currentState.whenKeysReadyDeferred.resolve(dataWithKeys);
>+            },
>+            (err) => {
>+              currentState.whenKeysReadyDeferred.reject(err);
>             }
>-            currentState.whenKeysReadyDeferred.resolve(data);
>-          },
>-          err => currentState.whenKeysReadyDeferred.reject(err)
>-        );
>+          );
>+        }
>       }
>       return currentState.whenKeysReadyDeferred.promise;
>     }).then(result => currentState.resolve(result));
>    },
> 
>   fetchAndUnwrapKeys: function(keyFetchToken) {
>     log.debug("fetchAndUnwrapKeys: token: " + keyFetchToken);
>     let currentState = this.currentAccountState;
>     return Task.spawn(function* task() {
>       // Sign out if we don't have a key fetch token.
>       if (!keyFetchToken) {
>+        log.warn("improper fetchAndUnwrapKeys() call: token missing");
>         yield this.signOut();
>         return null;
>       }
> 
>       let {kA, wrapKB} = yield this.fetchKeys(keyFetchToken);
> 
>       let data = yield currentState.getUserAccountData();
> 
>diff --git a/services/fxaccounts/tests/xpcshell/test_accounts.js b/services/fxaccounts/tests/xpcshell/test_accounts.js
>--- a/services/fxaccounts/tests/xpcshell/test_accounts.js
>+++ b/services/fxaccounts/tests/xpcshell/test_accounts.js
>@@ -300,34 +300,34 @@ add_test(function test_getKeys() {
>           do_test_finished();
>           run_next_test();
>         });
>       });
>     });
>   });
> });
> 
>-// getKeys with no keyFetchToken should trigger signOut
>-add_test(function test_getKeys_no_token() {
>+//  fetchAndUnwrapKeys with no keyFetchToken should trigger signOut
>+add_test(function test_fetchAndUnwrapKeys_no_token() {
>   do_test_pending();
> 
>   let fxa = new MockFxAccounts();
>   let user = getTestUser("lettuce.protheroe");
>   delete user.keyFetchToken
> 
>   makeObserver(ONLOGOUT_NOTIFICATION, function() {
>-    log.debug("test_getKeys_no_token observed logout");
>+    log.debug("test_fetchAndUnwrapKeys_no_token observed logout");
>     fxa.internal.getUserAccountData().then(user => {
>       do_test_finished();
>       run_next_test();
>     });
>   });
> 
>   fxa.setSignedInUser(user).then((user) => {
>-    fxa.internal.getKeys();
>+    fxa.internal.fetchAndUnwrapKeys();
>   });
> });
> 
> // Alice (User A) signs up but never verifies her email.  Then Bob (User B)
> // signs in with a verified email.  Ensure that no sign-in events are triggered
> // on Alice's behalf.  In the end, Bob should be the signed-in user.
> add_test(function test_overlapping_signins() {
>   do_test_pending();
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/68ba8f6f4f03
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Backed out for xpcshell bustage. Please make sure this passes on Try before re-requesting checkin.
https://hg.mozilla.org/integration/fx-team/rev/711ddfbb8bbf

https://tbpl.mozilla.org/php/getParsedLog.php?id=37949327&tree=Fx-Team
Whiteboard: [fixed-in-fx-team]
Assignee

Comment 11

5 years ago
Mark, the test that's failing is here:

https://hg.mozilla.org/mozilla-central/file/b735e618c2a8/services/sync/tests/unit/test_browserid_identity.js#l555

"BrowserIDManager correctly handles getKeys succeeding but not returning keys."

It's not obvious to me how my tweaks to the promises in FxAccounts.jsm/getKeys() have either broken the code or merely messed up the mocks in the test module. Would you mind taking a look and seeing if anything jumps out at you?
Flags: needinfo?(mhammond)
Assignee

Comment 12

5 years ago
To be specific, the purpose of this patch is to not call FxAccounts.fetchAndUnwrapKeys on B2G when it will never succeed because we don't have a keyFetchToken. The test above triggers getKeys() returning early by deleting kA and kB from its mock, but the mock doesn't have a keyFetchToken. So it's unclear to me if:

  - the test should be deleted
  - the test should mock fetchAndUnwrapKeys() and or/ keyFetchToken differently
  - the test's concurrency handling needs tweaking

If you can give me some guidance on which of these areas to focus on, I'd be much obliged.
Assignee

Comment 13

5 years ago
OK, per Mark's helpful guidance in IRC, I have tweaked the sync test that was breaking and also fixed an unhappy path where a promise was not getting rejected.
Attachment #8407183 - Attachment is obsolete: true
Flags: needinfo?(mhammond)
I've starred all the failures and oranges.  I don't see this patch breaking anything.
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6ec2eefe9692
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
You need to log in before you can comment on or make changes to this bug.