1095752 - First use of privileged app w/ FxA triggers redundant refreshAuthentication

Status: REOPENED

Description

[Sam Penrose]

Created attachment 8519225 [details]
test_case.zip

STR:

1) Install a privileged app such as attached test_case.zip into an affected build of FxOS.
2) While not logged into FxA, open the app. The app will (correctly), invoke FxA's login flow.
3) Sign in with a verified account.

Expected: app displays white screen with some fxa events logged.

4) Actual: FxA throws up second password entry screen.

Builds:
  - seen on 20141106000201 (Flame)
  - not seen on v188/20141016183911 (Flame)

Comment 1

[(not reading bugmail) Nick Desaulniers [:\n]]

To add to the Actual flow:
After the second password entry screen, onerror is always fired rather than onlogin.

Comment 2

[Joshua Mitchell (Inactive)]

Possible Dupe of bug 1052267

Also: Please see  https://bugzilla.mozilla.org/show_bug.cgi?id=1095744#c0 as an example of how to format a bug - you are missing some very important key information - namely what branch you are testing on

Also: We do not do regression windows on non-blocking bugs - please see https://wiki.mozilla.org/B2G/QA/Triage#Summary_of_Keywords_and_Flags for information on when to add this tag

Comment 3

[(not reading bugmail) Nick Desaulniers [:\n]]

We haven't been seeing this since.  Closing as invalid.  This was on 2.0.

Comment 4

[(not reading bugmail) Nick Desaulniers [:\n]]

Nevermind.  Seeing this today, with the test case after the call to navigator.mozId.request.

Serial: 7f86ca16 (State: device)
Build ID               20141118000207
Gaia Revision          1ede2666f1e6c1b3fd3b282011caf0cbc59544b0
Gaia Date              2014-11-17 17:03:27
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/2bea026d4f86
Gecko Version          32.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20141118.035009
Firmware Date          Tue Nov 18 03:50:19 EST 2014
Bootloader             L1TC00011880

Comment 5

[:Harald Kirschner :digitarald]

I just had the same behaviour testing WMW:

Serial: e444da0c (State: device)
Build ID               20141120000206
Gaia Revision          1ede2666f1e6c1b3fd3b282011caf0cbc59544b0
Gaia Date              2014-11-17 17:03:27
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/54f1b0ee07a6
Gecko Version          32.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20141120.034342
Firmware Date          Thu Nov 20 03:43:52 EST 2014
Bootloader             L1TC10011880

Comment 7

[Sam Penrose]

The bug essentially amounts to "FxAccountsManager.getAssertion(), which is what mozId.request() boils down to, was called a second time with the same principal (app) and reached here:

  https://github.com/mozilla/gecko-dev/blob/master/services/fxaccounts/FxAccountsManager.jsm#L559

before the first call reached here:

  https://github.com/mozilla/gecko-dev/blob/master/services/fxaccounts/FxAccountsManager.jsm#L345

Nick observes in IRC that one factor may be whether the first call was from a verified account. If it wasn't, FxAccountsManager would never call _getAssertion, which means the app would never get added to the security manager. The fix would then be to move the call to _addPermission from _getAssertion to the result block in _uiRequest().

Comment 8

[(not reading bugmail) Nick Desaulniers [:\n]]

I was just able to repro this twice in a row.

STR:
1. Sign in to an app with an unverified account.  Do not verify.  Do not close the app.
2. Go to settings, FxA, sign out.  Do not close the app from step 1.
3. Return to app from step 1.
4. Enter email and password to a previously verified email address.

Expected:
success screen, onlogin to fire

Actual:
Second PW prompt and then onerror fires.  onlogin fires immediately if request is called again, without email/username prompt.

Comment 9

[Sam Penrose]

I've got a patch following up on comment #7 (plus a new unit test) in flight on Treeherder; if it passes I'll ask for review.

Comment 10

[Sam Penrose]

Created attachment 8530699 [details] [diff] [review]
1095752-move-addPermission.patch

Fernando, this patch closely the hole that Nick noticed, where a user who signs into FxA via a privileged app with an unverified account will not have their granting of FxA permission to that app recorded. I also added a small test, and Treeherder looks good. Thanks for your time, and no hurry on this one.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=04cf991f267e

Comment 11

[On parental leave]

Comment on attachment 8530699 [details] [diff] [review]
1095752-move-addPermission.patch

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

Thank you Sam! Nice catch. The patch looks good, but I'd like to try a different approach for the test if it is possible. Thanks!

::: services/fxaccounts/FxAccountsManager.jsm
@@ +321,5 @@
>        result => {
> +        if (result && aPrincipal) {
> +          this._addPermission(aPrincipal);
> +        }
> +        // Only return assertions for verified accounts.

The old comment was also a good explanation worth keeping :)

::: services/fxaccounts/tests/xpcshell/test_manager.js
@@ +174,5 @@
>  
> +FxAccountsManager._addPermissionCalled = false;
> +FxAccountsManager._addPermission = function() {
> +  this._addPermissionCalled = true;
> +}

We are testing FxAccountsManager here, so we should try not to mock its functionality whenever it's possible. I believe we can play with the PermissionManager here and see if we added the permission or not. We already do this in other tests like [1] or [2].

[1] https://github.com/mozilla/gecko-dev/blob/master/services/fxaccounts/tests/xpcshell/test_manager.js#L525
[2] https://github.com/mozilla/gecko-dev/blob/master/services/fxaccounts/tests/xpcshell/test_manager.js#L568