tursodatabase / turso-cli

Command line interface to Turso.
https://turso.tech
MIT License
220 stars 37 forks source link

Any time a 401 error occurs, show suggestion to log in with `turso auth login` #154

Closed CodingDoug closed 1 year ago

CodingDoug commented 1 year ago

Here is are a couple examples that shows less helpful error messages:

$ turso db show d1
Error: failed to get database listing: 401 Unauthorized
$ turso db create foo
Error: could not create database foo: Authentication required
CodingDoug commented 1 year ago

We still have a lot of inconsistency with error messaging when a token is expired. Could we survey all of the places where auth errors can occur and make them consistent, suggesting a login command at every opportunity? IMO "410 Unauthorized" should never be displayed.

$ turso --version
turso version v0.55.1

$ turso account show
Error: failed to get database listing: 401 Unauthorized

$ turso db list
Error: failed to get database listing: 401 Unauthorized

$ turso db show my-db
Error: user not logged in, please login with turso auth login

$ turso db locations
Error: unable to fetch locations: 401 Unauthorized

$ turso db shell my-db
Error: could not create turso client: user not logged in, please login with turso auth login

$ turso db replicate my-db sjc
Error: failed to get database listing: 401 Unauthorized

$ turso db inspect my-db
Error: user not logged in, please login with turso auth login

$ turso db create foo
Error: could not create database foo: Authentication required

$ turso db destroy my-db
Error: user not logged in, please login with turso auth login

$ turso db tokens create my-db -e none
Error: user not logged in, please login with turso auth login

$ turso db update my-db
Error: user not logged in, please login with turso auth login
haaawk commented 1 year ago

This is potentially a regression after https://github.com/chiselstrike/turso-cli/pull/332

penberg commented 1 year ago

@CodingDoug There are two different cases: (1) when you have not logged in (2) when you have logged in but are unauthorized to access something. The error message for the two should be different. What we need to do is check if we have a valid access token at the beginning of every command and if not, suggest login. For everything else, we should just propagate the "unauthorized" error back to the user (but probably not show the 404).

CodingDoug commented 1 year ago

@penberg FWIW the above set of examples shows the results while using an expired token, which is a normal, expected case. The error messaging should be more specific for that case - we don't want to user to think they've done something wrong, we just want them to do the usual ceremony to correct it. This leads there to be a few distinct cases:

Case Logged in? Result
No token no they can do nothing and need to log in first
Valid token, permission OK yes token validates and they are allowed to perform the action
Valid token, no permission yes token validates but they are not allowed to perform the action
Valid token, expired no token validates but current time > expiration; user must log in again; this check must be on the server because we can't trust the client's clock
Invalid token no something else about the token is not valid, something is very wrong (data corruption or security attack)

The "Logged in?" column is speculative on my part - what cases should we consider the user to be "logged in" to the CLI? I would argue that any condition that requires the user to go through the login process again formally considers the user to be logged out. The difference with expired tokens is that we can yield specific error messaging about what the problem is, so the user doesn't think that something has gone wrong.

I'll note that only the backend can safely determine each of these cases where a token is present, and it needs to send enough information to the client so that it can output a helpful message (either through error codes or constructing the actual error message).

The output for correcting permission problems might need very special considerations if we intend to propose a solution to specific problem rather than just say "unauthorized".

StefanoSaffran commented 1 year ago

I took a look at this and it seems to me that in the current version, the error messages are more consistent. The message is the same for no token or expired token.

$ turso --version
turso version v0.61.2

$ turso account show
Error: user not logged in, please login with turso auth login

$ turso db list
Error: user not logged in, please login with turso auth login

$ turso db show my-db
Error: user not logged in, please login with turso auth login

$ turso db locations
Error: user not logged in, please login with turso auth login

$ turso db shell my-db
Error: could not create turso client: user not logged in, please login with turso auth login

$ turso db replicate my-db sjc
Error: user not logged in, please login with turso auth login

$ turso db inspect my-db
Error: user not logged in, please login with turso auth login

$ turso db create foo
Error: user not logged in, please login with turso auth login

$ turso db destroy my-db
Error: user not logged in, please login with turso auth login

$ turso db tokens create my-db -e none
Error: user not logged in, please login with turso auth login

$ turso db update my-db
Error: user not logged in, please login with turso auth login

@CodingDoug do we need anything else here?

CodingDoug commented 1 year ago

@StefanoSaffran That looks a lot better! Thanks. This is fine for now, and we can revisit later if something needs to be improved.