From 478345e2778156bd2a081a43e87a93b489fd8c93 Mon Sep 17 00:00:00 2001 From: Jakub Date: Thu, 17 Mar 2022 10:57:51 +0100 Subject: [PATCH] GODT-1522: Rebuild macOS keychain notification --- internal/frontend/qml/Bridge_test.qml | 9 +++++ internal/frontend/qml/NotificationPopups.qml | 5 +++ .../qml/Notifications/Notifications.qml | 35 ++++++++++++++++++- internal/frontend/qt/frontend_events.go | 7 +++- internal/frontend/qt/qml_backend.go | 1 + internal/users/user.go | 6 ++++ internal/users/users.go | 13 +++++++ pkg/keychain/helper_darwin.go | 18 +++++++--- pkg/keychain/keychain.go | 3 ++ 9 files changed, 91 insertions(+), 6 deletions(-) diff --git a/internal/frontend/qml/Bridge_test.qml b/internal/frontend/qml/Bridge_test.qml index 728c1545..b7040386 100644 --- a/internal/frontend/qml/Bridge_test.qml +++ b/internal/frontend/qml/Bridge_test.qml @@ -601,6 +601,14 @@ Window { root.notifyHasNoKeychain() } } + + Button { + text: "Rebuild keychain" + colorScheme: root.colorScheme + onClicked: { + root.notifyRebuildKeychain() + } + } } } @@ -825,6 +833,7 @@ Window { } signal changeKeychainFinished() signal notifyHasNoKeychain() + signal notifyRebuildKeychain() signal noActiveKeyForRecipient(string email) signal showMainWindow() diff --git a/internal/frontend/qml/NotificationPopups.qml b/internal/frontend/qml/NotificationPopups.qml index 3e239122..0e7dab1d 100644 --- a/internal/frontend/qml/NotificationPopups.qml +++ b/internal/frontend/qml/NotificationPopups.qml @@ -115,4 +115,9 @@ Item { colorScheme: root.colorScheme notification: root.notifications.noKeychain } + + NotificationDialog { + colorScheme: root.colorScheme + notification: root.notifications.rebuildKeychain + } } diff --git a/internal/frontend/qml/Notifications/Notifications.qml b/internal/frontend/qml/Notifications/Notifications.qml index c4fc32ea..36ee23cd 100644 --- a/internal/frontend/qml/Notifications/Notifications.qml +++ b/internal/frontend/qml/Notifications/Notifications.qml @@ -73,7 +73,8 @@ QtObject { root.enableLocalCache, root.resetBridge, root.deleteAccount, - root.noKeychain + root.noKeychain, + root.rebuildKeychain ] // Connection @@ -901,4 +902,36 @@ QtObject { } ] } + + property Notification rebuildKeychain: Notification { + title: qsTr("Your macOS keychain might be corrupted") + description: qsTr("Bridge is not able to access your macOS keychain. Please consult the instructions on our support page.") + brief: title + icon: "./icons/ic-exclamation-circle-filled.svg" + type: Notification.NotificationType.Danger + group: Notifications.Group.Dialogs | Notifications.Group.Configuration + + property var supportLink: "https://protonmail.com/support/knowledge-base/macos-keychain-corrupted" + + + Connections { + target: root.backend + + onNotifyRebuildKeychain: { + console.log("notifications") + root.rebuildKeychain.active = true + } + } + + action: [ + Action { + text: qsTr("Open the support page") + + onTriggered: { + Qt.openUrlExternally(root.rebuildKeychain.supportLink) + root.backend.quit() + } + } + ] + } } diff --git a/internal/frontend/qt/frontend_events.go b/internal/frontend/qt/frontend_events.go index 15a7a18b..b2242037 100644 --- a/internal/frontend/qt/frontend_events.go +++ b/internal/frontend/qt/frontend_events.go @@ -26,6 +26,7 @@ import ( "github.com/ProtonMail/proton-bridge/internal/bridge" "github.com/ProtonMail/proton-bridge/internal/events" + "github.com/ProtonMail/proton-bridge/pkg/keychain" ) func (f *FrontendQt) watchEvents() { @@ -64,7 +65,11 @@ func (f *FrontendQt) watchEvents() { if strings.Contains(errorDetails, "SMTP failed") { f.qml.PortIssueSMTP() } - case <-credentialsErrorCh: + case reason := <-credentialsErrorCh: + if reason == keychain.ErrMacKeychainRebuild.Error() { + f.qml.NotifyRebuildKeychain() + continue + } f.qml.NotifyHasNoKeychain() case email := <-noActiveKeyForRecipientCh: f.qml.NoActiveKeyForRecipient(email) diff --git a/internal/frontend/qt/qml_backend.go b/internal/frontend/qt/qml_backend.go index b02f87c9..a9c7ac2e 100644 --- a/internal/frontend/qt/qml_backend.go +++ b/internal/frontend/qt/qml_backend.go @@ -142,6 +142,7 @@ type QMLBackend struct { _ func(keychain string) `slot:"changeKeychain"` _ func() `signal:"changeKeychainFinished"` _ func() `signal:"notifyHasNoKeychain"` + _ func() `signal:"notifyRebuildKeychain"` _ func(email string) `signal:noActiveKeyForRecipient` _ func() `signal:showMainWindow` diff --git a/internal/users/user.go b/internal/users/user.go index 31aefb00..ca893308 100644 --- a/internal/users/user.go +++ b/internal/users/user.go @@ -69,6 +69,7 @@ func newUser( creds, err := credStorer.Get(userID) if err != nil { + notifyKeychainRepair(eventListener, err) return nil, nil, errors.Wrap(err, "failed to load user credentials") } @@ -162,6 +163,7 @@ func (u *User) handleAuthRefresh(auth *pmapi.AuthRefresh) { creds, err := u.credStorer.UpdateToken(u.userID, auth.UID, auth.RefreshToken) if err != nil { + notifyKeychainRepair(u.listener, err) u.log.WithError(err).Error("Failed to update refresh token in credentials store") return } @@ -408,6 +410,7 @@ func (u *User) UpdateUser(ctx context.Context) error { creds, err := u.credStorer.UpdateEmails(u.userID, u.client.Addresses().ActiveEmails()) if err != nil { + notifyKeychainRepair(u.listener, err) return err } @@ -445,6 +448,7 @@ func (u *User) SwitchAddressMode() error { creds, err := u.credStorer.SwitchAddressMode(u.userID) if err != nil { + notifyKeychainRepair(u.listener, err) return errors.Wrap(err, "could not switch credentials store address mode") } @@ -490,9 +494,11 @@ func (u *User) Logout() error { creds, err := u.credStorer.Logout(u.userID) if err != nil { + notifyKeychainRepair(u.listener, err) u.log.WithError(err).Warn("Could not log user out from credentials store") if err := u.credStorer.Delete(u.userID); err != nil { + notifyKeychainRepair(u.listener, err) u.log.WithError(err).Error("Could not delete user from credentials store") } } else { diff --git a/internal/users/users.go b/internal/users/users.go index c0eda61b..a68afc61 100644 --- a/internal/users/users.go +++ b/internal/users/users.go @@ -27,6 +27,7 @@ import ( "github.com/ProtonMail/proton-bridge/internal/events" "github.com/ProtonMail/proton-bridge/internal/metrics" "github.com/ProtonMail/proton-bridge/internal/users/credentials" + "github.com/ProtonMail/proton-bridge/pkg/keychain" "github.com/ProtonMail/proton-bridge/pkg/listener" "github.com/ProtonMail/proton-bridge/pkg/pmapi" "github.com/hashicorp/go-multierror" @@ -130,6 +131,7 @@ func (u *Users) loadUsersFromCredentialsStore() error { userIDs, err := u.credStorer.List() if err != nil { + notifyKeychainRepair(u.events, err) return err } @@ -188,6 +190,7 @@ func (u *Users) loadConnectedUser(ctx context.Context, user *User, creds *creden // Update the user's credentials with the latest auth used to connect this user. if creds, err = u.credStorer.UpdateToken(creds.UserID, auth.UID, auth.RefreshToken); err != nil { + notifyKeychainRepair(u.events, err) return errors.Wrap(err, "could not create get user's refresh token") } @@ -226,12 +229,14 @@ func (u *Users) FinishLogin(client pmapi.Client, auth *pmapi.Auth, password []by // Update the user's credentials with the latest auth used to connect this user. if _, err := u.credStorer.UpdateToken(auth.UserID, auth.UID, auth.RefreshToken); err != nil { + notifyKeychainRepair(u.events, err) return nil, errors.Wrap(err, "failed to load user credentials") } // Update the password in case the user changed it. creds, err := u.credStorer.UpdatePassword(apiUser.ID, passphrase) if err != nil { + notifyKeychainRepair(u.events, err) return nil, errors.Wrap(err, "failed to update password of user in credentials store") } @@ -260,6 +265,7 @@ func (u *Users) addNewUser(client pmapi.Client, apiUser *pmapi.User, auth *pmapi defer u.lock.Unlock() if _, err := u.credStorer.Add(apiUser.ID, apiUser.Name, auth.UID, auth.RefreshToken, passphrase, client.Addresses().ActiveEmails()); err != nil { + notifyKeychainRepair(u.events, err) return errors.Wrap(err, "failed to add user credentials to credentials store") } @@ -384,6 +390,7 @@ func (u *Users) DeleteUser(userID string, clearStore bool) error { } if err := u.credStorer.Delete(userID); err != nil { + notifyKeychainRepair(u.events, err) log.WithError(err).Error("Cannot remove user") return err } @@ -443,3 +450,9 @@ func (u *Users) crashBandicoot(username string) { panic("Your wish is my command… I crash!") } } + +func notifyKeychainRepair(l listener.Listener, err error) { + if err == keychain.ErrMacKeychainRebuild { + l.Emit(events.CredentialsErrorEvent, err.Error()) + } +} diff --git a/pkg/keychain/helper_darwin.go b/pkg/keychain/helper_darwin.go index 5b0ab1cc..01315647 100644 --- a/pkg/keychain/helper_darwin.go +++ b/pkg/keychain/helper_darwin.go @@ -40,6 +40,16 @@ func init() { // nolint[noinit] defaultHelper = MacOSKeychain } +func parseError(original error) error { + if original == nil { + return nil + } + if strings.Contains(original.Error(), "25293") { + return ErrMacKeychainRebuild + } + return original +} + func newMacOSHelper(url string) (credentials.Helper, error) { return &macOSHelper{url: url}, nil } @@ -76,7 +86,7 @@ func (h *macOSHelper) Add(creds *credentials.Credentials) error { query := newQuery(hostURL, userID) query.SetData([]byte(creds.Secret)) - return keychain.AddItem(query) + return parseError(keychain.AddItem(query)) } func (h *macOSHelper) Delete(secretURL string) error { @@ -87,7 +97,7 @@ func (h *macOSHelper) Delete(secretURL string) error { query := newQuery(hostURL, userID) - return keychain.DeleteItem(query) + return parseError(keychain.DeleteItem(query)) } func (h *macOSHelper) Get(secretURL string) (string, string, error) { @@ -102,7 +112,7 @@ func (h *macOSHelper) Get(secretURL string) (string, string, error) { results, err := keychain.QueryItem(query) if err != nil { - return "", "", err + return "", "", parseError(err) } if len(results) == 0 { @@ -121,7 +131,7 @@ func (h *macOSHelper) List() (map[string]string, error) { userIDs, err := keychain.GetGenericPasswordAccounts(h.url) if err != nil { - return nil, err + return nil, parseError(err) } for _, userID := range userIDs { diff --git a/pkg/keychain/keychain.go b/pkg/keychain/keychain.go index af544847..2089ce70 100644 --- a/pkg/keychain/keychain.go +++ b/pkg/keychain/keychain.go @@ -37,6 +37,9 @@ var ( // ErrNoKeychain indicates that no suitable keychain implementation could be loaded. ErrNoKeychain = errors.New("no keychain") // nolint[noglobals] + // ErrMacKeychainRebuild is returned on macOS with blocked or corrupted keychain. + ErrMacKeychainRebuild = errors.New("keychain error -25293") + // Helpers holds all discovered keychain helpers. It is populated in init(). Helpers map[string]helperConstructor // nolint[noglobals]