From 720f662afeab26881b837c1b8b0290166e4d00ff Mon Sep 17 00:00:00 2001 From: Romain LE JEUNE Date: Wed, 28 Jun 2023 15:26:41 +0200 Subject: [PATCH] feat(GODT-2714): Set Configuration Status to Failure and send Recovery event when issue is solved. --- internal/bridge/settings.go | 4 +- internal/bridge/smtp_backend.go | 7 ++- internal/bridge/user.go | 20 ++++---- internal/bridge/user_events.go | 3 +- internal/configstatus/config_status.go | 7 +++ .../configstatus/configuration_recovery.go | 42 +++++++++++++++- internal/user/config_status.go | 48 +++++++++++++++++-- internal/user/user.go | 4 +- internal/user/user_test.go | 1 + 9 files changed, 118 insertions(+), 18 deletions(-) diff --git a/internal/bridge/settings.go b/internal/bridge/settings.go index dbbadecb..47b28ee6 100644 --- a/internal/bridge/settings.go +++ b/internal/bridge/settings.go @@ -297,10 +297,12 @@ func (bridge *Bridge) SetColorScheme(colorScheme string) error { // Note: it does not clear the keychain. The only entry in the keychain is the vault password, // which we need at next startup to decrypt the vault. func (bridge *Bridge) FactoryReset(ctx context.Context) { + telemetry := bridge.IsTelemetryAvailable() + // Delete all the users. safe.Lock(func() { for _, user := range bridge.users { - bridge.logoutUser(ctx, user, true, true) + bridge.logoutUser(ctx, user, true, true, telemetry) } }, bridge.usersLock) diff --git a/internal/bridge/smtp_backend.go b/internal/bridge/smtp_backend.go index 68723256..e07d4d25 100644 --- a/internal/bridge/smtp_backend.go +++ b/internal/bridge/smtp_backend.go @@ -70,8 +70,11 @@ func (s *smtpSession) AuthPlain(username, password string) error { "username": username, "pkg": "smtp", }).Error("Incorrect login credentials.") - - return fmt.Errorf("invalid username or password") + err := fmt.Errorf("invalid username or password") + for _, user := range s.users { + user.ReportConfigStatusFailure(err.Error()) + } + return err }, s.usersLock) } diff --git a/internal/bridge/user.go b/internal/bridge/user.go index 46df3ba6..3d0c2ea2 100644 --- a/internal/bridge/user.go +++ b/internal/bridge/user.go @@ -229,7 +229,7 @@ func (bridge *Bridge) LogoutUser(ctx context.Context, userID string) error { return ErrNoSuchUser } - bridge.logoutUser(ctx, user, true, false) + bridge.logoutUser(ctx, user, true, false, false) bridge.publish(events.UserLoggedOut{ UserID: userID, @@ -243,13 +243,15 @@ func (bridge *Bridge) LogoutUser(ctx context.Context, userID string) error { func (bridge *Bridge) DeleteUser(ctx context.Context, userID string) error { logrus.WithField("userID", userID).Info("Deleting user") + telemetry := bridge.IsTelemetryAvailable() + return safe.LockRet(func() error { if !bridge.vault.HasUser(userID) { return ErrNoSuchUser } if user, ok := bridge.users[userID]; ok { - bridge.logoutUser(ctx, user, true, true) + bridge.logoutUser(ctx, user, true, true, telemetry) } if err := bridge.vault.DeleteUser(userID); err != nil { @@ -351,7 +353,7 @@ func (bridge *Bridge) SendBadEventUserFeedback(_ context.Context, userID string, logrus.WithError(rerr).Error("Failed to report feedback failure") } - bridge.logoutUser(ctx, user, true, false) + bridge.logoutUser(ctx, user, true, false, false) bridge.publish(events.UserLoggedOut{ UserID: userID, @@ -595,9 +597,14 @@ func (bridge *Bridge) newVaultUser( } // logout logs out the given user, optionally logging them out from the API too. -func (bridge *Bridge) logoutUser(ctx context.Context, user *user.User, withAPI, withData bool) { +func (bridge *Bridge) logoutUser(ctx context.Context, user *user.User, withAPI, withData, withTelemetry bool) { defer delete(bridge.users, user.ID()) + // if this is actually a remove account + if withTelemetry && withData && withAPI { + user.SendConfigStatusAbort() + } + logrus.WithFields(logrus.Fields{ "userID": user.ID(), "withAPI": withAPI, @@ -608,11 +615,6 @@ func (bridge *Bridge) logoutUser(ctx context.Context, user *user.User, withAPI, logrus.WithError(err).Error("Failed to remove IMAP user") } - // if this is actually a remove account - if withData && withAPI { - user.SendConfigStatusAbort() - } - if err := user.Logout(ctx, withAPI); err != nil { logrus.WithError(err).Error("Failed to logout user") } diff --git a/internal/bridge/user_events.go b/internal/bridge/user_events.go index 8aee9e42..3577ce5c 100644 --- a/internal/bridge/user_events.go +++ b/internal/bridge/user_events.go @@ -166,7 +166,8 @@ func (bridge *Bridge) handleUserRefreshed(ctx context.Context, user *user.User, func (bridge *Bridge) handleUserDeauth(ctx context.Context, user *user.User) { safe.Lock(func() { - bridge.logoutUser(ctx, user, false, false) + bridge.logoutUser(ctx, user, false, false, false) + user.ReportConfigStatusFailure("User deauth.") }, bridge.usersLock) } diff --git a/internal/configstatus/config_status.go b/internal/configstatus/config_status.go index 2c8cffd3..fb3d37fd 100644 --- a/internal/configstatus/config_status.go +++ b/internal/configstatus/config_status.go @@ -93,6 +93,13 @@ func (status *ConfigurationStatus) IsPending() bool { return !status.Data.DataV1.PendingSince.IsZero() } +func (status *ConfigurationStatus) IsFromFailure() bool { + status.DataLock.RLock() + defer status.DataLock.RUnlock() + + return status.Data.DataV1.FailureDetails != "" +} + func (status *ConfigurationStatus) ApplySuccess() error { status.DataLock.Lock() defer status.DataLock.Unlock() diff --git a/internal/configstatus/configuration_recovery.go b/internal/configstatus/configuration_recovery.go index 74fa09ce..dae2b96e 100644 --- a/internal/configstatus/configuration_recovery.go +++ b/internal/configstatus/configuration_recovery.go @@ -17,4 +17,44 @@ package configstatus -// GODT-2714 +import ( + "time" +) + +type ConfigRecoveryValues struct { + Duration int `json:"duration"` +} + +type ConfigRecoveryDimensions struct { + Autoconf string `json:"autoconf"` + ReportClick interface{} `json:"report_click"` + ReportSent interface{} `json:"report_sent"` + ClickedLink uint64 `json:"clicked_link"` + FailureDetails string `json:"failure_details"` +} + +type ConfigRecoveryData struct { + MeasurementGroup string + Event string + Values ConfigRecoveryValues + Dimensions ConfigRecoveryDimensions +} + +type ConfigRecoveryBuilder struct{} + +func (*ConfigRecoveryBuilder) New(data *ConfigurationStatusData) ConfigRecoveryData { + return ConfigRecoveryData{ + MeasurementGroup: "bridge.any.configuration", + Event: "bridge_config_recovery", + Values: ConfigRecoveryValues{ + Duration: int(time.Since(data.DataV1.PendingSince).Minutes()), + }, + Dimensions: ConfigRecoveryDimensions{ + Autoconf: data.DataV1.Autoconf, + ReportClick: data.DataV1.ReportClick, + ReportSent: data.DataV1.ReportSent, + ClickedLink: data.DataV1.ClickedLink, + FailureDetails: data.DataV1.FailureDetails, + }, + } +} diff --git a/internal/user/config_status.go b/internal/user/config_status.go index e77ea16b..febcdf57 100644 --- a/internal/user/config_status.go +++ b/internal/user/config_status.go @@ -26,6 +26,10 @@ import ( ) func (user *User) SendConfigStatusSuccess() { + if user.configStatus.IsFromFailure() { + user.SendConfigStatusRecovery() + return + } if !user.telemetryManager.IsTelemetryAvailable() { return } @@ -54,9 +58,6 @@ func (user *User) SendConfigStatusSuccess() { } func (user *User) SendConfigStatusAbort() { - if !user.telemetryManager.IsTelemetryAvailable() { - return - } if !user.configStatus.IsPending() { return } @@ -79,6 +80,35 @@ func (user *User) SendConfigStatusAbort() { } func (user *User) SendConfigStatusRecovery() { + if !user.configStatus.IsFromFailure() { + user.SendConfigStatusSuccess() + return + } + if !user.telemetryManager.IsTelemetryAvailable() { + return + } + if !user.configStatus.IsPending() { + return + } + + var builder configstatus.ConfigRecoveryBuilder + success := builder.New(user.configStatus.Data) + data, err := json.Marshal(success) + if err != nil { + if err := user.reporter.ReportMessageWithContext("Cannot parse config_recovery data.", reporter.Context{ + "error": err, + }); err != nil { + user.log.WithError(err).Error("Failed to report config_recovery data parsing error.") + } + return + } + + if err := user.SendTelemetry(context.Background(), data); err == nil { + user.log.Info("Configuration Status Recovery event sent.") + if err := user.configStatus.ApplySuccess(); err != nil { + user.log.WithError(err).Error("Failed to ApplySuccess on config_status.") + } + } } func (user *User) SendConfigStatusProgress() { @@ -112,3 +142,15 @@ func (user *User) SendConfigStatusProgress() { } } } + +func (user *User) ReportConfigStatusFailure(errDetails string) { + if user.configStatus.IsPending() { + return + } + + if err := user.configStatus.ApplyFailure(errDetails); err != nil { + user.log.WithError(err).Error("Failed to ApplyFailure on config_status.") + } else { + user.log.Info("Configuration Status is back to Pending due to Failure.") + } +} diff --git a/internal/user/user.go b/internal/user/user.go index 2fa2e7bd..cc27b519 100644 --- a/internal/user/user.go +++ b/internal/user/user.go @@ -514,7 +514,9 @@ func (user *User) CheckAuth(email string, password []byte) (string, error) { } if subtle.ConstantTimeCompare(user.vault.BridgePass(), dec) != 1 { - return "", fmt.Errorf("invalid password") + err := fmt.Errorf("invalid password") + user.ReportConfigStatusFailure(err.Error()) + return "", err } return safe.RLockRetErr(func() (string, error) { diff --git a/internal/user/user_test.go b/internal/user/user_test.go index 594e49c0..5b0ada78 100644 --- a/internal/user/user_test.go +++ b/internal/user/user_test.go @@ -148,6 +148,7 @@ func withUser(tb testing.TB, ctx context.Context, _ *server.Server, m *proton.Ma ctl := gomock.NewController(tb) defer ctl.Finish() manager := mocks.NewMockHeartbeatManager(ctl) + manager.EXPECT().IsTelemetryAvailable().AnyTimes() user, err := New(ctx, vaultUser, client, nil, apiUser, nil, true, vault.DefaultMaxSyncMemory, tb.TempDir(), manager) require.NoError(tb, err) defer user.Close()