Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error handling for invalid tokens during login #2575

Closed

Conversation

WizKnight
Copy link
Contributor

@WizKnight WizKnight commented Sep 27, 2024

This PR enhances the error handling during the login process to provide users with more informative error messages when they attempt to log in with an invalid token. This addresses issue #2565 .

Key Changes:

Modified get_token_permission :

  • The get_token_permission function now directly raises a ValueError if there is an HTTPError during the whoami call. This allows the original error message from the server to be included, giving users more context about the problem (e.g., "Invalid or expired token").
  • The LocalTokenNotFoundError is no longer explicitly caught, as it's essentially an invalid token scenario, and the HTTPError from whoami will handle it.

Updated _login.py:

  • The _login function continues to use get_token_permission to check token validity.
  • If get_token_permission raises a ValueError, the error is sent to the user, providing them with the more informative server error message.

@WizKnight
Copy link
Contributor Author

Hi @Wauplin :), changes have been implemented in this PR.
Please take a look and let me know if you have any further feedback or modifications!

Thanks!

@Wauplin
Copy link
Contributor

Wauplin commented Sep 30, 2024

Hi @WizKnight, actually @hanouticelina is working on a refacto of the login process to be able to set several tokens on a user's machine. This will affect this section of the code (see #2549 (comment)). I'm sorry if I have been unclear in #2565. The goal was not to change get_token_permission but to change how it is used in _login.py.

Thanks again for your work but I think for this one it's best to close the PR without merging. Sorry about that!

@WizKnight
Copy link
Contributor Author

Hi @WizKnight, actually @hanouticelina is working on a refacto of the login process to be able to set several tokens on a user's machine. This will affect this section of the code (see #2549 (comment)). I'm sorry if I have been unclear in #2565. The goal was not to change get_token_permission but to change how it is used in _login.py.

Thanks again for your work but I think for this one it's best to close the PR without merging. Sorry about that!

Hi @Wauplin,
It's alright, thanks for clarifying this. Please go ahead and close this PR without merging.

And I'd appreciate, if you could assign me another task once this is done.

Thanks

@Wauplin
Copy link
Contributor

Wauplin commented Oct 1, 2024

Hi @WizKnight, thanks for your understanding. Regarding assigning another task, I don't have a good one in mind at the moment. We will have to set back a bit and think about the next steps. In any case, thanks for your support!

@Wauplin Wauplin closed this Oct 1, 2024
@WizKnight
Copy link
Contributor Author

Hi @Wauplin, please do let me know if you need any assistance with upcoming tasks or any existing issues.
I'm happy to contribute🤗

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants