From 185ef8ef437e2c442160a9cb010ea4fd983096e3 Mon Sep 17 00:00:00 2001 From: Sam Wedgwood Date: Thu, 27 Jul 2023 15:05:35 +0100 Subject: [PATCH] remove request/response structs from SetRoomAlias --- clientapi/routing/directory.go | 11 +++------ roomserver/api/alias.go | 16 ------------- roomserver/api/api.go | 6 ++++- roomserver/internal/alias.go | 24 +++++++++---------- .../internal/perform/perform_create_room.go | 15 ++++-------- .../internal/perform/perform_upgrade.go | 13 +++++++--- roomserver/roomserver_test.go | 19 ++++++++++----- 7 files changed, 47 insertions(+), 57 deletions(-) diff --git a/clientapi/routing/directory.go b/clientapi/routing/directory.go index 32f7f2ec3..42655b7e6 100644 --- a/clientapi/routing/directory.go +++ b/clientapi/routing/directory.go @@ -206,13 +206,8 @@ func SetLocalAlias( } } - queryReq := roomserverAPI.SetRoomAliasRequest{ - SenderID: senderID, - RoomID: r.RoomID, - Alias: alias, - } - var queryRes roomserverAPI.SetRoomAliasResponse - if err := rsAPI.SetRoomAlias(req.Context(), &queryReq, &queryRes); err != nil { + 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, @@ -220,7 +215,7 @@ func SetLocalAlias( } } - if queryRes.AliasExists { + if aliasAlreadyExists { return util.JSONResponse{ Code: http.StatusConflict, JSON: spec.Unknown("The alias " + alias + " already exists."), diff --git a/roomserver/api/alias.go b/roomserver/api/alias.go index 5a6d48000..0bb1386c6 100644 --- a/roomserver/api/alias.go +++ b/roomserver/api/alias.go @@ -20,22 +20,6 @@ import ( "github.com/matrix-org/gomatrixserverlib/spec" ) -// SetRoomAliasRequest is a request to SetRoomAlias -type SetRoomAliasRequest struct { - // ID of the user setting the alias - SenderID spec.SenderID `json:"sender"` - // 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 diff --git a/roomserver/api/api.go b/roomserver/api/api.go index 28b381d35..2d1d363cd 100644 --- a/roomserver/api/api.go +++ b/roomserver/api/api.go @@ -237,7 +237,11 @@ 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 + // 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 SigningIdentityFor(ctx context.Context, roomID spec.RoomID, senderID spec.UserID) (fclient.SigningIdentity, error) } diff --git a/roomserver/internal/alias.go b/roomserver/internal/alias.go index a3db9e97f..2f93c6e45 100644 --- a/roomserver/internal/alias.go +++ b/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, string(request.SenderID)); 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 diff --git a/roomserver/internal/perform/perform_create_room.go b/roomserver/internal/perform/perform_create_room.go index 247a5bcd4..cd6629d28 100644 --- a/roomserver/internal/perform/perform_create_room.go +++ b/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{ - SenderID: senderID, - Alias: roomAlias, - RoomID: roomID.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."), diff --git a/roomserver/internal/perform/perform_upgrade.go b/roomserver/internal/perform/perform_upgrade.go index 2e16e1685..ad51128e1 100644 --- a/roomserver/internal/perform/perform_upgrade.go +++ b/roomserver/internal/perform/perform_upgrade.go @@ -181,6 +181,12 @@ 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{} @@ -188,10 +194,11 @@ func moveLocalAliases(ctx context.Context, return fmt.Errorf("Failed to remove old room alias: %w", err) } - setAliasReq := api.SetRoomAliasRequest{SenderID: senderID, 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 diff --git a/roomserver/roomserver_test.go b/roomserver/roomserver_test.go index d24a31f61..1626bf831 100644 --- a/roomserver/roomserver_test.go +++ b/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,8 +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", SenderID: spec.SenderID(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 @@ -929,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) }