From 66916c27cb36cc2ed941d93b573b196de3a5c60a Mon Sep 17 00:00:00 2001 From: David Dworken Date: Thu, 21 Sep 2023 12:39:20 -0700 Subject: [PATCH] Revert "Add preliminary support for persisting pre-saved history entries remotely" This reverts commit ff98a7907c9d05b4763920f46dec7c4b331b91d1. That commit is incomplete since it doesn't include support for the continous deletion of pre-saved history entries as soon as they finish running. Support for this will require a good bit more work/thought, so reverting for and keeping this code in the git history. --- 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, 42 insertions(+), 76 deletions(-) diff --git a/backend/server/internal/database/db.go b/backend/server/internal/database/db.go index 6c6e520..602a2ee 100644 --- a/backend/server/internal/database/db.go +++ b/backend/server/internal/database/db.go @@ -152,14 +152,7 @@ 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 { - // 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)) + tx = tx.Or(db.WithContext(ctx).Where("user_id = ? AND device_id = ? AND date = ?", request.UserId, message.DeviceId, message.Date)) } result := tx.Delete(&shared.EncHistoryEntry{}) if tx.Error != nil { @@ -233,7 +226,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 > 1000") + r = db.WithContext(ctx).Exec("DELETE FROM deletion_requests WHERE read_count > 100") 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 0f706e6..b5ca045 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, EndTime: entry1.EndTime}, + {DeviceId: devId1, Date: 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, EndTime: entry1.EndTime}, + {DeviceId: devId1, Date: entry1.EndTime}, }}, } if diff := deep.Equal(*deletionRequest, expected); diff != nil { diff --git a/client/client_test.go b/client/client_test.go index cade01e..6543d80 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -2364,24 +2364,15 @@ func testPresaving(t *testing.T, tester shellTester) { out = tester.RunInteractiveShell(t, ` hishtory query sleep 13371337 -export -tquery`) testutils.CompareGoldens(t, out, "testPresaving-query") - // 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) + // 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 } func testUninstall(t *testing.T, tester shellTester) { diff --git a/client/cmd/redact.go b/client/cmd/redact.go index a79903e..aadb8e0 100644 --- a/client/cmd/redact.go +++ b/client/cmd/redact.go @@ -86,9 +86,7 @@ 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{StartTime: entry.StartTime, EndTime: entry.EndTime, DeviceId: entry.DeviceId}, - ) + deletionRequest.Messages.Ids = append(deletionRequest.Messages.Ids, shared.MessageIdentifier{Date: entry.EndTime, DeviceId: entry.DeviceId}) } return lib.SendDeletionRequest(deletionRequest) } diff --git a/client/cmd/saveHistoryEntry.go b/client/cmd/saveHistoryEntry.go index 9922c74..2bd0ec2 100644 --- a/client/cmd/saveHistoryEntry.go +++ b/client/cmd/saveHistoryEntry.go @@ -55,7 +55,6 @@ 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 { @@ -82,21 +81,6 @@ 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 { @@ -135,13 +119,12 @@ func presaveHistoryEntry(ctx context.Context) { lib.CheckFatalError(err) db.Commit() - // 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) - } + // 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 } func saveHistoryEntry(ctx context.Context) { @@ -192,7 +175,6 @@ 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) @@ -201,6 +183,17 @@ 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 4caf710..9a4faa7 100644 --- a/client/lib/lib.go +++ b/client/lib/lib.go @@ -600,10 +600,7 @@ func ProcessDeletionRequests(ctx context.Context) error { db := hctx.GetDb(ctx) for _, request := range deletionRequests { for _, entry := range request.Messages.Ids { - // 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{}) + res := db.Where("device_id = ? AND end_time = ?", entry.DeviceId, entry.Date).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 ed65dcf..e0d73fa 100644 --- a/client/tui/tui.go +++ b/client/tui/tui.go @@ -601,9 +601,7 @@ 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{StartTime: entry.StartTime, EndTime: entry.EndTime, DeviceId: entry.DeviceId}, - ) + dr.Messages.Ids = append(dr.Messages.Ids, shared.MessageIdentifier{Date: entry.EndTime, DeviceId: entry.DeviceId}) return lib.SendDeletionRequest(dr) } diff --git a/shared/data.go b/shared/data.go index 0c6cc79..2a59282 100644 --- a/shared/data.go +++ b/shared/data.go @@ -9,14 +9,13 @@ 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"` - // Note that EncHistoryEntry.Date == HistoryEntry.EndTime - 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"` + Date time.Time `json:"time"` + EncryptedId string `json:"id"` + ReadCount int `json:"read_count"` } /* @@ -88,11 +87,8 @@ 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 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"` + // The timestamp when the message finished running + Date time.Time `json:"date"` } func (m *MessageIdentifiers) Scan(value interface{}) error {