Alias key backup endpoints onto /unstable, fix key backup bugs (#1947)

* Default /unstable requests to stable endpoints if not overridden specifically with a custom route

* Rewrite URL

* Try something different

* Fix routing manually

* Fix selectLatestVersionSQL

* Don't return 0 if no backup version exists

* Log more useful error

* fix up replace keys check

* Don't enforce uniqueness on e2e_room_keys_versions_idx

Co-authored-by: kegsay <kegan@matrix.org>

clientapi/routing/routing.go

@@ -898,157 +898,171 @@ func Setup(
 
 	// Key Backup Versions (Metadata)
 
-	r0mux.Handle("/room_keys/version/{version}",
-		httputil.MakeAuthAPI("get_backup_keys_version", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
-			vars, err := httputil.URLDecodeMapValues(mux.Vars(req))
-			if err != nil {
-				return util.ErrorResponse(err)
-			}
-			return KeyBackupVersion(req, userAPI, device, vars["version"])
-		}),
-	).Methods(http.MethodGet, http.MethodOptions)
-	r0mux.Handle("/room_keys/version",
-		httputil.MakeAuthAPI("get_latest_backup_keys_version", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
-			return KeyBackupVersion(req, userAPI, device, "")
-		}),
-	).Methods(http.MethodGet, http.MethodOptions)
-	r0mux.Handle("/room_keys/version/{version}",
-		httputil.MakeAuthAPI("put_backup_keys_version", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
-			vars, err := httputil.URLDecodeMapValues(mux.Vars(req))
-			if err != nil {
-				return util.ErrorResponse(err)
-			}
-			return ModifyKeyBackupVersionAuthData(req, userAPI, device, vars["version"])
-		}),
-	).Methods(http.MethodPut)
-	r0mux.Handle("/room_keys/version/{version}",
-		httputil.MakeAuthAPI("delete_backup_keys_version", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
-			vars, err := httputil.URLDecodeMapValues(mux.Vars(req))
-			if err != nil {
-				return util.ErrorResponse(err)
-			}
-			return DeleteKeyBackupVersion(req, userAPI, device, vars["version"])
-		}),
-	).Methods(http.MethodDelete)
-	r0mux.Handle("/room_keys/version",
-		httputil.MakeAuthAPI("post_new_backup_keys_version", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
-			return CreateKeyBackupVersion(req, userAPI, device)
-		}),
-	).Methods(http.MethodPost, http.MethodOptions)
+	getBackupKeysVersion := httputil.MakeAuthAPI("get_backup_keys_version", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
+		vars, err := httputil.URLDecodeMapValues(mux.Vars(req))
+		if err != nil {
+			return util.ErrorResponse(err)
+		}
+		return KeyBackupVersion(req, userAPI, device, vars["version"])
+	})
+
+	getLatestBackupKeysVersion := httputil.MakeAuthAPI("get_latest_backup_keys_version", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
+		return KeyBackupVersion(req, userAPI, device, "")
+	})
+
+	putBackupKeysVersion := httputil.MakeAuthAPI("put_backup_keys_version", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
+		vars, err := httputil.URLDecodeMapValues(mux.Vars(req))
+		if err != nil {
+			return util.ErrorResponse(err)
+		}
+		return ModifyKeyBackupVersionAuthData(req, userAPI, device, vars["version"])
+	})
+
+	deleteBackupKeysVersion := httputil.MakeAuthAPI("delete_backup_keys_version", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
+		vars, err := httputil.URLDecodeMapValues(mux.Vars(req))
+		if err != nil {
+			return util.ErrorResponse(err)
+		}
+		return DeleteKeyBackupVersion(req, userAPI, device, vars["version"])
+	})
+
+	postNewBackupKeysVersion := httputil.MakeAuthAPI("post_new_backup_keys_version", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
+		return CreateKeyBackupVersion(req, userAPI, device)
+	})
+
+	r0mux.Handle("/room_keys/version/{version}", getBackupKeysVersion).Methods(http.MethodGet, http.MethodOptions)
+	r0mux.Handle("/room_keys/version", getLatestBackupKeysVersion).Methods(http.MethodGet, http.MethodOptions)
+	r0mux.Handle("/room_keys/version/{version}", putBackupKeysVersion).Methods(http.MethodPut)
+	r0mux.Handle("/room_keys/version/{version}", deleteBackupKeysVersion).Methods(http.MethodDelete)
+	r0mux.Handle("/room_keys/version", postNewBackupKeysVersion).Methods(http.MethodPost, http.MethodOptions)
+
+	unstableMux.Handle("/room_keys/version/{version}", getBackupKeysVersion).Methods(http.MethodGet, http.MethodOptions)
+	unstableMux.Handle("/room_keys/version", getLatestBackupKeysVersion).Methods(http.MethodGet, http.MethodOptions)
+	unstableMux.Handle("/room_keys/version/{version}", putBackupKeysVersion).Methods(http.MethodPut)
+	unstableMux.Handle("/room_keys/version/{version}", deleteBackupKeysVersion).Methods(http.MethodDelete)
+	unstableMux.Handle("/room_keys/version", postNewBackupKeysVersion).Methods(http.MethodPost, http.MethodOptions)
 
 	// Inserting E2E Backup Keys
 
 	// Bulk room and session
-	r0mux.Handle("/room_keys/keys",
-		httputil.MakeAuthAPI("put_backup_keys", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
-			version := req.URL.Query().Get("version")
-			if version == "" {
-				return util.JSONResponse{
-					Code: 400,
-					JSON: jsonerror.InvalidArgumentValue("version must be specified"),
-				}
+	putBackupKeys := httputil.MakeAuthAPI("put_backup_keys", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
+		version := req.URL.Query().Get("version")
+		if version == "" {
+			return util.JSONResponse{
+				Code: 400,
+				JSON: jsonerror.InvalidArgumentValue("version must be specified"),
 			}
-			var reqBody keyBackupSessionRequest
-			resErr := clientutil.UnmarshalJSONRequest(req, &reqBody)
-			if resErr != nil {
-				return *resErr
-			}
-			return UploadBackupKeys(req, userAPI, device, version, &reqBody)
-		}),
-	).Methods(http.MethodPut)
+		}
+		var reqBody keyBackupSessionRequest
+		resErr := clientutil.UnmarshalJSONRequest(req, &reqBody)
+		if resErr != nil {
+			return *resErr
+		}
+		return UploadBackupKeys(req, userAPI, device, version, &reqBody)
+	})
+
 	// Single room bulk session
-	r0mux.Handle("/room_keys/keys/{roomID}",
-		httputil.MakeAuthAPI("put_backup_keys_room", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
-			vars, err := httputil.URLDecodeMapValues(mux.Vars(req))
-			if err != nil {
-				return util.ErrorResponse(err)
+	putBackupKeysRoom := httputil.MakeAuthAPI("put_backup_keys_room", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
+		vars, err := httputil.URLDecodeMapValues(mux.Vars(req))
+		if err != nil {
+			return util.ErrorResponse(err)
+		}
+		version := req.URL.Query().Get("version")
+		if version == "" {
+			return util.JSONResponse{
+				Code: 400,
+				JSON: jsonerror.InvalidArgumentValue("version must be specified"),
 			}
-			version := req.URL.Query().Get("version")
-			if version == "" {
-				return util.JSONResponse{
-					Code: 400,
-					JSON: jsonerror.InvalidArgumentValue("version must be specified"),
-				}
-			}
-			roomID := vars["roomID"]
-			var reqBody keyBackupSessionRequest
-			reqBody.Rooms = make(map[string]struct {
-				Sessions map[string]userapi.KeyBackupSession `json:"sessions"`
-			})
-			reqBody.Rooms[roomID] = struct {
-				Sessions map[string]userapi.KeyBackupSession `json:"sessions"`
-			}{
-				Sessions: map[string]userapi.KeyBackupSession{},
-			}
-			body := reqBody.Rooms[roomID]
-			resErr := clientutil.UnmarshalJSONRequest(req, &body)
-			if resErr != nil {
-				return *resErr
-			}
-			reqBody.Rooms[roomID] = body
-			return UploadBackupKeys(req, userAPI, device, version, &reqBody)
-		}),
-	).Methods(http.MethodPut)
+		}
+		roomID := vars["roomID"]
+		var reqBody keyBackupSessionRequest
+		reqBody.Rooms = make(map[string]struct {
+			Sessions map[string]userapi.KeyBackupSession `json:"sessions"`
+		})
+		reqBody.Rooms[roomID] = struct {
+			Sessions map[string]userapi.KeyBackupSession `json:"sessions"`
+		}{
+			Sessions: map[string]userapi.KeyBackupSession{},
+		}
+		body := reqBody.Rooms[roomID]
+		resErr := clientutil.UnmarshalJSONRequest(req, &body)
+		if resErr != nil {
+			return *resErr
+		}
+		reqBody.Rooms[roomID] = body
+		return UploadBackupKeys(req, userAPI, device, version, &reqBody)
+	})
+
 	// Single room, single session
-	r0mux.Handle("/room_keys/keys/{roomID}/{sessionID}",
-		httputil.MakeAuthAPI("put_backup_keys_room_session", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
-			vars, err := httputil.URLDecodeMapValues(mux.Vars(req))
-			if err != nil {
-				return util.ErrorResponse(err)
+	putBackupKeysRoomSession := httputil.MakeAuthAPI("put_backup_keys_room_session", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
+		vars, err := httputil.URLDecodeMapValues(mux.Vars(req))
+		if err != nil {
+			return util.ErrorResponse(err)
+		}
+		version := req.URL.Query().Get("version")
+		if version == "" {
+			return util.JSONResponse{
+				Code: 400,
+				JSON: jsonerror.InvalidArgumentValue("version must be specified"),
 			}
-			version := req.URL.Query().Get("version")
-			if version == "" {
-				return util.JSONResponse{
-					Code: 400,
-					JSON: jsonerror.InvalidArgumentValue("version must be specified"),
-				}
-			}
-			var reqBody userapi.KeyBackupSession
-			resErr := clientutil.UnmarshalJSONRequest(req, &reqBody)
-			if resErr != nil {
-				return *resErr
-			}
-			roomID := vars["roomID"]
-			sessionID := vars["sessionID"]
-			var keyReq keyBackupSessionRequest
-			keyReq.Rooms = make(map[string]struct {
-				Sessions map[string]userapi.KeyBackupSession `json:"sessions"`
-			})
-			keyReq.Rooms[roomID] = struct {
-				Sessions map[string]userapi.KeyBackupSession `json:"sessions"`
-			}{
-				Sessions: make(map[string]userapi.KeyBackupSession),
-			}
-			keyReq.Rooms[roomID].Sessions[sessionID] = reqBody
-			return UploadBackupKeys(req, userAPI, device, version, &keyReq)
-		}),
-	).Methods(http.MethodPut)
+		}
+		var reqBody userapi.KeyBackupSession
+		resErr := clientutil.UnmarshalJSONRequest(req, &reqBody)
+		if resErr != nil {
+			return *resErr
+		}
+		roomID := vars["roomID"]
+		sessionID := vars["sessionID"]
+		var keyReq keyBackupSessionRequest
+		keyReq.Rooms = make(map[string]struct {
+			Sessions map[string]userapi.KeyBackupSession `json:"sessions"`
+		})
+		keyReq.Rooms[roomID] = struct {
+			Sessions map[string]userapi.KeyBackupSession `json:"sessions"`
+		}{
+			Sessions: make(map[string]userapi.KeyBackupSession),
+		}
+		keyReq.Rooms[roomID].Sessions[sessionID] = reqBody
+		return UploadBackupKeys(req, userAPI, device, version, &keyReq)
+	})
+
+	r0mux.Handle("/room_keys/keys", putBackupKeys).Methods(http.MethodPut)
+	r0mux.Handle("/room_keys/keys/{roomID}", putBackupKeysRoom).Methods(http.MethodPut)
+	r0mux.Handle("/room_keys/keys/{roomID}/{sessionID}", putBackupKeysRoomSession).Methods(http.MethodPut)
+
+	unstableMux.Handle("/room_keys/keys", putBackupKeys).Methods(http.MethodPut)
+	unstableMux.Handle("/room_keys/keys/{roomID}", putBackupKeysRoom).Methods(http.MethodPut)
+	unstableMux.Handle("/room_keys/keys/{roomID}/{sessionID}", putBackupKeysRoomSession).Methods(http.MethodPut)
 
 	// Querying E2E Backup Keys
 
-	r0mux.Handle("/room_keys/keys",
-		httputil.MakeAuthAPI("get_backup_keys", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
-			return GetBackupKeys(req, userAPI, device, req.URL.Query().Get("version"), "", "")
-		}),
-	).Methods(http.MethodGet, http.MethodOptions)
-	r0mux.Handle("/room_keys/keys/{roomID}",
-		httputil.MakeAuthAPI("get_backup_keys_room", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
-			vars, err := httputil.URLDecodeMapValues(mux.Vars(req))
-			if err != nil {
-				return util.ErrorResponse(err)
-			}
-			return GetBackupKeys(req, userAPI, device, req.URL.Query().Get("version"), vars["roomID"], "")
-		}),
-	).Methods(http.MethodGet, http.MethodOptions)
-	r0mux.Handle("/room_keys/keys/{roomID}/{sessionID}",
-		httputil.MakeAuthAPI("get_backup_keys_room_session", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
-			vars, err := httputil.URLDecodeMapValues(mux.Vars(req))
-			if err != nil {
-				return util.ErrorResponse(err)
-			}
-			return GetBackupKeys(req, userAPI, device, req.URL.Query().Get("version"), vars["roomID"], vars["sessionID"])
-		}),
-	).Methods(http.MethodGet, http.MethodOptions)
+	getBackupKeys := httputil.MakeAuthAPI("get_backup_keys", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
+		return GetBackupKeys(req, userAPI, device, req.URL.Query().Get("version"), "", "")
+	})
+
+	getBackupKeysRoom := httputil.MakeAuthAPI("get_backup_keys_room", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
+		vars, err := httputil.URLDecodeMapValues(mux.Vars(req))
+		if err != nil {
+			return util.ErrorResponse(err)
+		}
+		return GetBackupKeys(req, userAPI, device, req.URL.Query().Get("version"), vars["roomID"], "")
+	})
+
+	getBackupKeysRoomSession := httputil.MakeAuthAPI("get_backup_keys_room_session", userAPI, func(req *http.Request, device *userapi.Device) util.JSONResponse {
+		vars, err := httputil.URLDecodeMapValues(mux.Vars(req))
+		if err != nil {
+			return util.ErrorResponse(err)
+		}
+		return GetBackupKeys(req, userAPI, device, req.URL.Query().Get("version"), vars["roomID"], vars["sessionID"])
+	})
+
+	r0mux.Handle("/room_keys/keys", getBackupKeys).Methods(http.MethodGet, http.MethodOptions)
+	r0mux.Handle("/room_keys/keys/{roomID}", getBackupKeysRoom).Methods(http.MethodGet, http.MethodOptions)
+	r0mux.Handle("/room_keys/keys/{roomID}/{sessionID}", getBackupKeysRoomSession).Methods(http.MethodGet, http.MethodOptions)
+
+	unstableMux.Handle("/room_keys/keys", getBackupKeys).Methods(http.MethodGet, http.MethodOptions)
+	unstableMux.Handle("/room_keys/keys/{roomID}", getBackupKeysRoom).Methods(http.MethodGet, http.MethodOptions)
+	unstableMux.Handle("/room_keys/keys/{roomID}/{sessionID}", getBackupKeysRoomSession).Methods(http.MethodGet, http.MethodOptions)
 
 	// Deleting E2E Backup Keys
 

userapi/api/api.go

@@ -72,13 +72,11 @@ func (a *KeyBackupSession) ShouldReplaceRoomKey(newKey *KeyBackupSession) bool {
 	// "if the keys have different values for is_verified, then it will keep the key that has is_verified set to true"
 	if newKey.IsVerified && !a.IsVerified {
 		return true
-	}
-	// "if they have the same values for is_verified, then it will keep the key with a lower first_message_index"
-	if newKey.FirstMessageIndex < a.FirstMessageIndex {
+	} else if newKey.FirstMessageIndex < a.FirstMessageIndex {
+		// "if they have the same values for is_verified, then it will keep the key with a lower first_message_index"
 		return true
-	}
-	// "and finally, is is_verified and first_message_index are equal, then it will keep the key with a lower forwarded_count"
-	if newKey.ForwardedCount < a.ForwardedCount {
+	} else if newKey.ForwardedCount < a.ForwardedCount {
+		// "and finally, is is_verified and first_message_index are equal, then it will keep the key with a lower forwarded_count"
 		return true
 	}
 	return false

userapi/storage/accounts/postgres/key_backup_table.go

@@ -36,7 +36,7 @@ CREATE TABLE IF NOT EXISTS account_e2e_room_keys (
     session_data TEXT NOT NULL
 );
 CREATE UNIQUE INDEX IF NOT EXISTS e2e_room_keys_idx ON account_e2e_room_keys(user_id, room_id, session_id, version);
-CREATE UNIQUE INDEX IF NOT EXISTS e2e_room_keys_versions_idx ON account_e2e_room_keys(user_id, version);
+CREATE INDEX IF NOT EXISTS e2e_room_keys_versions_idx ON account_e2e_room_keys(user_id, version);
 `
 
 const insertBackupKeySQL = "" +

userapi/storage/accounts/postgres/key_backup_version_table.go

@@ -146,12 +146,19 @@ func (s *keyBackupVersionStatements) selectKeyBackup(
 ) (versionResult, algorithm string, authData json.RawMessage, etag string, deleted bool, err error) {
 	var versionInt int64
 	if version == "" {
-		err = txn.Stmt(s.selectLatestVersionStmt).QueryRowContext(ctx, userID).Scan(&versionInt)
+		var v *int64 // allows nulls
+		if err = txn.Stmt(s.selectLatestVersionStmt).QueryRowContext(ctx, userID).Scan(&v); err != nil {
+			return
+		}
+		if v == nil {
+			err = sql.ErrNoRows
+			return
+		}
+		versionInt = *v
 	} else {
-		versionInt, err = strconv.ParseInt(version, 10, 64)
-	}
-	if err != nil {
-		return
+		if versionInt, err = strconv.ParseInt(version, 10, 64); err != nil {
+			return
+		}
 	}
 	versionResult = strconv.FormatInt(versionInt, 10)
 	var deletedInt int

userapi/storage/accounts/postgres/storage.go

@@ -479,7 +479,7 @@ func (d *Database) UpsertBackupKeys(
 						err = d.keyBackups.updateBackupKey(ctx, txn, userID, version, newKey)
 						changed = true
 						if err != nil {
-							return err
+							return fmt.Errorf("d.keyBackups.updateBackupKey: %w", err)
 						}
 					}
 					// if we shouldn't replace the key we do nothing with it
@@ -490,7 +490,7 @@ func (d *Database) UpsertBackupKeys(
 			err = d.keyBackups.insertBackupKey(ctx, txn, userID, version, newKey)
 			changed = true
 			if err != nil {
-				return err
+				return fmt.Errorf("d.keyBackups.insertBackupKey: %w", err)
 			}
 		}
 

userapi/storage/accounts/sqlite3/key_backup_table.go

@@ -36,7 +36,7 @@ CREATE TABLE IF NOT EXISTS account_e2e_room_keys (
     session_data TEXT NOT NULL
 );
 CREATE UNIQUE INDEX IF NOT EXISTS e2e_room_keys_idx ON account_e2e_room_keys(user_id, room_id, session_id, version);
-CREATE UNIQUE INDEX IF NOT EXISTS e2e_room_keys_versions_idx ON account_e2e_room_keys(user_id, version);
+CREATE INDEX IF NOT EXISTS e2e_room_keys_versions_idx ON account_e2e_room_keys(user_id, version);
 `
 
 const insertBackupKeySQL = "" +

userapi/storage/accounts/sqlite3/key_backup_version_table.go

@@ -144,12 +144,19 @@ func (s *keyBackupVersionStatements) selectKeyBackup(
 ) (versionResult, algorithm string, authData json.RawMessage, etag string, deleted bool, err error) {
 	var versionInt int64
 	if version == "" {
-		err = txn.Stmt(s.selectLatestVersionStmt).QueryRowContext(ctx, userID).Scan(&versionInt)
+		var v *int64 // allows nulls
+		if err = txn.Stmt(s.selectLatestVersionStmt).QueryRowContext(ctx, userID).Scan(&v); err != nil {
+			return
+		}
+		if v == nil {
+			err = sql.ErrNoRows
+			return
+		}
+		versionInt = *v
 	} else {
-		versionInt, err = strconv.ParseInt(version, 10, 64)
-	}
-	if err != nil {
-		return
+		if versionInt, err = strconv.ParseInt(version, 10, 64); err != nil {
+			return
+		}
 	}
 	versionResult = strconv.FormatInt(versionInt, 10)
 	var deletedInt int

userapi/storage/accounts/sqlite3/storage.go

@@ -520,7 +520,7 @@ func (d *Database) UpsertBackupKeys(
 						err = d.keyBackups.updateBackupKey(ctx, txn, userID, version, newKey)
 						changed = true
 						if err != nil {
-							return err
+							return fmt.Errorf("d.keyBackups.updateBackupKey: %w", err)
 						}
 					}
 					// if we shouldn't replace the key we do nothing with it
@@ -531,7 +531,7 @@ func (d *Database) UpsertBackupKeys(
 			err = d.keyBackups.insertBackupKey(ctx, txn, userID, version, newKey)
 			changed = true
 			if err != nil {
-				return err
+				return fmt.Errorf("d.keyBackups.insertBackupKey: %w", err)
 			}
 		}