Skip to content

Commit 787d074

Browse files
Mathieu Turcotteclaude
andcommitted
refactor(auth): improve code quality and add missing tests
- Replace deprecated datetime.utcnow() with datetime.now(timezone.utc) - Add case-insensitive username matching in session resolution - Cache MSAL app per tenant in list_accounts to avoid redundant creation - Log TypeError instead of silently swallowing in _get_matching_account - Add clarifying comments on acquire_token branching logic - Only reset MSAL app when tenant actually changes (set_tenant, _sync) - Expand docs with parameter descriptions and behavior details - Add edge case tests: env var block, case-insensitive switch, 3+ account prompt, cancelled prompt, empty list, app preservation on tenant match Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 12709c6 commit 787d074

6 files changed

Lines changed: 336 additions & 14 deletions

File tree

docs/commands/auth/index.md

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,19 +40,27 @@ fab auth login [-u <client_id>] [-p <client_secret>] [--federated-token <token>]
4040

4141
### logout
4242

43-
End the current authentication session.
43+
End the current authentication session. When multiple user sessions are stored you can target a specific session by account name and/or tenant.
4444

4545
**Usage:**
4646

4747
```
4848
fab auth logout [-u <account_name>] [-t <tenant_id>] [--all]
4949
```
5050

51+
**Parameters:**
52+
53+
- `-u, --username`: Account name of the session to log out. Case-insensitive. Optional.
54+
- `-t, --tenant`: Tenant ID to disambiguate when the same account exists in multiple tenants. Optional.
55+
- `--all`: Clear all stored authentication sessions. Optional.
56+
57+
When neither `-u` nor `--all` is provided, the CLI removes the current active session. If other sessions remain, the next most recently used session becomes active.
58+
5159
---
5260

5361
### list
5462

55-
List stored user authentication sessions.
63+
List stored user authentication sessions. Each row shows whether the session is active, the account name, tenant, token validity, and the last used timestamp.
5664

5765
**Usage:**
5866

@@ -76,14 +84,25 @@ fab auth status
7684

7785
### switch
7886

79-
Switch the active stored user authentication session.
87+
Switch the active stored user authentication session. You can specify the target account directly to avoid interactive selection.
8088

8189
**Usage:**
8290

8391
```
8492
fab auth switch [-u <account_name>] [-t <tenant_id>]
8593
```
8694

95+
**Parameters:**
96+
97+
- `-u, --username`: Account name to switch to. Case-insensitive. Optional.
98+
- `-t, --tenant`: Tenant ID to disambiguate when the same account exists in multiple tenants. Optional.
99+
100+
**Behavior:**
101+
102+
- With `-u` (and optionally `-t`): switches directly to the matching session.
103+
- With two stored sessions and no flags: automatically toggles to the other session.
104+
- With three or more sessions and no flags: presents an interactive prompt.
105+
87106
---
88107

89108
For more examples and detailed scenarios, see [Authentication Examples](../../examples/auth_examples.md).

docs/examples/auth_examples.md

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,16 +131,40 @@ fab auth list
131131

132132
### Switch to another stored user session
133133

134+
When two sessions are stored, `switch` automatically toggles to the other account:
135+
134136
```
135137
fab auth switch
136138
```
137139

140+
With three or more sessions and no flags, an interactive prompt is shown.
141+
142+
### Switch directly to a specific stored account
143+
144+
Account name matching is case-insensitive:
145+
146+
```
147+
fab auth switch -u alice@example.com
148+
```
149+
138150
### Switch directly to a specific stored account and tenant
139151

140152
```
141153
fab auth switch -u <account_name> -t <tenant_id>
142154
```
143155

156+
### Log out a specific stored session
157+
158+
```
159+
fab auth logout -u alice@example.com
160+
```
161+
162+
### Log out all stored sessions
163+
164+
```
165+
fab auth logout --all
166+
```
167+
144168

145169
## Authentication Status
146170

src/fabric_cli/commands/auth/fab_auth.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -356,14 +356,21 @@ def list_accounts(args: Namespace) -> None:
356356
active_session = auth.get_active_user_session()
357357
active_session_id = active_session.get("session_id") if active_session else None
358358

359+
# Cache one MSAL app per tenant to avoid redundant app creation during validation.
360+
tenant_apps: dict[str, Any] = {}
359361
rows = []
360362
for session in sessions:
363+
tid = session.get("tenant_id")
364+
if tid not in tenant_apps:
365+
tenant_apps[tid] = auth._create_user_app(tid)
361366
rows.append(
362367
{
363368
"active": str(session.get("session_id") == active_session_id).lower(),
364369
"account": session.get("account_name", "Unknown"),
365370
"tenant_id": session.get("tenant_id", "Unknown"),
366-
"valid": str(auth.is_user_session_valid(session)).lower(),
371+
"valid": str(
372+
auth.is_user_session_valid(session, app=tenant_apps[tid])
373+
).lower(),
367374
"last_used_at": session.get("last_used_at", ""),
368375
}
369376
)
@@ -428,8 +435,9 @@ def _resolve_user_session(
428435
active_session_id = active_session.get("session_id") if active_session else None
429436

430437
candidates = []
438+
username_lower = username.lower() if username else None
431439
for session in sessions:
432-
if username and session.get("account_name") != username:
440+
if username_lower and (session.get("account_name") or "").lower() != username_lower:
433441
continue
434442
if tenant_id and session.get("tenant_id") != tenant_id:
435443
continue

src/fabric_cli/core/fab_auth.py

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
import os
66
import uuid
77
from binascii import hexlify
8-
from datetime import datetime
8+
from datetime import datetime, timezone
99
from typing import Any, NamedTuple, Optional
1010

1111
import jwt
@@ -263,7 +263,9 @@ def _get_matching_account(
263263
accounts = (
264264
app.get_accounts(account_name) if account_name else app.get_accounts()
265265
)
266-
except TypeError:
266+
except TypeError as e:
267+
# MSAL may not accept a username filter in all broker configurations.
268+
fab_logger.log_debug(f"Filtered get_accounts failed, retrying unfiltered: {e}")
267269
accounts = app.get_accounts()
268270

269271
if not isinstance(accounts, list):
@@ -295,7 +297,8 @@ def _get_matching_account(
295297
if tenant_id:
296298
try:
297299
all_accounts = app.get_accounts()
298-
except TypeError:
300+
except TypeError as e:
301+
fab_logger.log_debug(f"get_accounts failed during tenant fallback: {e}")
299302
all_accounts = []
300303

301304
if not isinstance(all_accounts, list):
@@ -361,7 +364,12 @@ def _build_current_user_session_from_cache(self) -> Optional[dict[str, Any]]:
361364

362365
@staticmethod
363366
def _utc_now() -> str:
364-
return datetime.utcnow().replace(microsecond=0).isoformat() + "Z"
367+
return (
368+
datetime.now(timezone.utc)
369+
.replace(microsecond=0)
370+
.isoformat()
371+
.replace("+00:00", "Z")
372+
)
365373

366374
def _build_user_session(
367375
self,
@@ -465,9 +473,13 @@ def _sync_active_user_session(self, save: bool = True) -> Optional[dict[str, Any
465473
self._save_auth()
466474
return None
467475

468-
self.app = None
476+
# Only reset the MSAL app when the tenant changes so that the active
477+
# WAM broker session is preserved during the login flow.
478+
new_tenant = session.get("tenant_id")
479+
if self._auth_info.get(con.FAB_TENANT_ID) != new_tenant:
480+
self.app = None
469481
self._auth_info[con.IDENTITY_TYPE] = "user"
470-
self._auth_info[con.FAB_TENANT_ID] = session.get("tenant_id")
482+
self._auth_info[con.FAB_TENANT_ID] = new_tenant
471483
self._auth_info[ACTIVE_USER_ACCOUNT_NAME_KEY] = session.get("account_name")
472484

473485
if session.get("home_account_id"):
@@ -619,9 +631,14 @@ def remove_user_session(self, session_id: str) -> Optional[dict[str, Any]]:
619631
self._save_auth()
620632
return self._find_user_session()
621633

622-
def is_user_session_valid(self, session: dict[str, Any]) -> bool:
634+
def is_user_session_valid(
635+
self,
636+
session: dict[str, Any],
637+
app: Optional[msal.PublicClientApplication] = None,
638+
) -> bool:
623639
try:
624-
app = self._create_user_app(session.get("tenant_id"))
640+
if app is None:
641+
app = self._create_user_app(session.get("tenant_id"))
625642
account = self._get_matching_account(app, session)
626643
if account is None:
627644
return False
@@ -782,7 +799,11 @@ def set_access_mode(self, mode, tenant_id=None):
782799
def set_tenant(self, tenant_id):
783800
if tenant_id is not None:
784801
if self.get_identity_type() == "user":
785-
self.app = None
802+
# Only reset the MSAL app when the tenant actually changes so
803+
# that the active WAM broker session is preserved across scope
804+
# requests during login.
805+
if self.get_tenant_id() != tenant_id:
806+
self.app = None
786807
self._set_auth_properties(
787808
{
788809
con.FAB_TENANT_ID: tenant_id,
@@ -959,6 +980,8 @@ def acquire_token(
959980
scopes=scope, account=account
960981
)
961982

983+
# When force_interactive=True the silent block above is skipped so token
984+
# remains None, falling through here to trigger the interactive prompt.
962985
if token is None and interactive_renew and identity_type == "user":
963986
token = self._get_app().acquire_token_interactive(
964987
scopes=scope,
@@ -975,6 +998,7 @@ def acquire_token(
975998
if session is not None:
976999
self._persist_user_session(session)
9771000
elif token and active_session is not None and not force_interactive:
1001+
# Silent acquisition succeeded — update the session timestamp.
9781002
self._persist_user_session(active_session)
9791003

9801004
if token and token.get("error"):

tests/test_commands/test_auth.py

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1044,6 +1044,149 @@ def test_auth_logout_user_session(self, mock_fab_auth, mock_fab_context):
10441044
mock_fab_context_instance.reset_context.assert_called_once()
10451045
mock_print.assert_called_once()
10461046

1047+
def test_auth_list_no_sessions(self, mock_fab_auth):
1048+
args = argparse.Namespace(command="auth", output_format=None)
1049+
mock_fab_auth_instance = mock_fab_auth.get("instance")
1050+
mock_fab_auth_instance.get_user_sessions.return_value = []
1051+
1052+
with patch(
1053+
"fabric_cli.commands.auth.fab_auth.fab_ui.print_output_format"
1054+
) as mock_print:
1055+
fab_auth.list_accounts(args)
1056+
1057+
mock_print.assert_called_once()
1058+
assert "message" in mock_print.call_args.kwargs
1059+
1060+
def test_auth_switch_env_vars_blocks(self, mock_fab_auth):
1061+
args = argparse.Namespace(
1062+
command="auth",
1063+
output_format=None,
1064+
username=None,
1065+
tenant=None,
1066+
)
1067+
with patch.dict(os.environ, {"FAB_TOKEN": "some-token"}):
1068+
with pytest.raises(FabricCLIError) as exc_info:
1069+
fab_auth.switch(args)
1070+
assert exc_info.value.status_code == fab_constant.ERROR_INVALID_OPERATION
1071+
1072+
def test_auth_switch_case_insensitive_username(self, mock_fab_auth, mock_fab_context):
1073+
args = argparse.Namespace(
1074+
command="auth",
1075+
output_format=None,
1076+
username="ALICE@EXAMPLE.COM",
1077+
tenant=None,
1078+
)
1079+
mock_fab_auth_instance = mock_fab_auth.get("instance")
1080+
mock_fab_auth_instance.get_user_sessions.return_value = [
1081+
{
1082+
"session_id": "session-a",
1083+
"account_name": "alice@example.com",
1084+
"tenant_id": "tenant-a",
1085+
"last_used_at": "2026-04-07T12:00:00Z",
1086+
}
1087+
]
1088+
mock_fab_auth_instance.get_active_user_session.return_value = {
1089+
"session_id": "session-a"
1090+
}
1091+
1092+
with patch(
1093+
"fabric_cli.commands.auth.fab_auth.fab_ui.print_output_format"
1094+
):
1095+
fab_auth.switch(args)
1096+
1097+
mock_fab_auth_instance.activate_user_session.assert_called_once_with(
1098+
"session-a"
1099+
)
1100+
1101+
def test_auth_switch_interactive_prompt_with_three_accounts(
1102+
self, mock_fab_auth, mock_fab_context
1103+
):
1104+
args = argparse.Namespace(
1105+
command="auth",
1106+
output_format=None,
1107+
username=None,
1108+
tenant=None,
1109+
)
1110+
mock_fab_auth_instance = mock_fab_auth.get("instance")
1111+
mock_fab_auth_instance.get_user_sessions.return_value = [
1112+
{
1113+
"session_id": "session-a",
1114+
"account_name": "alice@example.com",
1115+
"tenant_id": "tenant-a",
1116+
"last_used_at": "2026-04-07T12:00:00Z",
1117+
},
1118+
{
1119+
"session_id": "session-b",
1120+
"account_name": "bob@example.com",
1121+
"tenant_id": "tenant-b",
1122+
"last_used_at": "2026-04-06T12:00:00Z",
1123+
},
1124+
{
1125+
"session_id": "session-c",
1126+
"account_name": "carol@example.com",
1127+
"tenant_id": "tenant-c",
1128+
"last_used_at": "2026-04-05T12:00:00Z",
1129+
},
1130+
]
1131+
mock_fab_auth_instance.get_active_user_session.return_value = {
1132+
"session_id": "session-a"
1133+
}
1134+
1135+
with (
1136+
patch(
1137+
"fabric_cli.commands.auth.fab_auth.fab_ui.prompt_select_item",
1138+
return_value="bob@example.com (tenant-b)",
1139+
),
1140+
patch(
1141+
"fabric_cli.commands.auth.fab_auth.fab_ui.print_output_format"
1142+
),
1143+
):
1144+
fab_auth.switch(args)
1145+
1146+
mock_fab_auth_instance.activate_user_session.assert_called_once_with(
1147+
"session-b"
1148+
)
1149+
1150+
def test_auth_switch_cancelled_prompt(self, mock_fab_auth):
1151+
args = argparse.Namespace(
1152+
command="auth",
1153+
output_format=None,
1154+
username=None,
1155+
tenant=None,
1156+
)
1157+
mock_fab_auth_instance = mock_fab_auth.get("instance")
1158+
mock_fab_auth_instance.get_user_sessions.return_value = [
1159+
{
1160+
"session_id": "session-a",
1161+
"account_name": "alice@example.com",
1162+
"tenant_id": "tenant-a",
1163+
"last_used_at": "2026-04-07T12:00:00Z",
1164+
},
1165+
{
1166+
"session_id": "session-b",
1167+
"account_name": "bob@example.com",
1168+
"tenant_id": "tenant-b",
1169+
"last_used_at": "2026-04-06T12:00:00Z",
1170+
},
1171+
{
1172+
"session_id": "session-c",
1173+
"account_name": "carol@example.com",
1174+
"tenant_id": "tenant-c",
1175+
"last_used_at": "2026-04-05T12:00:00Z",
1176+
},
1177+
]
1178+
mock_fab_auth_instance.get_active_user_session.return_value = {
1179+
"session_id": "session-a"
1180+
}
1181+
1182+
with patch(
1183+
"fabric_cli.commands.auth.fab_auth.fab_ui.prompt_select_item",
1184+
return_value=None,
1185+
):
1186+
with pytest.raises(FabricCLIError) as exc_info:
1187+
fab_auth.switch(args)
1188+
assert exc_info.value.status_code == fab_constant.ERROR_OPERATION_CANCELLED
1189+
10471190
def test_init_when_user_cancels_the_prompt(
10481191
self, mock_fab_auth, mock_fab_context, mock_fab_logger_log_warning, capsys
10491192
):

0 commit comments

Comments
 (0)