Report a malformed --account file instead of panicking

command/oauth/cmd.go

@@ -348,6 +348,88 @@ func (o *options) Validate() error {
 	return nil
 }
 
+// accountCredentials holds the OAuth endpoints and client credentials read from
+// an --account file.
+type accountCredentials struct {
+	authzEp      string
+	tokenEp      string
+	clientID     string
+	clientSecret string
+	issuer       string
+	do2lo        bool
+}
+
+// accountString reads a string value out of a decoded account file, naming the
+// key when it is absent or of the wrong type. The values come straight from the
+// file, so a type assertion here would panic on any partial or non-Google JSON.
+func accountString(m map[string]interface{}, key, filename string) (string, error) {
+	v, ok := m[key].(string)
+	if !ok {
+		return "", errors.Errorf("error reading %s: missing or invalid %q", filename, key)
+	}
+	return v, nil
+}
+
+// readAccountCredentials parses an --account file. It supports the "installed"
+// shape written by the Google console and Google service accounts.
+func readAccountCredentials(filename string) (*accountCredentials, error) {
+	b, err := os.ReadFile(filename)
+	if err != nil {
+		return nil, errors.Wrapf(err, "error reading account from %s", filename)
+	}
+
+	account := make(map[string]interface{})
+	if err := json.Unmarshal(b, &account); err != nil {
+		return nil, errors.Wrapf(err, "error reading %s: unsupported format", filename)
+	}
+
+	creds := &accountCredentials{}
+	if _, ok := account["installed"]; ok {
+		details, ok := account["installed"].(map[string]interface{})
+		if !ok {
+			return nil, errors.Errorf("error reading %s: %q must be an object", filename, "installed")
+		}
+		for _, f := range []struct {
+			dst *string
+			key string
+		}{
+			{&creds.authzEp, "auth_uri"},
+			{&creds.tokenEp, "token_uri"},
+			{&creds.clientID, "client_id"},
+			{&creds.clientSecret, "client_secret"},
+		} {
+			if *f.dst, err = accountString(details, f.key, filename); err != nil {
+				return nil, err
+			}
+		}
+		return creds, nil
+	}
+
+	if accountType, ok := account["type"]; ok && accountType == "service_account" {
+		for _, f := range []struct {
+			dst *string
+			key string
+		}{
+			{&creds.authzEp, "auth_uri"},
+			{&creds.tokenEp, "token_uri"},
+			{&creds.clientID, "private_key_id"},
+			{&creds.clientSecret, "private_key"},
+			{&creds.issuer, "client_email"},
+		} {
+			if *f.dst, err = accountString(account, f.key, filename); err != nil {
+				return nil, err
+			}
+		}
+		creds.do2lo = true
+		return creds, nil
+	}
+
+	// The original code wrapped a nil err here, and errors.Wrapf(nil, ...) is
+	// nil, so an unsupported file reported nothing and the flow continued with
+	// empty endpoints.
+	return nil, errors.Errorf("error reading %s: unsupported account type", filename)
+}
+
 func oauthCmd(c *cli.Context) error {
 	opts := &options{
 		Provider:            c.String("provider"),
@@ -432,32 +514,16 @@ func oauthCmd(c *cli.Context) error {
 	// This code supports Google service accounts. Probably maybe also support JWKs?
 	if c.IsSet("account") {
 		opts.Provider = ""
-		filename := c.String("account")
-		b, err := os.ReadFile(filename)
+		creds, err := readAccountCredentials(c.String("account"))
 		if err != nil {
-			return errors.Wrapf(err, "error reading account from %s", filename)
-		}
-		account := make(map[string]interface{})
-		if err = json.Unmarshal(b, &account); err != nil {
-			return errors.Wrapf(err, "error reading %s: unsupported format", filename)
-		}
-
-		if _, ok := account["installed"]; ok {
-			details := account["installed"].(map[string]interface{})
-			authzEp = details["auth_uri"].(string)
-			tokenEp = details["token_uri"].(string)
-			clientID = details["client_id"].(string)
-			clientSecret = details["client_secret"].(string)
-		} else if accountType, ok := account["type"]; ok && accountType == "service_account" {
-			authzEp = account["auth_uri"].(string)
-			tokenEp = account["token_uri"].(string)
-			clientID = account["private_key_id"].(string)
-			clientSecret = account["private_key"].(string)
-			issuer = account["client_email"].(string)
-			do2lo = true
-		} else {
-			return errors.Wrapf(err, "error reading %s: unsupported account type", filename)
+			return err
 		}
+		authzEp = creds.authzEp
+		tokenEp = creds.tokenEp
+		clientID = creds.clientID
+		clientSecret = creds.clientSecret
+		issuer = creds.issuer
+		do2lo = creds.do2lo
 	}
 
 	scope := "openid email"

command/oauth/cmd_test.go

@@ -0,0 +1,125 @@
+package oauth
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// The --account file is read and unmarshalled into map[string]interface{}, and
+// every value used to be pulled out with a bare type assertion. Each case below
+// panicked, except the unsupported-type one, which returned a nil error because
+// errors.Wrapf(nil, ...) is nil.
+func TestReadAccountCredentialsErrors(t *testing.T) {
+	dir := t.TempDir()
+
+	for _, tt := range []struct {
+		name    string
+		content string
+		wantErr string
+	}{
+		{
+			name:    "installed is not an object",
+			content: `{"installed":"notamap"}`,
+			wantErr: `"installed" must be an object`,
+		},
+		{
+			name:    "installed is missing its keys",
+			content: `{"installed":{}}`,
+			wantErr: `missing or invalid "auth_uri"`,
+		},
+		{
+			name:    "installed has a non-string value",
+			content: `{"installed":{"auth_uri":1}}`,
+			wantErr: `missing or invalid "auth_uri"`,
+		},
+		{
+			name:    "service account is missing its keys",
+			content: `{"type":"service_account"}`,
+			wantErr: `missing or invalid "auth_uri"`,
+		},
+		{
+			name:    "unsupported account type",
+			content: `{"other":1}`,
+			wantErr: "unsupported account type",
+		},
+		{
+			name:    "not json",
+			content: `not json at all`,
+			wantErr: "unsupported format",
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			path := filepath.Join(dir, strings.ReplaceAll(tt.name, " ", "_")+".json")
+			if err := os.WriteFile(path, []byte(tt.content), 0o600); err != nil {
+				t.Fatal(err)
+			}
+
+			creds, err := readAccountCredentials(path)
+			if err == nil {
+				t.Fatalf("expected an error, got credentials %+v", creds)
+			}
+			if !strings.Contains(err.Error(), tt.wantErr) {
+				t.Fatalf("expected error containing %q, got %v", tt.wantErr, err)
+			}
+		})
+	}
+}
+
+func TestReadAccountCredentialsMissingFile(t *testing.T) {
+	if _, err := readAccountCredentials(filepath.Join(t.TempDir(), "nope.json")); err == nil {
+		t.Fatal("expected an error for a missing file")
+	}
+}
+
+func TestReadAccountCredentialsInstalled(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "installed.json")
+	content := `{"installed":{"auth_uri":"https://accounts.example.com/auth",` +
+		`"token_uri":"https://oauth2.example.com/token",` +
+		`"client_id":"cid","client_secret":"secret"}}`
+	if err := os.WriteFile(path, []byte(content), 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	creds, err := readAccountCredentials(path)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if creds.authzEp != "https://accounts.example.com/auth" {
+		t.Errorf("unexpected authzEp %q", creds.authzEp)
+	}
+	if creds.tokenEp != "https://oauth2.example.com/token" {
+		t.Errorf("unexpected tokenEp %q", creds.tokenEp)
+	}
+	if creds.clientID != "cid" || creds.clientSecret != "secret" {
+		t.Errorf("unexpected client credentials %q / %q", creds.clientID, creds.clientSecret)
+	}
+	if creds.do2lo {
+		t.Error("do2lo should be false for an installed account")
+	}
+}
+
+func TestReadAccountCredentialsServiceAccount(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "sa.json")
+	content := `{"type":"service_account","auth_uri":"https://accounts.example.com/auth",` +
+		`"token_uri":"https://oauth2.example.com/token","private_key_id":"kid",` +
+		`"private_key":"pk","client_email":"svc@example.com"}`
+	if err := os.WriteFile(path, []byte(content), 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	creds, err := readAccountCredentials(path)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if creds.clientID != "kid" || creds.clientSecret != "pk" {
+		t.Errorf("unexpected client credentials %q / %q", creds.clientID, creds.clientSecret)
+	}
+	if creds.issuer != "svc@example.com" {
+		t.Errorf("unexpected issuer %q", creds.issuer)
+	}
+	if !creds.do2lo {
+		t.Error("do2lo should be true for a service account")
+	}
+}