From 665e814c5d17d21ce35dec1074081b6c14fb2136 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Fri, 18 Oct 2024 12:58:00 -0700 Subject: [PATCH 1/6] Setup acceptance testing for auth and tests for auth-token and auth-status --- acceptance/acceptance_test.go | 9 +++++++++ acceptance/testdata/auth/auth-status.txtar | 3 +++ acceptance/testdata/auth/auth-token.txtar | 3 +++ 3 files changed, 15 insertions(+) create mode 100644 acceptance/testdata/auth/auth-status.txtar create mode 100644 acceptance/testdata/auth/auth-token.txtar diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index 017e6e62b..8804b1534 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -63,6 +63,15 @@ func TestAPI(t *testing.T) { testscript.Run(t, testScriptParamsFor(tsEnv, "api")) } +func TestAuth(t *testing.T) { + var tsEnv testScriptEnv + if err := tsEnv.fromEnv(); err != nil { + t.Fatal(err) + } + + testscript.Run(t, testScriptParamsFor(tsEnv, "auth")) +} + func TestReleases(t *testing.T) { var tsEnv testScriptEnv if err := tsEnv.fromEnv(); err != nil { diff --git a/acceptance/testdata/auth/auth-status.txtar b/acceptance/testdata/auth/auth-status.txtar new file mode 100644 index 000000000..da3011d1a --- /dev/null +++ b/acceptance/testdata/auth/auth-status.txtar @@ -0,0 +1,3 @@ +# Check the authentication status +exec gh auth status --hostname $GH_HOST +stdout $GH_HOST \ No newline at end of file diff --git a/acceptance/testdata/auth/auth-token.txtar b/acceptance/testdata/auth/auth-token.txtar new file mode 100644 index 000000000..614d11817 --- /dev/null +++ b/acceptance/testdata/auth/auth-token.txtar @@ -0,0 +1,3 @@ +# Check authentication token +exec gh auth token --hostname $GH_HOST +stdout $GH_TOKEN \ No newline at end of file From 52daa9cf7d6f446726e11061e0525377d50d3843 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Fri, 18 Oct 2024 14:59:15 -0700 Subject: [PATCH 2/6] Add --token flag to `gh auth login` to accept a PAT as a flag Additionally, this commit adds acceptance testing for `gh auth login` and `gh auth logout`. The --token flag was necessary for adding testing for `gh auth login` because the current implementation with `--with-token` appears to be broken. It hangs, waiting for user input, but user input doesn't exit it. Additionally, it appears that `--with-token` is intended to allow for TTY input of an auth token, but it isn't implemented. `--with-token` does work when used with the redirect operator `<` when the token is saved in a file. However, due to limitations of testscripts, I could not use a file for saving the token in a repeatable manner. Thus, implementing the `--token` flag seemed like a quick solution to validate that the direction I was going during testing was valid. Whether the flag stays or not is up for discussion, and I'd love to get input on that from the team. --- acceptance/acceptance_test.go | 26 ++++++++++++++++- .../testdata/auth/auth-login-logout.txtar | 29 +++++++++++++++++++ pkg/cmd/auth/login/login.go | 15 ++++++---- pkg/cmd/auth/logout/logout.go | 1 + 4 files changed, 65 insertions(+), 6 deletions(-) create mode 100644 acceptance/testdata/auth/auth-login-logout.txtar diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index 8804b1534..3c2b3358b 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -95,7 +95,7 @@ func TestRepo(t *testing.T) { if err := tsEnv.fromEnv(); err != nil { t.Fatal(err) } - + testscript.Run(t, testScriptParamsFor(tsEnv, "repo")) } @@ -240,6 +240,30 @@ func sharedCmds(tsEnv testScriptEnv) map[string]func(ts *testscript.TestScript, ts.Check(os.WriteFile(src, data, mode)) }, + // perhaps these need to be extracted to different files, soon? + // I'm prompted to ask because "deleteEnvVar" could just be + // syntactic sugar for setEnvVar with an empty value. Can't do that + // with the current implementation + "deleteEnvVar": func(ts *testscript.TestScript, neg bool, args []string) { + if neg { + ts.Fatalf("unsupported: ! deleteEnvVar") + } + if len(args) != 1 { + ts.Fatalf("usage: deleteEnvVar name") + } + + ts.Setenv(args[0], "") + }, + "setEnvVar": func(ts *testscript.TestScript, neg bool, args []string) { + if neg { + ts.Fatalf("unsupported: ! setEnvVar") + } + if len(args) != 2 { + ts.Fatalf("usage: setEnvVar name value") + } + + ts.Setenv(args[0], args[1]) + }, "stdout2env": func(ts *testscript.TestScript, neg bool, args []string) { if neg { ts.Fatalf("unsupported: ! stdout2env") diff --git a/acceptance/testdata/auth/auth-login-logout.txtar b/acceptance/testdata/auth/auth-login-logout.txtar new file mode 100644 index 000000000..3e46d4a0f --- /dev/null +++ b/acceptance/testdata/auth/auth-login-logout.txtar @@ -0,0 +1,29 @@ +# We aren't logged in at the moment, but GH_HOST will override the +# need to login. We are going to clear GH_HOST first to ensure no +# overrides are happening + +# Copy $GH_TOKEN to a new env var +setEnvVar LOGIN_TOKEN $GH_TOKEN +exec echo $LOGIN_TOKEN +stdout $GH_TOKEN + +# Remove GH_TOKEN env var so we don't fall back to it +deleteEnvVar GH_TOKEN + +# Ensure the token was deleted +exec echo $GH_TOKEN +! stdout '.' + +# Login to the host +exec gh auth login --hostname=$GH_HOST --with-token --token=$LOGIN_TOKEN --insecure-storage + +# Check that we are logged in +exec gh auth status --hostname $GH_HOST +stdout $GH_HOST + +# Logout of the host +exec gh auth logout --hostname $GH_HOST +stderr 'Logged out of' + +# Check that we are logged out +! exec gh auth status --hostname $GH_HOST diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index 653c9e6c0..eb0b2e1c4 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -104,14 +104,18 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm } if tokenStdin { - defer opts.IO.In.Close() - token, err := io.ReadAll(opts.IO.In) - if err != nil { - return fmt.Errorf("failed to read token from standard input: %w", err) + if opts.Token == "" { + defer opts.IO.In.Close() + token, err := io.ReadAll(opts.IO.In) + if err != nil { + return fmt.Errorf("failed to read token from standard input: %w", err) + } + opts.Token = strings.TrimSpace(string(token)) } - opts.Token = strings.TrimSpace(string(token)) } + // I think this is dead code because the token will always be set if we make it past + // the conditional above (lines 106-115) if opts.IO.CanPrompt() && opts.Token == "" { opts.Interactive = true } @@ -139,6 +143,7 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm cmd.Flags().StringSliceVarP(&opts.Scopes, "scopes", "s", nil, "Additional authentication scopes to request") cmd.Flags().BoolVar(&tokenStdin, "with-token", false, "Read token from standard input") cmd.Flags().BoolVarP(&opts.Web, "web", "w", false, "Open a browser to authenticate") + cmd.Flags().StringVarP(&opts.Token, "token", "t", "", "Authenticate using a personal access token") cmdutil.StringEnumFlag(cmd, &opts.GitProtocol, "git-protocol", "p", "", []string{"ssh", "https"}, "The protocol to use for git operations on this host") // secure storage became the default on 2023/4/04; this flag is left as a no-op for backwards compatibility diff --git a/pkg/cmd/auth/logout/logout.go b/pkg/cmd/auth/logout/logout.go index ef978ebac..f1eaa4e24 100644 --- a/pkg/cmd/auth/logout/logout.go +++ b/pkg/cmd/auth/logout/logout.go @@ -58,6 +58,7 @@ func NewCmdLogout(f *cmdutil.Factory, runF func(*LogoutOptions) error) *cobra.Co } cmd.Flags().StringVarP(&opts.Hostname, "hostname", "h", "", "The hostname of the GitHub instance to log out of") + // This flag doesn't seem to be working with --user but it is working with -u in acceptance testing cmd.Flags().StringVarP(&opts.Username, "user", "u", "", "The account to log out of") return cmd From aaf4c4e4e3a3c91f5d7a5f87e197bf0de4b59476 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Mon, 21 Oct 2024 09:22:27 -0700 Subject: [PATCH 3/6] Clean up auth-login-logout acceptance test with native functionality The previous commit introduced two new functions, setEnvVar and deleteEnvVar that are duplicative of functionality native to testscripts. This commit switches to the native functionality and removes the duplicative functions introduced in the previous commit. Additionally, it removes the `--token` flag that was added to `gh auth login` --- acceptance/acceptance_test.go | 24 ------------------- .../testdata/auth/auth-login-logout.txtar | 10 ++++---- pkg/cmd/auth/login/login.go | 15 ++++-------- 3 files changed, 11 insertions(+), 38 deletions(-) diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index 3c2b3358b..dd5b8b0d1 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -240,30 +240,6 @@ func sharedCmds(tsEnv testScriptEnv) map[string]func(ts *testscript.TestScript, ts.Check(os.WriteFile(src, data, mode)) }, - // perhaps these need to be extracted to different files, soon? - // I'm prompted to ask because "deleteEnvVar" could just be - // syntactic sugar for setEnvVar with an empty value. Can't do that - // with the current implementation - "deleteEnvVar": func(ts *testscript.TestScript, neg bool, args []string) { - if neg { - ts.Fatalf("unsupported: ! deleteEnvVar") - } - if len(args) != 1 { - ts.Fatalf("usage: deleteEnvVar name") - } - - ts.Setenv(args[0], "") - }, - "setEnvVar": func(ts *testscript.TestScript, neg bool, args []string) { - if neg { - ts.Fatalf("unsupported: ! setEnvVar") - } - if len(args) != 2 { - ts.Fatalf("usage: setEnvVar name value") - } - - ts.Setenv(args[0], args[1]) - }, "stdout2env": func(ts *testscript.TestScript, neg bool, args []string) { if neg { ts.Fatalf("unsupported: ! stdout2env") diff --git a/acceptance/testdata/auth/auth-login-logout.txtar b/acceptance/testdata/auth/auth-login-logout.txtar index 3e46d4a0f..3933ce1da 100644 --- a/acceptance/testdata/auth/auth-login-logout.txtar +++ b/acceptance/testdata/auth/auth-login-logout.txtar @@ -3,19 +3,21 @@ # overrides are happening # Copy $GH_TOKEN to a new env var -setEnvVar LOGIN_TOKEN $GH_TOKEN +env LOGIN_TOKEN=$GH_TOKEN exec echo $LOGIN_TOKEN stdout $GH_TOKEN # Remove GH_TOKEN env var so we don't fall back to it -deleteEnvVar GH_TOKEN +env GH_TOKEN='' # Ensure the token was deleted exec echo $GH_TOKEN ! stdout '.' -# Login to the host -exec gh auth login --hostname=$GH_HOST --with-token --token=$LOGIN_TOKEN --insecure-storage +# Login to the host by feeding the token to stdin +exec echo $LOGIN_TOKEN +stdin stdout +exec gh auth login --hostname=$GH_HOST --with-token --insecure-storage # Check that we are logged in exec gh auth status --hostname $GH_HOST diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index eb0b2e1c4..653c9e6c0 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -104,18 +104,14 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm } if tokenStdin { - if opts.Token == "" { - defer opts.IO.In.Close() - token, err := io.ReadAll(opts.IO.In) - if err != nil { - return fmt.Errorf("failed to read token from standard input: %w", err) - } - opts.Token = strings.TrimSpace(string(token)) + defer opts.IO.In.Close() + token, err := io.ReadAll(opts.IO.In) + if err != nil { + return fmt.Errorf("failed to read token from standard input: %w", err) } + opts.Token = strings.TrimSpace(string(token)) } - // I think this is dead code because the token will always be set if we make it past - // the conditional above (lines 106-115) if opts.IO.CanPrompt() && opts.Token == "" { opts.Interactive = true } @@ -143,7 +139,6 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm cmd.Flags().StringSliceVarP(&opts.Scopes, "scopes", "s", nil, "Additional authentication scopes to request") cmd.Flags().BoolVar(&tokenStdin, "with-token", false, "Read token from standard input") cmd.Flags().BoolVarP(&opts.Web, "web", "w", false, "Open a browser to authenticate") - cmd.Flags().StringVarP(&opts.Token, "token", "t", "", "Authenticate using a personal access token") cmdutil.StringEnumFlag(cmd, &opts.GitProtocol, "git-protocol", "p", "", []string{"ssh", "https"}, "The protocol to use for git operations on this host") // secure storage became the default on 2023/4/04; this flag is left as a no-op for backwards compatibility From 0614d85027c0b05f2c2a37a2f88352d9c51bda8d Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Mon, 21 Oct 2024 15:21:47 -0700 Subject: [PATCH 4/6] Add acceptance tests for auth-setup-git and formattedStringToEnv helper func To test this, I decided to look into the .gitconfig used for the test and examine the credential helpers. However, the format of the git command is `git config --get credential..helper` What's awkward about this is that the depends on the host the user specified when running the tests, meaning I'd need to create a key like credential.https://github.com.helper to access what I need while setting this up. There was no functionality for string formatting before, so I added the command formattedStringToEnv which essentially wraps fmt.Sprintf() and saves the string to an environment variable. This allowed me to dynamically create the config key in the test. --- acceptance/acceptance_test.go | 19 +++++++++++++++++++ acceptance/testdata/auth/auth-setup-git.txtar | 13 +++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 acceptance/testdata/auth/auth-setup-git.txtar diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index dd5b8b0d1..ace985af6 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -203,6 +203,17 @@ func sharedCmds(tsEnv testScriptEnv) map[string]func(ts *testscript.TestScript, ts.Setenv(env[:i], strings.ToUpper(env[i+1:])) } }, + // Creates a formatted string using standard fmt.Sprintf syntax + // and sets it as an environment variable of the given name. + "formattedStringToEnv": func(ts *testscript.TestScript, neg bool, args []string) { + if neg { + ts.Fatalf("unsupported: ! formattedStringToEnv") + } + if len(args) < 3 { + ts.Fatalf("usage: formattedStringToEnv name string args...") + } + ts.Setenv(args[0], fmt.Sprintf(args[1], toAnySlice(args[2:])...)) + }, "replace": func(ts *testscript.TestScript, neg bool, args []string) { if neg { ts.Fatalf("unsupported: ! replace") @@ -270,6 +281,14 @@ func sharedCmds(tsEnv testScriptEnv) map[string]func(ts *testscript.TestScript, } } +func toAnySlice(strings []string) []any { + anys := make([]any, len(strings)) + for i, s := range strings { + anys[i] = s + } + return anys +} + var letters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ") func randomString(n int) string { diff --git a/acceptance/testdata/auth/auth-setup-git.txtar b/acceptance/testdata/auth/auth-setup-git.txtar new file mode 100644 index 000000000..84794ae8f --- /dev/null +++ b/acceptance/testdata/auth/auth-setup-git.txtar @@ -0,0 +1,13 @@ +# Setup the credential key name string +formattedStringToEnv CREDENTIAL_HELPER_KEY 'credential.https://%s.helper' $GH_HOST + +# Check that the credential helper is unset for the host. This command is +# expected to fail before gh auth setup-git is run. +! exec git config --get $CREDENTIAL_HELPER_KEY + +# Run the setup-git command +exec gh auth setup-git + +# Check that the credential helper is set to gh +exec git config --get $CREDENTIAL_HELPER_KEY +stdout gh From 59aedc4e89c4aa359c87c55ad17b2a38233bb008 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Mon, 21 Oct 2024 16:09:58 -0700 Subject: [PATCH 5/6] Remove comment from gh auth logout --- pkg/cmd/auth/logout/logout.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/cmd/auth/logout/logout.go b/pkg/cmd/auth/logout/logout.go index f1eaa4e24..ef978ebac 100644 --- a/pkg/cmd/auth/logout/logout.go +++ b/pkg/cmd/auth/logout/logout.go @@ -58,7 +58,6 @@ func NewCmdLogout(f *cmdutil.Factory, runF func(*LogoutOptions) error) *cobra.Co } cmd.Flags().StringVarP(&opts.Hostname, "hostname", "h", "", "The hostname of the GitHub instance to log out of") - // This flag doesn't seem to be working with --user but it is working with -u in acceptance testing cmd.Flags().StringVarP(&opts.Username, "user", "u", "", "The account to log out of") return cmd From 2b480daf7abba51332ef1493a6d19120bc4c9ddd Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Wed, 23 Oct 2024 13:56:09 -0700 Subject: [PATCH 6/6] Address PR feedback --- acceptance/acceptance_test.go | 19 ------------------- .../testdata/auth/auth-login-logout.txtar | 10 ++-------- acceptance/testdata/auth/auth-setup-git.txtar | 9 +++------ acceptance/testdata/auth/auth-status.txtar | 2 +- 4 files changed, 6 insertions(+), 34 deletions(-) diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index ace985af6..dd5b8b0d1 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -203,17 +203,6 @@ func sharedCmds(tsEnv testScriptEnv) map[string]func(ts *testscript.TestScript, ts.Setenv(env[:i], strings.ToUpper(env[i+1:])) } }, - // Creates a formatted string using standard fmt.Sprintf syntax - // and sets it as an environment variable of the given name. - "formattedStringToEnv": func(ts *testscript.TestScript, neg bool, args []string) { - if neg { - ts.Fatalf("unsupported: ! formattedStringToEnv") - } - if len(args) < 3 { - ts.Fatalf("usage: formattedStringToEnv name string args...") - } - ts.Setenv(args[0], fmt.Sprintf(args[1], toAnySlice(args[2:])...)) - }, "replace": func(ts *testscript.TestScript, neg bool, args []string) { if neg { ts.Fatalf("unsupported: ! replace") @@ -281,14 +270,6 @@ func sharedCmds(tsEnv testScriptEnv) map[string]func(ts *testscript.TestScript, } } -func toAnySlice(strings []string) []any { - anys := make([]any, len(strings)) - for i, s := range strings { - anys[i] = s - } - return anys -} - var letters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ") func randomString(n int) string { diff --git a/acceptance/testdata/auth/auth-login-logout.txtar b/acceptance/testdata/auth/auth-login-logout.txtar index 3933ce1da..fb25f9eb5 100644 --- a/acceptance/testdata/auth/auth-login-logout.txtar +++ b/acceptance/testdata/auth/auth-login-logout.txtar @@ -1,19 +1,13 @@ -# We aren't logged in at the moment, but GH_HOST will override the -# need to login. We are going to clear GH_HOST first to ensure no +# We aren't logged in at the moment, but GH_TOKEN will override the +# need to login. We are going to clear GH_TOKEN first to ensure no # overrides are happening # Copy $GH_TOKEN to a new env var env LOGIN_TOKEN=$GH_TOKEN -exec echo $LOGIN_TOKEN -stdout $GH_TOKEN # Remove GH_TOKEN env var so we don't fall back to it env GH_TOKEN='' -# Ensure the token was deleted -exec echo $GH_TOKEN -! stdout '.' - # Login to the host by feeding the token to stdin exec echo $LOGIN_TOKEN stdin stdout diff --git a/acceptance/testdata/auth/auth-setup-git.txtar b/acceptance/testdata/auth/auth-setup-git.txtar index 84794ae8f..e3be28cd5 100644 --- a/acceptance/testdata/auth/auth-setup-git.txtar +++ b/acceptance/testdata/auth/auth-setup-git.txtar @@ -1,13 +1,10 @@ -# Setup the credential key name string -formattedStringToEnv CREDENTIAL_HELPER_KEY 'credential.https://%s.helper' $GH_HOST - # Check that the credential helper is unset for the host. This command is # expected to fail before gh auth setup-git is run. -! exec git config --get $CREDENTIAL_HELPER_KEY +! exec git config --get credential.https://${GH_HOST}.helper # Run the setup-git command exec gh auth setup-git # Check that the credential helper is set to gh -exec git config --get $CREDENTIAL_HELPER_KEY -stdout gh +exec git config --get credential.https://${GH_HOST}.helper +stdout '^.*gh auth git-credential$' diff --git a/acceptance/testdata/auth/auth-status.txtar b/acceptance/testdata/auth/auth-status.txtar index da3011d1a..2afee1eb6 100644 --- a/acceptance/testdata/auth/auth-status.txtar +++ b/acceptance/testdata/auth/auth-status.txtar @@ -1,3 +1,3 @@ # Check the authentication status exec gh auth status --hostname $GH_HOST -stdout $GH_HOST \ No newline at end of file +stdout '✓ Logged in to ' \ No newline at end of file