Use `*spec.SenderID` for `QuerySenderIDForUser` (#3164)

There are cases where a dendrite instance is unaware of a pseudo ID for
a user, the user is not a member of that room. To represent this case,
we currently use the 'zero' value, which is often not checked and so
causes errors later down the line. To make this case more explict, and
to be consistent with `QueryUserIDForSender`, this PR changes this to
use a pointer (and `nil` to mean no sender ID).

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

7 files changed, 40 insertions, 25 deletions

diff --git a/roomserver/api/api.go b/roomserver/api/api.go
index ed87ce93..69997fc4 100644
--- a/roomserver/api/api.go
+++ b/roomserver/api/api.go
@@ -97,7 +97,7 @@ type InputRoomEventsAPI interface {
 }
 
 type QuerySenderIDAPI interface {
-	QuerySenderIDForUser(ctx context.Context, roomID spec.RoomID, userID spec.UserID) (spec.SenderID, error)
+	QuerySenderIDForUser(ctx context.Context, roomID spec.RoomID, userID spec.UserID) (*spec.SenderID, error)
 	QueryUserIDForSender(ctx context.Context, roomID spec.RoomID, senderID spec.SenderID) (*spec.UserID, error)
 }
 
diff --git a/roomserver/internal/perform/perform_admin.go b/roomserver/internal/perform/perform_admin.go
index dd713262..2888067b 100644
--- a/roomserver/internal/perform/perform_admin.go
+++ b/roomserver/internal/perform/perform_admin.go
@@ -292,10 +292,12 @@ func (r *Admin) PerformAdminDownloadState(
 	senderID, err := r.Queryer.QuerySenderIDForUser(ctx, *validRoomID, *fullUserID)
 	if err != nil {
 		return err
+	} else if senderID == nil {
+		return fmt.Errorf("sender ID not found for %s in %s", *fullUserID, *validRoomID)
 	}
 	proto := &gomatrixserverlib.ProtoEvent{
 		Type:     "org.matrix.dendrite.state_download",
-		SenderID: string(senderID),
+		SenderID: string(*senderID),
 		RoomID:   roomID,
 		Content:  spec.RawJSON("{}"),
 	}
diff --git a/roomserver/internal/perform/perform_invite.go b/roomserver/internal/perform/perform_invite.go
index 278ddd7d..e07780d6 100644
--- a/roomserver/internal/perform/perform_invite.go
+++ b/roomserver/internal/perform/perform_invite.go
@@ -133,6 +133,8 @@ func (r *Inviter) PerformInvite(
 	senderID, err := r.RSAPI.QuerySenderIDForUser(ctx, req.InviteInput.RoomID, req.InviteInput.Inviter)
 	if err != nil {
 		return err
+	} else if senderID == nil {
+		return fmt.Errorf("sender ID not found for %s in %s", req.InviteInput.Inviter, req.InviteInput.RoomID)
 	}
 	info, err := r.DB.RoomInfo(ctx, req.InviteInput.RoomID.String())
 	if err != nil {
@@ -140,7 +142,7 @@ func (r *Inviter) PerformInvite(
 	}
 
 	proto := gomatrixserverlib.ProtoEvent{
-		SenderID: string(senderID),
+		SenderID: string(*senderID),
 		RoomID:   req.InviteInput.RoomID.String(),
 		Type:     "m.room.member",
 	}
@@ -187,7 +189,7 @@ func (r *Inviter) PerformInvite(
 		UserIDQuerier: func(roomID spec.RoomID, senderID spec.SenderID) (*spec.UserID, error) {
 			return r.RSAPI.QueryUserIDForSender(ctx, roomID, senderID)
 		},
-		SenderIDQuerier: func(roomID spec.RoomID, userID spec.UserID) (spec.SenderID, error) {
+		SenderIDQuerier: func(roomID spec.RoomID, userID spec.UserID) (*spec.SenderID, error) {
 			return r.RSAPI.QuerySenderIDForUser(ctx, roomID, userID)
 		},
 		SenderIDCreator: func(ctx context.Context, userID spec.UserID, roomID spec.RoomID, roomVersion string) (spec.SenderID, ed25519.PrivateKey, error) {
diff --git a/roomserver/internal/perform/perform_join.go b/roomserver/internal/perform/perform_join.go
index dfce9cc7..23988e46 100644
--- a/roomserver/internal/perform/perform_join.go
+++ b/roomserver/internal/perform/perform_join.go
@@ -201,11 +201,11 @@ func (r *Joiner) performJoinRoomByID(
 	if err == nil && info != nil {
 		switch info.RoomVersion {
 		case gomatrixserverlib.RoomVersionPseudoIDs:
-			senderID, err = r.Queryer.QuerySenderIDForUser(ctx, *roomID, *userID)
-			if err == nil {
+			senderIDPtr, queryErr := r.Queryer.QuerySenderIDForUser(ctx, *roomID, *userID)
+			if queryErr == nil {
 				checkInvitePending = true
 			}
-			if senderID == "" {
+			if senderIDPtr == nil {
 				// create user room key if needed
 				key, keyErr := r.RSAPI.GetOrCreateUserRoomPrivateKey(ctx, *userID, *roomID)
 				if keyErr != nil {
@@ -213,6 +213,8 @@ func (r *Joiner) performJoinRoomByID(
 					return "", "", fmt.Errorf("GetOrCreateUserRoomPrivateKey failed: %w", keyErr)
 				}
 				senderID = spec.SenderIDFromPseudoIDKey(key)
+			} else {
+				senderID = *senderIDPtr
 			}
 		default:
 			checkInvitePending = true
diff --git a/roomserver/internal/perform/perform_leave.go b/roomserver/internal/perform/perform_leave.go
index a20896cf..5c63a668 100644
--- a/roomserver/internal/perform/perform_leave.go
+++ b/roomserver/internal/perform/perform_leave.go
@@ -73,6 +73,7 @@ func (r *Leaver) PerformLeave(
 	return nil, fmt.Errorf("room ID %q is invalid", req.RoomID)
 }
 
+// nolint:gocyclo
 func (r *Leaver) performLeaveRoomByID(
 	ctx context.Context,
 	req *api.PerformLeaveRequest,
@@ -83,20 +84,20 @@ func (r *Leaver) performLeaveRoomByID(
 		return nil, err
 	}
 	leaver, err := r.RSAPI.QuerySenderIDForUser(ctx, *roomID, req.Leaver)
-	if err != nil {
+	if err != nil || leaver == nil {
 		return nil, fmt.Errorf("leaver %s has no matching senderID in this room", req.Leaver.String())
 	}
 
 	// If there's an invite outstanding for the room then respond to
 	// that.
-	isInvitePending, senderUser, eventID, _, err := helpers.IsInvitePending(ctx, r.DB, req.RoomID, leaver)
+	isInvitePending, senderUser, eventID, _, err := helpers.IsInvitePending(ctx, r.DB, req.RoomID, *leaver)
 	if err == nil && isInvitePending {
 		sender, serr := r.RSAPI.QueryUserIDForSender(ctx, *roomID, senderUser)
 		if serr != nil || sender == nil {
 			return nil, fmt.Errorf("sender %q has no matching userID", senderUser)
 		}
 		if !r.Cfg.Matrix.IsLocalServerName(sender.Domain()) {
-			return r.performFederatedRejectInvite(ctx, req, res, *sender, eventID, leaver)
+			return r.performFederatedRejectInvite(ctx, req, res, *sender, eventID, *leaver)
 		}
 		// check that this is not a "server notice room"
 		accData := &userapi.QueryAccountDataResponse{}
@@ -132,7 +133,7 @@ func (r *Leaver) performLeaveRoomByID(
 		StateToFetch: []gomatrixserverlib.StateKeyTuple{
 			{
 				EventType: spec.MRoomMember,
-				StateKey:  string(leaver),
+				StateKey:  string(*leaver),
 			},
 		},
 	}
@@ -157,7 +158,7 @@ func (r *Leaver) performLeaveRoomByID(
 	}
 
 	// Prepare the template for the leave event.
-	senderIDString := string(leaver)
+	senderIDString := string(*leaver)
 	proto := gomatrixserverlib.ProtoEvent{
 		Type:     spec.MRoomMember,
 		SenderID: senderIDString,
diff --git a/roomserver/internal/perform/perform_upgrade.go b/roomserver/internal/perform/perform_upgrade.go
index 74c62cd9..c32e10d5 100644
--- a/roomserver/internal/perform/perform_upgrade.go
+++ b/roomserver/internal/perform/perform_upgrade.go
@@ -62,10 +62,13 @@ func (r *Upgrader) performRoomUpgrade(
 	if err != nil {
 		util.GetLogger(ctx).WithError(err).Error("Failed getting senderID for user")
 		return "", err
+	} else if senderID == nil {
+		util.GetLogger(ctx).WithField("userID", userID).WithField("roomID", *fullRoomID).Error("No senderID for user")
+		return "", fmt.Errorf("No sender ID for %s in %s", userID, *fullRoomID)
 	}
 
 	// 1. Check if the user is authorized to actually perform the upgrade (can send m.room.tombstone)
-	if !r.userIsAuthorized(ctx, senderID, roomID) {
+	if !r.userIsAuthorized(ctx, *senderID, roomID) {
 		return "", api.ErrNotAllowed{Err: fmt.Errorf("You don't have permission to upgrade the room, power level too low.")}
 	}
 
@@ -83,20 +86,20 @@ func (r *Upgrader) performRoomUpgrade(
 	}
 
 	// Make the tombstone event
-	tombstoneEvent, pErr := r.makeTombstoneEvent(ctx, evTime, senderID, userID.Domain(), roomID, newRoomID)
+	tombstoneEvent, pErr := r.makeTombstoneEvent(ctx, evTime, *senderID, userID.Domain(), roomID, newRoomID)
 	if pErr != nil {
 		return "", pErr
 	}
 
 	// Generate the initial events we need to send into the new room. This includes copied state events and bans
 	// as well as the power level events needed to set up the room
-	eventsToMake, pErr := r.generateInitialEvents(ctx, oldRoomRes, senderID, roomID, roomVersion, tombstoneEvent)
+	eventsToMake, pErr := r.generateInitialEvents(ctx, oldRoomRes, *senderID, roomID, roomVersion, tombstoneEvent)
 	if pErr != nil {
 		return "", pErr
 	}
 
 	// Send the setup events to the new room
-	if pErr = r.sendInitialEvents(ctx, evTime, senderID, userID.Domain(), newRoomID, roomVersion, eventsToMake); pErr != nil {
+	if pErr = r.sendInitialEvents(ctx, evTime, *senderID, userID.Domain(), newRoomID, roomVersion, eventsToMake); pErr != nil {
 		return "", pErr
 	}
 
@@ -111,17 +114,17 @@ func (r *Upgrader) performRoomUpgrade(
 	}
 
 	// If the old room had a canonical alias event, it should be deleted in the old room
-	if pErr = r.clearOldCanonicalAliasEvent(ctx, oldRoomRes, evTime, senderID, userID.Domain(), roomID); pErr != nil {
+	if pErr = r.clearOldCanonicalAliasEvent(ctx, oldRoomRes, evTime, *senderID, userID.Domain(), roomID); pErr != nil {
 		return "", pErr
 	}
 
 	// 4. Move local aliases to the new room
-	if pErr = moveLocalAliases(ctx, roomID, newRoomID, senderID, r.URSAPI); pErr != nil {
+	if pErr = moveLocalAliases(ctx, roomID, newRoomID, *senderID, r.URSAPI); pErr != nil {
 		return "", pErr
 	}
 
 	// 6. Restrict power levels in the old room
-	if pErr = r.restrictOldRoomPowerLevels(ctx, evTime, senderID, userID.Domain(), roomID); pErr != nil {
+	if pErr = r.restrictOldRoomPowerLevels(ctx, evTime, *senderID, userID.Domain(), roomID); pErr != nil {
 		return "", pErr
 	}
 
diff --git a/roomserver/internal/query/query.go b/roomserver/internal/query/query.go
index 11e5564d..0fe0f4f2 100644
--- a/roomserver/internal/query/query.go
+++ b/roomserver/internal/query/query.go
@@ -283,7 +283,7 @@ func (r *Queryer) QueryMembershipForUser(
 		return err
 	}
 
-	return r.QueryMembershipForSenderID(ctx, *roomID, senderID, response)
+	return r.QueryMembershipForSenderID(ctx, *roomID, *senderID, response)
 }
 
 // QueryMembershipAtEvent returns the known memberships at a given event.
@@ -1009,21 +1009,26 @@ func (r *Queryer) QueryRestrictedJoinAllowed(ctx context.Context, roomID spec.Ro
 	return verImpl.CheckRestrictedJoin(ctx, r.Cfg.Global.ServerName, &api.JoinRoomQuerier{Roomserver: r}, roomID, senderID)
 }
 
-func (r *Queryer) QuerySenderIDForUser(ctx context.Context, roomID spec.RoomID, userID spec.UserID) (spec.SenderID, error) {
+func (r *Queryer) QuerySenderIDForUser(ctx context.Context, roomID spec.RoomID, userID spec.UserID) (*spec.SenderID, error) {
 	version, err := r.DB.GetRoomVersion(ctx, roomID.String())
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 
 	switch version {
 	case gomatrixserverlib.RoomVersionPseudoIDs:
 		key, err := r.DB.SelectUserRoomPublicKey(ctx, userID, roomID)
 		if err != nil {
-			return "", err
+			return nil, err
+		} else if key == nil {
+			return nil, nil
+		} else {
+			senderID := spec.SenderID(spec.Base64Bytes(key).Encode())
+			return &senderID, nil
 		}
-		return spec.SenderID(spec.Base64Bytes(key).Encode()), nil
 	default:
-		return spec.SenderID(userID.String()), nil
+		senderID := spec.SenderID(userID.String())
+		return &senderID, nil
 	}
 }