Better error from InteractiveBrowserCredential for invalid redirect_uri (Azure#15814)

Author: chlowell

sdk/identity/azure-identity/azure/identity/_credentials/browser.py

@@ -6,6 +6,8 @@
 import uuid
 import webbrowser
 
+from six.moves.urllib_parse import urlparse
+
 from azure.core.exceptions import ClientAuthenticationError
 
 from .. import CredentialUnavailableError
@@ -35,7 +37,7 @@ class InteractiveBrowserCredential(InteractiveCredential):
     :keyword str tenant_id: an Azure Active Directory tenant ID. Defaults to the 'organizations' tenant, which can
           authenticate work or school accounts.
     :keyword str client_id: Client ID of the Azure Active Directory application users will sign in to. If
-          unspecified, the Azure CLI's ID will be used.
+          unspecified, users will authenticate to an Azure development application.
     :keyword str redirect_uri: a redirect URI for the application identified by `client_id` as configured in Azure
           Active Directory, for example "http://localhost:8400". This is only required when passing a value for
           `client_id`, and must match a redirect URI in the application's registration. The credential must be able to
@@ -48,11 +50,19 @@ class InteractiveBrowserCredential(InteractiveCredential):
     :keyword bool allow_unencrypted_cache: if True, the credential will fall back to a plaintext cache on platforms
           where encryption is unavailable. Default to False. Has no effect when `enable_persistent_cache` is False.
     :keyword int timeout: seconds to wait for the user to complete authentication. Defaults to 300 (5 minutes).
+    :raises ValueError: invalid `redirect_uri`
     """
 
     def __init__(self, **kwargs):
         # type: (**Any) -> None
-        self._redirect_uri = kwargs.pop("redirect_uri", None)
+        redirect_uri = kwargs.pop("redirect_uri", None)
+        if redirect_uri:
+            self._parsed_url = urlparse(redirect_uri)
+            if not (self._parsed_url.hostname and self._parsed_url.port):
+                raise ValueError('"redirect_uri" must be a URL with port number, for example "http://localhost:8400"')
+        else:
+            self._parsed_url = None
+
         self._timeout = kwargs.pop("timeout", 300)
         self._server_class = kwargs.pop("_server_class", AuthCodeRedirectServer)
         client_id = kwargs.pop("client_id", DEVELOPER_SIGN_ON_CLIENT_ID)
@@ -64,17 +74,17 @@ def _request_token(self, *scopes, **kwargs):
 
         # start an HTTP server to receive the redirect
         server = None
-        redirect_uri = self._redirect_uri
-        if redirect_uri:
+        if self._parsed_url:
             try:
-                server = self._server_class(redirect_uri, timeout=self._timeout)
+                redirect_uri = "http://{}:{}".format(self._parsed_url.hostname, self._parsed_url.port)
+                server = self._server_class(self._parsed_url.hostname, self._parsed_url.port, timeout=self._timeout)
             except socket.error:
                 raise CredentialUnavailableError(message="Couldn't start an HTTP server on " + redirect_uri)
         else:
             for port in range(8400, 9000):
                 try:
+                    server = self._server_class("localhost", port, timeout=self._timeout)
                     redirect_uri = "http://localhost:{}".format(port)
-                    server = self._server_class(redirect_uri, timeout=self._timeout)
                     break
                 except socket.error:
                     continue  # keep looking for an open port

sdk/identity/azure-identity/azure/identity/_internal/auth_code_redirect_handler.py

@@ -4,7 +4,7 @@
 # ------------------------------------
 from typing import TYPE_CHECKING
 
-from six.moves.urllib_parse import parse_qs, urlparse
+from six.moves.urllib_parse import parse_qs
 
 try:
     from http.server import HTTPServer, BaseHTTPRequestHandler
@@ -45,10 +45,9 @@ class AuthCodeRedirectServer(HTTPServer):
 
     query_params = {}  # type: Mapping[str, Any]
 
-    def __init__(self, uri, timeout):
-        # type: (str, int) -> None
-        parsed = urlparse(uri)
-        HTTPServer.__init__(self, (parsed.hostname, parsed.port), AuthCodeRedirectHandler)
+    def __init__(self, hostname, port, timeout):
+        # type: (str, int, int) -> None
+        HTTPServer.__init__(self, (hostname, port), AuthCodeRedirectHandler)
         self.timeout = timeout
 
     def wait_for_redirect(self):

sdk/identity/azure-identity/tests/test_browser_credential.py

@@ -238,7 +238,8 @@ def test_interactive_credential(mock_open, redirect_url):
     assert server_class.call_count == 1
 
     if redirect_url:
-        server_class.assert_called_once_with(redirect_url, timeout=ANY)
+        parsed = urllib_parse.urlparse(redirect_url)
+        server_class.assert_called_once_with(parsed.hostname, parsed.port, timeout=ANY)
 
     # token should be cached, get_token shouldn't prompt again
     token = credential.get_token("scope")
@@ -287,11 +288,11 @@ def handle_request(self):
 def test_redirect_server():
     # binding a random port prevents races when running the test in parallel
     server = None
+    hostname = "127.0.0.1"
     for _ in range(4):
         try:
             port = random.randint(1024, 65535)
-            url = "http://127.0.0.1:{}".format(port)
-            server = AuthCodeRedirectServer(url, timeout=10)
+            server = AuthCodeRedirectServer(hostname, port, timeout=10)
             break
         except socket.error:
             continue  # keep looking for an open port
@@ -307,7 +308,8 @@ def test_redirect_server():
     thread.start()
 
     # send a request, verify the server exposes the query
-    response = urllib.request.urlopen(url + "?{}={}".format(expected_param, expected_value))  # nosec
+    url = "http://{}:{}".format(hostname, port) + "?{}={}".format(expected_param, expected_value)
+    response = urllib.request.urlopen(url)  # nosec
 
     assert response.code == 200
     assert server.query_params[expected_param] == [expected_value]
@@ -323,6 +325,14 @@ def test_no_browser():
         credential.get_token("scope")
 
 
+@pytest.mark.parametrize("redirect_uri", ("http://localhost", "host", "host:42"))
+def test_invalid_redirect_uri(redirect_uri):
+    """The credential should raise ValueError when redirect_uri is invalid or doesn't include a port"""
+
+    with pytest.raises(ValueError):
+        InteractiveBrowserCredential(redirect_uri=redirect_uri)
+
+
 def test_cannot_bind_port():
     """get_token should raise CredentialUnavailableError when the redirect listener can't bind a port"""
 
@@ -334,15 +344,13 @@ def test_cannot_bind_port():
 def test_cannot_bind_redirect_uri():
     """When a user specifies a redirect URI, the credential shouldn't attempt to bind another"""
 
-    expected_uri = "http://localhost:42"
-
     server = Mock(side_effect=socket.error)
-    credential = InteractiveBrowserCredential(redirect_uri=expected_uri, _server_class=server)
+    credential = InteractiveBrowserCredential(redirect_uri="http://localhost:42", _server_class=server)
 
     with pytest.raises(CredentialUnavailableError):
         credential.get_token("scope")
 
-    server.assert_called_once_with(expected_uri, timeout=ANY)
+    server.assert_called_once_with("localhost", 42, timeout=ANY)
 
 
 def _validate_auth_request_url(url):