Skip to content

Commit 2d082bd

Browse files
fix(auth): stop misreporting login failures and revoking credentials
`HubApi.login` caught `HubError` -- the base class of every SDK exception -- and re-raised everything as `AuthenticationError("...token was rejected by the server")` while calling `clear_token()`. Three problems followed: * network, timeout and 5xx failures were all reported as an invalid token * the server's own message and RequestId were discarded, leaving no way to diagnose the failure or trace it server-side * a failed attempt deleted the credential already on disk, so a single mistyped token logged the user out of a working session Both ModelScope sites answer an unknown token with the same HTTP 400 and business code 10010103009, so the server cannot distinguish "invalid token" from "token issued by the other site" -- only the client knows which site it addressed, which is why that disambiguation now happens here. Changes: * classify 4xx by the server business code, mapping 10010103009 to AuthenticationError (E3001); 5xx stays a retryable ServerError so a transient outage is never reclassified as permanent * propagate non-authentication failures unchanged, preserving their type, error code, message and RequestId * leave persisted credentials untouched on failure and roll back only the in-memory state of the instance * probe the peer site when the endpoint was not pinned, and point at `--endpoint` when the token turns out to be valid there * centralise endpoint normalisation in `HubConfig.normalize_endpoint` so a bare or upper-case scheme no longer fails deep inside the transport layer * remove every credential artefact together on teardown, and propagate a cleared token to an already constructed legacy client * print the full error cause chain under `--verbose` Adds 29 regression tests that stub only the `requests` transport, so the config, facade, legacy client and error-translation layers all take part.
1 parent 74a6357 commit 2d082bd

7 files changed

Lines changed: 687 additions & 49 deletions

File tree

src/modelscope_hub/api.py

Lines changed: 110 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ def __init__(
156156
base._endpoint_overridden = was_overridden
157157
self._config = base
158158
if endpoint is not None:
159-
self._config.endpoint = endpoint.rstrip("/")
159+
self._config.endpoint = HubConfig.normalize_endpoint(endpoint)
160160
self._config._endpoint_overridden = True
161161
if token is not None:
162162
self._config.token = token
@@ -188,7 +188,9 @@ def legacy(self) -> LegacyClient:
188188
endpoint=self._config.endpoint or DEFAULT_ENDPOINT,
189189
user_agent=build_user_agent(self._config.get_session_id()),
190190
)
191-
elif self._legacy.token != self._config.token and self._config.token:
191+
elif self._legacy.token != self._config.token:
192+
# Clears propagate as well as changes: a cached client left holding a
193+
# revoked token would keep authenticating with it.
192194
self._legacy.token = self._config.token
193195
return self._legacy
194196

@@ -483,8 +485,19 @@ def login(self, token: str) -> UserInfo:
483485
InvalidParameter
484486
When ``token`` is empty or whitespace-only.
485487
AuthenticationError
486-
When the server rejects the token. The bad token is cleared
487-
from local storage before re-raising.
488+
When the server rejects the token. The server's own explanation is
489+
preserved, and an endpoint hint is appended when the token turns
490+
out to be valid on the peer ModelScope site.
491+
HubError
492+
Transport, timeout and server-side failures propagate unchanged --
493+
they are never reported as a rejected token.
494+
495+
Notes
496+
-----
497+
A failed attempt leaves persisted credentials untouched. Until the
498+
server has accepted the new token, the stored credential is still the
499+
caller's only working one, so revoking it on failure would turn a
500+
mistyped token into an unintended logout.
488501
489502
Examples
490503
--------
@@ -497,6 +510,8 @@ def login(self, token: str) -> UserInfo:
497510
raise InvalidParameter("token must be a non-empty string")
498511

499512
token = token.strip()
513+
previous_token = self._config.token
514+
previous_logged_out = self._config._logged_out
500515
self._config.token = token
501516
self._config._logged_out = False
502517
self._openapi = None
@@ -505,12 +520,12 @@ def login(self, token: str) -> UserInfo:
505520

506521
try:
507522
data, cookies = self.legacy.login(token)
508-
except (AuthenticationError, HubError) as exc:
509-
self._config.clear_token()
510-
raise AuthenticationError(
511-
"Login failed: the provided token was rejected by the server.",
512-
status_code=getattr(exc, "status_code", None),
513-
) from exc
523+
except HubError as exc:
524+
self._restore_credential_state(previous_token, previous_logged_out)
525+
explained = self._explain_login_failure(token, exc)
526+
if explained is exc:
527+
raise
528+
raise explained from exc
514529

515530
git_token = data.get("AccessToken", "")
516531
username = data.get("Username", "")
@@ -524,6 +539,91 @@ def login(self, token: str) -> UserInfo:
524539

525540
return self.whoami()
526541

542+
def _restore_credential_state(self, token: str | None, logged_out: bool) -> None:
543+
"""Roll the in-memory credential back to its pre-login value.
544+
545+
Persisted credentials are deliberately left alone; only this instance's
546+
transient state is rewound, so a failed attempt leaves the object
547+
exactly as it was found instead of poisoning it with a rejected token.
548+
"""
549+
self._config.token = token
550+
self._config._logged_out = logged_out
551+
self._openapi = None
552+
if self._legacy is not None:
553+
self._legacy.token = token
554+
555+
def _explain_login_failure(self, token: str, exc: HubError) -> HubError:
556+
"""Return the exception to surface for a failed login attempt.
557+
558+
Only authentication failures are re-worded. Network, timeout and
559+
server-side errors are handed back untouched, because presenting them
560+
as a rejected token would send the caller after the wrong remedy.
561+
562+
The two ModelScope sites keep separate account systems and answer an
563+
unknown token with the same business code, so the server cannot tell
564+
"invalid token" apart from "token issued by the other site". Only the
565+
client knows which site it addressed, which is why that disambiguation
566+
has to happen here.
567+
"""
568+
if not isinstance(exc, AuthenticationError):
569+
return exc
570+
peer = self._peer_site_endpoint()
571+
if peer is None or not self._token_valid_on(token, peer):
572+
return exc
573+
return AuthenticationError(
574+
f"{exc.message} This token is valid on {peer} instead; retry with "
575+
f"--endpoint {peer} (or set MODELSCOPE_ENDPOINT={peer}).",
576+
status_code=exc.status_code,
577+
request_id=exc.request_id,
578+
response_body=exc.response_body,
579+
url=exc.url,
580+
method=exc.method,
581+
)
582+
583+
def _peer_site_endpoint(self) -> str | None:
584+
"""Return the sibling ModelScope site, or ``None`` when not applicable.
585+
586+
An explicitly configured endpoint is always respected, mirroring
587+
:meth:`resolve_endpoint_for_read`: when the caller has pinned a site we
588+
do not second-guess it.
589+
"""
590+
if self._config._endpoint_overridden:
591+
return None
592+
from .constants import DEFAULT_INTL_ENDPOINT
593+
594+
def site_key(url: str) -> str:
595+
host = (urlparse(url).hostname or "").lower()
596+
return host[4:] if host.startswith("www.") else host
597+
598+
current = site_key(self._config.endpoint or DEFAULT_ENDPOINT)
599+
for candidate in (DEFAULT_ENDPOINT, DEFAULT_INTL_ENDPOINT):
600+
if site_key(candidate) != current:
601+
return candidate
602+
return None
603+
604+
@staticmethod
605+
def _token_valid_on(token: str, endpoint: str) -> bool:
606+
"""Best-effort check of whether *token* authenticates against *endpoint*.
607+
608+
Runs on the failure path only and is strictly advisory: any error means
609+
"cannot confirm", so a probe outage degrades to the plain server message
610+
rather than producing a misleading hint. Retries are disabled to keep
611+
the failure path responsive.
612+
"""
613+
from .constants import API_CONNECT_TIMEOUT
614+
615+
probe = LegacyClient(
616+
token=None,
617+
endpoint=endpoint,
618+
timeout=API_CONNECT_TIMEOUT,
619+
max_retries=0,
620+
)
621+
try:
622+
probe.login(token)
623+
except Exception: # advisory only -- never mask the original failure
624+
return False
625+
return True
626+
527627
def logout(self) -> None:
528628
"""Clear the locally persisted token.
529629

src/modelscope_hub/cli/main.py

Lines changed: 45 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ def _build_parser() -> argparse.ArgumentParser:
8989
"-v",
9090
"--verbose",
9191
action="store_true",
92-
help="Enable verbose (DEBUG) logging.",
92+
help="Enable DEBUG logging and print the full error cause chain.",
9393
)
9494

9595
subparsers = parser.add_subparsers(dest="command", metavar="COMMAND")
@@ -191,6 +191,45 @@ def _discover_plugins(subparsers) -> None:
191191
logging.getLogger(__name__).debug("Failed to load CLI plugin %r: %s", ep.name, exc)
192192

193193

194+
# ---------------------------------------------------------------------------
195+
# Error reporting
196+
# ---------------------------------------------------------------------------
197+
def _next_cause(exc: BaseException) -> BaseException | None:
198+
"""Return what *exc* was raised from, honouring ``raise ... from None``."""
199+
if exc.__cause__ is not None:
200+
return exc.__cause__
201+
if exc.__suppress_context__:
202+
return None
203+
return exc.__context__
204+
205+
206+
def _report_hub_error(exc: HubError, *, verbose: bool, max_depth: int = 5) -> None:
207+
"""Print a structured report for an SDK error.
208+
209+
``str(exc)`` already carries the error code, HTTP status, request id and --
210+
for API errors -- the request/response detail. Verbose mode additionally
211+
unwinds the cause chain: wrapping an exception is convenient for callers but
212+
otherwise hides the originating failure from whoever has to diagnose it.
213+
214+
The walk is bounded by *max_depth* and skips exceptions already visited, so
215+
a self-referential chain cannot stall the error path.
216+
"""
217+
error(str(exc))
218+
if exc.suggestion and exc.error_code != "E9001":
219+
info(f"Suggestion: {exc.suggestion}")
220+
if not verbose:
221+
return
222+
223+
seen = {id(exc)}
224+
cause = _next_cause(exc)
225+
depth = 1
226+
while cause is not None and id(cause) not in seen and depth <= max_depth:
227+
info(f"{' ' * depth}Caused by: {cause.__class__.__name__}: {cause}")
228+
seen.add(id(cause))
229+
cause = _next_cause(cause)
230+
depth += 1
231+
232+
194233
# ---------------------------------------------------------------------------
195234
# Entry point
196235
# ---------------------------------------------------------------------------
@@ -200,8 +239,9 @@ def run_cmd(argv: Sequence[str] | None = None) -> int:
200239
parser = _build_parser()
201240
args = parser.parse_args(argv)
202241

242+
verbose = bool(getattr(args, "verbose", False))
203243
logging.basicConfig(
204-
level=logging.DEBUG if getattr(args, "verbose", False) else logging.INFO,
244+
level=logging.DEBUG if verbose else logging.INFO,
205245
format="%(levelname)s %(name)s: %(message)s",
206246
)
207247

@@ -218,14 +258,10 @@ def run_cmd(argv: Sequence[str] | None = None) -> int:
218258
except SystemExit as exc: # honour explicit SystemExit from subcommands
219259
return int(exc.code) if isinstance(exc.code, int) else (0 if exc.code is None else 1)
220260
except (InvalidParameter, NotSupportedError) as exc:
221-
error(str(exc))
222-
if exc.suggestion:
223-
info(f"Suggestion: {exc.suggestion}")
261+
_report_hub_error(exc, verbose=verbose)
224262
return 2
225263
except HubError as exc:
226-
error(str(exc))
227-
if exc.suggestion and exc.error_code != "E9001":
228-
info(f"Suggestion: {exc.suggestion}")
264+
_report_hub_error(exc, verbose=verbose)
229265
return 1
230266
except ValueError as exc:
231267
error(str(exc))
@@ -235,7 +271,7 @@ def run_cmd(argv: Sequence[str] | None = None) -> int:
235271
return 2
236272
except Exception as exc: # pragma: no cover - unexpected
237273
error(f"Unexpected error: {exc.__class__.__name__}: {exc}")
238-
if getattr(args, "verbose", False):
274+
if verbose:
239275
raise
240276
return 1
241277

src/modelscope_hub/config.py

Lines changed: 39 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,14 @@
3838
ENV_TOKEN = "MODELSCOPE_API_TOKEN"
3939
ENV_HOME = "MODELSCOPE_HOME"
4040

41+
# Files that together constitute a persisted login. ``session`` is deliberately
42+
# excluded: it is an anonymous SDK install identifier, not a credential.
43+
_CREDENTIAL_FILE_NAMES: tuple[str, ...] = (
44+
COOKIES_FILE_NAME,
45+
GIT_TOKEN_FILE_NAME,
46+
USER_INFO_FILE_NAME,
47+
)
48+
4149

4250
def _expand(path: str | os.PathLike[str]) -> Path:
4351
return Path(path).expanduser().resolve()
@@ -85,10 +93,7 @@ def __post_init__(self) -> None:
8593
self._endpoint_overridden = True
8694
else:
8795
self.endpoint = DEFAULT_ENDPOINT
88-
# Ensure endpoint always has a scheme
89-
if self.endpoint and not self.endpoint.startswith(("http://", "https://")):
90-
self.endpoint = f"https://{self.endpoint}"
91-
self.endpoint = (self.endpoint or DEFAULT_ENDPOINT).rstrip("/")
96+
self.endpoint = self.normalize_endpoint(self.endpoint)
9297
# Token precedence: explicit arg > MODELSCOPE_API_TOKEN env var >
9398
# persisted credential. An explicitly provided value wins even when
9499
# empty ("" means "use no token"), so an explicit override never
@@ -101,6 +106,25 @@ def __post_init__(self) -> None:
101106
else:
102107
self.token = self.load_token()
103108

109+
@staticmethod
110+
def normalize_endpoint(endpoint: str | None) -> str:
111+
"""Return *endpoint* with a scheme guaranteed and no trailing slash.
112+
113+
Bare domains such as ``modelscope.ai`` are common input, especially from
114+
the CLI. Without a scheme every request built from them fails deep in
115+
the transport layer instead of surfacing a usable error, so the
116+
normalisation lives here and is reused by every entry point that
117+
accepts an endpoint.
118+
119+
Scheme detection is case-insensitive because URI schemes are, so an
120+
input like ``HTTPS://host`` is recognised instead of being prefixed a
121+
second time.
122+
"""
123+
value = (endpoint or "").strip() or DEFAULT_ENDPOINT
124+
if not value.lower().startswith(("http://", "https://")):
125+
value = f"https://{value}"
126+
return value.rstrip("/")
127+
104128
# ------------------------------------------------------------------
105129
# Path helpers
106130
# ------------------------------------------------------------------
@@ -186,14 +210,19 @@ def load_token(self) -> str | None:
186210
return None
187211

188212
def clear_token(self) -> None:
189-
"""Remove persisted credentials (deletes ``credentials/cookies``)."""
213+
"""Remove every persisted credential artefact.
214+
215+
All login artefacts are dropped together. Removing only the session
216+
cookie would leave the git token and the cached identity behind, a
217+
half-logged-out state that later reads can still pick up.
218+
"""
190219
self.token = None
191220
self._logged_out = True
192-
path = self.credentials_dir / COOKIES_FILE_NAME
193-
try:
194-
path.unlink(missing_ok=True)
195-
except OSError:
196-
pass
221+
for name in _CREDENTIAL_FILE_NAMES:
222+
try:
223+
(self.credentials_dir / name).unlink(missing_ok=True)
224+
except OSError:
225+
pass
197226

198227
# ------------------------------------------------------------------
199228
# Credentials persistence (compat with old modelscope SDK)

0 commit comments

Comments
 (0)