From af01c63298642d1b5d616df141dc5ecdee458da5 Mon Sep 17 00:00:00 2001 From: Atanas Janeshliev Date: Mon, 11 Nov 2024 15:49:23 +0100 Subject: [PATCH] fix(BRIDGE-261): delete gluon data during user deletion; integration tests; FF kill switch; Sentry report if error; --- internal/bridge/bridge.go | 5 ++ internal/bridge/user.go | 5 +- internal/services/imapservice/service.go | 13 +++++ internal/unleash/service.go | 5 +- internal/user/user.go | 25 +++++++-- tests/features/user/delete_imap.feature | 25 +++++++++ tests/steps_test.go | 1 + tests/user_test.go | 66 ++++++++++++++++++++++++ 8 files changed, 137 insertions(+), 8 deletions(-) create mode 100644 tests/features/user/delete_imap.feature diff --git a/internal/bridge/bridge.go b/internal/bridge/bridge.go index 88ef7676..fc9a3b00 100644 --- a/internal/bridge/bridge.go +++ b/internal/bridge/bridge.go @@ -733,3 +733,8 @@ func (bridge *Bridge) ReportMessageWithContext(message string, messageCtx report }).Info("Error occurred when sending Report to Sentry") } } + +// GetUsers is only used for testing purposes. +func (bridge *Bridge) GetUsers() map[string]*user.User { + return bridge.users +} diff --git a/internal/bridge/user.go b/internal/bridge/user.go index 717d6c29..fda3e1b3 100644 --- a/internal/bridge/user.go +++ b/internal/bridge/user.go @@ -33,6 +33,7 @@ import ( "github.com/ProtonMail/proton-bridge/v3/internal/safe" "github.com/ProtonMail/proton-bridge/v3/internal/services/imapservice" "github.com/ProtonMail/proton-bridge/v3/internal/try" + "github.com/ProtonMail/proton-bridge/v3/internal/unleash" "github.com/ProtonMail/proton-bridge/v3/internal/user" "github.com/ProtonMail/proton-bridge/v3/internal/vault" "github.com/go-resty/resty/v2" @@ -607,7 +608,7 @@ func (bridge *Bridge) newVaultUser( return bridge.vault.GetOrAddUser(apiUser.ID, apiUser.Name, apiUser.Email, authUID, authRef, saltedKeyPass) } -// logout logs out the given user, optionally logging them out from the API too. +// logoutUser logs out the given user, optionally logging them out from the API and deleting user related gluon data. func (bridge *Bridge) logoutUser(ctx context.Context, user *user.User, withAPI, withData bool) { defer delete(bridge.users, user.ID()) @@ -617,7 +618,7 @@ func (bridge *Bridge) logoutUser(ctx context.Context, user *user.User, withAPI, "withData": withData, }).Debug("Logging out user") - if err := user.Logout(ctx, withAPI); err != nil { + if err := user.Logout(ctx, withAPI, withData, bridge.unleashService.GetFlagValue(unleash.UserRemovalGluonDataCleanupDisabled)); err != nil { logUser.WithError(err).Error("Failed to logout user") } diff --git a/internal/services/imapservice/service.go b/internal/services/imapservice/service.go index 989b78fb..99d18788 100644 --- a/internal/services/imapservice/service.go +++ b/internal/services/imapservice/service.go @@ -233,6 +233,12 @@ func (s *Service) OnLogout(ctx context.Context) error { return err } +func (s *Service) OnDelete(ctx context.Context) error { + _, err := s.cpc.Send(ctx, &onDeleteReq{}) + + return err +} + func (s *Service) ShowAllMail(ctx context.Context, v bool) error { _, err := s.cpc.Send(ctx, &showAllMailReq{v: v}) @@ -362,6 +368,11 @@ func (s *Service) run(ctx context.Context) { //nolint gocyclo err := s.removeConnectorsFromServer(ctx, s.connectors, false) req.Reply(ctx, nil, err) + case *onDeleteReq: + s.log.Debug("Delete Request") + err := s.removeConnectorsFromServer(ctx, s.connectors, true) + req.Reply(ctx, nil, err) + case *showAllMailReq: s.log.Debug("Show all mail request") req.Reply(ctx, nil, nil) @@ -644,6 +655,8 @@ type onLogoutReq struct{} type showAllMailReq struct{ v bool } +type onDeleteReq struct{} + type setAddressModeReq struct { mode usertypes.AddressMode } diff --git a/internal/unleash/service.go b/internal/unleash/service.go index 56f56135..0ae4fb48 100644 --- a/internal/unleash/service.go +++ b/internal/unleash/service.go @@ -37,8 +37,9 @@ var pollJitter = 2 * time.Minute //nolint:gochecknoglobals const filename = "unleash_flags" const ( - EventLoopNotificationDisabled = "InboxBridgeEventLoopNotificationDisabled" - IMAPAuthenticateCommandDisabled = "InboxBridgeImapAuthenticateCommandDisabled" + EventLoopNotificationDisabled = "InboxBridgeEventLoopNotificationDisabled" + IMAPAuthenticateCommandDisabled = "InboxBridgeImapAuthenticateCommandDisabled" + UserRemovalGluonDataCleanupDisabled = "InboxBridgeUserRemovalGluonDataCleanupDisabled" ) type requestFeaturesFn func(ctx context.Context) (proton.FeatureFlagResult, error) diff --git a/internal/user/user.go b/internal/user/user.go index 0626588d..536c1b3a 100644 --- a/internal/user/user.go +++ b/internal/user/user.go @@ -592,8 +592,13 @@ func (user *User) CheckAuth(email string, password []byte) (string, error) { } // Logout logs the user out from the API. -func (user *User) Logout(ctx context.Context, withAPI bool) error { - user.log.WithField("withAPI", withAPI).Info("Logging out user") +func (user *User) Logout(ctx context.Context, withAPI, withData, withDataDisabledKillSwitch bool) error { + user.log.WithFields( + logrus.Fields{ + "withAPI": withAPI, + "withData": withData, + "withDataDisabledKillSwitch": withDataDisabledKillSwitch, + }).Info("Logging out user") user.log.Debug("Canceling ongoing tasks") @@ -601,8 +606,20 @@ func (user *User) Logout(ctx context.Context, withAPI bool) error { return fmt.Errorf("failed to remove user from smtp server: %w", err) } - if err := user.imapService.OnLogout(ctx); err != nil { - return fmt.Errorf("failed to remove user from imap server: %w", err) + if withData && !withDataDisabledKillSwitch { + if err := user.imapService.OnDelete(ctx); err != nil { + if rerr := user.reporter.ReportMessageWithContext("Failed to delete user IMAP data", map[string]any{ + "error": err.Error(), + }); rerr != nil { + logrus.WithError(rerr).Info("Failed to report user IMAP deletion issue to Sentry") + } + + return fmt.Errorf("failed to delete user from imap server: %w", err) + } + } else { + if err := user.imapService.OnLogout(ctx); err != nil { + return fmt.Errorf("failed to remove user from imap server: %w", err) + } } user.tasks.CancelAndWait() diff --git a/tests/features/user/delete_imap.feature b/tests/features/user/delete_imap.feature new file mode 100644 index 00000000..0ffaa875 --- /dev/null +++ b/tests/features/user/delete_imap.feature @@ -0,0 +1,25 @@ +Feature: User deletion with IMAP data removal + Background: + Given there exists an account with username "[user:user]" and password "password" + And the account "[user:user]" has the following custom mailboxes: + | name | type | + | one | folder | + And the address "[user:user]@[domain]" of account "[user:user]" has the following messages in "Folders/one": + | from | to | subject | unread | + | a@example.com | a@example.com | one | true | + | b@example.com | b@example.com | two | false | + | c@example.com | c@example.com | three | true | + | c@example.com | c@example.com | four | false | + Then it succeeds + When bridge starts + And the user logs in with username "[user:user]" and password "password" + And user "[user:user]" finishes syncing + Then it succeeds + + Scenario: User is deleted from Bridge and IMAP data is removed + When user "[user:user]" connects and authenticates IMAP client "1" + Then IMAP client "1" sees the following mailbox info for "Folders/one": + | name | total | unread | + | Folders/one | 4 | 2 | + And user "[user:user]" is deleted alongside IMAP data for client "1" + Then it succeeds diff --git a/tests/steps_test.go b/tests/steps_test.go index b3f73be0..7dd4085e 100644 --- a/tests/steps_test.go +++ b/tests/steps_test.go @@ -116,6 +116,7 @@ func (s *scenario) steps(ctx *godog.ScenarioContext) { ctx.Step(`^user "([^"]*)" has telemetry set to (\d+)$`, s.userHasTelemetrySetTo) ctx.Step(`^the bridge password of user "([^"]*)" is changed to "([^"]*)"`, s.bridgePasswordOfUserIsChangedTo) ctx.Step(`^the bridge password of user "([^"]*)" is equal to "([^"]*)"`, s.bridgePasswordOfUserIsEqualTo) + ctx.Step(`^user "([^"]*)" is deleted alongside IMAP data for client "([^"]*)"$`, s.userIsDeletedAndImapDataRemoved) // ==== ACCOUNT SETTINGS ==== ctx.Step(`^the account "([^"]*)" has public key attachment "([^"]*)"`, s.accountHasPublicKeyAttachment) diff --git a/tests/user_test.go b/tests/user_test.go index 5ac4be9f..e7b56674 100644 --- a/tests/user_test.go +++ b/tests/user_test.go @@ -22,6 +22,8 @@ import ( "errors" "fmt" "net/mail" + "os" + "path/filepath" "strings" "time" @@ -388,6 +390,70 @@ func (s *scenario) userIsDeleted(username string) error { return s.t.bridge.DeleteUser(context.Background(), s.t.getUserByName(username).getUserID()) } +func (s *scenario) userIsDeletedAndImapDataRemoved(username string) error { + gluonCacheDir := s.t.bridge.GetGluonCacheDir() + userID := s.t.getUserByName(username).userID + userMap := s.t.bridge.GetUsers() + userObj, ok := userMap[userID] + if !ok { + return fmt.Errorf("could not find user object") + } + + gluonIDMap := userObj.GetGluonIDs() + gluonIDs := make([]string, 0, len(gluonIDMap)) + for _, id := range gluonIDMap { + gluonIDs = append(gluonIDs, id) + } + + var relevantPaths []string + if err := filepath.Walk(gluonCacheDir, func(path string, _ os.FileInfo, err error) error { + if err != nil { + return err + } + for _, gluonID := range gluonIDs { + if strings.Contains(path, gluonID) { + relevantPaths = append(relevantPaths, path) + } + } + return nil + }); err != nil { + return err + } + + if len(relevantPaths) == 0 { + return fmt.Errorf("found no user related gluon paths") + } + + if err := s.t.bridge.DeleteUser(context.Background(), userID); err != nil { + return fmt.Errorf("could not delete user: %w", err) + } + + foundDeferredDelete := false + var remainingPaths []string + if err := filepath.Walk(gluonCacheDir, func(path string, _ os.FileInfo, err error) error { + if err != nil { + return err + } + for _, gluonID := range gluonIDs { + if strings.Contains(path, gluonID) { + remainingPaths = append(remainingPaths, path) + } + } + if strings.Contains(path, "deferred_delete") { + foundDeferredDelete = true + } + return nil + }); err != nil { + return err + } + + if len(remainingPaths) == 0 && foundDeferredDelete { + return nil + } + + return fmt.Errorf("user gluon data is still present or could not find deferred deletion directory") +} + func (s *scenario) theAuthOfUserIsRevoked(username string) error { return s.t.withClient(context.Background(), username, func(ctx context.Context, client *proton.Client) error { return client.AuthRevokeAll(ctx)