From d3c980febc0ef446648f0a41409643afeace6f8c Mon Sep 17 00:00:00 2001 From: Aleksandr Goncharov Date: Wed, 20 Mar 2024 14:27:02 +0300 Subject: [PATCH 01/16] Use in-memory account info storage when using env vars for auth --- b2/_internal/_cli/b2api.py | 6 ++ b2/_internal/_cli/b2args.py | 10 ++ b2/_internal/console_tool.py | 50 ++++------ test/integration/conftest.py | 14 ++- test/integration/test_b2_command_line.py | 111 ++++++++++++++++++++++ test/unit/test_console_tool.py | 82 +++++++++++++--- test/unit/test_represent_file_metadata.py | 15 ++- 7 files changed, 242 insertions(+), 46 deletions(-) diff --git a/b2/_internal/_cli/b2api.py b/b2/_internal/_cli/b2api.py index adfd6a797..bf2bbffd8 100644 --- a/b2/_internal/_cli/b2api.py +++ b/b2/_internal/_cli/b2api.py @@ -15,6 +15,8 @@ AuthInfoCache, B2Api, B2HttpApiConfig, + InMemoryAccountInfo, + InMemoryCache, SqliteAccountInfo, ) @@ -46,5 +48,9 @@ def _get_b2api_for_profile(profile: Optional[str] = None, **kwargs) -> B2Api: return b2api +def _get_inmemory_b2api(**kwargs) -> B2Api: + return B2Api(InMemoryAccountInfo(), cache=InMemoryCache(), **kwargs) + + def _get_b2httpapiconfig(): return B2HttpApiConfig(user_agent_append=os.environ.get(B2_USER_AGENT_APPEND_ENV_VAR),) diff --git a/b2/_internal/_cli/b2args.py b/b2/_internal/_cli/b2args.py index b27cd7686..e3dc73b35 100644 --- a/b2/_internal/_cli/b2args.py +++ b/b2/_internal/_cli/b2args.py @@ -12,9 +12,15 @@ """ import argparse import functools +from os import environ +from typing import Optional from b2._internal._cli.arg_parser_types import wrap_with_argument_type_error from b2._internal._cli.argcompleters import b2uri_file_completer +from b2._internal._cli.const import ( + B2_APPLICATION_KEY_ENV_VAR, + B2_APPLICATION_KEY_ID_ENV_VAR, +) from b2._internal._utils.uri import B2URI, B2URIBase, parse_b2_uri @@ -76,3 +82,7 @@ def add_b2id_or_file_like_b2_uri_argument(parser: argparse.ArgumentParser, name= type=B2ID_OR_FILE_LIKE_B2_URI_ARG_TYPE, help="B2 URI pointing to a file, e.g. b2://yourBucket/file.txt or b2id://fileId", ).completer = b2uri_file_completer + + +def get_keyid_and_key_from_env_vars() -> tuple[Optional[str], Optional[str]]: + return environ.get(B2_APPLICATION_KEY_ID_ENV_VAR), environ.get(B2_APPLICATION_KEY_ENV_VAR) diff --git a/b2/_internal/console_tool.py b/b2/_internal/console_tool.py index e60618f3b..513ad435c 100644 --- a/b2/_internal/console_tool.py +++ b/b2/_internal/console_tool.py @@ -130,10 +130,11 @@ AutocompleteInstallError, autocomplete_install, ) -from b2._internal._cli.b2api import _get_b2api_for_profile +from b2._internal._cli.b2api import _get_b2api_for_profile, _get_inmemory_b2api from b2._internal._cli.b2args import ( add_b2id_or_b2_uri_argument, add_b2id_or_file_like_b2_uri_argument, + get_keyid_and_key_from_env_vars, ) from b2._internal._cli.const import ( B2_APPLICATION_KEY_ENV_VAR, @@ -4179,11 +4180,11 @@ class ConsoleTool: Implements the commands available in the B2 command-line tool using the B2Api library. - Uses a ``b2sdk.SqlitedAccountInfo`` object to keep account data between runs. + Uses a ``b2sdk.SqlitedAccountInfo`` object to keep account data between runs + (unless authorization is performed via environment variables). """ - def __init__(self, b2_api: B2Api | None, stdout, stderr): - self.api = b2_api + def __init__(self, stdout, stderr): self.stdout = stdout self.stderr = stderr self.b2_binary_name = 'b2' @@ -4214,32 +4215,20 @@ def run_command(self, argv): # in case any control characters slip through escaping, just delete them self.stdout = NoControlCharactersStdout(self.stdout) self.stderr = NoControlCharactersStdout(self.stderr) - if self.api: - if ( - args.profile or getattr(args, 'write_buffer_size', None) or - getattr(args, 'skip_hash_verification', None) or - getattr(args, 'max_download_streams_per_file', None) - ): - self._print_stderr( - 'ERROR: cannot change configuration on already initialized object' - ) - return 1 + kwargs = {} + with suppress(AttributeError): + kwargs['save_to_buffer_size'] = args.write_buffer_size + with suppress(AttributeError): + kwargs['check_download_hash'] = not args.skip_hash_verification + with suppress(AttributeError): + kwargs['max_download_streams_per_file'] = args.max_download_streams_per_file + + if all(get_keyid_and_key_from_env_vars()) and args.command_class not in (AuthorizeAccount, ClearAccount): + # when user specifies keys via env variables, we switch to in-memory account info + self.api = _get_inmemory_b2api(**kwargs) else: - kwargs = { - 'profile': args.profile, - } - - if 'write_buffer_size' in args: - kwargs['save_to_buffer_size'] = args.write_buffer_size - - if 'skip_hash_verification' in args: - kwargs['check_download_hash'] = not args.skip_hash_verification - - if 'max_download_streams_per_file' in args: - kwargs['max_download_streams_per_file'] = args.max_download_streams_per_file - - self.api = _get_b2api_for_profile(**kwargs) + self.api = _get_b2api_for_profile(profile=args.profile, **kwargs) b2_command = B2(self) command_class = b2_command.run(args) @@ -4278,8 +4267,7 @@ def authorize_from_env(self, command_class): if not command_class.REQUIRES_AUTH: return 0 - key_id = os.environ.get(B2_APPLICATION_KEY_ID_ENV_VAR) - key = os.environ.get(B2_APPLICATION_KEY_ENV_VAR) + key_id, key = get_keyid_and_key_from_env_vars() if key_id is None and key is None: return 0 @@ -4349,7 +4337,7 @@ def _setup_logging(cls, args, argv): def main(): - ct = ConsoleTool(b2_api=None, stdout=sys.stdout, stderr=sys.stderr) + ct = ConsoleTool(stdout=sys.stdout, stderr=sys.stderr) exit_status = ct.run_command(sys.argv) logger.info('\\\\ %s %s %s //', SEPARATOR, ('exit=%s' % exit_status).center(8), SEPARATOR) diff --git a/test/integration/conftest.py b/test/integration/conftest.py index a734862b9..7634d7822 100755 --- a/test/integration/conftest.py +++ b/test/integration/conftest.py @@ -205,11 +205,12 @@ def monkeysession(): @pytest.fixture(scope='session', autouse=True) -def auto_change_account_info_dir(monkeysession) -> dir: +def auto_change_account_info_dir(monkeysession) -> str: """ Automatically for the whole testing: 1) temporary remove B2_APPLICATION_KEY and B2_APPLICATION_KEY_ID from environment 2) create a temporary directory for storing account info database + 3) set B2_ACCOUNT_INFO_ENV_VAR to point to the temporary account info file """ monkeysession.delenv('B2_APPLICATION_KEY_ID', raising=False) @@ -269,6 +270,17 @@ def b2_tool(global_b2_tool): return global_b2_tool +@pytest.fixture +def account_info_file() -> pathlib.Path: + return pathlib.Path(os.environ[B2_ACCOUNT_INFO_ENV_VAR]).expanduser() + + +@pytest.fixture +def no_account_info_file(account_info_file): + """Remove account info file to deauthorize b2_tool""" + yield account_info_file.unlink() + + @pytest.fixture def schedule_bucket_cleanup(global_b2_tool): """ diff --git a/test/integration/test_b2_command_line.py b/test/integration/test_b2_command_line.py index 7ee1b388c..562183461 100755 --- a/test/integration/test_b2_command_line.py +++ b/test/integration/test_b2_command_line.py @@ -24,6 +24,7 @@ from pathlib import Path import pytest +from b2sdk.account_info.exception import MissingAccountData from b2sdk.v2 import ( B2_ACCOUNT_INFO_ENV_VAR, SSE_C_KEY_ID_FILE_INFO_KEY_NAME, @@ -33,9 +34,14 @@ FileRetentionSetting, LegalHold, RetentionMode, + SqliteAccountInfo, fix_windows_path_limit, ) +from b2._internal._cli.const import ( + B2_APPLICATION_KEY_ENV_VAR, + B2_APPLICATION_KEY_ID_ENV_VAR, +) from b2._internal.console_tool import current_time_millis from ..helpers import skip_on_windows @@ -56,6 +62,111 @@ ) +def test_authorize_account_via_params_saving_credentials( + b2_tool, + application_key, + application_key_id, + account_info_file, + no_account_info_file, +): + """ + When calling `authorize-account` and passing credentials as params, + we want the credentials to be saved. + """ + + assert not account_info_file.exists() + assert B2_APPLICATION_KEY_ID_ENV_VAR not in os.environ + assert B2_APPLICATION_KEY_ENV_VAR not in os.environ + + b2_tool.should_succeed(['authorize-account', application_key_id, application_key]) + + assert account_info_file.exists() + account_info = SqliteAccountInfo() + assert account_info.get_application_key() == application_key + assert account_info.get_application_key_id() == application_key_id + + +def test_authorize_account_via_env_vars_saving_credentials( + b2_tool, + application_key, + application_key_id, + account_info_file, + no_account_info_file, +): + """ + When calling `authorize-account` and passing credentials + via env vars, we still want the credentials to be saved. + """ + + assert not account_info_file.exists() + assert B2_APPLICATION_KEY_ID_ENV_VAR not in os.environ + assert B2_APPLICATION_KEY_ENV_VAR not in os.environ + + b2_tool.should_succeed(['authorize-account'], additional_env={ + B2_APPLICATION_KEY_ID_ENV_VAR: application_key_id, + B2_APPLICATION_KEY_ENV_VAR: application_key, + }) + + assert account_info_file.exists() + account_info = SqliteAccountInfo() + assert account_info.get_application_key() == application_key + assert account_info.get_application_key_id() == application_key_id + + +def test_clear_account_with_env_vars( + b2_tool, + application_key, + application_key_id, + account_info_file, +): + """ + When calling `clear-account` and passing credentials via env vars, + we want the credentials to be removed from the file. + """ + + assert account_info_file.exists() + account_info = SqliteAccountInfo() + assert account_info.get_application_key() == application_key + assert account_info.get_application_key_id() == application_key_id + + b2_tool.should_succeed(['clear-account'], additional_env={ + B2_APPLICATION_KEY_ID_ENV_VAR: application_key_id, + B2_APPLICATION_KEY_ENV_VAR: application_key, + }) + + assert account_info_file.exists() + account_info = SqliteAccountInfo() + with pytest.raises(MissingAccountData): + account_info.get_application_key() + with pytest.raises(MissingAccountData): + account_info.get_application_key_id() + + +def test_command_with_env_vars_not_saving_credentials( + b2_tool, + application_key, + application_key_id, + account_info_file, + uploaded_sample_file, + no_account_info_file, +): + """ + When calling any command other then `authorize-account` and passing credentials + via env vars, we don't want them to be saved. + """ + + assert not account_info_file.exists() + assert B2_APPLICATION_KEY_ID_ENV_VAR not in os.environ + assert B2_APPLICATION_KEY_ENV_VAR not in os.environ + + b2_tool.should_succeed(['ls', f"b2id://{uploaded_sample_file['fileId']}"], additional_env={ + B2_APPLICATION_KEY_ID_ENV_VAR: application_key_id, + B2_APPLICATION_KEY_ENV_VAR: application_key, + }) + + assert not account_info_file.exists() + + @pytest.fixture def uploaded_sample_file(b2_tool, bucket_name, sample_filepath): return b2_tool.should_succeed_json( diff --git a/test/unit/test_console_tool.py b/test/unit/test_console_tool.py index 495a7390c..49ebf63ba 100644 --- a/test/unit/test_console_tool.py +++ b/test/unit/test_console_tool.py @@ -12,11 +12,11 @@ import os import pathlib import re -import unittest.mock as mock from io import StringIO from itertools import chain, product from test.helpers import skip_on_windows from typing import List, Optional +from unittest import mock import pytest from b2sdk import v1 @@ -57,11 +57,29 @@ class BaseConsoleToolTest(TestBase): def setUp(self): self.account_info = StubAccountInfo() - self.b2_api = B2Api( - self.account_info, None, api_config=B2HttpApiConfig(_raw_api_class=RawSimulator) - ) + # this is a hack - B2HttpApiConfig expects a class, but we want to use an instance + # which will be reused during the test, thus instead of class we pass a lambda which + # returns already predefined instance + self.raw_simulator = RawSimulator() + self.api_config = B2HttpApiConfig(_raw_api_class=lambda *args, **kwargs: self.raw_simulator) + + def _get_b2api(**kwargs) -> B2Api: + kwargs.pop('profile', None) + return B2Api( + account_info=self.account_info, + cache=None, + api_config=self.api_config, + **kwargs, + ) + + self.mp = pytest.MonkeyPatch() + self.mp.setattr('b2._internal.console_tool._get_b2api_for_profile', _get_b2api) + self.mp.setattr('b2._internal.console_tool._get_inmemory_b2api', _get_b2api) + + self.b2_api = _get_b2api() self.raw_api = self.b2_api.session.raw_api - (self.account_id, self.master_key) = self.raw_api.create_account() + self.account_id, self.master_key = self.raw_api.create_account() + for env_var_name in [ B2_APPLICATION_KEY_ID_ENV_VAR, B2_APPLICATION_KEY_ENV_VAR, @@ -69,6 +87,10 @@ def setUp(self): ]: os.environ.pop(env_var_name, None) + def tearDown(self) -> None: + super().tearDown() + self.mp.undo() + def _get_stdouterr(self): stdout = StringIO() stderr = StringIO() @@ -80,7 +102,7 @@ def _run_command_ignore_output(self, argv): success, but ignoring the stdout. """ stdout, stderr = self._get_stdouterr() - actual_status = self.console_tool_class(self.b2_api, stdout, + actual_status = self.console_tool_class(stdout, stderr).run_command(['b2'] + argv) actual_stderr = self._trim_trailing_spaces(stderr.getvalue()) @@ -165,6 +187,12 @@ def _authorize_account(self): """ self._run_command_ignore_output(['authorize-account', self.account_id, self.master_key]) + def _clear_account(self): + """ + Clear account auth data + """ + self._run_command_ignore_output(['clear-account']) + def _create_my_bucket(self): self._run_command(['create-bucket', 'my-bucket', 'allPublic'], 'bucket_0\n', '', 0) @@ -194,7 +222,7 @@ def _run_command( """ expected_stderr = self._normalize_expected_output(expected_stderr, format_vars) stdout, stderr = self._get_stdouterr() - console_tool = self.console_tool_class(self.b2_api, stdout, stderr) + console_tool = self.console_tool_class(stdout, stderr) try: actual_status = console_tool.run_command(['b2'] + argv) except SystemExit as e: @@ -1777,7 +1805,7 @@ def test_get_bucket_with_hidden(self): # Hide some new files. Don't check the results here; it will be clear enough that # something has failed if the output of 'get-bucket' does not match the canon. stdout, stderr = self._get_stdouterr() - console_tool = self.console_tool_class(self.b2_api, stdout, stderr) + console_tool = self.console_tool_class(stdout, stderr) console_tool.run_command(['b2', 'hide-file', 'my-bucket', 'hidden1']) console_tool.run_command(['b2', 'hide-file', 'my-bucket', 'hidden2']) console_tool.run_command(['b2', 'hide-file', 'my-bucket', 'hidden3']) @@ -1838,7 +1866,7 @@ def test_get_bucket_complex(self): # Hide some new files. Don't check the results here; it will be clear enough that # something has failed if the output of 'get-bucket' does not match the canon. stdout, stderr = self._get_stdouterr() - console_tool = self.console_tool_class(self.b2_api, stdout, stderr) + console_tool = self.console_tool_class(stdout, stderr) console_tool.run_command(['b2', 'hide-file', 'my-bucket', '1/hidden1']) console_tool.run_command(['b2', 'hide-file', 'my-bucket', '1/hidden1']) console_tool.run_command(['b2', 'hide-file', 'my-bucket', '1/hidden2']) @@ -2405,10 +2433,11 @@ def test_bad_terminal(self): ] + list(range(25)) ) stderr = mock.MagicMock() - console_tool = self.console_tool_class(self.b2_api, stdout, stderr) + console_tool = self.console_tool_class(stdout, stderr) console_tool.run_command(['b2', 'authorize-account', self.account_id, self.master_key]) def test_passing_api_parameters(self): + self._authorize_account() commands = [ [ 'b2', 'download-file-by-name', '--profile', 'nonexistent', 'dummy-name', @@ -2433,12 +2462,11 @@ def test_passing_api_parameters(self): ] for command, params in product(commands, parameters): console_tool = self.console_tool_class( - None, # do not initialize b2 api to allow passing in additional parameters mock.MagicMock(), mock.MagicMock(), ) - args = list(map(str, filter(None, chain.from_iterable(params.items())))) + args = [str(val) for val in chain.from_iterable(params.items()) if val] console_tool.run_command(command + args) download_manager = console_tool.api.services.download_manager @@ -2451,7 +2479,35 @@ def test_passing_api_parameters(self): ) assert parallel_strategy.max_streams == params['--max-download-streams-per-file'] - @pytest.mark.apiver(from_ver=4) + def test_passing_api_parameters_with_auth_env_vars(self): + + os.environ[B2_APPLICATION_KEY_ID_ENV_VAR] = self.account_id + os.environ[B2_APPLICATION_KEY_ENV_VAR] = self.master_key + + command = [ + 'b2', 'download-file-by-id', 'dummy-id', 'dummy-local-file-name', + '--write-buffer-size', '123', + '--max-download-streams-per-file', '5', + '--skip-hash-verification', + ] + + console_tool = self.console_tool_class( + mock.MagicMock(), + mock.MagicMock(), + ) + console_tool.run_command(command) + + download_manager = console_tool.api.services.download_manager + assert download_manager.write_buffer_size == 123 + assert download_manager.check_hash is False + + parallel_strategy = one( + strategy for strategy in download_manager.strategies + if isinstance(strategy, download_manager.PARALLEL_DOWNLOADER_CLASS) + ) + assert parallel_strategy.max_streams == 5 + + @pytest.mark.cli_version(from_version=4) def test_ls_b2id(self): self._authorize_account() self._create_my_bucket() diff --git a/test/unit/test_represent_file_metadata.py b/test/unit/test_represent_file_metadata.py index df78d6179..2544edefa 100644 --- a/test/unit/test_represent_file_metadata.py +++ b/test/unit/test_represent_file_metadata.py @@ -24,6 +24,7 @@ RetentionMode, StubAccountInfo, ) +import pytest from b2._internal.console_tool import ConsoleTool, DownloadCommand @@ -60,9 +61,21 @@ def setUp(self): 'production', self.restricted_key_id, self.restricted_key ) + def _get_b2api(**kwargs) -> B2Api: + kwargs.pop('profile', None) + return self.master_b2_api + + self.mp = pytest.MonkeyPatch() + self.mp.setattr('b2._internal.console_tool._get_b2api_for_profile', _get_b2api) + self.mp.setattr('b2._internal.console_tool._get_inmemory_b2api', _get_b2api) + self.stdout = StringIO() self.stderr = StringIO() - self.console_tool = ConsoleTool(self.master_b2_api, self.stdout, self.stderr) + self.console_tool = ConsoleTool(self.stdout, self.stderr) + + def tearDown(self): + self.mp.undo() + super().tearDown() def assertRetentionRepr(self, file_id: str, api: B2Api, expected_repr: str): file_version = api.get_file_info(file_id) From 531ffff9cbbbfac93b8debdcab7b098b2d17cf28 Mon Sep 17 00:00:00 2001 From: Aleksandr Goncharov Date: Wed, 20 Mar 2024 14:30:09 +0300 Subject: [PATCH 02/16] Lint fixes --- b2/_internal/_cli/b2args.py | 4 +-- b2/_internal/console_tool.py | 3 +- ...ot-saving-auth-data-when-env-vars.fixed.md | 1 + test/integration/test_b2_command_line.py | 33 ++++++++++++------- test/unit/test_console_tool.py | 14 +++++--- test/unit/test_represent_file_metadata.py | 2 +- 6 files changed, 36 insertions(+), 21 deletions(-) create mode 100644 changelog.d/+not-saving-auth-data-when-env-vars.fixed.md diff --git a/b2/_internal/_cli/b2args.py b/b2/_internal/_cli/b2args.py index e3dc73b35..2a71a0cd7 100644 --- a/b2/_internal/_cli/b2args.py +++ b/b2/_internal/_cli/b2args.py @@ -13,7 +13,7 @@ import argparse import functools from os import environ -from typing import Optional +from typing import Optional, Tuple from b2._internal._cli.arg_parser_types import wrap_with_argument_type_error from b2._internal._cli.argcompleters import b2uri_file_completer @@ -84,5 +84,5 @@ def add_b2id_or_file_like_b2_uri_argument(parser: argparse.ArgumentParser, name= ).completer = b2uri_file_completer -def get_keyid_and_key_from_env_vars() -> tuple[Optional[str], Optional[str]]: +def get_keyid_and_key_from_env_vars() -> Tuple[Optional[str], Optional[str]]: return environ.get(B2_APPLICATION_KEY_ID_ENV_VAR), environ.get(B2_APPLICATION_KEY_ENV_VAR) diff --git a/b2/_internal/console_tool.py b/b2/_internal/console_tool.py index 513ad435c..5449728c0 100644 --- a/b2/_internal/console_tool.py +++ b/b2/_internal/console_tool.py @@ -4224,7 +4224,8 @@ def run_command(self, argv): with suppress(AttributeError): kwargs['max_download_streams_per_file'] = args.max_download_streams_per_file - if all(get_keyid_and_key_from_env_vars()) and args.command_class not in (AuthorizeAccount, ClearAccount): + if all(get_keyid_and_key_from_env_vars() + ) and args.command_class not in (AuthorizeAccount, ClearAccount): # when user specifies keys via env variables, we switch to in-memory account info self.api = _get_inmemory_b2api(**kwargs) else: diff --git a/changelog.d/+not-saving-auth-data-when-env-vars.fixed.md b/changelog.d/+not-saving-auth-data-when-env-vars.fixed.md new file mode 100644 index 000000000..7c422c46f --- /dev/null +++ b/changelog.d/+not-saving-auth-data-when-env-vars.fixed.md @@ -0,0 +1 @@ +Fix persisting credentials on disk even if environment variables were used for authentication. diff --git a/test/integration/test_b2_command_line.py b/test/integration/test_b2_command_line.py index 562183461..dca74e328 100755 --- a/test/integration/test_b2_command_line.py +++ b/test/integration/test_b2_command_line.py @@ -102,10 +102,13 @@ def test_authorize_account_via_env_vars_saving_credentials( assert B2_APPLICATION_KEY_ID_ENV_VAR not in os.environ assert B2_APPLICATION_KEY_ENV_VAR not in os.environ - b2_tool.should_succeed(['authorize-account'], additional_env={ - B2_APPLICATION_KEY_ID_ENV_VAR: application_key_id, - B2_APPLICATION_KEY_ENV_VAR: application_key, - }) + b2_tool.should_succeed( + ['authorize-account'], + additional_env={ + B2_APPLICATION_KEY_ID_ENV_VAR: application_key_id, + B2_APPLICATION_KEY_ENV_VAR: application_key, + } + ) assert account_info_file.exists() account_info = SqliteAccountInfo() @@ -129,10 +132,13 @@ def test_clear_account_with_env_vars( assert account_info.get_application_key() == application_key assert account_info.get_application_key_id() == application_key_id - b2_tool.should_succeed(['clear-account'], additional_env={ - B2_APPLICATION_KEY_ID_ENV_VAR: application_key_id, - B2_APPLICATION_KEY_ENV_VAR: application_key, - }) + b2_tool.should_succeed( + ['clear-account'], + additional_env={ + B2_APPLICATION_KEY_ID_ENV_VAR: application_key_id, + B2_APPLICATION_KEY_ENV_VAR: application_key, + } + ) assert account_info_file.exists() account_info = SqliteAccountInfo() @@ -159,10 +165,13 @@ def test_command_with_env_vars_not_saving_credentials( assert B2_APPLICATION_KEY_ID_ENV_VAR not in os.environ assert B2_APPLICATION_KEY_ENV_VAR not in os.environ - b2_tool.should_succeed(['ls', f"b2id://{uploaded_sample_file['fileId']}"], additional_env={ - B2_APPLICATION_KEY_ID_ENV_VAR: application_key_id, - B2_APPLICATION_KEY_ENV_VAR: application_key, - }) + b2_tool.should_succeed( + ['ls', f"b2id://{uploaded_sample_file['fileId']}"], + additional_env={ + B2_APPLICATION_KEY_ID_ENV_VAR: application_key_id, + B2_APPLICATION_KEY_ENV_VAR: application_key, + } + ) assert not account_info_file.exists() diff --git a/test/unit/test_console_tool.py b/test/unit/test_console_tool.py index 49ebf63ba..554bba5fe 100644 --- a/test/unit/test_console_tool.py +++ b/test/unit/test_console_tool.py @@ -102,8 +102,7 @@ def _run_command_ignore_output(self, argv): success, but ignoring the stdout. """ stdout, stderr = self._get_stdouterr() - actual_status = self.console_tool_class(stdout, - stderr).run_command(['b2'] + argv) + actual_status = self.console_tool_class(stdout, stderr).run_command(['b2'] + argv) actual_stderr = self._trim_trailing_spaces(stderr.getvalue()) if actual_stderr != '': @@ -2485,9 +2484,14 @@ def test_passing_api_parameters_with_auth_env_vars(self): os.environ[B2_APPLICATION_KEY_ENV_VAR] = self.master_key command = [ - 'b2', 'download-file-by-id', 'dummy-id', 'dummy-local-file-name', - '--write-buffer-size', '123', - '--max-download-streams-per-file', '5', + 'b2', + 'download-file-by-id', + 'dummy-id', + 'dummy-local-file-name', + '--write-buffer-size', + '123', + '--max-download-streams-per-file', + '5', '--skip-hash-verification', ] diff --git a/test/unit/test_represent_file_metadata.py b/test/unit/test_represent_file_metadata.py index 2544edefa..e92f19225 100644 --- a/test/unit/test_represent_file_metadata.py +++ b/test/unit/test_represent_file_metadata.py @@ -10,6 +10,7 @@ from io import StringIO +import pytest from b2sdk.v2 import ( SSE_B2_AES, B2Api, @@ -24,7 +25,6 @@ RetentionMode, StubAccountInfo, ) -import pytest from b2._internal.console_tool import ConsoleTool, DownloadCommand From 40e92548db132c9976a0f18eb3a297b40696247b Mon Sep 17 00:00:00 2001 From: Aleksandr Goncharov Date: Thu, 21 Mar 2024 11:50:44 +0300 Subject: [PATCH 03/16] Expose last instance of ConsoleTool after BaseConsoleToolTest.run_command --- test/unit/console_tool/test_download_file.py | 2 +- test/unit/console_tool/test_upload_file.py | 2 +- test/unit/test_console_tool.py | 8 +++++--- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/test/unit/console_tool/test_download_file.py b/test/unit/console_tool/test_download_file.py index 71c214032..fb5dd2c78 100644 --- a/test/unit/console_tool/test_download_file.py +++ b/test/unit/console_tool/test_download_file.py @@ -205,4 +205,4 @@ def test__download_file__threads(b2_cli, local_file, uploaded_file, tmp_path): ) assert output_path.read_text() == uploaded_file['content'] - assert b2_cli.b2_api.services.download_manager.get_thread_pool_size() == num_threads + assert b2_cli.console_tool.api.services.download_manager.get_thread_pool_size() == num_threads diff --git a/test/unit/console_tool/test_upload_file.py b/test/unit/console_tool/test_upload_file.py index 84089b9ab..69107331e 100644 --- a/test/unit/console_tool/test_upload_file.py +++ b/test/unit/console_tool/test_upload_file.py @@ -155,4 +155,4 @@ def test_upload_file__threads_setting(b2_cli, bucket, tmp_path): remove_version=True, ) - assert b2_cli.b2_api.services.upload_manager.get_thread_pool_size() == num_threads + assert b2_cli.console_tool.api.services.upload_manager.get_thread_pool_size() == num_threads diff --git a/test/unit/test_console_tool.py b/test/unit/test_console_tool.py index 554bba5fe..79149e73e 100644 --- a/test/unit/test_console_tool.py +++ b/test/unit/test_console_tool.py @@ -217,13 +217,15 @@ def _run_command( format_vars. The ConsoleTool is stateless, so we can make a new one for each - call, with a fresh stdout and stderr + call, with a fresh stdout and stderr. However, last instance of ConsoleTool + is stored in `self.console_tool`, may be handy for testing internals + of the tool after last command invocation. """ expected_stderr = self._normalize_expected_output(expected_stderr, format_vars) stdout, stderr = self._get_stdouterr() - console_tool = self.console_tool_class(stdout, stderr) + self.console_tool = self.console_tool_class(stdout, stderr) try: - actual_status = console_tool.run_command(['b2'] + argv) + actual_status = self.console_tool.run_command(['b2'] + argv) except SystemExit as e: actual_status = e.code From 6d3d0fc7c86118206c6c38ab20376430c1d13044 Mon Sep 17 00:00:00 2001 From: Aleksandr Goncharov Date: Thu, 21 Mar 2024 12:40:28 +0300 Subject: [PATCH 04/16] Minor fixes after rebase --- test/unit/test_console_tool.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/unit/test_console_tool.py b/test/unit/test_console_tool.py index 79149e73e..456a50bb6 100644 --- a/test/unit/test_console_tool.py +++ b/test/unit/test_console_tool.py @@ -12,6 +12,7 @@ import os import pathlib import re +from functools import lru_cache from io import StringIO from itertools import chain, product from test.helpers import skip_on_windows @@ -63,6 +64,7 @@ def setUp(self): self.raw_simulator = RawSimulator() self.api_config = B2HttpApiConfig(_raw_api_class=lambda *args, **kwargs: self.raw_simulator) + @lru_cache(maxsize=None) def _get_b2api(**kwargs) -> B2Api: kwargs.pop('profile', None) return B2Api( @@ -2513,7 +2515,7 @@ def test_passing_api_parameters_with_auth_env_vars(self): ) assert parallel_strategy.max_streams == 5 - @pytest.mark.cli_version(from_version=4) + @pytest.mark.apiver(from_ver=4) def test_ls_b2id(self): self._authorize_account() self._create_my_bucket() From a6fbceec8c4ef0300359586eb0134214fbd0e158 Mon Sep 17 00:00:00 2001 From: Aleksandr Goncharov Date: Thu, 21 Mar 2024 12:50:52 +0300 Subject: [PATCH 05/16] Fix integration test due to not creating account info file --- test/integration/test_b2_command_line.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/test_b2_command_line.py b/test/integration/test_b2_command_line.py index dca74e328..e5d2cca01 100755 --- a/test/integration/test_b2_command_line.py +++ b/test/integration/test_b2_command_line.py @@ -558,7 +558,7 @@ def test_account(b2_tool, cli_version): ['create-bucket', bucket_name, 'allPrivate', *b2_tool.get_bucket_info_args()] ) b2_tool.should_succeed(['delete-bucket', bucket_name]) - assert os.path.exists(new_creds), 'sqlite file not created' + assert not os.path.exists(new_creds), 'sqlite file was created' os.environ.pop('B2_APPLICATION_KEY') os.environ.pop('B2_APPLICATION_KEY_ID') From 9a502ad23940dd462d673ef857f58f9ada2db726 Mon Sep 17 00:00:00 2001 From: Aleksandr Goncharov Date: Thu, 21 Mar 2024 13:08:27 +0300 Subject: [PATCH 06/16] Fix integration test due to lack of valid account for testing --- test/integration/test_b2_command_line.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/integration/test_b2_command_line.py b/test/integration/test_b2_command_line.py index e5d2cca01..ba6d1f437 100755 --- a/test/integration/test_b2_command_line.py +++ b/test/integration/test_b2_command_line.py @@ -153,8 +153,9 @@ def test_command_with_env_vars_not_saving_credentials( application_key, application_key_id, account_info_file, - uploaded_sample_file, + bucket_name, no_account_info_file, + b2_uri_args, ): """ When calling any command other then `authorize-account` and passing credentials @@ -166,7 +167,7 @@ def test_command_with_env_vars_not_saving_credentials( assert B2_APPLICATION_KEY_ENV_VAR not in os.environ b2_tool.should_succeed( - ['ls', f"b2id://{uploaded_sample_file['fileId']}"], + ['ls', '--long', *b2_uri_args(bucket_name)], additional_env={ B2_APPLICATION_KEY_ID_ENV_VAR: application_key_id, B2_APPLICATION_KEY_ENV_VAR: application_key, From bd44cb19f258cd35c0699eb6d4838391d58452e4 Mon Sep 17 00:00:00 2001 From: Aleksandr Goncharov Date: Thu, 21 Mar 2024 14:34:24 +0300 Subject: [PATCH 07/16] Don't delete account_info_file --- test/integration/conftest.py | 6 ------ test/integration/test_b2_command_line.py | 19 ++++++++++++------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/test/integration/conftest.py b/test/integration/conftest.py index 7634d7822..e4d070996 100755 --- a/test/integration/conftest.py +++ b/test/integration/conftest.py @@ -275,12 +275,6 @@ def account_info_file() -> pathlib.Path: return pathlib.Path(os.environ[B2_ACCOUNT_INFO_ENV_VAR]).expanduser() -@pytest.fixture -def no_account_info_file(account_info_file): - """Remove account info file to deauthorize b2_tool""" - yield account_info_file.unlink() - - @pytest.fixture def schedule_bucket_cleanup(global_b2_tool): """ diff --git a/test/integration/test_b2_command_line.py b/test/integration/test_b2_command_line.py index ba6d1f437..1b043b890 100755 --- a/test/integration/test_b2_command_line.py +++ b/test/integration/test_b2_command_line.py @@ -67,14 +67,14 @@ def test_authorize_account_via_params_saving_credentials( application_key, application_key_id, account_info_file, - no_account_info_file, ): """ When calling `authorize-account` and passing credentials as params, we want the credentials to be saved. """ - assert not account_info_file.exists() + b2_tool.should_succeed(['clear-account']) + assert B2_APPLICATION_KEY_ID_ENV_VAR not in os.environ assert B2_APPLICATION_KEY_ENV_VAR not in os.environ @@ -91,14 +91,14 @@ def test_authorize_account_via_env_vars_saving_credentials( application_key, application_key_id, account_info_file, - no_account_info_file, ): """ When calling `authorize-account` and passing credentials via env vars, we still want the credentials to be saved. """ - assert not account_info_file.exists() + b2_tool.should_succeed(['clear-account']) + assert B2_APPLICATION_KEY_ID_ENV_VAR not in os.environ assert B2_APPLICATION_KEY_ENV_VAR not in os.environ @@ -154,7 +154,6 @@ def test_command_with_env_vars_not_saving_credentials( application_key_id, account_info_file, bucket_name, - no_account_info_file, b2_uri_args, ): """ @@ -162,7 +161,8 @@ def test_command_with_env_vars_not_saving_credentials( via env vars, we don't want them to be saved. """ - assert not account_info_file.exists() + b2_tool.should_succeed(['clear-account']) + assert B2_APPLICATION_KEY_ID_ENV_VAR not in os.environ assert B2_APPLICATION_KEY_ENV_VAR not in os.environ @@ -174,7 +174,12 @@ def test_command_with_env_vars_not_saving_credentials( } ) - assert not account_info_file.exists() + assert account_info_file.exists() + account_info = SqliteAccountInfo() + with pytest.raises(MissingAccountData): + account_info.get_application_key() + with pytest.raises(MissingAccountData): + account_info.get_application_key_id() @pytest.fixture From e5215b594b8d13378330800cc2cf347ee9c7ab74 Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 25 Mar 2024 14:23:55 +0300 Subject: [PATCH 08/16] Update changelog.d/+not-saving-auth-data-when-env-vars.fixed.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Maciej Urbański <122983254+mjurbanski-reef@users.noreply.github.com> --- changelog.d/+not-saving-auth-data-when-env-vars.fixed.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/+not-saving-auth-data-when-env-vars.fixed.md b/changelog.d/+not-saving-auth-data-when-env-vars.fixed.md index 7c422c46f..9795fca3c 100644 --- a/changelog.d/+not-saving-auth-data-when-env-vars.fixed.md +++ b/changelog.d/+not-saving-auth-data-when-env-vars.fixed.md @@ -1 +1 @@ -Fix persisting credentials on disk even if environment variables were used for authentication. +Don't persist credentials provided in the Enviornment variables in any command other than `authorize-account`. From 86d7574b78aa76295fc5c492f111451002ac27fc Mon Sep 17 00:00:00 2001 From: Aleksandr Goncharov Date: Mon, 25 Mar 2024 15:03:44 +0300 Subject: [PATCH 09/16] Use in-memory account info since v4 only --- b2/_internal/b2v3/registry.py | 10 +++++++ b2/_internal/console_tool.py | 16 ++++++----- test/integration/test_b2_command_line.py | 34 ++++++++++++++++++++++++ test/unit/test_console_tool.py | 8 +----- 4 files changed, 55 insertions(+), 13 deletions(-) diff --git a/b2/_internal/b2v3/registry.py b/b2/_internal/b2v3/registry.py index 0a4466d68..3017f2370 100644 --- a/b2/_internal/b2v3/registry.py +++ b/b2/_internal/b2v3/registry.py @@ -10,12 +10,22 @@ # ruff: noqa: F405 from b2._internal._b2v4.registry import * # noqa +from b2._internal._cli.b2api import _get_b2api_for_profile from b2._internal.arg_parser import enable_camel_case_arguments from .rm import Rm enable_camel_case_arguments() +class ConsoleTool(ConsoleTool): + # same as original console tool, but does not use InMemoryAccountInfo and InMemoryCache + # when auth env vars are used + + @classmethod + def _initialize_b2_api(cls, args: argparse.Namespace, kwargs: dict) -> B2Api: + return _get_b2api_for_profile(profile=args.profile, **kwargs) + + class Ls(B2URIBucketNFolderNameArgMixin, BaseLs): """ {BASELS} diff --git a/b2/_internal/console_tool.py b/b2/_internal/console_tool.py index 5449728c0..3ecf5d1c7 100644 --- a/b2/_internal/console_tool.py +++ b/b2/_internal/console_tool.py @@ -4224,12 +4224,7 @@ def run_command(self, argv): with suppress(AttributeError): kwargs['max_download_streams_per_file'] = args.max_download_streams_per_file - if all(get_keyid_and_key_from_env_vars() - ) and args.command_class not in (AuthorizeAccount, ClearAccount): - # when user specifies keys via env variables, we switch to in-memory account info - self.api = _get_inmemory_b2api(**kwargs) - else: - self.api = _get_b2api_for_profile(profile=args.profile, **kwargs) + self.api = self._initialize_b2_api(args=args, kwargs=kwargs) b2_command = B2(self) command_class = b2_command.run(args) @@ -4264,6 +4259,15 @@ def run_command(self, argv): logger.exception('ConsoleTool unexpected exception') raise + @classmethod + def _initialize_b2_api(cls, args: argparse.Namespace, kwargs: dict) -> B2Api: + if all(get_keyid_and_key_from_env_vars() + ) and args.command_class not in (AuthorizeAccount, ClearAccount): + # when user specifies keys via env variables, we switch to in-memory account info + return _get_inmemory_b2api(**kwargs) + + return _get_b2api_for_profile(profile=args.profile, **kwargs) + def authorize_from_env(self, command_class): if not command_class.REQUIRES_AUTH: return 0 diff --git a/test/integration/test_b2_command_line.py b/test/integration/test_b2_command_line.py index 1b043b890..cb2a19370 100755 --- a/test/integration/test_b2_command_line.py +++ b/test/integration/test_b2_command_line.py @@ -148,6 +148,40 @@ def test_clear_account_with_env_vars( account_info.get_application_key_id() +@pytest.mark.apiver(to_ver=3) +def test_command_with_env_vars_saving_credentials( + b2_tool, + application_key, + application_key_id, + account_info_file, + bucket_name, + b2_uri_args, +): + """ + When calling any command other then `authorize-account` and passing credentials + via env vars, we don't want them to be saved. + """ + + b2_tool.should_succeed(['clear-account']) + + assert B2_APPLICATION_KEY_ID_ENV_VAR not in os.environ + assert B2_APPLICATION_KEY_ENV_VAR not in os.environ + + b2_tool.should_succeed( + ['ls', '--long', *b2_uri_args(bucket_name)], + additional_env={ + B2_APPLICATION_KEY_ID_ENV_VAR: application_key_id, + B2_APPLICATION_KEY_ENV_VAR: application_key, + } + ) + + assert account_info_file.exists() + account_info = SqliteAccountInfo() + assert account_info.get_application_key() == application_key + assert account_info.get_application_key_id() == application_key_id + + +@pytest.mark.apiver(from_ver=4) def test_command_with_env_vars_not_saving_credentials( b2_tool, application_key, diff --git a/test/unit/test_console_tool.py b/test/unit/test_console_tool.py index 456a50bb6..66526171e 100644 --- a/test/unit/test_console_tool.py +++ b/test/unit/test_console_tool.py @@ -74,9 +74,7 @@ def _get_b2api(**kwargs) -> B2Api: **kwargs, ) - self.mp = pytest.MonkeyPatch() - self.mp.setattr('b2._internal.console_tool._get_b2api_for_profile', _get_b2api) - self.mp.setattr('b2._internal.console_tool._get_inmemory_b2api', _get_b2api) + self.console_tool_class._initialize_b2_api = lambda cls, args, kwargs: _get_b2api(**kwargs) self.b2_api = _get_b2api() self.raw_api = self.b2_api.session.raw_api @@ -89,10 +87,6 @@ def _get_b2api(**kwargs) -> B2Api: ]: os.environ.pop(env_var_name, None) - def tearDown(self) -> None: - super().tearDown() - self.mp.undo() - def _get_stdouterr(self): stdout = StringIO() stderr = StringIO() From 41cb0f7e4c093030475fbd1f7f8b2c8abd8e97c6 Mon Sep 17 00:00:00 2001 From: Aleksandr Goncharov Date: Mon, 25 Mar 2024 17:13:35 +0300 Subject: [PATCH 10/16] Don't use in-memory account info if it would match account info on disk --- b2/_internal/console_tool.py | 32 ++++++++++++------- test/integration/test_b2_command_line.py | 39 ++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 11 deletions(-) diff --git a/b2/_internal/console_tool.py b/b2/_internal/console_tool.py index 3ecf5d1c7..f01145a8b 100644 --- a/b2/_internal/console_tool.py +++ b/b2/_internal/console_tool.py @@ -4236,9 +4236,10 @@ def run_command(self, argv): logger.info('starting command [%s] with arguments: %s', command, argv) try: - auth_ret = self.authorize_from_env(command_class) - if auth_ret: - return auth_ret + if command_class.REQUIRES_AUTH: + auth_ret = self.authorize_from_env() + if auth_ret: + return auth_ret return command.run(args) except MissingAccountData as e: logger.exception('ConsoleTool missing account data error') @@ -4261,16 +4262,25 @@ def run_command(self, argv): @classmethod def _initialize_b2_api(cls, args: argparse.Namespace, kwargs: dict) -> B2Api: - if all(get_keyid_and_key_from_env_vars() - ) and args.command_class not in (AuthorizeAccount, ClearAccount): - # when user specifies keys via env variables, we switch to in-memory account info - return _get_inmemory_b2api(**kwargs) - return _get_b2api_for_profile(profile=args.profile, **kwargs) + # here we initialize regular b2 api on disk, and check whether it matches + # the keys from env vars; if they indeed match then there's no need to + # initialize in-memory account info cause it's already stored on disk + b2_api = _get_b2api_for_profile(profile=args.profile, **kwargs) - def authorize_from_env(self, command_class): - if not command_class.REQUIRES_AUTH: - return 0 + key_id, key = get_keyid_and_key_from_env_vars() + if key_id and key: + realm = os.environ.get(B2_ENVIRONMENT_ENV_VAR) or 'production' + if ( + not b2_api.account_info.is_same_key(key_id, realm) and + args.command_class not in (AuthorizeAccount, ClearAccount) + ): + # when user specifies keys via env variables, we switch to in-memory account info + return _get_inmemory_b2api(**kwargs) + + return b2_api + + def authorize_from_env(self) -> int: key_id, key = get_keyid_and_key_from_env_vars() diff --git a/test/integration/test_b2_command_line.py b/test/integration/test_b2_command_line.py index cb2a19370..2a7b72dc5 100755 --- a/test/integration/test_b2_command_line.py +++ b/test/integration/test_b2_command_line.py @@ -216,6 +216,45 @@ def test_command_with_env_vars_not_saving_credentials( account_info.get_application_key_id() +@pytest.mark.apiver(from_ver=4) +def test_command_with_env_vars_reusing_existing_account_info( + b2_tool, + application_key, + application_key_id, + account_info_file, + bucket_name, + b2_uri_args, +): + """ + When calling any command with credentials passed via env vars, and the account + info file already contains the same credentials, we want to use filesystem for + storing cache, not the in-memory cache. + """ + + assert B2_APPLICATION_KEY_ID_ENV_VAR not in os.environ + assert B2_APPLICATION_KEY_ENV_VAR not in os.environ + + assert account_info_file.exists() + account_info = SqliteAccountInfo() + assert account_info.get_application_key() == application_key + assert account_info.get_application_key_id() == application_key_id + + account_info.remove_bucket_name(bucket_name) + assert account_info.get_bucket_id_or_none_from_bucket_name(bucket_name) is None + + b2_tool.should_succeed( + ['ls', '--long', *b2_uri_args(bucket_name)], + additional_env={ + B2_APPLICATION_KEY_ID_ENV_VAR: application_key_id, + B2_APPLICATION_KEY_ENV_VAR: application_key, + } + ) + + assert account_info_file.exists() + account_info = SqliteAccountInfo() + assert account_info.get_bucket_id_or_none_from_bucket_name(bucket_name) is not None + + @pytest.fixture def uploaded_sample_file(b2_tool, bucket_name, sample_filepath): return b2_tool.should_succeed_json( From 98ea664273bd8e96590a50bd366c62142daca2fe Mon Sep 17 00:00:00 2001 From: Aleksandr Goncharov Date: Tue, 26 Mar 2024 14:54:33 +0300 Subject: [PATCH 11/16] Preserve storing account info in v3 --- b2/_internal/b2v3/registry.py | 18 ++++++++++++++++++ b2/_internal/console_tool.py | 4 +++- test/integration/conftest.py | 13 +++++++++---- test/integration/test_b2_command_line.py | 14 ++++++++++++-- 4 files changed, 42 insertions(+), 7 deletions(-) diff --git a/b2/_internal/b2v3/registry.py b/b2/_internal/b2v3/registry.py index 3017f2370..c099bfba2 100644 --- a/b2/_internal/b2v3/registry.py +++ b/b2/_internal/b2v3/registry.py @@ -26,6 +26,24 @@ def _initialize_b2_api(cls, args: argparse.Namespace, kwargs: dict) -> B2Api: return _get_b2api_for_profile(profile=args.profile, **kwargs) +def main() -> None: + # this is a copy of v4 `main()` but with custom console tool class + + ct = ConsoleTool(stdout=sys.stdout, stderr=sys.stderr) + exit_status = ct.run_command(sys.argv) + logger.info('\\\\ %s %s %s //', SEPARATOR, ('exit=%s' % exit_status).center(8), SEPARATOR) + + # I haven't tracked down the root cause yet, but in Python 2.7, the futures + # packages is hanging on exit sometimes, waiting for a thread to finish. + # This happens when using sync to upload files. + sys.stdout.flush() + sys.stderr.flush() + + logging.shutdown() + + os._exit(exit_status) + + class Ls(B2URIBucketNFolderNameArgMixin, BaseLs): """ {BASELS} diff --git a/b2/_internal/console_tool.py b/b2/_internal/console_tool.py index f01145a8b..48b0edb12 100644 --- a/b2/_internal/console_tool.py +++ b/b2/_internal/console_tool.py @@ -4265,7 +4265,9 @@ def _initialize_b2_api(cls, args: argparse.Namespace, kwargs: dict) -> B2Api: # here we initialize regular b2 api on disk, and check whether it matches # the keys from env vars; if they indeed match then there's no need to - # initialize in-memory account info cause it's already stored on disk + # initialize in-memory account info cause it's already stored on disk; + # beware that this has side effect of creating an empty account info file + # if it didn't exist before b2_api = _get_b2api_for_profile(profile=args.profile, **kwargs) key_id, key = get_keyid_and_key_from_env_vars() diff --git a/test/integration/conftest.py b/test/integration/conftest.py index e4d070996..4ad692871 100755 --- a/test/integration/conftest.py +++ b/test/integration/conftest.py @@ -100,13 +100,18 @@ def get_raw_cli_int_version(config) -> int | None: return None +def get_cli_int_version(config) -> int: + return get_raw_cli_int_version(config) or get_int_version(LATEST_STABLE_VERSION) + + @pytest.fixture(scope='session') -def apiver(request): - return f"v{get_cli_int_version(request.config)}" +def apiver_int(request): + return get_cli_int_version(request.config) -def get_cli_int_version(config) -> int: - return get_raw_cli_int_version(config) or get_int_version(LATEST_STABLE_VERSION) +@pytest.fixture(scope='session') +def apiver(apiver_int): + return f"v{apiver_int}" @pytest.hookimpl diff --git a/test/integration/test_b2_command_line.py b/test/integration/test_b2_command_line.py index 2a7b72dc5..0872c15f2 100755 --- a/test/integration/test_b2_command_line.py +++ b/test/integration/test_b2_command_line.py @@ -595,7 +595,7 @@ def test_rapid_bucket_operations(b2_tool): b2_tool.should_succeed(['delete-bucket', new_bucket_name]) -def test_account(b2_tool, cli_version): +def test_account(b2_tool, cli_version, apiver_int): with b2_tool.env_var_test_context: b2_tool.should_succeed(['clear-account']) bad_application_key = random_hex(len(b2_tool.application_key)) @@ -637,7 +637,17 @@ def test_account(b2_tool, cli_version): ['create-bucket', bucket_name, 'allPrivate', *b2_tool.get_bucket_info_args()] ) b2_tool.should_succeed(['delete-bucket', bucket_name]) - assert not os.path.exists(new_creds), 'sqlite file was created' + + assert os.path.exists(new_creds), 'sqlite file was not created' + account_info = SqliteAccountInfo(new_creds) + if apiver_int >= 4: + with pytest.raises(MissingAccountData): + account_info.get_application_key_id() + with pytest.raises(MissingAccountData): + account_info.get_application_key() + else: + assert account_info.get_application_key_id() == os.environ['B2_TEST_APPLICATION_KEY_ID'] + assert account_info.get_application_key() == os.environ['B2_TEST_APPLICATION_KEY'] os.environ.pop('B2_APPLICATION_KEY') os.environ.pop('B2_APPLICATION_KEY_ID') From d75670a8d71f90b88302e3ea07f2c5372f8e61d4 Mon Sep 17 00:00:00 2001 From: Aleksandr Goncharov Date: Tue, 26 Mar 2024 15:40:04 +0300 Subject: [PATCH 12/16] Use random temp folder for EnvVarTestContext --- test/integration/helpers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/helpers.py b/test/integration/helpers.py index b1feaaee3..c8752e3aa 100755 --- a/test/integration/helpers.py +++ b/test/integration/helpers.py @@ -29,7 +29,7 @@ from datetime import datetime, timedelta from os import environ, linesep, path from pathlib import Path -from tempfile import gettempdir, mkdtemp, mktemp +from tempfile import mkdtemp, mktemp import backoff from b2sdk.v2 import ( @@ -349,7 +349,7 @@ def __init__(self, account_info_file_name: str): def __enter__(self): src = self.account_info_file_name - dst = path.join(gettempdir(), 'b2_account_info') + dst = path.join(mkdtemp(), 'b2_account_info') shutil.copyfile(src, dst) shutil.move(src, src + self.suffix) environ[self.ENV_VAR] = dst From 8657852263a86e19738d166ba53b51ee6a5fc7eb Mon Sep 17 00:00:00 2001 From: Aleksandr Goncharov Date: Tue, 26 Mar 2024 16:39:17 +0300 Subject: [PATCH 13/16] WIP remove EnvVarTestContext --- test/integration/test_b2_command_line.py | 25 ++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/test/integration/test_b2_command_line.py b/test/integration/test_b2_command_line.py index 0872c15f2..c0f478f6c 100755 --- a/test/integration/test_b2_command_line.py +++ b/test/integration/test_b2_command_line.py @@ -22,6 +22,7 @@ import sys import time from pathlib import Path +from tempfile import mkdtemp import pytest from b2sdk.account_info.exception import MissingAccountData @@ -595,8 +596,12 @@ def test_rapid_bucket_operations(b2_tool): b2_tool.should_succeed(['delete-bucket', new_bucket_name]) -def test_account(b2_tool, cli_version, apiver_int): - with b2_tool.env_var_test_context: +def test_account(b2_tool, cli_version, apiver_int, monkeypatch): + + with monkeypatch.context() as mp: + account_info_file_path = os.path.join(mkdtemp(), 'b2_account_info') + mp.setenv(B2_ACCOUNT_INFO_ENV_VAR, account_info_file_path) + b2_tool.should_succeed(['clear-account']) bad_application_key = random_hex(len(b2_tool.application_key)) b2_tool.should_fail( @@ -613,10 +618,11 @@ def test_account(b2_tool, cli_version, apiver_int): ) # Testing (B2_APPLICATION_KEY, B2_APPLICATION_KEY_ID) for commands other than authorize-account - with b2_tool.env_var_test_context as new_creds: - os.remove(new_creds) + with monkeypatch.context() as mp: + account_info_file_path = os.path.join(mkdtemp(), 'b2_account_info') + mp.setenv(B2_ACCOUNT_INFO_ENV_VAR, account_info_file_path) - # first, let's make sure "create-bucket" doesn't work without auth data - i.e. that the sqlite file hs been + # first, let's make sure "create-bucket" doesn't work without auth data - i.e. that the sqlite file has been # successfully removed bucket_name = b2_tool.generate_bucket_name() b2_tool.should_fail( @@ -625,7 +631,10 @@ def test_account(b2_tool, cli_version, apiver_int): fr'Use: {cli_version}(\.(exe|EXE))? authorize-account or provide auth data with \'B2_APPLICATION_KEY_ID\' and ' r'\'B2_APPLICATION_KEY\' environment variables' ) - os.remove(new_creds) + + with monkeypatch.context() as mp: + account_info_file_path = os.path.join(mkdtemp(), 'b2_account_info') + mp.setenv(B2_ACCOUNT_INFO_ENV_VAR, account_info_file_path) # then, let's see that auth data from env vars works os.environ['B2_APPLICATION_KEY'] = os.environ['B2_TEST_APPLICATION_KEY'] @@ -638,8 +647,8 @@ def test_account(b2_tool, cli_version, apiver_int): ) b2_tool.should_succeed(['delete-bucket', bucket_name]) - assert os.path.exists(new_creds), 'sqlite file was not created' - account_info = SqliteAccountInfo(new_creds) + assert os.path.exists(account_info_file_path), 'sqlite file was not created' + account_info = SqliteAccountInfo(account_info_file_path) if apiver_int >= 4: with pytest.raises(MissingAccountData): account_info.get_application_key_id() From 78a1fdc87d075a5d12d8f4cc176bdc0925586ed4 Mon Sep 17 00:00:00 2001 From: Aleksandr Goncharov Date: Tue, 26 Mar 2024 17:18:06 +0300 Subject: [PATCH 14/16] Remove EnvVarTestContext --- test/integration/helpers.py | 31 +------------------------------ 1 file changed, 1 insertion(+), 30 deletions(-) diff --git a/test/integration/helpers.py b/test/integration/helpers.py index c8752e3aa..5e9cdb6e2 100755 --- a/test/integration/helpers.py +++ b/test/integration/helpers.py @@ -27,7 +27,7 @@ import warnings from dataclasses import dataclass from datetime import datetime, timedelta -from os import environ, linesep, path +from os import environ, linesep from pathlib import Path from tempfile import mkdtemp, mktemp @@ -47,7 +47,6 @@ InMemoryCache, LegalHold, RetentionMode, - SqliteAccountInfo, fix_windows_path_limit, ) from b2sdk.v2.exception import ( @@ -336,33 +335,6 @@ def read_from(self, f): self.string = str(e) -class EnvVarTestContext: - """ - Establish config for environment variable test. - Copy the B2 credential file and rename the existing copy - """ - ENV_VAR = 'B2_ACCOUNT_INFO' - - def __init__(self, account_info_file_name: str): - self.account_info_file_name = account_info_file_name - self.suffix = ''.join(RNG.choice('abcdefghijklmnopqrstuvwxyz0123456789') for _ in range(7)) - - def __enter__(self): - src = self.account_info_file_name - dst = path.join(mkdtemp(), 'b2_account_info') - shutil.copyfile(src, dst) - shutil.move(src, src + self.suffix) - environ[self.ENV_VAR] = dst - return dst - - def __exit__(self, exc_type, exc_val, exc_tb): - os.remove(environ.get(self.ENV_VAR)) - fname = self.account_info_file_name - shutil.move(fname + self.suffix, fname) - if environ.get(self.ENV_VAR) is not None: - del environ[self.ENV_VAR] - - def should_equal(expected, actual): print(' expected:') print_json_indented(expected) @@ -406,7 +378,6 @@ def __init__( self.realm = realm self.bucket_name_prefix = bucket_name_prefix self.env_file_cmd_placeholder = env_file_cmd_placeholder - self.env_var_test_context = EnvVarTestContext(SqliteAccountInfo().filename) self.api_wrapper = api_wrapper self.b2_uri_args = b2_uri_args From 3f658c3fc86cb91ec715517641dcdfcba3a00ac6 Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 27 Mar 2024 11:49:16 +0300 Subject: [PATCH 15/16] Update changelog.d/+not-saving-auth-data-when-env-vars.fixed.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Maciej Urbański <122983254+mjurbanski-reef@users.noreply.github.com> --- changelog.d/+not-saving-auth-data-when-env-vars.fixed.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/+not-saving-auth-data-when-env-vars.fixed.md b/changelog.d/+not-saving-auth-data-when-env-vars.fixed.md index 9795fca3c..a088b8c17 100644 --- a/changelog.d/+not-saving-auth-data-when-env-vars.fixed.md +++ b/changelog.d/+not-saving-auth-data-when-env-vars.fixed.md @@ -1 +1 @@ -Don't persist credentials provided in the Enviornment variables in any command other than `authorize-account`. +Don't persist credentials provided in the Enviornment variables in any command other than `authorize-account` when using `b2v4`. From e7e6a4323a7964fa0a3b255f4b0d2509fbd9ee74 Mon Sep 17 00:00:00 2001 From: Aleksandr Goncharov Date: Wed, 27 Mar 2024 12:34:34 +0300 Subject: [PATCH 16/16] Don't create unnecessary account info file --- b2/_internal/_cli/b2api.py | 13 ++++++++++- b2/_internal/console_tool.py | 29 +++++++++++++----------- test/integration/test_b2_command_line.py | 11 ++++----- 3 files changed, 33 insertions(+), 20 deletions(-) diff --git a/b2/_internal/_cli/b2api.py b/b2/_internal/_cli/b2api.py index bf2bbffd8..10c0517a2 100644 --- a/b2/_internal/_cli/b2api.py +++ b/b2/_internal/_cli/b2api.py @@ -19,11 +19,22 @@ InMemoryCache, SqliteAccountInfo, ) +from b2sdk.v2.exception import MissingAccountData from b2._internal._cli.const import B2_USER_AGENT_APPEND_ENV_VAR -def _get_b2api_for_profile(profile: Optional[str] = None, **kwargs) -> B2Api: +def _get_b2api_for_profile( + profile: Optional[str] = None, + raise_if_does_not_exist: bool = False, + **kwargs, +) -> B2Api: + + if raise_if_does_not_exist: + account_info_file = SqliteAccountInfo._get_user_account_info_path(profile=profile) + if not os.path.exists(account_info_file): + raise MissingAccountData(account_info_file) + account_info = SqliteAccountInfo(profile=profile) b2api = B2Api( api_config=_get_b2httpapiconfig(), diff --git a/b2/_internal/console_tool.py b/b2/_internal/console_tool.py index 48b0edb12..1047167d2 100644 --- a/b2/_internal/console_tool.py +++ b/b2/_internal/console_tool.py @@ -4262,25 +4262,28 @@ def run_command(self, argv): @classmethod def _initialize_b2_api(cls, args: argparse.Namespace, kwargs: dict) -> B2Api: - - # here we initialize regular b2 api on disk, and check whether it matches - # the keys from env vars; if they indeed match then there's no need to - # initialize in-memory account info cause it's already stored on disk; - # beware that this has side effect of creating an empty account info file - # if it didn't exist before - b2_api = _get_b2api_for_profile(profile=args.profile, **kwargs) - + b2_api = None key_id, key = get_keyid_and_key_from_env_vars() if key_id and key: - realm = os.environ.get(B2_ENVIRONMENT_ENV_VAR) or 'production' - if ( - not b2_api.account_info.is_same_key(key_id, realm) and - args.command_class not in (AuthorizeAccount, ClearAccount) + try: + # here we initialize regular b2 api on disk and check whether it matches + # the keys from env vars; if they indeed match then there's no need to + # initialize in-memory account info cause it's already stored on disk + b2_api = _get_b2api_for_profile( + profile=args.profile, raise_if_does_not_exist=True, **kwargs + ) + realm = os.environ.get(B2_ENVIRONMENT_ENV_VAR) or 'production' + is_same_key_on_disk = b2_api.account_info.is_same_key(key_id, realm) + except MissingAccountData: + is_same_key_on_disk = False + + if not is_same_key_on_disk and args.command_class not in ( + AuthorizeAccount, ClearAccount ): # when user specifies keys via env variables, we switch to in-memory account info return _get_inmemory_b2api(**kwargs) - return b2_api + return b2_api or _get_b2api_for_profile(profile=args.profile, **kwargs) def authorize_from_env(self) -> int: diff --git a/test/integration/test_b2_command_line.py b/test/integration/test_b2_command_line.py index c0f478f6c..40f5137d2 100755 --- a/test/integration/test_b2_command_line.py +++ b/test/integration/test_b2_command_line.py @@ -647,14 +647,13 @@ def test_account(b2_tool, cli_version, apiver_int, monkeypatch): ) b2_tool.should_succeed(['delete-bucket', bucket_name]) - assert os.path.exists(account_info_file_path), 'sqlite file was not created' - account_info = SqliteAccountInfo(account_info_file_path) if apiver_int >= 4: - with pytest.raises(MissingAccountData): - account_info.get_application_key_id() - with pytest.raises(MissingAccountData): - account_info.get_application_key() + assert not os.path.exists( + account_info_file_path + ), 'sqlite file was created while it shouldn\'t' else: + assert os.path.exists(account_info_file_path), 'sqlite file was not created' + account_info = SqliteAccountInfo(account_info_file_path) assert account_info.get_application_key_id() == os.environ['B2_TEST_APPLICATION_KEY_ID'] assert account_info.get_application_key() == os.environ['B2_TEST_APPLICATION_KEY']