From 810705ba010887ea48b0af14564c3d24af33afb1 Mon Sep 17 00:00:00 2001 From: James Houlahan Date: Thu, 23 Feb 2023 16:21:41 +0100 Subject: [PATCH] fix(GODT-1945): Handle disabled addresses correctly When listing all a user's email addresses (e.g. for apple mail autoconf) we need to exclude disabled addresses. Similarly, we need to remove them from gluon if using split mode. --- internal/bridge/user_event_test.go | 77 +++++++++++++++++++++ internal/bridge/user_events.go | 104 +++++++++++++++++++---------- internal/events/address.go | 24 +++++++ internal/frontend/cli/frontend.go | 23 ++++++- internal/user/events.go | 78 +++++++++++++++++++--- internal/user/user.go | 6 +- 6 files changed, 264 insertions(+), 48 deletions(-) diff --git a/internal/bridge/user_event_test.go b/internal/bridge/user_event_test.go index 87d5cfc2..f38da332 100644 --- a/internal/bridge/user_event_test.go +++ b/internal/bridge/user_event_test.go @@ -534,6 +534,83 @@ func TestBridge_User_SendDraftRemoveDraftFlag(t *testing.T) { }) } +func TestBridge_User_DisableEnableAddress(t *testing.T) { + withEnv(t, func(ctx context.Context, s *server.Server, netCtl *proton.NetCtl, locator bridge.Locator, storeKey []byte) { + // Create a user. + userID, _, err := s.CreateUser("user", password) + require.NoError(t, err) + + // Create an additional address for the user. + aliasID, err := s.CreateAddress(userID, "alias@"+s.GetDomain(), password) + require.NoError(t, err) + + withBridge(ctx, t, s.GetHostURL(), netCtl, locator, storeKey, func(bridge *bridge.Bridge, mocks *bridge.Mocks) { + require.NoError(t, getErr(bridge.LoginFull(ctx, "user", password, nil, nil))) + + // Initially we should list the address. + info, err := bridge.QueryUserInfo("user") + require.NoError(t, err) + require.Contains(t, info.Addresses, "alias@"+s.GetDomain()) + }) + + // Disable the address. + withClient(ctx, t, s, "user", password, func(ctx context.Context, c *proton.Client) { + require.NoError(t, c.DisableAddress(ctx, aliasID)) + }) + + withBridge(ctx, t, s.GetHostURL(), netCtl, locator, storeKey, func(bridge *bridge.Bridge, mocks *bridge.Mocks) { + // Eventually we shouldn't list the address. + require.Eventually(t, func() bool { + info, err := bridge.QueryUserInfo("user") + require.NoError(t, err) + + return xslices.Index(info.Addresses, "alias@"+s.GetDomain()) < 0 + }, 5*time.Second, 100*time.Millisecond) + }) + + // Enable the address. + withClient(ctx, t, s, "user", password, func(ctx context.Context, c *proton.Client) { + require.NoError(t, c.EnableAddress(ctx, aliasID)) + }) + + withBridge(ctx, t, s.GetHostURL(), netCtl, locator, storeKey, func(bridge *bridge.Bridge, mocks *bridge.Mocks) { + // Eventually we should list the address. + require.Eventually(t, func() bool { + info, err := bridge.QueryUserInfo("user") + require.NoError(t, err) + + return xslices.Index(info.Addresses, "alias@"+s.GetDomain()) >= 0 + }, 5*time.Second, 100*time.Millisecond) + }) + }) +} + +func TestBridge_User_CreateDisabledAddress(t *testing.T) { + withEnv(t, func(ctx context.Context, s *server.Server, netCtl *proton.NetCtl, locator bridge.Locator, storeKey []byte) { + // Create a user. + userID, _, err := s.CreateUser("user", password) + require.NoError(t, err) + + // Create an additional address for the user. + aliasID, err := s.CreateAddress(userID, "alias@"+s.GetDomain(), password) + require.NoError(t, err) + + // Immediately disable the address. + withClient(ctx, t, s, "user", password, func(ctx context.Context, c *proton.Client) { + require.NoError(t, c.DisableAddress(ctx, aliasID)) + }) + + withBridge(ctx, t, s.GetHostURL(), netCtl, locator, storeKey, func(bridge *bridge.Bridge, mocks *bridge.Mocks) { + require.NoError(t, getErr(bridge.LoginFull(ctx, "user", password, nil, nil))) + + // Initially we shouldn't list the address. + info, err := bridge.QueryUserInfo("user") + require.NoError(t, err) + require.NotContains(t, info.Addresses, "alias@"+s.GetDomain()) + }) + }) +} + // userLoginAndSync logs in user and waits until user is fully synced. func userLoginAndSync( ctx context.Context, diff --git a/internal/bridge/user_events.go b/internal/bridge/user_events.go index 4422b27f..6d53f0b7 100644 --- a/internal/bridge/user_events.go +++ b/internal/bridge/user_events.go @@ -37,9 +37,14 @@ func (bridge *Bridge) handleUserEvent(ctx context.Context, user *user.User, even return fmt.Errorf("failed to handle user address created event: %w", err) } - case events.UserAddressUpdated: - if err := bridge.handleUserAddressUpdated(ctx, user, event); err != nil { - return fmt.Errorf("failed to handle user address updated event: %w", err) + case events.UserAddressEnabled: + if err := bridge.handleUserAddressEnabled(ctx, user, event); err != nil { + return fmt.Errorf("failed to handle user address enabled event: %w", err) + } + + case events.UserAddressDisabled: + if err := bridge.handleUserAddressDisabled(ctx, user, event); err != nil { + return fmt.Errorf("failed to handle user address disabled event: %w", err) } case events.UserAddressDeleted: @@ -66,55 +71,84 @@ func (bridge *Bridge) handleUserEvent(ctx context.Context, user *user.User, even } func (bridge *Bridge) handleUserAddressCreated(ctx context.Context, user *user.User, event events.UserAddressCreated) error { - if user.GetAddressMode() == vault.SplitMode { - if bridge.imapServer == nil { - return fmt.Errorf("no imap server instance running") - } + if user.GetAddressMode() != vault.SplitMode { + return nil + } - gluonID, err := bridge.imapServer.AddUser(ctx, user.NewIMAPConnector(event.AddressID), user.GluonKey()) - if err != nil { - return fmt.Errorf("failed to add user to IMAP server: %w", err) - } + if bridge.imapServer == nil { + return fmt.Errorf("no imap server instance running") + } - if err := user.SetGluonID(event.AddressID, gluonID); err != nil { - return fmt.Errorf("failed to set gluon ID: %w", err) - } + gluonID, err := bridge.imapServer.AddUser(ctx, user.NewIMAPConnector(event.AddressID), user.GluonKey()) + if err != nil { + return fmt.Errorf("failed to add user to IMAP server: %w", err) + } + + if err := user.SetGluonID(event.AddressID, gluonID); err != nil { + return fmt.Errorf("failed to set gluon ID: %w", err) } return nil } -// GODT-1948: Handle addresses that have been disabled! -func (bridge *Bridge) handleUserAddressUpdated(_ context.Context, user *user.User, _ events.UserAddressUpdated) error { - switch user.GetAddressMode() { - case vault.CombinedMode: - return fmt.Errorf("not implemented") +func (bridge *Bridge) handleUserAddressEnabled(ctx context.Context, user *user.User, event events.UserAddressEnabled) error { + if user.GetAddressMode() != vault.SplitMode { + return nil + } - case vault.SplitMode: - return fmt.Errorf("not implemented") + gluonID, err := bridge.imapServer.AddUser(ctx, user.NewIMAPConnector(event.AddressID), user.GluonKey()) + if err != nil { + return fmt.Errorf("failed to add user to IMAP server: %w", err) + } + + if err := user.SetGluonID(event.AddressID, gluonID); err != nil { + return fmt.Errorf("failed to set gluon ID: %w", err) + } + + return nil +} + +func (bridge *Bridge) handleUserAddressDisabled(ctx context.Context, user *user.User, event events.UserAddressDisabled) error { + if user.GetAddressMode() != vault.SplitMode { + return nil + } + + gluonID, ok := user.GetGluonID(event.AddressID) + if !ok { + return fmt.Errorf("gluon ID not found for address %s", event.AddressID) + } + + if err := bridge.imapServer.RemoveUser(ctx, gluonID, true); err != nil { + return fmt.Errorf("failed to remove user from IMAP server: %w", err) + } + + if err := user.RemoveGluonID(event.AddressID, gluonID); err != nil { + return fmt.Errorf("failed to remove gluon ID for address: %w", err) } return nil } func (bridge *Bridge) handleUserAddressDeleted(ctx context.Context, user *user.User, event events.UserAddressDeleted) error { - if user.GetAddressMode() == vault.SplitMode { - if bridge.imapServer == nil { - return fmt.Errorf("no imap server instance running") - } + if user.GetAddressMode() != vault.SplitMode { + return nil + } - gluonID, ok := user.GetGluonID(event.AddressID) - if !ok { - return fmt.Errorf("gluon ID not found for address %s", event.AddressID) - } + if bridge.imapServer == nil { + return fmt.Errorf("no imap server instance running") + } - if err := bridge.imapServer.RemoveUser(ctx, gluonID, true); err != nil { - return fmt.Errorf("failed to remove user from IMAP server: %w", err) - } + gluonID, ok := user.GetGluonID(event.AddressID) + if !ok { + return fmt.Errorf("gluon ID not found for address %s", event.AddressID) + } - if err := user.RemoveGluonID(event.AddressID, gluonID); err != nil { - return fmt.Errorf("failed to remove gluon ID for address: %w", err) - } + if err := bridge.imapServer.RemoveUser(ctx, gluonID, true); err != nil { + return fmt.Errorf("failed to remove user from IMAP server: %w", err) + } + + if err := user.RemoveGluonID(event.AddressID, gluonID); err != nil { + return fmt.Errorf("failed to remove gluon ID for address: %w", err) } return nil diff --git a/internal/events/address.go b/internal/events/address.go index 687ea578..96695e26 100644 --- a/internal/events/address.go +++ b/internal/events/address.go @@ -35,6 +35,30 @@ func (event UserAddressCreated) String() string { return fmt.Sprintf("UserAddressCreated: UserID: %s, AddressID: %s, Email: %s", event.UserID, event.AddressID, logging.Sensitive(event.Email)) } +type UserAddressEnabled struct { + eventBase + + UserID string + AddressID string + Email string +} + +func (event UserAddressEnabled) String() string { + return fmt.Sprintf("UserAddressEnabled: UserID: %s, AddressID: %s, Email: %s", event.UserID, event.AddressID, logging.Sensitive(event.Email)) +} + +type UserAddressDisabled struct { + eventBase + + UserID string + AddressID string + Email string +} + +func (event UserAddressDisabled) String() string { + return fmt.Sprintf("UserAddressDisabled: UserID: %s, AddressID: %s, Email: %s", event.UserID, event.AddressID, logging.Sensitive(event.Email)) +} + type UserAddressUpdated struct { eventBase diff --git a/internal/frontend/cli/frontend.go b/internal/frontend/cli/frontend.go index 07719c4f..e06c3a3a 100644 --- a/internal/frontend/cli/frontend.go +++ b/internal/frontend/cli/frontend.go @@ -310,6 +310,14 @@ func (f *frontendCLI) watchEvents(eventCh <-chan events.Event) { // nolint:gocyc case events.IMAPLoginFailed: f.Printf("An IMAP login attempt failed for user %v\n", event.Username) + case events.UserAddressEnabled: + user, err := f.bridge.GetUserInfo(event.UserID) + if err != nil { + return + } + + f.Printf("An address for %s was enabled. You may need to reconfigure your email client.\n", user.Username) + case events.UserAddressUpdated: user, err := f.bridge.GetUserInfo(event.UserID) if err != nil { @@ -318,8 +326,21 @@ func (f *frontendCLI) watchEvents(eventCh <-chan events.Event) { // nolint:gocyc f.Printf("Address changed for %s. You may need to reconfigure your email client.\n", user.Username) + case events.UserAddressDisabled: + user, err := f.bridge.GetUserInfo(event.UserID) + if err != nil { + return + } + + f.Printf("An address for %s was disabled. You may need to reconfigure your email client.\n", user.Username) + case events.UserAddressDeleted: - f.notifyLogout(event.Email) + user, err := f.bridge.GetUserInfo(event.UserID) + if err != nil { + return + } + + f.Printf("An address for %s was disabled. You may need to reconfigure your email client.\n", user.Username) case events.SyncStarted: user, err := f.bridge.GetUserInfo(event.UserID) diff --git a/internal/user/events.go b/internal/user/events.go index b80180b2..25854222 100644 --- a/internal/user/events.go +++ b/internal/user/events.go @@ -205,6 +205,12 @@ func (user *User) handleCreateAddressEvent(ctx context.Context, event proton.Add user.apiAddrs[event.Address.ID] = event.Address + // If the address is disabled. + if event.Address.Status != proton.AddressStatusEnabled { + return nil + } + + // If the address is enabled, we need to hook it up to the update channels. switch user.vault.AddressMode() { case vault.CombinedMode: primAddr, err := getAddrIdx(user.apiAddrs, 0) @@ -231,6 +237,10 @@ func (user *User) handleCreateAddressEvent(ctx context.Context, event proton.Add // Perform the sync in an RLock. return safe.RLockRet(func() error { + if event.Address.Status != proton.AddressStatusEnabled { + return nil + } + if user.vault.AddressMode() == vault.SplitMode { if err := syncLabels(ctx, user.apiLabels, user.updateCh[event.Address.ID]); err != nil { return fmt.Errorf("failed to sync labels to new address: %w", err) @@ -248,18 +258,58 @@ func (user *User) handleUpdateAddressEvent(_ context.Context, event proton.Addre "email": logging.Sensitive(event.Address.Email), }).Info("Handling address updated event") - if _, ok := user.apiAddrs[event.Address.ID]; !ok { + oldAddr, ok := user.apiAddrs[event.Address.ID] + if !ok { user.log.Debugf("Address %q does not exist", event.Address.ID) return nil } user.apiAddrs[event.Address.ID] = event.Address - user.eventCh.Enqueue(events.UserAddressUpdated{ - UserID: user.apiUser.ID, - AddressID: event.Address.ID, - Email: event.Address.Email, - }) + switch { + // If the address was newly enabled: + case oldAddr.Status != proton.AddressStatusEnabled && event.Address.Status == proton.AddressStatusEnabled: + switch user.vault.AddressMode() { + case vault.CombinedMode: + primAddr, err := getAddrIdx(user.apiAddrs, 0) + if err != nil { + return fmt.Errorf("failed to get primary address: %w", err) + } + + user.updateCh[event.Address.ID] = user.updateCh[primAddr.ID] + + case vault.SplitMode: + user.updateCh[event.Address.ID] = queue.NewQueuedChannel[imap.Update](0, 0) + } + + user.eventCh.Enqueue(events.UserAddressEnabled{ + UserID: user.apiUser.ID, + AddressID: event.Address.ID, + Email: event.Address.Email, + }) + + // If the address was newly disabled: + case oldAddr.Status == proton.AddressStatusEnabled && event.Address.Status != proton.AddressStatusEnabled: + if user.vault.AddressMode() == vault.SplitMode { + user.updateCh[event.ID].CloseAndDiscardQueued() + } + + delete(user.updateCh, event.ID) + + user.eventCh.Enqueue(events.UserAddressDisabled{ + UserID: user.apiUser.ID, + AddressID: event.Address.ID, + Email: event.Address.Email, + }) + + // Otherwise it's just an update: + default: + user.eventCh.Enqueue(events.UserAddressUpdated{ + UserID: user.apiUser.ID, + AddressID: event.Address.ID, + Email: event.Address.Email, + }) + } return nil }, user.apiAddrsLock) @@ -275,12 +325,20 @@ func (user *User) handleDeleteAddressEvent(_ context.Context, event proton.Addre return nil } - if user.vault.AddressMode() == vault.SplitMode { - user.updateCh[event.ID].CloseAndDiscardQueued() - delete(user.updateCh, event.ID) + delete(user.apiAddrs, event.ID) + + // If the address was disabled to begin with, we don't need to do anything. + if addr.Status != proton.AddressStatusEnabled { + return nil } - delete(user.apiAddrs, event.ID) + // Otherwise, in split mode, drop the update queue. + if user.vault.AddressMode() == vault.SplitMode { + user.updateCh[event.ID].CloseAndDiscardQueued() + } + + // And in either mode, remove the address from the update channel map. + delete(user.updateCh, event.ID) user.eventCh.Enqueue(events.UserAddressDeleted{ UserID: user.apiUser.ID, diff --git a/internal/user/user.go b/internal/user/user.go index 5a690a36..e3e31539 100644 --- a/internal/user/user.go +++ b/internal/user/user.go @@ -264,11 +264,13 @@ func (user *User) Match(query string) bool { }, user.apiUserLock, user.apiAddrsLock) } -// Emails returns all the user's email addresses. +// Emails returns all the user's active email addresses. // It returns them in sorted order; the user's primary address is first. func (user *User) Emails() []string { return safe.RLockRet(func() []string { - addresses := maps.Values(user.apiAddrs) + addresses := xslices.Filter(maps.Values(user.apiAddrs), func(addr proton.Address) bool { + return addr.Status == proton.AddressStatusEnabled + }) slices.SortFunc(addresses, func(a, b proton.Address) bool { return a.Order < b.Order