[pseudoIDs] Fixes for room alias tests (#3159)

Some (deceptively) simple fixes for some bugs that caused room alias
tests to fail (sytext `tests/30rooms/05aliases.pl`). Each commit has
details about what it fixes.

Sytest results:

- Sytest before (79d4a0e):
https://gist.github.com/swedgwood/972ac4ef93edd130d3db0930703d6c82
- Sytest after (4b09bed):
https://gist.github.com/swedgwood/504b00ac4ee892acb757b7fac55fa28a

Room aliases go from `8/15` to `15/15`, but looks like these fixes also
managed to fix about `4` other tests, which is a nice bonus :)

Signed-off-by: `Sam Wedgwood <sam@wedgwood.dev>`

clientapi/routing/directory.go

@@ -181,13 +181,33 @@ func SetLocalAlias(
 		return *resErr
 	}
 
-	queryReq := roomserverAPI.SetRoomAliasRequest{
-		UserID: device.UserID,
-		RoomID: r.RoomID,
-		Alias:  alias,
+	roomID, err := spec.NewRoomID(r.RoomID)
+	if err != nil {
+		return util.JSONResponse{
+			Code: http.StatusBadRequest,
+			JSON: spec.InvalidParam("invalid room ID"),
+		}
 	}
-	var queryRes roomserverAPI.SetRoomAliasResponse
-	if err := rsAPI.SetRoomAlias(req.Context(), &queryReq, &queryRes); err != nil {
+
+	userID, err := spec.NewUserID(device.UserID, true)
+	if err != nil {
+		return util.JSONResponse{
+			Code: http.StatusInternalServerError,
+			JSON: spec.Unknown("internal server error"),
+		}
+	}
+
+	senderID, err := rsAPI.QuerySenderIDForUser(req.Context(), *roomID, *userID)
+	if err != nil {
+		util.GetLogger(req.Context()).WithError(err).Error("QuerySenderIDForUser failed")
+		return util.JSONResponse{
+			Code: http.StatusInternalServerError,
+			JSON: spec.Unknown("internal server error"),
+		}
+	}
+
+	aliasAlreadyExists, err := rsAPI.SetRoomAlias(req.Context(), senderID, *roomID, alias)
+	if err != nil {
 		util.GetLogger(req.Context()).WithError(err).Error("aliasAPI.SetRoomAlias failed")
 		return util.JSONResponse{
 			Code: http.StatusInternalServerError,
@@ -195,7 +215,7 @@ func SetLocalAlias(
 		}
 	}
 
-	if queryRes.AliasExists {
+	if aliasAlreadyExists {
 		return util.JSONResponse{
 			Code: http.StatusConflict,
 			JSON: spec.Unknown("The alias " + alias + " already exists."),
@@ -240,6 +260,31 @@ func RemoveLocalAlias(
 			JSON: spec.NotFound("The alias does not exist."),
 		}
 	}
+
+	// This seems like the kind of auth check that should be done in the roomserver, but
+	// if this check fails (user is not in the room), then there will be no SenderID for the user
+	// for pseudo-ID rooms - it will just return "". However, we can't use lack of a sender ID
+	// as meaning they are not in the room, since lacking a sender ID could be caused by other bugs.
+	// TODO: maybe have QuerySenderIDForUser return richer errors?
+	var queryResp roomserverAPI.QueryMembershipForUserResponse
+	err = rsAPI.QueryMembershipForUser(req.Context(), &roomserverAPI.QueryMembershipForUserRequest{
+		RoomID: validRoomID.String(),
+		UserID: *userID,
+	}, &queryResp)
+	if err != nil {
+		util.GetLogger(req.Context()).WithError(err).Error("roomserverAPI.QueryMembershipForUser failed")
+		return util.JSONResponse{
+			Code: http.StatusInternalServerError,
+			JSON: spec.Unknown("internal server error"),
+		}
+	}
+	if !queryResp.IsInRoom {
+		return util.JSONResponse{
+			Code: http.StatusForbidden,
+			JSON: spec.Forbidden("You do not have permission to remove this alias."),
+		}
+	}
+
 	deviceSenderID, err := rsAPI.QuerySenderIDForUser(req.Context(), *validRoomID, *userID)
 	if err != nil {
 		return util.JSONResponse{
@@ -247,28 +292,31 @@ func RemoveLocalAlias(
 			JSON: spec.NotFound("The alias does not exist."),
 		}
 	}
-
-	queryReq := roomserverAPI.RemoveRoomAliasRequest{
-		Alias:    alias,
-		SenderID: deviceSenderID,
-	}
-	var queryRes roomserverAPI.RemoveRoomAliasResponse
-	if err := rsAPI.RemoveRoomAlias(req.Context(), &queryReq, &queryRes); err != nil {
-		util.GetLogger(req.Context()).WithError(err).Error("aliasAPI.RemoveRoomAlias failed")
+	// TODO: how to handle this case? missing user/room keys seem to be a whole new class of errors
+	if deviceSenderID == "" {
 		return util.JSONResponse{
 			Code: http.StatusInternalServerError,
-			JSON: spec.InternalServerError{},
+			JSON: spec.Unknown("internal server error"),
 		}
 	}
 
-	if !queryRes.Found {
+	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.Unknown("internal server error"),
+		}
+	}
+
+	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,26 +16,8 @@ package api
 
 import (
 	"regexp"
-
-	"github.com/matrix-org/gomatrixserverlib/spec"
 )
 
-// SetRoomAliasRequest is a request to SetRoomAlias
-type SetRoomAliasRequest struct {
-	// ID of the user setting the alias
-	UserID string `json:"user_id"`
-	// New alias for the room
-	Alias string `json:"alias"`
-	// The room ID the alias is referring to
-	RoomID string `json:"room_id"`
-}
-
-// SetRoomAliasResponse is a response to SetRoomAlias
-type SetRoomAliasResponse struct {
-	// Does the alias already refer to a room?
-	AliasExists bool `json:"alias_exists"`
-}
-
 // GetRoomIDForAliasRequest is a request to GetRoomIDForAlias
 type GetRoomIDForAliasRequest struct {
 	// Alias we want to lookup
@@ -63,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,8 +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
-	SetRoomAlias(ctx context.Context, req *SetRoomAliasRequest, res *SetRoomAliasResponse) error
-	RemoveRoomAlias(ctx context.Context, req *RemoveRoomAliasRequest, res *RemoveRoomAliasResponse) 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
+	// 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

@@ -35,27 +35,27 @@ import (
 // SetRoomAlias implements alias.RoomserverInternalAPI
 func (r *RoomserverInternalAPI) SetRoomAlias(
 	ctx context.Context,
-	request *api.SetRoomAliasRequest,
-	response *api.SetRoomAliasResponse,
-) error {
+	senderID spec.SenderID,
+	roomID spec.RoomID,
+	alias string,
+) (aliasAlreadyUsed bool, err error) {
 	// Check if the alias isn't already referring to a room
-	roomID, err := r.DB.GetRoomIDForAlias(ctx, request.Alias)
+	existingRoomID, err := r.DB.GetRoomIDForAlias(ctx, alias)
 	if err != nil {
-		return err
+		return false, err
 	}
-	if len(roomID) > 0 {
+
+	if len(existingRoomID) > 0 {
 		// If the alias already exists, stop the process
-		response.AliasExists = true
-		return nil
+		return true, nil
 	}
-	response.AliasExists = false
 
 	// Save the new alias
-	if err := r.DB.SetRoomAlias(ctx, request.Alias, request.RoomID, request.UserID); err != nil {
-		return err
+	if err := r.DB.SetRoomAlias(ctx, alias, roomID.String(), string(senderID)); err != nil {
+		return false, err
 	}
 
-	return nil
+	return false, nil
 }
 
 // GetRoomIDForAlias implements alias.RoomserverInternalAPI
@@ -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_create_room.go

@@ -433,23 +433,16 @@ func (c *Creator) PerformCreateRoom(ctx context.Context, userID spec.UserID, roo
 	// from creating the room but still failing due to the alias having already
 	// been taken.
 	if roomAlias != "" {
-		aliasReq := api.SetRoomAliasRequest{
-			Alias:  roomAlias,
-			RoomID: roomID.String(),
-			UserID: userID.String(),
-		}
-
-		var aliasResp api.SetRoomAliasResponse
-		err = c.RSAPI.SetRoomAlias(ctx, &aliasReq, &aliasResp)
-		if err != nil {
-			util.GetLogger(ctx).WithError(err).Error("aliasAPI.SetRoomAlias failed")
+		aliasAlreadyExists, aliasErr := c.RSAPI.SetRoomAlias(ctx, senderID, roomID, roomAlias)
+		if aliasErr != nil {
+			util.GetLogger(ctx).WithError(aliasErr).Error("aliasAPI.SetRoomAlias failed")
 			return "", &util.JSONResponse{
 				Code: http.StatusInternalServerError,
 				JSON: spec.InternalServerError{},
 			}
 		}
 
-		if aliasResp.AliasExists {
+		if aliasAlreadyExists {
 			return "", &util.JSONResponse{
 				Code: http.StatusBadRequest,
 				JSON: spec.RoomInUse("Room alias already exists."),

roomserver/internal/perform/perform_upgrade.go

@@ -116,7 +116,7 @@ func (r *Upgrader) performRoomUpgrade(
 	}
 
 	// 4. Move local aliases to the new room
-	if pErr = moveLocalAliases(ctx, roomID, newRoomID, senderID, userID, r.URSAPI); pErr != nil {
+	if pErr = moveLocalAliases(ctx, roomID, newRoomID, senderID, r.URSAPI); pErr != nil {
 		return "", pErr
 	}
 
@@ -171,7 +171,7 @@ func (r *Upgrader) restrictOldRoomPowerLevels(ctx context.Context, evTime time.T
 }
 
 func moveLocalAliases(ctx context.Context,
-	roomID, newRoomID string, senderID spec.SenderID, userID spec.UserID,
+	roomID, newRoomID string, senderID spec.SenderID,
 	URSAPI api.RoomserverInternalAPI,
 ) (err error) {
 
@@ -181,17 +181,27 @@ func moveLocalAliases(ctx context.Context,
 		return fmt.Errorf("Failed to get old room aliases: %w", err)
 	}
 
+	// TODO: this should be spec.RoomID further up the call stack
+	parsedNewRoomID, err := spec.NewRoomID(newRoomID)
+	if err != nil {
+		return err
+	}
+
 	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")
 		}
 
-		setAliasReq := api.SetRoomAliasRequest{UserID: userID.String(), Alias: alias, RoomID: newRoomID}
-		setAliasRes := api.SetRoomAliasResponse{}
-		if err = URSAPI.SetRoomAlias(ctx, &setAliasReq, &setAliasRes); err != nil {
+		aliasAlreadyExists, err := URSAPI.SetRoomAlias(ctx, senderID, *parsedNewRoomID, alias)
+		if err != nil {
 			return fmt.Errorf("Failed to set new room alias: %w", err)
+		} else if aliasAlreadyExists {
+			return fmt.Errorf("Failed to set new room alias: alias exists when it should have just been removed")
 		}
 	}
 	return nil

roomserver/roomserver_test.go

@@ -227,6 +227,11 @@ func TestPurgeRoom(t *testing.T) {
 	bob := test.NewUser(t)
 	room := test.NewRoom(t, alice, test.RoomPreset(test.PresetTrustedPrivateChat))
 
+	roomID, err := spec.NewRoomID(room.ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	// Invite Bob
 	inviteEvent := room.CreateAndInsert(t, alice, spec.MRoomMember, map[string]interface{}{
 		"membership": "invite",
@@ -274,9 +279,7 @@ func TestPurgeRoom(t *testing.T) {
 		if !isPublished {
 			t.Fatalf("room should be published before purging")
 		}
-
-		aliasResp := &api.SetRoomAliasResponse{}
-		if err = rsAPI.SetRoomAlias(ctx, &api.SetRoomAliasRequest{RoomID: room.ID, Alias: "myalias", UserID: alice.ID}, aliasResp); err != nil {
+		if _, err = rsAPI.SetRoomAlias(ctx, spec.SenderID(alice.ID), *roomID, "myalias"); err != nil {
 			t.Fatal(err)
 		}
 		// check the alias is actually there
@@ -930,14 +933,17 @@ func TestUpgrade(t *testing.T) {
 			upgradeUser: alice.ID,
 			roomFunc: func(rsAPI api.RoomserverInternalAPI) string {
 				r := test.NewRoom(t, alice)
+				roomID, err := spec.NewRoomID(r.ID)
+				if err != nil {
+					t.Fatal(err)
+				}
 				if err := api.SendEvents(ctx, rsAPI, api.KindNew, r.Events(), "test", "test", "test", nil, false); err != nil {
 					t.Errorf("failed to send events: %v", err)
 				}
 
-				if err := rsAPI.SetRoomAlias(ctx, &api.SetRoomAliasRequest{
-					RoomID: r.ID,
-					Alias:  "#myroomalias:test",
-				}, &api.SetRoomAliasResponse{}); err != nil {
+				if _, err := rsAPI.SetRoomAlias(ctx, spec.SenderID(alice.ID),
+					*roomID,
+					"#myroomalias:test"); err != nil {
 					t.Fatal(err)
 				}
 

syncapi/streams/stream_pdu.go

@@ -519,7 +519,7 @@ func (p *PDUStreamProvider) updatePowerLevelEvent(ctx context.Context, ev *rstyp
 	}
 
 	var evNew gomatrixserverlib.PDU
-	evNew, err = gomatrixserverlib.MustGetRoomVersion(gomatrixserverlib.RoomVersionPseudoIDs).NewEventFromTrustedJSON(newEv, false)
+	evNew, err = gomatrixserverlib.MustGetRoomVersion(gomatrixserverlib.RoomVersionPseudoIDs).NewEventFromTrustedJSONWithEventID(ev.EventID(), newEv, false)
 	if err != nil {
 		return nil, err
 	}