better error handling

handle oauth discovery failure and set client id on token

- use SRC CLI client id as default and handle discovery failures
- add clientID flag and set it on the token

improve error message and panic in apiClient if no usable token

- warn if we fail to store the token on login
- panic if apiClient has no accessToken or OAuth token to use

Author: burmudar

cmd/src/login.go

@@ -74,6 +74,7 @@ Examples:
 			endpoint:         endpoint,
 			out:              os.Stdout,
 			useOAuth:         *useOAuth,
+			oauthClientID:    *OAuthClientID,
 			apiFlags:         apiFlags,
 			deviceFlowClient: oauth.NewClient(oauth.DefaultClientID),
 		})
@@ -92,6 +93,7 @@ type loginParams struct {
 	endpoint         string
 	out              io.Writer
 	useOAuth         bool
+	oauthClientID    string
 	apiFlags         *api.Flags
 	deviceFlowClient oauth.Client
 }
@@ -125,19 +127,27 @@ func loginCmd(ctx context.Context, p loginParams) error {
 	cfg.Endpoint = endpointArg
 
 	if p.useOAuth {
-		token, err := runOAuthDeviceFlow(ctx, endpointArg, out, p.deviceFlowClient)
+		token, err := runOAuthDeviceFlow(ctx, endpointArg, p.oauthClientID, out, p.deviceFlowClient)
 		if err != nil {
 			printProblem(fmt.Sprintf("OAuth Device flow authentication failed: %s", err))
 			fmt.Fprintln(out, createAccessTokenMessage)
 			return cmderrors.ExitCode1
 		}
 
 		if err := oauth.StoreToken(ctx, token); err != nil {
-			printProblem(fmt.Sprintf("Failed to store token in keyring store: %s", err))
-			return cmderrors.ExitCode1
+			fmt.Fprintln(out)
+			fmt.Fprintf(out, "⚠️  Warning: Failed to store token in keyring store: %q. Continuing with this session only.\n", err)
 		}
 
-		client = cfg.apiClient(p.apiFlags, out)
+		client = api.NewClient(api.ClientOpts{
+			Endpoint:          cfg.Endpoint,
+			AdditionalHeaders: cfg.AdditionalHeaders,
+			Flags:             p.apiFlags,
+			Out:               out,
+			ProxyURL:          cfg.ProxyURL,
+			ProxyPath:         cfg.ProxyPath,
+			OAuthToken:        token,
+		})
 	} else if noToken || endpointConflict {
 		fmt.Fprintln(out)
 		switch {
@@ -184,7 +194,7 @@ func loginCmd(ctx context.Context, p loginParams) error {
 	return nil
 }
 
-func runOAuthDeviceFlow(ctx context.Context, endpoint string, out io.Writer, client oauth.Client) (*oauth.Token, error) {
+func runOAuthDeviceFlow(ctx context.Context, endpoint, clientID string, out io.Writer, client oauth.Client) (*oauth.Token, error) {
 	authResp, err := client.Start(ctx, endpoint, nil)
 	if err != nil {
 		return nil, err
@@ -214,7 +224,9 @@ func runOAuthDeviceFlow(ctx context.Context, endpoint string, out io.Writer, cli
 		return nil, err
 	}
 
-	return resp.Token(endpoint), nil
+	token := resp.Token(endpoint)
+	token.ClientID = clientID
+	return token, nil
 }
 
 func openInBrowser(url string) error {

cmd/src/main.go

@@ -138,6 +138,9 @@ func (c *config) apiClient(flags *api.Flags, out io.Writer) api.Client {
 	if c.AccessToken == "" {
 		if t, err := oauthdevice.LoadToken(context.Background(), c.Endpoint); err == nil {
 			opts.OAuthToken = t
+		} else {
+			// TODO(burmudar): should return an error instead
+			panic("No access token set and no OAuth token found either - unable to create api client: " + err.Error())
 		}
 	}
 

internal/oauth/flow.go

@@ -3,6 +3,7 @@
 package oauth
 
 import (
+	"cmp"
 	"context"
 	"encoding/json"
 	"fmt"
@@ -67,6 +68,7 @@ type TokenResponse struct {
 
 type Token struct {
 	Endpoint     string    `json:"endpoint"`
+	ClientID     string    `json:"client_id,omitempty"`
 	AccessToken  string    `json:"access_token"`
 	RefreshToken string    `json:"refresh_token,omitempty"`
 	ExpiresAt    time.Time `json:"expires_at"`
@@ -92,17 +94,14 @@ type httpClient struct {
 }
 
 func NewClient(clientID string) Client {
-	return &httpClient{
-		clientID: clientID,
-		client: &http.Client{
-			Timeout: 30 * time.Second,
-		},
-		configCache: make(map[string]*OIDCConfiguration),
-	}
+	return NewClientWithHTTPClient(clientID, &http.Client{
+		Timeout: 30 * time.Second,
+	})
 }
 
-func NewClientWithHTTPClient(c *http.Client) Client {
+func NewClientWithHTTPClient(clientID string, c *http.Client) Client {
 	return &httpClient{
+		clientID:    cmp.Or(clientID, DefaultClientID),
 		client:      c,
 		configCache: make(map[string]*OIDCConfiguration),
 	}
@@ -170,7 +169,7 @@ func (c *httpClient) Start(ctx context.Context, endpoint string, scopes []string
 	}
 
 	data := url.Values{}
-	data.Set("client_id", DefaultClientID)
+	data.Set("client_id", c.clientID)
 	if len(scopes) > 0 {
 		data.Set("scope", strings.Join(scopes, " "))
 	} else {
@@ -284,7 +283,7 @@ func (e *PollError) Error() string {
 
 func (c *httpClient) pollOnce(ctx context.Context, tokenEndpoint, deviceCode string) (*TokenResponse, error) {
 	data := url.Values{}
-	data.Set("client_id", DefaultClientID)
+	data.Set("client_id", c.clientID)
 	data.Set("device_code", deviceCode)
 	data.Set("grant_type", GrantTypeDeviceCode)
 
@@ -326,11 +325,11 @@ func (c *httpClient) pollOnce(ctx context.Context, tokenEndpoint, deviceCode str
 func (c *httpClient) Refresh(ctx context.Context, token *Token) (*TokenResponse, error) {
 	config, err := c.Discover(ctx, token.Endpoint)
 	if err != nil {
-		errors.Wrap(err, "failed to discover OIDC configuration")
+		return nil, errors.Wrap(err, "failed to discover OIDC configuration")
 	}
 
 	if config.TokenEndpoint == "" {
-		errors.New("OIDC configuration has no token endpoint")
+		return nil, errors.New("OIDC configuration has no token endpoint")
 	}
 
 	data := url.Values{}

internal/oauth/flow_test.go

@@ -555,3 +555,19 @@ func TestRefresh_Success(t *testing.T) {
 		t.Errorf("RefreshToken = %q, want %q", resp.RefreshToken, "new-refresh-token")
 	}
 }
+
+func TestRefresh_DiscoverFailure(t *testing.T) {
+	client := NewClient(DefaultClientID)
+	token := &Token{
+		Endpoint:     "http://127.0.0.1:1",
+		RefreshToken: "test-refresh-token",
+	}
+
+	_, err := client.Refresh(context.Background(), token)
+	if err == nil {
+		t.Fatal("Refresh() expected error, got nil")
+	}
+	if !strings.Contains(err.Error(), "failed to discover OIDC configuration") {
+		t.Errorf("error = %q, want discovery failure context", err.Error())
+	}
+}