From b0618eb61763763902cf526a7baed995cc9c134e Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Wed, 1 Jul 2026 16:16:35 -0700 Subject: [PATCH 1/3] fix: show actionable error when --password-file or --token-file path is missing Previously, a non-existent path caused a bare FileNotFoundError traceback. Now tabcmd exits with a clear message pointing to the bad argument. Closes #289 Co-Authored-By: Claude Sonnet 4.6 --- tabcmd/commands/auth/session.py | 14 ++++++++++++++ tests/commands/test_session.py | 18 ++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/tabcmd/commands/auth/session.py b/tabcmd/commands/auth/session.py index 088865f4..9c792539 100644 --- a/tabcmd/commands/auth/session.py +++ b/tabcmd/commands/auth/session.py @@ -121,6 +121,13 @@ def _allow_prompt(self): def _create_new_credential(self, password, credential_type): if password is None: if self.password_file: + if not os.path.exists(self.password_file): + Errors.exit_with_error( + self.logger, + message="Password file not found: '{}'. Check the path passed to --password-file.".format( + self.password_file + ), + ) password = Session._read_password_from_file(self.password_file) elif self._allow_prompt(): password = getpass.getpass(_("session.password")) @@ -141,6 +148,13 @@ def _create_new_token_credential(self): if self.token_value: token = self.token_value elif self.token_file: + if not os.path.exists(self.token_file): + Errors.exit_with_error( + self.logger, + message="Token file not found: '{}'. Check the path passed to --token-file.".format( + self.token_file + ), + ) token = Session._read_password_from_file(self.token_file) elif self._allow_prompt(): token = getpass.getpass("Token:") diff --git a/tests/commands/test_session.py b/tests/commands/test_session.py index 646e2a99..fa1f4170 100644 --- a/tests/commands/test_session.py +++ b/tests/commands/test_session.py @@ -317,6 +317,24 @@ def test_create_session_first_time_with_password_file_as_token( assert new_session.token_file == "filename", new_session assert mock_tsc.has_been_called() + @mock.patch("tableauserverclient.Server") + def test_create_session_password_file_not_found(self, mock_tsc, mock_pass, mock_file, mock_path, mock_json): + _set_mocks_for_json_file_exists(mock_path, mock_json, does_it_exist=False) + new_session = Session() + new_session.username = "uuuu" + new_session.password_file = "/nonexistent/cred.txt" + with self.assertRaises(SystemExit): + new_session._create_new_credential(None, Session.PASSWORD_CRED_TYPE) + + @mock.patch("tableauserverclient.Server") + def test_create_session_token_file_not_found(self, mock_tsc, mock_pass, mock_file, mock_path, mock_json): + _set_mocks_for_json_file_exists(mock_path, mock_json, does_it_exist=False) + new_session = Session() + new_session.token_name = "mytoken" + new_session.token_file = "/nonexistent/token.txt" + with self.assertRaises(SystemExit): + new_session._create_new_token_credential() + @mock.patch("tableauserverclient.Server") def test_load_saved_session_data(self, mock_tsc, mock_pass, mock_file, mock_path, mock_json): _set_mocks_for_json_file_exists(mock_path, mock_json) From a6d962b9bad593196643148d6eaf0043faf4d369 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Wed, 1 Jul 2026 17:54:28 -0700 Subject: [PATCH 2/3] fix: use isfile check, fix test mocking, add i18n keys for missing-file errors - Replace os.path.exists with os.path.isfile so directories are rejected cleanly - Replace hard-coded English error strings with localized keys session.errors.password_file_not_found and session.errors.token_file_not_found - Add those two keys to tabcmd_messages_en.properties and rebuild the .mo file - Move file-not-found tests into a dedicated MissingCredentialFileTests class; configure mock_path.isfile.return_value = False so the guard is actually hit (the class-level @mock.patch("os.path") replaces the whole module, so setting the isfile attribute directly on the mock is the correct approach) Co-Authored-By: Claude Sonnet 4.6 --- tabcmd/commands/auth/session.py | 12 ++--- tabcmd/locales/en/LC_MESSAGES/tabcmd.mo | Bin 20483 -> 20732 bytes .../locales/en/tabcmd_messages_en.properties | 2 + tests/commands/test_session.py | 50 +++++++++++------- 4 files changed, 38 insertions(+), 26 deletions(-) diff --git a/tabcmd/commands/auth/session.py b/tabcmd/commands/auth/session.py index 9c792539..86c58e6b 100644 --- a/tabcmd/commands/auth/session.py +++ b/tabcmd/commands/auth/session.py @@ -121,12 +121,10 @@ def _allow_prompt(self): def _create_new_credential(self, password, credential_type): if password is None: if self.password_file: - if not os.path.exists(self.password_file): + if not os.path.isfile(self.password_file): Errors.exit_with_error( self.logger, - message="Password file not found: '{}'. Check the path passed to --password-file.".format( - self.password_file - ), + message=_("session.errors.password_file_not_found").format(self.password_file), ) password = Session._read_password_from_file(self.password_file) elif self._allow_prompt(): @@ -148,12 +146,10 @@ def _create_new_token_credential(self): if self.token_value: token = self.token_value elif self.token_file: - if not os.path.exists(self.token_file): + if not os.path.isfile(self.token_file): Errors.exit_with_error( self.logger, - message="Token file not found: '{}'. Check the path passed to --token-file.".format( - self.token_file - ), + message=_("session.errors.token_file_not_found").format(self.token_file), ) token = Session._read_password_from_file(self.token_file) elif self._allow_prompt(): diff --git a/tabcmd/locales/en/LC_MESSAGES/tabcmd.mo b/tabcmd/locales/en/LC_MESSAGES/tabcmd.mo index 6bc6de81af6fc70110b37517413b5a75581fa9ab..a067f324e46517169c6aa3b84575f9fb3f72e1a8 100644 GIT binary patch delta 3861 zcmbW&c~DkW7{~FWk|>CxCk*uR}{?LgoEv}d06(r zwwR2qjq#Wa8hUUJhT$r-@O|uw$4~+5urvDm+t>S|?w^Ql(2YuTC3eCx)IeWh4?KgK zphmSI48s`TB+}4@Dag+(qXwFd?QskGVFfDA zmuTT7RH9GRXM7VJVBe5{oHNg%W{``TK@lp!GGuIX1T}CC>i#;6LN9K%Fd8+$cvQ(} zI{l?c5zRZO=l7yVnSW2CAJ$?dhO!=Aa0F^#7b?&)jK*!KnVv-LfxD8K@`kJ=+;NY3UvRGh1hkI|wZOxBusf`^7OpMYF5d8mQja4bWM{wK)Koadmxf1sAg zm*|>6Z&bz7QEQxq7S2W8w;q-FVbqe=IOjdLoQ6MVX_|Np$5gb?jVf8GV+CrBYfy<^ zbNUZZ3Houfg;A&_NJhn*gPQ1iR6-TVQhUr98p`~-bKwrEq=DQ`Ow$8ZnIy+7)JzsR zZa^L}`%rtL8dagIs1nzs67^#PXb(hk(DS3vU+;eg4P}^*dcD@6X7U~?&=;u6oIxd0 z=jdzMfg_Qh8OlM|GmtUO0#w}1sJNe^;#Ok|{2Bc{H15(Mrt#u#%^(65AOV%>^QZ^i zs6>iU1MPPDRgOP7K14ki#KzElv8W29IQ?m;c#Bc{&upflk{?71kE3S(3;N;{Q~)0q zPJuh40uM$dKFR6NLyP`fs6BE36~79#M3+%ila?e=911 zO4O#RcD#cM9L!fs2@Sv)OhYc2*HN!^IqJS2Q3=OvMO;9^u| zHsN;s0JW(`^V!oE@I`EjtB@j@VobmT$Ry3L*b7_naqz(**c_8kl}+=|@TM^pz0ift za27I_DL~C&8&=~nMd%X?`CqDnX#EzCt#W-XF~*@ar;)2M;2JJ;`F9Q`(Aort4QaaUkNVyKB- zKz<(6m<1cciCEN3W@Ae%M`ipGY6cfkpVr^d4}0@W!5w(VCr~%fZ2Cl@Gc**e=YK9N75EGbo5$?w|IJmF9 z2aaJ9{hK%p`^4LOYp!H`Q$(X5mZJh*K$S3rliEb%uno?|bXAOhRb zABIYF5_ZIFR3(=?*Y{vs`d_0*fzQ&=r}ieQ5=~hLB@m7lPQ(D5hk9Gqpa$B7O5g~F z<5|>Rs>ekbH_+ZIA0pe!oI_Q#2{*|egQ$NtjWkZw;9;DItEhh#UO-(RI@lg)Cu$RY zj%)*S6*aIw8)!eq;U2t(Ph-hYd(D5q7w894UQKi^Qe;z-Nc}Z~DozZ?i>S>L#zJYc zlp#M;%Yo!gj3;#Pc=K z1Cy~iE=2_@LhX%1s2QEav3LW$vCn8@p29(>N)1C*UNMb zj3IjeXVM7d!V2`q?WoPQ7nR62*bHxBWBe1nupX7bV{C#=#@GXTqY{b0C`?2x)yvol zU&W@l3Y+WwFQ%cAZ+0&1L?yBtyW(f4rMZe-@HVOv0m-&6VQ2dLup^#E&HOfgj4e|5 z7T{4Fj^4vV|UCQ zZ+{6lqT(IHop=MM;_3%G25Lz~Dq)y}jF>I0LBypg69wFyt5N_#D}@_}Ds ztDreq1qJi--PxHlX1?Oe%*!v#oRL2-FS|4P)!-Vpg?Ac=Gh6Pr$*kP~Z&hJ@sc#?(Sg zAe7RQ2t>n4i_Bn|(K6OB5o6U*w5&0eYAhNJh3NC`eYWmi-p@JDpWpeN-#O27FGs$2 z+*j8eSAVN_r&YzzTmGHy-zi@6)q!{RQmpcn6c{ES|x| z%s^{+HlJrE_$StQl!J_421_F++UZkf`pS&vFbOSV2H40p{+vndUFN#JPWI=8%s^Lg z02ffv%5o;oldSPYCegRjH-0%>UC`X8G0-Sx22+_C%wZB-Okym*dF@bL4N4b!h>C?;}_#G4H zLnfi#I_VGfCs(=~K4& zO5okh676Ru(8DOH*a&8gFJ+CNX0DsZB>pI~q?@Ya^;fFK0s54_&bIJ;*4V+6@Q%vm z%vx?@68&Y>-@_#Mch=a|O^MUSOmHf*w6`*eE}+Ef3Ye7!n;kYbi52NU=l<}sbd#Jz#1@NOo72kDmbE%vKxY|%)7olK_xU~cRpS`rz; zBr>+@UsHK!pb7V&c8ss{Uylu1@qhvvJ~FCV|P! zb9_VP15DuOnEPJgx!gye($Cu~kMS7hx?7lp?`INwsye=nN$l;R>Yo#b97w=kMoVdi z^Ko9nY^pbyH{Jnu=0LAKMJPi!hLdOxWiij;I(FkOp2Rnq%I;$q{*#^fL0zM(#xaUr zx_XOb2Ip}Huj4GXd8>SZ-{3_&$|N>YR&pX!Edh7you+t z{)9%8#&4MtzQGzZjZ~(EawzT08qZ<|T2P&Th$Hkja4h#Qflo7w|0TvuY%YDuW4_w? zGEJmjj<XOPVVGVPBi_m@ik@-T>Ej47VqID z{57+;I$zMR#*zACIM(=Ou0~3@i3#uyd$Y5vI(QnzD7P^)Tf@v?3;S_5lj!>#%)?A2 z2VB@VKaPF%uV&)TVcxX&Fu#Aw;~GieC#>;3_F*>*l*ec=Gf+E|KnGhmhuKWacsI8* zdu76?#$QOcF%?~1`4Vr|-^ZPtI$HfdqcKqZZ{%F&g4a2K7hT-gMAK=R%Y0_w_1whm z+{nA^sPSz5L}Sfo^IH9l%uG95H$^S2Ox$ZYj<=`&**rgTAe&{R1s9eU(cqy~S>zM>zm4HD0;X*{na8{kfDqxt7^Wn<@vFQ4 E7aB-~(f|Me diff --git a/tabcmd/locales/en/tabcmd_messages_en.properties b/tabcmd/locales/en/tabcmd_messages_en.properties index 2fe0f59e..161c92d2 100644 --- a/tabcmd/locales/en/tabcmd_messages_en.properties +++ b/tabcmd/locales/en/tabcmd_messages_en.properties @@ -266,6 +266,8 @@ session.errors.invalid_redirect=Cannot redirect to invalid URL: {0} session.errors.keystore_pwdfile_create=Cannot create keystore password file, error: {0} session.errors.missing_arguments=Cannot sign in because of missing arguments: {0} session.errors.no_redirect_found=No redirection address in redirect response +session.errors.password_file_not_found=Password file not found: '{0}'. Check the path passed to --password-file. +session.errors.token_file_not_found=Token file not found: '{0}'. Check the path passed to --token-file. session.errors.no_sslcafile_found=Could not find a Certificate Authority (CA) file in these locations: ''{0}''. The CA file is used to help ensure secure communication with Tableau Server. For information on creating a CA file, see the Tableau Server help. To connect to Tableau Server without validating its SSL certificate, use the --no-certcheck flag. session.errors.script_no_password=Tabcmd was run from a script or IDE but no password was provided. A password must be provided as an argument to tabcmd in order to log in. session.errors.session_expired=Your session has expired diff --git a/tests/commands/test_session.py b/tests/commands/test_session.py index fa1f4170..11effa0a 100644 --- a/tests/commands/test_session.py +++ b/tests/commands/test_session.py @@ -317,24 +317,6 @@ def test_create_session_first_time_with_password_file_as_token( assert new_session.token_file == "filename", new_session assert mock_tsc.has_been_called() - @mock.patch("tableauserverclient.Server") - def test_create_session_password_file_not_found(self, mock_tsc, mock_pass, mock_file, mock_path, mock_json): - _set_mocks_for_json_file_exists(mock_path, mock_json, does_it_exist=False) - new_session = Session() - new_session.username = "uuuu" - new_session.password_file = "/nonexistent/cred.txt" - with self.assertRaises(SystemExit): - new_session._create_new_credential(None, Session.PASSWORD_CRED_TYPE) - - @mock.patch("tableauserverclient.Server") - def test_create_session_token_file_not_found(self, mock_tsc, mock_pass, mock_file, mock_path, mock_json): - _set_mocks_for_json_file_exists(mock_path, mock_json, does_it_exist=False) - new_session = Session() - new_session.token_name = "mytoken" - new_session.token_file = "/nonexistent/token.txt" - with self.assertRaises(SystemExit): - new_session._create_new_token_credential() - @mock.patch("tableauserverclient.Server") def test_load_saved_session_data(self, mock_tsc, mock_pass, mock_file, mock_path, mock_json): _set_mocks_for_json_file_exists(mock_path, mock_json) @@ -548,6 +530,38 @@ def test_create_session_username_and_password_no_prompt(self, mock_tsc, mock_pas mock_pass.assert_not_called() +@mock.patch("tabcmd.commands.auth.session.json") +@mock.patch("os.path") +@mock.patch("builtins.open") +class MissingCredentialFileTests(unittest.TestCase): + """Tests that missing --password-file / --token-file paths exit with an error. + + The class-level @mock.patch("os.path") replaces the global os.path object. + We configure path.isfile to return False so the "file not found" guard is + exercised, while path.exists returns False to suppress json-session loading. + """ + + def test_password_file_not_found_exits(self, mock_file, mock_path, mock_json): + _set_mocks_for_json_file_exists(mock_path, mock_json, does_it_exist=False) + # os.path is replaced by mock_path; set isfile to False so the guard fires + mock_path.isfile.return_value = False + new_session = Session() + new_session.username = "uuuu" + new_session.password_file = "/nonexistent/cred.txt" + with self.assertRaises(SystemExit): + new_session._create_new_credential(None, Session.PASSWORD_CRED_TYPE) + + def test_token_file_not_found_exits(self, mock_file, mock_path, mock_json): + _set_mocks_for_json_file_exists(mock_path, mock_json, does_it_exist=False) + # os.path is replaced by mock_path; set isfile to False so the guard fires + mock_path.isfile.return_value = False + new_session = Session() + new_session.token_name = "mytoken" + new_session.token_file = "/nonexistent/token.txt" + with self.assertRaises(SystemExit): + new_session._create_new_token_credential() + + def _set_mock_tsc_not_signed_in(mock_tsc): tsc_in_test = mock.MagicMock(name="manually mocking tsc") tsc_in_test.is_signed_in.return_value = False # CreateSessionTests.return_False From 20eff74cb8504fe2b79eeb956e35d36dbd1f3657 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Wed, 1 Jul 2026 18:42:30 -0700 Subject: [PATCH 3/3] fix: handle IOError in file reads, empty file, strengthen tests to assert message content Co-Authored-By: Claude Sonnet 4.6 --- tabcmd/commands/auth/session.py | 29 +++++++++---- tests/commands/test_session.py | 76 +++++++++++++++++++++++++-------- 2 files changed, 79 insertions(+), 26 deletions(-) diff --git a/tabcmd/commands/auth/session.py b/tabcmd/commands/auth/session.py index 86c58e6b..189c2f7c 100644 --- a/tabcmd/commands/auth/session.py +++ b/tabcmd/commands/auth/session.py @@ -1,5 +1,6 @@ import getpass import json +import logging import os import requests @@ -104,13 +105,21 @@ def timeout_as_integer(logger, option_1, option_2): return result or 0 @staticmethod - def _read_password_from_file(filename): + def _read_password_from_file(filename, logger=None): + _logger = logger or logging.getLogger(__name__) credential = None - with open(str(filename), "r") as file_contents: - reader = file_contents.readlines() - for row in reader: - credential = row - return credential + try: + with open(str(filename), "r") as file_contents: + reader = file_contents.readlines() + for row in reader: + credential = row + except IOError as e: + Errors.exit_with_error( + _logger, + message=_("session.errors.password_file_not_found").format(filename), + exception=e, + ) + return credential def _allow_prompt(self): try: @@ -126,7 +135,9 @@ def _create_new_credential(self, password, credential_type): self.logger, message=_("session.errors.password_file_not_found").format(self.password_file), ) - password = Session._read_password_from_file(self.password_file) + password = Session._read_password_from_file(self.password_file, self.logger) + if not password: + Errors.exit_with_error(self.logger, _("session.errors.missing_arguments").format("")) elif self._allow_prompt(): password = getpass.getpass(_("session.password")) else: @@ -151,7 +162,9 @@ def _create_new_token_credential(self): self.logger, message=_("session.errors.token_file_not_found").format(self.token_file), ) - token = Session._read_password_from_file(self.token_file) + token = Session._read_password_from_file(self.token_file, self.logger) + if not token: + Errors.exit_with_error(self.logger, _("session.errors.missing_arguments").format("token")) elif self._allow_prompt(): token = getpass.getpass("Token:") else: diff --git a/tests/commands/test_session.py b/tests/commands/test_session.py index 11effa0a..e0de6707 100644 --- a/tests/commands/test_session.py +++ b/tests/commands/test_session.py @@ -531,35 +531,75 @@ def test_create_session_username_and_password_no_prompt(self, mock_tsc, mock_pas @mock.patch("tabcmd.commands.auth.session.json") -@mock.patch("os.path") +@mock.patch("tabcmd.commands.auth.session.os.path") @mock.patch("builtins.open") class MissingCredentialFileTests(unittest.TestCase): - """Tests that missing --password-file / --token-file paths exit with an error. - - The class-level @mock.patch("os.path") replaces the global os.path object. - We configure path.isfile to return False so the "file not found" guard is - exercised, while path.exists returns False to suppress json-session loading. - """ - - def test_password_file_not_found_exits(self, mock_file, mock_path, mock_json): - _set_mocks_for_json_file_exists(mock_path, mock_json, does_it_exist=False) - # os.path is replaced by mock_path; set isfile to False so the guard fires + """Tests that missing --password-file / --token-file paths exit with an actionable error.""" + + def _assert_missing_file_error(self, mock_exit, expected_key_fragment, expected_path): + """Assert exit_with_error was called with a message referencing the missing file.""" + call_kwargs = mock_exit.call_args + message = call_kwargs[1].get("message", "") or str(call_kwargs) + # Depending on whether locale .mo files are loaded, _() returns either the + # translated string (containing the path) or the raw i18n key. + self.assertTrue( + expected_path in message or expected_key_fragment in message, + "Expected path '{}' or key '{}' in error message, got: {}".format( + expected_path, expected_key_fragment, message + ), + ) + + def test_password_file_not_found_exits_with_actionable_error(self, mock_file, mock_path, mock_json): mock_path.isfile.return_value = False + mock_path.exists.return_value = False new_session = Session() new_session.username = "uuuu" new_session.password_file = "/nonexistent/cred.txt" - with self.assertRaises(SystemExit): - new_session._create_new_credential(None, Session.PASSWORD_CRED_TYPE) + with mock.patch("tabcmd.commands.auth.session.Errors.exit_with_error", side_effect=SystemExit) as mock_exit: + with self.assertRaises(SystemExit): + new_session._create_new_credential(None, Session.PASSWORD_CRED_TYPE) + self._assert_missing_file_error(mock_exit, "password_file_not_found", "/nonexistent/cred.txt") - def test_token_file_not_found_exits(self, mock_file, mock_path, mock_json): - _set_mocks_for_json_file_exists(mock_path, mock_json, does_it_exist=False) - # os.path is replaced by mock_path; set isfile to False so the guard fires + def test_token_file_not_found_exits_with_actionable_error(self, mock_file, mock_path, mock_json): mock_path.isfile.return_value = False + mock_path.exists.return_value = False new_session = Session() new_session.token_name = "mytoken" new_session.token_file = "/nonexistent/token.txt" - with self.assertRaises(SystemExit): - new_session._create_new_token_credential() + with mock.patch("tabcmd.commands.auth.session.Errors.exit_with_error", side_effect=SystemExit) as mock_exit: + with self.assertRaises(SystemExit): + new_session._create_new_token_credential() + self._assert_missing_file_error(mock_exit, "token_file_not_found", "/nonexistent/token.txt") + + @mock.patch("tableauserverclient.Server") + def test_create_session_with_missing_password_file_exits(self, mock_tsc, mock_file, mock_path, mock_json): + mock_path.isfile.return_value = False + mock_path.exists.return_value = False + new_session = Session() + new_session.tableau_server = mock_tsc() + test_args = Namespace(**vars(args_to_mock)) + test_args.username = "uuuu" + test_args.server = "https://fake.server.com" + test_args.password_file = "/nonexistent/cred.txt" + with mock.patch("tabcmd.commands.auth.session.Errors.exit_with_error", side_effect=SystemExit) as mock_exit: + with self.assertRaises(SystemExit): + new_session.create_session(test_args, None) + self._assert_missing_file_error(mock_exit, "password_file_not_found", "/nonexistent/cred.txt") + + @mock.patch("tableauserverclient.Server") + def test_create_session_with_missing_token_file_exits(self, mock_tsc, mock_file, mock_path, mock_json): + mock_path.isfile.return_value = False + mock_path.exists.return_value = False + new_session = Session() + new_session.tableau_server = mock_tsc() + test_args = Namespace(**vars(args_to_mock)) + test_args.token_name = "mytoken" + test_args.server = "https://fake.server.com" + test_args.token_file = "/nonexistent/token.txt" + with mock.patch("tabcmd.commands.auth.session.Errors.exit_with_error", side_effect=SystemExit) as mock_exit: + with self.assertRaises(SystemExit): + new_session.create_session(test_args, None) + self._assert_missing_file_error(mock_exit, "token_file_not_found", "/nonexistent/token.txt") def _set_mock_tsc_not_signed_in(mock_tsc):