From efa74be71b5f7de6f923727c2b3e8e1de3c9eea6 Mon Sep 17 00:00:00 2001 From: Pawel Kuc Date: Fri, 13 Dec 2024 18:28:19 +0100 Subject: [PATCH] feat: nctl auth login in non-interactive env and without set token should not block If you execute `nctl auth login` in a non-interactive environment (e.g. CI/CD) and forget to set `--api-token`, the browser should not try to open - you should see an error message instead. --- auth/login.go | 18 ++++-- auth/login_test.go | 122 +++++++++++++++++++++++++++------------ internal/format/print.go | 14 +++-- 3 files changed, 106 insertions(+), 48 deletions(-) diff --git a/auth/login.go b/auth/login.go index 65e156b..5666ed3 100644 --- a/auth/login.go +++ b/auth/login.go @@ -2,6 +2,7 @@ package auth import ( "context" + "errors" "fmt" "net/url" "os" @@ -17,13 +18,16 @@ import ( ) type LoginCmd struct { - APIURL string `help:"The URL of the Nine API" default:"https://nineapis.ch" env:"NCTL_API_URL" name:"api-url"` - APIToken string `help:"Use a static API token instead of using an OIDC login. You need to specify the --organization parameter as well." env:"NCTL_API_TOKEN"` - Organization string `help:"The name of your organization to use when providing an API token. This parameter is only used when providing a API token. This parameter needs to be set if you use --api-token." env:"NCTL_ORGANIZATION"` - IssuerURL string `help:"Issuer URL is the OIDC issuer URL of the API." default:"https://auth.nine.ch/auth/realms/pub"` - ClientID string `help:"Client ID is the OIDC client ID of the API." default:"nineapis.ch-f178254"` + APIURL string `help:"The URL of the Nine API" default:"https://nineapis.ch" env:"NCTL_API_URL" name:"api-url"` + APIToken string `help:"Use a static API token instead of using an OIDC login. You need to specify the --organization parameter as well." env:"NCTL_API_TOKEN"` + Organization string `help:"The name of your organization to use when providing an API token. This parameter is only used when providing a API token. This parameter needs to be set if you use --api-token." env:"NCTL_ORGANIZATION"` + IssuerURL string `help:"Issuer URL is the OIDC issuer URL of the API." default:"https://auth.nine.ch/auth/realms/pub"` + ClientID string `help:"Client ID is the OIDC client ID of the API." default:"nineapis.ch-f178254"` + ForceInteractiveEnvOverride bool `help:"Used for internal purposes only. Set to true to force interactive environment explicit override. Set to false to fall back to automatic interactivity detection." default:"false" hidden:""` } +const ErrNonInteractiveEnvironmentEmptyToken = "a static API token is required in non-interactive environments" + func (l *LoginCmd) Run(ctx context.Context, command string, tk api.TokenGetter) error { apiURL, err := url.Parse(l.APIURL) if err != nil { @@ -58,6 +62,10 @@ func (l *LoginCmd) Run(ctx context.Context, command string, tk api.TokenGetter) return login(ctx, cfg, loadingRules.GetDefaultFilename(), userInfo.User, "", project(l.Organization)) } + if !l.ForceInteractiveEnvOverride && !format.IsInteractiveEnvironment(os.Stdout) { + return errors.New(ErrNonInteractiveEnvironmentEmptyToken) + } + usePKCE := true token, err := tk.GetTokenString(ctx, l.IssuerURL, l.ClientID, usePKCE) diff --git a/auth/login_test.go b/auth/login_test.go index 273381d..14d409a 100644 --- a/auth/login_test.go +++ b/auth/login_test.go @@ -8,7 +8,9 @@ import ( "path" "testing" + "github.com/ninech/nctl/api" "github.com/ninech/nctl/internal/test" + "github.com/stretchr/testify/require" "k8s.io/client-go/tools/clientcmd" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" ) @@ -19,6 +21,16 @@ func (f *fakeTokenGetter) GetTokenString(ctx context.Context, issuerURL, clientI return test.FakeJWTToken, nil } +func checkErrorRequire(t *testing.T, err error, expectError bool, expectedErrMsg string) { + t.Helper() + if expectError { + require.Error(t, err, "expected an error but got none") + require.EqualError(t, err, expectedErrMsg, "unexpected error message") + } else { + require.NoError(t, err, "expected no error but got one") + } +} + func TestLoginCmd(t *testing.T) { // write our "existing" kubeconfig to a temp kubeconfig kubeconfig, err := os.CreateTemp("", "*-kubeconfig.yaml") @@ -36,8 +48,9 @@ func TestLoginCmd(t *testing.T) { apiHost := "api.example.org" tk := &fakeTokenGetter{} cmd := &LoginCmd{ - APIURL: "https://" + apiHost, - IssuerURL: "https://auth.example.org", + APIURL: "https://" + apiHost, + IssuerURL: "https://auth.example.org", + ForceInteractiveEnvOverride: true, } if err := cmd.Run(context.Background(), "", tk); err != nil { t.Fatal(err) @@ -58,42 +71,74 @@ func TestLoginCmd(t *testing.T) { } func TestLoginStaticToken(t *testing.T) { - kubeconfig, err := os.CreateTemp("", "*-kubeconfig.yaml") - if err != nil { - log.Fatal(err) - } - defer os.Remove(kubeconfig.Name()) - - os.Setenv(clientcmd.RecommendedConfigPathEnvVar, kubeconfig.Name()) - apiHost := "api.example.org" - token := test.FakeJWTToken - - cmd := &LoginCmd{APIURL: "https://" + apiHost, APIToken: token, Organization: "test"} - tk := &fakeTokenGetter{} - if err := cmd.Run(context.Background(), "", tk); err != nil { - t.Fatal(err) - } - - // read out the kubeconfig again to test the contents - b, err := io.ReadAll(kubeconfig) - if err != nil { - t.Fatal(err) - } - - kc, err := clientcmd.Load(b) - if err != nil { - t.Fatal(err) - } - - checkConfig(t, kc, 1, "") - - if token != kc.AuthInfos[apiHost].Token { - t.Fatalf("expected token to be %s, got %s", token, kc.AuthInfos[apiHost].Token) - } - if kc.AuthInfos[apiHost].Exec != nil { - t.Fatalf("expected execConfig to be empty, got %v", kc.AuthInfos[apiHost].Exec) + tests := []struct { + name string + cmd *LoginCmd + tk api.TokenGetter + wantToken string + wantErr bool + wantErrMessage string + }{ + { + name: "interactive environment with token", + cmd: &LoginCmd{APIURL: "https://" + apiHost, APIToken: test.FakeJWTToken, Organization: "test", ForceInteractiveEnvOverride: true}, + tk: &fakeTokenGetter{}, + wantToken: test.FakeJWTToken, + }, + { + name: "non-interactive environment with token", + cmd: &LoginCmd{APIURL: "https://" + apiHost, APIToken: test.FakeJWTToken, Organization: "test", ForceInteractiveEnvOverride: false}, + tk: &fakeTokenGetter{}, + wantToken: test.FakeJWTToken, + }, + { + name: "non-interactive environment with empty token", + cmd: &LoginCmd{APIURL: "https://" + apiHost, APIToken: "", Organization: "test", ForceInteractiveEnvOverride: false}, + wantErr: true, + wantErrMessage: ErrNonInteractiveEnvironmentEmptyToken, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + kubeconfig, err := os.CreateTemp("", "*-kubeconfig.yaml") + if err != nil { + log.Fatal(err) + } + defer os.Remove(kubeconfig.Name()) + os.Setenv(clientcmd.RecommendedConfigPathEnvVar, kubeconfig.Name()) + + err = tt.cmd.Run(context.Background(), "", tt.tk) + checkErrorRequire(t, err, tt.wantErr, tt.wantErrMessage) + + if tt.wantErr { + return + } + + // read out the kubeconfig again to test the contents + b, err := io.ReadAll(kubeconfig) + if err != nil { + t.Fatal(err) + } + + kc, err := clientcmd.Load(b) + if err != nil { + t.Fatal(err) + } + + checkConfig(t, kc, 1, "") + + if tt.wantToken != kc.AuthInfos[apiHost].Token { + t.Fatalf("expected token to be %s, got %s", tt.wantToken, kc.AuthInfos[apiHost].Token) + } + + if kc.AuthInfos[apiHost].Exec != nil { + t.Fatalf("expected execConfig to be empty, got %v", kc.AuthInfos[apiHost].Exec) + } + }) } } @@ -109,8 +154,9 @@ func TestLoginCmdWithoutExistingKubeconfig(t *testing.T) { apiHost := "api.example.org" cmd := &LoginCmd{ - APIURL: "https://" + apiHost, - IssuerURL: "https://auth.example.org", + APIURL: "https://" + apiHost, + IssuerURL: "https://auth.example.org", + ForceInteractiveEnvOverride: true, } tk := &fakeTokenGetter{} if err := cmd.Run(context.Background(), "", tk); err != nil { diff --git a/internal/format/print.go b/internal/format/print.go index 93b8721..af6befc 100644 --- a/internal/format/print.go +++ b/internal/format/print.go @@ -193,11 +193,7 @@ func getPrinter(out io.Writer) (printer.Printer, error) { p := printer.Printer{ LineNumber: false, } - f, isFile := out.(*os.File) - if !isFile { - return p, nil - } - if isatty.IsTerminal(f.Fd()) || isatty.IsCygwinTerminal(f.Fd()) { + if IsInteractiveEnvironment(out) { p.Bool = printerProperty(&printer.Property{ Prefix: format(color.FgHiMagenta), Suffix: format(color.Reset), @@ -218,6 +214,14 @@ func getPrinter(out io.Writer) (printer.Printer, error) { return p, nil } +func IsInteractiveEnvironment(out io.Writer) bool { + f, isFile := out.(*os.File) + if !isFile { + return false + } + return isatty.IsTerminal(f.Fd()) || isatty.IsCygwinTerminal(f.Fd()) +} + // stripObj removes some fields which simply add clutter to the yaml output. // The object should still be applyable afterwards as just defaults and // computed fields get removed.