remove request/response structs from RemoveRoomAlias

clientapi/routing/directory.go

@@ -300,27 +300,23 @@ func RemoveLocalAlias(
 		}
 	}
 
-	queryReq := roomserverAPI.RemoveRoomAliasRequest{
-		Alias:    alias,
-		SenderID: deviceSenderID,
-	}
-	var queryRes roomserverAPI.RemoveRoomAliasResponse
-	if err := rsAPI.RemoveRoomAlias(req.Context(), &queryReq, &queryRes); err != nil {
+	aliasFound, aliasRemoved, err := rsAPI.RemoveRoomAlias(req.Context(), deviceSenderID, alias)
+	if err != nil {
 		util.GetLogger(req.Context()).WithError(err).Error("aliasAPI.RemoveRoomAlias failed")
 		return util.JSONResponse{
 			Code: http.StatusInternalServerError,
-			JSON: spec.InternalServerError{},
+			JSON: spec.Unknown("internal server error"),
 		}
 	}
 
-	if !queryRes.Found {
+	if !aliasFound {
 		return util.JSONResponse{
 			Code: http.StatusNotFound,
 			JSON: spec.NotFound("The alias does not exist."),
 		}
 	}
 
-	if !queryRes.Removed {
+	if !aliasRemoved {
 		return util.JSONResponse{
 			Code: http.StatusForbidden,
 			JSON: spec.Forbidden("You do not have permission to remove this alias."),

roomserver/api/alias.go

@@ -16,8 +16,6 @@ package api
 
 import (
 	"regexp"
-
-	"github.com/matrix-org/gomatrixserverlib/spec"
 )
 
 // GetRoomIDForAliasRequest is a request to GetRoomIDForAlias
@@ -47,22 +45,6 @@ type GetAliasesForRoomIDResponse struct {
 	Aliases []string `json:"aliases"`
 }
 
-// RemoveRoomAliasRequest is a request to RemoveRoomAlias
-type RemoveRoomAliasRequest struct {
-	// ID of the user removing the alias
-	SenderID spec.SenderID `json:"user_id"`
-	// The room alias to remove
-	Alias string `json:"alias"`
-}
-
-// RemoveRoomAliasResponse is a response to RemoveRoomAlias
-type RemoveRoomAliasResponse struct {
-	// Did the alias exist before?
-	Found bool `json:"found"`
-	// Did we remove it?
-	Removed bool `json:"removed"`
-}
-
 type AliasEvent struct {
 	Alias      string   `json:"alias"`
 	AltAliases []string `json:"alt_aliases"`

roomserver/api/api.go

@@ -237,12 +237,19 @@ type ClientRoomserverAPI interface {
 	PerformPublish(ctx context.Context, req *PerformPublishRequest) error
 	// PerformForget forgets a rooms history for a specific user
 	PerformForget(ctx context.Context, req *PerformForgetRequest, resp *PerformForgetResponse) error
+
 	// Sets a room alias, as provided sender, pointing to the provided room ID.
 	//
 	// If err is nil, then the returned boolean indicates if the alias is already in use.
 	// If true, then the alias has not been set to the provided room, as it already in use.
 	SetRoomAlias(ctx context.Context, senderID spec.SenderID, roomID spec.RoomID, alias string) (aliasAlreadyExists bool, err error)
-	RemoveRoomAlias(ctx context.Context, req *RemoveRoomAliasRequest, res *RemoveRoomAliasResponse) error
+
+	//RemoveRoomAlias(ctx context.Context, req *RemoveRoomAliasRequest, res *RemoveRoomAliasResponse) error
+	// Removes a room alias, as provided sender.
+	//
+	// Returns whether the alias was found, whether it was removed, and an error (if any occurred)
+	RemoveRoomAlias(ctx context.Context, senderID spec.SenderID, alias string) (aliasFound bool, aliasRemoved bool, err error)
+
 	SigningIdentityFor(ctx context.Context, roomID spec.RoomID, senderID spec.UserID) (fclient.SigningIdentity, error)
 }
 

roomserver/internal/alias.go

@@ -116,90 +116,79 @@ func (r *RoomserverInternalAPI) GetAliasesForRoomID(
 // nolint:gocyclo
 // RemoveRoomAlias implements alias.RoomserverInternalAPI
 // nolint: gocyclo
-func (r *RoomserverInternalAPI) RemoveRoomAlias(
-	ctx context.Context,
-	request *api.RemoveRoomAliasRequest,
-	response *api.RemoveRoomAliasResponse,
-) error {
-	roomID, err := r.DB.GetRoomIDForAlias(ctx, request.Alias)
+func (r *RoomserverInternalAPI) RemoveRoomAlias(ctx context.Context, senderID spec.SenderID, alias string) (aliasFound bool, aliasRemoved bool, err error) {
+	roomID, err := r.DB.GetRoomIDForAlias(ctx, alias)
 	if err != nil {
-		return fmt.Errorf("r.DB.GetRoomIDForAlias: %w", err)
+		return false, false, fmt.Errorf("r.DB.GetRoomIDForAlias: %w", err)
 	}
 	if roomID == "" {
-		response.Found = false
-		response.Removed = false
-		return nil
+		return false, false, nil
 	}
 
 	validRoomID, err := spec.NewRoomID(roomID)
 	if err != nil {
-		return err
+		return true, false, err
 	}
 
-	sender, err := r.QueryUserIDForSender(ctx, *validRoomID, request.SenderID)
+	sender, err := r.QueryUserIDForSender(ctx, *validRoomID, senderID)
 	if err != nil || sender == nil {
-		return fmt.Errorf("r.QueryUserIDForSender: %w", err)
+		return true, false, fmt.Errorf("r.QueryUserIDForSender: %w", err)
 	}
 	virtualHost := sender.Domain()
 
-	response.Found = true
-	creatorID, err := r.DB.GetCreatorIDForAlias(ctx, request.Alias)
+	creatorID, err := r.DB.GetCreatorIDForAlias(ctx, alias)
 	if err != nil {
-		return fmt.Errorf("r.DB.GetCreatorIDForAlias: %w", err)
+		return true, false, fmt.Errorf("r.DB.GetCreatorIDForAlias: %w", err)
 	}
 
-	if spec.SenderID(creatorID) != request.SenderID {
+	if spec.SenderID(creatorID) != senderID {
 		var plEvent *types.HeaderedEvent
 		var pls *gomatrixserverlib.PowerLevelContent
 
 		plEvent, err = r.DB.GetStateEvent(ctx, roomID, spec.MRoomPowerLevels, "")
 		if err != nil {
-			return fmt.Errorf("r.DB.GetStateEvent: %w", err)
+			return true, false, fmt.Errorf("r.DB.GetStateEvent: %w", err)
 		}
 
 		pls, err = plEvent.PowerLevels()
 		if err != nil {
-			return fmt.Errorf("plEvent.PowerLevels: %w", err)
+			return true, false, fmt.Errorf("plEvent.PowerLevels: %w", err)
 		}
 
-		if pls.UserLevel(request.SenderID) < pls.EventLevel(spec.MRoomCanonicalAlias, true) {
-			response.Removed = false
-			return nil
+		if pls.UserLevel(senderID) < pls.EventLevel(spec.MRoomCanonicalAlias, true) {
+			return true, false, nil
 		}
 	}
 
 	ev, err := r.DB.GetStateEvent(ctx, roomID, spec.MRoomCanonicalAlias, "")
 	if err != nil && err != sql.ErrNoRows {
-		return err
+		return true, false, err
 	} else if ev != nil {
 		stateAlias := gjson.GetBytes(ev.Content(), "alias").Str
 		// the alias to remove is currently set as the canonical alias, remove it
-		if stateAlias == request.Alias {
+		if stateAlias == alias {
 			res, err := sjson.DeleteBytes(ev.Content(), "alias")
 			if err != nil {
-				return err
+				return true, false, err
 			}
 
-			senderID := request.SenderID
-			if request.SenderID != ev.SenderID() {
-				senderID = ev.SenderID()
-			}
-			sender, err := r.QueryUserIDForSender(ctx, *validRoomID, senderID)
-			if err != nil || sender == nil {
-				return err
+			canonicalSenderID := ev.SenderID()
+			canonicalSender, err := r.QueryUserIDForSender(ctx, *validRoomID, canonicalSenderID)
+			if err != nil || canonicalSender == nil {
+				return true, false, err
 			}
 
 			validRoomID, err := spec.NewRoomID(roomID)
 			if err != nil {
-				return err
+				return true, false, err
 			}
-			identity, err := r.SigningIdentityFor(ctx, *validRoomID, *sender)
+			identity, err := r.SigningIdentityFor(ctx, *validRoomID, *canonicalSender)
 			if err != nil {
-				return err
+				return true, false, err
 			}
 
 			proto := &gomatrixserverlib.ProtoEvent{
-				SenderID: string(senderID),
+				SenderID: string(canonicalSenderID),
 				RoomID:   ev.RoomID(),
 				Type:     ev.Type(),
 				StateKey: ev.StateKey(),
@@ -208,34 +197,33 @@ func (r *RoomserverInternalAPI) RemoveRoomAlias(
 
 			eventsNeeded, err := gomatrixserverlib.StateNeededForProtoEvent(proto)
 			if err != nil {
-				return fmt.Errorf("gomatrixserverlib.StateNeededForEventBuilder: %w", err)
+				return true, false, fmt.Errorf("gomatrixserverlib.StateNeededForEventBuilder: %w", err)
 			}
 			if len(eventsNeeded.Tuples()) == 0 {
-				return errors.New("expecting state tuples for event builder, got none")
+				return true, false, errors.New("expecting state tuples for event builder, got none")
 			}
 
 			stateRes := &api.QueryLatestEventsAndStateResponse{}
 			if err = helpers.QueryLatestEventsAndState(ctx, r.DB, r, &api.QueryLatestEventsAndStateRequest{RoomID: roomID, StateToFetch: eventsNeeded.Tuples()}, stateRes); err != nil {
-				return err
+				return true, false, err
 			}
 
 			newEvent, err := eventutil.BuildEvent(ctx, proto, &identity, time.Now(), &eventsNeeded, stateRes)
 			if err != nil {
-				return err
+				return true, false, err
 			}
 
 			err = api.SendEvents(ctx, r, api.KindNew, []*types.HeaderedEvent{newEvent}, virtualHost, r.ServerName, r.ServerName, nil, false)
 			if err != nil {
-				return err
+				return true, false, err
 			}
 		}
 	}
 
 	// Remove the alias from the database
-	if err := r.DB.RemoveRoomAlias(ctx, request.Alias); err != nil {
-		return err
+	if err := r.DB.RemoveRoomAlias(ctx, alias); err != nil {
+		return true, false, err
 	}
 
-	response.Removed = true
-	return nil
+	return true, true, nil
 }

roomserver/internal/perform/perform_upgrade.go

@@ -188,10 +188,13 @@ func moveLocalAliases(ctx context.Context,
 	}
 
 	for _, alias := range aliasRes.Aliases {
-		removeAliasReq := api.RemoveRoomAliasRequest{SenderID: senderID, Alias: alias}
-		removeAliasRes := api.RemoveRoomAliasResponse{}
-		if err = URSAPI.RemoveRoomAlias(ctx, &removeAliasReq, &removeAliasRes); err != nil {
+		aliasFound, aliasRemoved, err := URSAPI.RemoveRoomAlias(ctx, senderID, alias)
+		if err != nil {
 			return fmt.Errorf("Failed to remove old room alias: %w", err)
+		} else if !aliasFound {
+			return fmt.Errorf("Failed to remove old room alias: alias not found, possible race")
+		} else if !aliasRemoved {
+			return fmt.Errorf("Failed to remove old alias")
 		}
 
 		aliasAlreadyExists, err := URSAPI.SetRoomAlias(ctx, senderID, *parsedNewRoomID, alias)