Merge remote-tracking branch 'origin/main' into peterguy/clean-up-config

Amp-Thread-ID: https://ampcode.com/threads/T-019cd85e-222f-769f-9950-b0ed936a38dd
Co-authored-by: Amp <amp@ampcode.com>

# Conflicts:
#	cmd/src/login.go
#	cmd/src/login_oauth.go
#	cmd/src/login_test.go
#	cmd/src/login_validate.go
#	cmd/src/main.go
#	cmd/src/main_test.go
#	internal/api/api.go

Author: peterguy

cmd/src/login.go

@@ -29,14 +29,9 @@ Examples:
 
     $ src login https://sourcegraph.com
 
-  Use OAuth device flow to authenticate:
+  If no access token is configured, 'src login' uses OAuth device flow automatically:
 
-    $ src login --oauth https://sourcegraph.com
-
-
-  Override the default client id used during device flow when authenticating:
-
-    $ src login --oauth https://sourcegraph.com
+    $ src login https://sourcegraph.com
 `
 
 	flagSet := flag.NewFlagSet("login", flag.ExitOnError)
@@ -47,7 +42,6 @@ Examples:
 
 	var (
 		apiFlags = api.NewFlags(flagSet)
-		useOAuth = flagSet.Bool("oauth", false, "Use OAuth device flow to obtain an access token interactively")
 	)
 
 	handler := func(args []string) error {
@@ -72,7 +66,6 @@ Examples:
 			cfg:         cfg,
 			client:      client,
 			out:         os.Stdout,
-			useOAuth:    *useOAuth,
 			apiFlags:    apiFlags,
 			oauthClient: oauth.NewClient(oauth.DefaultClientID),
 		})
@@ -89,7 +82,6 @@ type loginParams struct {
 	cfg         *config
 	client      api.Client
 	out         io.Writer
-	useOAuth    bool
 	apiFlags    *api.Flags
 	oauthClient oauth.Client
 }
@@ -104,41 +96,26 @@ const (
 	loginFlowValidate
 )
 
-var loadStoredOAuthToken = oauth.LoadToken
-
 func loginCmd(ctx context.Context, p loginParams) error {
 	if p.cfg.configFilePath != "" {
 		fmt.Fprintln(p.out)
 		fmt.Fprintf(p.out, "⚠️  Warning: Configuring src with a JSON file is deprecated. Please migrate to using the env vars SRC_ENDPOINT, SRC_ACCESS_TOKEN, and SRC_PROXY instead, and then remove %s. See https://github.com/sourcegraph/src-cli#readme for more information.\n", p.cfg.configFilePath)
 	}
 
-	_, flow := selectLoginFlow(ctx, p)
+	_, flow := selectLoginFlow(p)
 	return flow(ctx, p)
 }
 
-// selectLoginFlow decides what login flow to run based on flags and config.
-func selectLoginFlow(ctx context.Context, p loginParams) (loginFlowKind, loginFlow) {
-	if p.useOAuth {
+// selectLoginFlow decides what login flow to run based on configured AuthMode.
+func selectLoginFlow(p loginParams) (loginFlowKind, loginFlow) {
+	switch p.cfg.AuthMode() {
+	case AuthModeOAuth:
 		return loginFlowOAuth, runOAuthLogin
-	}
-	if !hasEffectiveAuth(ctx, p.cfg) {
+	case AuthModeAccessToken:
+		return loginFlowValidate, runValidatedLogin
+	default:
 		return loginFlowMissingAuth, runMissingAuthLogin
 	}
-	return loginFlowValidate, runValidatedLogin
-}
-
-// hasEffectiveAuth determines whether we have auth credentials to continue. It first checks for a resolved Access Token in
-// config, then it checks for a stored OAuth token.
-func hasEffectiveAuth(ctx context.Context, cfg *config) bool {
-	if cfg.accessToken != "" {
-		return true
-	}
-
-	if _, err := loadStoredOAuthToken(ctx, cfg.endpointURL.String()); err == nil {
-		return true
-	}
-
-	return false
 }
 
 func printLoginProblem(out io.Writer, problem string) {
@@ -153,6 +130,6 @@ func loginAccessTokenMessage(endpoint string) string {
 
    To verify that it's working, run the login command again.
 
-   Alternatively, you can try logging in using OAuth by running: src login --oauth %s
+   Alternatively, you can try logging in interactively by running: src login %s
 `, endpoint, endpoint, endpoint)
 }

cmd/src/login_oauth.go

@@ -13,6 +13,8 @@ import (
 	"github.com/sourcegraph/src-cli/internal/oauth"
 )
 
+var loadStoredOAuthToken = oauth.LoadToken
+
 func runOAuthLogin(ctx context.Context, p loginParams) error {
 	endpoint := p.cfg.endpointURL.String()
 	client, err := oauthLoginClient(ctx, p, endpoint)
@@ -32,7 +34,15 @@ func runOAuthLogin(ctx context.Context, p loginParams) error {
 	return nil
 }
 
+// oauthLoginClient returns a api.Client with the OAuth token set. It will check secret storage for a token
+// and use it if one is present.
+// If no token is found, it will start a OAuth Device flow to get a token and storage in secret storage.
 func oauthLoginClient(ctx context.Context, p loginParams, endpoint string) (api.Client, error) {
+	// if we have a stored token, used it. Otherwise run the device flow
+	if token, err := loadStoredOAuthToken(ctx, endpoint); err == nil {
+		return newOAuthAPIClient(p, endpoint, token), nil
+	}
+
 	token, err := runOAuthDeviceFlow(ctx, endpoint, p.out, p.oauthClient)
 	if err != nil {
 		return nil, err
@@ -43,6 +53,10 @@ func oauthLoginClient(ctx context.Context, p loginParams, endpoint string) (api.
 		fmt.Fprintf(p.out, "⚠️  Warning: Failed to store token in keyring store: %q. Continuing with this session only.\n", err)
 	}
 
+	return newOAuthAPIClient(p, endpoint, token), nil
+}
+
+func newOAuthAPIClient(p loginParams, endpoint string, token *oauth.Token) api.Client {
 	return api.NewClient(api.ClientOpts{
 		EndpointURL:       p.cfg.endpointURL,
 		AdditionalHeaders: p.cfg.additionalHeaders,
@@ -51,7 +65,7 @@ func oauthLoginClient(ctx context.Context, p loginParams, endpoint string) (api.
 		ProxyURL:          p.cfg.proxyURL,
 		ProxyPath:         p.cfg.proxyPath,
 		OAuthToken:        token,
-	}), nil
+	})
 }
 
 func runOAuthDeviceFlow(ctx context.Context, endpoint string, out io.Writer, client oauth.Client) (*oauth.Token, error) {

cmd/src/login_test.go

@@ -10,6 +10,7 @@ import (
 	"net/url"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/sourcegraph/src-cli/internal/cmderrors"
 	"github.com/sourcegraph/src-cli/internal/oauth"
@@ -19,16 +20,12 @@ func TestLogin(t *testing.T) {
 	check := func(t *testing.T, cfg *config) (output string, err error) {
 		t.Helper()
 
-		restoreStoredOAuthLoader(t, func(context.Context, string) (*oauth.Token, error) {
-			return nil, fmt.Errorf("not found")
-		})
-
 		var out bytes.Buffer
 		err = loginCmd(context.Background(), loginParams{
 			cfg:         cfg,
 			client:      cfg.apiClient(nil, io.Discard),
 			out:         &out,
-			oauthClient: oauth.NewClient(oauth.DefaultClientID),
+			oauthClient: fakeOAuthClient{startErr: fmt.Errorf("oauth unavailable")},
 		})
 		return strings.TrimSpace(out.String()), err
 	}
@@ -39,9 +36,8 @@ func TestLogin(t *testing.T) {
 		if err != cmderrors.ExitCode1 {
 			t.Fatal(err)
 		}
-		wantOut := "❌ Problem: No access token is configured.\n\n🛠  To fix: Create an access token by going to https://sourcegraph.example.com/user/settings/tokens, then set the following environment variables in your terminal:\n\n   export SRC_ENDPOINT=https://sourcegraph.example.com\n   export SRC_ACCESS_TOKEN=(your access token)\n\n   To verify that it's working, run the login command again.\n\n   Alternatively, you can try logging in using OAuth by running: src login --oauth https://sourcegraph.example.com"
-		if out != wantOut {
-			t.Errorf("got output %q, want %q", out, wantOut)
+		if !strings.Contains(out, "OAuth Device flow authentication failed:") {
+			t.Errorf("got output %q, want oauth failure output", out)
 		}
 	})
 
@@ -51,9 +47,11 @@ func TestLogin(t *testing.T) {
 		if err != cmderrors.ExitCode1 {
 			t.Fatal(err)
 		}
-		wantOut := "⚠️  Warning: Configuring src with a JSON file is deprecated. Please migrate to using the env vars SRC_ENDPOINT, SRC_ACCESS_TOKEN, and SRC_PROXY instead, and then remove f. See https://github.com/sourcegraph/src-cli#readme for more information.\n\n❌ Problem: No access token is configured.\n\n🛠  To fix: Create an access token by going to https://example.com/user/settings/tokens, then set the following environment variables in your terminal:\n\n   export SRC_ENDPOINT=https://example.com\n   export SRC_ACCESS_TOKEN=(your access token)\n\n   To verify that it's working, run the login command again.\n\n   Alternatively, you can try logging in using OAuth by running: src login --oauth https://example.com"
-		if out != wantOut {
-			t.Errorf("got output %q, want %q", out, wantOut)
+		if !strings.Contains(out, "Configuring src with a JSON file is deprecated") {
+			t.Errorf("got output %q, want deprecation warning", out)
+		}
+		if !strings.Contains(out, "OAuth Device flow authentication failed:") {
+			t.Errorf("got output %q, want oauth failure output", out)
 		}
 	})
 
@@ -68,7 +66,7 @@ func TestLogin(t *testing.T) {
 		if err != cmderrors.ExitCode1 {
 			t.Fatal(err)
 		}
-		wantOut := "❌ Problem: Invalid access token.\n\n🛠  To fix: Create an access token by going to $ENDPOINT/user/settings/tokens, then set the following environment variables in your terminal:\n\n   export SRC_ENDPOINT=$ENDPOINT\n   export SRC_ACCESS_TOKEN=(your access token)\n\n   To verify that it's working, run the login command again.\n\n   Alternatively, you can try logging in using OAuth by running: src login --oauth $ENDPOINT\n\n   (If you need to supply custom HTTP request headers, see information about SRC_HEADER_* and SRC_HEADERS env vars at https://github.com/sourcegraph/src-cli/blob/main/AUTH_PROXY.md)"
+		wantOut := "❌ Problem: Invalid access token.\n\n🛠  To fix: Create an access token by going to $ENDPOINT/user/settings/tokens, then set the following environment variables in your terminal:\n\n   export SRC_ENDPOINT=$ENDPOINT\n   export SRC_ACCESS_TOKEN=(your access token)\n\n   To verify that it's working, run the login command again.\n\n   Alternatively, you can try logging in interactively by running: src login $ENDPOINT\n\n   (If you need to supply custom HTTP request headers, see information about SRC_HEADER_* and SRC_HEADERS env vars at https://github.com/sourcegraph/src-cli/blob/main/AUTH_PROXY.md)"
 		wantOut = strings.ReplaceAll(wantOut, "$ENDPOINT", s.URL)
 		if out != wantOut {
 			t.Errorf("got output %q, want %q", out, wantOut)
@@ -86,12 +84,81 @@ func TestLogin(t *testing.T) {
 		if err != nil {
 			t.Fatal(err)
 		}
-		wantOut := "✔️  Authenticated as alice on $ENDPOINT"
+		wantOut := "✔︎ Authenticated as alice on $ENDPOINT"
 		wantOut = strings.ReplaceAll(wantOut, "$ENDPOINT", s.URL)
 		if out != wantOut {
 			t.Errorf("got output %q, want %q", out, wantOut)
 		}
 	})
+
+	t.Run("reuses stored oauth token before device flow", func(t *testing.T) {
+		s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			fmt.Fprintln(w, `{"data":{"currentUser":{"username":"alice"}}}`)
+		}))
+		defer s.Close()
+
+		restoreStoredOAuthLoader(t, func(context.Context, string) (*oauth.Token, error) {
+			return &oauth.Token{
+				Endpoint:    s.URL,
+				ClientID:    oauth.DefaultClientID,
+				AccessToken: "oauth-token",
+				ExpiresAt:   time.Now().Add(time.Hour),
+			}, nil
+		})
+
+		u, _ := url.ParseRequestURI(s.URL)
+		startCalled := false
+		var out bytes.Buffer
+		err := loginCmd(context.Background(), loginParams{
+			cfg:    &config{endpointURL: u},
+			client: (&config{endpointURL: u}).apiClient(nil, io.Discard),
+			out:    &out,
+			oauthClient: fakeOAuthClient{
+				startErr:    fmt.Errorf("unexpected call to Start"),
+				startCalled: &startCalled,
+			},
+		})
+		if err != nil {
+			t.Fatal(err)
+		}
+		if startCalled {
+			t.Fatal("expected stored oauth token to avoid device flow")
+		}
+		gotOut := strings.TrimSpace(out.String())
+		wantOut := "✔︎ Authenticated as alice on $ENDPOINT\n\n\n✔︎ Authenticated with OAuth credentials"
+		wantOut = strings.ReplaceAll(wantOut, "$ENDPOINT", s.URL)
+		if gotOut != wantOut {
+			t.Errorf("got output %q, want %q", gotOut, wantOut)
+		}
+	})
+}
+
+type fakeOAuthClient struct {
+	startErr    error
+	startCalled *bool
+}
+
+func (f fakeOAuthClient) ClientID() string {
+	return oauth.DefaultClientID
+}
+
+func (f fakeOAuthClient) Discover(context.Context, string) (*oauth.OIDCConfiguration, error) {
+	return nil, fmt.Errorf("unexpected call to Discover")
+}
+
+func (f fakeOAuthClient) Start(context.Context, string, []string) (*oauth.DeviceAuthResponse, error) {
+	if f.startCalled != nil {
+		*f.startCalled = true
+	}
+	return nil, f.startErr
+}
+
+func (f fakeOAuthClient) Poll(context.Context, string, string, time.Duration, int) (*oauth.TokenResponse, error) {
+	return nil, fmt.Errorf("unexpected call to Poll")
+}
+
+func (f fakeOAuthClient) Refresh(context.Context, *oauth.Token) (*oauth.TokenResponse, error) {
+	return nil, fmt.Errorf("unexpected call to Refresh")
 }
 
 func TestSelectLoginFlow(t *testing.T) {
@@ -103,28 +170,13 @@ func TestSelectLoginFlow(t *testing.T) {
 		return u
 	}
 
-	restoreStoredOAuthLoader(t, func(context.Context, string) (*oauth.Token, error) {
-		return nil, fmt.Errorf("not found")
-	})
-
-	t.Run("uses oauth flow when oauth flag is set", func(t *testing.T) {
-		params := loginParams{
-			cfg:      &config{endpointURL: mustParseURL("https://example.com")},
-			useOAuth: true,
-		}
-
-		if got, _ := selectLoginFlow(context.Background(), params); got != loginFlowOAuth {
-			t.Fatalf("flow = %v, want %v", got, loginFlowOAuth)
-		}
-	})
-
-	t.Run("uses missing auth flow when auth is unavailable", func(t *testing.T) {
+	t.Run("uses oauth flow when no access token is configured", func(t *testing.T) {
 		params := loginParams{
 			cfg: &config{endpointURL: mustParseURL("https://example.com")},
 		}
 
-		if got, _ := selectLoginFlow(context.Background(), params); got != loginFlowMissingAuth {
-			t.Fatalf("flow = %v, want %v", got, loginFlowMissingAuth)
+		if got, _ := selectLoginFlow(params); got != loginFlowOAuth {
+			t.Fatalf("flow = %v, want %v", got, loginFlowOAuth)
 		}
 	})
 
@@ -133,21 +185,7 @@ func TestSelectLoginFlow(t *testing.T) {
 			cfg: &config{endpointURL: mustParseURL("https://example.com"), accessToken: "x"},
 		}
 
-		if got, _ := selectLoginFlow(context.Background(), params); got != loginFlowValidate {
-			t.Fatalf("flow = %v, want %v", got, loginFlowValidate)
-		}
-	})
-
-	t.Run("treats stored oauth as effective auth", func(t *testing.T) {
-		restoreStoredOAuthLoader(t, func(context.Context, string) (*oauth.Token, error) {
-			return &oauth.Token{AccessToken: "oauth-token"}, nil
-		})
-
-		params := loginParams{
-			cfg: &config{endpointURL: mustParseURL("https://example.com")},
-		}
-
-		if got, _ := selectLoginFlow(context.Background(), params); got != loginFlowValidate {
+		if got, _ := selectLoginFlow(params); got != loginFlowValidate {
 			t.Fatalf("flow = %v, want %v", got, loginFlowValidate)
 		}
 	})

cmd/src/login_validate.go

@@ -46,7 +46,7 @@ func validateCurrentUser(ctx context.Context, client api.Client, out io.Writer,
 		return cmderrors.ExitCode1
 	}
 	fmt.Fprintln(out)
-	fmt.Fprintf(out, "✔️  Authenticated as %s on %s\n", result.CurrentUser.Username, endpoint)
+	fmt.Fprintf(out, "✔︎ Authenticated as %s on %s\n", result.CurrentUser.Username, endpoint)
 	fmt.Fprintln(out)
 	return nil
 }