Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PECO-1715] Remove username/password (BasicAuth) auth option #409

Merged
merged 1 commit into from
Jul 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 7 additions & 10 deletions src/databricks/sql/auth/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
from databricks.sql.auth.authenticators import (
AuthProvider,
AccessTokenAuthProvider,
BasicAuthProvider,
ExternalAuthProvider,
DatabricksOAuthProvider,
)
Expand All @@ -13,16 +12,14 @@
class AuthType(Enum):
DATABRICKS_OAUTH = "databricks-oauth"
AZURE_OAUTH = "azure-oauth"
# other supported types (access_token, user/pass) can be inferred
# other supported types (access_token) can be inferred
# we can add more types as needed later


class ClientContext:
def __init__(
self,
hostname: str,
username: Optional[str] = None,
password: Optional[str] = None,
access_token: Optional[str] = None,
auth_type: Optional[str] = None,
oauth_scopes: Optional[List[str]] = None,
Expand All @@ -34,8 +31,6 @@ def __init__(
credentials_provider=None,
):
self.hostname = hostname
self.username = username
self.password = password
self.access_token = access_token
self.auth_type = auth_type
self.oauth_scopes = oauth_scopes
Expand Down Expand Up @@ -65,8 +60,6 @@ def get_auth_provider(cfg: ClientContext):
)
elif cfg.access_token is not None:
return AccessTokenAuthProvider(cfg.access_token)
elif cfg.username is not None and cfg.password is not None:
return BasicAuthProvider(cfg.username, cfg.password)
elif cfg.use_cert_as_auth and cfg.tls_client_cert_file:
# no op authenticator. authentication is performed using ssl certificate outside of headers
return AuthProvider()
Expand Down Expand Up @@ -100,12 +93,16 @@ def get_python_sql_connector_auth_provider(hostname: str, **kwargs):
(client_id, redirect_port_range) = get_client_id_and_redirect_port(
auth_type == AuthType.AZURE_OAUTH.value
)
if kwargs.get("username") or kwargs.get("password"):
raise ValueError(
"Username/password authentication is no longer supported. "
"Please use OAuth or access token instead."
)

cfg = ClientContext(
hostname=normalize_host_name(hostname),
auth_type=auth_type,
access_token=kwargs.get("access_token"),
username=kwargs.get("_username"),
password=kwargs.get("_password"),
use_cert_as_auth=kwargs.get("_use_cert_as_auth"),
tls_client_cert_file=kwargs.get("_tls_client_cert_file"),
oauth_scopes=PYSQL_OAUTH_SCOPES,
Expand Down
15 changes: 0 additions & 15 deletions src/databricks/sql/auth/authenticators.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,21 +43,6 @@ def add_headers(self, request_headers: Dict[str, str]):
request_headers["Authorization"] = self.__authorization_header_value


# Private API: this is an evolving interface and it will change in the future.
# Please must not depend on it in your applications.
class BasicAuthProvider(AuthProvider):
def __init__(self, username: str, password: str):
auth_credentials = f"{username}:{password}".encode("UTF-8")
auth_credentials_base64 = base64.standard_b64encode(auth_credentials).decode(
"UTF-8"
)

self.__authorization_header_value = f"Basic {auth_credentials_base64}"

def add_headers(self, request_headers: Dict[str, str]):
request_headers["Authorization"] = self.__authorization_header_value


# Private API: this is an evolving interface and it will change in the future.
# Please must not depend on it in your applications.
class DatabricksOAuthProvider(AuthProvider):
Expand Down
4 changes: 1 addition & 3 deletions src/databricks/sql/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,8 @@ def read(self) -> Optional[OAuthToken]:
# Internal arguments in **kwargs:
# _user_agent_entry
# Tag to add to User-Agent header. For use by partners.
# _username, _password
# Username and password Basic authentication (no official support)
# _use_cert_as_auth
# Use a TLS cert instead of a token or username / password (internal use only)
# Use a TLS cert instead of a token
# _enable_ssl
# Connect over HTTP instead of HTTPS
# _port
Expand Down
40 changes: 9 additions & 31 deletions tests/unit/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

from databricks.sql.auth.auth import (
AccessTokenAuthProvider,
BasicAuthProvider,
AuthProvider,
ExternalAuthProvider,
AuthType,
Expand Down Expand Up @@ -33,21 +32,6 @@ def test_access_token_provider(self):
self.assertEqual(len(http_request.keys()), 2)
self.assertEqual(http_request["myKey"], "myVal")

def test_basic_auth_provider(self):
username = "moderakh"
password = "Elevate Databricks 123!!!"
auth = BasicAuthProvider(username=username, password=password)

http_request = {"myKey": "myVal"}
auth.add_headers(http_request)

self.assertEqual(
http_request["Authorization"],
"Basic bW9kZXJha2g6RWxldmF0ZSBEYXRhYnJpY2tzIDEyMyEhIQ==",
)
self.assertEqual(len(http_request.keys()), 2)
self.assertEqual(http_request["myKey"], "myVal")

def test_noop_auth_provider(self):
auth = AuthProvider()

Expand Down Expand Up @@ -175,21 +159,6 @@ def __call__(self, *args, **kwargs) -> HeaderFactory:
auth_provider.add_headers(headers)
self.assertEqual(headers["foo"], "bar")

def test_get_python_sql_connector_auth_provider_username_password(self):
username = "moderakh"
password = "Elevate Databricks 123!!!"
hostname = "moderakh-test.cloud.databricks.com"
kwargs = {"_username": username, "_password": password}
auth_provider = get_python_sql_connector_auth_provider(hostname, **kwargs)
self.assertTrue(type(auth_provider).__name__, "BasicAuthProvider")

headers = {}
auth_provider.add_headers(headers)
self.assertEqual(
headers["Authorization"],
"Basic bW9kZXJha2g6RWxldmF0ZSBEYXRhYnJpY2tzIDEyMyEhIQ==",
)

def test_get_python_sql_connector_auth_provider_noop(self):
tls_client_cert_file = "fake.cert"
use_cert_as_auth = "abc"
Expand All @@ -200,3 +169,12 @@ def test_get_python_sql_connector_auth_provider_noop(self):
}
auth_provider = get_python_sql_connector_auth_provider(hostname, **kwargs)
self.assertTrue(type(auth_provider).__name__, "CredentialProvider")

def test_get_python_sql_connector_basic_auth(self):
kwargs = {
"username": "username",
"password": "password",
}
with self.assertRaises(ValueError) as e:
get_python_sql_connector_auth_provider("foo.cloud.databricks.com", **kwargs)
self.assertIn("Username/password authentication is no longer supported", str(e.exception))
8 changes: 0 additions & 8 deletions tests/unit/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,21 +103,13 @@ def test_close_uses_the_correct_session_id(self, mock_client_class):
def test_auth_args(self, mock_client_class):
# Test that the following auth args work:
# token = foo,
# token = None, _username = foo, _password = bar
# token = None, _tls_client_cert_file = something, _use_cert_as_auth = True
connection_args = [
{
"server_hostname": "foo",
"http_path": None,
"access_token": "tok",
},
{
"server_hostname": "foo",
"http_path": None,
"_username": "foo",
"_password": "bar",
"access_token": None,
},
{
"server_hostname": "foo",
"http_path": None,
Expand Down
2 changes: 0 additions & 2 deletions tests/unit/test_oauth_persistence.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@

import unittest

from databricks.sql.auth.auth import AccessTokenAuthProvider, BasicAuthProvider, AuthProvider
from databricks.sql.auth.auth import get_python_sql_connector_auth_provider
from databricks.sql.experimental.oauth_persistence import DevOnlyFilePersistence, OAuthToken
import tempfile
import os
Expand Down
Loading