From b2f098e5f4410fabd761ae44a6da28b6f3a2bb6b Mon Sep 17 00:00:00 2001 From: avallete Date: Tue, 10 Sep 2024 17:09:16 +0200 Subject: [PATCH 01/10] fix: logout delete all stored credentials --- internal/link/link_test.go | 2 +- internal/login/login_test.go | 2 +- internal/logout/logout.go | 9 + internal/logout/logout_test.go | 25 +- internal/projects/delete/delete.go | 2 +- internal/projects/delete/delete_test.go | 2 +- internal/unlink/unlink.go | 2 +- internal/unlink/unlink_test.go | 2 +- internal/utils/access_token.go | 2 +- internal/utils/access_token_test.go | 2 +- internal/utils/credentials/keyring/keyring.go | 50 ++++ .../credentials/keyring/keyring_darwin.go | 136 +++++++++ .../credentials/keyring/keyring_fallback.go | 27 ++ .../utils/credentials/keyring/keyring_mock.go | 71 +++++ .../credentials/keyring/keyring_mock_test.go | 118 ++++++++ .../utils/credentials/keyring/keyring_test.go | 169 ++++++++++++ .../utils/credentials/keyring/keyring_unix.go | 150 ++++++++++ .../credentials/keyring/keyring_windows.go | 98 +++++++ .../keyring/secret_service/secret_service.go | 257 ++++++++++++++++++ internal/utils/credentials/store.go | 15 +- 20 files changed, 1131 insertions(+), 10 deletions(-) create mode 100644 internal/utils/credentials/keyring/keyring.go create mode 100644 internal/utils/credentials/keyring/keyring_darwin.go create mode 100644 internal/utils/credentials/keyring/keyring_fallback.go create mode 100644 internal/utils/credentials/keyring/keyring_mock.go create mode 100644 internal/utils/credentials/keyring/keyring_mock_test.go create mode 100644 internal/utils/credentials/keyring/keyring_test.go create mode 100644 internal/utils/credentials/keyring/keyring_unix.go create mode 100644 internal/utils/credentials/keyring/keyring_windows.go create mode 100644 internal/utils/credentials/keyring/secret_service/secret_service.go diff --git a/internal/link/link_test.go b/internal/link/link_test.go index f33961293..c8824c50e 100644 --- a/internal/link/link_test.go +++ b/internal/link/link_test.go @@ -15,11 +15,11 @@ import ( "github.com/supabase/cli/internal/testing/fstest" "github.com/supabase/cli/internal/testing/helper" "github.com/supabase/cli/internal/utils" + "github.com/supabase/cli/internal/utils/credentials/keyring" "github.com/supabase/cli/internal/utils/tenant" "github.com/supabase/cli/pkg/api" "github.com/supabase/cli/pkg/migration" "github.com/supabase/cli/pkg/pgtest" - "github.com/zalando/go-keyring" ) var dbConfig = pgconn.Config{ diff --git a/internal/login/login_test.go b/internal/login/login_test.go index faf351e99..0194467be 100644 --- a/internal/login/login_test.go +++ b/internal/login/login_test.go @@ -15,7 +15,7 @@ import ( "github.com/supabase/cli/internal/testing/apitest" "github.com/supabase/cli/internal/utils" "github.com/supabase/cli/internal/utils/credentials" - "github.com/zalando/go-keyring" + "github.com/supabase/cli/internal/utils/credentials/keyring" ) type MockEncryption struct { diff --git a/internal/logout/logout.go b/internal/logout/logout.go index c117abd94..b8dca0979 100644 --- a/internal/logout/logout.go +++ b/internal/logout/logout.go @@ -8,6 +8,7 @@ import ( "github.com/go-errors/errors" "github.com/spf13/afero" "github.com/supabase/cli/internal/utils" + "github.com/supabase/cli/internal/utils/credentials" ) func Run(ctx context.Context, stdout *os.File, fsys afero.Fs) error { @@ -24,6 +25,14 @@ func Run(ctx context.Context, stdout *os.File, fsys afero.Fs) error { return err } + // Delete all credentials + if err := credentials.DeleteAll(); err != nil { + // If the credentials are not supported, we can ignore the error + if err != credentials.ErrNotSupported { + return err + } + } + fmt.Fprintln(stdout, "Access token deleted successfully. You are now logged out.") return nil } diff --git a/internal/logout/logout_test.go b/internal/logout/logout_test.go index 0bb39be5c..34ee73858 100644 --- a/internal/logout/logout_test.go +++ b/internal/logout/logout_test.go @@ -12,7 +12,7 @@ import ( "github.com/supabase/cli/internal/testing/fstest" "github.com/supabase/cli/internal/utils" "github.com/supabase/cli/internal/utils/credentials" - "github.com/zalando/go-keyring" + "github.com/supabase/cli/internal/utils/credentials/keyring" ) func TestLogoutCommand(t *testing.T) { @@ -33,6 +33,29 @@ func TestLogoutCommand(t *testing.T) { assert.Empty(t, saved) }) + t.Run("removes all Supabase CLI credentials", func(t *testing.T) { + keyring.MockInit() + require.NoError(t, credentials.Set(utils.AccessTokenKey, token)) + require.NoError(t, credentials.Set("project1", "password1")) + require.NoError(t, credentials.Set("project2", "password2")) + // Run test + err := Run(context.Background(), os.Stdout, afero.NewMemMapFs()) + // Check error + assert.NoError(t, err) + // Check that access token has been removed + saved, err := credentials.Get(utils.AccessTokenKey) + assert.NoError(t, err) + assert.Empty(t, saved) + // check that project 1 has been removed + saved, err = credentials.Get("project1") + assert.NoError(t, err) + assert.Empty(t, saved) + // check that project 2 has been removed + saved, err = credentials.Get("project2") + assert.NoError(t, err) + assert.Empty(t, saved) + }) + t.Run("skips logout by default", func(t *testing.T) { keyring.MockInit() require.NoError(t, credentials.Set(utils.AccessTokenKey, token)) diff --git a/internal/projects/delete/delete.go b/internal/projects/delete/delete.go index c8ce9d535..8859c389d 100644 --- a/internal/projects/delete/delete.go +++ b/internal/projects/delete/delete.go @@ -11,7 +11,7 @@ import ( "github.com/supabase/cli/internal/unlink" "github.com/supabase/cli/internal/utils" "github.com/supabase/cli/internal/utils/credentials" - "github.com/zalando/go-keyring" + "github.com/supabase/cli/internal/utils/credentials/keyring" ) func PreRun(ctx context.Context, ref string) error { diff --git a/internal/projects/delete/delete_test.go b/internal/projects/delete/delete_test.go index b83d4cf8c..39376a83f 100644 --- a/internal/projects/delete/delete_test.go +++ b/internal/projects/delete/delete_test.go @@ -12,8 +12,8 @@ import ( "github.com/stretchr/testify/require" "github.com/supabase/cli/internal/testing/apitest" "github.com/supabase/cli/internal/utils" + "github.com/supabase/cli/internal/utils/credentials/keyring" "github.com/supabase/cli/pkg/api" - "github.com/zalando/go-keyring" ) func TestDeleteCommand(t *testing.T) { diff --git a/internal/unlink/unlink.go b/internal/unlink/unlink.go index aa0f76871..5f6f13893 100644 --- a/internal/unlink/unlink.go +++ b/internal/unlink/unlink.go @@ -9,8 +9,8 @@ import ( "github.com/spf13/afero" "github.com/supabase/cli/internal/utils" "github.com/supabase/cli/internal/utils/credentials" + "github.com/supabase/cli/internal/utils/credentials/keyring" "github.com/supabase/cli/internal/utils/flags" - "github.com/zalando/go-keyring" ) func Run(ctx context.Context, fsys afero.Fs) error { diff --git a/internal/unlink/unlink_test.go b/internal/unlink/unlink_test.go index e8bd284b6..34100722d 100644 --- a/internal/unlink/unlink_test.go +++ b/internal/unlink/unlink_test.go @@ -11,7 +11,7 @@ import ( "github.com/supabase/cli/internal/testing/apitest" "github.com/supabase/cli/internal/utils" "github.com/supabase/cli/internal/utils/credentials" - "github.com/zalando/go-keyring" + "github.com/supabase/cli/internal/utils/credentials/keyring" ) func TestUnlinkCommand(t *testing.T) { diff --git a/internal/utils/access_token.go b/internal/utils/access_token.go index c94b9de83..3120581e6 100644 --- a/internal/utils/access_token.go +++ b/internal/utils/access_token.go @@ -8,7 +8,7 @@ import ( "github.com/go-errors/errors" "github.com/spf13/afero" "github.com/supabase/cli/internal/utils/credentials" - "github.com/zalando/go-keyring" + "github.com/supabase/cli/internal/utils/credentials/keyring" ) var ( diff --git a/internal/utils/access_token_test.go b/internal/utils/access_token_test.go index 846158487..0d22d7abb 100644 --- a/internal/utils/access_token_test.go +++ b/internal/utils/access_token_test.go @@ -10,7 +10,7 @@ import ( "github.com/supabase/cli/internal/testing/apitest" "github.com/supabase/cli/internal/testing/fstest" "github.com/supabase/cli/internal/utils/credentials" - "github.com/zalando/go-keyring" + "github.com/supabase/cli/internal/utils/credentials/keyring" ) func TestLoadToken(t *testing.T) { diff --git a/internal/utils/credentials/keyring/keyring.go b/internal/utils/credentials/keyring/keyring.go new file mode 100644 index 000000000..921b51594 --- /dev/null +++ b/internal/utils/credentials/keyring/keyring.go @@ -0,0 +1,50 @@ +package keyring + +import "errors" + +// provider set in the init function by the relevant os file e.g.: +// keyring_unix.go +var provider Keyring = fallbackServiceProvider{} + +var ( + // ErrNotFound is the expected error if the secret isn't found in the + // keyring. + ErrNotFound = errors.New("secret not found in keyring") + // ErrSetDataTooBig is returned if `Set` was called with too much data. + // On MacOS: The combination of service, username & password should not exceed ~3000 bytes + // On Windows: The service is limited to 32KiB while the password is limited to 2560 bytes + // On Linux/Unix: There is no theoretical limit but performance suffers with big values (>100KiB) + ErrSetDataTooBig = errors.New("data passed to Set was too big") +) + +// Keyring provides a simple set/get interface for a keyring service. +type Keyring interface { + // Set password in keyring for user. + Set(service, user, password string) error + // Get password from keyring given service and user name. + Get(service, user string) (string, error) + // Delete secret from keyring. + Delete(service, user string) error + // DeleteAll deletes all secrets for a given service + DeleteAll(service string) error +} + +// Set password in keyring for user. +func Set(service, user, password string) error { + return provider.Set(service, user, password) +} + +// Get password from keyring given service and user name. +func Get(service, user string) (string, error) { + return provider.Get(service, user) +} + +// Delete secret from keyring. +func Delete(service, user string) error { + return provider.Delete(service, user) +} + +// DeleteAll deletes all secrets for a given service +func DeleteAll(service string) error { + return provider.DeleteAll(service) +} diff --git a/internal/utils/credentials/keyring/keyring_darwin.go b/internal/utils/credentials/keyring/keyring_darwin.go new file mode 100644 index 000000000..8e402432e --- /dev/null +++ b/internal/utils/credentials/keyring/keyring_darwin.go @@ -0,0 +1,136 @@ +// Copyright 2013 Google Inc. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package keyring + +import ( + "encoding/base64" + "encoding/hex" + "fmt" + "io" + "os/exec" + "strings" + + "github.com/alessio/shellescape" +) + +const ( + execPathKeychain = "/usr/bin/security" + + // encodingPrefix is a well-known prefix added to strings encoded by Set. + encodingPrefix = "go-keyring-encoded:" + base64EncodingPrefix = "go-keyring-base64:" +) + +type macOSXKeychain struct{} + +// func (*MacOSXKeychain) IsAvailable() bool { +// return exec.Command(execPathKeychain).Run() != exec.ErrNotFound +// } + +// Get password from macos keyring given service and user name. +func (k macOSXKeychain) Get(service, username string) (string, error) { + out, err := exec.Command( + execPathKeychain, + "find-generic-password", + "-s", service, + "-wa", username).CombinedOutput() + if err != nil { + if strings.Contains(string(out), "could not be found") { + err = ErrNotFound + } + return "", err + } + + trimStr := strings.TrimSpace(string(out[:])) + // if the string has the well-known prefix, assume it's encoded + if strings.HasPrefix(trimStr, encodingPrefix) { + dec, err := hex.DecodeString(trimStr[len(encodingPrefix):]) + return string(dec), err + } else if strings.HasPrefix(trimStr, base64EncodingPrefix) { + dec, err := base64.StdEncoding.DecodeString(trimStr[len(base64EncodingPrefix):]) + return string(dec), err + } + + return trimStr, nil +} + +// Set stores a secret in the macos keyring given a service name and a user. +func (k macOSXKeychain) Set(service, username, password string) error { + // if the added secret has multiple lines or some non ascii, + // osx will hex encode it on return. To avoid getting garbage, we + // encode all passwords + password = base64EncodingPrefix + base64.StdEncoding.EncodeToString([]byte(password)) + + cmd := exec.Command(execPathKeychain, "-i") + stdIn, err := cmd.StdinPipe() + if err != nil { + return err + } + + if err = cmd.Start(); err != nil { + return err + } + + command := fmt.Sprintf("add-generic-password -U -s %s -a %s -w %s\n", shellescape.Quote(service), shellescape.Quote(username), shellescape.Quote(password)) + if len(command) > 4096 { + return ErrSetDataTooBig + } + + if _, err := io.WriteString(stdIn, command); err != nil { + return err + } + + if err = stdIn.Close(); err != nil { + return err + } + + err = cmd.Wait() + return err +} + +// Delete deletes a secret, identified by service & user, from the keyring. +func (k macOSXKeychain) Delete(service, username string) error { + out, err := exec.Command( + execPathKeychain, + "delete-generic-password", + "-s", service, + "-a", username).CombinedOutput() + if strings.Contains(string(out), "could not be found") { + err = ErrNotFound + } + return err +} + +// DeleteAll deletes all secrets for a given service +func (k macOSXKeychain) DeleteAll(service string) error { + // Delete each secret in a while loop until there is no more left + // under the service + for { + out, err := exec.Command( + execPathKeychain, + "delete-generic-password", + "-s", service).CombinedOutput() + if strings.Contains(string(out), "could not be found") { + return nil + } else if err != nil { + return err + } + } + +} + +func init() { + provider = macOSXKeychain{} +} diff --git a/internal/utils/credentials/keyring/keyring_fallback.go b/internal/utils/credentials/keyring/keyring_fallback.go new file mode 100644 index 000000000..75c61ff1c --- /dev/null +++ b/internal/utils/credentials/keyring/keyring_fallback.go @@ -0,0 +1,27 @@ +package keyring + +import ( + "errors" + "runtime" +) + +// All of the following methods error out on unsupported platforms +var ErrUnsupportedPlatform = errors.New("unsupported platform: " + runtime.GOOS) + +type fallbackServiceProvider struct{} + +func (fallbackServiceProvider) Set(service, user, pass string) error { + return ErrUnsupportedPlatform +} + +func (fallbackServiceProvider) Get(service, user string) (string, error) { + return "", ErrUnsupportedPlatform +} + +func (fallbackServiceProvider) Delete(service, user string) error { + return ErrUnsupportedPlatform +} + +func (fallbackServiceProvider) DeleteAll(service string) error { + return ErrUnsupportedPlatform +} diff --git a/internal/utils/credentials/keyring/keyring_mock.go b/internal/utils/credentials/keyring/keyring_mock.go new file mode 100644 index 000000000..e46b2e538 --- /dev/null +++ b/internal/utils/credentials/keyring/keyring_mock.go @@ -0,0 +1,71 @@ +package keyring + +type mockProvider struct { + mockStore map[string]map[string]string + mockError error +} + +// Set stores user and pass in the keyring under the defined service +// name. +func (m *mockProvider) Set(service, user, pass string) error { + if m.mockError != nil { + return m.mockError + } + if m.mockStore == nil { + m.mockStore = make(map[string]map[string]string) + } + if m.mockStore[service] == nil { + m.mockStore[service] = make(map[string]string) + } + m.mockStore[service][user] = pass + return nil +} + +// Get gets a secret from the keyring given a service name and a user. +func (m *mockProvider) Get(service, user string) (string, error) { + if m.mockError != nil { + return "", m.mockError + } + if b, ok := m.mockStore[service]; ok { + if v, ok := b[user]; ok { + return v, nil + } + } + return "", ErrNotFound +} + +// Delete deletes a secret, identified by service & user, from the keyring. +func (m *mockProvider) Delete(service, user string) error { + if m.mockError != nil { + return m.mockError + } + if m.mockStore != nil { + if _, ok := m.mockStore[service]; ok { + if _, ok := m.mockStore[service][user]; ok { + delete(m.mockStore[service], user) + return nil + } + } + } + return ErrNotFound +} + +// DeleteAll deletes all secrets for a given service +func (m *mockProvider) DeleteAll(service string) error { + if m.mockError != nil { + return m.mockError + } + delete(m.mockStore, service) + return nil +} + +// MockInit sets the provider to a mocked memory store +func MockInit() { + provider = &mockProvider{} +} + +// MockInitWithError sets the provider to a mocked memory store +// that returns the given error on all operations +func MockInitWithError(err error) { + provider = &mockProvider{mockError: err} +} diff --git a/internal/utils/credentials/keyring/keyring_mock_test.go b/internal/utils/credentials/keyring/keyring_mock_test.go new file mode 100644 index 000000000..4688679d9 --- /dev/null +++ b/internal/utils/credentials/keyring/keyring_mock_test.go @@ -0,0 +1,118 @@ +package keyring + +import ( + "errors" + "testing" +) + +// TestSet tests setting a user and password in the keyring. +func TestMockSet(t *testing.T) { + mp := mockProvider{} + err := mp.Set(service, user, password) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } +} + +// TestGet tests getting a password from the keyring. +func TestMockGet(t *testing.T) { + mp := mockProvider{} + err := mp.Set(service, user, password) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + pw, err := mp.Get(service, user) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + if password != pw { + t.Errorf("Expected password %s, got %s", password, pw) + } +} + +// TestGetNonExisting tests getting a secret not in the keyring. +func TestMockGetNonExisting(t *testing.T) { + mp := mockProvider{} + + _, err := mp.Get(service, user+"fake") + assertError(t, err, ErrNotFound) +} + +// TestDelete tests deleting a secret from the keyring. +func TestMockDelete(t *testing.T) { + mp := mockProvider{} + + err := mp.Set(service, user, password) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + err = mp.Delete(service, user) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } +} + +// TestDeleteNonExisting tests deleting a secret not in the keyring. +func TestMockDeleteNonExisting(t *testing.T) { + mp := mockProvider{} + + err := mp.Delete(service, user+"fake") + assertError(t, err, ErrNotFound) +} + +func TestMockWithError(t *testing.T) { + mp := mockProvider{mockError: errors.New("mock error")} + + err := mp.Set(service, user, password) + assertError(t, err, mp.mockError) + + _, err = mp.Get(service, user) + assertError(t, err, mp.mockError) + + err = mp.Delete(service, user) + assertError(t, err, mp.mockError) +} + +// TestMockDeleteAll tests deleting all secrets for a given service. +func TestMockDeleteAll(t *testing.T) { + mp := mockProvider{} + + // Set up multiple secrets for the same service + err := mp.Set(service, user, password) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + err = mp.Set(service, user+"2", password+"2") + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + // Delete all secrets for the service + err = mp.DeleteAll(service) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + // Verify that all secrets for the service are deleted + _, err = mp.Get(service, user) + assertError(t, err, ErrNotFound) + + _, err = mp.Get(service, user+"2") + assertError(t, err, ErrNotFound) + + // Verify that DeleteAll on an empty service doesn't cause an error + err = mp.DeleteAll(service) + if err != nil { + t.Errorf("Should not fail on empty service, got: %s", err) + } +} + +func assertError(t *testing.T, err error, expected error) { + if err != expected { + t.Errorf("Expected error %s, got %s", expected, err) + } +} diff --git a/internal/utils/credentials/keyring/keyring_test.go b/internal/utils/credentials/keyring/keyring_test.go new file mode 100644 index 000000000..f1a8af36b --- /dev/null +++ b/internal/utils/credentials/keyring/keyring_test.go @@ -0,0 +1,169 @@ +package keyring + +import ( + "runtime" + "strings" + "testing" +) + +const ( + service = "test-service" + user = "test-user" + password = "test-password" +) + +// TestSet tests setting a user and password in the keyring. +func TestSet(t *testing.T) { + err := Set(service, user, password) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } +} + +func TestSetTooLong(t *testing.T) { + extraLongPassword := "ba" + strings.Repeat("na", 5000) + err := Set(service, user, extraLongPassword) + + if runtime.GOOS == "windows" || runtime.GOOS == "darwin" { + // should fail on those platforms + if err != ErrSetDataTooBig { + t.Errorf("Should have failed, got: %s", err) + } + } +} + +// TestGetMultiline tests getting a multi-line password from the keyring +func TestGetMultiLine(t *testing.T) { + multilinePassword := `this password +has multiple +lines and will be +encoded by some keyring implementiations +like osx` + err := Set(service, user, multilinePassword) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + pw, err := Get(service, user) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + if multilinePassword != pw { + t.Errorf("Expected password %s, got %s", multilinePassword, pw) + } +} + +// TestGetMultiline tests getting a multi-line password from the keyring +func TestGetUmlaut(t *testing.T) { + umlautPassword := "at least on OSX üöäÜÖÄß will be encoded" + err := Set(service, user, umlautPassword) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + pw, err := Get(service, user) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + if umlautPassword != pw { + t.Errorf("Expected password %s, got %s", umlautPassword, pw) + } +} + +// TestGetSingleLineHex tests getting a single line hex string password from the keyring. +func TestGetSingleLineHex(t *testing.T) { + hexPassword := "abcdef123abcdef123" + err := Set(service, user, hexPassword) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + pw, err := Get(service, user) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + if hexPassword != pw { + t.Errorf("Expected password %s, got %s", hexPassword, pw) + } +} + +// TestGet tests getting a password from the keyring. +func TestGet(t *testing.T) { + err := Set(service, user, password) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + pw, err := Get(service, user) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + if password != pw { + t.Errorf("Expected password %s, got %s", password, pw) + } +} + +// TestGetNonExisting tests getting a secret not in the keyring. +func TestGetNonExisting(t *testing.T) { + _, err := Get(service, user+"fake") + if err != ErrNotFound { + t.Errorf("Expected error ErrNotFound, got %s", err) + } +} + +// TestDelete tests deleting a secret from the keyring. +func TestDelete(t *testing.T) { + err := Delete(service, user) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } +} + +// TestDeleteNonExisting tests deleting a secret not in the keyring. +func TestDeleteNonExisting(t *testing.T) { + err := Delete(service, user+"fake") + if err != ErrNotFound { + t.Errorf("Expected error ErrNotFound, got %s", err) + } +} + +// TestDeleteAll tests deleting all secrets for a given service. +func TestDeleteAll(t *testing.T) { + // Set up multiple secrets for the same service + err := Set(service, user, password) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + err = Set(service, user+"2", password+"2") + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + // Delete all secrets for the service + err = DeleteAll(service) + if err != nil { + t.Errorf("Should not fail, got: %s", err) + } + + // Verify that all secrets for the service are deleted + _, err = Get(service, user) + if err != ErrNotFound { + t.Errorf("Expected error ErrNotFound, got %s", err) + } + + _, err = Get(service, user+"2") + if err != ErrNotFound { + t.Errorf("Expected error ErrNotFound, got %s", err) + } + + // Verify that DeleteAll on an empty service doesn't cause an error + err = DeleteAll(service) + if err != nil { + t.Errorf("Should not fail on empty service, got: %s", err) + } +} diff --git a/internal/utils/credentials/keyring/keyring_unix.go b/internal/utils/credentials/keyring/keyring_unix.go new file mode 100644 index 000000000..daa345f4b --- /dev/null +++ b/internal/utils/credentials/keyring/keyring_unix.go @@ -0,0 +1,150 @@ +//go:build (dragonfly && cgo) || (freebsd && cgo) || linux || netbsd || openbsd + +package keyring + +import ( + "fmt" + + dbus "github.com/godbus/dbus/v5" + ss "github.com/supabase/cli/internal/utils/credentials/keyring/secret_service" +) + +type secretServiceProvider struct{} + +// Set stores user and pass in the keyring under the defined service +// name. +func (s secretServiceProvider) Set(service, user, pass string) error { + svc, err := ss.NewSecretService() + if err != nil { + return err + } + + // open a session + session, err := svc.OpenSession() + if err != nil { + return err + } + defer svc.Close(session) + + attributes := map[string]string{ + "username": user, + "service": service, + } + + secret := ss.NewSecret(session.Path(), pass) + + collection := svc.GetLoginCollection() + + err = svc.Unlock(collection.Path()) + if err != nil { + return err + } + + err = svc.CreateItem(collection, + fmt.Sprintf("Password for '%s' on '%s'", user, service), + attributes, secret) + if err != nil { + return err + } + + return nil +} + +// findItem looksup an item by service and user. +func (s secretServiceProvider) findItem(svc *ss.SecretService, service, user string) (dbus.ObjectPath, error) { + collection := svc.GetLoginCollection() + + search := map[string]string{ + "username": user, + "service": service, + } + + err := svc.Unlock(collection.Path()) + if err != nil { + return "", err + } + + results, err := svc.SearchItems(collection, search) + if err != nil { + return "", err + } + + if len(results) == 0 { + return "", ErrNotFound + } + + return results[0], nil +} + +// Get gets a secret from the keyring given a service name and a user. +func (s secretServiceProvider) Get(service, user string) (string, error) { + svc, err := ss.NewSecretService() + if err != nil { + return "", err + } + + item, err := s.findItem(svc, service, user) + if err != nil { + return "", err + } + + // open a session + session, err := svc.OpenSession() + if err != nil { + return "", err + } + defer svc.Close(session) + + // unlock if invdividual item is locked + err = svc.Unlock(item) + if err != nil { + return "", err + } + + secret, err := svc.GetSecret(item, session.Path()) + if err != nil { + return "", err + } + + return string(secret.Value), nil +} + +// Delete deletes a secret, identified by service & user, from the keyring. +func (s secretServiceProvider) Delete(service, user string) error { + svc, err := ss.NewSecretService() + if err != nil { + return err + } + + item, err := s.findItem(svc, service, user) + if err != nil { + return err + } + + return svc.Delete(item) +} + +// DeleteAll deletes all secrets for a given service +func (s secretServiceProvider) DeleteAll(service string) error { + svc, err := ss.NewSecretService() + if err != nil { + return err + } + for { + item, err := s.findItem(svc, service, "") + if err != nil { + if err == ErrNotFound { + return nil + } + return err + } + err = svc.Delete(item) + if err != nil { + return err + } + } +} + +func init() { + provider = secretServiceProvider{} +} diff --git a/internal/utils/credentials/keyring/keyring_windows.go b/internal/utils/credentials/keyring/keyring_windows.go new file mode 100644 index 000000000..85641cd23 --- /dev/null +++ b/internal/utils/credentials/keyring/keyring_windows.go @@ -0,0 +1,98 @@ +package keyring + +import ( + "strings" + "syscall" + + "github.com/danieljoos/wincred" +) + +type windowsKeychain struct{} + +// Get gets a secret from the keyring given a service name and a user. +func (k windowsKeychain) Get(service, username string) (string, error) { + cred, err := wincred.GetGenericCredential(k.credName(service, username)) + if err != nil { + if err == syscall.ERROR_NOT_FOUND { + return "", ErrNotFound + } + return "", err + } + + return string(cred.CredentialBlob), nil +} + +// Set stores stores user and pass in the keyring under the defined service +// name. +func (k windowsKeychain) Set(service, username, password string) error { + // password may not exceed 2560 bytes (https://github.com/jaraco/keyring/issues/540#issuecomment-968329967) + if len(password) > 2560 { + return ErrSetDataTooBig + } + + // service may not exceed 512 bytes (might need more testing) + if len(service) >= 512 { + return ErrSetDataTooBig + } + + // service may not exceed 32k but problems occur before that + // so we limit it to 30k + if len(service) > 1024*30 { + return ErrSetDataTooBig + } + + cred := wincred.NewGenericCredential(k.credName(service, username)) + cred.UserName = username + cred.CredentialBlob = []byte(password) + return cred.Write() +} + +// Delete deletes a secret, identified by service & user, from the keyring. +func (k windowsKeychain) Delete(service, username string) error { + cred, err := wincred.GetGenericCredential(k.credName(service, username)) + if err != nil { + if err == syscall.ERROR_NOT_FOUND { + return ErrNotFound + } + return err + } + + return cred.Delete() +} + +func (k windowsKeychain) DeleteAll(service string) error { + creds, err := wincred.List() + if err != nil { + return err + } + + prefix := k.credName(service, "") + deletedCount := 0 + + for _, cred := range creds { + if strings.HasPrefix(cred.TargetName, prefix) { + genericCred, err := wincred.GetGenericCredential(cred.TargetName) + if err != nil { + if err != syscall.ERROR_NOT_FOUND { + return err + } + } else { + err := genericCred.Delete() + if err != nil { + return err + } + deletedCount++ + } + } + } + return nil +} + +// credName combines service and username to a single string. +func (k windowsKeychain) credName(service, username string) string { + return service + ":" + username +} + +func init() { + provider = windowsKeychain{} +} diff --git a/internal/utils/credentials/keyring/secret_service/secret_service.go b/internal/utils/credentials/keyring/secret_service/secret_service.go new file mode 100644 index 000000000..4acce61ba --- /dev/null +++ b/internal/utils/credentials/keyring/secret_service/secret_service.go @@ -0,0 +1,257 @@ +package ss + +import ( + "fmt" + + "errors" + + dbus "github.com/godbus/dbus/v5" +) + +const ( + serviceName = "org.freedesktop.secrets" + servicePath = "/org/freedesktop/secrets" + serviceInterface = "org.freedesktop.Secret.Service" + collectionInterface = "org.freedesktop.Secret.Collection" + collectionsInterface = "org.freedesktop.Secret.Service.Collections" + itemInterface = "org.freedesktop.Secret.Item" + sessionInterface = "org.freedesktop.Secret.Session" + promptInterface = "org.freedesktop.Secret.Prompt" + + loginCollectionAlias = "/org/freedesktop/secrets/aliases/default" + collectionBasePath = "/org/freedesktop/secrets/collection/" +) + +// Secret defines a org.freedesk.Secret.Item secret struct. +type Secret struct { + Session dbus.ObjectPath + Parameters []byte + Value []byte + ContentType string `dbus:"content_type"` +} + +// NewSecret initializes a new Secret. +func NewSecret(session dbus.ObjectPath, secret string) Secret { + return Secret{ + Session: session, + Parameters: []byte{}, + Value: []byte(secret), + ContentType: "text/plain; charset=utf8", + } +} + +// SecretService is an interface for the Secret Service dbus API. +type SecretService struct { + *dbus.Conn + object dbus.BusObject +} + +// NewSecretService inializes a new SecretService object. +func NewSecretService() (*SecretService, error) { + conn, err := dbus.SessionBus() + if err != nil { + return nil, err + } + + return &SecretService{ + conn, + conn.Object(serviceName, servicePath), + }, nil +} + +// OpenSession opens a secret service session. +func (s *SecretService) OpenSession() (dbus.BusObject, error) { + var disregard dbus.Variant + var sessionPath dbus.ObjectPath + err := s.object.Call(serviceInterface+".OpenSession", 0, "plain", dbus.MakeVariant("")).Store(&disregard, &sessionPath) + if err != nil { + return nil, err + } + + return s.Object(serviceName, sessionPath), nil +} + +// CheckCollectionPath accepts dbus path and returns nil if the path is found +// in the collection interface (and can be used). +func (s *SecretService) CheckCollectionPath(path dbus.ObjectPath) error { + obj := s.Conn.Object(serviceName, servicePath) + val, err := obj.GetProperty(collectionsInterface) + if err != nil { + return err + } + paths := val.Value().([]dbus.ObjectPath) + for _, p := range paths { + if p == path { + return nil + } + } + return errors.New("path not found") +} + +// GetCollection returns a collection from a name. +func (s *SecretService) GetCollection(name string) dbus.BusObject { + return s.Object(serviceName, dbus.ObjectPath(collectionBasePath+name)) +} + +// GetLoginCollection decides and returns the dbus collection to be used for login. +func (s *SecretService) GetLoginCollection() dbus.BusObject { + path := dbus.ObjectPath(collectionBasePath + "login") + if err := s.CheckCollectionPath(path); err != nil { + path = dbus.ObjectPath(loginCollectionAlias) + } + return s.Object(serviceName, path) +} + +// Unlock unlocks a collection. +func (s *SecretService) Unlock(collection dbus.ObjectPath) error { + var unlocked []dbus.ObjectPath + var prompt dbus.ObjectPath + err := s.object.Call(serviceInterface+".Unlock", 0, []dbus.ObjectPath{collection}).Store(&unlocked, &prompt) + if err != nil { + return err + } + + _, v, err := s.handlePrompt(prompt) + if err != nil { + return err + } + + collections := v.Value() + switch c := collections.(type) { + case []dbus.ObjectPath: + unlocked = append(unlocked, c...) + } + + if len(unlocked) != 1 || (collection != loginCollectionAlias && unlocked[0] != collection) { + return fmt.Errorf("failed to unlock correct collection '%v'", collection) + } + + return nil +} + +// Close closes a secret service dbus session. +func (s *SecretService) Close(session dbus.BusObject) error { + return session.Call(sessionInterface+".Close", 0).Err +} + +// CreateCollection with the supplied label. +func (s *SecretService) CreateCollection(label string) (dbus.BusObject, error) { + properties := map[string]dbus.Variant{ + collectionInterface + ".Label": dbus.MakeVariant(label), + } + var collection, prompt dbus.ObjectPath + err := s.object.Call(serviceInterface+".CreateCollection", 0, properties, ""). + Store(&collection, &prompt) + if err != nil { + return nil, err + } + + _, v, err := s.handlePrompt(prompt) + if err != nil { + return nil, err + } + + if v.String() != "" { + collection = dbus.ObjectPath(v.String()) + } + + return s.Object(serviceName, collection), nil +} + +// CreateItem creates an item in a collection, with label, attributes and a +// related secret. +func (s *SecretService) CreateItem(collection dbus.BusObject, label string, attributes map[string]string, secret Secret) error { + properties := map[string]dbus.Variant{ + itemInterface + ".Label": dbus.MakeVariant(label), + itemInterface + ".Attributes": dbus.MakeVariant(attributes), + } + + var item, prompt dbus.ObjectPath + err := collection.Call(collectionInterface+".CreateItem", 0, + properties, secret, true).Store(&item, &prompt) + if err != nil { + return err + } + + _, _, err = s.handlePrompt(prompt) + if err != nil { + return err + } + + return nil +} + +// handlePrompt checks if a prompt should be handles and handles it by +// triggering the prompt and waiting for the Secret service daemon to display +// the prompt to the user. +func (s *SecretService) handlePrompt(prompt dbus.ObjectPath) (bool, dbus.Variant, error) { + if prompt != dbus.ObjectPath("/") { + err := s.AddMatchSignal(dbus.WithMatchObjectPath(prompt), + dbus.WithMatchInterface(promptInterface), + ) + if err != nil { + return false, dbus.MakeVariant(""), err + } + + defer func(s *SecretService, options ...dbus.MatchOption) { + _ = s.RemoveMatchSignal(options...) + }(s, dbus.WithMatchObjectPath(prompt), dbus.WithMatchInterface(promptInterface)) + + promptSignal := make(chan *dbus.Signal, 1) + s.Signal(promptSignal) + + err = s.Object(serviceName, prompt).Call(promptInterface+".Prompt", 0, "").Err + if err != nil { + return false, dbus.MakeVariant(""), err + } + + signal := <-promptSignal + switch signal.Name { + case promptInterface + ".Completed": + dismissed := signal.Body[0].(bool) + result := signal.Body[1].(dbus.Variant) + return dismissed, result, nil + } + + } + + return false, dbus.MakeVariant(""), nil +} + +// SearchItems returns a list of items matching the search object. +func (s *SecretService) SearchItems(collection dbus.BusObject, search interface{}) ([]dbus.ObjectPath, error) { + var results []dbus.ObjectPath + err := collection.Call(collectionInterface+".SearchItems", 0, search).Store(&results) + if err != nil { + return nil, err + } + + return results, nil +} + +// GetSecret gets secret from an item in a given session. +func (s *SecretService) GetSecret(itemPath dbus.ObjectPath, session dbus.ObjectPath) (*Secret, error) { + var secret Secret + err := s.Object(serviceName, itemPath).Call(itemInterface+".GetSecret", 0, session).Store(&secret) + if err != nil { + return nil, err + } + + return &secret, nil +} + +// Delete deletes an item from the collection. +func (s *SecretService) Delete(itemPath dbus.ObjectPath) error { + var prompt dbus.ObjectPath + err := s.Object(serviceName, itemPath).Call(itemInterface+".Delete", 0).Store(&prompt) + if err != nil { + return err + } + + _, _, err = s.handlePrompt(prompt) + if err != nil { + return err + } + + return nil +} diff --git a/internal/utils/credentials/store.go b/internal/utils/credentials/store.go index 32a2abd24..591b3eef8 100644 --- a/internal/utils/credentials/store.go +++ b/internal/utils/credentials/store.go @@ -6,7 +6,7 @@ import ( "os/exec" "github.com/go-errors/errors" - "github.com/zalando/go-keyring" + "github.com/supabase/cli/internal/utils/credentials/keyring" ) const namespace = "Supabase CLI" @@ -53,6 +53,19 @@ func Delete(project string) error { return nil } +// Deletes all stored credentials for the namespace +func DeleteAll() error { + if err := assertKeyringSupported(); err != nil { + return err + } + if err := keyring.DeleteAll(namespace); errors.Is(err, exec.ErrNotFound) { + return errors.New(ErrNotSupported) + } else if err != nil { + return errors.Errorf("failed to delete all credentials: %w", err) + } + return nil +} + func assertKeyringSupported() error { // Suggested check: https://github.com/microsoft/WSL/issues/423 if f, err := os.ReadFile("/proc/sys/kernel/osrelease"); err == nil { From 173c6fc58b2d7c5db8d4e8776630921cc96bf450 Mon Sep 17 00:00:00 2001 From: avallete Date: Tue, 10 Sep 2024 17:10:26 +0200 Subject: [PATCH 02/10] chore: change logout prevention message --- internal/logout/logout.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/logout/logout.go b/internal/logout/logout.go index b8dca0979..36607d50d 100644 --- a/internal/logout/logout.go +++ b/internal/logout/logout.go @@ -12,7 +12,7 @@ import ( ) func Run(ctx context.Context, stdout *os.File, fsys afero.Fs) error { - if shouldLogout, err := utils.NewConsole().PromptYesNo(ctx, "Do you want to log out? This will remove the access token from your system.", false); err != nil { + if shouldLogout, err := utils.NewConsole().PromptYesNo(ctx, "Do you want to log out? This will remove the access token and all supabase credentials from your system.", false); err != nil { return err } else if !shouldLogout { return errors.New(context.Canceled) From 8b42663215e4cca63450bae957a32c02a9c3b9ac Mon Sep 17 00:00:00 2001 From: avallete Date: Tue, 10 Sep 2024 17:12:41 +0200 Subject: [PATCH 03/10] chore: change comment --- internal/logout/logout.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/logout/logout.go b/internal/logout/logout.go index 36607d50d..5d631de7f 100644 --- a/internal/logout/logout.go +++ b/internal/logout/logout.go @@ -25,7 +25,7 @@ func Run(ctx context.Context, stdout *os.File, fsys afero.Fs) error { return err } - // Delete all credentials + // Delete all possibles stored projects credentials if err := credentials.DeleteAll(); err != nil { // If the credentials are not supported, we can ignore the error if err != credentials.ErrNotSupported { From 173f86198c263c1b4df523eedf8fcf6ec5342d96 Mon Sep 17 00:00:00 2001 From: avallete Date: Tue, 10 Sep 2024 17:48:42 +0200 Subject: [PATCH 04/10] chore: fix lint --- internal/logout/logout.go | 11 +- internal/logout/logout_test.go | 10 +- .../credentials/keyring/keyring_darwin.go | 1 - .../utils/credentials/keyring/keyring_test.go | 162 ------------------ .../keyring/secret_service/secret_service.go | 2 + 5 files changed, 9 insertions(+), 177 deletions(-) diff --git a/internal/logout/logout.go b/internal/logout/logout.go index 5d631de7f..48560aea3 100644 --- a/internal/logout/logout.go +++ b/internal/logout/logout.go @@ -12,7 +12,7 @@ import ( ) func Run(ctx context.Context, stdout *os.File, fsys afero.Fs) error { - if shouldLogout, err := utils.NewConsole().PromptYesNo(ctx, "Do you want to log out? This will remove the access token and all supabase credentials from your system.", false); err != nil { + if shouldLogout, err := utils.NewConsole().PromptYesNo(ctx, "Do you want to log out? This will remove the access token from your system.", false); err != nil { return err } else if !shouldLogout { return errors.New(context.Canceled) @@ -25,13 +25,8 @@ func Run(ctx context.Context, stdout *os.File, fsys afero.Fs) error { return err } - // Delete all possibles stored projects credentials - if err := credentials.DeleteAll(); err != nil { - // If the credentials are not supported, we can ignore the error - if err != credentials.ErrNotSupported { - return err - } - } + // Delete all possible stored project credentials + _ = credentials.DeleteAll() fmt.Fprintln(stdout, "Access token deleted successfully. You are now logged out.") return nil diff --git a/internal/logout/logout_test.go b/internal/logout/logout_test.go index 34ee73858..07a4b72a5 100644 --- a/internal/logout/logout_test.go +++ b/internal/logout/logout_test.go @@ -38,21 +38,19 @@ func TestLogoutCommand(t *testing.T) { require.NoError(t, credentials.Set(utils.AccessTokenKey, token)) require.NoError(t, credentials.Set("project1", "password1")) require.NoError(t, credentials.Set("project2", "password2")) + t.Cleanup(fstest.MockStdin(t, "y")) // Run test err := Run(context.Background(), os.Stdout, afero.NewMemMapFs()) // Check error assert.NoError(t, err) // Check that access token has been removed - saved, err := credentials.Get(utils.AccessTokenKey) - assert.NoError(t, err) + saved, _ := credentials.Get(utils.AccessTokenKey) assert.Empty(t, saved) // check that project 1 has been removed - saved, err = credentials.Get("project1") - assert.NoError(t, err) + saved, _ = credentials.Get("project1") assert.Empty(t, saved) // check that project 2 has been removed - saved, err = credentials.Get("project2") - assert.NoError(t, err) + saved, _ = credentials.Get("project2") assert.Empty(t, saved) }) diff --git a/internal/utils/credentials/keyring/keyring_darwin.go b/internal/utils/credentials/keyring/keyring_darwin.go index 8e402432e..28625215a 100644 --- a/internal/utils/credentials/keyring/keyring_darwin.go +++ b/internal/utils/credentials/keyring/keyring_darwin.go @@ -128,7 +128,6 @@ func (k macOSXKeychain) DeleteAll(service string) error { return err } } - } func init() { diff --git a/internal/utils/credentials/keyring/keyring_test.go b/internal/utils/credentials/keyring/keyring_test.go index f1a8af36b..d98433199 100644 --- a/internal/utils/credentials/keyring/keyring_test.go +++ b/internal/utils/credentials/keyring/keyring_test.go @@ -1,169 +1,7 @@ package keyring -import ( - "runtime" - "strings" - "testing" -) - const ( service = "test-service" user = "test-user" password = "test-password" ) - -// TestSet tests setting a user and password in the keyring. -func TestSet(t *testing.T) { - err := Set(service, user, password) - if err != nil { - t.Errorf("Should not fail, got: %s", err) - } -} - -func TestSetTooLong(t *testing.T) { - extraLongPassword := "ba" + strings.Repeat("na", 5000) - err := Set(service, user, extraLongPassword) - - if runtime.GOOS == "windows" || runtime.GOOS == "darwin" { - // should fail on those platforms - if err != ErrSetDataTooBig { - t.Errorf("Should have failed, got: %s", err) - } - } -} - -// TestGetMultiline tests getting a multi-line password from the keyring -func TestGetMultiLine(t *testing.T) { - multilinePassword := `this password -has multiple -lines and will be -encoded by some keyring implementiations -like osx` - err := Set(service, user, multilinePassword) - if err != nil { - t.Errorf("Should not fail, got: %s", err) - } - - pw, err := Get(service, user) - if err != nil { - t.Errorf("Should not fail, got: %s", err) - } - - if multilinePassword != pw { - t.Errorf("Expected password %s, got %s", multilinePassword, pw) - } -} - -// TestGetMultiline tests getting a multi-line password from the keyring -func TestGetUmlaut(t *testing.T) { - umlautPassword := "at least on OSX üöäÜÖÄß will be encoded" - err := Set(service, user, umlautPassword) - if err != nil { - t.Errorf("Should not fail, got: %s", err) - } - - pw, err := Get(service, user) - if err != nil { - t.Errorf("Should not fail, got: %s", err) - } - - if umlautPassword != pw { - t.Errorf("Expected password %s, got %s", umlautPassword, pw) - } -} - -// TestGetSingleLineHex tests getting a single line hex string password from the keyring. -func TestGetSingleLineHex(t *testing.T) { - hexPassword := "abcdef123abcdef123" - err := Set(service, user, hexPassword) - if err != nil { - t.Errorf("Should not fail, got: %s", err) - } - - pw, err := Get(service, user) - if err != nil { - t.Errorf("Should not fail, got: %s", err) - } - - if hexPassword != pw { - t.Errorf("Expected password %s, got %s", hexPassword, pw) - } -} - -// TestGet tests getting a password from the keyring. -func TestGet(t *testing.T) { - err := Set(service, user, password) - if err != nil { - t.Errorf("Should not fail, got: %s", err) - } - - pw, err := Get(service, user) - if err != nil { - t.Errorf("Should not fail, got: %s", err) - } - - if password != pw { - t.Errorf("Expected password %s, got %s", password, pw) - } -} - -// TestGetNonExisting tests getting a secret not in the keyring. -func TestGetNonExisting(t *testing.T) { - _, err := Get(service, user+"fake") - if err != ErrNotFound { - t.Errorf("Expected error ErrNotFound, got %s", err) - } -} - -// TestDelete tests deleting a secret from the keyring. -func TestDelete(t *testing.T) { - err := Delete(service, user) - if err != nil { - t.Errorf("Should not fail, got: %s", err) - } -} - -// TestDeleteNonExisting tests deleting a secret not in the keyring. -func TestDeleteNonExisting(t *testing.T) { - err := Delete(service, user+"fake") - if err != ErrNotFound { - t.Errorf("Expected error ErrNotFound, got %s", err) - } -} - -// TestDeleteAll tests deleting all secrets for a given service. -func TestDeleteAll(t *testing.T) { - // Set up multiple secrets for the same service - err := Set(service, user, password) - if err != nil { - t.Errorf("Should not fail, got: %s", err) - } - - err = Set(service, user+"2", password+"2") - if err != nil { - t.Errorf("Should not fail, got: %s", err) - } - - // Delete all secrets for the service - err = DeleteAll(service) - if err != nil { - t.Errorf("Should not fail, got: %s", err) - } - - // Verify that all secrets for the service are deleted - _, err = Get(service, user) - if err != ErrNotFound { - t.Errorf("Expected error ErrNotFound, got %s", err) - } - - _, err = Get(service, user+"2") - if err != ErrNotFound { - t.Errorf("Expected error ErrNotFound, got %s", err) - } - - // Verify that DeleteAll on an empty service doesn't cause an error - err = DeleteAll(service) - if err != nil { - t.Errorf("Should not fail on empty service, got: %s", err) - } -} diff --git a/internal/utils/credentials/keyring/secret_service/secret_service.go b/internal/utils/credentials/keyring/secret_service/secret_service.go index 4acce61ba..ea6791436 100644 --- a/internal/utils/credentials/keyring/secret_service/secret_service.go +++ b/internal/utils/credentials/keyring/secret_service/secret_service.go @@ -184,6 +184,8 @@ func (s *SecretService) CreateItem(collection dbus.BusObject, label string, attr // handlePrompt checks if a prompt should be handles and handles it by // triggering the prompt and waiting for the Secret service daemon to display // the prompt to the user. +// +//nolint:all func (s *SecretService) handlePrompt(prompt dbus.ObjectPath) (bool, dbus.Variant, error) { if prompt != dbus.ObjectPath("/") { err := s.AddMatchSignal(dbus.WithMatchObjectPath(prompt), From 73065d0ec30f0a4a8cf63242caa89609d6f7bf12 Mon Sep 17 00:00:00 2001 From: avallete Date: Thu, 12 Sep 2024 09:34:21 +0200 Subject: [PATCH 05/10] chore: restore go-keyring imports --- internal/link/link_test.go | 2 +- internal/login/login_test.go | 2 +- internal/logout/logout_test.go | 2 +- internal/projects/delete/delete.go | 2 +- internal/projects/delete/delete_test.go | 2 +- internal/unlink/unlink.go | 2 +- internal/unlink/unlink_test.go | 2 +- internal/utils/access_token.go | 2 +- internal/utils/access_token_test.go | 2 +- 9 files changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/link/link_test.go b/internal/link/link_test.go index c8824c50e..f33961293 100644 --- a/internal/link/link_test.go +++ b/internal/link/link_test.go @@ -15,11 +15,11 @@ import ( "github.com/supabase/cli/internal/testing/fstest" "github.com/supabase/cli/internal/testing/helper" "github.com/supabase/cli/internal/utils" - "github.com/supabase/cli/internal/utils/credentials/keyring" "github.com/supabase/cli/internal/utils/tenant" "github.com/supabase/cli/pkg/api" "github.com/supabase/cli/pkg/migration" "github.com/supabase/cli/pkg/pgtest" + "github.com/zalando/go-keyring" ) var dbConfig = pgconn.Config{ diff --git a/internal/login/login_test.go b/internal/login/login_test.go index 0194467be..faf351e99 100644 --- a/internal/login/login_test.go +++ b/internal/login/login_test.go @@ -15,7 +15,7 @@ import ( "github.com/supabase/cli/internal/testing/apitest" "github.com/supabase/cli/internal/utils" "github.com/supabase/cli/internal/utils/credentials" - "github.com/supabase/cli/internal/utils/credentials/keyring" + "github.com/zalando/go-keyring" ) type MockEncryption struct { diff --git a/internal/logout/logout_test.go b/internal/logout/logout_test.go index 07a4b72a5..a31ee92d7 100644 --- a/internal/logout/logout_test.go +++ b/internal/logout/logout_test.go @@ -12,7 +12,7 @@ import ( "github.com/supabase/cli/internal/testing/fstest" "github.com/supabase/cli/internal/utils" "github.com/supabase/cli/internal/utils/credentials" - "github.com/supabase/cli/internal/utils/credentials/keyring" + "github.com/zalando/go-keyring" ) func TestLogoutCommand(t *testing.T) { diff --git a/internal/projects/delete/delete.go b/internal/projects/delete/delete.go index 8859c389d..c8ce9d535 100644 --- a/internal/projects/delete/delete.go +++ b/internal/projects/delete/delete.go @@ -11,7 +11,7 @@ import ( "github.com/supabase/cli/internal/unlink" "github.com/supabase/cli/internal/utils" "github.com/supabase/cli/internal/utils/credentials" - "github.com/supabase/cli/internal/utils/credentials/keyring" + "github.com/zalando/go-keyring" ) func PreRun(ctx context.Context, ref string) error { diff --git a/internal/projects/delete/delete_test.go b/internal/projects/delete/delete_test.go index 39376a83f..b83d4cf8c 100644 --- a/internal/projects/delete/delete_test.go +++ b/internal/projects/delete/delete_test.go @@ -12,8 +12,8 @@ import ( "github.com/stretchr/testify/require" "github.com/supabase/cli/internal/testing/apitest" "github.com/supabase/cli/internal/utils" - "github.com/supabase/cli/internal/utils/credentials/keyring" "github.com/supabase/cli/pkg/api" + "github.com/zalando/go-keyring" ) func TestDeleteCommand(t *testing.T) { diff --git a/internal/unlink/unlink.go b/internal/unlink/unlink.go index 5f6f13893..aa0f76871 100644 --- a/internal/unlink/unlink.go +++ b/internal/unlink/unlink.go @@ -9,8 +9,8 @@ import ( "github.com/spf13/afero" "github.com/supabase/cli/internal/utils" "github.com/supabase/cli/internal/utils/credentials" - "github.com/supabase/cli/internal/utils/credentials/keyring" "github.com/supabase/cli/internal/utils/flags" + "github.com/zalando/go-keyring" ) func Run(ctx context.Context, fsys afero.Fs) error { diff --git a/internal/unlink/unlink_test.go b/internal/unlink/unlink_test.go index 34100722d..e8bd284b6 100644 --- a/internal/unlink/unlink_test.go +++ b/internal/unlink/unlink_test.go @@ -11,7 +11,7 @@ import ( "github.com/supabase/cli/internal/testing/apitest" "github.com/supabase/cli/internal/utils" "github.com/supabase/cli/internal/utils/credentials" - "github.com/supabase/cli/internal/utils/credentials/keyring" + "github.com/zalando/go-keyring" ) func TestUnlinkCommand(t *testing.T) { diff --git a/internal/utils/access_token.go b/internal/utils/access_token.go index 3120581e6..c94b9de83 100644 --- a/internal/utils/access_token.go +++ b/internal/utils/access_token.go @@ -8,7 +8,7 @@ import ( "github.com/go-errors/errors" "github.com/spf13/afero" "github.com/supabase/cli/internal/utils/credentials" - "github.com/supabase/cli/internal/utils/credentials/keyring" + "github.com/zalando/go-keyring" ) var ( diff --git a/internal/utils/access_token_test.go b/internal/utils/access_token_test.go index 0d22d7abb..846158487 100644 --- a/internal/utils/access_token_test.go +++ b/internal/utils/access_token_test.go @@ -10,7 +10,7 @@ import ( "github.com/supabase/cli/internal/testing/apitest" "github.com/supabase/cli/internal/testing/fstest" "github.com/supabase/cli/internal/utils/credentials" - "github.com/supabase/cli/internal/utils/credentials/keyring" + "github.com/zalando/go-keyring" ) func TestLoadToken(t *testing.T) { From bdc54797f2d1646ff38450145e5ba91a30c60a38 Mon Sep 17 00:00:00 2001 From: avallete Date: Thu, 12 Sep 2024 12:21:54 +0200 Subject: [PATCH 06/10] chore: fix tests with store mock --- internal/logout/logout_test.go | 2 +- internal/utils/credentials/store.go | 63 +++++++++++++------ internal/utils/credentials/store_mock.go | 77 ++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 19 deletions(-) create mode 100644 internal/utils/credentials/store_mock.go diff --git a/internal/logout/logout_test.go b/internal/logout/logout_test.go index a31ee92d7..7131be345 100644 --- a/internal/logout/logout_test.go +++ b/internal/logout/logout_test.go @@ -34,7 +34,7 @@ func TestLogoutCommand(t *testing.T) { }) t.Run("removes all Supabase CLI credentials", func(t *testing.T) { - keyring.MockInit() + credentials.MockInit() require.NoError(t, credentials.Set(utils.AccessTokenKey, token)) require.NoError(t, credentials.Set("project1", "password1")) require.NoError(t, credentials.Set("project2", "password2")) diff --git a/internal/utils/credentials/store.go b/internal/utils/credentials/store.go index ac7f01866..f9986ae52 100644 --- a/internal/utils/credentials/store.go +++ b/internal/utils/credentials/store.go @@ -13,9 +13,21 @@ const namespace = "Supabase CLI" var ErrNotSupported = errors.New("Keyring is not supported on WSL") -// Retrieves the stored password of a project and username -func Get(project string) (string, error) { - if err := assertKeyringSupported(); err != nil { +type Store interface { + Get(project string) (string, error) + Set(project, password string) error + Delete(project string) error + DeleteAll() error + assertKeyringSupported() error +} + +type KeyringStore struct{} + +var storeProvider Store = &KeyringStore{} + +// Get retrieves the password for a project from the keyring. +func (provider *KeyringStore) Get(project string) (string, error) { + if err := provider.assertKeyringSupported(); err != nil { return "", err } val, err := keyring.Get(namespace, project) @@ -27,38 +39,53 @@ func Get(project string) (string, error) { return val, nil } -// Stores the password of a project and username -func Set(project, password string) error { - if err := assertKeyringSupported(); err != nil { +func Get(project string) (string, error) { + return storeProvider.Get(project) +} + +func (ks *KeyringStore) Set(project, password string) error { + if err := ks.assertKeyringSupported(); err != nil { return err } - if err := keyring.Set(namespace, project, password); errors.Is(err, exec.ErrNotFound) { - return errors.New(ErrNotSupported) - } else if err != nil { + if err := keyring.Set(namespace, project, password); err != nil { + if errors.Is(err, exec.ErrNotFound) { + return ErrNotSupported + } return errors.Errorf("failed to set credentials: %w", err) } return nil } -// Erases the stored password of a project and username -func Delete(project string) error { - if err := assertKeyringSupported(); err != nil { +func Set(project, password string) error { + return storeProvider.Set(project, password) +} + +func (ks *KeyringStore) Delete(project string) error { + if err := ks.assertKeyringSupported(); err != nil { return err } - if err := keyring.Delete(namespace, project); errors.Is(err, exec.ErrNotFound) { - return errors.New(ErrNotSupported) - } else if err != nil { + if err := keyring.Delete(namespace, project); err != nil { + if errors.Is(err, exec.ErrNotFound) { + return ErrNotSupported + } return errors.Errorf("failed to delete credentials: %w", err) } return nil } -// Deletes all stored credentials for the namespace -func DeleteAll() error { +func Delete(project string) error { + return storeProvider.Delete(project) +} + +func (ks *KeyringStore) DeleteAll() error { return deleteAll(namespace) } -func assertKeyringSupported() error { +func DeleteAll() error { + return storeProvider.DeleteAll() +} + +func (ks *KeyringStore) assertKeyringSupported() error { // Suggested check: https://github.com/microsoft/WSL/issues/423 if f, err := os.ReadFile("/proc/sys/kernel/osrelease"); err == nil { if bytes.Contains(f, []byte("WSL")) || bytes.Contains(f, []byte("Microsoft")) { diff --git a/internal/utils/credentials/store_mock.go b/internal/utils/credentials/store_mock.go new file mode 100644 index 000000000..8250d25b8 --- /dev/null +++ b/internal/utils/credentials/store_mock.go @@ -0,0 +1,77 @@ +package credentials + +import ( + "github.com/zalando/go-keyring" +) + +type mockProvider struct { + mockStore map[string]map[string]string + mockError error +} + +var keyringMock *mockProvider = &mockProvider{} + +// Get retrieves the password for a project from the mock store. +func (m *mockProvider) Get(project string) (string, error) { + if m.mockError != nil { + return "", m.mockError + } + if pass, ok := m.mockStore[namespace][project]; ok { + return pass, nil + } + return "", keyring.ErrNotFound +} + +// Set stores the password for a project in the mock store. +func (m *mockProvider) Set(project, password string) error { + if m.mockError != nil { + return m.mockError + } + if m.mockStore == nil { + m.mockStore = make(map[string]map[string]string) + } + if m.mockStore[namespace] == nil { + m.mockStore[namespace] = make(map[string]string) + } + m.mockStore[namespace][project] = password + return nil +} + +// Delete removes the password for a project from the mock store. +func (m *mockProvider) Delete(project string) error { + if m.mockError != nil { + return m.mockError + } + if _, ok := m.mockStore[namespace][project]; ok { + delete(m.mockStore[namespace], project) + return nil + } + return keyring.ErrNotFound +} + +// DeleteAll removes all passwords from the mock store. +func (m *mockProvider) DeleteAll() error { + if m.mockError != nil { + return m.mockError + } + delete(m.mockStore, namespace) + return nil +} + +func (m *mockProvider) assertKeyringSupported() error { + if m.mockError != nil { + return m.mockError + } + return nil +} + +func MockInit() { + keyringMock = &mockProvider{ + mockStore: make(map[string]map[string]string), + } + storeProvider = keyringMock +} + +func MockInitWithError(err error) { + keyringMock = &mockProvider{mockError: err} +} From 910640e3691e7250a9716d4d8e1bce658573d6e2 Mon Sep 17 00:00:00 2001 From: avallete Date: Thu, 12 Sep 2024 12:28:26 +0200 Subject: [PATCH 07/10] chore: fix lint --- internal/utils/credentials/store.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/utils/credentials/store.go b/internal/utils/credentials/store.go index f9986ae52..46eb0c81d 100644 --- a/internal/utils/credentials/store.go +++ b/internal/utils/credentials/store.go @@ -26,8 +26,8 @@ type KeyringStore struct{} var storeProvider Store = &KeyringStore{} // Get retrieves the password for a project from the keyring. -func (provider *KeyringStore) Get(project string) (string, error) { - if err := provider.assertKeyringSupported(); err != nil { +func (ks *KeyringStore) Get(project string) (string, error) { + if err := ks.assertKeyringSupported(); err != nil { return "", err } val, err := keyring.Get(namespace, project) From a9a09821a03cb44f2093b5ff5b67eba1c0b14ac7 Mon Sep 17 00:00:00 2001 From: avallete Date: Fri, 13 Sep 2024 10:45:55 +0200 Subject: [PATCH 08/10] fix: apply pr comments --- internal/logout/logout.go | 4 +- internal/logout/logout_test.go | 5 +- internal/utils/credentials/store.go | 15 ++--- internal/utils/credentials/store_mock.go | 71 ++++++++++-------------- 4 files changed, 45 insertions(+), 50 deletions(-) diff --git a/internal/logout/logout.go b/internal/logout/logout.go index 48560aea3..1c9ec25d3 100644 --- a/internal/logout/logout.go +++ b/internal/logout/logout.go @@ -26,7 +26,9 @@ func Run(ctx context.Context, stdout *os.File, fsys afero.Fs) error { } // Delete all possible stored project credentials - _ = credentials.DeleteAll() + if err := credentials.DeleteAll(); err != nil { + fmt.Fprintln(utils.GetDebugLogger(), err) + } fmt.Fprintln(stdout, "Access token deleted successfully. You are now logged out.") return nil diff --git a/internal/logout/logout_test.go b/internal/logout/logout_test.go index 7131be345..7ed3c8a4b 100644 --- a/internal/logout/logout_test.go +++ b/internal/logout/logout_test.go @@ -34,7 +34,10 @@ func TestLogoutCommand(t *testing.T) { }) t.Run("removes all Supabase CLI credentials", func(t *testing.T) { - credentials.MockInit() + storeProvider := &credentials.MockProvider{ + MockStore: map[string]map[string]string{}, + } + t.Cleanup(credentials.MockInit(storeProvider)) require.NoError(t, credentials.Set(utils.AccessTokenKey, token)) require.NoError(t, credentials.Set("project1", "password1")) require.NoError(t, credentials.Set("project2", "password2")) diff --git a/internal/utils/credentials/store.go b/internal/utils/credentials/store.go index 46eb0c81d..92a479085 100644 --- a/internal/utils/credentials/store.go +++ b/internal/utils/credentials/store.go @@ -14,11 +14,10 @@ const namespace = "Supabase CLI" var ErrNotSupported = errors.New("Keyring is not supported on WSL") type Store interface { - Get(project string) (string, error) - Set(project, password string) error + Get(key string) (string, error) + Set(key, value string) error Delete(project string) error DeleteAll() error - assertKeyringSupported() error } type KeyringStore struct{} @@ -27,7 +26,7 @@ var storeProvider Store = &KeyringStore{} // Get retrieves the password for a project from the keyring. func (ks *KeyringStore) Get(project string) (string, error) { - if err := ks.assertKeyringSupported(); err != nil { + if err := assertKeyringSupported(); err != nil { return "", err } val, err := keyring.Get(namespace, project) @@ -39,12 +38,14 @@ func (ks *KeyringStore) Get(project string) (string, error) { return val, nil } +// TODO: Remove global accessors (Get, Set, Delete, DeleteAll) in favor of directly using the Store interface. +// This will improve testability and dependency injection. Refactor code to pass Store instances where needed. func Get(project string) (string, error) { return storeProvider.Get(project) } func (ks *KeyringStore) Set(project, password string) error { - if err := ks.assertKeyringSupported(); err != nil { + if err := assertKeyringSupported(); err != nil { return err } if err := keyring.Set(namespace, project, password); err != nil { @@ -61,7 +62,7 @@ func Set(project, password string) error { } func (ks *KeyringStore) Delete(project string) error { - if err := ks.assertKeyringSupported(); err != nil { + if err := assertKeyringSupported(); err != nil { return err } if err := keyring.Delete(namespace, project); err != nil { @@ -85,7 +86,7 @@ func DeleteAll() error { return storeProvider.DeleteAll() } -func (ks *KeyringStore) assertKeyringSupported() error { +func assertKeyringSupported() error { // Suggested check: https://github.com/microsoft/WSL/issues/423 if f, err := os.ReadFile("/proc/sys/kernel/osrelease"); err == nil { if bytes.Contains(f, []byte("WSL")) || bytes.Contains(f, []byte("Microsoft")) { diff --git a/internal/utils/credentials/store_mock.go b/internal/utils/credentials/store_mock.go index 8250d25b8..acdf532bb 100644 --- a/internal/utils/credentials/store_mock.go +++ b/internal/utils/credentials/store_mock.go @@ -4,74 +4,63 @@ import ( "github.com/zalando/go-keyring" ) -type mockProvider struct { - mockStore map[string]map[string]string - mockError error +type MockProvider struct { + MockStore map[string]map[string]string + MockError error } -var keyringMock *mockProvider = &mockProvider{} - // Get retrieves the password for a project from the mock store. -func (m *mockProvider) Get(project string) (string, error) { - if m.mockError != nil { - return "", m.mockError +func (m *MockProvider) Get(project string) (string, error) { + if m.MockError != nil { + return "", m.MockError } - if pass, ok := m.mockStore[namespace][project]; ok { + if pass, ok := m.MockStore[namespace][project]; ok { return pass, nil } return "", keyring.ErrNotFound } // Set stores the password for a project in the mock store. -func (m *mockProvider) Set(project, password string) error { - if m.mockError != nil { - return m.mockError +func (m *MockProvider) Set(project, password string) error { + if m.MockError != nil { + return m.MockError } - if m.mockStore == nil { - m.mockStore = make(map[string]map[string]string) + if m.MockStore == nil { + m.MockStore = make(map[string]map[string]string) } - if m.mockStore[namespace] == nil { - m.mockStore[namespace] = make(map[string]string) + if m.MockStore[namespace] == nil { + m.MockStore[namespace] = make(map[string]string) } - m.mockStore[namespace][project] = password + m.MockStore[namespace][project] = password return nil } // Delete removes the password for a project from the mock store. -func (m *mockProvider) Delete(project string) error { - if m.mockError != nil { - return m.mockError +func (m *MockProvider) Delete(project string) error { + if m.MockError != nil { + return m.MockError } - if _, ok := m.mockStore[namespace][project]; ok { - delete(m.mockStore[namespace], project) + if _, ok := m.MockStore[namespace][project]; ok { + delete(m.MockStore[namespace], project) return nil } return keyring.ErrNotFound } // DeleteAll removes all passwords from the mock store. -func (m *mockProvider) DeleteAll() error { - if m.mockError != nil { - return m.mockError +func (m *MockProvider) DeleteAll() error { + if m.MockError != nil { + return m.MockError } - delete(m.mockStore, namespace) + delete(m.MockStore, namespace) return nil } -func (m *mockProvider) assertKeyringSupported() error { - if m.mockError != nil { - return m.mockError +func MockInit(mockProvider Store) func() { + oldStore := storeProvider + teardown := func() { + storeProvider = oldStore } - return nil -} - -func MockInit() { - keyringMock = &mockProvider{ - mockStore: make(map[string]map[string]string), - } - storeProvider = keyringMock -} - -func MockInitWithError(err error) { - keyringMock = &mockProvider{mockError: err} + storeProvider = mockProvider + return teardown } From 8d1b7d271b9a2137b4d55e636171b2efb77a5268 Mon Sep 17 00:00:00 2001 From: avallete Date: Fri, 13 Sep 2024 11:00:23 +0200 Subject: [PATCH 09/10] chore: refactor credentials global accessors to methods call --- internal/link/link.go | 2 +- internal/login/login_test.go | 4 ++-- internal/logout/logout.go | 2 +- internal/logout/logout_test.go | 16 ++++++++-------- internal/projects/create/create.go | 2 +- internal/projects/delete/delete.go | 2 +- internal/unlink/unlink.go | 2 +- internal/unlink/unlink_test.go | 4 ++-- internal/utils/access_token.go | 8 ++++---- internal/utils/access_token_test.go | 4 ++-- internal/utils/credentials/store.go | 20 +------------------- internal/utils/credentials/store_mock.go | 6 +++--- internal/utils/flags/db_url.go | 2 +- 13 files changed, 28 insertions(+), 46 deletions(-) diff --git a/internal/link/link.go b/internal/link/link.go index 9113f5434..3a2831070 100644 --- a/internal/link/link.go +++ b/internal/link/link.go @@ -43,7 +43,7 @@ func Run(ctx context.Context, projectRef string, fsys afero.Fs, options ...func( return err } // Save database password - if err := credentials.Set(projectRef, config.Password); err != nil { + if err := credentials.StoreProvider.Set(projectRef, config.Password); err != nil { fmt.Fprintln(os.Stderr, "Failed to save database password:", err) } } diff --git a/internal/login/login_test.go b/internal/login/login_test.go index faf351e99..758fbc5c4 100644 --- a/internal/login/login_test.go +++ b/internal/login/login_test.go @@ -40,7 +40,7 @@ func TestLoginCommand(t *testing.T) { Token: token, Fsys: afero.NewMemMapFs(), })) - saved, err := credentials.Get(utils.AccessTokenKey) + saved, err := credentials.StoreProvider.Get(utils.AccessTokenKey) assert.NoError(t, err) assert.Equal(t, token, saved) }) @@ -83,7 +83,7 @@ func TestLoginCommand(t *testing.T) { expectedBrowserUrl := fmt.Sprintf("%s/cli/login?session_id=%s&token_name=%s&public_key=%s", utils.GetSupabaseDashboardURL(), sessionId, tokenName, publicKey) assert.Contains(t, out.String(), expectedBrowserUrl) - saved, err := credentials.Get(utils.AccessTokenKey) + saved, err := credentials.StoreProvider.Get(utils.AccessTokenKey) assert.NoError(t, err) assert.Equal(t, token, saved) assert.Empty(t, apitest.ListUnmatchedRequests()) diff --git a/internal/logout/logout.go b/internal/logout/logout.go index 1c9ec25d3..abbd191b8 100644 --- a/internal/logout/logout.go +++ b/internal/logout/logout.go @@ -26,7 +26,7 @@ func Run(ctx context.Context, stdout *os.File, fsys afero.Fs) error { } // Delete all possible stored project credentials - if err := credentials.DeleteAll(); err != nil { + if err := credentials.StoreProvider.DeleteAll(); err != nil { fmt.Fprintln(utils.GetDebugLogger(), err) } diff --git a/internal/logout/logout_test.go b/internal/logout/logout_test.go index 7ed3c8a4b..4cb51021d 100644 --- a/internal/logout/logout_test.go +++ b/internal/logout/logout_test.go @@ -38,35 +38,35 @@ func TestLogoutCommand(t *testing.T) { MockStore: map[string]map[string]string{}, } t.Cleanup(credentials.MockInit(storeProvider)) - require.NoError(t, credentials.Set(utils.AccessTokenKey, token)) - require.NoError(t, credentials.Set("project1", "password1")) - require.NoError(t, credentials.Set("project2", "password2")) + require.NoError(t, credentials.StoreProvider.Set(utils.AccessTokenKey, token)) + require.NoError(t, credentials.StoreProvider.Set("project1", "password1")) + require.NoError(t, credentials.StoreProvider.Set("project2", "password2")) t.Cleanup(fstest.MockStdin(t, "y")) // Run test err := Run(context.Background(), os.Stdout, afero.NewMemMapFs()) // Check error assert.NoError(t, err) // Check that access token has been removed - saved, _ := credentials.Get(utils.AccessTokenKey) + saved, _ := credentials.StoreProvider.Get(utils.AccessTokenKey) assert.Empty(t, saved) // check that project 1 has been removed - saved, _ = credentials.Get("project1") + saved, _ = credentials.StoreProvider.Get("project1") assert.Empty(t, saved) // check that project 2 has been removed - saved, _ = credentials.Get("project2") + saved, _ = credentials.StoreProvider.Get("project2") assert.Empty(t, saved) }) t.Run("skips logout by default", func(t *testing.T) { keyring.MockInit() - require.NoError(t, credentials.Set(utils.AccessTokenKey, token)) + require.NoError(t, credentials.StoreProvider.Set(utils.AccessTokenKey, token)) // Setup in-memory fs fsys := afero.NewMemMapFs() // Run test err := Run(context.Background(), os.Stdout, fsys) // Check error assert.ErrorIs(t, err, context.Canceled) - saved, err := credentials.Get(utils.AccessTokenKey) + saved, err := credentials.StoreProvider.Get(utils.AccessTokenKey) assert.NoError(t, err) assert.Equal(t, token, saved) }) diff --git a/internal/projects/create/create.go b/internal/projects/create/create.go index 1df4b3878..fbda006eb 100644 --- a/internal/projects/create/create.go +++ b/internal/projects/create/create.go @@ -30,7 +30,7 @@ func Run(ctx context.Context, params api.V1CreateProjectBody, fsys afero.Fs) err flags.ProjectRef = resp.JSON201.Id viper.Set("DB_PASSWORD", params.DbPass) - if err := credentials.Set(flags.ProjectRef, params.DbPass); err != nil { + if err := credentials.StoreProvider.Set(flags.ProjectRef, params.DbPass); err != nil { fmt.Fprintln(os.Stderr, "Failed to save database password:", err) } diff --git a/internal/projects/delete/delete.go b/internal/projects/delete/delete.go index c8ce9d535..f042f62e1 100644 --- a/internal/projects/delete/delete.go +++ b/internal/projects/delete/delete.go @@ -43,7 +43,7 @@ func Run(ctx context.Context, ref string, fsys afero.Fs) error { } // Unlink project - if err := credentials.Delete(ref); err != nil && !errors.Is(err, keyring.ErrNotFound) { + if err := credentials.StoreProvider.Delete(ref); err != nil && !errors.Is(err, keyring.ErrNotFound) { fmt.Fprintln(os.Stderr, err) } if match, err := afero.FileContainsBytes(fsys, utils.ProjectRefPath, []byte(ref)); match { diff --git a/internal/unlink/unlink.go b/internal/unlink/unlink.go index aa0f76871..c6297581e 100644 --- a/internal/unlink/unlink.go +++ b/internal/unlink/unlink.go @@ -34,7 +34,7 @@ func Unlink(projectRef string, fsys afero.Fs) error { allErrors = append(allErrors, wrapped) } // Remove linked credentials - if err := credentials.Delete(projectRef); err != nil && + if err := credentials.StoreProvider.Delete(projectRef); err != nil && !errors.Is(err, credentials.ErrNotSupported) && !errors.Is(err, keyring.ErrNotFound) { allErrors = append(allErrors, err) diff --git a/internal/unlink/unlink_test.go b/internal/unlink/unlink_test.go index e8bd284b6..9b526e109 100644 --- a/internal/unlink/unlink_test.go +++ b/internal/unlink/unlink_test.go @@ -23,7 +23,7 @@ func TestUnlinkCommand(t *testing.T) { fsys := afero.NewMemMapFs() require.NoError(t, afero.WriteFile(fsys, utils.ProjectRefPath, []byte(project), 0644)) // Save database password - require.NoError(t, credentials.Set(project, "test")) + require.NoError(t, credentials.StoreProvider.Set(project, "test")) // Run test err := Run(context.Background(), fsys) // Check error @@ -33,7 +33,7 @@ func TestUnlinkCommand(t *testing.T) { assert.NoError(t, err) assert.False(t, exists) // Check credentials does not exist - _, err = credentials.Get(project) + _, err = credentials.StoreProvider.Get(project) assert.ErrorIs(t, err, keyring.ErrNotFound) }) diff --git a/internal/utils/access_token.go b/internal/utils/access_token.go index c94b9de83..f7ef90a01 100644 --- a/internal/utils/access_token.go +++ b/internal/utils/access_token.go @@ -41,7 +41,7 @@ func loadAccessToken(fsys afero.Fs) (string, error) { return accessToken, nil } // Load from native credentials store - if accessToken, err := credentials.Get(AccessTokenKey); err == nil { + if accessToken, err := credentials.StoreProvider.Get(AccessTokenKey); err == nil { return accessToken, nil } // Fallback to token file @@ -68,7 +68,7 @@ func SaveAccessToken(accessToken string, fsys afero.Fs) error { return errors.New(ErrInvalidToken) } // Save to native credentials store - if err := credentials.Set(AccessTokenKey, accessToken); err == nil { + if err := credentials.StoreProvider.Set(AccessTokenKey, accessToken); err == nil { return nil } // Fallback to token file @@ -94,13 +94,13 @@ func DeleteAccessToken(fsys afero.Fs) error { if err := fallbackDeleteToken(fsys); err == nil { // Typically user system should only have either token file or keyring. // But we delete from both just in case. - _ = credentials.Delete(AccessTokenKey) + _ = credentials.StoreProvider.Delete(AccessTokenKey) return nil } else if !errors.Is(err, os.ErrNotExist) { return err } // Fallback not found, delete from native credentials store - err := credentials.Delete(AccessTokenKey) + err := credentials.StoreProvider.Delete(AccessTokenKey) if errors.Is(err, credentials.ErrNotSupported) || errors.Is(err, keyring.ErrNotFound) { return errors.New(ErrNotLoggedIn) } else if err != nil { diff --git a/internal/utils/access_token_test.go b/internal/utils/access_token_test.go index 846158487..f31988876 100644 --- a/internal/utils/access_token_test.go +++ b/internal/utils/access_token_test.go @@ -165,7 +165,7 @@ func TestSaveTokenFallback(t *testing.T) { func TestDeleteToken(t *testing.T) { t.Run("deletes both keyring and fallback", func(t *testing.T) { token := string(apitest.RandomAccessToken(t)) - require.NoError(t, credentials.Set(AccessTokenKey, token)) + require.NoError(t, credentials.StoreProvider.Set(AccessTokenKey, token)) // Setup in-memory fs fsys := afero.NewMemMapFs() require.NoError(t, fallbackSaveToken(token, fsys)) @@ -173,7 +173,7 @@ func TestDeleteToken(t *testing.T) { err := DeleteAccessToken(fsys) // Check error assert.NoError(t, err) - _, err = credentials.Get(AccessTokenKey) + _, err = credentials.StoreProvider.Get(AccessTokenKey) assert.ErrorIs(t, err, keyring.ErrNotFound) path, err := getAccessTokenPath() assert.NoError(t, err) diff --git a/internal/utils/credentials/store.go b/internal/utils/credentials/store.go index 92a479085..02f97e200 100644 --- a/internal/utils/credentials/store.go +++ b/internal/utils/credentials/store.go @@ -22,7 +22,7 @@ type Store interface { type KeyringStore struct{} -var storeProvider Store = &KeyringStore{} +var StoreProvider Store = &KeyringStore{} // Get retrieves the password for a project from the keyring. func (ks *KeyringStore) Get(project string) (string, error) { @@ -38,12 +38,6 @@ func (ks *KeyringStore) Get(project string) (string, error) { return val, nil } -// TODO: Remove global accessors (Get, Set, Delete, DeleteAll) in favor of directly using the Store interface. -// This will improve testability and dependency injection. Refactor code to pass Store instances where needed. -func Get(project string) (string, error) { - return storeProvider.Get(project) -} - func (ks *KeyringStore) Set(project, password string) error { if err := assertKeyringSupported(); err != nil { return err @@ -57,10 +51,6 @@ func (ks *KeyringStore) Set(project, password string) error { return nil } -func Set(project, password string) error { - return storeProvider.Set(project, password) -} - func (ks *KeyringStore) Delete(project string) error { if err := assertKeyringSupported(); err != nil { return err @@ -74,18 +64,10 @@ func (ks *KeyringStore) Delete(project string) error { return nil } -func Delete(project string) error { - return storeProvider.Delete(project) -} - func (ks *KeyringStore) DeleteAll() error { return deleteAll(namespace) } -func DeleteAll() error { - return storeProvider.DeleteAll() -} - func assertKeyringSupported() error { // Suggested check: https://github.com/microsoft/WSL/issues/423 if f, err := os.ReadFile("/proc/sys/kernel/osrelease"); err == nil { diff --git a/internal/utils/credentials/store_mock.go b/internal/utils/credentials/store_mock.go index acdf532bb..a5c8b0e79 100644 --- a/internal/utils/credentials/store_mock.go +++ b/internal/utils/credentials/store_mock.go @@ -57,10 +57,10 @@ func (m *MockProvider) DeleteAll() error { } func MockInit(mockProvider Store) func() { - oldStore := storeProvider + oldStore := StoreProvider teardown := func() { - storeProvider = oldStore + StoreProvider = oldStore } - storeProvider = mockProvider + StoreProvider = mockProvider return teardown } diff --git a/internal/utils/flags/db_url.go b/internal/utils/flags/db_url.go index c183aa1ad..6fc912179 100644 --- a/internal/utils/flags/db_url.go +++ b/internal/utils/flags/db_url.go @@ -104,7 +104,7 @@ func getPassword(projectRef string) string { if password := viper.GetString("DB_PASSWORD"); len(password) > 0 { return password } - if password, err := credentials.Get(projectRef); err == nil { + if password, err := credentials.StoreProvider.Get(projectRef); err == nil { return password } resetUrl := fmt.Sprintf("%s/project/%s/settings/database", utils.GetSupabaseDashboardURL(), projectRef) From 6d273974480c0b1bf3724dadf6dab993b207d138 Mon Sep 17 00:00:00 2001 From: avallete Date: Fri, 13 Sep 2024 14:01:59 +0200 Subject: [PATCH 10/10] chore: init mock store in MockInit --- internal/logout/logout_test.go | 5 +-- internal/utils/credentials/store_mock.go | 54 ++++++++++++------------ 2 files changed, 29 insertions(+), 30 deletions(-) diff --git a/internal/logout/logout_test.go b/internal/logout/logout_test.go index 4cb51021d..42f9f8ead 100644 --- a/internal/logout/logout_test.go +++ b/internal/logout/logout_test.go @@ -34,10 +34,7 @@ func TestLogoutCommand(t *testing.T) { }) t.Run("removes all Supabase CLI credentials", func(t *testing.T) { - storeProvider := &credentials.MockProvider{ - MockStore: map[string]map[string]string{}, - } - t.Cleanup(credentials.MockInit(storeProvider)) + t.Cleanup(credentials.MockInit()) require.NoError(t, credentials.StoreProvider.Set(utils.AccessTokenKey, token)) require.NoError(t, credentials.StoreProvider.Set("project1", "password1")) require.NoError(t, credentials.StoreProvider.Set("project2", "password2")) diff --git a/internal/utils/credentials/store_mock.go b/internal/utils/credentials/store_mock.go index a5c8b0e79..32715fc37 100644 --- a/internal/utils/credentials/store_mock.go +++ b/internal/utils/credentials/store_mock.go @@ -4,63 +4,65 @@ import ( "github.com/zalando/go-keyring" ) -type MockProvider struct { - MockStore map[string]map[string]string - MockError error +type mockProvider struct { + mockStore map[string]map[string]string + mockError error } // Get retrieves the password for a project from the mock store. -func (m *MockProvider) Get(project string) (string, error) { - if m.MockError != nil { - return "", m.MockError +func (m *mockProvider) Get(project string) (string, error) { + if m.mockError != nil { + return "", m.mockError } - if pass, ok := m.MockStore[namespace][project]; ok { + if pass, ok := m.mockStore[namespace][project]; ok { return pass, nil } return "", keyring.ErrNotFound } // Set stores the password for a project in the mock store. -func (m *MockProvider) Set(project, password string) error { - if m.MockError != nil { - return m.MockError +func (m *mockProvider) Set(project, password string) error { + if m.mockError != nil { + return m.mockError } - if m.MockStore == nil { - m.MockStore = make(map[string]map[string]string) + if m.mockStore == nil { + m.mockStore = make(map[string]map[string]string) } - if m.MockStore[namespace] == nil { - m.MockStore[namespace] = make(map[string]string) + if m.mockStore[namespace] == nil { + m.mockStore[namespace] = make(map[string]string) } - m.MockStore[namespace][project] = password + m.mockStore[namespace][project] = password return nil } // Delete removes the password for a project from the mock store. -func (m *MockProvider) Delete(project string) error { - if m.MockError != nil { - return m.MockError +func (m *mockProvider) Delete(project string) error { + if m.mockError != nil { + return m.mockError } - if _, ok := m.MockStore[namespace][project]; ok { - delete(m.MockStore[namespace], project) + if _, ok := m.mockStore[namespace][project]; ok { + delete(m.mockStore[namespace], project) return nil } return keyring.ErrNotFound } // DeleteAll removes all passwords from the mock store. -func (m *MockProvider) DeleteAll() error { - if m.MockError != nil { - return m.MockError +func (m *mockProvider) DeleteAll() error { + if m.mockError != nil { + return m.mockError } - delete(m.MockStore, namespace) + delete(m.mockStore, namespace) return nil } -func MockInit(mockProvider Store) func() { +func MockInit() func() { oldStore := StoreProvider teardown := func() { StoreProvider = oldStore } - StoreProvider = mockProvider + StoreProvider = &mockProvider{ + mockStore: map[string]map[string]string{}, + } return teardown }