Fix review findings in saved-session restore (follow-up to #490) by jacalata · Pull Request #491 · tableau/tabcmd

jacalata · 2026-11-03T01:50:06Z

Follow-up to #490. Addresses the Copilot review comments on that PR and the problems found in a full review of session.py. Each item is stated as problem, then fix.

Saved-session reuse

Problem: a saved token was reattached regardless of which server or site the new command targeted. Fix: it is reused only when server and site match the saved ones; otherwise the command signs in normally.
Problem: if the saved session's server URL could not be opened (for example a malformed URL), the process exited instead of falling back to a normal sign-in. Fix: the failure is caught and the command falls back.
Problem: logout only deleted the local file, leaving the token valid on the server. Fix: it signs the saved token out on the server first. A failed server sign-out is logged and does not stop the local cleanup, and a live session is signed out once, not twice.
Problem: site names were lowercased when saved, but site names are case-sensitive. Fix: they keep their case. default still maps to the default site, and username and server URL are still lowercased.
--server clears the saved site, so logging in to a new server does not carry over the old site.

Session file

Problem: ~/tableau_auth.json was written with default permissions and holds the token or PAT secret. Fix: it is created with mode 0600. A failed chmod is logged and the write still happens.
Problem: proxy and HTTP option values could appear in logs, including proxy credentials. Fix: they are redacted.
Problem: a partially written or empty file could be half-loaded and then wiped. Fix: loading uses safe lookups and stops after clearing an empty file.

Cleanups

Removed the duplicated TOKEN_CRED_TYPE / PASSWORD_CRED_TYPE definitions, the unused last_command, and several dead checks.
Sign-in reads the token from the public auth_token property instead of a private attribute.

Tests

test_session.py goes from a handful of tests to about 105. Several earlier assertions always passed (was_called() is always truthy on a mock); they now assert real behaviour.
The test module no longer reads or deletes the real ~/tableau_auth.json.

Sessions saved by older versions have a lowercased site name. If a later command passes -t with different casing, that session is not reused and the command signs in again.

🤖 Generated with Claude Code

Review of the #490 session-restore change, plus fixes found while reviewing session.py as a whole. Restore saved session: - Do not reuse the saved token when --server or --site targets a different server or site than the one it was saved for. - Attach the saved token inside the try block, so a failure falls back to a fresh sign-in instead of aborting the command. Session file: - Create the file with mode 0600 and tighten existing files on save. - Redact user:password in proxy URLs wherever they are logged. - Load optional keys with .get() so a file from another version keeps the session. Logic fixes: - host:port server URLs get the default https scheme before parsing. - logout signs out the saved token on the server; local cleanup still runs if that fails. - _clear_data resets server_url (it previously reset a nonexistent attribute). - Token name prints only when set; certificate prints independently. - Trailing newline is stripped from password and token files. - Proxy applies to https as well as http. - Timeout values of zero or less become 0. - Empty session file is wiped once. - Site name keeps its case; only the "default" check is case-insensitive. Cleanup: remove duplicate credential-type constants, duplicate logging_level assignment, redundant session-file read, and unused exception bindings. Tests: replace was_called()/has_been_called() (always truthy on a mock) with real assertions, fix clear_session which was never collected, and add coverage for the fixes and for _update_session_data, _sign_in, _validate_existing_signin, saved credentials, --no-cookie, and logout. Co-Authored-By: Claude Code <noreply@anthropic.com>

… tests - Read the saved token from TSC's public auth_token property, not _auth_token. - Log validation failures with the shared internal_error message. - Remove connection checks that could not fire after the connection is set. - Remove the unused last_command attribute. - Add tests for _remove_json, _wipe_bad_json and an unreadable session file. Co-Authored-By: Claude Code <noreply@anthropic.com>

…ate session tests - Catch SystemExit from _open_connection_with_opts in session restore and saved-token sign-out so a bad server URL falls back instead of exiting. - Restore the unexpected_server_response message for ServerResponseError. - Sign out the saved token only when there is no live connection. - Patch Session._get_file_path for the whole test module so tests never touch the real ~/tableau_auth.json. - Add tests for connection failures, live-session logout and chmod failure. Co-Authored-By: Claude Code <noreply@anthropic.com>

github-actions · 2026-11-03T01:50:49Z

Coverage Report

File	Stmts	Miss	Cover	Missing
tabcmd
__main__.py	12	12	12	0%
_version.py	11	11	11	0%
tabcmd.py	15	15	15	0%
version.py	10	6	6	40%
tabcmd/commands
commands.py	10	10	10	0%
constants.py	77	14	14	82%
server.py	135	18	18	87%
tabcmd/commands/auth
session.py	455	31	31	93%
tabcmd/commands/datasources_and_workbooks
datasources_and_workbooks_command.py	170	18	18	89%
datasources_workbooks_views_url_parser.py	142	5	5	96%
delete_command.py	70	23	23	67%
export_command.py	126	21	21	83%
get_url_command.py	127	47	47	63%
publish_command.py	134	16	16	88%
runschedule_command.py	21	7	7	67%
tabcmd/commands/extracts
create_extracts_command.py	42	8	8	81%
decrypt_extracts_command.py	27	2	2	93%
delete_extracts_command.py	37	6	6	84%
encrypt_extracts_command.py	27	2	2	93%
extracts.py	20	2	2	90%
reencrypt_extracts_command.py	27	2	2	93%
refresh_extracts_command.py	48	10	10	79%
tabcmd/commands/group
create_group_command.py	29	5	5	83%
delete_group_command.py	27	2	2	93%
tabcmd/commands/project
create_project_command.py	46	8	8	83%
delete_project_command.py	35	4	4	89%
publish_samples_command.py	30	4	4	87%
tabcmd/commands/site
create_site_command.py	34	5	5	85%
delete_site_command.py	27	2	2	93%
edit_site_command.py	66	18	18	73%
list_command.py	77	12	12	84%
list_sites_command.py	29	2	2	93%
tabcmd/commands/user
add_users_command.py	29	5	5	83%
create_site_users.py	115	14	14	88%
create_users_command.py	59	9	9	85%
delete_site_users_command.py	43	5	5	88%
user_data.py	222	31	31	86%
tabcmd/execution
_version.py	2	2	2	0%
global_options.py	155	4	4	97%
localize.py	67	11	11	84%
logger_config.py	72	6	6	92%
tabcmd_controller.py	40	7	7	82%
TOTAL	3103	442	86%

Copilot

🟡 Changes recommended

Scheme-relative server URLs break sign-in, and logout ignores explicit HTTP options and can block indefinitely without a finite timeout, preventing local cleanup.

3 open issues

What changed in this PR

This follow-up to #490 hardens saved-session handling in tabcmd and expands regression coverage for authentication and local session storage.

Changes:

Restricts saved-token reuse to matching servers and sites, preserves site-name casing, and adds server-side logout.
Tightens session-file permissions, redacts proxy credentials in logs, and improves URL, timeout, and credential-file handling.
Expands session tests, replaces ineffective mock assertions, and isolates tests from the real session file.

File	Description
`tests/commands/test_session.py`	Expands authentication, logout, storage, and connection-option tests with isolated session files.
`tabcmd/commands/auth/session.py`	Hardens session restoration, logout, credential storage, and connection handling.

_{Review effort: Balanced}

…acks - _get_server_base_url: only a leading scheme:// counts as a scheme; handle scheme-relative URLs; reject URLs with no host instead of silently falling back to the saved server or http://localhost. - logout honours --proxy, --no-proxy, --timeout, --certificate and --no-certcheck. --server and --site are not forwarded. - Session probe and logout sign-out default to a 30 second timeout when --timeout is not set. Other commands still wait for the server. - Those two paths raise instead of printing an error and exiting when the connection cannot be opened. Co-Authored-By: Claude Code <noreply@anthropic.com>

Copilot

🟡 Changes recommended

The 30-second probe timeout persists on the returned session, potentially causing long-running commands to fail unexpectedly; restore the configured timeout and add regression coverage.

2 open issues

2 resolved since last review

_{Review effort: Balanced}

+            server = self._open_connection_with_opts(
+                raise_on_failure=True, default_timeout=DEFAULT_SESSION_REQUEST_TIMEOUT
+            )


Co-Authored-By: Claude Code <noreply@anthropic.com>

Copilot

🟡 Changes recommended

The restore probe’s 30-second timeout persists on the returned server, causing long-running commands using saved sessions to time out unexpectedly when no user timeout was specified.

3 open issues

1 resolved since last review

Forward explicit HTTP options to the logout connection

_{Review effort: Balanced}

        logger.debug(_("tabcmd.launching"))
        session = Session()
+        # only the connection options; --server and --site must not redirect the saved token
+        session._update_http_options(args)


The probe used a 30 second timeout, and that value stayed on the server returned to the command. Commands that run with a saved session and no --timeout could then fail on slow responses instead of waiting. Reset the timeout to the configured value once the probe succeeds. Logout also reapplies the requested logging level, since creating the Session resets the root logger to INFO. Co-Authored-By: Claude Code <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

jacalata and others added 3 commits November 2, 2026 17:15

Copilot AI balanced review requested due to automatic review settings November 3, 2026 01:50

Copilot started reviewing on behalf of jacalata November 3, 2026 01:50

View session

Copilot AI reviewed Nov 3, 2026

View reviewed changes

Comment thread tabcmd/commands/auth/session.py Resolved

Comment thread tabcmd/commands/auth/session.py Resolved

Comment thread tabcmd/commands/auth/session.py Resolved

Copilot AI balanced review requested due to automatic review settings November 3, 2026 05:13

Copilot started reviewing on behalf of jacalata November 3, 2026 05:13

View session

Copilot AI reviewed Nov 3, 2026

View reviewed changes

Comment thread tabcmd/commands/auth/session.py

Comment on lines +363 to +365

server = self._open_connection_with_opts(

raise_on_failure=True, default_timeout=DEFAULT_SESSION_REQUEST_TIMEOUT

)

Merge origin/development into jac/fix-session-restore-review

9fa94e3

Co-Authored-By: Claude Code <noreply@anthropic.com>

Copilot AI balanced review requested due to automatic review settings November 4, 2026 01:51

Copilot started reviewing on behalf of jacalata November 4, 2026 01:51

View session

Copilot AI reviewed Nov 4, 2026

View reviewed changes

Comment thread tabcmd/commands/auth/logout_command.py

logger.debug(_("tabcmd.launching"))

session = Session()

# only the connection options; --server and --site must not redirect the saved token

session._update_http_options(args)

Comment thread tests/commands/test_session.py Resolved

Copilot AI balanced review requested due to automatic review settings November 7, 2026 06:19

Copilot AI reviewed Nov 7, 2026

Navigation Menu

Repository navigation

Fix review findings in saved-session restore (follow-up to #490) - #491

Fix review findings in saved-session restore (follow-up to #490)#491
jacalata wants to merge 6 commits into
developmentfrom
jac/fix-session-restore-review

jacalata commented Nov 3, 2026

Uh oh!

github-actions Bot commented Nov 3, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jacalata commented Nov 3, 2026

Uh oh!

github-actions Bot commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Nov 3, 2026 •

edited

Loading