Offer a fresh login when saved credentials stop working - #1029
Open
sjbrownrigg wants to merge 1 commit into
Open
Offer a fresh login when saved credentials stop working#1029sjbrownrigg wants to merge 1 commit into
sjbrownrigg wants to merge 1 commit into
Conversation
has_creds() only checks that something is stored, not that it still works. An expired token is still a non-empty string, so the check passes, login() is attempted, and the failure surfaced as a bare traceback -- even though the prompter that fixes it is already built and reached in the has_creds() == False branch a few lines above. get_logged_in_client now catches AuthenticationError and MissingCredentialsError from login() and offers to re-authorise. Qobuz and Deezer already raise those types, so the only change needed to cover all three sources was typing Tidal's two bare Exceptions -- 'Access token not found in config.' and 'Refresh failed', the latter being what you get when the refresh token itself lapses. Declining re-raises the original error, so nothing is swallowed. Non-interactive runs never prompt. rip is used from cron and from scripts, where a hidden y/n hangs forever instead of failing, so without a tty it raises with an explanation of what to do. This follows the isatty guard already used for artist album selection. Tests cover all three paths and fail without the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
has_creds()only checks that a credential is stored, not that it still works:An expired token is still a non-empty string. So the check passes,
login()isattempted, and the failure surfaces as a bare traceback — even though the
prompter that would fix it is already built, and is reached in the
has_creds() == Falsebranch a few lines above.For Tidal the two failure points were untyped, so nothing could catch them
precisely:
The second is the common one. Tidal access tokens are refreshed automatically,
but the refresh token eventually expires too, and when it does there is
nothing left to refresh from. Today that ends in:
Change
get_logged_in_client()catchesAuthenticationErrorandMissingCredentialsErrorfromlogin()and offers a fresh login.Qobuz and Deezer already raise those types, so the only change needed to cover
all three sources was giving Tidal's two bare
Exceptions the same types.Declining re-raises the original error, so nothing is swallowed.
Non-interactive runs never prompt.
ripis used from cron and fromscripts, where a hidden y/n hangs forever instead of failing. Without a tty it
raises with an explanation instead, following the
sys.stdin.isatty()guardalready used for artist album selection.
Relationship to #955
#955 by @mikelandzelo173 got here first and modifies the same lines,
wrapping
await client.login()intry/except AuthenticationError. Itshandler is deliberately Qobuz-only:
That PR's real subject is the new Qobuz token flow; this one takes the same
idea and generalises it — every source, both credential exception types, and a
guard for non-interactive use.
These will conflict. They should not both be merged as-is. If #955 lands
first I am happy to rebase this on top and reduce it to the generalisation, or
to close it if the Qobuz-specific form is preferred. Flagging it rather than
letting a merge conflict be the discovery.
Testing
tests/test_reauth_prompt.pycovers three paths — accept, decline, and notty — and each fails without this change.
Full suite: 62 passed, 7 skipped.
tests/test_meta.py::test_album_metadata_qobuzfails, but it fails identically on unmodified
dev.