401 authorization UI then restart request/save successful auth creds

[DDEFISHER]

Step 7 of the NCSU student project Implement HTTP authorization UI

make an authorization UI appear when a 401 HTTP response is received (StatusCode::Unauthorized) - in load in http_loader.rs, right before trying to process an HTTP redirection, use the new tinyfiledialogs library to make two prompts appear (username and password), then restart the request with the new authorization value present applied. If an authorization value was present and the response is successful, add the credentials to the authorization cache.

---

This change is

[highfive]

Heads up! This PR modifies the following files:

• @KiChjang: components/net/Cargo.toml, components/net/lib.rs, components/net/http_loader.rs

[highfive]

Warning

• These commits modify net code, but no tests are modified. Please consider adding a test!

[jdm]

Great! I've suggested some ways to restructure the code to make it testable as well as avoid potentially including authorization headers when they aren't necessary.
-S-awaiting-review +S-needs-code-changes +S-tests-failed

---

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

---

vi, line 0 [r1] (raw file):
Remove this file, please.

---

components/net/http_loader.rs, line 690 [r1] (raw file):
Let's add a UIProvider trait that has a single method that duplicates the API for tinyfiledialogs::input_box. Then we can write:

fn load<A,B>(mut load_data: LoadData,
             ui_provider: &B,
             ...)
             -> ... where B: UIProvider

and write a unit test that verifies that this code works. load_for_consumer can use load::<WrappedHttpRequest, TFDProvider> where TFDProvider is an empty struct that implements the UIProvider trait.

---

components/net/http_loader.rs, line 768 [r1] (raw file):
Why the iters == 1? This prevents us from authenticating to any page that followed an HTTP redirection.

---

components/net/http_loader.rs, line 773 [r1] (raw file):
If no username is returned we shouldn't try to provide any credentials at all.

---

components/net/http_loader.rs, line 779 [r1] (raw file):
Instead of choosing a default (which would prevent us from accessing sites that have no password), we should use the Option that's returned already, since the Basic password field is already an Option.

---

components/net/http_loader.rs, line 782 [r1] (raw file):
Rather than directly modifying the preserved_headers, let's create a local variable in this function of type Option<Authorization>. Right before making the request, we can set the header appropriately if the variable is a Some value, and we can clear the variable right before this if block.

---

components/net/http_loader.rs, line 788 [r1] (raw file):
nit: let's remove this newline.

---

components/net/http_loader.rs, line 792 [r1] (raw file):
nit: these lines should only be indented four spaces past the let.

---

components/net/http_loader.rs, line 793 [r1] (raw file):
nit: this should be aligned with the let.

---

components/net/http_loader.rs, line 798 [r1] (raw file):
nit: add a newline above this line.

---

components/servo/Cargo.lock, line 2085 [r1] (raw file):
You'll need to run ./mach cargo-update -p net to ensure the other Cargo.lock files are updated as well.

---

Comments from the review on Reviewable.io

[DDEFISHER]

I am having trouble coming up with the unit test. I can make a mock request with a 401, however I do not know how to check the headers in the request create function and change the mock request to a non 401 after the headers have been set. What I have started for the unit test is in this pr.

[DDEFISHER]

I just realized I should not be checking the response headers in the unit test but instead maybe the response status code

[jdm]

That is correct.

[jdm]

Looking good! I've laid out how I think the unit test can be implmented; let me know if anything's unclear!
-S-awaiting-review +S-needs-code-changes

---

Reviewed 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

components/net/http_loader.rs, line 145 [r2] (raw file):
nit: no need for the {};

---

components/net/http_loader.rs, line 694 [r2] (raw file):
Let's combine these into a single input_username_and_password that returns (Option<String>, Option<String>).

---

components/net/http_loader.rs, line 715 [r2] (raw file):
This structure belongs in the unit test file instead.

---

components/net/http_loader.rs, line 719 [r2] (raw file):
This can be just struct TFDProvider;

---

components/net/http_loader.rs, line 724 [r2] (raw file):
nit: let's indent the rest of these arguments to match the first one.

---

components/net/http_loader.rs, line 800 [r2] (raw file):

if let Some(ref auth_header) = new_auth_header {
    request_headers.set(auth_header.clone());
}

---

components/net/http_loader.rs, line 815 [r2] (raw file):
Instead of retrying (ie. returning to the start of the loop), we should allow the regular code path to continue here without trying to gather any further authorization information.

---

components/net/http_loader.rs, line 829 [r2] (raw file):
We still need to set new_auth_header back to None after the previous if block.

---

tests/unit/net/http_loader.rs, line 102 [r2] (raw file):
We shouldn't be relying on a Location header for the test, since real servers don't provide one.

---

tests/unit/net/http_loader.rs, line 1576 [r2] (raw file):
This is a good start, but we'll need to do something a bit more complex for this test. We'll need to create a type which implements HttpRequest and stores a Headers member (see AssertRequestMustHaveHeaders for an example). In its send implementation, it should check for the presence of the Authorization header containing the appropriate username and password; if it's present, it should return a successful MockResponse value, and if not, it should return the 401 response. This factory should return an instance of the new type. Does that make sense?

---

tests/unit/net/http_loader.rs, line 1608 [r2] (raw file):
The goal of this test is to verify that when a server replies with a 401 response, our "user" (ie. the TestProvider) provides the right credentials that it's expecting, and the actual response that we get from load is a 200 success response.

---

Comments from Reviewable

[DDEFISHER]

Should I wait for #10555 before going further with the unit test stuff?

[jdm]

No, I was intending to hold off merging that until after these changes.

[DDEFISHER]

This new unit test makes sure a auth header is set with the ui on a 401 response. It does not explicitly check if the response was a 200, however the new testing type returns a 200 response once the auth header is set with the correct credentials. I am not sure if that is sufficient or not.

[jdm]

This looks great! Please do check that the response we end up with in the test is the one we expect, but otherwise this is ready to merge after a rebase and squash.
+S-needs-squash -S-needs-rebase +S-needs-code-changes

---

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

components/net/http_loader.rs, line 796 [r4] (raw file):
This isn't necessary any more; we can use name directly inside the match.

---

tests/unit/net/http_loader.rs, line 1652 [r4] (raw file):
We should assert that the response's status is a success code.

---

Comments from Reviewable

[DDEFISHER]

rebasing now

[KiChjang]

@bors-servo r=jdm

[bors-servo]

📌 Commit c73edcb has been approved by jdm

[bors-servo]

⌛ Testing commit c73edcb with merge 91597b6...

[bors-servo]

💔 Test failed - mac-dev-unit

[DDEFISHER]

ran cargo-update -p net

[KiChjang]

@bors-servo r=jdm

[bors-servo]

📌 Commit b0e1f10 has been approved by jdm

[bors-servo]

⌛ Testing commit b0e1f10 with merge 7bd2381...

[bors-servo]

💔 Test failed - linux-rel

[jdm]

@bors-servo: retry

• #10473

[bors-servo]

⚡ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only linux-rel...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt