Skip to content

Commit 54fa694

Browse files
committed
fix: Incorrect credentials are masked by traceback
* find_session_uri masks a proper error message when someone passes incorrect credentials (either by vars or user/pass). * response body seems to be a JSON object and not a redfish root object and we are not generating a useful error, instead the user gets a traceback that masks the real cause. before (see #517) after -=>>PYTHONPATH="./src" python3 src/badfish/main.py -H mgmt-d23-h31-000-r650.example.com -u root -p awrongpassword --power-state - WARNING - Passing secrets via command line arguments can be unsafe. Consider using environment variables (BADFISH_USERNAME, BADFISH_PASSWORD). - ERROR - Failed to authenticate. Verify your credentials for mgmt-d23-h31-000-r650.example.com * Also remove asyncio import from tests/test_main_coverage.py as it wasn't being utilized. fixes: #517
1 parent 87c27fd commit 54fa694

File tree

2 files changed

+87
-7
lines changed

2 files changed

+87
-7
lines changed

src/badfish/main.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -382,9 +382,19 @@ async def find_session_uri(self):
382382
if not response:
383383
raise BadfishException(f"Failed to communicate with {self.host}")
384384

385+
if response.status == 401:
386+
raise BadfishException(
387+
f"Failed to authenticate. Verify your credentials for {self.host}"
388+
)
389+
385390
raw = await response.text("utf-8", "ignore")
386391
data = json.loads(raw.strip())
387-
redfish_version = int(data["RedfishVersion"].replace(".", ""))
392+
try:
393+
redfish_version = int(data["RedfishVersion"].replace(".", ""))
394+
except KeyError:
395+
raise BadfishException(
396+
"Was unable to get Redfish Version. Please verify credentials/host."
397+
)
388398
session_uri = None
389399
if redfish_version >= 160:
390400
session_uri = "/redfish/v1/SessionService/Sessions"

tests/test_main_coverage.py

Lines changed: 76 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,91 @@
11
import pytest
2-
from unittest.mock import patch, MagicMock
3-
import asyncio
2+
from unittest.mock import patch, MagicMock, AsyncMock
43
import logging
4+
from collections import defaultdict
5+
6+
# CORRECT IMPORT: Must match the application's runtime context (PYTHONPATH=src)
7+
# Using 'src.badfish' here would cause isinstance checks to fail.
8+
from badfish.main import execute_badfish
9+
from badfish.helpers.exceptions import BadfishException
10+
11+
12+
@pytest.fixture
13+
def mock_args():
14+
"""Returns a dictionary that returns None for missing keys."""
15+
return defaultdict(lambda: None)
516

6-
from src.badfish.main import execute_badfish
717

818
@pytest.mark.asyncio
919
async def test_missing_credentials():
1020
host = "test_host"
11-
args = {} # No 'u' or 'p' in args
21+
args = {}
1222
logger = MagicMock(spec=logging.Logger)
1323
format_handler = None
1424

15-
with patch('os.environ.get', side_effect=lambda k: None): # Mock env vars as missing
25+
with patch('os.environ.get', side_effect=lambda k: None):
1626
result = await execute_badfish(host, args, logger, format_handler)
1727

1828
assert result == (host, False)
1929
logger.error.assert_called_once_with(
20-
"Missing credentials. Please provide credentials via CLI arguments or environment variables."
30+
"Missing credentials. Please provide credentials via CLI arguments "
31+
"or environment variables."
2132
)
33+
34+
35+
@pytest.mark.asyncio
36+
async def test_init_401_unauthorized(mock_args):
37+
"""Test that a 401 response raises a clean BadfishException."""
38+
host = "test_host"
39+
mock_args.update({"u": "user", "p": "pass", "retries": 1})
40+
logger = MagicMock(spec=logging.Logger)
41+
42+
# Mock response for HTTPClient.get_request
43+
mock_response = MagicMock()
44+
mock_response.status = 401
45+
mock_response.text = AsyncMock(return_value='{"error": "Unauthorized"}')
46+
47+
# Patch 'badfish' (not src.badfish) to match the import above
48+
with patch("badfish.main.HTTPClient.get_request",
49+
new_callable=AsyncMock) as mock_get:
50+
mock_get.return_value = mock_response
51+
52+
result = await execute_badfish(host, mock_args, logger, None)
53+
54+
assert result == (host, False)
55+
56+
# Verify logger was called with a BadfishException containing expected msg
57+
args, _ = logger.error.call_args
58+
exception_obj = args[0]
59+
60+
assert isinstance(exception_obj, BadfishException)
61+
assert str(exception_obj) == f"Failed to authenticate. Verify your credentials for {host}"
62+
63+
64+
@pytest.mark.asyncio
65+
async def test_init_key_error_missing_version(mock_args):
66+
"""Test that missing RedfishVersion key raises a clean BadfishException."""
67+
host = "test_host"
68+
mock_args.update({"u": "user", "p": "pass", "retries": 1})
69+
logger = MagicMock(spec=logging.Logger)
70+
71+
# Mock response for HTTPClient.get_request (Success 200 but bad payload)
72+
mock_response = MagicMock()
73+
mock_response.status = 200
74+
# Payload missing "RedfishVersion"
75+
mock_response.text = AsyncMock(return_value='{"OtherKey": "Value"}')
76+
77+
# Patch 'badfish' (not src.badfish) to match the import above
78+
with patch("badfish.main.HTTPClient.get_request",
79+
new_callable=AsyncMock) as mock_get:
80+
mock_get.return_value = mock_response
81+
82+
result = await execute_badfish(host, mock_args, logger, None)
83+
84+
assert result == (host, False)
85+
86+
# Verify logger was called with a BadfishException containing expected msg
87+
args, _ = logger.error.call_args
88+
exception_obj = args[0]
89+
90+
assert isinstance(exception_obj, BadfishException)
91+
assert str(exception_obj) == "Was unable to get Redfish Version. Please verify credentials/host."

0 commit comments

Comments
 (0)