Merge pull request #241 from dropbox/bundled-team-app-keys

Bundle team app keys and fix ls error handling

Author: AndreyVMarkelov

README.md

@@ -131,17 +131,19 @@ $ dbxcli login
 Commands require saved credentials. If no saved credentials are available, run
 `dbxcli login` first or provide a token with `DBXCLI_ACCESS_TOKEN`.
 
-Personal login uses the bundled Dropbox app key by default. You can pass a
-custom app key as an option:
+Personal and team logins use bundled Dropbox app keys by default. You can pass
+a custom app key as an option:
 
 ```sh
 $ dbxcli login --app-key=your-app-key
 ```
 
-You can also set it with an environment variable:
+You can also set custom app keys with environment variables:
 
 ```sh
 $ DROPBOX_PERSONAL_APP_KEY=your-app-key dbxcli login
+$ DROPBOX_TEAM_APP_KEY=your-app-key dbxcli login team-access
+$ DROPBOX_MANAGE_APP_KEY=your-app-key dbxcli login team-manage
 ```
 
 Saved login credentials include a Dropbox refresh token and are refreshed

cmd/auth.go

@@ -108,10 +108,10 @@ func oauthConfigWithAppKey(appKey string, domain string) *oauth2.Config {
 	}
 }
 
-func (t *storedCredential) UnmarshalJSON(b []byte) error {
+func (c *storedCredential) UnmarshalJSON(b []byte) error {
 	var accessToken string
 	if err := json.Unmarshal(b, &accessToken); err == nil {
-		*t = storedCredential{AccessToken: accessToken}
+		*c = storedCredential{AccessToken: accessToken}
 		return nil
 	}
 
@@ -120,7 +120,7 @@ func (t *storedCredential) UnmarshalJSON(b []byte) error {
 	if err := json.Unmarshal(b, &credential); err != nil {
 		return err
 	}
-	*t = storedCredential(credential)
+	*c = storedCredential(credential)
 	return nil
 }
 
@@ -138,7 +138,7 @@ func storedCredentialFromOAuthToken(token *oauth2.Token, appKey string) storedCr
 	return credential
 }
 
-func (c storedCredential) oauthToken() *oauth2.Token {
+func (c *storedCredential) oauthToken() *oauth2.Token {
 	token := &oauth2.Token{
 		AccessToken:  c.AccessToken,
 		TokenType:    c.TokenType,
@@ -150,7 +150,7 @@ func (c storedCredential) oauthToken() *oauth2.Token {
 	return token
 }
 
-func (c storedCredential) shouldRefresh(now time.Time) bool {
+func (c *storedCredential) shouldRefresh(now time.Time) bool {
 	if c.RefreshToken == "" {
 		return false
 	}
@@ -253,9 +253,9 @@ func getAccessToken(tokType string, domain string, force bool) (string, string,
 func loginCommand(tokType string) string {
 	switch tokType {
 	case tokenTeamAccess:
-		return "dbxcli login team-access --app-key=<your-app-key>"
+		return "dbxcli login team-access"
 	case tokenTeamManage:
-		return "dbxcli login team-manage --app-key=<your-app-key>"
+		return "dbxcli login team-manage"
 	default:
 		return "dbxcli login"
 	}

cmd/auth_test.go

@@ -465,8 +465,8 @@ func TestLoginCommandForTokenType(t *testing.T) {
 		want    string
 	}{
 		{tokenPersonal, "dbxcli login"},
-		{tokenTeamAccess, "dbxcli login team-access --app-key=<your-app-key>"},
-		{tokenTeamManage, "dbxcli login team-manage --app-key=<your-app-key>"},
+		{tokenTeamAccess, "dbxcli login team-access"},
+		{tokenTeamManage, "dbxcli login team-manage"},
 	}
 
 	for _, tt := range tests {
@@ -543,7 +543,7 @@ func TestRequestAccessTokenReturnsReadError(t *testing.T) {
 	}
 }
 
-func TestRequestAccessTokenPromptsForAppKeyWhenUsingBundledTeamDefaults(t *testing.T) {
+func TestRequestAccessTokenUsesDefaultTeamManageAppKey(t *testing.T) {
 	restoreOAuthCredentials(t)
 	setOAuthCredentials(tokenTeamManage, defaultTeamManageAppKey)
 
@@ -555,17 +555,15 @@ func TestRequestAccessTokenPromptsForAppKeyWhenUsingBundledTeamDefaults(t *testi
 	})
 
 	readAppCredentials = func(tokType string) (appCredentials, error) {
-		if tokType != tokenTeamManage {
-			t.Fatalf("expected team manage app credentials prompt, got %q", tokType)
-		}
-		return appCredentials{Key: "prompt-key"}, nil
+		t.Fatal("app credential prompt should not be used for the default team manage app key")
+		return appCredentials{}, nil
 	}
 	readAuthorizationCode = func() (string, error) {
 		return "auth-code", nil
 	}
 	exchangeAuthorizationCode = func(ctx context.Context, conf *oauth2.Config, code string, verifier string) (*oauth2.Token, error) {
-		if conf.ClientID != "prompt-key" {
-			t.Fatalf("expected prompted app key, got %q", conf.ClientID)
+		if conf.ClientID != defaultTeamManageAppKey {
+			t.Fatalf("expected default team manage app key, got %q", conf.ClientID)
 		}
 		if conf.ClientSecret != "" {
 			t.Fatalf("expected no client secret for PKCE, got %q", conf.ClientSecret)
@@ -580,9 +578,6 @@ func TestRequestAccessTokenPromptsForAppKeyWhenUsingBundledTeamDefaults(t *testi
 	if token != "access-token" {
 		t.Fatalf("expected access token, got %q", token)
 	}
-	if teamManageAppKey != "prompt-key" {
-		t.Fatalf("expected prompted app key to be saved for this process, got %q", teamManageAppKey)
-	}
 }
 
 func TestRequestAccessTokenUsesDefaultPersonalAppKey(t *testing.T) {
@@ -724,7 +719,7 @@ func TestRequestAccessTokenUsesConfiguredAppCredentials(t *testing.T) {
 
 func TestRequestAccessTokenRejectsEmptyAppCredentials(t *testing.T) {
 	restoreOAuthCredentials(t)
-	setOAuthCredentials(tokenTeamManage, defaultTeamManageAppKey)
+	setOAuthCredentials(tokenTeamManage, "")
 
 	origReadAuthorizationCode := readAuthorizationCode
 	origExchangeAuthorizationCode := exchangeAuthorizationCode

cmd/ls.go

@@ -15,6 +15,7 @@
 package cmd
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -43,12 +44,12 @@ func getFileMetadata(c files.Client, path string) (files.IsMetadata, error) {
 
 // Invoked by search.go
 func printFolderMetadata(w io.Writer, e *files.FolderMetadata, longFormat bool) {
-	fmt.Fprint(w, formatFolderMetadata(e, longFormat))
+	_, _ = fmt.Fprint(w, formatFolderMetadata(e, longFormat))
 }
 
 // Invoked by search.go and revs.go
 func printFileMetadata(w io.Writer, e *files.FileMetadata, longFormat bool) {
-	fmt.Fprint(w, formatFileMetadata(e, longFormat))
+	_, _ = fmt.Fprint(w, formatFileMetadata(e, longFormat))
 }
 
 func formatFolderMetadata(e *files.FolderMetadata, longFormat bool) string {
@@ -80,7 +81,7 @@ func formatDeletedMetadata(e *files.DeletedMetadata, longFormat bool) string {
 	return text
 }
 
-func SetPathDisplayAsDeleted(metadata files.IsMetadata) {
+func setPathDisplayAsDeleted(metadata files.IsMetadata) {
 	switch item := metadata.(type) {
 	case *files.FileMetadata:
 		item.PathDisplay = fmt.Sprintf(deletedItemFormatString, item.PathDisplay)
@@ -127,16 +128,16 @@ func ls(cmd *cobra.Command, args []string) (err error) {
 	itemCounter := 0
 	printItem := func(message string) {
 		itemCounter = itemCounter + 1
-		fmt.Fprint(w, message)
+		_, _ = fmt.Fprint(w, message)
 		if (itemCounter%4 == 0) || opts.long {
-			fmt.Fprintln(w)
+			_, _ = fmt.Fprintln(w)
 		}
 	}
 
 	dbx := files.New(config)
 
 	if opts.long {
-		fmt.Fprint(w, "Revision\tSize\tLast modified\tPath\n")
+		_, _ = fmt.Fprint(w, "Revision\tSize\tLast modified\tPath\n")
 	}
 
 	if path != "" {
@@ -150,8 +151,7 @@ func ls(cmd *cobra.Command, args []string) (err error) {
 		case *files.FileMetadata:
 			if !onlyDeleted {
 				printItem(formatFileMetadataWithOpts(f, opts))
-				err = w.Flush()
-				return err
+				return finishListOutput(w, itemCounter, opts)
 			}
 		}
 	}
@@ -160,22 +160,14 @@ func ls(cmd *cobra.Command, args []string) (err error) {
 
 	var entries []files.IsMetadata
 	if err != nil {
-		listRevisionError, ok := err.(files.ListRevisionsAPIError)
-		if ok {
-			// Don't treat a "not_folder" error as fatal; recover by sending a
-			// get_metadata request for the same path and using that response instead.
-			if listRevisionError.EndpointError.Path.Tag == files.LookupErrorNotFolder {
-				var metaRes files.IsMetadata
-				metaRes, _ = getFileMetadata(dbx, path)
-				entries = []files.IsMetadata{metaRes}
-			} else {
-				// Return if there's an error other than "not_folder" or if the follow-up
-				// metadata request fails.
-				return err
-			}
-		} else {
+		if !isListFolderNotFolderError(err) {
 			return err
 		}
+		// Don't treat a "not_folder" error as fatal; recover by sending a
+		// get_metadata request for the same path and using that response instead.
+		var metaRes files.IsMetadata
+		metaRes, _ = getFileMetadata(dbx, path)
+		entries = []files.IsMetadata{metaRes}
 	} else {
 		entries = res.Entries
 
@@ -199,26 +191,22 @@ func ls(cmd *cobra.Command, args []string) (err error) {
 			revisionArg := files.NewListRevisionsArg(deletedItem.PathLower)
 			res, err := dbx.ListRevisions(revisionArg)
 			if err != nil {
-				listRevisionError, ok := err.(files.ListRevisionsAPIError)
-				if ok {
-					// We have a ListRevisionsAPIERror
-					if listRevisionError.EndpointError.Path.Tag == files.LookupErrorNotFile {
-						// Don't treat a "not_file" error as fatal; recover by sending a
-						// get_metadata request for the same path and using that response instead.
-						revision, err := getFileMetadata(dbx, deletedItem.PathLower)
-						if err != nil {
-							return err
-						}
-						entry = revision
+				if isListRevisionsNotFileError(err) {
+					// Don't treat a "not_file" error as fatal; recover by sending a
+					// get_metadata request for the same path and using that response instead.
+					revision, err := getFileMetadata(dbx, deletedItem.PathLower)
+					if err != nil {
+						return err
 					}
+					entry = revision
 				}
 			} else if len(res.Entries) == 0 {
 				// Occasionally revisions will be returned with an empty Revision entry list.
 				// So we just use the original entry.
 			} else {
 				entry = res.Entries[0]
 			}
-			SetPathDisplayAsDeleted(entry)
+			setPathDisplayAsDeleted(entry)
 		}
 		switch f := entry.(type) {
 		case *files.FileMetadata:
@@ -234,8 +222,30 @@ func ls(cmd *cobra.Command, args []string) (err error) {
 		}
 	}
 
-	err = w.Flush()
-	return err
+	return finishListOutput(w, itemCounter, opts)
+}
+
+func isListFolderNotFolderError(err error) bool {
+	var apiErr files.ListFolderAPIError
+	return errors.As(err, &apiErr) &&
+		apiErr.EndpointError != nil &&
+		apiErr.EndpointError.Path != nil &&
+		apiErr.EndpointError.Path.Tag == files.LookupErrorNotFolder
+}
+
+func isListRevisionsNotFileError(err error) bool {
+	var apiErr files.ListRevisionsAPIError
+	return errors.As(err, &apiErr) &&
+		apiErr.EndpointError != nil &&
+		apiErr.EndpointError.Path != nil &&
+		apiErr.EndpointError.Path.Tag == files.LookupErrorNotFile
+}
+
+func finishListOutput(w *tabwriter.Writer, itemCounter int, opts listOptions) error {
+	if itemCounter > 0 && !opts.long && itemCounter%4 != 0 {
+		_, _ = fmt.Fprintln(w)
+	}
+	return w.Flush()
 }
 
 // lsCmd represents the ls command

cmd/ls_test.go

@@ -1,7 +1,10 @@
 package cmd
 
 import (
+	"fmt"
+	"strings"
 	"testing"
+	"text/tabwriter"
 	"time"
 
 	"github.com/dropbox/dropbox-sdk-go-unofficial/v6/dropbox"
@@ -76,23 +79,23 @@ func TestSetPathDisplayAsDeleted(t *testing.T) {
 	file := &files.FileMetadata{
 		Metadata: files.Metadata{PathDisplay: "/file.txt"},
 	}
-	SetPathDisplayAsDeleted(file)
+	setPathDisplayAsDeleted(file)
 	if file.PathDisplay != "<</file.txt>>" {
 		t.Errorf("file PathDisplay = %q, want %q", file.PathDisplay, "<</file.txt>>")
 	}
 
 	folder := &files.FolderMetadata{
 		Metadata: files.Metadata{PathDisplay: "/folder"},
 	}
-	SetPathDisplayAsDeleted(folder)
+	setPathDisplayAsDeleted(folder)
 	if folder.PathDisplay != "<</folder>>" {
 		t.Errorf("folder PathDisplay = %q, want %q", folder.PathDisplay, "<</folder>>")
 	}
 
 	deleted := &files.DeletedMetadata{
 		Metadata: files.Metadata{PathDisplay: "/gone"},
 	}
-	SetPathDisplayAsDeleted(deleted)
+	setPathDisplayAsDeleted(deleted)
 	if deleted.PathDisplay != "<</gone>>" {
 		t.Errorf("deleted PathDisplay = %q, want %q", deleted.PathDisplay, "<</gone>>")
 	}
@@ -132,6 +135,51 @@ func TestGetFileMetadataNotCalledForRoot(t *testing.T) {
 	}
 }
 
+func TestFinishListOutputAddsTrailingNewlineForPartialShortRows(t *testing.T) {
+	var out strings.Builder
+	w := new(tabwriter.Writer)
+	w.Init(&out, 4, 8, 1, ' ', 0)
+
+	fmt.Fprint(w, "/one\t")
+	if err := finishListOutput(w, 1, listOptions{}); err != nil {
+		t.Fatalf("finishListOutput returned error: %v", err)
+	}
+
+	if got := out.String(); !strings.HasSuffix(got, "\n") {
+		t.Fatalf("output %q does not end with newline", got)
+	}
+}
+
+func TestIsListFolderNotFolderErrorHandlesWrappedErrors(t *testing.T) {
+	apiErr := files.ListFolderAPIError{
+		EndpointError: &files.ListFolderError{
+			Path: &files.LookupError{Tagged: dropbox.Tagged{Tag: files.LookupErrorNotFolder}},
+		},
+	}
+
+	if !isListFolderNotFolderError(apiErr) {
+		t.Fatal("expected raw list_folder not_folder error to match")
+	}
+	if !isListFolderNotFolderError(fmt.Errorf("wrapped: %w", apiErr)) {
+		t.Fatal("expected wrapped list_folder not_folder error to match")
+	}
+}
+
+func TestIsListRevisionsNotFileErrorHandlesWrappedErrors(t *testing.T) {
+	apiErr := files.ListRevisionsAPIError{
+		EndpointError: &files.ListRevisionsError{
+			Path: &files.LookupError{Tagged: dropbox.Tagged{Tag: files.LookupErrorNotFile}},
+		},
+	}
+
+	if !isListRevisionsNotFileError(apiErr) {
+		t.Fatal("expected raw list_revisions not_file error to match")
+	}
+	if !isListRevisionsNotFileError(fmt.Errorf("wrapped: %w", apiErr)) {
+		t.Fatal("expected wrapped list_revisions not_file error to match")
+	}
+}
+
 func contains(s, substr string) bool {
 	return len(s) >= len(substr) && (s == substr || len(s) > 0 && stringContains(s, substr))
 }