From bde16f5c8c0da02122409013a83d63ad8d954ad7 Mon Sep 17 00:00:00 2001 From: Stepan Burlakov Date: Tue, 25 Apr 2023 11:50:58 +0300 Subject: [PATCH 1/3] create acceptance unit test --- tests/unit/common/test_settings.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/unit/common/test_settings.py b/tests/unit/common/test_settings.py index 70e9166a975..21eea5bec63 100644 --- a/tests/unit/common/test_settings.py +++ b/tests/unit/common/test_settings.py @@ -1,4 +1,6 @@ +import os from typing import Tuple +from unittest.mock import Mock, patch from pydantic import ValidationError from pytest import mark, raises @@ -46,3 +48,14 @@ def test_settings_auth_credentials(kwargs) -> None: err = exc_info.value assert len(err.errors()) > 0 + + +@patch("firebolt.common.settings.logger") +def test_deprecation_warning_with_env(logger_mock: Mock): + with patch.dict( + os.environ, + {"FIREBOLT_USER": "user", "FIREBOLT_PASSWORD": "password"}, + clear=True, + ): + Settings(server="server", default_region="region") + logger_mock.warning.assert_not_called() From c932e76d3f88cfcdba188a6a06b30accc93b13b9 Mon Sep 17 00:00:00 2001 From: Stepan Burlakov Date: Tue, 25 Apr 2023 18:28:15 +0300 Subject: [PATCH 2/3] fix deprecation warning in settings --- src/firebolt/common/settings.py | 67 +++++++++++++-------- src/firebolt/service/manager.py | 2 +- tests/unit/async_db/test_connection.py | 14 ++--- tests/unit/client/conftest.py | 2 +- tests/unit/common/test_settings.py | 13 ++-- tests/unit/conftest.py | 7 +-- tests/unit/db/test_connection.py | 14 ++--- tests/unit/service/test_resource_manager.py | 10 +-- 8 files changed, 65 insertions(+), 64 deletions(-) diff --git a/src/firebolt/common/settings.py b/src/firebolt/common/settings.py index f14fda233e3..f77db994626 100644 --- a/src/firebolt/common/settings.py +++ b/src/firebolt/common/settings.py @@ -1,9 +1,9 @@ import logging -from typing import Optional +import os +from dataclasses import dataclass, field +from typing import Any, Callable, Optional -from pydantic import BaseSettings, Field, SecretStr, root_validator - -from firebolt.client.auth import Auth +from firebolt.client.auth import Auth, UsernamePassword logger = logging.getLogger(__name__) @@ -18,8 +18,31 @@ >>> ... >>> settings = Settings(auth=Token(access_token), ...)""" +USERNAME_ENV = "FIREBOLT_USER" +PASSWORD_ENV = "FIREBOLT_PASSWORD" +AUTH_TOKEN_ENV = "FIREBOLT_AUTH_TOKEN" +ACCOUNT_ENV = "FIREBOLT_ACCOUNT" +SERVER_ENV = "FIREBOLT_SERVER" +DEFAULT_REGION_ENV = "FIREBOLT_DEFAULT_REGION" + + +def from_env(var_name: str, default: Any = None) -> Callable: + def inner() -> Any: + os.environ.get(var_name, default) + + return inner + -class Settings(BaseSettings): +def auth_from_env() -> Optional[Auth]: + username = os.environ.get(USERNAME_ENV, None) + password = os.environ.get(PASSWORD_ENV, None) + if username and password: + return UsernamePassword(username, password) + return None + + +@dataclass +class Settings: """Settings for Firebolt SDK. Attributes: @@ -34,25 +57,19 @@ class Settings(BaseSettings): default_region (str): Default region for provisioning """ - auth: Optional[Auth] = Field(None) + auth: Optional[Auth] = field(default_factory=auth_from_env) # Authorization - user: Optional[str] = Field(None, env="FIREBOLT_USER") - password: Optional[SecretStr] = Field(None, env="FIREBOLT_PASSWORD") + user: Optional[str] = field(default=None) + password: Optional[str] = field(default=None) # Or - access_token: Optional[str] = Field(None, env="FIREBOLT_AUTH_TOKEN") - - account_name: Optional[str] = Field(None, env="FIREBOLT_ACCOUNT") - server: str = Field(..., env="FIREBOLT_SERVER") - default_region: str = Field(..., env="FIREBOLT_DEFAULT_REGION") - use_token_cache: bool = Field(True) + access_token: Optional[str] = field(default_factory=from_env(AUTH_TOKEN_ENV)) - class Config: - """Internal pydantic config.""" + account_name: Optional[str] = field(default_factory=from_env(ACCOUNT_ENV)) + server: str = field(default_factory=from_env(SERVER_ENV)) + default_region: str = field(default_factory=from_env(DEFAULT_REGION_ENV)) + use_token_cache: bool = field(default=True) - env_file = ".env" - - @root_validator - def mutual_exclusive_with_creds(cls, values: dict) -> dict: + def __post_init__(self) -> None: """Validate that either creds or token is provided. Args: @@ -66,9 +83,9 @@ def mutual_exclusive_with_creds(cls, values: dict) -> dict: """ params_present = ( - values.get("user") is not None or values.get("password") is not None, - values.get("access_token") is not None, - values.get("auth") is not None, + self.user is not None or self.password is not None, + self.access_token is not None, + self.auth is not None, ) if sum(params_present) == 0: raise ValueError( @@ -76,7 +93,5 @@ def mutual_exclusive_with_creds(cls, values: dict) -> dict: ) if sum(params_present) > 1: raise ValueError("Provide only one of auth, user/password or access_token") - if any(values.get(f) for f in ("user", "password", "access_token")): + if any((self.user, self.password, self.access_token)): logger.warning(AUTH_CREDENTIALS_DEPRECATION_MESSAGE) - - return values diff --git a/src/firebolt/service/manager.py b/src/firebolt/service/manager.py index 8e9aeca3a57..70657301dfb 100644 --- a/src/firebolt/service/manager.py +++ b/src/firebolt/service/manager.py @@ -41,7 +41,7 @@ def __init__(self, settings: Optional[Settings] = None): assert self.settings.password auth = UsernamePassword( self.settings.user, - self.settings.password.get_secret_value(), + self.settings.password, self.settings.use_token_cache, ) diff --git a/tests/unit/async_db/test_connection.py b/tests/unit/async_db/test_connection.py index 2d665297972..570a3ed1217 100644 --- a/tests/unit/async_db/test_connection.py +++ b/tests/unit/async_db/test_connection.py @@ -272,7 +272,7 @@ async def test_connection_token_caching( async with await connect( database=db_name, username=settings.user, - password=settings.password.get_secret_value(), + password=settings.password, engine_url=settings.server, account_name=settings.account_name, api_endpoint=settings.server, @@ -281,9 +281,7 @@ async def test_connection_token_caching( assert await connection.cursor().execute("select*") == len( python_query_data ) - ts = TokenSecureStorage( - username=settings.user, password=settings.password.get_secret_value() - ) + ts = TokenSecureStorage(username=settings.user, password=settings.password) assert ts.get_cached_token() == access_token, "Invalid token value cached" # Do the same, but with use_token_cache=False @@ -291,7 +289,7 @@ async def test_connection_token_caching( async with await connect( database=db_name, username=settings.user, - password=settings.password.get_secret_value(), + password=settings.password, engine_url=settings.server, account_name=settings.account_name, api_endpoint=settings.server, @@ -300,9 +298,7 @@ async def test_connection_token_caching( assert await connection.cursor().execute("select*") == len( python_query_data ) - ts = TokenSecureStorage( - username=settings.user, password=settings.password.get_secret_value() - ) + ts = TokenSecureStorage(username=settings.user, password=settings.password) assert ( ts.get_cached_token() is None ), "Token is cached even though caching is disabled" @@ -324,7 +320,7 @@ async def test_connect_with_auth( for auth in ( UsernamePassword( settings.user, - settings.password.get_secret_value(), + settings.password, use_token_cache=False, ), Token(access_token), diff --git a/tests/unit/client/conftest.py b/tests/unit/client/conftest.py index 5b83992e3fe..8fe56a0ec33 100644 --- a/tests/unit/client/conftest.py +++ b/tests/unit/client/conftest.py @@ -25,7 +25,7 @@ def test_username(settings: Settings) -> str: @fixture def test_password(settings: Settings) -> str: - return settings.password.get_secret_value() + return settings.password @fixture diff --git a/tests/unit/common/test_settings.py b/tests/unit/common/test_settings.py index 21eea5bec63..58112fae240 100644 --- a/tests/unit/common/test_settings.py +++ b/tests/unit/common/test_settings.py @@ -2,7 +2,6 @@ from typing import Tuple from unittest.mock import Mock, patch -from pydantic import ValidationError from pytest import mark, raises from firebolt.client.auth import Auth @@ -23,8 +22,6 @@ def test_settings_happy_path(fields: Tuple[str]) -> None: for f in fields: field = getattr(s, f) - if hasattr(field, "get_secret_value"): - field = field.get_secret_value() assert ( (field == f) if f != "auth" else isinstance(field, Auth) ), f"Invalid settings value {f}" @@ -43,12 +40,9 @@ def test_settings_happy_path(fields: Tuple[str]) -> None: ), ) def test_settings_auth_credentials(kwargs) -> None: - with raises(ValidationError) as exc_info: + with raises(ValueError) as exc_info: Settings(**kwargs) - err = exc_info.value - assert len(err.errors()) > 0 - @patch("firebolt.common.settings.logger") def test_deprecation_warning_with_env(logger_mock: Mock): @@ -57,5 +51,8 @@ def test_deprecation_warning_with_env(logger_mock: Mock): {"FIREBOLT_USER": "user", "FIREBOLT_PASSWORD": "password"}, clear=True, ): - Settings(server="server", default_region="region") + s = Settings(server="server", default_region="region") logger_mock.warning.assert_not_called() + assert s.auth is not None, "Settings.auth wasn't populated from env variables" + assert s.auth.username == "user", "Invalid username in Settings.auth" + assert s.auth.password == "password", "Invalid password in Settings.auth" diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 7828e349c3c..d9b278334b3 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -4,7 +4,6 @@ import httpx from httpx import Request, Response -from pydantic import SecretStr from pyfakefs.fake_filesystem_unittest import Patcher from pytest import fixture @@ -122,7 +121,7 @@ def settings(server: str, region_1: str, username: str, password: str) -> Settin return Settings( server=server, user=username, - password=SecretStr(password), + password=password, default_region=region_1.name, account_name=None, ) @@ -354,9 +353,7 @@ def check_credentials( assert "username" in body, "Missing username" assert body["username"] == settings.user, "Invalid username" assert "password" in body, "Missing password" - assert ( - body["password"] == settings.password.get_secret_value() - ), "Invalid password" + assert body["password"] == settings.password, "Invalid password" return Response( status_code=httpx.codes.OK, diff --git a/tests/unit/db/test_connection.py b/tests/unit/db/test_connection.py index be1f7ee325d..594a99060ce 100644 --- a/tests/unit/db/test_connection.py +++ b/tests/unit/db/test_connection.py @@ -290,16 +290,14 @@ def test_connection_token_caching( with connect( database=db_name, username=settings.user, - password=settings.password.get_secret_value(), + password=settings.password, engine_url=settings.server, account_name=settings.account_name, api_endpoint=settings.server, use_token_cache=True, ) as connection: assert connection.cursor().execute("select*") == len(python_query_data) - ts = TokenSecureStorage( - username=settings.user, password=settings.password.get_secret_value() - ) + ts = TokenSecureStorage(username=settings.user, password=settings.password) assert ts.get_cached_token() == access_token, "Invalid token value cached" # Do the same, but with use_token_cache=False @@ -307,16 +305,14 @@ def test_connection_token_caching( with connect( database=db_name, username=settings.user, - password=settings.password.get_secret_value(), + password=settings.password, engine_url=settings.server, account_name=settings.account_name, api_endpoint=settings.server, use_token_cache=False, ) as connection: assert connection.cursor().execute("select*") == len(python_query_data) - ts = TokenSecureStorage( - username=settings.user, password=settings.password.get_secret_value() - ) + ts = TokenSecureStorage(username=settings.user, password=settings.password) assert ( ts.get_cached_token() is None ), "Token is cached even though caching is disabled" @@ -338,7 +334,7 @@ def test_connect_with_auth( for auth in ( UsernamePassword( settings.user, - settings.password.get_secret_value(), + settings.password, use_token_cache=False, ), Token(access_token), diff --git a/tests/unit/service/test_resource_manager.py b/tests/unit/service/test_resource_manager.py index a76f6f8ba05..42060b2785e 100644 --- a/tests/unit/service/test_resource_manager.py +++ b/tests/unit/service/test_resource_manager.py @@ -45,7 +45,7 @@ def test_rm_credentials( rm.client.get(url) auth_username_password_settings = Settings( - auth=UsernamePassword(settings.user, settings.password.get_secret_value()), + auth=UsernamePassword(settings.user, settings.password), server=settings.server, default_region=settings.default_region, ) @@ -87,7 +87,7 @@ def test_rm_token_cache( with Patcher(): local_settings = Settings( user=settings.user, - password=settings.password.get_secret_value(), + password=settings.password, server=settings.server, default_region=settings.default_region, use_token_cache=True, @@ -95,14 +95,14 @@ def test_rm_token_cache( rm = ResourceManager(local_settings) rm.client.get(url) - ts = TokenSecureStorage(settings.user, settings.password.get_secret_value()) + ts = TokenSecureStorage(settings.user, settings.password) assert ts.get_cached_token() == access_token, "Invalid token value cached" # Do the same, but with use_token_cache=False with Patcher(): local_settings = Settings( user=settings.user, - password=settings.password.get_secret_value(), + password=settings.password, server=settings.server, default_region=settings.default_region, use_token_cache=False, @@ -110,7 +110,7 @@ def test_rm_token_cache( rm = ResourceManager(local_settings) rm.client.get(url) - ts = TokenSecureStorage(settings.user, settings.password.get_secret_value()) + ts = TokenSecureStorage(settings.user, settings.password) assert ( ts.get_cached_token() is None ), "Token is cached even though caching is disabled" From 3989f0347a091069f979d2b6268d34cae9d9353b Mon Sep 17 00:00:00 2001 From: Stepan Burlakov Date: Wed, 10 May 2023 12:24:37 +0300 Subject: [PATCH 3/3] address comments --- tests/unit/common/test_settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/common/test_settings.py b/tests/unit/common/test_settings.py index 58112fae240..3d1cb96d81b 100644 --- a/tests/unit/common/test_settings.py +++ b/tests/unit/common/test_settings.py @@ -45,7 +45,7 @@ def test_settings_auth_credentials(kwargs) -> None: @patch("firebolt.common.settings.logger") -def test_deprecation_warning_with_env(logger_mock: Mock): +def test_no_deprecation_warning_with_env(logger_mock: Mock): with patch.dict( os.environ, {"FIREBOLT_USER": "user", "FIREBOLT_PASSWORD": "password"},