tursodatabase / turso-cli

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

Clarity about the source of auth token used #271

Closed CodingDoug closed 1 year ago

CodingDoug commented 1 year ago

Right now, we have three ways of finding an auth token:

Since all three may be present, it's potentially confusing since the CLI is not very clear about which one of these it's using when a command runs successfully. Also, if the CLI fails because the chosen token is expired or invalid, there is no feedback about which one was problematic, leaving the user to potentially take action that doesn't resolve the issue.

Proposal:

luisfvieirasilva commented 1 year ago

@CodingDoug, what do you think about just send the warning message if the token fails? IMHO, printing a warn every time (even if the found token is valid) would be a little bit annoying to an user that knows what is doing. We could be more direct when a token fails. For instance, if the TURSO_API_TOKEN env token is expired we could say something like Error: no user logged in. Update the token at TURSO_API_TOKEN variable or unset it to use turso default configuration token (ps: this message would need improvements but I hope it works as an example)

athoscouto commented 1 year ago

Any chance we could drop the -t flag? It can get confusing, @dyasny had a problem with it here, for instance.

I don't think it offers much value since we already support the env var.

CodingDoug commented 1 year ago

Let's think about this from specific use cases that we expect people to do.

-t flag

I can't think of anything that absolutely requires the use of the CLI flag to specify the token. Chances are seem real good that if you can populate the CLI args then you can also populate the env. The one place where I feel it might be more convenient to use the CLI args is when someone is scripting the operation of the CLI in a program that is not a shell script. Let's drop it if we can't come up with a distinct use case.

If we drop it, then there is only settings.json and env, and env can take precendence.

warnings

If we back up for a moment and think about this in terms of use case, what is the real scenario where someone needs to be both signed in to the CLI (token in settings) and then intentionally override that with env at the same time? I can't come up with a good one, and I'm thinking that having both is more likely to be a mistake or misunderstanding on the part of the user, which makes it a good condition to continually warn about.

The error message you're proposing @luisfvieirasilva makes sense to me only if we are able to get more information about what caused the token to fail auth. We have at least 3 different reasons:

  1. can't decode - completely invalid/malformed
  2. Expired
  3. No permission for the subject (specific user not allowed to access)

Right now I think the backend is just giving a 401 for all failures, which doesn't work well for providing actionable error messages. All we are really saying in the response is "you can't do this" without giving the reason. So if the failure was item 3 above, then the warning message you're proposing to update the token isn't going to work for the user at all.

Maybe the CLI can help by decoding the token before sending it over, which helps build more accurate and actionable messages without backend support.

StefanoSaffran commented 1 year ago

Hi guys, I am opening a PR #363 trying to solve this one, I am removing the flag and adding an option to show a warning if there are multiple token sources.
But I am not sure if it covers #270, so I did not add it in the PR.

haaawk commented 1 year ago

Hi guys, I am opening a PR #363 trying to solve this one, I am removing the flag and adding an option to show a warning if there are multiple token sources. But I am not sure if it covers #270, so I did not add it in the PR.

I think for #270 we're still missing:

If there is a token in env, all auth commands should fail saying that auth commands aren't effective when a token is in env.