influxdata / influx-cli

CLI for managing resources in InfluxDB v2
MIT License
61 stars 23 forks source link

Replace token flags with = to prevent bad parsing of leading dash in token #399

Closed candrewlee14 closed 2 years ago

candrewlee14 commented 2 years ago

This is a pretty awkward problem, and it stems from the fact that those wrapping quotes are parsed away when Go gets the program's arguments for os.Args.

I ran dlv debug ./cmd/influx -- config create -n targetflux -u "http://localhost:8087" -o "organization" -t "-therestofmytoken" And this is the os.Args value which urfave/cli uses directly.

(dlv) p os.Args
[]string len: 11, cap: 11, [
        "...+2 more",
        "config",
        "create",
        "-n",
        "targetflux",
        "-u",
        "http://localhost:8087",
        "-o",
        "organization",
        "-t",
        "-therestofmytoken",      <--- no wrapping quotes
]

It seems like a bespoke method specifically for replacing -t "-FOO-TOKEN" with -t=-FOO-TOKEN in os.Args before passing into urfave/cli makes the most sense here.

Closing #376 and #378

samhld commented 2 years ago

Will both options still be available from the user perspective?

candrewlee14 commented 2 years ago

Will both options still be available from the user perspective?

Yeah, it just rewrites the space-flag syntax to an equal-flag syntax for the token flag behind the scenes. User passes their arguments the same way.

lesam commented 2 years ago

The problem would be if people write something like:

./influx config create -n local_2 -u http://localhost:8086/ -t -a
Active  Name    URL                     Org
        local_2 http://localhost:8086/

The -a gets interpreted as a token when it was intended as an active marker, which is weird.

On the whole, I think this is better than the alterative though.

Maybe we could have a stderr print like warning: -t -a interpretted as -t=-a, consider using -t=-a syntax when tokens may start with a hyphen.

@samhld thoughts?

samhld commented 2 years ago

@lesam I think that works if this is the path we're taking. What was the last sentiment on just changing the token generation server-side to disallow this type of token. Do you think it's reasonable users would bring their own token starting with a hyphen? I suppose we'd have to support that even if it's an edge case, if we're going to support "bring your own token" at all.

lesam commented 2 years ago

Yeah I think we're stuck dealing with existing tokens that have a hyphen.