Clean up TODOs (#1752)

RELEASE_NOTES=n/a

Signed-off-by: Dominik Schulz <[email protected]>

internal/action/env_test.go

@@ -3,12 +3,14 @@ package action
 import (
 	"bytes"
 	"context"
+	"fmt"
 	"os"
 	"runtime"
 	"testing"
 
 	"github.com/gopasspw/gopass/internal/out"
 	"github.com/gopasspw/gopass/pkg/ctxutil"
+	"github.com/gopasspw/gopass/pkg/pwgen"
 	"github.com/gopasspw/gopass/tests/gptest"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -37,12 +39,13 @@ func TestEnvLeafHappyPath(t *testing.T) {
 
 	// Command-line would be: "gopass env foo env", where "foo" is an existing
 	// secret with value "secret". We expect to see the key/value in the output
-	// of the /usr/bin/env utility in the form "FOO=secret".
-	//
-	// TODO(@dominikschulz): consider populating foo with a long, random password to
-	// absolutely ensure that the correct secret is displayed.
-	assert.NoError(t, act.Env(gptest.CliCtx(ctx, t, "foo", "env")))
-	assert.Contains(t, buf.String(), "FOO=secret\n")
+	// of the /usr/bin/env utility in the form "BAZ=secret".
+	pw := pwgen.GeneratePassword(24, false)
+	assert.NoError(t, act.insertStdin(ctx, "baz", []byte(pw), false))
+	buf.Reset()
+
+	assert.NoError(t, act.Env(gptest.CliCtx(ctx, t, "baz", "env")))
+	assert.Contains(t, buf.String(), fmt.Sprintf("BAZ=%s\n", pw))
 }
 
 func TestEnvSecretNotFound(t *testing.T) {

internal/action/rcs.go

@@ -38,24 +38,20 @@ func (s *Action) RCSInit(c *cli.Context) error {
 }
 
 func (s *Action) rcsInit(ctx context.Context, store, un, ue string) error {
-	// TODO this is a hack, we just skip rcsInit if the backend is FS, which
-	// doesn't support this. The check should rather ask the backend if it
-	// can be initialized for RCS.
-	if backend.GetStorageBackend(ctx) == backend.FS {
-		return nil
-	}
 	bn := backend.StorageBackendName(backend.GetStorageBackend(ctx))
-	out.Print(ctx, "Initializing git repository (%s) for %s / %s...", bn, un, ue)
-
 	userName, userEmail := s.getUserData(ctx, store, un, ue)
 	if err := s.Store.RCSInit(ctx, store, userName, userEmail); err != nil {
+		if errors.Is(err, backend.ErrNotSupported) {
+			debug.Log("RCSInit not supported for backend %s", bn)
+			return nil
+		}
 		if gtv := os.Getenv("GPG_TTY"); gtv == "" {
 			out.Print(ctx, "Git initialization failed. You may want to try to 'export GPG_TTY=$(tty)' and start over.")
 		}
 		return fmt.Errorf("failed to run git init: %w", err)
 	}
 
-	out.Print(ctx, "Git initialized")
+	out.Print(ctx, "Initialized git repository (%s) for %s / %s...", bn, un, ue)
 	return nil
 }
 

internal/action/rcs_test.go

@@ -53,10 +53,10 @@ func TestGit(t *testing.T) {
 	buf.Reset()
 
 	// GitPull
-	assert.NoError(t, act.RCSPull(c))
+	assert.Error(t, act.RCSPull(c))
 	buf.Reset()
 
 	// GitPush
-	assert.NoError(t, act.RCSPush(c))
+	assert.Error(t, act.RCSPush(c))
 	buf.Reset()
 }

internal/action/sync.go

@@ -76,7 +76,7 @@ func (s *Action) syncMount(ctx context.Context, mp string) error {
 	}
 	out.Print(ctxno, color.GreenString("[%s] ", name))
 
-	ctx, sub, err := s.Store.GetSubStore(ctx, mp)
+	sub, err := s.Store.GetSubStore(mp)
 	if err != nil {
 		out.Error(ctx, "Failed to get sub store '%s': %s", name, err)
 		return fmt.Errorf("failed to get sub stores (%s)", err)

internal/backend/crypto/age/age.go

@@ -8,6 +8,7 @@ import (
 	"path/filepath"
 	"sort"
 	"strings"
+	"time"
 
 	"filippo.io/age"
 	"filippo.io/age/agessh"
@@ -38,7 +39,7 @@ type Age struct {
 
 // New creates a new Age backend
 func New() (*Age, error) {
-	cDir, err := cache.NewOnDisk("age-github")
+	cDir, err := cache.NewOnDisk("age-github", 6*time.Hour)
 	if err != nil {
 		return nil, err
 	}

internal/backend/crypto/age/unsupported.go

@@ -9,7 +9,7 @@ import (
 	"github.com/gopasspw/gopass/pkg/debug"
 )
 
-// FindIdentities it TODO
+// FindIdentities returns all usable identities (SSH and native)
 func (a *Age) FindIdentities(ctx context.Context, keys ...string) ([]string, error) {
 	nk, err := a.getAllIdentities(ctx)
 	if err != nil {
@@ -53,27 +53,27 @@ func (a *Age) FindRecipients(ctx context.Context, keys ...string) ([]string, err
 	return append(ids, remote...), nil
 }
 
-// FormatKey is TODO
+// FormatKey returns the key id
 func (a *Age) FormatKey(ctx context.Context, id, tpl string) string {
 	return id
 }
 
-// Fingerprint return the id
+// Fingerprint returns the id
 func (a *Age) Fingerprint(ctx context.Context, id string) string {
 	return id
 }
 
-// ListRecipients is TODO
+// ListRecipients is not supported for the age backend
 func (a *Age) ListRecipients(context.Context) ([]string, error) {
 	return nil, fmt.Errorf("not implemented")
 }
 
-// ReadNamesFromKey is TODO
+// ReadNamesFromKey is not supported for the age backend
 func (a *Age) ReadNamesFromKey(ctx context.Context, buf []byte) ([]string, error) {
 	return nil, fmt.Errorf("not implemented")
 }
 
-// RecipientIDs is not supported by design
+// RecipientIDs is not supported for the age backend
 func (a *Age) RecipientIDs(ctx context.Context, buf []byte) ([]string, error) {
 	return nil, fmt.Errorf("reading recipient IDs is not supported by the age backend by design")
 }

internal/backend/crypto/gpg/cli/gpg.go

@@ -1,5 +1,4 @@
 // Package cli implements a GPG CLI crypto backend.
-// TODO(2.x) DEPRECATED and slated for removal in the 2.0.0 release.
 package cli
 
 import (

internal/backend/crypto/plain/backend.go

@@ -1,5 +1,4 @@
 // Package plain implements a plaintext backend
-// TODO(2.x) DEPRECATED and slated for removal in the 2.0.0 release.
 package plain
 
 import (

internal/backend/storage.go

@@ -9,6 +9,11 @@ import (
 	"github.com/gopasspw/gopass/pkg/debug"
 )
 
+var (
+	// ErrNotSupported is returned by backends for unsupported calls
+	ErrNotSupported = fmt.Errorf("not supported")
+)
+
 // StorageBackend is a type of storage backend
 type StorageBackend int
 

internal/backend/storage/fs/rcs.go

@@ -5,26 +5,27 @@ import (
 	"time"
 
 	"github.com/gopasspw/gopass/internal/backend"
+	"github.com/gopasspw/gopass/internal/store"
 )
 
 // Add does nothing
 func (s *Store) Add(ctx context.Context, args ...string) error {
-	return nil
+	return store.ErrGitNotInit
 }
 
 // Commit does nothing
 func (s *Store) Commit(ctx context.Context, msg string) error {
-	return nil
+	return store.ErrGitNotInit
 }
 
 // Push does nothing
 func (s *Store) Push(ctx context.Context, origin, branch string) error {
-	return nil
+	return store.ErrGitNotInit
 }
 
 // Pull does nothing
 func (s *Store) Pull(ctx context.Context, origin, branch string) error {
-	return nil
+	return store.ErrGitNotInit
 }
 
 // Cmd does nothing
@@ -34,7 +35,7 @@ func (s *Store) Cmd(ctx context.Context, name string, args ...string) error {
 
 // Init does nothing
 func (s *Store) Init(context.Context, string, string) error {
-	return nil
+	return backend.ErrNotSupported
 }
 
 // InitConfig does nothing

internal/backend/storage/fs/rcs_test.go

@@ -15,12 +15,13 @@ func TestRCS(t *testing.T) {
 	defer cleanup()
 
 	g := New(path)
-	assert.NoError(t, g.Add(ctx, "foo", "bar"))
-	assert.NoError(t, g.Commit(ctx, "foobar"))
-	assert.NoError(t, g.Push(ctx, "foo", "bar"))
-	assert.NoError(t, g.Pull(ctx, "foo", "bar"))
+	// the fs backend does not support the RCS operations
+	assert.Error(t, g.Add(ctx, "foo", "bar"))
+	assert.Error(t, g.Commit(ctx, "foobar"))
+	assert.Error(t, g.Push(ctx, "foo", "bar"))
+	assert.Error(t, g.Pull(ctx, "foo", "bar"))
 	assert.NoError(t, g.Cmd(ctx, "foo", "bar"))
-	assert.NoError(t, g.Init(ctx, "foo", "bar"))
+	assert.Error(t, g.Init(ctx, "foo", "bar"))
 	assert.NoError(t, g.InitConfig(ctx, "foo", "bar"))
 	assert.Equal(t, g.Version(ctx), semver.Version{Minor: 1})
 	assert.Equal(t, "fs", g.Name())

internal/backend/storage/fs/store.go

@@ -1,6 +1,5 @@
 // Package fs implement a password-store compatible on disk storage layout
 // with unencrypted paths.
-// TODO(2.x) DEPRECATED and slated for removal in the 2.0.0 release.
 package fs
 
 import (

internal/cache/disk.go

@@ -1,10 +1,12 @@
 package cache
 
 import (
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
 	"strings"
+	"time"
 
 	"github.com/gopasspw/gopass/pkg/appdir"
 	"github.com/gopasspw/gopass/pkg/debug"
@@ -13,18 +15,20 @@ import (
 
 // OnDisk is a simple on disk cache.
 type OnDisk struct {
+	ttl  time.Duration
 	name string
 	dir  string
 }
 
 // NewOnDisk creates a new on disk cache.
-func NewOnDisk(name string) (*OnDisk, error) {
+func NewOnDisk(name string, ttl time.Duration) (*OnDisk, error) {
 	d := filepath.Join(appdir.UserCache(), "gopass", name)
 	if err := os.MkdirAll(d, 0755); err != nil {
 		return nil, err
 	}
 	debug.Log("New on disk cache %s created at %s", name, d)
 	return &OnDisk{
+		ttl:  ttl,
 		name: name,
 		dir:  d,
 	}, nil
@@ -33,7 +37,15 @@ func NewOnDisk(name string) (*OnDisk, error) {
 // Get fetches an entry from the cache.
 func (o *OnDisk) Get(key string) ([]string, error) {
 	key = fsutil.CleanFilename(key)
-	buf, err := ioutil.ReadFile(filepath.Join(o.dir, key))
+	fn := filepath.Join(o.dir, key)
+	fi, err := os.Stat(fn)
+	if err != nil {
+		return nil, err
+	}
+	if time.Now().After(fi.ModTime().Add(o.ttl)) {
+		return nil, fmt.Errorf("expired")
+	}
+	buf, err := ioutil.ReadFile(fn)
 	if err != nil {
 		return nil, err
 	}

internal/cache/disk_test.go

@@ -4,6 +4,7 @@ import (
 	"io/ioutil"
 	"os"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -23,7 +24,7 @@ func TestOnDisk(t *testing.T) {
 		os.Setenv("GOPASS_HOMEDIR", ogh)
 	}()
 
-	odc, err := NewOnDisk("test")
+	odc, err := NewOnDisk("test", time.Hour)
 	assert.NoError(t, err)
 
 	assert.NoError(t, odc.Set("foo", []string{"bar"}))

internal/config/location.go

@@ -59,7 +59,7 @@ func PwStoreDir(mount string) string {
 		cleanName := strings.Replace(mount, string(filepath.Separator), "-", -1)
 		return fsutil.CleanPath(filepath.Join(appdir.UserData(), "stores", cleanName))
 	}
-	// TODO(2.x): PASSWORD_STORE_DIR support is deprecated
+	// PASSWORD_STORE_DIR support is discouraged
 	if d := os.Getenv("PASSWORD_STORE_DIR"); d != "" {
 		return fsutil.CleanPath(d)
 	}

internal/store/leaf/move.go

@@ -87,10 +87,10 @@ func (s *Store) delete(ctx context.Context, name string, recurse bool) error {
 	}
 
 	if err := s.storage.Commit(ctx, fmt.Sprintf("Remove %s from store.", name)); err != nil {
-		switch errors.Unwrap(err) {
-		case store.ErrGitNotInit:
+		switch {
+		case errors.Is(err, store.ErrGitNotInit):
 			debug.Log("skipping git commit - git not initialized")
-		case store.ErrGitNothingToCommit:
+		case errors.Is(err, store.ErrGitNothingToCommit):
 			debug.Log("skipping git commit - nothing to commit")
 		default:
 			return fmt.Errorf("failed to commit changes to git: %w", err)

internal/store/leaf/rcs_test.go

@@ -31,8 +31,9 @@ func TestGit(t *testing.T) {
 	assert.NoError(t, s.Storage().InitConfig(ctx, "foo", "[email protected]"))
 	assert.Equal(t, semver.Version{Minor: 1}, s.Storage().Version(ctx))
 	assert.NoError(t, s.Storage().AddRemote(ctx, "foo", "bar"))
-	assert.NoError(t, s.Storage().Pull(ctx, "origin", "master"))
-	assert.NoError(t, s.Storage().Push(ctx, "origin", "master"))
+	// RCS ops not supported by the fs backend
+	assert.Error(t, s.Storage().Pull(ctx, "origin", "master"))
+	assert.Error(t, s.Storage().Push(ctx, "origin", "master"))
 
 	assert.NoError(t, s.GitInit(ctx))
 	assert.NoError(t, s.GitInit(backend.WithStorageBackend(ctx, backend.FS)))

internal/store/leaf/reencrypt.go

@@ -103,10 +103,10 @@ func (s *Store) reencrypt(ctx context.Context) error {
 	}
 
 	if err := s.storage.Commit(ctx, ctxutil.GetCommitMessage(ctx)); err != nil {
-		switch errors.Unwrap(err) {
-		case store.ErrGitNotInit:
+		switch {
+		case errors.Is(err, store.ErrGitNotInit):
 			debug.Log("skipping git commit - git not initialized")
-		case store.ErrGitNothingToCommit:
+		case errors.Is(err, store.ErrGitNothingToCommit):
 			debug.Log("skipping git commit - nothing to commit")
 		default:
 			return fmt.Errorf("failed to commit changes to git: %w", err)

internal/store/leaf/write.go

@@ -69,10 +69,10 @@ func (s *Store) Set(ctx context.Context, name string, sec gopass.Byter) error {
 
 func (s *Store) gitCommitAndPush(ctx context.Context, name string) error {
 	if err := s.storage.Commit(ctx, fmt.Sprintf("Save secret to %s: %s", name, ctxutil.GetCommitMessage(ctx))); err != nil {
-		switch errors.Unwrap(err) {
-		case store.ErrGitNotInit:
+		switch {
+		case errors.Is(err, store.ErrGitNotInit):
 			debug.Log("commitAndPush - skipping git commit - git not initialized")
-		case store.ErrGitNothingToCommit:
+		case errors.Is(err, store.ErrGitNothingToCommit):
 			debug.Log("commitAndPush - skipping git commit - nothing to commit")
 		default:
 			return fmt.Errorf("failed to commit changes to git: %w", err)

internal/store/mockstore/inmem/store.go

@@ -1,5 +1,4 @@
 // Package inmem implements an in memory storage backend for tests.
-// TODO(2.x) DEPRECATED and slated for removal in the 2.0.0 release.
 package inmem
 
 import (

internal/store/root/convert.go

@@ -10,7 +10,7 @@ import (
 // Convert will try to convert a given mount to a different set of
 // backends.
 func (r *Store) Convert(ctx context.Context, name string, cryptoBe backend.CryptoBackend, storageBe backend.StorageBackend, move bool) error {
-	_, sub, err := r.GetSubStore(ctx, name)
+	sub, err := r.GetSubStore(name)
 	if err != nil {
 		return err
 	}

internal/store/root/crypto.go

@@ -9,7 +9,7 @@ import (
 
 // Crypto returns the crypto backend
 func (r *Store) Crypto(ctx context.Context, name string) backend.Crypto {
-	_, sub, _ := r.getStore(ctx, name)
+	sub, _ := r.getStore(name)
 	if !sub.Valid() {
 		debug.Log("Sub-Store not found for %s. Returning nil crypto backend", name)
 		return nil