Create debugging pref that forces Sync to fetch a new token for every Sync request

RESOLVED FIXED in Firefox 33

Status

Cloud Services
Firefox Sync: Backend
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ckarlof, Assigned: ckarlof)

Tracking

unspecified
mozilla34
Points:
---

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed, firefox35 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
This will make debugging issues like bug 1056523, which involve password change or reset, easier. After you change or reset your password, you have to wait until certain cached credentials expire (the token and signed BID public key) before you'll be asked to "reconnect to sync". I suggest we add a pref that says "don't cache sync authentication credentials".
(Assignee)

Comment 1

4 years ago
Created attachment 8480109 [details] [diff] [review]
0001-Bug-1059391-Add-pref-to-disable-caching-of-Sync-auth.patch
Attachment #8480109 - Flags: feedback?(mhammond)
(Assignee)

Comment 2

4 years ago
Created attachment 8480116 [details] [diff] [review]
0001-Bug-1059391-Add-pref-to-disable-caching-of-Sync-auth.patch
Attachment #8480116 - Flags: feedback?(mhammond)
(Assignee)

Updated

4 years ago
Attachment #8480109 - Attachment is obsolete: true
Attachment #8480109 - Flags: feedback?(mhammond)
(Assignee)

Comment 3

4 years ago
Comment on attachment 8480116 [details] [diff] [review]
0001-Bug-1059391-Add-pref-to-disable-caching-of-Sync-auth.patch

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

::: services/fxaccounts/FxAccounts.jsm
@@ +162,5 @@
>      ).then(result => this.resolve(result));
>    },
>  
>    getKeyPair: function(mustBeValidUntil) {
> +    let useCachedAuthCredentials = Services.prefs.getBoolPref("services.sync.debug.useCachedAuthCredentials");

Mark, this line is causing a failure in the tests:

0:10.30 TEST-UNEXPECTED-FAIL | /Users/Chris/git/gecko-dev/testing/xpcshell/head.js | Unexpected exception NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref] - See following stack

I get it when I run test_accounts.js: 
% ./mach xpcshell-test services/fxaccounts/tests/xpcshell/test_accounts.js

The rest of the tests seem to pass and manual testing of the pref seems to work for me.

Any ideas?
Comment on attachment 8480109 [details] [diff] [review]
0001-Bug-1059391-Add-pref-to-disable-caching-of-Sync-auth.patch

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

This looks like a good idea, but I'd be inclined to negate the pref (ie, ...debug.skipCachedAuthCredentials), not bother trying to create a default value for it, and wrap accesses around a try/catch (the negating means the missing pref gets treated as though the pref is false)
Attachment #8480109 - Flags: feedback+
(Assignee)

Comment 5

4 years ago
(In reply to Mark Hammond [:markh] from comment #4) 
> This looks like a good idea, but I'd be inclined to negate the pref (ie,
> ...debug.skipCachedAuthCredentials), not bother trying to create a default
> value for it, and wrap accesses around a try/catch (the negating means the
> missing pref gets treated as though the pref is false)

Yeah, I started with that, but it can lead to double negatives in the code, which is potentially confusing, but I'll go with your approach.

Thanks!
(Assignee)

Updated

4 years ago
Assignee: nobody → ckarlof
(Assignee)

Comment 6

4 years ago
Created attachment 8480721 [details] [diff] [review]
0001-Bug-1059391-Add-pref-to-disable-caching-of-Sync-auth.patch
Attachment #8480116 - Attachment is obsolete: true
Attachment #8480116 - Flags: feedback?(mhammond)
Attachment #8480721 - Flags: review?(mhammond)
(Assignee)

Comment 7

4 years ago
Manual testing done, tests pass locally, and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=e8793b2c4e85
:ckarlof do you have somebody from one of the QA teams to test this Try build?
Status: NEW → ASSIGNED
Whiteboard: [qa+]
(Assignee)

Comment 9

4 years ago
james, no I don't. This patch itself doesn't need testing, but this try build (in particular, setting the debugging flag) would make testing bug 1056523 much easier.
Comment on attachment 8480721 [details] [diff] [review]
0001-Bug-1059391-Add-pref-to-disable-caching-of-Sync-auth.patch

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

::: services/fxaccounts/FxAccounts.jsm
@@ +163,5 @@
>    },
>  
>    getKeyPair: function(mustBeValidUntil) {
> +    // If the debugging pref to ignore cached authentication credentials is set for Sync,
> +    // then don't any cached key pair, i.e., generate a new one and get it signed.

"then don't any" - I think you probably meant to add "use" in there.

@@ +168,5 @@
> +    // The purpose of this pref is to expedite any auth errors as the result of a
> +    // expired or revoked FxA session token, e.g., from resetting or changing the FxA
> +    // password.
> +    let ignoreCachedAuthCredentials = false;
> +    try { ignoreCachedAuthCredentials = Services.prefs.getBoolPref("services.sync.debug.ignoreCachedAuthCredentials"); } catch(e) {}

probably not worth trying to save a few lines here - ie, please just indent as normal (and ditton in bid_identity)
Attachment #8480721 - Flags: review?(mhammond) → review+
(Assignee)

Comment 11

4 years ago
Created attachment 8480969 [details] [diff] [review]
0001-Bug-1059391-Add-pref-to-disable-caching-of-Sync-auth.patch

r=markh from previous patch
Attachment #8480721 - Attachment is obsolete: true
Attachment #8480969 - Flags: review+
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

4 years ago
:markh tells me I shouldn't mark as fixed until it's merged to m-c. Sorry!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

4 years ago
Status: REOPENED → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/18563fc91735
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
[Tracking Requested - why for this release]:

Can this be pushed to other channels like beta (I think it has arrived in Aurora - if not then aurora as well), it would help for testing.
tracking-firefox33: --- → ?
We're not going to track, but please go ahead and request patch approvals.
tracking-firefox33: ? → ---
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #16)
> We're not going to track, but please go ahead and request patch approvals.
Chris, can you please take care of this? I`m not familiar with the process and I don`t even think I have the rights to do that.
Flags: needinfo?(ckarlof)
(Assignee)

Comment 18

4 years ago
Comment on attachment 8480969 [details] [diff] [review]
0001-Bug-1059391-Add-pref-to-disable-caching-of-Sync-auth.patch

Approval Request Comment
[Feature/regressing bug #]: Add support for Sync debugging pref.
[User impact if declined]: It will make testing sync password reset bugs harder.
[Describe test coverage new/current, TBPL]:
[Risks and why]: None.
[String/UUID change made/needed]:
Attachment #8480969 - Flags: approval-mozilla-beta?
Flags: needinfo?(ckarlof)
status-firefox33: --- → affected
status-firefox34: --- → fixed
status-firefox35: --- → fixed
Attachment #8480969 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Removing [qa+] based on comment 9.
Whiteboard: [qa+]
You need to log in before you can comment on or make changes to this bug.