diff --git a/app/secrets/secrets.go b/app/secrets/secrets.go index 609874f..ca4b77e 100644 --- a/app/secrets/secrets.go +++ b/app/secrets/secrets.go @@ -2,6 +2,7 @@ package secrets import ( "errors" + "sync" "github.com/0skillallluck/scanline/internal/g" ) @@ -10,8 +11,14 @@ var ( ErrKeyNotFound = errors.New("key not found") ) +// getService returns the platform secrets service wrapped in an in-memory cache. +// The cache makes each unique key cost at most one keychain prompt per process, +// which matters on macOS where unsigned binaries re-prompt on every access. var getService = g.Lazy(func() Service { - return newService() + return &cachedService{ + inner: newService(), + items: make(map[string]cacheEntry), + } }) type Service interface { @@ -36,3 +43,85 @@ type ServiceError struct { func Healthcheck() *ServiceError { return getService().Available() } + +type cacheEntry struct { + item Item + found bool // false → memoized ErrKeyNotFound +} + +type cachedService struct { + inner Service + mu sync.RWMutex + items map[string]cacheEntry +} + +func (c *cachedService) Available() *ServiceError { + return c.inner.Available() +} + +func (c *cachedService) Get(key string) (Item, error) { + c.mu.RLock() + entry, ok := c.items[key] + c.mu.RUnlock() + if ok { + if !entry.found { + return Item{}, ErrKeyNotFound + } + return entry.item, nil + } + + // Call the keychain unlocked so concurrent misses for distinct keys + // (e.g. RefreshServers reading one token per account) parallelize + // instead of serializing. Concurrent misses for the same key may both + // fetch — acceptable here since no caller reads the same key twice in + // parallel. + item, err := c.inner.Get(key) + if err == ErrKeyNotFound { + c.mu.Lock() + c.items[key] = cacheEntry{found: false} + c.mu.Unlock() + return Item{}, ErrKeyNotFound + } + if err != nil { + // Transient errors may resolve on retry; caching would mask recovery. + return Item{}, err + } + c.mu.Lock() + c.items[key] = cacheEntry{item: item, found: true} + c.mu.Unlock() + return item, nil +} + +func (c *cachedService) Has(key string) (bool, error) { + _, err := c.Get(key) + if err == ErrKeyNotFound { + return false, nil + } + if err != nil { + return false, err + } + return true, nil +} + +func (c *cachedService) Set(key string, value Item) error { + if err := c.inner.Set(key, value); err != nil { + return err + } + c.mu.Lock() + c.items[key] = cacheEntry{item: value, found: true} + c.mu.Unlock() + return nil +} + +func (c *cachedService) Delete(key string) error { + err := c.inner.Delete(key) + c.mu.Lock() + if err == nil || err == ErrKeyNotFound { + c.items[key] = cacheEntry{found: false} + } else { + // Real error — invalidate so the next Get refetches. + delete(c.items, key) + } + c.mu.Unlock() + return err +} diff --git a/app/secrets/secrets_test.go b/app/secrets/secrets_test.go new file mode 100644 index 0000000..2cb33b4 --- /dev/null +++ b/app/secrets/secrets_test.go @@ -0,0 +1,206 @@ +package secrets + +import ( + "errors" + "sync" + "sync/atomic" + "testing" +) + +type fakeService struct { + mu sync.Mutex + store map[string]Item + gets int64 + sets int64 + dels int64 + getErr error // injected error for the next Get if non-nil +} + +func newFakeService() *fakeService { + return &fakeService{store: make(map[string]Item)} +} + +func (f *fakeService) Available() *ServiceError { return nil } + +func (f *fakeService) Get(key string) (Item, error) { + atomic.AddInt64(&f.gets, 1) + f.mu.Lock() + defer f.mu.Unlock() + if f.getErr != nil { + err := f.getErr + f.getErr = nil + return Item{}, err + } + item, ok := f.store[key] + if !ok { + return Item{}, ErrKeyNotFound + } + return item, nil +} + +func (f *fakeService) Has(key string) (bool, error) { + _, err := f.Get(key) + if err == ErrKeyNotFound { + return false, nil + } + return err == nil, err +} + +func (f *fakeService) Set(key string, value Item) error { + atomic.AddInt64(&f.sets, 1) + f.mu.Lock() + defer f.mu.Unlock() + f.store[key] = value + return nil +} + +func (f *fakeService) Delete(key string) error { + atomic.AddInt64(&f.dels, 1) + f.mu.Lock() + defer f.mu.Unlock() + if _, ok := f.store[key]; !ok { + return ErrKeyNotFound + } + delete(f.store, key) + return nil +} + +func newCached(inner Service) *cachedService { + return &cachedService{inner: inner, items: make(map[string]cacheEntry)} +} + +func TestCachedService_GetCachesHits(t *testing.T) { + fake := newFakeService() + fake.store["k"] = Item{Password: "v"} + cache := newCached(fake) + + for i := 0; i < 5; i++ { + got, err := cache.Get("k") + if err != nil || got.Password != "v" { + t.Fatalf("Get #%d: got=%v err=%v", i, got, err) + } + } + if got := atomic.LoadInt64(&fake.gets); got != 1 { + t.Errorf("inner.Get called %d times, want 1", got) + } +} + +func TestCachedService_GetCachesMisses(t *testing.T) { + fake := newFakeService() + cache := newCached(fake) + + for i := 0; i < 5; i++ { + _, err := cache.Get("missing") + if err != ErrKeyNotFound { + t.Fatalf("Get #%d: err=%v, want ErrKeyNotFound", i, err) + } + } + if got := atomic.LoadInt64(&fake.gets); got != 1 { + t.Errorf("inner.Get called %d times, want 1 (negative-cache memoization)", got) + } +} + +func TestCachedService_TransientErrorNotCached(t *testing.T) { + fake := newFakeService() + cache := newCached(fake) + + fake.getErr = errors.New("boom") + if _, err := cache.Get("k"); err == nil { + t.Fatal("expected error") + } + // Next call should hit the inner service again (no cached error). + fake.store["k"] = Item{Password: "v"} + got, err := cache.Get("k") + if err != nil || got.Password != "v" { + t.Fatalf("got=%v err=%v", got, err) + } + if got := atomic.LoadInt64(&fake.gets); got != 2 { + t.Errorf("inner.Get called %d times, want 2", got) + } +} + +func TestCachedService_SetIsWriteThrough(t *testing.T) { + fake := newFakeService() + cache := newCached(fake) + + if err := cache.Set("k", Item{Password: "v"}); err != nil { + t.Fatal(err) + } + if got := atomic.LoadInt64(&fake.sets); got != 1 { + t.Errorf("inner.Set called %d times, want 1", got) + } + got, err := cache.Get("k") + if err != nil || got.Password != "v" { + t.Fatalf("got=%v err=%v", got, err) + } + if got := atomic.LoadInt64(&fake.gets); got != 0 { + t.Errorf("inner.Get called %d times after Set, want 0 (Set should populate cache)", got) + } +} + +func TestCachedService_DeleteInvalidates(t *testing.T) { + fake := newFakeService() + fake.store["k"] = Item{Password: "v"} + cache := newCached(fake) + + if _, err := cache.Get("k"); err != nil { + t.Fatal(err) + } + if err := cache.Delete("k"); err != nil { + t.Fatal(err) + } + if _, err := cache.Get("k"); err != ErrKeyNotFound { + t.Fatalf("post-delete Get err=%v, want ErrKeyNotFound", err) + } + // inner.Get should only have been called once (initial); Delete memoizes + // not-found so the post-delete Get is a cache hit. + if got := atomic.LoadInt64(&fake.gets); got != 1 { + t.Errorf("inner.Get called %d times, want 1", got) + } +} + +func TestCachedService_HasUsesCache(t *testing.T) { + fake := newFakeService() + fake.store["k"] = Item{Password: "v"} + cache := newCached(fake) + + for i := 0; i < 3; i++ { + ok, err := cache.Has("k") + if err != nil || !ok { + t.Fatalf("Has #%d: ok=%v err=%v", i, ok, err) + } + } + if got := atomic.LoadInt64(&fake.gets); got != 1 { + t.Errorf("inner.Get called %d times, want 1", got) + } +} + +func TestCachedService_ConcurrentDistinctKeys(t *testing.T) { + fake := newFakeService() + for i := 0; i < 32; i++ { + fake.store[key(i)] = Item{Password: "v"} + } + cache := newCached(fake) + + var wg sync.WaitGroup + for i := 0; i < 32; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + for j := 0; j < 100; j++ { + if _, err := cache.Get(key(i)); err != nil { + t.Errorf("Get(%s): %v", key(i), err) + } + } + }(i) + } + wg.Wait() + + if got := atomic.LoadInt64(&fake.gets); got != 32 { + t.Errorf("inner.Get called %d times, want 32 (one per distinct key)", got) + } +} + +func key(i int) string { + return string(rune('a'+i%26)) + string(rune('0'+i/26)) +} diff --git a/app/secrets/service_darwin.go b/app/secrets/service_darwin.go index bd74de1..6ecfcb5 100644 --- a/app/secrets/service_darwin.go +++ b/app/secrets/service_darwin.go @@ -8,7 +8,7 @@ import ( ) const ( - serviceName = "dev.skillless.Scanless" + serviceName = "dev.skillless.Scanline" ) type serviceDarwin struct{} @@ -86,7 +86,11 @@ func (s *serviceDarwin) Set(key string, value Item) error { item.SetAccount(key) item.SetLabel(value.Label) item.SetData([]byte(value.Password)) - item.SetSynchronizable(keychain.AccessibleWhenUnlocked) + // SynchronizableNo is required — the macOS file-based keychain rejects + // SetAccessible alone with errSecParam. Net effect: device-local storage + // with access tied to keychain unlock state. + item.SetSynchronizable(keychain.SynchronizableNo) + item.SetAccessible(keychain.AccessibleWhenUnlocked) if err := keychain.AddItem(item); err == keychain.ErrorDuplicateItem { query := keychain.NewItem()