370242 - check HTTP status code to determine auth (and other) failures

Status: RESOLVED FIXED

(Firefox Graveyard :: Microsummaries, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

Currently, MicrosummaryResource determines if a load has failed because the server requested authentication by setting an _authFailed property when nsIAuthPromptProvider, nsIAuthPrompt, or (after the fix for bug 347310) authentication-related nsIPrompt notification callback methods are called by the channel.

As biesi pointed out in bug 347310, comment 14, we could just check the requestSucceeded property of the HTTP channel (XHR.channel.requestSucceeded) or the HTTP response code (XHR.status) to determine if the load failed because it needed authentication, and doing this would also allow us to trap other kinds of failures (like 404 "file not found").

Since MicrosummaryResource only supports http, https, and file schemes at the moment, and the file channel doesn't implement auth (as far as I know), we can just remove the _authfailed flag.

But if we ever want to support additional schemes in the future (f.e. ftp) that do implement authentication, then we should probably leave _authFailed in as an additional mechanism (since requestSucceeded seems HTTP-specific).

Comment 1

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

- Adds status and method properties (I'll be using the latter in 341341) to MicrosummaryResource
- Traps statuses >= 400 and fires off the error callback
- Preprocesses out the license block

Comment 2

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

- Adds status and method properties (I'll be using the latter in 341341) to MicrosummaryResource
- Traps statuses >= 400 and fires off the error callback
- Preprocesses out the license block

Comment 3

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Comment on attachment 275719 [details] [diff] [review]
Patch

Three cheers for Bugzilla!

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 275720 [details] [diff] [review]
Patch

>Index: nsMicrosummaryService.js

>+# Contributor(s):
>+#  Myk Melez <[e-mail]> (Original Author)
>+#  Simon Bünzli <[e-mail]>
>+#  Asaf Romano <[e-mail]>
>+#  Dan Mills <[e-mail]>

Nit: you should add yourself to this list of contributors.


>+          // XXX For now HTTP is the only protocol we handle that might fail
>+          // auth. This message will need to change once we support FTP, which
>+          // returns a status of 0.
>+          LOG(this._self.uri.spec + " load failed; HTTP status: " + this._self.status);

If FTP returns a status of 0 for authentication failures, then we can remove _authFailed, as suggested in comment 0.  Otherwise we should probably leave it in if we plan to support FTP at some point.

This patch looks great, thanks for the fixes!

r=myk

Comment 5

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(In reply to comment #4)
> Nit: you should add yourself to this list of contributors.
Done :)

> If FTP returns a status of 0 for authentication failures, then we can remove
> _authFailed, as suggested in comment 0.  Otherwise we should probably leave it
> in if we plan to support FTP at some point.
> 
FTP returns 0 for all statuses, I've clarified the comment to reflect this.