Closed
Bug 1045738
Opened 10 years ago
Closed 10 years ago
Allow FxA to sign an assertion while offline if its certificate is viable
Categories
(Firefox OS Graveyard :: FxA, defect)
Tracking
(firefox33 wontfix, firefox34 wontfix, firefox35 fixed, b2g-v2.1 verified, b2g-v2.2 verified)
VERIFIED
FIXED
2.1 S7 (24Oct)
People
(Reporter: spenrose, Assigned: spenrose)
References
Details
Attachments
(1 file, 1 obsolete file)
11.98 KB,
patch
|
ferjm
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
The entry point to services/fxaccounts for FxOS immediately rejects if the device is offline: https://github.com/mozilla/gecko-dev/blob/master/services/fxaccounts/FxAccountsManager.jsm#L511 In theory, if the device has an unexpired certificate, it should be able to sign assertions and let the caller decide what to do if it can't reach the verifier. There are sensible cases where it might not care.
Assignee | ||
Comment 1•10 years ago
|
||
Quick status update: I have a patch that probably works. If we want to go for 2.1 uplift, we face a pretty good chance of some regression work.
Assignee | ||
Comment 2•10 years ago
|
||
Given how late we are in the 2.1 cycle and the considerable riskiness of this patch, I am not planning to try to land it for 2.1. Uploading it to avoid losing track of the code.
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8498962 [details] [diff] [review] 1045738-offline-ok.patch I have been encouraged to attempt to land this patch for 2.1. A try build looks good: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ece190d3b0ff Per comment 0, the purpose of this patch is to allow FxOS devices to create assertions while all of (offline, logged in, have unexpired certificate) are true. This will improve UX when, for example, an app calls request() during a transient network outage. The challenge of the patch is making sure that all the unhappy paths are noticed and handled correctly. It first moves the blanket reject-on-offline at [1] to the beginning of any open-UI call [2], all of which require network connectivity, covering most of the unhappy paths. Next in getAccount() we reject if the account is unverified (which would otherwise trigger a network call). If we are verified, we need to ask the shared FxAccounts object for cached account data; it will not hit the network unless the account is unverified and we already stopped that. If that succeeds, we ask FxAccounts for an assertion [3] which requires signing with an unexpired certificate. Chris Karlof informs me that sync deliberately does not try to catch offline errors. If the certificate is expired, I believe that would eventually result in a rejection inside hawkclient.js which would bubble up. I have chosen instead to have FxAccounts test and reject before descending into the networking code, which will change the error path that sync experiences. Mark, could you give me feedback on that choice? Finally, on B2g any rejection path starting when we ask for an assertion for a verified account will run through _handleGetAssertionError(). Fernando, could you tell me what you think of the changes as a whole? Thanks very much! [1] https://github.com/mozilla/gecko-dev/blob/master/services/fxaccounts/FxAccountsManager.jsm#L510 [2] https://github.com/mozilla/gecko-dev/blob/master/services/fxaccounts/FxAccountsManager.jsm#L308 [3] https://github.com/mozilla/gecko-dev/blob/master/services/fxaccounts/FxAccounts.jsm#L478
Attachment #8498962 -
Flags: feedback?(mhammond)
Attachment #8498962 -
Flags: feedback?(ferjmoreno)
Comment 4•10 years ago
|
||
Comment on attachment 8498962 [details] [diff] [review] 1045738-offline-ok.patch Review of attachment 8498962 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/fxaccounts/FxAccounts.jsm @@ +145,5 @@ > if (this.cert && this.cert.validUntil > mustBeValidUntil) { > log.debug(" getCertificate already had one"); > return this.resolve(this.cert.cert); > } > + whitespace ;) @@ +147,5 @@ > return this.resolve(this.cert.cert); > } > + > + if (Services.io.offline) { > + return this.reject(ERROR_OFFLINE); this looks like the wrong thing to reject - FxAccountsManager ends up rejecting with an object. Who catches and handles these exceptions? Tests? ;)
Comment 5•10 years ago
|
||
Comment on attachment 8498962 [details] [diff] [review] 1045738-offline-ok.patch Review of attachment 8498962 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Sam. The overall approach looks ok. Only a few comments. It seems that we don't have any previous test for this case. I believe Services.io.offline can be modified from xpcshell, so it should be easy to add a few tests. ::: services/fxaccounts/FxAccountsManager.jsm @@ +390,5 @@ > // We check first if we have session details cached. > if (this._activeSession) { > // If our cache says that the account is not yet verified, > + // we kick off verification before returning what we have ... > + if (this._activeSession && !this._activeSession.verified) { I know it was already there, but there is no need to check for this._activeSession again as we do in the previous condition. @@ +393,5 @@ > + // we kick off verification before returning what we have ... > + if (this._activeSession && !this._activeSession.verified) { > + if (Services.io.offline) { // ... unless we're offline. > + return this._error(ERROR_OFFLINE); > + } I don't think we want to return here if we are offline. We still can provide the account details even if it is not verified yet. What we can't do is to check the verification status as that requires a network request. Note that we are not waiting for the verification status check to return the account status. It seems that you can safely remove the "!Services.io.offline" condition from [1] and [2] cause we are already checking that at [3]. And it seems that we are missing two early returns at [4] and [5] :(. [1] https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsManager.jsm#391 [2] https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsManager.jsm#410 [3] https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsManager.jsm#463 [4] https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsManager.jsm#453 [5] https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsManager.jsm#464 @@ +516,5 @@ > > let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"] > .getService(Ci.nsIScriptSecurityManager); > let uri = Services.io.newURI(aPrincipal.origin, null, null); > + log.debug("FxAccountsManager.getAssertion() aPrincipal: " + aPrincipal.origin, aPrincipal.appId, aPrincipal.isInBrowserElement); nit: lines shorter than 80 chars, please =)
Attachment #8498962 -
Flags: feedback?(ferjmoreno)
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Mark Hammond [:markh] from comment #4) ... > > + > > + if (Services.io.offline) { > > + return this.reject(ERROR_OFFLINE); > > this looks like the wrong thing to reject - FxAccountsManager ends up > rejecting with an object. Who catches and handles these exceptions? Tests? > ;) Thanks Mark -- I will fix this and add tests.
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #5) ... > And it seems that we are missing two early returns at [4] and [5] :(. ... > [4] > https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/ > FxAccountsManager.jsm#453 > [5] > https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/ > FxAccountsManager.jsm#464 Thanks for the very helpful feedback. I will address your points. The last one I do not quite understand -- should we not reject at those places?
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #5) > Comment on attachment 8498962 [details] [diff] [review] ... > It seems that you can safely remove the "!Services.io.offline" condition > from [1] and [2] cause we are already checking that at [3]. > > And it seems that we are missing two early returns at [4] and [5] :(. > > [1] > https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/ > FxAccountsManager.jsm#391 > [2] > https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/ > FxAccountsManager.jsm#410 > [3] > https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/ > FxAccountsManager.jsm#463 [1] and [2], are in getAccount(), which is called by b2g/components/FxAccountsMgmtService.jsm, which does not test for offline (though there may have been a test in Gaia before the message was sent). getAccount() is also called by signOut(), which is called externally in a few places. I think we should leave them in. As for verificationStatus(), maybe just test at the start of it and return null (not reject) with a warning if offline?
Assignee | ||
Comment 9•10 years ago
|
||
FxAccounts: error object passed (and test added), whitespace fixed. FxAccountsManager: line length and extra this._activeSession fixed. Tests added for happy path, for failure due to certificate expiration, and for failure due to UI needed. Per previous comments, I consolidated the verificationStatus() offline testing in that function. Happy to reconsider. Thanks Fernando!
Attachment #8498962 -
Attachment is obsolete: true
Attachment #8498962 -
Flags: feedback?(mhammond)
Attachment #8501417 -
Flags: review?(ferjmoreno)
Comment 10•10 years ago
|
||
Comment on attachment 8501417 [details] [diff] [review] 1045738-offline-ok.patch Thanks Sam! Looks great. Make sure that you have a try run before pushing, please.
Attachment #8501417 -
Flags: review?(ferjmoreno) → review+
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #10) > Comment on attachment 8501417 [details] [diff] [review] > 1045738-offline-ok.patch > > Thanks Sam! Looks great. Make sure that you have a try run before pushing, > please. Thanks Fernando! https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4e0f9073f40d
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8501417 [details] [diff] [review] 1045738-offline-ok.patch Approval Request Comment [Feature/regressing bug #]: FxA works when offline if possible. [User impact if declined]: Unnecessary FxA (and FMD) breakage. [Describe test coverage new/current, TBPL]: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4e0f9073f40d [Risks and why]: We could have created new and untested corner cases. [String/UUID change made/needed]: N/A
Attachment #8501417 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/72af2eacba65
Flags: in-testsuite+
Keywords: checkin-needed
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/72af2eacba65
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (Oct24)
Comment 15•10 years ago
|
||
Comment on attachment 8501417 [details] [diff] [review] 1045738-offline-ok.patch Sam, Can you please help QA with some test cases around this to verify the patch and cover corner cases? Thanks!
Attachment #8501417 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Flags: needinfo?(spenrose)
Assignee | ||
Comment 16•10 years ago
|
||
The test cases are "add loss of connectivity during existing test cases." 1) If connectivity drops a few minutes after signing in and enabling FMD, FMD should stay enabled. 2) If the user attempts to disable FMD while offline, FMD should not allow it. 3) If the user has NOT enabled FMD and signs out of FXA, that should work (but signing back in should fail until connectivity is reestablished). 4) In the middle of the signin (and password challenge) flow, sudden loss of connectivity should result in acceptable behavior.
Flags: needinfo?(spenrose)
Comment 17•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/cb72824df1d0
status-b2g-v2.1:
--- → fixed
status-b2g-v2.2:
--- → fixed
status-firefox33:
--- → wontfix
status-firefox34:
--- → wontfix
status-firefox35:
--- → fixed
Comment 18•10 years ago
|
||
This issue is verified fixed on Flame 2.1. Result: Following the cases in Comment 16, all expected behaviors were presented. Device: Flame 2.1 (319mb, KK, Shallow Flash) BuildID: 20141111001201 Gaia: 7154f9aa0a69af56c8bd89be0c98d9791449527b Gecko: 452db1a0c9a0 Version: 34.0 (2.1) Firmware Version: v188-1 User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0 =========================================== Unable to verify on Flame 2.2 due to bug 1096603. Leaving verifyme.
Updated•10 years ago
|
Comment 19•10 years ago
|
||
This issue is verified fixed on Flame 2.2. Result: Following the cases in Comment 16, all expected behaviors were presented. Device: Flame 2.2 (319mb, KK, Shallow Flash) BuildID: 20141119040205 Gaia: e64428c5b2dce5db90b75a5055077a04f4bd4819 Gecko: bc2c36dda0a9 Version: 36.0a1 (2.2 Master) Firmware: V188-1 User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in
before you can comment on or make changes to this bug.
Description
•