feat(BRIDGE-281): disable keychain test on macOS.

Author: xmichelo

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 {
@@ -212,6 +215,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)
 	}
 
@@ -283,8 +287,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() {
@@ -548,11 +551,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 {
@@ -573,38 +576,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"
 }

internal/app/app_test.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

pkg/keychain/helper_darwin.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("")) {

pkg/keychain/helper_linux.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("")) {

pkg/keychain/helper_windows.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()

pkg/keychain/keychain.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)

pkg/keychain/keychain_test.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 {

utils/bridge-rollout/bridge-rollout.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 {