Merge lp:~mandel/ubuntu-sso-client/support-user-name-url into lp:ubuntu-sso-client

[Manuel de la Peña]

Adds support for username:password@domain urls in the qt implementation of the webclient

Adds support for username:password@domain urls in the qt implementation of the webclient

[Alejandro J. Cura (alecu)]

"iwth" -> "with"
"suername" -> "username"

----

In this branch there are *only* the tests I've written... shame on you!
We should add tests in ProxySettingsTestCase for the logic you've added in get_proxy_settings()

review: Needs Fixing

[Natalia Bidart (nataliabidart)]

I see a not minor issue with the logic of parsing username and password from the gsettings["http.host"] (correct me if I'm missing something):

* isn't there a chance to end up with a username from the url, and the password from the gsetting dict? or viceversa, the password of one and the username of the other?

My point is that I would expect the parsing logic to be really explicit about which config takes precedence over the other. If I have both u:<email address hidden>, and gsettings["http.use-authentication"] enabled, which one wins? is it ok that the gsettings one wins, like in this branch?

Minor fix: there is a typo in "settins".

Thanks!

review: Needs Information

[Alejandro J. Cura (alecu)]

Reviewed code, ran tests on ubuntu. Looks fine!

review: Approve

[Roberto Alsina (ralsina)]

+1

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

Voting does not meet specified criteria. Required: Approve >= 1, Disapprove == 0, Needs Fixing == 0, Needs Information == 0, Resubmit == 0, Pending == 0. Got: 2 Approve, 1 Needs Information.

[Natalia Bidart (nataliabidart)]

I can re-review this as soon as mandel answers my question.

Thanks!

[Manuel de la Peña (mandel)]

> I see a not minor issue with the logic of parsing username and password from
> the gsettings["http.host"] (correct me if I'm missing something):
>
> * isn't there a chance to end up with a username from the url, and the
> password from the gsetting dict? or viceversa, the password of one and the
> username of the other?

Yes, there is.
>
> My point is that I would expect the parsing logic to be really explicit about
> which config takes precedence over the other. If I have both u:<email address hidden>, and
> gsettings["http.use-authentication"] enabled, which one wins? is it ok that
> the gsettings one wins, like in this branch?
>

After talking with alecu we decided to go for gsettings to step over the settings found in the url. AFAIK in the future there are going to be changes in gnome in which the creds will be correctly stored and gsettings or the keyring will be used.

> Minor fix: there is a typo in "settins".

Fixed! :)
>
> Thanks!

[Manuel de la Peña (mandel)]

> > I see a not minor issue with the logic of parsing username and password from
> > the gsettings["http.host"] (correct me if I'm missing something):
> >
> > * isn't there a chance to end up with a username from the url, and the
> > password from the gsetting dict? or viceversa, the password of one and the
> > username of the other?
>
> Yes, there is.

Sorry I'm utterly stupid and understood the wrong question. If gsettings is present there is no possibility that we mix the username and passwords. Because:

1. gsettings will always have both, username and password.
2. If anyone is missing we will have an exception because a key is missing.

Maybe is a good idea not to access the settings and gsettings['key'] but use the get method so that we don't crash. What do you think?

> >
> > My point is that I would expect the parsing logic to be really explicit
> about
> > which config takes precedence over the other. If I have both u:<email address hidden>,
> and
> > gsettings["http.use-authentication"] enabled, which one wins? is it ok that
> > the gsettings one wins, like in this branch?
> >
>
> After talking with alecu we decided to go for gsettings to step over the
> settings found in the url. AFAIK in the future there are going to be changes
> in gnome in which the creds will be correctly stored and gsettings or the
> keyring will be used.
>
> > Minor fix: there is a typo in "settins".
>
> Fixed! :)
> >
> > Thanks!

[Natalia Bidart (nataliabidart)]

> > > I see a not minor issue with the logic of parsing username and password
> from
> > > the gsettings["http.host"] (correct me if I'm missing something):
> > >
> > > * isn't there a chance to end up with a username from the url, and the
> > > password from the gsetting dict? or viceversa, the password of one and the
> > > username of the other?
> >
> > Yes, there is.
>
> Sorry I'm utterly stupid and understood the wrong question. If gsettings is
> present there is no possibility that we mix the username and passwords.
> Because:
>
> 1. gsettings will always have both, username and password.
> 2. If anyone is missing we will have an exception because a key is missing.
>
> Maybe is a good idea not to access the settings and gsettings['key'] but use
> the get method so that we don't crash. What do you think?

Since we're depending on our logic that the gsettings have the 2 values, do not change that to .get() but keep using the [] so if our assumptions are wrong, we need that to blow in our faces.

[Natalia Bidart (nataliabidart)]

review: Abstain

=== modified file 'ubuntu_sso/utils/webclient/gsettings.py'
--- ubuntu_sso/utils/webclient/gsettings.py	2012-01-10 20:51:04 +0000
+++ ubuntu_sso/utils/webclient/gsettings.py	2012-02-10 19:24:02 +0000
@@ -20,6 +20,17 @@
 GSETTINGS_CMDLINE = "gsettings list-recursively org.gnome.system.proxy"
 
 
+def parse_proxy_host(hostname):
+    """Parse the host to get username and password."""
+    username = None
+    password = None
+    if "@" in hostname:
+        username, hostname = hostname.rsplit("@", 1)
+        if ":" in username:
+            username, password = username.split(":", 1)
+    return hostname, username, password
+
+
 def get_proxy_settings():
     """Parse the proxy settings as returned by the gsettings executable."""
     output = subprocess.check_output(GSETTINGS_CMDLINE.split())
@@ -41,19 +52,23 @@
             parsed_value = int(value)
         relative_key = (path + "." + key)[base_len:]
         gsettings[relative_key] = parsed_value
-
     mode = gsettings["mode"]
     if mode == "none":
         settings = {}
     elif mode == "manual":
+        # attempt to parse the host
+        host, username, pwd = parse_proxy_host(gsettings["http.host"])
         settings = {
-            "host": gsettings["http.host"],
+            "host": host,
             "port": gsettings["http.port"],
         }
         if gsettings["http.use-authentication"]:
+            username = gsettings["http.authentication-user"]
+            pwd = gsettings["http.authentication-password"]
+        if username is not None and pwd is not None:
             settings.update({
-                "username": gsettings["http.authentication-user"],
-                "password": gsettings["http.authentication-password"],
+                "username": username,
+                "password": pwd,
             })
     else:
         # If mode is automatic the PAC javascript should be interpreted

=== modified file 'ubuntu_sso/utils/webclient/tests/test_gsettings.py'
--- ubuntu_sso/utils/webclient/tests/test_gsettings.py	2012-01-09 19:08:48 +0000
+++ ubuntu_sso/utils/webclient/tests/test_gsettings.py	2012-02-10 19:24:02 +0000
@@ -129,3 +129,111 @@
                    lambda _: fake_output)
         ps = gsettings.get_proxy_settings()
         self.assertEqual(ps, expected)
+
+    def test_gsettings_parser_authenticated_url(self):
+        """Test a parser of gsettings with creds in the url."""
+        template_values = dict(BASE_GSETTINGS_VALUES)
+        expected_host = "expected_host"
+        expected_port = 54321
+        expected_user = "carlitos"
+        expected_password = "very secret password"
+        composed_url = '%s:%s@%s' % (expected_user, expected_password,
+                                     expected_host)
+        expected = {
+            "host": expected_host,
+            "port": expected_port,
+            "username": expected_user,
+            "password": expected_password,
+        }
+        template_values.update({
+            "mode": "manual",
+            "http_host": composed_url,
+            "http_port": expected_port,
+            "http_use_auth": "false",
+        })
+        fake_output = TEMPLATE_GSETTINGS_OUTPUT.format(**template_values)
+        self.patch(gsettings.subprocess, "check_output",
+                   lambda _: fake_output)
+        ps = gsettings.get_proxy_settings()
+        self.assertEqual(ps, expected)
+
+    def test_gsettings_auth_over_url(self):
+        """Test that the settings are more important that the url."""
+        template_values = dict(BASE_GSETTINGS_VALUES)
+        expected_host = "expected_host"
+        expected_port = 54321
+        expected_user = "carlitos"
+        expected_password = "very secret password"
+        composed_url = '%s:%s@%s' % ('user', 'random',
+                                     expected_host)
+        expected = {
+            "host": expected_host,
+            "port": expected_port,
+            "username": expected_user,
+            "password": expected_password,
+        }
+        template_values.update({
+            "mode": "manual",
+            "http_host": composed_url,
+            "http_port": expected_port,
+            "auth_user": expected_user,
+            "auth_password": expected_password,
+            "http_use_auth": "true",
+        })
+        fake_output = TEMPLATE_GSETTINGS_OUTPUT.format(**template_values)
+        self.patch(gsettings.subprocess, "check_output",
+                   lambda _: fake_output)
+        ps = gsettings.get_proxy_settings()
+        self.assertEqual(ps, expected)
+
+
+class ParseProxyHostTestCase(TestCase):
+    """Test the parsing of the domain."""
+
+    def test_onlyhost(self):
+        """Parse a host with no username or password."""
+        sample = "hostname"
+        hostname, username, password = gsettings.parse_proxy_host(sample)
+        self.assertEqual(username, None)
+        self.assertEqual(password, None)
+        self.assertEqual(hostname, "hostname")
+
+    def test_user_and_host(self):
+        """Parse host just with the username."""
+        sample = "username@hostname"
+        hostname, username, password = gsettings.parse_proxy_host(sample)
+        self.assertEqual(username, "username")
+        self.assertEqual(password, None)
+        self.assertEqual(hostname, "hostname")
+
+    def test_user_pass_and_host(self):
+        """Test parsing a host with a username and password."""
+        sample = "username:password@hostname"
+        hostname, username, password = gsettings.parse_proxy_host(sample)
+        self.assertEqual(username, "username")
+        self.assertEqual(password, "password")
+        self.assertEqual(hostname, "hostname")
+
+    def test_username_with_at(self):
+        """Test parsing the host with a username with @."""
+        sample = "[e-mail]:password@hostname"
+        hostname, username, password = gsettings.parse_proxy_host(sample)
+        self.assertEqual(username, "[e-mail]")
+        self.assertEqual(password, "password")
+        self.assertEqual(hostname, "hostname")
+
+    def test_username_with_at_nopass(self):
+        """Test parsing the host without a password."""
+        sample = "[e-mail]@hostname"
+        hostname, username, password = gsettings.parse_proxy_host(sample)
+        self.assertEqual(username, "[e-mail]")
+        self.assertEqual(password, None)
+        self.assertEqual(hostname, "hostname")
+
+    def test_user_pass_with_colon_and_host(self):
+        """Test parsing the host with a password that contains :."""
+        sample = "username:pass:word@hostname"
+        hostname, username, password = gsettings.parse_proxy_host(sample)
+        self.assertEqual(username, "username")
+        self.assertEqual(password, "pass:word")
+        self.assertEqual(hostname, "hostname")