fix some redact bugs (#2320)

* Consistently return UNKNOWN_MSGID for unknown or invalid msgids
* If both client's DMs are stored in persistent history, a single
  server.DeleteMessage will delete the single canonical copy of the message.
  So the second call will fail, which is fine.

irc/handlers.go

@@ -2842,10 +2842,16 @@ func redactHandler(server *Server, client *Client, msg ircmsg.Message, rb *Respo
 	}
 
 	err := server.DeleteMessage(target, targetmsgid, accountName)
-	if err == errNoop {
+	switch err {
+	case history.ErrNotFound:
 		rb.Add(nil, server.name, "FAIL", "REDACT", "UNKNOWN_MSGID", utils.SafeErrorParam(target), utils.SafeErrorParam(targetmsgid), client.t("This message does not exist or is too old"))
 		return false
-	} else if err != nil {
+	case history.ErrDisallowed:
+		rb.Add(nil, server.name, "FAIL", "REDACT", "REDACT_FORBIDDEN", utils.SafeErrorParam(target), utils.SafeErrorParam(targetmsgid), client.t("You are not authorized to delete this message"))
+		return false
+	case nil:
+		// OK
+	default:
 		isOper := client.HasRoleCapabs("history")
 		if isOper {
 			rb.Add(nil, server.name, "FAIL", "REDACT", "REDACT_FORBIDDEN", utils.SafeErrorParam(target), utils.SafeErrorParam(targetmsgid), fmt.Sprintf(client.t("Error deleting message: %v"), err))
@@ -2860,7 +2866,8 @@ func redactHandler(server *Server, client *Client, msg ircmsg.Message, rb *Respo
 		// now we have to remove it from the buffer of the client who sent the REDACT command
 		err := server.DeleteMessage(client.Nick(), targetmsgid, accountName)
 
-		if err != nil {
+		// ErrNotFound is expected if both clients are using persistent history
+		if err != nil && err != history.ErrNotFound {
 			client.server.logger.Error("internal", fmt.Sprintf("Private message %s is not deletable by %s from their own buffer's even though we just deleted it from %s's. This is a bug, please report it in details.", targetmsgid, client.Nick(), target), client.Nick())
 			isOper := client.HasRoleCapabs("history")
 			if isOper {

irc/history/database.go

@@ -4,10 +4,16 @@
 package history
 
 import (
+	"errors"
 	"io"
 	"time"
 )
 
+var (
+	ErrDisallowed = errors.New("disallowed")
+	ErrNotFound   = errors.New("not found")
+)
+
 // Database is an interface for persistent history storage backends.
 type Database interface {
 	// Close closes the database connection and releases resources.

irc/mysql/history.go

@@ -7,7 +7,6 @@ import (
 	"context"
 	"database/sql"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io"
 	"runtime/debug"
@@ -23,10 +22,6 @@ import (
 	_ "github.com/go-sql-driver/mysql"
 )
 
-var (
-	ErrDisallowed = errors.New("disallowed")
-)
-
 const (
 	// maximum length in bytes of any message target (nickname or channel name) in its
 	// canonicalized (i.e., casefolded) state:
@@ -749,6 +744,9 @@ func (mysql *MySQL) DeleteMsgid(msgid, accountName string) (err error) {
 
 	_, id, data, err := mysql.lookupMsgid(ctx, msgid, true)
 	if err != nil {
+		if err == sql.ErrNoRows {
+			return history.ErrNotFound
+		}
 		return
 	}
 
@@ -757,7 +755,7 @@ func (mysql *MySQL) DeleteMsgid(msgid, accountName string) (err error) {
 		err = history.UnmarshalItem(data, &item)
 		// delete if the entry is corrupt
 		if err == nil && item.AccountName != accountName {
-			return ErrDisallowed
+			return history.ErrDisallowed
 		}
 	}
 
@@ -830,6 +828,9 @@ func (mysql *MySQL) Export(account string, writer io.Writer) {
 func (mysql *MySQL) lookupMsgid(ctx context.Context, msgid string, includeData bool) (result time.Time, id uint64, data []byte, err error) {
 	decoded, err := utils.DecodeSecretToken(msgid)
 	if err != nil {
+		// use sql.ErrNoRows internally for consistency, translate to history.ErrNotFound
+		// at the package boundary if necessary
+		err = sql.ErrNoRows
 		return
 	}
 	cols := `sequence.nanotime, conversations.nanotime`

irc/server.go

@@ -1242,7 +1242,7 @@ func (server *Server) DeleteMessage(target, msgid, accountName string) (err erro
 			return item.Message.Msgid == msgid && (accountName == "*" || item.AccountName == accountName)
 		})
 		if count == 0 {
-			err = errNoop
+			err = history.ErrNotFound
 		}
 	}