bug: only require CI=true and AccessToken when making API requests (#1278)

* check when inCI and whether we required Access Token

* fast fail in APIClient when CI=true and no Access Token

* use apiClient constructor in search_jobs

* fail fast in login / auth token when CI=true and AccessToken not set

* add comment

* review feedback

- rename `RequireAccessToken` to `checkIfCIAccessTokenRequired`
- provide more context in error on why it happened

* use sentinel error

* use lib errors

(cherry picked from commit 0e5e006)

Author: burmudar

cmd/src/auth_token.go

@@ -50,6 +50,10 @@ func init() {
 }
 
 func resolveAuthToken(ctx context.Context, cfg *config) (string, error) {
+	if err := cfg.requireCIAccessToken(); err != nil {
+		return "", err
+	}
+
 	if cfg.AccessToken != "" {
 		return cfg.AccessToken, nil
 	}

cmd/src/auth_token_test.go

@@ -17,12 +17,12 @@ func TestResolveAuthToken(t *testing.T) {
 		newOAuthTokenRefresher = func(*oauth.Token) oauthTokenRefresher {
 			newRefresherCalled = true
 			return fakeOAuthTokenRefresher{}
-			}
+		}
 
-			token, err := resolveAuthToken(context.Background(), &config{
-				AccessToken: "access-token",
-				Endpoint:    "https://example.com",
-			})
+		token, err := resolveAuthToken(context.Background(), &config{
+			AccessToken: "access-token",
+			Endpoint:    "https://example.com",
+		})
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -34,23 +34,45 @@ func TestResolveAuthToken(t *testing.T) {
 		}
 	})
 
+	t.Run("requires access token in CI", func(t *testing.T) {
+		reset := stubAuthTokenDependencies(t)
+		defer reset()
+
+		loadCalled := false
+		loadOAuthToken = func(context.Context, string) (*oauth.Token, error) {
+			loadCalled = true
+			return nil, nil
+		}
+
+		_, err := resolveAuthToken(context.Background(), &config{
+			inCI:     true,
+			Endpoint: "https://example.com",
+		})
+		if err != errCIAccessTokenRequired {
+			t.Fatalf("err = %v, want %v", err, errCIAccessTokenRequired)
+		}
+		if loadCalled {
+			t.Fatal("expected OAuth token loader not to be called")
+		}
+	})
+
 	t.Run("uses stored oauth token", func(t *testing.T) {
 		reset := stubAuthTokenDependencies(t)
 		defer reset()
 
-			loadOAuthToken = func(context.Context, string) (*oauth.Token, error) {
-				return &oauth.Token{
-					AccessToken: "oauth-token",
-				}, nil
+		loadOAuthToken = func(context.Context, string) (*oauth.Token, error) {
+			return &oauth.Token{
+				AccessToken: "oauth-token",
+			}, nil
 		}
 
 		newOAuthTokenRefresher = func(*oauth.Token) oauthTokenRefresher {
 			return fakeOAuthTokenRefresher{token: oauth.Token{AccessToken: "oauth-token"}}
-			}
+		}
 
-			token, err := resolveAuthToken(context.Background(), &config{
-				Endpoint: "https://example.com",
-			})
+		token, err := resolveAuthToken(context.Background(), &config{
+			Endpoint: "https://example.com",
+		})
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -63,17 +85,17 @@ func TestResolveAuthToken(t *testing.T) {
 		reset := stubAuthTokenDependencies(t)
 		defer reset()
 
-			loadOAuthToken = func(context.Context, string) (*oauth.Token, error) {
-				return &oauth.Token{AccessToken: "old-token"}, nil
-			}
+		loadOAuthToken = func(context.Context, string) (*oauth.Token, error) {
+			return &oauth.Token{AccessToken: "old-token"}, nil
+		}
 
 		newOAuthTokenRefresher = func(*oauth.Token) oauthTokenRefresher {
 			return fakeOAuthTokenRefresher{token: oauth.Token{AccessToken: "new-token"}}
-			}
+		}
 
-			token, err := resolveAuthToken(context.Background(), &config{
-				Endpoint: "https://example.com",
-			})
+		token, err := resolveAuthToken(context.Background(), &config{
+			Endpoint: "https://example.com",
+		})
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -86,20 +108,20 @@ func TestResolveAuthToken(t *testing.T) {
 		reset := stubAuthTokenDependencies(t)
 		defer reset()
 
-			loadOAuthToken = func(context.Context, string) (*oauth.Token, error) {
-				return &oauth.Token{AccessToken: "old-token"}, nil
-			}
+		loadOAuthToken = func(context.Context, string) (*oauth.Token, error) {
+			return &oauth.Token{AccessToken: "old-token"}, nil
+		}
 		newOAuthTokenRefresher = func(*oauth.Token) oauthTokenRefresher {
 			return fakeOAuthTokenRefresher{err: fmt.Errorf("refresh failed")}
 		}
 
-			_, err := resolveAuthToken(context.Background(), &config{
-				Endpoint: "https://example.com",
-			})
-			if err == nil {
-				t.Fatal("expected error")
-			}
+		_, err := resolveAuthToken(context.Background(), &config{
+			Endpoint: "https://example.com",
 		})
+		if err == nil {
+			t.Fatal("expected error")
+		}
+	})
 }
 
 func stubAuthTokenDependencies(t *testing.T) func() {

cmd/src/login.go

@@ -96,6 +96,10 @@ const (
 )
 
 func loginCmd(ctx context.Context, p loginParams) error {
+	if err := p.cfg.requireCIAccessToken(); err != nil {
+		return err
+	}
+
 	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)

cmd/src/login_test.go

@@ -50,6 +50,16 @@ func TestLogin(t *testing.T) {
 		}
 	})
 
+	t.Run("CI requires access token", func(t *testing.T) {
+		out, err := check(t, &config{Endpoint: "https://example.com", inCI: true}, "https://example.com")
+		if err != errCIAccessTokenRequired {
+			t.Fatalf("err = %v, want %v", err, errCIAccessTokenRequired)
+		}
+		if out != "" {
+			t.Fatalf("output = %q, want empty output", out)
+		}
+	})
+
 	t.Run("warning when using config file", func(t *testing.T) {
 		out, err := check(t, &config{Endpoint: "https://example.com", ConfigFilePath: "f"}, "https://example.com")
 		if err == nil {

cmd/src/main.go

@@ -83,7 +83,7 @@ var (
 
 	errConfigMerge                 = errors.New("when using a configuration file, zero or all environment variables must be set")
 	errConfigAuthorizationConflict = errors.New("when passing an 'Authorization' additional headers, SRC_ACCESS_TOKEN must never be set")
-	errCIAccessTokenRequired       = errors.New("SRC_ACCESS_TOKEN must be set in CI")
+	errCIAccessTokenRequired       = errors.New("CI is true and SRC_ACCESS_TOKEN is not set or empty. When running in CI OAuth tokens cannot be used, only SRC_ACCESS_TOKEN. Either set CI=false or define a SRC_ACCESS_TOKEN")
 )
 
 // commands contains all registered subcommands.
@@ -122,6 +122,7 @@ type config struct {
 	ProxyURL          *url.URL
 	ProxyPath         string
 	ConfigFilePath    string
+	inCI              bool
 }
 
 type AuthMode int
@@ -138,16 +139,31 @@ func (c *config) AuthMode() AuthMode {
 	return AuthModeOAuth
 }
 
+func (c *config) InCI() bool {
+	return c.inCI
+}
+
+func (c *config) requireCIAccessToken() error {
+	// In CI we typically do not have access to the keyring and the machine is also
+	// typically headless, so OAuth tokens are not a reliable fallback.
+	if c.InCI() && c.AuthMode() != AuthModeAccessToken {
+		return errCIAccessTokenRequired
+	}
+
+	return nil
+}
+
 // apiClient returns an api.Client built from the configuration.
 func (c *config) apiClient(flags *api.Flags, out io.Writer) api.Client {
 	opts := api.ClientOpts{
-		Endpoint:          c.Endpoint,
-		AccessToken:       c.AccessToken,
-		AdditionalHeaders: c.AdditionalHeaders,
-		Flags:             flags,
-		Out:               out,
-		ProxyURL:          c.ProxyURL,
-		ProxyPath:         c.ProxyPath,
+		Endpoint:               c.Endpoint,
+		AccessToken:            c.AccessToken,
+		AdditionalHeaders:      c.AdditionalHeaders,
+		Flags:                  flags,
+		Out:                    out,
+		ProxyURL:               c.ProxyURL,
+		ProxyPath:              c.ProxyPath,
+		RequireAccessTokenInCI: c.InCI(),
 	}
 
 	// Only use OAuth if we do not have SRC_ACCESS_TOKEN set
@@ -179,6 +195,7 @@ func readConfig() (*config, error) {
 		return nil, err
 	}
 	var cfg config
+	cfg.inCI = isCI()
 	if err == nil {
 		cfg.ConfigFilePath = cfgPath
 		if err := json.Unmarshal(data, &cfg); err != nil {
@@ -276,10 +293,6 @@ func readConfig() (*config, error) {
 
 	cfg.Endpoint = cleanEndpoint(cfg.Endpoint)
 
-	if isCI() && cfg.AccessToken == "" {
-		return nil, errCIAccessTokenRequired
-	}
-
 	return &cfg, nil
 }
 

cmd/src/main_test.go

@@ -1,7 +1,10 @@
 package main
 
 import (
+	"context"
 	"encoding/json"
+	"errors"
+	"io"
 	"net/url"
 	"os"
 	"path/filepath"
@@ -285,9 +288,13 @@ func TestReadConfig(t *testing.T) {
 			wantErr:     errConfigAuthorizationConflict.Error(),
 		},
 		{
-			name:    "CI requires access token",
-			envCI:   "1",
-			wantErr: errCIAccessTokenRequired.Error(),
+			name:  "CI does not require access token during config read",
+			envCI: "1",
+			want: &config{
+				Endpoint:          "https://sourcegraph.com",
+				AdditionalHeaders: map[string]string{},
+				inCI:              true,
+			},
 		},
 		{
 			name:  "CI allows access token from config file",
@@ -300,6 +307,7 @@ func TestReadConfig(t *testing.T) {
 				Endpoint:          "https://example.com",
 				AccessToken:       "deadbeef",
 				AdditionalHeaders: map[string]string{},
+				inCI:              true,
 			},
 		},
 	}
@@ -351,8 +359,8 @@ func TestReadConfig(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			config, err := readConfig()
-			if diff := cmp.Diff(test.want, config); diff != "" {
+			gotConfig, err := readConfig()
+			if diff := cmp.Diff(test.want, gotConfig, cmp.AllowUnexported(config{})); diff != "" {
 				t.Errorf("config: %v", diff)
 			}
 			var errMsg string
@@ -379,3 +387,36 @@ func TestConfigAuthMode(t *testing.T) {
 		}
 	})
 }
+
+func TestConfigAPIClientCIAccessTokenGate(t *testing.T) {
+	endpoint := "https://example.com"
+
+	t.Run("requires access token in CI", func(t *testing.T) {
+		client := (&config{Endpoint: endpoint, inCI: true}).apiClient(nil, io.Discard)
+
+		_, err := client.NewHTTPRequest(context.Background(), "GET", ".api/src-cli/version", nil)
+		if !errors.Is(err, api.ErrCIAccessTokenRequired) {
+			t.Fatalf("NewHTTPRequest() error = %v, want %v", err, api.ErrCIAccessTokenRequired)
+		}
+	})
+
+	t.Run("allows access token in CI", func(t *testing.T) {
+		client := (&config{Endpoint: endpoint, inCI: true, AccessToken: "abc"}).apiClient(nil, io.Discard)
+
+		req, err := client.NewHTTPRequest(context.Background(), "GET", ".api/src-cli/version", nil)
+		if err != nil {
+			t.Fatalf("NewHTTPRequest() unexpected error: %s", err)
+		}
+		if got := req.Header.Get("Authorization"); got != "token abc" {
+			t.Fatalf("Authorization header = %q, want %q", got, "token abc")
+		}
+	})
+
+	t.Run("allows oauth mode outside CI", func(t *testing.T) {
+		client := (&config{Endpoint: endpoint}).apiClient(nil, io.Discard)
+
+		if _, err := client.NewHTTPRequest(context.Background(), "GET", ".api/src-cli/version", nil); err != nil {
+			t.Fatalf("NewHTTPRequest() unexpected error: %s", err)
+		}
+	})
+}

cmd/src/search_jobs.go

@@ -155,12 +155,7 @@ func parseColumns(columnsFlag string) []string {
 
 // createSearchJobsClient creates a reusable API client for search jobs commands
 func createSearchJobsClient(out *flag.FlagSet, apiFlags *api.Flags) api.Client {
-	return api.NewClient(api.ClientOpts{
-		Endpoint:    cfg.Endpoint,
-		AccessToken: cfg.AccessToken,
-		Out:         out.Output(),
-		Flags:       apiFlags,
-	})
+	return cfg.apiClient(apiFlags, out.Output())
 }
 
 // parseSearchJobsArgs parses command arguments with the provided flag set