From b72ba9ba01fae443fb32e2437bb9d83e3cdca986 Mon Sep 17 00:00:00 2001 From: Andrey Markelov Date: Tue, 16 Jun 2026 21:54:08 -0700 Subject: [PATCH] Bundle team app keys and fix ls error handling - Use official bundled app keys for team-access and team-manage logins, so app-key override is optional when defaults are available - Simplify app-key prompting to only ask when the effective key is empty - Fix ls error handling to use errors.As with correct Dropbox API error types: ListFolderAPIError for list-folder and ListRevisionsAPIError for list-revisions - Ensure ls short output ends with a newline for partial rows - Unexport setPathDisplayAsDeleted because it is package-local - Use pointer receivers on storedCredential methods consistently - Silence errcheck warnings on fmt.Fprint calls to tabwriter --- README.md | 8 +++-- cmd/auth.go | 14 ++++----- cmd/auth_test.go | 21 +++++-------- cmd/ls.go | 82 +++++++++++++++++++++++++++--------------------- cmd/ls_test.go | 54 +++++++++++++++++++++++++++++-- cmd/root.go | 27 ++-------------- 6 files changed, 120 insertions(+), 86 deletions(-) diff --git a/README.md b/README.md index 96215e8..0d7dc85 100644 --- a/README.md +++ b/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 diff --git a/cmd/auth.go b/cmd/auth.go index c0d9c3e..f6ee776 100644 --- a/cmd/auth.go +++ b/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=" + return "dbxcli login team-access" case tokenTeamManage: - return "dbxcli login team-manage --app-key=" + return "dbxcli login team-manage" default: return "dbxcli login" } diff --git a/cmd/auth_test.go b/cmd/auth_test.go index 1b52344..1b4e05d 100644 --- a/cmd/auth_test.go +++ b/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="}, - {tokenTeamManage, "dbxcli login team-manage --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 diff --git a/cmd/ls.go b/cmd/ls.go index 03088cc..5af2e50 100644 --- a/cmd/ls.go +++ b/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,18 +191,14 @@ 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. @@ -218,7 +206,7 @@ func ls(cmd *cobra.Command, args []string) (err error) { } 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 diff --git a/cmd/ls_test.go b/cmd/ls_test.go index 6820a9c..1642565 100644 --- a/cmd/ls_test.go +++ b/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,7 +79,7 @@ func TestSetPathDisplayAsDeleted(t *testing.T) { file := &files.FileMetadata{ Metadata: files.Metadata{PathDisplay: "/file.txt"}, } - SetPathDisplayAsDeleted(file) + setPathDisplayAsDeleted(file) if file.PathDisplay != "<>" { t.Errorf("file PathDisplay = %q, want %q", file.PathDisplay, "<>") } @@ -84,7 +87,7 @@ func TestSetPathDisplayAsDeleted(t *testing.T) { folder := &files.FolderMetadata{ Metadata: files.Metadata{PathDisplay: "/folder"}, } - SetPathDisplayAsDeleted(folder) + setPathDisplayAsDeleted(folder) if folder.PathDisplay != "<>" { t.Errorf("folder PathDisplay = %q, want %q", folder.PathDisplay, "<>") } @@ -92,7 +95,7 @@ func TestSetPathDisplayAsDeleted(t *testing.T) { deleted := &files.DeletedMetadata{ Metadata: files.Metadata{PathDisplay: "/gone"}, } - SetPathDisplayAsDeleted(deleted) + setPathDisplayAsDeleted(deleted) if deleted.PathDisplay != "<>" { t.Errorf("deleted PathDisplay = %q, want %q", deleted.PathDisplay, "<>") } @@ -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)) } diff --git a/cmd/root.go b/cmd/root.go index 93a8fd7..b103ad2 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -30,8 +30,8 @@ const ( tokenTeamManage = "teamManage" defaultPersonalAppKey = "07o23gulcj8qi69" - defaultTeamAccessAppKey = "zud1va492pnehkc" - defaultTeamManageAppKey = "xxe04eai4wmlitv" + defaultTeamAccessAppKey = "qyy1w4mbkj2wpiv" + defaultTeamManageAppKey = "sa9pv32eixm1i3p" ) func getEnv(key, fallback string) string { @@ -63,19 +63,6 @@ func oauthCredentials(tokenType string) string { } } -func defaultOAuthCredentials(tokenType string) string { - switch tokenType { - case tokenPersonal: - return defaultPersonalAppKey - case tokenTeamAccess: - return defaultTeamAccessAppKey - case tokenTeamManage: - return defaultTeamManageAppKey - default: - return "" - } -} - func setOAuthCredentials(tokenType string, appKey string) { switch tokenType { case tokenPersonal: @@ -88,15 +75,7 @@ func setOAuthCredentials(tokenType string, appKey string) { } func needsOAuthCredentialsOverride(tokenType string) bool { - appKey := oauthCredentials(tokenType) - if appKey == "" { - return true - } - if tokenType == tokenPersonal { - return false - } - defaultAppKey := defaultOAuthCredentials(tokenType) - return defaultAppKey != "" && appKey == defaultAppKey + return oauthCredentials(tokenType) == "" } func validatePath(p string) (path string, err error) {