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

6 files changed, 46 insertions, 19 deletions

diff --git a/clientapi/routing/directory.go b/clientapi/routing/directory.go
index 3ec959b4..90772766 100644
--- a/clientapi/routing/directory.go
+++ b/clientapi/routing/directory.go
@@ -204,9 +204,15 @@ func SetLocalAlias(
 			Code: http.StatusInternalServerError,
 			JSON: spec.Unknown("internal server error"),
 		}
+	} else if senderID == nil {
+		util.GetLogger(req.Context()).WithField("roomID", *roomID).WithField("userID", *userID).Error("Sender ID not found")
+		return util.JSONResponse{
+			Code: http.StatusInternalServerError,
+			JSON: spec.Unknown("internal server error"),
+		}
 	}
 
-	aliasAlreadyExists, err := rsAPI.SetRoomAlias(req.Context(), senderID, *roomID, alias)
+	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{
@@ -293,14 +299,14 @@ func RemoveLocalAlias(
 		}
 	}
 	// TODO: how to handle this case? missing user/room keys seem to be a whole new class of errors
-	if deviceSenderID == "" {
+	if deviceSenderID == nil {
 		return util.JSONResponse{
 			Code: http.StatusInternalServerError,
 			JSON: spec.Unknown("internal server error"),
 		}
 	}
 
-	aliasFound, aliasRemoved, err := rsAPI.RemoveRoomAlias(req.Context(), deviceSenderID, alias)
+	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{
@@ -385,7 +391,7 @@ func SetVisibility(
 		}
 	}
 	senderID, err := rsAPI.QuerySenderIDForUser(req.Context(), *validRoomID, *deviceUserID)
-	if err != nil {
+	if err != nil || senderID == nil {
 		return util.JSONResponse{
 			Code: http.StatusBadRequest,
 			JSON: spec.Unknown("failed to find senderID for this user"),
@@ -416,7 +422,7 @@ func SetVisibility(
 
 	// NOTSPEC: Check if the user's power is greater than power required to change m.room.canonical_alias event
 	power, _ := gomatrixserverlib.NewPowerLevelContentFromEvent(queryEventsRes.StateEvents[0].PDU)
-	if power.UserLevel(senderID) < power.EventLevel(spec.MRoomCanonicalAlias, true) {
+	if power.UserLevel(*senderID) < power.EventLevel(spec.MRoomCanonicalAlias, true) {
 		return util.JSONResponse{
 			Code: http.StatusForbidden,
 			JSON: spec.Forbidden("userID doesn't have power level to change visibility"),
diff --git a/clientapi/routing/membership.go b/clientapi/routing/membership.go
index def6f061..8b8cc47b 100644
--- a/clientapi/routing/membership.go
+++ b/clientapi/routing/membership.go
@@ -71,7 +71,7 @@ func SendBan(
 		}
 	}
 	senderID, err := rsAPI.QuerySenderIDForUser(req.Context(), *validRoomID, *deviceUserID)
-	if err != nil {
+	if err != nil || senderID == nil {
 		return util.JSONResponse{
 			Code: http.StatusForbidden,
 			JSON: spec.Forbidden("You don't have permission to ban this user, unknown senderID"),
@@ -87,7 +87,7 @@ func SendBan(
 	if errRes != nil {
 		return *errRes
 	}
-	allowedToBan := pl.UserLevel(senderID) >= pl.Ban
+	allowedToBan := pl.UserLevel(*senderID) >= pl.Ban
 	if !allowedToBan {
 		return util.JSONResponse{
 			Code: http.StatusForbidden,
@@ -169,7 +169,7 @@ func SendKick(
 		}
 	}
 	senderID, err := rsAPI.QuerySenderIDForUser(req.Context(), *validRoomID, *deviceUserID)
-	if err != nil {
+	if err != nil || senderID == nil {
 		return util.JSONResponse{
 			Code: http.StatusForbidden,
 			JSON: spec.Forbidden("You don't have permission to kick this user, unknown senderID"),
@@ -185,7 +185,7 @@ func SendKick(
 	if errRes != nil {
 		return *errRes
 	}
-	allowedToKick := pl.UserLevel(senderID) >= pl.Kick
+	allowedToKick := pl.UserLevel(*senderID) >= pl.Kick
 	if !allowedToKick {
 		return util.JSONResponse{
 			Code: http.StatusForbidden,
@@ -476,6 +476,8 @@ func buildMembershipEvent(
 	senderID, err := rsAPI.QuerySenderIDForUser(ctx, *validRoomID, *userID)
 	if err != nil {
 		return nil, err
+	} else if senderID == nil {
+		return nil, fmt.Errorf("no sender ID for %s in %s", *userID, *validRoomID)
 	}
 
 	targetID, err := spec.NewUserID(targetUserID, true)
@@ -485,6 +487,8 @@ func buildMembershipEvent(
 	targetSenderID, err := rsAPI.QuerySenderIDForUser(ctx, *validRoomID, *targetID)
 	if err != nil {
 		return nil, err
+	} else if targetSenderID == nil {
+		return nil, fmt.Errorf("no sender ID for %s in %s", *targetID, *validRoomID)
 	}
 
 	identity, err := rsAPI.SigningIdentityFor(ctx, *validRoomID, *userID)
@@ -492,8 +496,8 @@ func buildMembershipEvent(
 		return nil, err
 	}
 
-	return buildMembershipEventDirect(ctx, targetSenderID, reason, profile.DisplayName, profile.AvatarURL,
-		senderID, device.UserDomain(), membership, roomID, isDirect, identity.KeyID, identity.PrivateKey, evTime, rsAPI)
+	return buildMembershipEventDirect(ctx, *targetSenderID, reason, profile.DisplayName, profile.AvatarURL,
+		*senderID, device.UserDomain(), membership, roomID, isDirect, identity.KeyID, identity.PrivateKey, evTime, rsAPI)
 }
 
 // loadProfile lookups the profile of a given user from the database and returns
diff --git a/clientapi/routing/profile.go b/clientapi/routing/profile.go
index 35da15e0..66b58507 100644
--- a/clientapi/routing/profile.go
+++ b/clientapi/routing/profile.go
@@ -16,6 +16,7 @@ package routing
 
 import (
 	"context"
+	"fmt"
 	"net/http"
 	"time"
 
@@ -362,8 +363,10 @@ func buildMembershipEvents(
 		senderID, err := rsAPI.QuerySenderIDForUser(ctx, *validRoomID, *fullUserID)
 		if err != nil {
 			return nil, err
+		} else if senderID == nil {
+			return nil, fmt.Errorf("sender ID not found for %s in %s", *fullUserID, *validRoomID)
 		}
-		senderIDString := string(senderID)
+		senderIDString := string(*senderID)
 		proto := gomatrixserverlib.ProtoEvent{
 			SenderID: senderIDString,
 			RoomID:   roomID,
diff --git a/clientapi/routing/redaction.go b/clientapi/routing/redaction.go
index 1b9a5a81..230c96d2 100644
--- a/clientapi/routing/redaction.go
+++ b/clientapi/routing/redaction.go
@@ -74,6 +74,16 @@ func SendRedaction(
 		return *resErr
 	}
 
+	// if user is member of room, and sender ID is nil, then this user doesn't have a pseudo ID for some reason,
+	// which is unexpected.
+	if senderID == nil {
+		util.GetLogger(req.Context()).WithField("userID", *deviceUserID).WithField("roomID", roomID).Error("missing sender ID for user, despite having membership")
+		return util.JSONResponse{
+			Code: http.StatusInternalServerError,
+			JSON: spec.Unknown("internal server error"),
+		}
+	}
+
 	if txnID != nil {
 		// Try to fetch response from transactionsCache
 		if res, ok := txnCache.FetchTransaction(device.AccessToken, *txnID, req.URL); ok {
@@ -98,7 +108,7 @@ func SendRedaction(
 	// "Users may redact their own events, and any user with a power level greater than or equal
 	// to the redact power level of the room may redact events there"
 	// https://matrix.org/docs/spec/client_server/r0.6.1#put-matrix-client-r0-rooms-roomid-redact-eventid-txnid
-	allowedToRedact := ev.SenderID() == senderID
+	allowedToRedact := ev.SenderID() == *senderID
 	if !allowedToRedact {
 		plEvent := roomserverAPI.GetStateEvent(req.Context(), rsAPI, roomID, gomatrixserverlib.StateKeyTuple{
 			EventType: spec.MRoomPowerLevels,
@@ -119,7 +129,7 @@ func SendRedaction(
 				),
 			}
 		}
-		allowedToRedact = pl.UserLevel(senderID) >= pl.Redact
+		allowedToRedact = pl.UserLevel(*senderID) >= pl.Redact
 	}
 	if !allowedToRedact {
 		return util.JSONResponse{
@@ -136,7 +146,7 @@ func SendRedaction(
 
 	// create the new event and set all the fields we can
 	proto := gomatrixserverlib.ProtoEvent{
-		SenderID: string(senderID),
+		SenderID: string(*senderID),
 		RoomID:   roomID,
 		Type:     spec.MRoomRedaction,
 		Redacts:  eventID,
diff --git a/clientapi/routing/sendevent.go b/clientapi/routing/sendevent.go
index 41a3793a..17200171 100644
--- a/clientapi/routing/sendevent.go
+++ b/clientapi/routing/sendevent.go
@@ -251,8 +251,10 @@ func updatePowerLevels(req *http.Request, r map[string]interface{}, roomID strin
 		senderID, err := rsAPI.QuerySenderIDForUser(req.Context(), *validRoomID, *uID)
 		if err != nil {
 			return err
+		} else if senderID == nil {
+			return fmt.Errorf("sender ID not found for %s in %s", uID, *validRoomID)
 		}
-		userMap[string(senderID)] = level
+		userMap[string(*senderID)] = level
 		delete(userMap, user)
 	}
 	r["users"] = userMap
@@ -314,7 +316,7 @@ func generateSendEvent(
 		}
 	}
 	senderID, err := rsAPI.QuerySenderIDForUser(ctx, *validRoomID, *fullUserID)
-	if err != nil {
+	if err != nil || senderID == nil {
 		return nil, &util.JSONResponse{
 			Code: http.StatusNotFound,
 			JSON: spec.NotFound("Unable to find senderID for user"),
@@ -323,7 +325,7 @@ func generateSendEvent(
 
 	// create the new event and set all the fields we can
 	proto := gomatrixserverlib.ProtoEvent{
-		SenderID: string(senderID),
+		SenderID: string(*senderID),
 		RoomID:   roomID,
 		Type:     eventType,
 		StateKey: stateKey,
diff --git a/clientapi/threepid/invites.go b/clientapi/threepid/invites.go
index d15cc6d4..365e9f86 100644
--- a/clientapi/threepid/invites.go
+++ b/clientapi/threepid/invites.go
@@ -366,9 +366,11 @@ func emit3PIDInviteEvent(
 	sender, err := rsAPI.QuerySenderIDForUser(ctx, *validRoomID, *userID)
 	if err != nil {
 		return err
+	} else if sender == nil {
+		return fmt.Errorf("sender ID not found for %s in %s", *userID, *validRoomID)
 	}
 	proto := &gomatrixserverlib.ProtoEvent{
-		SenderID: string(sender),
+		SenderID: string(*sender),
 		RoomID:   roomID,
 		Type:     "m.room.third_party_invite",
 		StateKey: &res.Token,