Closed
Bug 1095752
Opened 11 years ago
Closed 7 years ago
First use of privileged app w/ FxA triggers redundant refreshAuthentication
Categories
(Firefox OS Graveyard :: FxA, defect)
Firefox OS Graveyard
FxA
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: spenrose, Unassigned)
References
Details
(Whiteboard: [dependency: marketplace-partners])
Attachments
(2 files)
985 bytes,
application/zip
|
Details | |
3.68 KB,
patch
|
ferjm
:
feedback+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
To add to the Actual flow:
After the second password entry screen, onerror is always fired rather than onlogin.
Comment 2•11 years ago
|
||
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•11 years ago
|
Whiteboard: [dependency: marketplace-partners]
Comment 3•11 years ago
|
||
We haven't been seeing this since. Closing as invalid. This was on 2.0.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID
Comment 4•11 years ago
|
||
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 → ---
Comment 5•11 years ago
|
||
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
Reporter | ||
Comment 7•11 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().
Comment 8•11 years ago
|
||
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•11 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•11 years ago
|
||
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•11 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•10 years ago
|
Assignee: spenrose → nobody
Comment 12•7 years ago
|
||
Firefox OS is not being worked on
Status: REOPENED → RESOLVED
Closed: 11 years ago → 7 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•