1907443 - Exchange autodiscover authentication failures no longer handled correctly [regression]

Status: RESOLVED DUPLICATE of bug 1951827

(Thunderbird :: Account Manager, defect)

Description

[[e-mail]]

As of bug 1397646, FetchHTTP can't reliably use statusText any more as a means of detecting network failures, so if the autodiscover server doesn't provide any status text (e.g. because it uses HTTP/2), FetchHTTP now misidentifies these as network failures.

Furthermore, the code path changes caused by bug 1859656 prevent the authentication error from interrupting the autodiscover process like it used to.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[Ben Bucksch (:BenB)]

Thanks for the good patch. The new code is easier to read and more reliable.

Comment 3

[Ben Bucksch (:BenB)]

Just a suggestion (not required): Should we use an exception class ExchangeUsernameException like in bug 1907130, to be more explicit about the code path?

Comment 4

[[e-mail]]

Attachment: Addressed review comments

Comment 5

[[e-mail]]

Attachment: Thunderbird 115 backport

Comment 6

[Ben Bucksch (:BenB)]

Comment on attachment 9413036 [details] [diff] [review]
Addressed review comments

r=BenB

Comment 7

[Ben Bucksch (:BenB)]

The problem is worse in TB 115 and the fix needed even more urgently in TB 115 than on trunk. See bug 1907130.

Comment 8

[Alessandro Castellani [:aleca]]

Thanks for the report and the suggested patches.
Could you please upload them to Phabricator? We don't handle bugzilla patches anymore.

I'm curious, how is this issue present in 115 if the regressors landed in 118 and 126?
Would it be possible to identify only a single regressor? That would be more accurate.

Comment 9

[[e-mail]]

(In reply to Alessandro Castellani from comment #8)

I'm curious, how is this issue present in 115 if the regressors landed in 118 and 126?

Bug 1907130 is only present in 115, but the fix happens to be equivalent to a backport of this fix.

Comment 10

[Ben Bucksch (:BenB)]

Good question. It's complicated, so I wanted to spare you the details. Happy to explain.

The 115 problem was a race condition which has always been present, but was made very apparent and common only last year, when Office365 disabled Basic auth, which made certain HTTP calls always fail with a 401 (and apparently fail very fast, faster than the ISPDB responds) and triggered this case far more often.

In TB 128, that race does not actually happen, by coincidence, because the new OAuth2 prompt during AutoDiscover waits for user input and blocked the racing function. Nonetheless, we submit the code change for trunk as well, because a) the fix makes the code both more reliable and b) more readable, and c) to allow easier testing of the change on trunk.

In TB 128, there is another regression caused by bug 1397646, which changed the implementation so that XMLHttpRequest.statusText now returns nothing for HTTP2 calls, and this change caused our code to fail. This problem is fixed only in the TB 128 patch and does not apply to TB 115, so we left that part out of the TB 115 patch.

Comment 11

[Alessandro Castellani [:aleca]]

Thanks for the explanation.
I'm gonna mark Bug 1907130 as duplicate of this one since they're basically tackling the same thing.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9413036 [details] [diff] [review]
Addressed review comments

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

::: mail/components/accountcreation/content/accountSetup.js
@@ +829,5 @@
>    },
>  
> +  _showExchangeUsername() {
> +    this.onStartOver();
> +    this.stopLoadingState(); // clears status message

(Like :aleca wrote, please submit using phabricator). 
Anyway, these two lines seems not to belong in a function that should show the username.

Comment 14

[[e-mail]]

Attachment: Bug 1907443 - Exchange autodiscover authentication failures no longer handled correctly. r=benb

Comment 15

[Ben Bucksch (:BenB)]

these two lines seems not to belong in a function that should show the username.

They belong there where they are, because they set the dialog in the state to show the username, be consistent with the state, and re-run the autoconfig after that. Without these lines, the dialog display is in an inconsistant state. Every caller who calls this showExchangeUsername() function must call these lines.

Comment 16

[Ben Bucksch (:BenB)]

This bug here fixed 2 related, but separate bugs at the same time. On top, the symptom on TB 115 and TB 128 was also different. This caused confusion for the reviewers, causing a stalemate. The patches and fixes were correct, but never reviewed, due to that confusion. To solve that, I now filed 2 separate bugs: bug 1951827 and bug 1951824.