From c2c8d92cae3fd4a15e20c90a1369e97c0c46ced2 Mon Sep 17 00:00:00 2001 From: reggie-k Date: Fri, 17 Jan 2025 23:00:21 +0200 Subject: [PATCH] bearer token only for git repos Signed-off-by: reggie-k --- cmd/argocd/commands/admin/repo.go | 11 +- cmd/argocd/commands/admin/repo_test.go | 82 -------- cmd/argocd/commands/repo.go | 11 +- cmd/argocd/commands/repo_test.go | 82 -------- cmd/argocd/commands/repocreds.go | 16 +- cmd/argocd/commands/repocreds_test.go | 82 -------- cmd/util/common.go | 32 ++++ cmd/util/common_test.go | 179 ++++++++++++++++++ .../components/repos-list/repos-list.tsx | 24 ++- ui/src/app/shared/models.ts | 1 + .../app/shared/services/repocreds-service.ts | 1 + 11 files changed, 235 insertions(+), 286 deletions(-) delete mode 100644 cmd/argocd/commands/admin/repo_test.go delete mode 100644 cmd/argocd/commands/repo_test.go delete mode 100644 cmd/argocd/commands/repocreds_test.go create mode 100644 cmd/util/common_test.go diff --git a/cmd/argocd/commands/admin/repo.go b/cmd/argocd/commands/admin/repo.go index 7b3962f96a15e..aec8b766f0e71 100644 --- a/cmd/argocd/commands/admin/repo.go +++ b/cmd/argocd/commands/admin/repo.go @@ -24,14 +24,6 @@ const ( repoSecretPrefix = "repo" ) -func validateBearerTokenAndPasswordCombo(bearerToken string, password string) { - // Either the password or the bearer token must be set, but not both - if bearerToken != "" && password != "" { - err := stderrors.New("only --bearer-token or --password is allowed, not both") - errors.CheckError(err) - } -} - func NewRepoCommand() *cobra.Command { command := &cobra.Command{ Use: "repo", @@ -148,7 +140,8 @@ func NewGenRepoSpecCommand() *cobra.Command { repoOpts.Repo.Password = cli.PromptPassword(repoOpts.Repo.Password) } - validateBearerTokenAndPasswordCombo(repoOpts.Repo.BearerToken, repoOpts.Repo.Password) + cmdutil.ValidateBearerTokenAndPasswordCombo(repoOpts.Repo.BearerToken, repoOpts.Repo.Password) + cmdutil.ValidateBearerTokenForGitOnly(repoOpts.Repo.BearerToken, repoOpts.Repo.Type) argoCDCM := &corev1.ConfigMap{ TypeMeta: metav1.TypeMeta{ diff --git a/cmd/argocd/commands/admin/repo_test.go b/cmd/argocd/commands/admin/repo_test.go deleted file mode 100644 index 650c3657a84fb..0000000000000 --- a/cmd/argocd/commands/admin/repo_test.go +++ /dev/null @@ -1,82 +0,0 @@ -package admin - -import ( - "bytes" - "errors" - "os" - "os/exec" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestValidateBearerTokenAndPasswordCombo(t *testing.T) { - tests := []struct { - name string - bearerToken string - password string - expectError bool - errorMsg string - }{ - { - name: "Both token and password set", - bearerToken: "some-token", - password: "some-password", - expectError: true, - errorMsg: "only --bearer-token or --password is allowed, not both", - }, - { - name: "Only token set", - bearerToken: "some-token", - password: "", - expectError: false, - }, - { - name: "Only password set", - bearerToken: "", - password: "some-password", - expectError: false, - }, - { - name: "Neither token nor password set", - bearerToken: "", - password: "", - expectError: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if tt.expectError { - // The command for "Invalid non-HTTPS with Bearer Token" case does not return an error. - // This is because the function validateBearerTokenAndPasswordCombo() does not return an error. - // Instead, the errors.CheckError(err) performs non-zero code system exit. - // So in order to test this case, we need to run the command in a separate process. - // https://stackoverflow.com/a/33404435 - // TODO: consider whether to change all the cmd commands to return an error instead of performing a non-zero code system exit. - // TODO: generalize this pattern to be used in other tests for failing cmd commands. - if os.Getenv("BE_CRASHER") == "1" { - validateBearerTokenAndPasswordCombo(tt.bearerToken, tt.password) - return - } - cmd := exec.Command(os.Args[0], "-test.run=TestValidateBearerTokenAndPasswordCombo") - cmd.Env = append(os.Environ(), "BE_CRASHER=1") - var stderr bytes.Buffer - cmd.Stderr = &stderr - err := cmd.Run() - var exitErr *exec.ExitError - if errors.As(err, &exitErr) && !exitErr.Success() { - assert.Contains(t, stderr.String(), tt.errorMsg) - return - } - t.Fatalf("process ran with err %v, want exit status 1", err) - } else { - // This will never actually panic because the function validateBearerTokenAndPasswordCombo() does not return - // an error. - assert.NotPanics(t, func() { - validateBearerTokenAndPasswordCombo(tt.bearerToken, tt.password) - }) - } - }) - } -} diff --git a/cmd/argocd/commands/repo.go b/cmd/argocd/commands/repo.go index df0f9e506f502..83f1f3d18227d 100644 --- a/cmd/argocd/commands/repo.go +++ b/cmd/argocd/commands/repo.go @@ -22,14 +22,6 @@ import ( "github.com/argoproj/argo-cd/v3/util/io" ) -func validateBearerTokenAndPasswordCombo(bearerToken string, password string) { - // Either the password or the bearer token must be set, but not both - if bearerToken != "" && password != "" { - err := stderrors.New("only --bearer-token or --password is allowed, not both") - errors.CheckError(err) - } -} - // NewRepoCommand returns a new instance of an `argocd repo` command func NewRepoCommand(clientOpts *argocdclient.ClientOptions) *cobra.Command { command := &cobra.Command{ @@ -204,7 +196,8 @@ func NewRepoAddCommand(clientOpts *argocdclient.ClientOptions) *cobra.Command { repoOpts.Repo.Password = cli.PromptPassword(repoOpts.Repo.Password) } - validateBearerTokenAndPasswordCombo(repoOpts.Repo.BearerToken, repoOpts.Repo.Password) + cmdutil.ValidateBearerTokenAndPasswordCombo(repoOpts.Repo.BearerToken, repoOpts.Repo.Password) + cmdutil.ValidateBearerTokenForGitOnly(repoOpts.Repo.BearerToken, repoOpts.Repo.Type) // We let the server check access to the repository before adding it. If // it is a private repo, but we cannot access with with the credentials diff --git a/cmd/argocd/commands/repo_test.go b/cmd/argocd/commands/repo_test.go deleted file mode 100644 index 6e60d8a64ac20..0000000000000 --- a/cmd/argocd/commands/repo_test.go +++ /dev/null @@ -1,82 +0,0 @@ -package commands - -import ( - "bytes" - "errors" - "os" - "os/exec" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestValidateBearerTokenAndPasswordCombo(t *testing.T) { - tests := []struct { - name string - bearerToken string - password string - expectError bool - errorMsg string - }{ - { - name: "Both token and password set", - bearerToken: "some-token", - password: "some-password", - expectError: true, - errorMsg: "only --bearer-token or --password is allowed, not both", - }, - { - name: "Only token set", - bearerToken: "some-token", - password: "", - expectError: false, - }, - { - name: "Only password set", - bearerToken: "", - password: "some-password", - expectError: false, - }, - { - name: "Neither token nor password set", - bearerToken: "", - password: "", - expectError: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if tt.expectError { - // The command for "Invalid non-HTTPS with Bearer Token" case does not return an error. - // This is because the function validateBearerTokenAndPasswordCombo() does not return an error. - // Instead, the errors.CheckError(err) performs non-zero code system exit. - // So in order to test this case, we need to run the command in a separate process. - // https://stackoverflow.com/a/33404435 - // TODO: consider whether to change all the cmd commands to return an error instead of performing a non-zero code system exit. - // TODO: generalize this pattern to be used in other tests for failing cmd commands. - if os.Getenv("BE_CRASHER") == "1" { - validateBearerTokenAndPasswordCombo(tt.bearerToken, tt.password) - return - } - cmd := exec.Command(os.Args[0], "-test.run=TestValidateBearerTokenAndPasswordCombo") - cmd.Env = append(os.Environ(), "BE_CRASHER=1") - var stderr bytes.Buffer - cmd.Stderr = &stderr - err := cmd.Run() - var exitErr *exec.ExitError - if errors.As(err, &exitErr) && !exitErr.Success() { - assert.Contains(t, stderr.String(), tt.errorMsg) - return - } - t.Fatalf("process ran with err %v, want exit status 1", err) - } else { - // This will never actually panic because the function validateBearerTokenAndPasswordCombo() does not return - // an error. - assert.NotPanics(t, func() { - validateBearerTokenAndPasswordCombo(tt.bearerToken, tt.password) - }) - } - }) - } -} diff --git a/cmd/argocd/commands/repocreds.go b/cmd/argocd/commands/repocreds.go index 168af6b1ec628..f84555de956f6 100644 --- a/cmd/argocd/commands/repocreds.go +++ b/cmd/argocd/commands/repocreds.go @@ -11,6 +11,7 @@ import ( "github.com/argoproj/argo-cd/v3/cmd/argocd/commands/headless" "github.com/argoproj/argo-cd/v3/cmd/argocd/commands/utils" + cmdutil "github.com/argoproj/argo-cd/v3/cmd/util" "github.com/argoproj/argo-cd/v3/common" argocdclient "github.com/argoproj/argo-cd/v3/pkg/apiclient" repocredspkg "github.com/argoproj/argo-cd/v3/pkg/apiclient/repocreds" @@ -49,16 +50,6 @@ func NewRepoCredsCommand(clientOpts *argocdclient.ClientOptions) *cobra.Command return command } -func validateBearerTokenForHTTPSRepoOnly(bearerToken string, isHTTPS bool) { - // Specifying bearerToken is only valid for HTTPS repositories - if bearerToken != "" { - if !isHTTPS { - err := stderrors.New("--bearer-token is only supported for HTTPS repositories") - errors.CheckError(err) - } - } -} - // NewRepoCredsAddCommand returns a new instance of an `argocd repocreds add` command func NewRepoCredsAddCommand(clientOpts *argocdclient.ClientOptions) *cobra.Command { var ( @@ -170,7 +161,7 @@ func NewRepoCredsAddCommand(clientOpts *argocdclient.ClientOptions) *cobra.Comma defer io.Close(conn) // Specifying bearerToken is only valid for HTTPS repositories - validateBearerTokenForHTTPSRepoOnly(repo.BearerToken, git.IsHTTPSURL(repo.URL)) + cmdutil.ValidateBearerTokenForHTTPSRepoOnly(repo.BearerToken, git.IsHTTPSURL(repo.URL)) // If the user set a username, but didn't supply password via --password, // then we prompt for it @@ -178,7 +169,8 @@ func NewRepoCredsAddCommand(clientOpts *argocdclient.ClientOptions) *cobra.Comma repo.Password = cli.PromptPassword(repo.Password) } - validateBearerTokenAndPasswordCombo(repo.BearerToken, repo.Password) + cmdutil.ValidateBearerTokenAndPasswordCombo(repo.BearerToken, repo.Password) + cmdutil.ValidateBearerTokenForGitOnly(repo.BearerToken, repo.Type) repoCreateReq := repocredspkg.RepoCredsCreateRequest{ Creds: &repo, diff --git a/cmd/argocd/commands/repocreds_test.go b/cmd/argocd/commands/repocreds_test.go deleted file mode 100644 index 8b1ab95cd2867..0000000000000 --- a/cmd/argocd/commands/repocreds_test.go +++ /dev/null @@ -1,82 +0,0 @@ -package commands - -import ( - "bytes" - "errors" - "os" - "os/exec" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestValidateBearerTokenForHTTPSRepoOnly(t *testing.T) { - tests := []struct { - name string - bearerToken string - isHTTPS bool - expectError bool - errorMsg string - }{ - { - name: "Valid HTTPS with Bearer Token", - bearerToken: "some-token", - isHTTPS: true, - expectError: false, - }, - { - name: "Invalid non-HTTPS with Bearer Token", - bearerToken: "some-token", - isHTTPS: false, - expectError: true, - errorMsg: "--bearer-token is only supported for HTTPS repositories", - }, - { - name: "Valid non-HTTPS without Bearer Token", - bearerToken: "", - isHTTPS: false, - expectError: false, - }, - { - name: "Valid HTTPS without Bearer Token", - bearerToken: "", - isHTTPS: true, - expectError: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if tt.expectError { - // The command for "Invalid non-HTTPS with Bearer Token" case does not return an error. - // This is because the function validateBearerTokenForHTTPSRepoOnly() does not return an error. - // Instead, the errors.CheckError(err) performs non-zero code system exit. - // So in order to test this case, we need to run the command in a separate process. - // https://stackoverflow.com/a/33404435 - // TODO: consider whether to change all the cmd commands to return an error instead of performing a non-zero code system exit. - // TODO: generalize this pattern to be used in other tests for failing cmd commands. - if os.Getenv("BE_CRASHER") == "1" { - validateBearerTokenForHTTPSRepoOnly(tt.bearerToken, tt.isHTTPS) - return - } - cmd := exec.Command(os.Args[0], "-test.run=TestValidateBearerTokenForHTTPSRepoOnly") - cmd.Env = append(os.Environ(), "BE_CRASHER=1") - var stderr bytes.Buffer - cmd.Stderr = &stderr - err := cmd.Run() - var exitErr *exec.ExitError - if errors.As(err, &exitErr) && !exitErr.Success() { - assert.Contains(t, stderr.String(), tt.errorMsg) - return - } - t.Fatalf("process ran with err %v, want exit status 1", err) - } else { - // This will never actually panic because the function validateBearerTokenForHTTPSRepoOnly() does not return - // an error. - assert.NotPanics(t, func() { - validateBearerTokenForHTTPSRepoOnly(tt.bearerToken, tt.isHTTPS) - }) - } - }) - } -} diff --git a/cmd/util/common.go b/cmd/util/common.go index 7c7b629ab4c98..b6c6c5668f3ca 100644 --- a/cmd/util/common.go +++ b/cmd/util/common.go @@ -1,6 +1,38 @@ package util +import ( + stderrors "errors" + + "github.com/argoproj/argo-cd/v3/util/errors" +) + var ( LogFormat string LogLevel string ) + +func ValidateBearerTokenForHTTPSRepoOnly(bearerToken string, isHTTPS bool) { + // Bearer token is only valid for HTTPS repositories + if bearerToken != "" { + if !isHTTPS { + err := stderrors.New("--bearer-token is only supported for HTTPS repositories") + errors.CheckError(err) + } + } +} + +func ValidateBearerTokenForGitOnly(bearerToken string, repoType string) { + // Bearer token is only valid for Git repositories + if bearerToken != "" && repoType != "git" { + err := stderrors.New("--bearer-token is only supported for Git repositories") + errors.CheckError(err) + } +} + +func ValidateBearerTokenAndPasswordCombo(bearerToken string, password string) { + // Either the password or the bearer token must be set, but not both + if bearerToken != "" && password != "" { + err := stderrors.New("only --bearer-token or --password is allowed, not both") + errors.CheckError(err) + } +} diff --git a/cmd/util/common_test.go b/cmd/util/common_test.go new file mode 100644 index 0000000000000..f1fd30cb2b146 --- /dev/null +++ b/cmd/util/common_test.go @@ -0,0 +1,179 @@ +package util + +import ( + "bytes" + "errors" + "os" + "os/exec" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestValidateBearerTokenAndPasswordCombo(t *testing.T) { + tests := []struct { + name string + bearerToken string + password string + expectError bool + errorMsg string + }{ + { + name: "Both token and password set", + bearerToken: "some-token", + password: "some-password", + expectError: true, + errorMsg: "only --bearer-token or --password is allowed, not both", + }, + { + name: "Only token set", + bearerToken: "some-token", + password: "", + expectError: false, + }, + { + name: "Only password set", + bearerToken: "", + password: "some-password", + expectError: false, + }, + { + name: "Neither token nor password set", + bearerToken: "", + password: "", + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // if tt.expectError { + runCmdAndCheckError(t, tt.expectError, "TestValidateBearerTokenAndPasswordCombo", tt.errorMsg, func() { + ValidateBearerTokenAndPasswordCombo(tt.bearerToken, tt.password) + }) + }) + } +} + +func TestValidateBearerTokenForGitOnly(t *testing.T) { + tests := []struct { + name string + bearerToken string + repoType string + expectError bool + errorMsg string + }{ + { + name: "Bearer token with helm repo", + bearerToken: "some-token", + repoType: "helm", + expectError: true, + errorMsg: "--bearer-token is only supported for Git repositories", + }, + { + name: "Bearer token with git repo", + bearerToken: "some-token", + repoType: "git", + expectError: false, + }, + { + name: "No bearer token with helm repo", + bearerToken: "", + repoType: "helm", + expectError: false, + }, + { + name: "No bearer token with git repo", + bearerToken: "", + repoType: "git", + expectError: false, + }, + { + name: "Bearer token with empty repo", + bearerToken: "some-token", + repoType: "", + expectError: true, + errorMsg: "--bearer-token is only supported for Git repositories", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + runCmdAndCheckError(t, tt.expectError, "TestValidateBearerTokenForGitOnly", tt.errorMsg, func() { + ValidateBearerTokenForGitOnly(tt.bearerToken, tt.repoType) + }) + }) + } +} + +func TestValidateBearerTokenForHTTPSRepoOnly(t *testing.T) { + tests := []struct { + name string + bearerToken string + isHTTPS bool + expectError bool + errorMsg string + }{ + { + name: "Bearer token with HTTPS repo", + bearerToken: "some-token", + isHTTPS: true, + expectError: false, + }, + { + name: "Bearer token with non-HTTPS repo", + bearerToken: "some-token", + isHTTPS: false, + expectError: true, + errorMsg: "--bearer-token is only supported for HTTPS repositories", + }, + { + name: "No bearer token with HTTPS repo", + bearerToken: "", + isHTTPS: true, + expectError: false, + }, + { + name: "No bearer token with non-HTTPS repo", + bearerToken: "", + isHTTPS: false, + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + runCmdAndCheckError(t, tt.expectError, "TestValidateBearerTokenForHTTPSRepoOnly", tt.errorMsg, func() { + ValidateBearerTokenForHTTPSRepoOnly(tt.bearerToken, tt.isHTTPS) + }) + }) + } +} + +func runCmdAndCheckError(t *testing.T, expectError bool, testName, errorMsg string, validationFunc func()) { + // All CLI commands do not return an error upon failure. + // Instead, the errors.CheckError(err) in each CLI command performs non-zero code system exit. + // So in order to test the commands, we need to run the command in a separate process and capture it's error message. + // https://stackoverflow.com/a/33404435 + // TODO: consider whether to change all the CLI commands to return an error instead of performing a non-zero code system exit. + if expectError { + if os.Getenv("BE_CRASHER") == "1" { + validationFunc() + return + } + cmd := exec.Command(os.Args[0], "-test.run="+testName) + cmd.Env = append(os.Environ(), "BE_CRASHER=1") + var stderr bytes.Buffer + cmd.Stderr = &stderr + err := cmd.Run() + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && !exitErr.Success() { + t.Log(stderr.String()) + assert.Contains(t, stderr.String(), errorMsg) + return + } + t.Fatalf("process ran with err %v, want exit status 1", err) + } else { + validationFunc() + } +} diff --git a/ui/src/app/settings/components/repos-list/repos-list.tsx b/ui/src/app/settings/components/repos-list/repos-list.tsx index 6b41f8580ea07..bbea7ffd82f43 100644 --- a/ui/src/app/settings/components/repos-list/repos-list.tsx +++ b/ui/src/app/settings/components/repos-list/repos-list.tsx @@ -214,7 +214,9 @@ export class ReposList extends React.Component< name: httpsValues.type === 'helm' && !httpsValues.name && 'Name is required', username: !httpsValues.username && httpsValues.password && 'Username is required if password is given.', password: !httpsValues.password && httpsValues.username && 'Password is required if username is given.', - bearerToken: httpsValues.password && httpsValues.bearerToken && 'Either the password or the bearer token must be set, but not both.', + bearerToken: + (httpsValues.password && httpsValues.bearerToken && 'Either the password or the bearer token must be set, but not both.') || + (httpsValues.type != "git" && 'Bearer token is only supported for Git repositories'), tlsClientCertKey: !httpsValues.tlsClientCertKey && httpsValues.tlsClientCertData && 'TLS client cert key is required if TLS client cert is given.' }; case ConnectionMethod.GITHUBAPP: @@ -682,15 +684,17 @@ export class ReposList extends React.Component< componentProps={{type: 'password'}} /> -
- -
+ {formApi.getFormState().values.type === 'git' && ( +
+ +
+ )}
diff --git a/ui/src/app/shared/models.ts b/ui/src/app/shared/models.ts index f338713df569c..5bbf0e55aa733 100644 --- a/ui/src/app/shared/models.ts +++ b/ui/src/app/shared/models.ts @@ -615,6 +615,7 @@ export interface RepositoryList extends ItemsList {} export interface RepoCreds { url: string; username?: string; + bearerToken?: string; } export interface RepoCredsList extends ItemsList {} diff --git a/ui/src/app/shared/services/repocreds-service.ts b/ui/src/app/shared/services/repocreds-service.ts index edac6d171d0fe..9d03113c76f3a 100644 --- a/ui/src/app/shared/services/repocreds-service.ts +++ b/ui/src/app/shared/services/repocreds-service.ts @@ -5,6 +5,7 @@ export interface HTTPSCreds { url: string; username: string; password: string; + bearerToken: string; tlsClientCertData: string; tlsClientCertKey: string; proxy: string;