From 7cf3b6fb7bb20addb5f4980ff688ef93335447d5 Mon Sep 17 00:00:00 2001 From: Xavier Michelon Date: Fri, 29 Nov 2024 09:14:29 +0100 Subject: [PATCH] feat(BRIDGE-281): disable keychain test on macOS. (cherry picked from commit 3f78f4d672528cff56899b7ecfffcb71b86b5d63) --- internal/app/app.go | 49 ++++---------------- internal/app/app_test.go | 64 -------------------------- pkg/keychain/helper_darwin.go | 15 ++---- pkg/keychain/helper_linux.go | 2 +- pkg/keychain/helper_windows.go | 2 +- pkg/keychain/keychain.go | 10 ++-- pkg/keychain/keychain_test.go | 2 +- utils/bridge-rollout/bridge-rollout.go | 4 +- utils/vault-editor/main.go | 4 +- 9 files changed, 25 insertions(+), 127 deletions(-) delete mode 100644 internal/app/app_test.go diff --git a/internal/app/app.go b/internal/app/app.go index be0ce6fe..edc7697a 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -97,18 +97,21 @@ const ( appShortName = "bridge" ) +// the two flags below have been deprecated by BRIDGE-281. We however keep them so that bridge does not error if they are passed on startup. var cliFlagEnableKeychainTest = &cli.BoolFlag{ //nolint:gochecknoglobals Name: flagEnableKeychainTest, - Usage: "Enable the keychain test for the current and future executions of the application", + Usage: "This flag is deprecated and does nothing", Value: false, DisableDefaultText: true, -} //nolint:gochecknoglobals + Hidden: true, +} var cliFlagDisableKeychainTest = &cli.BoolFlag{ //nolint:gochecknoglobals Name: flagDisableKeychainTest, - Usage: "Disable the keychain test for the current and future executions of the application", + Usage: "This flag is deprecated and does nothing", Value: false, DisableDefaultText: true, + Hidden: true, } func New() *cli.App { @@ -211,6 +214,7 @@ func New() *cli.App { if onMacOS() { // The two flags below were introduced for BRIDGE-116, and are available only on macOS. + // They have been later removed fro BRIDGE-281. app.Flags = append(app.Flags, cliFlagEnableKeychainTest, cliFlagDisableKeychainTest) } @@ -282,8 +286,7 @@ func run(c *cli.Context) error { return withSingleInstance(settings, locations.GetLockFile(), version, func() error { // Look for available keychains - skipKeychainTest := checkSkipKeychainTest(c, settings) - return WithKeychainList(crashHandler, skipKeychainTest, func(keychains *keychain.List) error { + return WithKeychainList(crashHandler, func(keychains *keychain.List) error { // Unlock the encrypted vault. return WithVault(reporter, locations, keychains, crashHandler, func(v *vault.Vault, insecure, corrupt bool) error { if !v.Migrated() { @@ -547,11 +550,11 @@ func withCookieJar(vault *vault.Vault, fn func(http.CookieJar) error) error { } // WithKeychainList init the list of usable keychains. -func WithKeychainList(panicHandler async.PanicHandler, skipKeychainTest bool, fn func(*keychain.List) error) error { +func WithKeychainList(panicHandler async.PanicHandler, fn func(*keychain.List) error) error { logrus.Debug("Creating keychain list") defer logrus.Debug("Keychain list stop") defer async.HandlePanic(panicHandler) - return fn(keychain.NewList(skipKeychainTest)) + return fn(keychain.NewList()) } func setDeviceCookies(jar *cookies.Jar) error { @@ -572,38 +575,6 @@ func setDeviceCookies(jar *cookies.Jar) error { return nil } -func checkSkipKeychainTest(c *cli.Context, settingsDir string) bool { - if !onMacOS() { - return false - } - - enable := c.Bool(flagEnableKeychainTest) - disable := c.Bool(flagDisableKeychainTest) - - skip, err := vault.GetShouldSkipKeychainTest(settingsDir) - if err != nil { - logrus.WithError(err).Error("Could not load keychain settings.") - } - - if (!enable) && (!disable) { - return skip - } - - // if both switches are passed, 'enable' has priority - if disable { - skip = true - } - if enable { - skip = false - } - - if err := vault.SetShouldSkipKeychainTest(settingsDir, skip); err != nil { - logrus.WithError(err).Error("Could not save keychain settings.") - } - - return skip -} - func onMacOS() bool { return runtime.GOOS == "darwin" } diff --git a/internal/app/app_test.go b/internal/app/app_test.go deleted file mode 100644 index 88d549e4..00000000 --- a/internal/app/app_test.go +++ /dev/null @@ -1,64 +0,0 @@ -// Copyright (c) 2024 Proton AG -// -// This file is part of Proton Mail Bridge. -// -// Proton Mail Bridge is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. -// -// Proton Mail Bridge is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. -// -// You should have received a copy of the GNU General Public License -// along with Proton Mail Bridge. If not, see . - -package app - -import ( - "testing" - - "github.com/stretchr/testify/require" - "github.com/urfave/cli/v2" -) - -func TestCheckSkipKeychainTest(t *testing.T) { - var expectedResult bool - dir := t.TempDir() - app := cli.App{ - Flags: []cli.Flag{ - cliFlagEnableKeychainTest, - cliFlagDisableKeychainTest, - }, - Action: func(c *cli.Context) error { - require.Equal(t, expectedResult, checkSkipKeychainTest(c, dir)) - return nil - }, - } - - noArgs := []string{"appName"} - enableArgs := []string{"appName", "-" + flagEnableKeychainTest} - disableArgs := []string{"appName", "-" + flagDisableKeychainTest} - bothArgs := []string{"appName", "-" + flagDisableKeychainTest, "-" + flagEnableKeychainTest} - - onMac := onMacOS() - - expectedResult = false - require.NoError(t, app.Run(noArgs)) - - expectedResult = onMac - require.NoError(t, app.Run(disableArgs)) - require.NoError(t, app.Run(noArgs)) - - expectedResult = false - require.NoError(t, app.Run(enableArgs)) - require.NoError(t, app.Run(noArgs)) - - expectedResult = onMac - require.NoError(t, app.Run(disableArgs)) - - expectedResult = false - require.NoError(t, app.Run(bothArgs)) -} diff --git a/pkg/keychain/helper_darwin.go b/pkg/keychain/helper_darwin.go index 867d2fec..77df8d02 100644 --- a/pkg/keychain/helper_darwin.go +++ b/pkg/keychain/helper_darwin.go @@ -31,21 +31,12 @@ const ( MacOSKeychain = "macos-keychain" ) -func listHelpers(skipKeychainTest bool) (Helpers, string) { +func listHelpers() (Helpers, string) { helpers := make(Helpers) // MacOS always provides a keychain. - if skipKeychainTest { - logrus.WithField("pkg", "keychain").Info("Skipping macOS keychain test") - helpers[MacOSKeychain] = newMacOSHelper - } else { - if isUsable(newMacOSHelper("")) { - helpers[MacOSKeychain] = newMacOSHelper - logrus.WithField("keychain", "MacOSKeychain").Info("Keychain is usable.") - } else { - logrus.WithField("keychain", "MacOSKeychain").Debug("Keychain is not available.") - } - } + logrus.WithField("pkg", "keychain").Info("Skipping macOS keychain test") + helpers[MacOSKeychain] = newMacOSHelper // Use MacOSKeychain by default. return helpers, MacOSKeychain diff --git a/pkg/keychain/helper_linux.go b/pkg/keychain/helper_linux.go index 7a395bc6..ce531faa 100644 --- a/pkg/keychain/helper_linux.go +++ b/pkg/keychain/helper_linux.go @@ -31,7 +31,7 @@ const ( SecretServiceDBus = "secret-service-dbus" ) -func listHelpers(_ bool) (Helpers, string) { +func listHelpers() (Helpers, string) { helpers := make(Helpers) if isUsable(newDBusHelper("")) { diff --git a/pkg/keychain/helper_windows.go b/pkg/keychain/helper_windows.go index 506601d9..6ac141ff 100644 --- a/pkg/keychain/helper_windows.go +++ b/pkg/keychain/helper_windows.go @@ -25,7 +25,7 @@ import ( const WindowsCredentials = "windows-credentials" -func listHelpers(_ bool) (Helpers, string) { +func listHelpers() (Helpers, string) { helpers := make(Helpers) // Windows always provides a keychain. if isUsable(newWinCredHelper("")) { diff --git a/pkg/keychain/keychain.go b/pkg/keychain/keychain.go index d734f398..efa0236b 100644 --- a/pkg/keychain/keychain.go +++ b/pkg/keychain/keychain.go @@ -62,9 +62,9 @@ type List struct { // NewList checks availability of every keychains detected on the User Operating System // This will ask the user to unlock keychain(s) to check their usability. // This should only be called once. -func NewList(skipKeychainTest bool) *List { +func NewList() *List { var list = List{locker: &sync.Mutex{}} - list.helpers, list.defaultHelper = listHelpers(skipKeychainTest) + list.helpers, list.defaultHelper = listHelpers() return &list } @@ -210,7 +210,7 @@ func (kc *Keychain) secretURL(userID string) string { } // isUsable returns whether the credentials helper is usable. -func isUsable(helper credentials.Helper, err error) bool { +func isUsable(helper credentials.Helper, err error) bool { //nolint:unused l := logrus.WithField("helper", reflect.TypeOf(helper)) if err != nil { @@ -240,7 +240,7 @@ func isUsable(helper credentials.Helper, err error) bool { return true } -func getTestCredentials() *credentials.Credentials { +func getTestCredentials() *credentials.Credentials { //nolint:unused // On macOS, a handful of users experience failures of the test credentials. if runtime.GOOS == "darwin" { return &credentials.Credentials{ @@ -257,7 +257,7 @@ func getTestCredentials() *credentials.Credentials { } } -func retry(condition func() error) error { +func retry(condition func() error) error { //nolint:unused var maxRetry = 5 for r := 0; ; r++ { err := condition() diff --git a/pkg/keychain/keychain_test.go b/pkg/keychain/keychain_test.go index 62ee659d..becbb4d3 100644 --- a/pkg/keychain/keychain_test.go +++ b/pkg/keychain/keychain_test.go @@ -117,7 +117,7 @@ func TestInsertReadRemove(t *testing.T) { func TestIsErrKeychainNoItem(t *testing.T) { r := require.New(t) - helpers := NewList(false).GetHelpers() + helpers := NewList().GetHelpers() for helperName := range helpers { kc, err := NewKeychain(helperName, "bridge-test", helpers, helperName) diff --git a/utils/bridge-rollout/bridge-rollout.go b/utils/bridge-rollout/bridge-rollout.go index becf8b4f..fc9d4b7d 100644 --- a/utils/bridge-rollout/bridge-rollout.go +++ b/utils/bridge-rollout/bridge-rollout.go @@ -61,7 +61,7 @@ func main() { func getRollout(_ *cli.Context) error { return app.WithLocations(func(locations *locations.Locations) error { - return app.WithKeychainList(async.NoopPanicHandler{}, false, func(keychains *keychain.List) error { + return app.WithKeychainList(async.NoopPanicHandler{}, func(keychains *keychain.List) error { return app.WithVault(nil, locations, keychains, async.NoopPanicHandler{}, func(vault *vault.Vault, _, _ bool) error { fmt.Println(vault.GetUpdateRollout()) return nil @@ -72,7 +72,7 @@ func getRollout(_ *cli.Context) error { func setRollout(c *cli.Context) error { return app.WithLocations(func(locations *locations.Locations) error { - return app.WithKeychainList(async.NoopPanicHandler{}, false, func(keychains *keychain.List) error { + return app.WithKeychainList(async.NoopPanicHandler{}, func(keychains *keychain.List) error { return app.WithVault(nil, locations, keychains, async.NoopPanicHandler{}, func(vault *vault.Vault, _, _ bool) error { clamped := max(0.0, min(1.0, c.Float64("value"))) if err := vault.SetUpdateRollout(clamped); err != nil { diff --git a/utils/vault-editor/main.go b/utils/vault-editor/main.go index 91d3bfdc..8acebe9d 100644 --- a/utils/vault-editor/main.go +++ b/utils/vault-editor/main.go @@ -51,7 +51,7 @@ func main() { func readAction(c *cli.Context) error { return app.WithLocations(func(locations *locations.Locations) error { - return app.WithKeychainList(async.NoopPanicHandler{}, false, func(keychains *keychain.List) error { + return app.WithKeychainList(async.NoopPanicHandler{}, func(keychains *keychain.List) error { return app.WithVault(nil, locations, keychains, async.NoopPanicHandler{}, func(vault *vault.Vault, insecure, corrupt bool) error { if _, err := os.Stdout.Write(vault.ExportJSON()); err != nil { return fmt.Errorf("failed to write vault: %w", err) @@ -65,7 +65,7 @@ func readAction(c *cli.Context) error { func writeAction(c *cli.Context) error { return app.WithLocations(func(locations *locations.Locations) error { - return app.WithKeychainList(async.NoopPanicHandler{}, false, func(keychains *keychain.List) error { + return app.WithKeychainList(async.NoopPanicHandler{}, func(keychains *keychain.List) error { return app.WithVault(nil, locations, keychains, async.NoopPanicHandler{}, func(vault *vault.Vault, insecure, corrupt bool) error { b, err := io.ReadAll(os.Stdin) if err != nil {