Open Bug 1395886 Opened 7 years ago Updated 1 year ago

Add support for proxy authentication (username / password)

Categories

(Remote Protocol :: Marionette, enhancement, P3)

Product:
Remote Protocol
Component:
Marionette
Version:
57 Branch
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: whimboo, Unassigned)

References

(Blocks 2 open bugs,
URL
)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1395886 Add support for proxy authentication
59 bytes, text/x-review-board-request
Details 
To allow the usage of proxies which require authentication, support for setting the username / password has to be added to Marionette.
Here is the webdriver-rust part for this bug: https://github.com/mozilla/webdriver-rust/pull/123

Implementing this feature actually means that Marionette has to use the password manager to store the credentials. There are some gaps to solve:

1) Correctly save the username and password in the password manager
2) Ensure it will be used when accessing the proxy server without requesting it from the user
3) Ensure 2) also works after a restart
Assignee: nobody → hskupin
Blocks: webdriver
Status: NEW → ASSIGNED
Priority: -- → P1
(In reply to Henrik Skupin (:whimboo) from comment #1)
> Here is the webdriver-rust part for this bug:
> https://github.com/mozilla/webdriver-rust/pull/123

Given that webdriver is part of the tree now, I will re-work this PR so it applies to m-c.
The demand for this feature might be low at the moment. So we will defer the implementation for a while.
No longer blocks: 1391605
Priority: P1 → P4
Assignee: hskupin → nobody
Status: ASSIGNED → NEW
Any idea when this will be implemented?
It depends on how many different requests we get. So far no-one asked for it. As such we do not have an ETA for it yet.
I see. Is there an official channel for asking for new features? 

Also, in addition to the webdriver-rust part, which other components would need to be updated. I'm asking because I might be interested in contributing to having the feature implemented. If such a contribution would be welcome of course.
Sure, and we would totally support you if you want to contribute to get this feature implemented. Basically all what is needed fro webdriver-rust I have already written and is part of https://github.com/mozilla/webdriver-rust/pull/123. Given that the repository has been moved into mozilla-central, the patch needs to be adapted.

Otherwise all the important work which needs to be done has to be in Marionette and is JS code. It mostly involves work around capability parsing, and interacting with the Firefox password manager to set the username and password for the specific proxy host.

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Using_nsIPasswordManager

Overall it might not be that complicated.

If you have questions as best join us on IRC and ask me directly. Details in how to get started can be found here: https://wiki.mozilla.org/User:Mjzffr/New_Contributors.
Mentor: hskupin
Whiteboard: [lang=js][lang=py][lang=rs]
CC'ing Ian who wants to work on this bug.
I would indeed like to work on this bug! I have a couple of questions:

Am I right that usernames and passwords should be stored in a nsIPasswordManager that would be a property of the session.Proxy class in session.js?

It seems that Proxy can have host and port info for each possible protocol. Should it then hold a separate username and password for each protocol in a single PasswordManager? If so, should it store a username for each protocol outside of the PasswordManager to handle cases of two different protocols with the same host but different authentication values?
(In reply to Ian MacLeod (:skeletor) from comment #9)
> Am I right that usernames and passwords should be stored in a
> nsIPasswordManager that would be a property of the session.Proxy class in
> session.js?

We don't need a property for that. When initializing the proxy data you would just create an instance of this interface. It has to be used to store the data for the given host. Later it would be not necessary given that Firefox itself uses the password manager to retrieve the information.

> It seems that Proxy can have host and port info for each possible protocol.
> Should it then hold a separate username and password for each protocol in a
> single PasswordManager? If so, should it store a username for each protocol
> outside of the PasswordManager to handle cases of two different protocols
> with the same host but different authentication values?

Hm, it's a good question in how we have to handle that case. I believe that there should still be a separation between different protocols when using the same host. But it could still be that for each proxy type the same host/port combination is used, and as such we should just overwrite the data. Personally I would call this more a user error. But maybe the webdriver spec should be clearer about such a situation. Would you mind filing an issue at https://github.com/w3c/webdriver/issues/ so it can be made more clear, or should I do it? Just let me know.
Here's a quick rundown on the problems I am having with loginManager.

When I try to use Services.logins in https://dxr.mozilla.org/mozilla-central/source/testing/marionette/session.js and run test_session.js, an "Initialization failed" error is thrown in obj-x86_64-pc-mingw32/dist/bin/components/storage-json.js. If I dump() the value of OS.Constants.Path.profileDir in storage-json.js its value is undefined.

When I run the test at https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/xpcshell/test_ext_browsingData_passwords.js there is no problem during the initialization of the loginManager, and dumping OS.Constants.Path.profileDir in storage-json.js yields a defined value.

I've tried putting a delay before the tests in test_session.js run, and I've tried initializing the loginManager in the same way in both files, but the results haven't changed.
Flags: needinfo?(hskupin)
Ian, do you see the same problem when you have this change in session.js, and normally run our Marionette unit tests? Or is that failure only present for xpcshell tests? If it's only visible for xpcshell, would you mind to pastebin your changes/additions? I would like to test it out myself locally. Btw. which platform are you on?
Flags: needinfo?(hskupin) → needinfo?(rubberdonkeysandwich)
It works when I run the Marionette unit tests. test_proxy.js still has an expected failure, but the initialization of the loginManager succeeds. I'm on 64 bit Windows 10.

Here's a pastebin of the diff showing my only change to session.js: https://pastebin.com/5t4dTvXb
Flags: needinfo?(rubberdonkeysandwich)
(In reply to Ian MacLeod (:skeletor) from comment #11)
> Here's a quick rundown on the problems I am having with loginManager.
> 
> When I try to use Services.logins in
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/session.js
> and run test_session.js, an "Initialization failed" error is thrown in
> obj-x86_64-pc-mingw32/dist/bin/components/storage-json.js. If I dump() the
> value of OS.Constants.Path.profileDir in storage-json.js its value is
> undefined.

I tried myself and with a debug build I see this helpful output:

0:03.79 PROCESS_OUTPUT: Thread-3 (pid:24845) "[24845, Main Thread] WARNING: NSS will be initialized without a profile directory. Some things may not work as expected.: file /builds/worker/workspace/build/src/security/manager/ssl/nsNSSComponent.cpp, line 1790"

Which means that there is no profile directory set, which is also described here:
https://wiki.mozilla.org/Security/CryptoEngineering/Platform_Use_of_NSS#The_Desired_Setup

Tim, do you know if it is possible to run certain xpcshell tests with a profile directory? We would need that for the Marionette tests. If not we would have to catch this failure in Marionette (session.js), and could not set any proxy authentication for the new session command. Mainly this would only affect xpcshell tests only, so for this case we could write Marionette unit tests.
Flags: needinfo?(ttaubert)
(In reply to Henrik Skupin (:whimboo) from comment #14)
> Tim, do you know if it is possible to run certain xpcshell tests with a
> profile directory? We would need that for the Marionette tests.

I don't know much about the implementation of the xpcshell test framework, sorry.
Flags: needinfo?(ttaubert)
Priority: P4 → P5
(In reply to Tim Taubert [:ttaubert] from comment #15)
> > Tim, do you know if it is possible to run certain xpcshell tests with a
> > profile directory? We would need that for the Marionette tests.
> 
> I don't know much about the implementation of the xpcshell test framework,
> sorry.

Ok, thanks anyway. We might have to find someone else then, but it's not a blocker right now given that for normal unit tests it should work.

Ian, the last patch which you submitted via pastebin does still not apply for me. Can you please check that again?
Flags: needinfo?(rubberdonkeysandwich)
As we noticed this bug is kinda complex to solve. As such Ian will work on something else, and we might have to fix this bug ourselves when there is time for. Ian, thank you anyway for trying it, and pointing us to the known issues!
Mentor: hskupin
Flags: needinfo?(rubberdonkeysandwich)
Whiteboard: [lang=js][lang=py][lang=rs]
The problem as has been seen above was that the call to the login manager happened in proxy.fromJSON(), while it should have happened in proxy.init(). Why this is happening you can read here on bug 1411207 comment 3.

Basically what we should do is to add properties on the proxy class like:

this.ftpProxyAuth = {username: "…", password: "…"}

Then init() can take this dictionary and setup the logins.

Ian, maybe that solves your blocker and you might be interested to continue to work on it?
Flags: needinfo?(rubberdonkeysandwich)
Thanks! Yeah, I think I'd be interested in continuing to work on this now, I'll let you know if I get stuck!
Flags: needinfo?(rubberdonkeysandwich)
(In reply to Ian MacLeod (:skeletor) from comment #19)
> Thanks! Yeah, I think I'd be interested in continuing to work on this now,
> I'll let you know if I get stuck!

Hi Ian, I wanted to check back with you. I hope all is good on your side, and maybe we can find some time to continue the work on this bug? 

The latest state I have is that storing the credentials works, but for the very first time when accessing the proxy the authentication dialog pops-up, even with the preference set to auto-login.
Flags: needinfo?(rubberdonkeysandwich)
Hi, yes, that is still where I'm at. I will try to be around the next few days to work on it.
Flags: needinfo?(rubberdonkeysandwich)
That sounds good. I have seen that you tried to reach me over the weekend. But I wasn't around at this time.

So for now I would suggest that you just continue on the patch, and when testing the changes fill out the dialog yourself. Once a working patch is available, I will apply the patch myself locally and have a look. Does that work for you?
Here is an update on where I'm at with this bug:
1. When I start a Marionette session using a proxy server with authentication, I store the login in the LoginManager. When the server requests authentication, I get a popup with the username and password fields filled in. I want to use the login from the LoginManager without a popup appearing, but I haven't managed to get that to work. I have tried setting "signon.autologin.proxy" to true in server.js to no avail. I get the same behavior when I am using Firefox without Marionette.
2. When adding logins to LoginManager an httpRealm is required. The proxy server includes an HTTP Realm when it requests authentication and if the login's stored value doesn't match, then the login isn't used. I'm not sure what the best way is to acquire this httpRealm for the proxy server.
3. An exception is thrown when adding a login if one already exists for that host/port/httpRealm. I was wondering what the right behavior is when this happens, and it seems to me that if the existing login matches the new one we can just continue, but if they are different we should throw a new exception, does that seem right?
(In reply to Ian MacLeod (:skeletor) from comment #23)
> 1. When I start a Marionette session using a proxy server with
> authentication, I store the login in the LoginManager. When the server
> requests authentication, I get a popup with the username and password fields
> filled in. I want to use the login from the LoginManager without a popup
> appearing, but I haven't managed to get that to work. I have tried setting
> "signon.autologin.proxy" to true in server.js to no avail. I get the same
> behavior when I am using Firefox without Marionette.

This might be bug 973064. It would be good if you could verify that, and if that is the case we should reopen that other bug.

For now we could add a workaround to the test which switches to the dialog, and accepts it. Check the modal dialog tests under Marionette unit tests in how to do it.

> 2. When adding logins to LoginManager an httpRealm is required. The proxy
> server includes an HTTP Realm when it requests authentication and if the
> login's stored value doesn't match, then the login isn't used. I'm not sure
> what the best way is to acquire this httpRealm for the proxy server.

If we really have to use the same HTTPRealm for storing the login credentials we would have to initiate a GET request to retrieve it. Maybe a HEAD request would also be sufficient. Could you check with eg curl how the proxy responds on both of such requests?

> 3. An exception is thrown when adding a login if one already exists for that
> host/port/httpRealm. I was wondering what the right behavior is when this
> happens, and it seems to me that if the existing login matches the new one
> we can just continue, but if they are different we should throw a new
> exception, does that seem right?

The WebDriver spec doesn't tell anything about it, but I would say we take the very first credential, and simply ignore the exception. We could write an info or warning message to the log mentioning that.

Ian, it would be great if you could upload the current code as WIP, so I could have a look at it, and test myself. Thanks.
(In reply to Henrik Skupin (:whimboo) from comment #24)
> This might be bug 973064. It would be good if you could verify that, and if
> that is the case we should reopen that other bug.

The behavior that I'm seeing is very similar to what Manish was reporting. One difference is that I have to close the alert 5 or 6 times before it gives up. How would I go about verifying that I'm experiencing the same issue?

> For now we could add a workaround to the test which switches to the dialog,
> and accepts it. Check the modal dialog tests under Marionette unit tests in
> how to do it.

Yeah, that works for me!

> If we really have to use the same HTTPRealm for storing the login
> credentials we would have to initiate a GET request to retrieve it. Maybe a
> HEAD request would also be sufficient. Could you check with eg curl how the
> proxy responds on both of such requests?

So I didn't have success sending requests to the proxy server directly, but if I send an unauthenticated request using the proxy server as a proxy it responds with a Proxy-Authenticate header which contains the httpRealm. For example:
curl -I -x <proxy url> <random url>

> Ian, it would be great if you could upload the current code as WIP, so I
> could have a look at it, and test myself. Thanks.

Here is a export of my current patch: https://pastebin.com/kh2Nv1SN

Let me know if it would be better if I pushed it for review.
Flags: needinfo?(hskupin)
The current version of my patch is here: https://pastebin.com/1bP0kLjw
(In reply to Ian MacLeod (:skeletor) from comment #25)
> (In reply to Henrik Skupin (:whimboo) from comment #24)
> > This might be bug 973064. It would be good if you could verify that, and if
> > that is the case we should reopen that other bug.
> 
> The behavior that I'm seeing is very similar to what Manish was reporting.
> One difference is that I have to close the alert 5 or 6 times before it
> gives up. How would I go about verifying that I'm experiencing the same
> issue?

Personally I would not care how many times it pops-up before it automatically signs in. So if you can replicate it via the following steps I would suggest we reopen the bug.

1. Create a new Firefox profile
2. Setup a proxy which needs authentication
3. Open a URL and enter the proxy authentication details
4. Restart Firefox
5. Open a URL
6. Check if auto-signin works
7. Repeat steps 5 and 6 until it works

Also I would suggest while doing this test you enable HTTP logging via the environment variable. See here for details:
https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging

> > For now we could add a workaround to the test which switches to the dialog,
> > and accepts it. Check the modal dialog tests under Marionette unit tests in
> > how to do it.
> 
> Yeah, that works for me!

Whereby we would have problems if you really have to do it 5 or 6 times. Maybe lets investigate the above problem first in depth.

> > If we really have to use the same HTTPRealm for storing the login
> > credentials we would have to initiate a GET request to retrieve it. Maybe a
> > HEAD request would also be sufficient. Could you check with eg curl how the
> > proxy responds on both of such requests?
> 
> So I didn't have success sending requests to the proxy server directly, but
> if I send an unauthenticated request using the proxy server as a proxy it
> responds with a Proxy-Authenticate header which contains the httpRealm. For
> example:
> curl -I -x <proxy url> <random url>

Sadly I do not have a proxy with authentication setup locally. Do you know of a public service which offers that? That would make it easier for me to setup something to check myself. For now I found the following which might help us:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Proxy-Authenticate

Otherwise I don't know if there are some helper APIs available in Firefox which we could use, to get our proxy authentication data added to the Login manager without having to retrieve the realm ourselves. Gijs, do you know something?
Flags: needinfo?(hskupin) → needinfo?(gijskruitbosch+bugs)
Attachment #8946589 - Flags: review?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #28)
> Otherwise I don't know if there are some helper APIs available in Firefox
> which we could use, to get our proxy authentication data added to the Login
> manager without having to retrieve the realm ourselves. Gijs, do you know
> something?

I expect the best thing to do would be to make a request using 'fetch' or XMLHttpRequest, and read headers. But maybe Matt has a better idea given he knows about login manager stuff.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(MattN+bmo)
(In reply to Henrik Skupin (:whimboo) from comment #28)
> Personally I would not care how many times it pops-up before it
> automatically signs in. So if you can replicate it via the following steps I
> would suggest we reopen the bug.
> 
> 1. Create a new Firefox profile
> 2. Setup a proxy which needs authentication
> 3. Open a URL and enter the proxy authentication details
> 4. Restart Firefox
> 5. Open a URL
> 6. Check if auto-signin works
> 7. Repeat steps 5 and 6 until it works

So it seems that with the latest version of Firefox I no longer experience this behavior. I tried downloading an older version (56.0b9) and I do get the same behavior as before. Would it still be useful to collect the HTTP logs for the older version?

I was actually still getting a popup when running the Marionette tests on the latest version, but I just now realized that I should set the "signon.autologin.proxy" setting in geckoinstance.py - not just server.js. Now everything seems to be working as intended except for the httpRealm issue. I will see if I can update the patch to use a head request to get the realm.

> Sadly I do not have a proxy with authentication setup locally. Do you know
> of a public service which offers that? That would make it easier for me to
> setup something to check myself. For now I found the following which might
> help us:
> 
> https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Proxy-Authenticate

There are a lot of proxy services out there, but I didn't see any for public use that include authentication. There are paid/private ones like https://www.myprivateproxy.net, but I have never tried any so I don't really have any basis to recommend one. I created a proxy server using a Digital Ocean droplet for testing that I will leave up if you want to use it. The information to connect to it is in the patch I uploaded.
(In reply to Tim Taubert [:ttaubert] from comment #15)
> (In reply to Henrik Skupin (:whimboo) from comment #14)
> > Tim, do you know if it is possible to run certain xpcshell tests with a
> > profile directory? We would need that for the Marionette tests.
> 
> I don't know much about the implementation of the xpcshell test framework,
> sorry.

do_get_profile() is what you need to use in xpcshell to get it to setup a profile. See https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Environment

(In reply to :Gijs from comment #29)
> (In reply to Henrik Skupin (:whimboo) from comment #28)
> > Otherwise I don't know if there are some helper APIs available in Firefox
> > which we could use, to get our proxy authentication data added to the Login
> > manager without having to retrieve the realm ourselves. Gijs, do you know
> > something?
> 
> I expect the best thing to do would be to make a request using 'fetch' or
> XMLHttpRequest, and read headers. But maybe Matt has a better idea given he
> knows about login manager stuff.

Yeah, that's what you'd have to do. There isn't a way to add an HTTP Auth login that matches all realms. If there is a helper to retrieve the realm then it's probably in the networking layer and probably not accessible via JS and wouldn't help with much I think.
Flags: needinfo?(MattN+bmo)
(In reply to Ian MacLeod (:skeletor) from comment #30)
> So it seems that with the latest version of Firefox I no longer experience
> this behavior. I tried downloading an older version (56.0b9) and I do get
> the same behavior as before. Would it still be useful to collect the HTTP
> logs for the older version?

No, but it would be good to know which bug fixed that. So what do you mean with latest version? Nightly, 59 beta, or 58 release? 
 
> I was actually still getting a popup when running the Marionette tests on
> the latest version, but I just now realized that I should set the
> "signon.autologin.proxy" setting in geckoinstance.py - not just server.js.

We evaluate the value of the pref at runtime when it is needed:

https://dxr.mozilla.org/mozilla-central/rev/ef1fefe4c6d1f95e2bdf872094e02e85c962aa86/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#572

So it's not only once when starting Firefox. I wonder if server.js is actually setting the preferences after your new proxy code . If that is the case we have to get this changed in a different bug, which should block this one.

> Now everything seems to be working as intended except for the httpRealm
> issue. I will see if I can update the patch to use a head request to get the
> realm.

Great. Let me know if you still have problems to get this done. Gijs already gave some great info in how to get this done via `fetch`.

> have any basis to recommend one. I created a proxy server using a Digital
> Ocean droplet for testing that I will leave up if you want to use it. The
> information to connect to it is in the patch I uploaded.

Thanks. For now this is fine, but for landing I have to find a better solution. Maybe just a local mock proxy.

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #31)
> do_get_profile() is what you need to use in xpcshell to get it to setup a
> profile. See
> https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-
> based_unit_tests#Environment

Oh, wonderful! Thanks!
(In reply to Henrik Skupin (:whimboo) from comment #32)
> No, but it would be good to know which bug fixed that. So what do you mean
> with latest version? Nightly, 59 beta, or 58 release? 

The 58 release. I installed a couple previous releases and found that the behavior changed between the 57.0.4 and 58.0 releases. Unfortunately, I've just moved and did not bring this computer with me, so it will be hard to provide more details moving forward.

> We evaluate the value of the pref at runtime when it is needed:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> ef1fefe4c6d1f95e2bdf872094e02e85c962aa86/toolkit/components/passwordmgr/
> nsLoginManagerPrompter.js#572
> 
> So it's not only once when starting Firefox. I wonder if server.js is
> actually setting the preferences after your new proxy code . If that is the
> case we have to get this changed in a different bug, which should block this
> one.

I'm not too sure of my understanding of this, but it seems that when Firefox is launched by the tests, marionette.prefs.recommended is false, which means that it ignores the settings found here:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/server.js#60

I think it was just my mistake to not realize that adding the preference in this place alone wouldn't affect the tests, is that not right?

> Great. Let me know if you still have problems to get this done. Gijs already
> gave some great info in how to get this done via `fetch`.

I am still having some issues with this. The response from the proxy server only seems to include the Proxy-Authenticate header when it's a 407. As when I was testing with curl, I can't get the server to respond with a 407 by sending a GET request to the server; it seems that I have to send a CONNECT request instead. But, from what I can tell, it isn't possible to use fetch or XMLHttpRequest to send a CONNECT request (https://fetch.spec.whatwg.org/#forbidden-method).

Is there some reliable way to get a 407 response from a proxy server using a GET or HEAD request?
(In reply to Ian MacLeod (:skeletor) from comment #33)
Hi Ian, sorry that I missed to give an update here. In case of questions it would be still good if you could pop into the IRC channel when you find the time.

> (In reply to Henrik Skupin (:whimboo) from comment #32)
> > No, but it would be good to know which bug fixed that. So what do you mean
> > with latest version? Nightly, 59 beta, or 58 release? 
> 
> The 58 release. I installed a couple previous releases and found that the
> behavior changed between the 57.0.4 and 58.0 releases. Unfortunately, I've
> just moved and did not bring this computer with me, so it will be hard to
> provide more details moving forward.

Ok, in that case no problem. If you find time it would be still interesting to know if that was fixed by one of the beta releases of 58, or before its first public beta.

> > We evaluate the value of the pref at runtime when it is needed:
> > 
> > https://dxr.mozilla.org/mozilla-central/rev/
> > ef1fefe4c6d1f95e2bdf872094e02e85c962aa86/toolkit/components/passwordmgr/
> > nsLoginManagerPrompter.js#572
> > 
> > So it's not only once when starting Firefox. I wonder if server.js is
> > actually setting the preferences after your new proxy code . If that is the
> > case we have to get this changed in a different bug, which should block this
> > one.
> 
> I'm not too sure of my understanding of this, but it seems that when Firefox
> is launched by the tests, marionette.prefs.recommended is false, which means
> that it ignores the settings found here:
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/server.
> js#60

Yes, when you add a preference for Marionette it also has to be done in geckoinstance.py. The recommended prefs are used by geckodriver, which also maybe need an update later.

> I am still having some issues with this. The response from the proxy server
> only seems to include the Proxy-Authenticate header when it's a 407. As when
> I was testing with curl, I can't get the server to respond with a 407 by
> sending a GET request to the server; it seems that I have to send a CONNECT
> request instead. But, from what I can tell, it isn't possible to use fetch
> or XMLHttpRequest to send a CONNECT request
> (https://fetch.spec.whatwg.org/#forbidden-method).
> 
> Is there some reliable way to get a 407 response from a proxy server using a
> GET or HEAD request?

If you can get the pref setting fixed I could have a look at your patch and see if I can figure out how to do it by using the referenced proxy server.
Assignee: nobody → rubberdonkeysandwich
Status: NEW → ASSIGNED
Flags: needinfo?(rubberdonkeysandwich)
Attachment #8946589 - Attachment is obsolete: true
Attachment #8960076 - Flags: review?(hskupin)
Comment on attachment 8960076 [details]
Bug 1395886 Add support for proxy authentication

https://reviewboard.mozilla.org/r/228858/#review234526


Code analysis found 11 defects in this patch:
 - 11 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/session.js:266
(Diff revision 1)
>          }
>        }
>  
> -      if (url.username != "" ||
> -          url.password != "" ||
> -          url.pathname != "/" ||
> +      let login = null;
> +      if (url.username !== "") {
> +        login = Components.classes["@mozilla.org/login-manager/loginInfo;1"]

Error: Use cc rather than components.classes [eslint: mozilla/use-cc-etc]

::: testing/marionette/session.js:269
(Diff revision 1)
> -      if (url.username != "" ||
> -          url.password != "" ||
> -          url.pathname != "/" ||
> +      let login = null;
> +      if (url.username !== "") {
> +        login = Components.classes["@mozilla.org/login-manager/loginInfo;1"]
> +          .createInstance(Ci.nsILoginInfo);
> +        login.init(
> +          `moz-proxy://${url.hostname}:${port}`,

Error: Expected indentation of 12 spaces but found 10. [eslint: indent-legacy]

::: testing/marionette/session.js:270
(Diff revision 1)
> -          url.password != "" ||
> -          url.pathname != "/" ||
> +      if (url.username !== "") {
> +        login = Components.classes["@mozilla.org/login-manager/loginInfo;1"]
> +          .createInstance(Ci.nsILoginInfo);
> +        login.init(
> +          `moz-proxy://${url.hostname}:${port}`,
> +          null,

Error: Expected indentation of 12 spaces but found 10. [eslint: indent-legacy]

::: testing/marionette/session.js:271
(Diff revision 1)
> -          url.pathname != "/" ||
> +        login = Components.classes["@mozilla.org/login-manager/loginInfo;1"]
> +          .createInstance(Ci.nsILoginInfo);
> +        login.init(
> +          `moz-proxy://${url.hostname}:${port}`,
> +          null,
> +          "Squid proxy-caching web server",

Error: Expected indentation of 12 spaces but found 10. [eslint: indent-legacy]

::: testing/marionette/session.js:272
(Diff revision 1)
> +          .createInstance(Ci.nsILoginInfo);
> +        login.init(
> +          `moz-proxy://${url.hostname}:${port}`,
> +          null,
> +          "Squid proxy-caching web server",
> +          url.username,

Error: Expected indentation of 12 spaces but found 10. [eslint: indent-legacy]

::: testing/marionette/session.js:273
(Diff revision 1)
> +        login.init(
> +          `moz-proxy://${url.hostname}:${port}`,
> +          null,
> +          "Squid proxy-caching web server",
> +          url.username,
> +          url.password,

Error: Expected indentation of 12 spaces but found 10. [eslint: indent-legacy]

::: testing/marionette/session.js:274
(Diff revision 1)
> +          `moz-proxy://${url.hostname}:${port}`,
> +          null,
> +          "Squid proxy-caching web server",
> +          url.username,
> +          url.password,
> +          "",

Error: Expected indentation of 12 spaces but found 10. [eslint: indent-legacy]

::: testing/marionette/session.js:275
(Diff revision 1)
> +          null,
> +          "Squid proxy-caching web server",
> +          url.username,
> +          url.password,
> +          "",
> +          "");

Error: Expected indentation of 12 spaces but found 10. [eslint: indent-legacy]

::: testing/marionette/session.js:368
(Diff revision 1)
> -        return `${hostname}:${port}`;
> +        hostname = `${hostname}:${port}`;
> +      }
> +
> +      let logins = Services.logins.findLogins(
> +        {},
> +        `moz-proxy://${hostname}`,

Error: Expected indentation of 10 spaces but found 8. [eslint: indent-legacy]

::: testing/marionette/session.js:369
(Diff revision 1)
> +      }
> +
> +      let logins = Services.logins.findLogins(
> +        {},
> +        `moz-proxy://${hostname}`,
> +        null,

Error: Expected indentation of 10 spaces but found 8. [eslint: indent-legacy]

::: testing/marionette/session.js:370
(Diff revision 1)
> +
> +      let logins = Services.logins.findLogins(
> +        {},
> +        `moz-proxy://${hostname}`,
> +        null,
> +        "Squid proxy-caching web server",

Error: Expected indentation of 10 spaces but found 8. [eslint: indent-legacy]
:skeletor

Are you still working on this bug?
Comment on attachment 8960076 [details]
Bug 1395886 Add support for proxy authentication

I'm sorry for not having replied on this patch yet. But at the point when it was uploaded we still had problems with how to best check for the authentication. By now we have a new mozbase module called mozproxy (https://firefox-source-docs.mozilla.org/mozbase/mozproxy.html) which allows to handle connections to a proxy server in this case mitmproxy (https://mitmproxy.org/).

mitmproxy can also handle authentication which seems to be what we want here for testing purposes.

Removing review request given that the patch is outdated.

If Ian has no time or interest to continue we will have to come back to this patch at some point in the future.
Flags: needinfo?(rubberdonkeysandwich)
Attachment #8960076 - Flags: review?(hskupin)
Assignee: rubberdonkeysandwich → nobody
Status: ASSIGNED → NEW
Priority: P5 → P3
Severity: normal → S3
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: