Code review comment for lp:~alecu/ubuntuone-client/proxy-tunnel-server

Alejandro J. Cura (alecu) wrote :

> I dont think that the following code is correct:
>
> 787 + else:
> 788 + return QNetworkProxy(QNetworkProxy.HttpProxy,
> 789 + hostName=settings.get("host", ""),
> 790 + port=settings.get("port", 0),
> 791 + user=settings.get("username", ""),
> 792 + password=settings.get("password", ""))
>
> The use of "" for the username and password will give problems when dealing
> with the auth proxies because the first attempt would be to use them and
> therefore you will get a 401 instead of a 407.

Those values are only used when the "settings" dictionary has no key with the given name.
And I'm using empty strings because that's what QNetworkProxy uses if no parameter is given:
 * https://qt-project.org/doc/qt-4.8/qnetworkproxy.html#QNetworkProxy-2

Since the current version of the tunnel is not getting the authentication, I think it's safe to use it like this. Perhaps we can add a comment or a TODO for it, and we can fix it in an upcoming branch to add authentication.

> 506 + def test_stop_but_never_connected(self):
> 507 + """Stop but it was never connected."""
> 508 + fake_protocol = FakeServerTunnelProtocol()
> 509 + client = tunnel_server.RemoteSocket(fake_protocol)
> 510 + yield client.stop()
>
> There is a missing decorator in the test.

Great catch! Thanks, I'm fixing it too.

« Back to merge proposal