Closed Bug 976633 Opened 6 years ago Closed 6 years ago

Clicking Cancel on merge warning should not initiate Firefox Account creation

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 + verified
firefox30 --- verified

People

(Reporter: jgruen, Assigned: ckarlof)

References

Details

Attachments

(3 files, 2 obsolete files)

Current Behavior:
Clicking cancel on merge warning dialogue triggers FxA account creation

Expected Behavior:
Clicking Cancel on merge warning dialogue should close the dialogue and not change user state.

attachments:
account created in error.gif > shows bad behavior
account created in error pt2.gif > shows same account already existing (this === bad)
Blocks: 969593
Blocks: 905997
To be clear, no account should be created, but we do briefly show the wrong UI before "resetting" the tab. Did you get an account confirmation email?

I thought we had an existing bug about this but can't find it now...
Oh, second screenshot suggests yes :/
No longer blocks: 905997
OS: Mac OS X → All
Hardware: x86 → All
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #2)
> To be clear, no account should be created, but we do briefly show the wrong
> UI before "resetting" the tab. Did you get an account confirmation email?

YES, verification email is sent
> 
> I thought we had an existing bug about this but can't find it now...
Assignee: nobody → ckarlof
Priority: -- → P2
To fix this, we obviously need to do this check before we actually create the account.

Proposal: Add an extra about:accounts <-> browser message that triggers this check and returns a success/fail message back to the about:accounts hosted page. If "success" then the page can continue with the creation.

The browser side work would involve factoring out the relink warning to a separate call.
The hosted page work would involve triggering this call before a creation.

I think we can do this without reving the about:accounts <-> browser API version. We just need to land the content server changes first.

Thoughts Gavin, Mark? If this approach is agreeable, I can arrange the necessary FxA work.
Flags: needinfo?(mhammond)
Flags: needinfo?(gavin.sharp)
I see no problem with this approach.
Flags: needinfo?(mhammond)
Flags: needinfo?(gavin.sharp)
I've opened a bug for the required work on the client side: https://github.com/mozilla/fxa-content-server/issues/682
Priority: P2 → P1
Attachment #8390947 - Flags: feedback?(mhammond)
Hosted code WIP at: https://github.com/mozilla/fxa-content-server/pull/726

I still need to investigate how to modify/add tests for this.
Attached patch 0001-WIP-for-976633.patch (obsolete) — Splinter Review
Attachment #8390949 - Flags: feedback?(mhammond)
Attachment #8390947 - Attachment is obsolete: true
Attachment #8390947 - Flags: feedback?(mhammond)
Comment on attachment 8390949 [details] [diff] [review]
0001-WIP-for-976633.patch

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

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +81,5 @@
>    return pressed == 0; // 0 is the "continue" button
>  }
>  
> +function shouldCancelRelink(acctName) {
> +  return !!(needRelinkWarning(acctName) && !promptForRelink(acctName));

I think it is safe to drop the !! here

@@ +142,5 @@
>      // but it's a little more seamless to do here, and sync is currently the
>      // only fxa consumer, so...
>      let newAccountEmail = accountData.email;
> +    // The hosted code may have already checked for the relink situation
> +    // by sending the maybe_show_relink_warning command. If it did, then

I'd like to see "maybe_show_relink_warning" renamed to something like "will_accept_new_account"

@@ +144,5 @@
>      let newAccountEmail = accountData.email;
> +    // The hosted code may have already checked for the relink situation
> +    // by sending the maybe_show_relink_warning command. If it did, then
> +    // it will indicate we don't need to ask twice.
> +    if (!accountData.skipRelinkWarning && shouldCancelRelink(newAccountEmail)) {

and maybe "skipRelinkWarning" to "verifiedWillAcceptNewAccount"?

@@ +183,5 @@
>      );
>    },
>  
> +  onMaybeShowRelinkWarning: function(accountData) {
> +    // If the last fxa account used for sync isn't this account, we display

Instead of duplicating this comment, I'd suggest moving it to the shouldCancelRelink function, and add a single line comment here, and at the other place shouldCancelRelink is called, saying something like "We need to confirm a relink - see shouldCancelRelink for more".  Or even just remove the duplicated comment here.

But not a big deal if that ends up looking even clunkier.
Attachment #8390949 - Flags: feedback?(mhammond) → feedback+
Attachment #8391570 - Flags: review?(mhammond)
Comment on attachment 8391570 [details] [diff] [review]
0001-Bug-976633-Clicking-Cancel-on-merge-warning-should-n.patch

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

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +150,4 @@
>        // we need to tell the page we successfully received the message, but
>        // then bail without telling fxAccounts
>        this.injectData("message", { status: "login" });
>        // and reload the page or else it remains in a "signed in" state.

this comment is now stale

@@ +186,5 @@
>  
> +  onCanLinkAccount: function(accountData) {
> +    // We need to confirm a relink - see shouldCancelRelink for more
> +    let cancel = shouldCancelRelink(accountData.email);
> +    this.injectData("message", { status: "can_link_account", data: { cancel: cancel } });

It kinda sucks that shouldCancelRelink is named that way - it should be shouldAllowRelink() - but no need to propagate that to this new method - so can we change the 'cancel' value to, say 'ok: !cancel'?  Or maybe better:

  let ok = !shouldCancelRelink...
   .... {ok: ok}
Attachment #8391570 - Flags: review?(mhammond) → review+
https://hg.mozilla.org/mozilla-central/rev/8aa53970769c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Comment on attachment 8391570 [details] [diff] [review]
0001-Bug-976633-Clicking-Cancel-on-merge-warning-should-n.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): When user is in account creation flow for Sync and cancels out during merge warning, it creates an account for user anyway
User impact if declined: Users who cancel will have account created anyway
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch:
Attachment #8391570 - Flags: approval-mozilla-aurora?
Comment on attachment 8391570 [details] [diff] [review]
0001-Bug-976633-Clicking-Cancel-on-merge-warning-should-n.patch

We merged aurora => beta. So, I guess that applies to beta too.
Attachment #8391570 - Flags: approval-mozilla-beta+
Attachment #8391570 - Flags: approval-mozilla-aurora?
Attachment #8391570 - Flags: approval-mozilla-aurora+
This is still broken as reported on latest Nightly (20140319). On Cancel to the merge warning, I receive confirmation email and the account is setup.  Is there additional client work that is supposed to be done here?

question: Is it intended that you can sign in via the set up process?  (because you can as long as you have the account and password correct)
The hosted code in about:accounts needs to be modified to support this patch. WIP here: https://github.com/mozilla/fxa-content-server/pull/726 

train-07 at the earliest.
I agree the tracking is confusing here. We should have split this into two different Bugzilla bugs. One for the Desktop portion and one for the hosted portion.
Upon sync data merge cancel, the user is no longer redirected to the account creation form - the get started page is displayed instead. 

Unfortunately, on the latest Beta (Build ID: 20140324101726) the account is still created, a confirmation email is received. The latest Aurora (Build ID: 20140325004002) and the latest Nightly (Build ID: 20140325030201) are not affected by this issue.

I'm also a bit confused about what this bug should actually fix, based on Comment 18 and Comment 20. Chris, could you please clarify this?
Flags: needinfo?(ckarlof)
Andrei, there is a server side fix to put the finishing touches on this behavior. That fix may be riding the next services train-07.  I have my eye on this bug.  :-)
Flags: needinfo?(ckarlof)
The server fix missed train-07. I'm pretty sure that Nightly and Aurora are also still affected.

I view this as a low impact bug. It will only impact users creating an account in a profile that was previously connected to FxA sync and then deciding to cancel.
Keywords: verifyme
QA Contact: twalker
This bug is verified fixed on Firefox 29 (Build ID: 20140421221237) and Aurora 30 (2014-04-28, 20140428004000), using:
 * Windows 7 64-bit [1],
 * Ubuntu 13.10 32-bit [2],
 * Mac OS X 10.9 [3].

1. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
2. Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0
3. Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.