1693883 - account provisioner doesn't handle utf-8 account configuration data

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Magnus Melin [:mkmelin]]

The account provisioner doesn't handle UTF-8 data in the configuration data.

There's also a few other minor fixes I'll attach to this bug.

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: bug1693883_contenttype_null.patch

Just cleanup to avoid unwarranted errors in the console. Doesn't change anything.

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: bug1693883_gMessageListeners.patch

I'm not sure exactly what's wrong with these as is. After setup there were (sometimes?) errors that "gMessageListeners[index].onStartHeaders is not a function". After I changed to for..of I don't see it, and it's easier to read this way.

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: bug1693883_utf8_provisioner.patch

UTF-8 e.g. in the password set by the provider was not working.
Some other buggy things cleared up as well:

• outgoing password was incorrectly set to incoming password
• config.password isn't a thing

• added a test for the successful account setup case which strangely doesn't seem like it was tested even if the unsuccessful cases were thoroughly tested.

Comment 4

[Alessandro Castellani [:aleca]]

Comment on attachment 9204286 [details] [diff] [review]
bug1693883_contenttype_null.patch

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

Looks good.

Comment 5

[Alessandro Castellani [:aleca]]

Comment on attachment 9204288 [details] [diff] [review]
bug1693883_gMessageListeners.patch

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

This is good.
I had similar issues in the past when the loop expects an index.
I'm not sure if the issue applies here, but I found out that sometimes the index didn't actually match the expected object. e.g.: index 0 was created empty therefore there's no actual listener available in that position.

Comment 6

[Alessandro Castellani [:aleca]]

Comment on attachment 9204290 [details] [diff] [review]
bug1693883_utf8_provisioner.patch

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

This is good, thanks.

::: mail/test/browser/newmailaccount/browser_newmailaccount.js
@@ +1178,5 @@
> +  );
> +
> +  Services.logins.removeAllLogins();
> +
> +  // TODO: check that the folder pane is now visible

Good stuff. I'll take care of this in bug 1689724

::: mail/test/browser/newmailaccount/html/config.xml
@@ +5,2 @@
>      <incomingServer type="imap">
> +      <hostname>imap-provisioned.%EMAILDOMAIN%</hostname>

Would we need to let the providers we have know that we're changing the XML config?

Comment 7

[Magnus Melin [:mkmelin]]

(In reply to Alessandro Castellani (:aleca) from comment #6)

Would we need to let the providers we have know that we're changing the XML
config?

I' only changed test data here. The test data was changed to read "imap-provisioned" because imap was hard to find, and the other test case threw me off for a while.
displayShortName is not used anywhere (by our code).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c241aae62d26
ignore responses that don't have contentType set (like for OPTIONS). r=aleca
https://hg.mozilla.org/comm-central/rev/1552e6e26e18
use for .. of loops for gMessageListeners. r=aleca
https://hg.mozilla.org/comm-central/rev/92ace0642be5
account provisioner doesn't handle utf-8 account configuration data. r=aleca

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9204290 [details] [diff] [review]
bug1693883_utf8_provisioner.patch

[Approval Request Comment]
User impact if declined: can end up with bad account configuration through account provisioner (mainly if using non-ascii in password), and for one of the cases: missing header pane for first account of profile, until restart.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): not too risky, adds a test

All three changesets.

Comment 10

[Wayne Mery (:wsmwk)]

Comment on attachment 9204290 [details] [diff] [review]
bug1693883_utf8_provisioner.patch

[Triage Comment]
approved for beta - all three patches

Comment 11

[Rob Lemley [:rjl]]

Thunderbird 87.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/94530b012ea1
https://hg.mozilla.org/releases/comm-beta/rev/6613590cabcb
https://hg.mozilla.org/releases/comm-beta/rev/ad4d383ac29b

Comment 12

[Wayne Mery (:wsmwk)]

Comment on attachment 9204290 [details] [diff] [review]
bug1693883_utf8_provisioner.patch

[Triage Comment]
Approved for esr78

All three changesets

Comment 13

[Rob Lemley [:rjl]]

Thunderbird 78.8.1:
https://hg.mozilla.org/releases/comm-esr78/rev/dd4919d726c8
https://hg.mozilla.org/releases/comm-esr78/rev/85af39f4b95c
https://hg.mozilla.org/releases/comm-esr78/rev/043a92dd3ae0