If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

First use of privileged app w/ FxA triggers redundant refreshAuthentication

REOPENED
Unassigned

Status

Firefox OS
FxA
REOPENED
3 years ago
3 years ago

People

(Reporter: Sam Penrose, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [dependency: marketplace-partners])

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
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)
To add to the Actual flow:
After the second password entry screen, onerror is always fired rather than onlogin.
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
Keywords: regressionwindow-wanted

Updated

3 years ago
Blocks: 1095816
Whiteboard: [dependency: marketplace-partners]
We haven't been seeing this since.  Closing as invalid.  This was on 2.0.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → INVALID
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
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
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
Duplicate of this bug: 1105449
(Reporter)

Comment 7

3 years ago
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().
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.
(Reporter)

Comment 9

3 years ago
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.
(Reporter)

Comment 10

3 years ago
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
Attachment #8530699 - Flags: review?(ferjmoreno)

Comment 11

3 years ago
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
Attachment #8530699 - Flags: review?(ferjmoreno) → feedback+
(Reporter)

Updated

3 years ago
Assignee: spenrose → nobody
You need to log in before you can comment on or make changes to this bug.