758237 - Account Provisioner tab should close and respawn AP dialog if provider XML is corrupt or returns error

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

STR:

1)  Go to File > New > Get a New Mail Account
2)  Make sure Hover.com is selected, and fill in a name
3)  Choose an address and fill in the form
4)  If there is no credit card field, press "Continue", and the field should reveal itself. Fill the credit card field with a valid credit card number  (it will not be charged) and press "Continue".  
5)  Choose to "Cancel this order"

What happens?

A big ugly XML page is displayed to the user.

What's expected?

The tab should close, and we should go back to the Account Provisioner dialog.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

If the provider returns corrupt XML, or XML that we cannot construct an account out of, this patch closes the tab and respawns the Account Provisioner dialog.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

patching file mail/test/mozmill/newmailaccount/test-newmailaccount.js
Hunk #2 FAILED at 618
Hunk #3 FAILED at 911
Hunk #4 FAILED at 981
3 out of 4 hunks FAILED -- saving rejects to file mail/test/mozmill/newmailaccount/test-newmailaccount.js.rej
patch failed, unable to continue (try -v)

:(

Hand-patching now; review coming soon.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #2)
> patching file mail/test/mozmill/newmailaccount/test-newmailaccount.js
> Hunk #2 FAILED at 618
> Hunk #3 FAILED at 911
> Hunk #4 FAILED at 981
> 3 out of 4 hunks FAILED -- saving rejects to file
> mail/test/mozmill/newmailaccount/test-newmailaccount.js.rej
> patch failed, unable to continue (try -v)
> 
> :(
> 
> Hand-patching now; review coming soon.

Yikes - I should have mentioned, this patch relies on the patch in bug 757823. Should apply cleanly on top of it (you're conflict was likely in the tests.)

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 626835 [details] [diff] [review]
Patch v1

Okay, I tried the steps to reproduce, and it looks much better now.  ui-r=me.

>+++ b/mail/components/newmailaccount/content/uriListener.js
>@@ -196,53 +196,56 @@ TracingListener.prototype = {
>       let accountConfig = accountCreationFuncs.readFromXML(xml);
>       accountCreationFuncs.replaceVariables(accountConfig,
>+                                            this.params.realName,
>+                                            this.params.email);
>+      account = accountCreationFuncs.createAccountInBackend(accountConfig);
>+      success = true;

I wonder if we should also check some properties on the account, or if we know that the createAccountInBackend will always throw if it can't create the account?

>     } catch (e) {
>       // Something went wrong.  Right now, we just dump the problem out
>       // to the Error Console.  We should really do something smarter and
>       // more user-facing, because if - for example - a provider passes
>       // some bogus XML, this routine silently fails.

Heh.  Prescient comment.  Now that we're doing something more, perhaps we can make the comment more appropriate?

>+++ b/mail/test/mozmill/newmailaccount/html/configError.xml
>@@ -0,0 +1,6 @@
>+<clientConfig version="1.1">
>+  <emailProvider id="%DOMAIN%"/>
>+  <error code="USER_CANCEL">
>+    You have cancelled your order.
>+  </error>
>+</clientConfig>

Should we test the other error codes, or will they all be handled the same?  (I'm pretty sure they'll all be handled the same for now.  Maybe fix the handling in v2?)

So r=me with those questions answered/nits fixed.

Thanks,
Blake.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #4)
> Comment on attachment 626835 [details] [diff] [review]
> Patch v1
> 
> Okay, I tried the steps to reproduce, and it looks much better now.  ui-r=me.

Cool, thanks.

> 
> >+++ b/mail/components/newmailaccount/content/uriListener.js
> >@@ -196,53 +196,56 @@ TracingListener.prototype = {
> >       let accountConfig = accountCreationFuncs.readFromXML(xml);
> >       accountCreationFuncs.replaceVariables(accountConfig,
> >+                                            this.params.realName,
> >+                                            this.params.email);
> >+      account = accountCreationFuncs.createAccountInBackend(accountConfig);
> >+      success = true;
> 
> I wonder if we should also check some properties on the account, or if we
> know that the createAccountInBackend will always throw if it can't create
> the account?

We do have a tool for checking configuration (http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/verifyConfig.js#65). This function asynchronously tries to log-in with the credentials given back with the XML to check their validity.

We could use that, but what should be the behaviour if the transaction completed, and the XML that was given back was valid, but the account cannot be logged into? What should TB do in this case?

> 
> >     } catch (e) {
> >       // Something went wrong.  Right now, we just dump the problem out
> >       // to the Error Console.  We should really do something smarter and
> >       // more user-facing, because if - for example - a provider passes
> >       // some bogus XML, this routine silently fails.
> 
> Heh.  Prescient comment.  Now that we're doing something more, perhaps we
> can make the comment more appropriate?

Agreed.

> 
> >+++ b/mail/test/mozmill/newmailaccount/html/configError.xml
> >@@ -0,0 +1,6 @@
> >+<clientConfig version="1.1">
> >+  <emailProvider id="%DOMAIN%"/>
> >+  <error code="USER_CANCEL">
> >+    You have cancelled your order.
> >+  </error>
> >+</clientConfig>
> 
> Should we test the other error codes, or will they all be handled the same? 
> (I'm pretty sure they'll all be handled the same for now.  Maybe fix the
> handling in v2?)

If the XML cannot be turned into incoming/outgoing servers, we close the tab and re-open the provisioner. Not the greatest situation, but we're past string freeze. :/

I'll file a bug to fix that handling.

> 
> So r=me with those questions answered/nits fixed.

Thanks for the review!

Comment 6

[Mike Conley (:mconley) (:⚙️)]

So the createAccountInBackend will throw if it attempts to reach for something that doesn't exist (like config.incoming.port, if incoming doesn't exist).

However, if incoming is provided but port is missing, I'm not sure what kind of protections we have against that sort of thing.

David, do you know?

Comment 7

[David :Bienvenu]

(In reply to Mike Conley (:mconley) from comment #6)
> So the createAccountInBackend will throw if it attempts to reach for
> something that doesn't exist (like config.incoming.port, if incoming doesn't
> exist).
> 
> However, if incoming is provided but port is missing, I'm not sure what kind
> of protections we have against that sort of thing.
> 
> David, do you know?

I don't. I believe the mail backend code will treat a missing port as meaning to use the default port for the protocol and connection type. But if we're really worried about it, we should verify it. I'm also not sure what the account autoconfig code will do with a missing port (e.g., will it create an incoming server in the backend?)

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Ok, so from what I can tell, readFromXML does the validation that bwinton is talking about, so I think we're covered. BenB can correct me if I'm wrong.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2 (for comm-beta and comm-aurora) - carrying forward ui-r+ / r+ from bwinton.

Thanks Blake - made the changes you suggested, we're safe via readFromXML, and I filed bug 758700.

I had to backport this for beta, so here's the beta patch.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 627297 [details] [diff] [review]
Patch v2 (for comm-beta and comm-aurora) - carrying forward ui-r+ / r+ from bwinton.

Looks like backport is required for both beta and aurora.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2 (for comm-central) - carrying forward ui-r+ / r+ from bwinton

Comment 12

[Mike Conley (:mconley) (:⚙️)]

comm-central: https://hg.mozilla.org/comm-central/rev/539dc131baf1

Comment 13

[Mike Conley (:mconley) (:⚙️)]

comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/f2e688291060

Comment 14

[Mike Conley (:mconley) (:⚙️)]

comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/f875a2e01339