From 2a5a6d65c4baf0e2177cff5a4baf155503e233cc Mon Sep 17 00:00:00 2001 From: David Dworken Date: Fri, 22 Sep 2023 13:13:46 -0700 Subject: [PATCH] Roll-forward "Add preliminary support for persisting pre-saved history entries remotely" This rolls-forward commit 66916c27cb36cc2ed941d93b573b196de3a5c60a. --- backend/server/internal/database/db.go | 11 ++++- backend/server/internal/server/server_test.go | 4 +- client/client_test.go | 27 ++++++++---- client/cmd/redact.go | 4 +- client/cmd/saveHistoryEntry.go | 41 +++++++++++-------- client/lib/lib.go | 5 ++- client/tui/tui.go | 4 +- shared/data.go | 22 ++++++---- 8 files changed, 76 insertions(+), 42 deletions(-) diff --git a/backend/server/internal/database/db.go b/backend/server/internal/database/db.go index 602a2ee..6c6e520 100644 --- a/backend/server/internal/database/db.go +++ b/backend/server/internal/database/db.go @@ -152,7 +152,14 @@ func (db *DB) DumpRequestDeleteForUserAndDevice(ctx context.Context, userID, dev func (db *DB) ApplyDeletionRequestsToBackend(ctx context.Context, request *shared.DeletionRequest) (int64, error) { tx := db.WithContext(ctx).Where("false") for _, message := range request.Messages.Ids { - tx = tx.Or(db.WithContext(ctx).Where("user_id = ? AND device_id = ? AND date = ?", request.UserId, message.DeviceId, message.Date)) + // Note that this won't do server-side deletion of pre-saved history entries. This is an inherent + // limitation of our current DB schema. This is sub-par, since it means that even after deletion, clients + // may still receive deleted history entries. But, a well-behaved client will immediately delete + // these (never writing them to disk) and mark them as received, so this won't happen again. + // + // TODO: This could be improved upon if we added a HistoryEntry.EntryId field, backfilled it, added + // it to EncHistoryEntry, and then used that as a key for deletion. + tx = tx.Or(db.WithContext(ctx).Where("user_id = ? AND device_id = ? AND date = ?", request.UserId, message.DeviceId, message.EndTime)) } result := tx.Delete(&shared.EncHistoryEntry{}) if tx.Error != nil { @@ -226,7 +233,7 @@ func (db *DB) Clean(ctx context.Context) error { if r.Error != nil { return r.Error } - r = db.WithContext(ctx).Exec("DELETE FROM deletion_requests WHERE read_count > 100") + r = db.WithContext(ctx).Exec("DELETE FROM deletion_requests WHERE read_count > 1000") if r.Error != nil { return r.Error } diff --git a/backend/server/internal/server/server_test.go b/backend/server/internal/server/server_test.go index b5ca045..0f706e6 100644 --- a/backend/server/internal/server/server_test.go +++ b/backend/server/internal/server/server_test.go @@ -411,7 +411,7 @@ func TestDeletionRequests(t *testing.T) { UserId: data.UserId("dkey"), SendTime: delReqTime, Messages: shared.MessageIdentifiers{Ids: []shared.MessageIdentifier{ - {DeviceId: devId1, Date: entry1.EndTime}, + {DeviceId: devId1, EndTime: entry1.EndTime}, }}, } reqBody, err = json.Marshal(delReq) @@ -507,7 +507,7 @@ func TestDeletionRequests(t *testing.T) { SendTime: delReqTime, ReadCount: 1, Messages: shared.MessageIdentifiers{Ids: []shared.MessageIdentifier{ - {DeviceId: devId1, Date: entry1.EndTime}, + {DeviceId: devId1, EndTime: entry1.EndTime}, }}, } if diff := deep.Equal(*deletionRequest, expected); diff != nil { diff --git a/client/client_test.go b/client/client_test.go index 513aeff..ac80335 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -2364,15 +2364,24 @@ func testPresaving(t *testing.T, tester shellTester) { out = tester.RunInteractiveShell(t, ` hishtory query sleep 13371337 -export -tquery`) testutils.CompareGoldens(t, out, "testPresaving-query") - // And the same for tquery - // out = captureTerminalOutputWithComplexCommands(t, tester, - // []TmuxCommand{ - // {Keys: "hishtory SPACE tquery ENTER", ExtraDelay: 2.0}, - // {Keys: "sleep SPACE 13371337 SPACE -export SPACE -tquery"}}) - // out = strings.TrimSpace(strings.Split(out, "hishtory tquery")[1]) - // testutils.CompareGoldens(t, out, "testPresaving-tquery") - // - // TODO: Debug why ^ is failing with flaky differences on Github Actions, see https://pastebin.com/BUa1btnh + // Create a new device, and confirm it shows up there too + restoreDevice1 := testutils.BackupAndRestoreWithId(t, "device1") + installHishtory(t, tester, userSecret) + tester.RunInteractiveShell(t, ` hishtory config-set displayed-columns CWD Runtime Command`) + out = tester.RunInteractiveShell(t, ` hishtory query sleep 13371337 -export -tquery`) + testutils.CompareGoldens(t, out, "testPresaving-query") + + // And then redact it from device2 + tester.RunInteractiveShell(t, ` HISHTORY_REDACT_FORCE=true hishtory redact sleep 13371337`) + + // And confirm it was redacted + out = tester.RunInteractiveShell(t, ` hishtory export sleep -export`) + require.Equal(t, "", out) + + // Then go back to device1 and confirm it was redacted there too + restoreDevice1() + out = tester.RunInteractiveShell(t, ` hishtory export sleep -export`) + require.Equal(t, "", out) } func testUninstall(t *testing.T, tester shellTester) { diff --git a/client/cmd/redact.go b/client/cmd/redact.go index aadb8e0..a79903e 100644 --- a/client/cmd/redact.go +++ b/client/cmd/redact.go @@ -86,7 +86,9 @@ func deleteOnRemoteInstances(ctx context.Context, historyEntries []*data.History deletionRequest.UserId = data.UserId(config.UserSecret) for _, entry := range historyEntries { - deletionRequest.Messages.Ids = append(deletionRequest.Messages.Ids, shared.MessageIdentifier{Date: entry.EndTime, DeviceId: entry.DeviceId}) + deletionRequest.Messages.Ids = append(deletionRequest.Messages.Ids, + shared.MessageIdentifier{StartTime: entry.StartTime, EndTime: entry.EndTime, DeviceId: entry.DeviceId}, + ) } return lib.SendDeletionRequest(deletionRequest) } diff --git a/client/cmd/saveHistoryEntry.go b/client/cmd/saveHistoryEntry.go index 2bd0ec2..9922c74 100644 --- a/client/cmd/saveHistoryEntry.go +++ b/client/cmd/saveHistoryEntry.go @@ -55,6 +55,7 @@ func maybeUploadSkippedHistoryEntries(ctx context.Context) error { // Upload the missing entries db := hctx.GetDb(ctx) + // TODO: There is a bug here because MissedUploadTimestamp is going to be a second or two after the history entry that needs to be uploaded query := fmt.Sprintf("after:%s", time.Unix(config.MissedUploadTimestamp, 0).Format("2006-01-02")) entries, err := lib.Search(ctx, db, query, 0) if err != nil { @@ -81,6 +82,21 @@ func maybeUploadSkippedHistoryEntries(ctx context.Context) error { return nil } +func handlePotentialUploadFailure(err error, config *hctx.ClientConfig) { + if err != nil { + if lib.IsOfflineError(err) { + hctx.GetLogger().Infof("Failed to remotely persist hishtory entry because we failed to connect to the remote server! This is likely because the device is offline, but also could be because the remote server is having reliability issues. Original error: %v", err) + if !config.HaveMissedUploads { + config.HaveMissedUploads = true + config.MissedUploadTimestamp = time.Now().Unix() + lib.CheckFatalError(hctx.SetConfig(*config)) + } + } else { + lib.CheckFatalError(err) + } + } +} + func presaveHistoryEntry(ctx context.Context) { config := hctx.GetConf(ctx) if !config.IsEnabled { @@ -119,12 +135,13 @@ func presaveHistoryEntry(ctx context.Context) { lib.CheckFatalError(err) db.Commit() - // Note that we aren't persisting these half-entries remotely, - // since they should be updated with the rest of the information very soon. If they - // are never updated (e.g. due to a long-running command that never finishes), then - // they will only be available on this device. That isn't perfect since it means - // history entries can get out of sync, but it is probably good enough. - // TODO: Consider improving this + // And persist it remotely + if !config.IsOffline { + jsonValue, err := lib.EncryptAndMarshal(config, []*data.HistoryEntry{entry}) + lib.CheckFatalError(err) + _, err = lib.ApiPost("/api/v1/submit?source_device_id="+config.DeviceId, "application/json", jsonValue) + handlePotentialUploadFailure(err, &config) + } } func saveHistoryEntry(ctx context.Context) { @@ -175,6 +192,7 @@ func saveHistoryEntry(ctx context.Context) { jsonValue, err := lib.EncryptAndMarshal(config, []*data.HistoryEntry{entry}) lib.CheckFatalError(err) w, err := lib.ApiPost("/api/v1/submit?source_device_id="+config.DeviceId, "application/json", jsonValue) + handlePotentialUploadFailure(err, &config) if err == nil { submitResponse := shared.SubmitResponse{} err := json.Unmarshal(w, &submitResponse) @@ -183,17 +201,6 @@ func saveHistoryEntry(ctx context.Context) { } shouldCheckForDeletionRequests = submitResponse.HaveDeletionRequests shouldCheckForDumpRequests = submitResponse.HaveDumpRequests - } else { - if lib.IsOfflineError(err) { - hctx.GetLogger().Infof("Failed to remotely persist hishtory entry because we failed to connect to the remote server! This is likely because the device is offline, but also could be because the remote server is having reliability issues. Original error: %v", err) - if !config.HaveMissedUploads { - config.HaveMissedUploads = true - config.MissedUploadTimestamp = time.Now().Unix() - lib.CheckFatalError(hctx.SetConfig(config)) - } - } else { - lib.CheckFatalError(err) - } } } diff --git a/client/lib/lib.go b/client/lib/lib.go index 9a4faa7..4caf710 100644 --- a/client/lib/lib.go +++ b/client/lib/lib.go @@ -600,7 +600,10 @@ func ProcessDeletionRequests(ctx context.Context) error { db := hctx.GetDb(ctx) for _, request := range deletionRequests { for _, entry := range request.Messages.Ids { - res := db.Where("device_id = ? AND end_time = ?", entry.DeviceId, entry.Date).Delete(&data.HistoryEntry{}) + // Note that entry.StartTime is not always present (for legacy reasons) and entry.EndTime is also + // not always present (for pre-saved entries). So we just check that one of them matches. + tx := db.Where("device_id = ? AND (start_time = ? OR end_time = ?)", entry.DeviceId, entry.StartTime, entry.EndTime) + res := tx.Delete(&data.HistoryEntry{}) if res.Error != nil { return fmt.Errorf("DB error: %w", res.Error) } diff --git a/client/tui/tui.go b/client/tui/tui.go index e0d73fa..ed65dcf 100644 --- a/client/tui/tui.go +++ b/client/tui/tui.go @@ -601,7 +601,9 @@ func deleteHistoryEntry(ctx context.Context, entry data.HistoryEntry) error { UserId: data.UserId(hctx.GetConf(ctx).UserSecret), SendTime: time.Now(), } - dr.Messages.Ids = append(dr.Messages.Ids, shared.MessageIdentifier{Date: entry.EndTime, DeviceId: entry.DeviceId}) + dr.Messages.Ids = append(dr.Messages.Ids, + shared.MessageIdentifier{StartTime: entry.StartTime, EndTime: entry.EndTime, DeviceId: entry.DeviceId}, + ) return lib.SendDeletionRequest(dr) } diff --git a/shared/data.go b/shared/data.go index 2a59282..0c6cc79 100644 --- a/shared/data.go +++ b/shared/data.go @@ -9,13 +9,14 @@ import ( // Represents an encrypted history entry type EncHistoryEntry struct { - EncryptedData []byte `json:"enc_data"` - Nonce []byte `json:"nonce"` - DeviceId string `json:"device_id"` - UserId string `json:"user_id"` - Date time.Time `json:"time"` - EncryptedId string `json:"id"` - ReadCount int `json:"read_count"` + EncryptedData []byte `json:"enc_data"` + Nonce []byte `json:"nonce"` + DeviceId string `json:"device_id"` + UserId string `json:"user_id"` + // Note that EncHistoryEntry.Date == HistoryEntry.EndTime + Date time.Time `json:"time"` + EncryptedId string `json:"id"` + ReadCount int `json:"read_count"` } /* @@ -87,8 +88,11 @@ type MessageIdentifiers struct { type MessageIdentifier struct { // The device that the entry was recorded on (NOT the device where it is stored/requesting deletion) DeviceId string `json:"device_id"` - // The timestamp when the message finished running - Date time.Time `json:"date"` + // The timestamp when the command finished running. Serialized as "date" for legacy compatibility. + EndTime time.Time `json:"date"` + // The timestamp when the command started running. + // Note this field was added as part of supporting pre-saving commands, so older clients do not set this field + StartTime time.Time `json:"start_time"` } func (m *MessageIdentifiers) Scan(value interface{}) error {