Closed Bug 1045738 Opened 5 years ago Closed 5 years ago

Allow FxA to sign an assertion while offline if its certificate is viable

Categories

(Firefox OS Graveyard :: FxA, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox33 wontfix, firefox34 wontfix, firefox35 fixed, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S7 (24Oct)
Tracking Status
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 --- fixed
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: spenrose, Assigned: spenrose)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Attached patch 1045738-offline-ok.patch (obsolete) — Splinter Review
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.
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 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 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)
(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.
(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?
(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?
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 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+
(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
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?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/72af2eacba65
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (Oct24)
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+
Keywords: verifyme
Flags: needinfo?(spenrose)
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)
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.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Keywords: verifyme
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)
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.