Skip to content

Commit b08afd3

Browse files
BartoszBlizniakBartoszBlizniak
committed
clean up tests and update function in keyring for cleaner code
1 parent eb5e56a commit b08afd3

5 files changed

Lines changed: 101 additions & 113 deletions

File tree

‎cloudsmith_cli/cli/tests/commands/test_auth.py‎

Lines changed: 20 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
"""Tests for the auth command."""
22

3-
import os
43
from unittest.mock import MagicMock, patch
54

65
import pytest
@@ -47,69 +46,58 @@ def mock_auth_server():
4746
class TestAuthenticateCommand:
4847
"""Tests for the authenticate command."""
4948

50-
def test_no_keyring_flag_passed_to_webserver(
49+
def test_auth_command_invokes_webserver(
5150
self,
5251
runner,
5352
mock_saml_session,
5453
mock_get_idp_url,
5554
mock_webbrowser,
5655
mock_auth_server,
5756
):
58-
"""Verify --no-keyring flag is passed to AuthenticationWebServer."""
57+
"""Verify auth command creates AuthenticationWebServer."""
5958
runner.invoke(
6059
authenticate,
61-
["--owner", "testorg", "--no-keyring"],
60+
["--owner", "testorg"],
6261
catch_exceptions=False,
6362
)
6463

65-
# Verify AuthenticationWebServer was called with no_keyring=True
64+
# Verify AuthenticationWebServer was called
6665
mock_auth_server.assert_called_once()
67-
call_kwargs = mock_auth_server.call_args.kwargs
68-
assert call_kwargs.get("no_keyring") is True
6966

70-
def test_no_keyring_flag_defaults_to_false(
67+
def test_auth_command_opens_browser(
7168
self,
7269
runner,
7370
mock_saml_session,
7471
mock_get_idp_url,
7572
mock_webbrowser,
7673
mock_auth_server,
7774
):
78-
"""Verify no_keyring defaults to False when flag not provided."""
75+
"""Verify auth command opens browser with IDP URL."""
7976
runner.invoke(
8077
authenticate,
8178
["--owner", "testorg"],
8279
catch_exceptions=False,
8380
)
8481

85-
# Verify AuthenticationWebServer was called with no_keyring=False
86-
mock_auth_server.assert_called_once()
87-
call_kwargs = mock_auth_server.call_args.kwargs
88-
assert call_kwargs.get("no_keyring") is False
82+
# Verify browser was opened
83+
mock_webbrowser.open.assert_called_once_with("https://idp.example.com/saml")
8984

90-
def test_no_keyring_flag_sets_env_var(
85+
def test_auth_command_passes_owner_to_webserver(
9186
self,
9287
runner,
9388
mock_saml_session,
9489
mock_get_idp_url,
9590
mock_webbrowser,
9691
mock_auth_server,
9792
):
98-
"""Verify --no-keyring flag sets CLOUDSMITH_NO_KEYRING env var."""
99-
# Ensure env var is not set before test
100-
env_backup = os.environ.copy()
101-
os.environ.pop("CLOUDSMITH_NO_KEYRING", None)
102-
103-
try:
104-
runner.invoke(
105-
authenticate,
106-
["--owner", "testorg", "--no-keyring"],
107-
catch_exceptions=False,
108-
)
109-
110-
# Verify environment variable was set
111-
assert os.environ.get("CLOUDSMITH_NO_KEYRING") == "1"
112-
finally:
113-
# Restore original environment
114-
os.environ.clear()
115-
os.environ.update(env_backup)
93+
"""Verify owner is passed to AuthenticationWebServer."""
94+
runner.invoke(
95+
authenticate,
96+
["--owner", "testorg"],
97+
catch_exceptions=False,
98+
)
99+
100+
# Verify AuthenticationWebServer was called with owner
101+
mock_auth_server.assert_called_once()
102+
call_kwargs = mock_auth_server.call_args.kwargs
103+
assert call_kwargs.get("owner") == "testorg"

‎cloudsmith_cli/cli/tests/test_webserver.py‎

Lines changed: 55 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
"""Tests for the webserver module."""
22

3-
import os
43
from unittest.mock import MagicMock, PropertyMock, patch
54

65
import pytest
@@ -28,74 +27,65 @@ def mock_handler(self):
2827
return handler
2928

3029
def test_store_sso_tokens_called_when_keyring_enabled(self, mock_handler):
31-
"""Verify store_sso_tokens is called when keyring is enabled."""
32-
# Ensure env var is not set
33-
env = os.environ.copy()
34-
env.pop("CLOUDSMITH_NO_KEYRING", None)
30+
"""Verify store_sso_tokens is called and returns True when keyring is enabled."""
31+
with patch(
32+
"cloudsmith_cli.cli.webserver.store_sso_tokens", return_value=True
33+
) as mock_store:
34+
with patch.object(mock_handler, "_return_success_response"):
35+
with patch.object(
36+
AuthenticationWebRequestHandler,
37+
"query_data",
38+
new_callable=PropertyMock,
39+
) as mock_query:
40+
with patch.object(
41+
AuthenticationWebRequestHandler,
42+
"api_host",
43+
new_callable=PropertyMock,
44+
) as mock_host:
45+
mock_query.return_value = {
46+
"access_token": "test_access_token",
47+
"refresh_token": "test_refresh_token",
48+
}
49+
mock_host.return_value = "https://api.cloudsmith.io"
3550

36-
with patch.dict(os.environ, env, clear=True):
37-
with patch("cloudsmith_cli.cli.webserver.store_sso_tokens") as mock_store:
38-
with patch(
39-
"cloudsmith_cli.cli.webserver.should_use_keyring", return_value=True
40-
):
41-
with patch.object(mock_handler, "_return_success_response"):
51+
mock_handler.do_GET()
52+
53+
mock_store.assert_called_once_with(
54+
"https://api.cloudsmith.io",
55+
"test_access_token",
56+
"test_refresh_token",
57+
)
58+
59+
def test_message_shown_when_keyring_disabled(self, mock_handler):
60+
"""Verify message is shown when store_sso_tokens returns False."""
61+
with patch(
62+
"cloudsmith_cli.cli.webserver.store_sso_tokens", return_value=False
63+
) as mock_store:
64+
with patch("click.echo") as mock_echo:
65+
with patch.object(mock_handler, "_return_success_response"):
66+
with patch.object(
67+
AuthenticationWebRequestHandler,
68+
"query_data",
69+
new_callable=PropertyMock,
70+
) as mock_query:
4271
with patch.object(
4372
AuthenticationWebRequestHandler,
44-
"query_data",
73+
"api_host",
4574
new_callable=PropertyMock,
46-
) as mock_query:
47-
with patch.object(
48-
AuthenticationWebRequestHandler,
49-
"api_host",
50-
new_callable=PropertyMock,
51-
) as mock_host:
52-
mock_query.return_value = {
53-
"access_token": "test_access_token",
54-
"refresh_token": "test_refresh_token",
55-
}
56-
mock_host.return_value = "https://api.cloudsmith.io"
57-
58-
mock_handler.do_GET()
59-
60-
mock_store.assert_called_once_with(
61-
"https://api.cloudsmith.io",
62-
"test_access_token",
63-
"test_refresh_token",
64-
)
65-
66-
def test_store_sso_tokens_not_called_when_keyring_disabled(self, mock_handler):
67-
"""Verify store_sso_tokens is NOT called when CLOUDSMITH_NO_KEYRING=1."""
68-
with patch.dict(os.environ, {"CLOUDSMITH_NO_KEYRING": "1"}):
69-
with patch("cloudsmith_cli.cli.webserver.store_sso_tokens") as mock_store:
70-
with patch(
71-
"cloudsmith_cli.cli.webserver.should_use_keyring",
72-
return_value=False,
73-
):
74-
with patch("click.echo") as mock_echo:
75-
with patch.object(mock_handler, "_return_success_response"):
76-
with patch.object(
77-
AuthenticationWebRequestHandler,
78-
"query_data",
79-
new_callable=PropertyMock,
80-
) as mock_query:
81-
with patch.object(
82-
AuthenticationWebRequestHandler,
83-
"api_host",
84-
new_callable=PropertyMock,
85-
) as mock_host:
86-
mock_query.return_value = {
87-
"access_token": "test_access_token",
88-
"refresh_token": "test_refresh_token",
89-
}
90-
mock_host.return_value = "https://api.cloudsmith.io"
75+
) as mock_host:
76+
mock_query.return_value = {
77+
"access_token": "test_access_token",
78+
"refresh_token": "test_refresh_token",
79+
}
80+
mock_host.return_value = "https://api.cloudsmith.io"
9181

92-
mock_handler.do_GET()
82+
mock_handler.do_GET()
9383

94-
# store_sso_tokens should NOT be called
95-
mock_store.assert_not_called()
84+
# store_sso_tokens should be called (returns False)
85+
mock_store.assert_called_once()
9686

97-
# Message should be displayed to stderr
98-
mock_echo.assert_called_once_with(
99-
"SSO tokens not stored (CLOUDSMITH_NO_KEYRING is set)",
100-
err=True,
101-
)
87+
# Message should be displayed to stderr
88+
mock_echo.assert_called_once_with(
89+
"SSO tokens not stored (CLOUDSMITH_NO_KEYRING is set)",
90+
err=True,
91+
)

‎cloudsmith_cli/cli/webserver.py‎

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99

1010
from ..core.api.exceptions import ApiException
1111
from ..core.api.init import initialise_api
12-
from ..core.keyring import should_use_keyring, store_sso_tokens
12+
from ..core.keyring import store_sso_tokens
1313
from .saml import exchange_2fa_token
1414

1515

@@ -200,13 +200,7 @@ def do_GET(self):
200200

201201
try:
202202
if access_token:
203-
if should_use_keyring():
204-
store_sso_tokens(
205-
self.api_host,
206-
access_token,
207-
refresh_token,
208-
)
209-
else:
203+
if not store_sso_tokens(self.api_host, access_token, refresh_token):
210204
click.echo(
211205
"SSO tokens not stored (CLOUDSMITH_NO_KEYRING is set)",
212206
err=True,
@@ -226,13 +220,7 @@ def do_GET(self):
226220
access_token, refresh_token = exchange_2fa_token(
227221
self.api_host, two_factor_token, totp_token, session=self.session
228222
)
229-
if should_use_keyring():
230-
store_sso_tokens(
231-
self.api_host,
232-
access_token,
233-
refresh_token,
234-
)
235-
else:
223+
if not store_sso_tokens(self.api_host, access_token, refresh_token):
236224
click.echo(
237225
"SSO tokens not stored (CLOUDSMITH_NO_KEYRING is set)",
238226
err=True,

‎cloudsmith_cli/core/keyring.py‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,9 +92,15 @@ def get_refresh_token(api_host):
9292

9393

9494
def store_sso_tokens(api_host, access_token, refresh_token):
95+
"""Store SSO tokens in keyring if enabled."""
96+
if not should_use_keyring():
97+
return False
98+
9599
if access_token:
96100
store_access_token(api_host=api_host, access_token=access_token)
97101
update_refresh_attempted_at(api_host=api_host)
98102

99103
if refresh_token:
100104
store_refresh_token(api_host=api_host, refresh_token=refresh_token)
105+
106+
return True

‎cloudsmith_cli/core/tests/test_keyring.py‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,8 +184,14 @@ def test_get_refresh_token_when_error_raised(
184184
@freeze_time("2024-06-01 10:00:00")
185185
def test_store_sso_tokens(self, mock_get_user, mock_set_password):
186186
refresh_attempted_at = datetime.utcnow().isoformat()
187-
store_sso_tokens(self.api_host, "access_token", "refresh_token")
188187

188+
# Ensure keyring is enabled
189+
env = os.environ.copy()
190+
env.pop("CLOUDSMITH_NO_KEYRING", None)
191+
with patch.dict(os.environ, env, clear=True):
192+
result = store_sso_tokens(self.api_host, "access_token", "refresh_token")
193+
194+
assert result is True
189195
assert mock_set_password.call_count == 3
190196
mock_set_password.assert_any_call(
191197
"cloudsmith_cli-access_token-https://example.com",
@@ -203,6 +209,16 @@ def test_store_sso_tokens(self, mock_get_user, mock_set_password):
203209
"refresh_token",
204210
)
205211

212+
def test_store_sso_tokens_returns_false_when_keyring_disabled(
213+
self, mock_get_user, mock_set_password
214+
):
215+
"""Verify store_sso_tokens returns False and doesn't store when keyring disabled."""
216+
with patch.dict(os.environ, {"CLOUDSMITH_NO_KEYRING": "1"}):
217+
result = store_sso_tokens(self.api_host, "access_token", "refresh_token")
218+
219+
assert result is False
220+
mock_set_password.assert_not_called()
221+
206222

207223
class TestShouldUseKeyring:
208224
"""Tests for the should_use_keyring function."""

0 commit comments

Comments
 (0)