engflow_auth: Add export subcommand
#17
Conversation
minor-fixes
force-pushed
the
scott/CUS-367
branch
from
474ef21
to
c18ce44
Compare
cmd/engflow_auth/main.go
Outdated
| ctx := cliCtx.Context | ||
|
|
||
| if cliCtx.NArg() != 1 { | ||
| return autherr.CodedErrorf(autherr.CodeBadParams, "expected exactly 1 positional argument; got %d positional arguments", cliCtx.NArg()) |
There was a problem hiding this comment.
Nit: give the user a hint on what they should pass here.
| return autherr.CodedErrorf(autherr.CodeBadParams, "expected exactly 1 positional argument; got %d positional arguments", cliCtx.NArg()) | |
| return autherr.CodedErrorf(autherr.CodeBadParams, "expected exactly 1 positional argument, a cluster host name; got %d", cliCtx.NArg()) |
There was a problem hiding this comment.
Not seeing the difference in this commit. Was it added?
There was a problem hiding this comment.
No, looks like I got bamboozled by VSCode - fixed
cmd/engflow_auth/main.go
Outdated
|
|
||
| token, err := r.tokenStore.Load(ctx, clusterURL.Host) | ||
| if err != nil { | ||
| if reauthErr := new(autherr.CodedError); errors.As(err, &reauthErr) && reauthErr.Code == autherr.CodeReauthRequired { |
There was a problem hiding this comment.
No point in allocating a new value with new then replacing with errors.As. Use nil instead.
| if reauthErr := new(autherr.CodedError); errors.As(err, &reauthErr) && reauthErr.Code == autherr.CodeReauthRequired { | |
| if reauthErr := (*autherr.CodedError)(nil); errors.As(err, &reauthErr) && reauthErr.Code == autherr.CodeReauthRequired { |
cmd/engflow_auth/main.go
Outdated
| return autherr.ReauthRequired(clusterURL.Host) | ||
| } | ||
|
|
||
| if _, err := fmt.Fprintf(cliCtx.App.Writer, "%s\n", token.AccessToken); err != nil { |
There was a problem hiding this comment.
This is a serialization format, so let's think about this carefully. It's quite likely that whatever's printed here will be read and written by different version of this program. If we only print the token, there's no way to extend the format and include more information.
I'd suggest instead that we stick this in a JSON structure and comment not to make any backward-incompatible changes. That will let us add new fields later on if we need to.
An example of something that might be nice: we could include the cluster URL so there's no need to specify it in the corresponding import command.
There was a problem hiding this comment.
Right - the token really has an affinity to the cluster URL as well.
Does it also include the expiry time too?
There was a problem hiding this comment.
Done; added tests to make this more clear
minor-fixes
force-pushed
the
scott/CUS-367
branch
from
c18ce44
to
8a3868f
Compare
cmd/engflow_auth/main.go
Outdated
| ctx := cliCtx.Context | ||
|
|
||
| if cliCtx.NArg() != 1 { | ||
| return autherr.CodedErrorf(autherr.CodeBadParams, "expected exactly 1 positional argument; got %d positional arguments", cliCtx.NArg()) |
There was a problem hiding this comment.
Not seeing the difference in this commit. Was it added?
This change adds an `export` subcommand which prints the `(*oauth2.Token).AccessToken` field for the specified cluster to stdout, when: * a token is stored for the specified cluster * the token is not expired This isn't any less secure than using `engflow_auth get`, which is another variation on the same intent; since the token storage currently used is backed by the OS keychain, only the current user should be able to fetch tokens they've stored. Tested: unit tests added Bug: linear/CUS-367
minor-fixes
force-pushed
the
scott/CUS-367
branch
from
8a3868f
to
5507d13
Compare
|
A gentle request for easier reviews: please don't force-push after someone has reviewed. It makes it difficult to see what has changed between reviews. Normally, I can click Show changes since your last review, but after a force-push, that doesn't work because the old commits are gone. If you need to update the branch, We squash and merge everything, so the end result of all these is the same. It's just a convenience for reviewers. |
This change adds an
exportsubcommand which prints the(*oauth2.Token).AccessTokenfield for the specified cluster to stdout, when:This isn't any less secure than using
engflow_auth get, which is another variation on the same intent; since the token storage currently used is backed by the OS keychain, only the current user should be able to fetch tokens they've stored.Tested: unit tests added
Bug: linear/CUS-367