[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>`

1 files changed, 64 insertions, 16 deletions

diff --git a/clientapi/routing/directory.go b/clientapi/routing/directory.go
index d9129d1b..3ec959b4 100644
--- a/clientapi/routing/directory.go
+++ b/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,
+	// 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.Unknown("internal server error"),
+		}
 	}
-	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."),