From 1d878195b201d6c279abb60c15063f7346c17028 Mon Sep 17 00:00:00 2001 From: David Dworken Date: Fri, 22 Sep 2023 14:03:41 -0700 Subject: [PATCH] Rework ff98a7907c9d05b4763920f46dec7c4b331b91d1 to use the newly added EntryId column rather than deleting based on the start time --- backend/server/internal/database/db.go | 10 ++-------- client/cmd/redact.go | 2 +- client/lib/lib.go | 6 +++--- client/tui/tui.go | 2 +- shared/data.go | 16 ++++++++++------ 5 files changed, 17 insertions(+), 19 deletions(-) diff --git a/backend/server/internal/database/db.go b/backend/server/internal/database/db.go index 6c6e520..f4ef87f 100644 --- a/backend/server/internal/database/db.go +++ b/backend/server/internal/database/db.go @@ -152,14 +152,8 @@ 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)) + // Note that we do an OR with date or the ID matching since the ID is not always recorded for older history entries. + tx = tx.Or(db.WithContext(ctx).Where("user_id = ? AND device_id = ? AND (date = ? OR id = ?)", request.UserId, message.DeviceId, message.EndTime, message.EntryId)) } result := tx.Delete(&shared.EncHistoryEntry{}) if tx.Error != nil { diff --git a/client/cmd/redact.go b/client/cmd/redact.go index a79903e..998cd93 100644 --- a/client/cmd/redact.go +++ b/client/cmd/redact.go @@ -87,7 +87,7 @@ func deleteOnRemoteInstances(ctx context.Context, historyEntries []*data.History for _, entry := range historyEntries { deletionRequest.Messages.Ids = append(deletionRequest.Messages.Ids, - shared.MessageIdentifier{StartTime: entry.StartTime, EndTime: entry.EndTime, DeviceId: entry.DeviceId}, + shared.MessageIdentifier{DeviceId: entry.DeviceId, EndTime: entry.EndTime, EntryId: entry.EntryId}, ) } return lib.SendDeletionRequest(deletionRequest) diff --git a/client/lib/lib.go b/client/lib/lib.go index 6d37a9c..817d20a 100644 --- a/client/lib/lib.go +++ b/client/lib/lib.go @@ -605,9 +605,9 @@ func HandleDeletionRequests(ctx context.Context, deletionRequests []*shared.Dele 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) + // Note that entry.EndTime is not always present (for pre-saved entries). And likewise, + // entry.EntryId is not always present for older entries. So we just check that one of them matches. + tx := db.Where("device_id = ? AND (end_time = ? OR entry_id = ?)", entry.DeviceId, entry.EndTime, entry.EntryId) 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 ed65dcf..4f81cd1 100644 --- a/client/tui/tui.go +++ b/client/tui/tui.go @@ -602,7 +602,7 @@ func deleteHistoryEntry(ctx context.Context, entry data.HistoryEntry) error { SendTime: time.Now(), } dr.Messages.Ids = append(dr.Messages.Ids, - shared.MessageIdentifier{StartTime: entry.StartTime, EndTime: entry.EndTime, DeviceId: entry.DeviceId}, + shared.MessageIdentifier{DeviceId: entry.DeviceId, EndTime: entry.EndTime, EntryId: entry.EntryId}, ) return lib.SendDeletionRequest(dr) } diff --git a/shared/data.go b/shared/data.go index b245bdb..a48e7bf 100644 --- a/shared/data.go +++ b/shared/data.go @@ -14,13 +14,15 @@ type EncHistoryEntry struct { 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"` + Date time.Time `json:"time"` + // Note that EncHistoryEntry.EncryptedId == HistoryEntry.Id (for entries created after pre-saving support) + EncryptedId string `json:"encrypted_id"` + ReadCount int `json:"read_count"` } /* Manually created the indices: +CREATE INDEX CONCURRENTLY entry_id_idx ON enc_history_entries USING btree(encrypted_id); CREATE INDEX CONCURRENTLY device_id_idx ON enc_history_entries USING btree(device_id); CREATE INDEX CONCURRENTLY read_count_idx ON enc_history_entries USING btree(read_count); CREATE INDEX CONCURRENTLY redact_idx ON enc_history_entries USING btree(user_id, device_id, date); @@ -82,7 +84,7 @@ type MessageIdentifiers struct { Ids []MessageIdentifier `json:"message_ids"` } -// Identifies a single history entry based on the device that recorded the entry, and the end time. Note that +// Identifies a single history entry based on the device that recorded the entry, and additional metadata. Note that // this does not include the command itself since that would risk including the sensitive data that is meant // to be deleted type MessageIdentifier struct { @@ -90,9 +92,11 @@ type MessageIdentifier struct { 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. + // The entry ID of the command. // 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"` + // And even for new clients, it may contain a per-device entry ID. For pre-saved entries, this is guaranteed to + // be present. + EntryId string `json:"entry_id"` } func (m *MessageIdentifiers) Scan(value interface{}) error {